TEP-0075: Validate object name and key name have no dots

Prior to this commit, dots are allowed to be used in param names to support domain-scoped names. However, object params have already supported domain-scoped names since it has a list of keys. In addition, using dots in object param names and key names have conflicts. See more details in tektoncd/community#711. In this change, we validate against those object names and key names that contain dots.
tektoncd · Jul 8, 2022 · 9a9c70a · 9a9c70a
1 parent dc9ea6c
commit 9a9c70a
Show file tree

Hide file tree

Showing 3 changed files with 115 additions and 18 deletions.
diff --git a/docs/tasks.md b/docs/tasks.md
@@ -443,20 +443,35 @@ steps:
 You can specify parameters, such as compilation flags or artifact names, that you want to supply to the `Task` at execution time.
  `Parameters` are passed to the `Task` from its corresponding `TaskRun`.
 
-Parameter names:
-
-- Must only contain alphanumeric characters, hyphens (`-`), underscores (`_`), and dots (`.`).
+#### Parameter name
+Parameter name format:
+- Must only contain alphanumeric characters, hyphens (`-`), underscores (`_`), and dots (`.`). However, `object` parameter name and its key names can't contain dots (`.`). See the reasons in the third item added in this [PR](https://github.com/tektoncd/community/pull/711).
 - Must begin with a letter or an underscore (`_`).
 
-For example, `foo.Is-Bar_` is a valid parameter name, but `barIsBa$` or `0banana` are not.
+For example, `foo.Is-Bar_` is a valid parameter name for string or array type, but is invalid for object parameter because it contains dots. On the other hand, `barIsBa$` or `0banana` are invalid for all types.
 
 > NOTE:
 > 1. Parameter names are **case insensitive**. For example, `APPLE` and `apple` will be treated as equal. If they appear in the same TaskSpec's params, it will be rejected as invalid.
 > 2. If a parameter name contains dots (.), it must be referenced by using the [bracket notation](#substituting-parameters-and-resources) with either single or double quotes i.e. `$(params['foo.bar'])`, `$(params["foo.bar"])`. See the following example for more information.
 
-Each declared parameter has a `type` field, which can be set to either `array` or `string`. `array` is useful in cases where the number
-of compilation flags being supplied to a task varies throughout the `Task's` execution. If not specified, the `type` field defaults to
-`string`. When the actual parameter value is supplied, its parsed type is validated against the `type` field.
+#### Parameter type
+Each declared parameter has a `type` field, which can be set to `string`, `array` or `object` (alpha feature). 
+
+- `object` type
+`object` type is useful in cases where users want to group related parameters. For example, an object parameter called `gitrepo` can contain both the `url` and the `commmit` to group related information.
+
+  > NOTE: 
+  > - `object` param is an `alpha` feature and gated by the `alpha` feature flag.
+  > - `object` param must specify the `properties` section to define the schema i.e. what keys are available for this object param. See how to define `properties` section in the following example and the [TEP-0075](https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md#defaulting-to-string-types-for-values).
+  > - When using object in variable replacement, users can only access its individual key ("child" member) of the object by its name i.e. `$(params.gitrepo.url)`. Using an entire object as a value is only allowed when the value is also an object. See more details about using object param from the [TEP-0075](https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md#using-objects-in-variable-replacement).
+
+
+
+- `array` type 
+`array` type is useful in cases where the number of compilation flags being supplied to a task varies throughout the `Task's` execution. 
+
+- `string` type
+If not specified, the `type` field defaults to `string`. When the actual parameter value is supplied, its parsed type is validated against the `type` field.
 
 The following example illustrates the use of `Parameters` in a `Task`. The `Task` declares two input parameters named `flags`
 (of type `array`) and `someURL` (of type `string`), and uses them in the `steps.args` list. You can expand parameters of type `array`
@@ -471,6 +486,13 @@ metadata:
   name: task-with-parameters
 spec:
   params:
+    - name: gitrepo
+      type: object
+      properties:
+        url:
+          type: string
+        commit:
+          type: string
     - name: flags
       type: array
     - name: someURL
@@ -482,6 +504,12 @@ spec:
     - name: echo-output
       description: "successful echo"
   steps:
+    - name: do-the-clone
+      image: some-git-image
+      args: [
+        "-url=$(params.gitrepo.url)",
+        "-revision=$(params.gitrepo.commit)"
+      ]
     - name: build
       image: my-builder
       args: [
@@ -499,7 +527,7 @@ spec:
         echo $(params["foo.bar"]) | tee $(results.echo-output.path)
 ```
 
-The following `TaskRun` supplies a dynamic number of strings within the `flags` parameter:
+The following `TaskRun` supplies the value for the parameter `gitrepo`, `flags` and `someURL`:
 
 ```yaml
 apiVersion: tekton.dev/v1beta1
@@ -510,6 +538,10 @@ spec:
   taskRef:
     name: task-with-parameters
   params:
+    - name: gitrepo
+      value:
+        url: "abc.com"
+        commit: "c12b72"
     - name: flags
       value:
         - "--set"
@@ -520,6 +552,7 @@ spec:
       value: "http://google.com"
 ```
 
+#### Default value
 Parameter declarations (within Tasks and Pipelines) can include default values which will be used if the parameter is
 not specified, for example to specify defaults for both string params and array params
 ([full example](../examples/v1beta1/taskruns/array-default.yaml)) :

diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go
@@ -39,6 +39,8 @@ var _ apis.Validatable = (*Task)(nil)
 
 const variableNameFormat = "^[_a-zA-Z][_a-zA-Z0-9.-]*$"
 
+var variableNameFormatRegex = regexp.MustCompile(variableNameFormat)
+
 // Validate implements apis.Validatable
 func (t *Task) Validate(ctx context.Context) *apis.FieldError {
 	errs := validate.ObjectMetadata(t.GetObjectMeta()).ViaField("metadata")
@@ -337,6 +339,7 @@ func ValidateParameterVariables(ctx context.Context, steps []Step, params []Para
 		}
 	}
 
+	errs = errs.Also(validateNameFormat(parameterNames, objectParamSpecs))
 	errs = errs.Also(validateVariables(ctx, steps, "params", parameterNames))
 	errs = errs.Also(validateArrayUsage(steps, "params", arrayParameterNames))
 	errs = errs.Also(validateObjectDefault(objectParamSpecs))
@@ -498,31 +501,57 @@ func validateStepArrayUsage(step Step, prefix string, vars sets.String) *apis.Fi
 }
 
 func validateVariables(ctx context.Context, steps []Step, prefix string, vars sets.String) (errs *apis.FieldError) {
-	// validate that the variable name format follows the rules
-	// - Must only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)
-	// - Must begin with a letter or an underscore (_)
-	re := regexp.MustCompile(variableNameFormat)
+	// We've checked param name format. Now, we want to check if param names are referenced correctly in each step
+	for idx, step := range steps {
+		errs = errs.Also(validateStepVariables(ctx, step, prefix, vars).ViaFieldIndex("steps", idx))
+	}
+	return errs
+}
+
+// validateNameFormat validates that the variable name format follows the rules
+// - Must only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)
+// - Must begin with a letter or an underscore (_)
+// - Exception: Object param name and key names cannot contain dots (.)
+func validateNameFormat(allParamNames sets.String, objectParams []ParamSpec) (errs *apis.FieldError) {
 	invalidNames := []string{}
 	// Converting to sorted list here rather than just looping map keys
 	// because we want the order of items in vars to be deterministic for purpose of unit testing
-	for _, name := range vars.List() {
-		if !re.MatchString(name) {
+	for _, name := range allParamNames.List() {
+		if !variableNameFormatRegex.MatchString(name) {
 			invalidNames = append(invalidNames, name)
 		}
 	}
 
 	if len(invalidNames) != 0 {
-		return &apis.FieldError{
+		errs = errs.Also(&apis.FieldError{
 			Message: fmt.Sprintf("The format of following variable names is invalid. %s", invalidNames),
 			Paths:   []string{"params"},
 			Details: "Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)\nMust begin with a letter or an underscore (_)",
+		})
+	}
+
+	invalidObjectVars := map[string][]string{}
+	for _, obj := range objectParams {
+		// check object param name
+		if strings.Contains(obj.Name, ".") {
+			invalidObjectVars[obj.Name] = []string{}
+		}
+
+		// check key names
+		for k := range obj.Properties {
+			if strings.Contains(k, ".") {
+				invalidObjectVars[obj.Name] = append(invalidObjectVars[obj.Name], k)
+			}
 		}
 	}
 
-	// We've checked param name format. Now, we want to check if param names are referenced correctly in each step
-	for idx, step := range steps {
-		errs = errs.Also(validateStepVariables(ctx, step, prefix, vars).ViaFieldIndex("steps", idx))
+	if len(invalidObjectVars) != 0 {
+		errs = errs.Also(&apis.FieldError{
+			Message: fmt.Sprintf("Object param name or key name shouldn't contain dots: %s", invalidObjectVars),
+			Paths:   []string{"params"},
+		})
 	}
+
 	return errs
 }
 

diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go
@@ -573,6 +573,41 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Paths:   []string{"params"},
 			Details: "Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)\nMust begin with a letter or an underscore (_)",
 		},
+	}, {
+		name: "invalid object param format - object param name and key name shouldn't contain dots.",
+		fields: fields{
+			Params: []v1beta1.ParamSpec{{
+				Name:        "invalid.name1",
+				Description: "object param name contains dots",
+				Properties: map[string]v1beta1.PropertySpec{
+					"mykey1": {},
+					"mykey2": {},
+				},
+			}, {
+				Name:        "valid.name",
+				Description: "object param key name contains dots",
+				Properties: map[string]v1beta1.PropertySpec{
+					"invalid.key.name": {},
+					"validKeyName":     {},
+				},
+			}, {
+				Name:        "invalid.name2",
+				Description: "both object param name and key names contain dots",
+				Properties: map[string]v1beta1.PropertySpec{
+					"invalid.key.name1": {},
+					"invalid.key.name2": {},
+				},
+			}},
+			Steps: validSteps,
+		},
+		expectedError: apis.FieldError{
+			Message: fmt.Sprintf("Object param name or key name shouldn't contain dots: %v", map[string][]string{
+				"invalid.name1": {},
+				"valid.name":    {"invalid.key.name"},
+				"invalid.name2": {"invalid.key.name1", "invalid.key.name2"},
+			}),
+			Paths: []string{"params"},
+		},
 	}, {
 		name: "duplicated param names",
 		fields: fields{