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.
chuangw6 · Jul 11, 2022 · ba212d3 · ba212d3
1 parent da43d0e
commit ba212d3
Show file tree

Hide file tree

Showing 3 changed files with 131 additions and 34 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
@@ -35,9 +35,20 @@ import (
 	"knative.dev/pkg/apis"
 )
 
-var _ apis.Validatable = (*Task)(nil)
+const (
+	// arrayAndStringVariableNameFormat is the regex to validate if string/array variable name format follows the following rules.
+	// - Must only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)
+	// - Must begin with a letter or an underscore (_)
+	arrayAndStringVariableNameFormat = "^[_a-zA-Z][_a-zA-Z0-9.-]*$"
 
-const variableNameFormat = "^[_a-zA-Z][_a-zA-Z0-9.-]*$"
+	// objectVariableNameFormat is the regext used to validate object name and key names format
+	// The difference with the array or string name format is that object variable names shouldn't contain dots.
+	objectVariableNameFormat = "^[_a-zA-Z][_a-zA-Z0-9-]*$"
+)
+
+var _ apis.Validatable = (*Task)(nil)
+var arrayOrStringVariableNameFormatRegex = regexp.MustCompile(arrayAndStringVariableNameFormat)
+var objectVariableNameFormatRegex = regexp.MustCompile(objectVariableNameFormat)
 
 // Validate implements apis.Validatable
 func (t *Task) Validate(ctx context.Context) *apis.FieldError {
@@ -319,25 +330,30 @@ func (p ParamSpec) ValidateObjectType() *apis.FieldError {
 
 // ValidateParameterVariables validates all variables within a slice of ParamSpecs against a slice of Steps
 func ValidateParameterVariables(ctx context.Context, steps []Step, params []ParamSpec) *apis.FieldError {
-	parameterNames := sets.NewString()
+	allParameterNames := sets.NewString()
+	stringParameterNames := sets.NewString()
 	arrayParameterNames := sets.NewString()
 	objectParamSpecs := []ParamSpec{}
 	var errs *apis.FieldError
 	for _, p := range params {
 		// validate no duplicate names
-		if parameterNames.Has(p.Name) {
+		if allParameterNames.Has(p.Name) {
 			errs = errs.Also(apis.ErrGeneric("parameter appears more than once", "").ViaFieldKey("params", p.Name))
 		}
-		parameterNames.Insert(p.Name)
-		if p.Type == ParamTypeArray {
+		allParameterNames.Insert(p.Name)
+
+		switch p.Type {
+		case ParamTypeArray:
 			arrayParameterNames.Insert(p.Name)
-		}
-		if p.Type == ParamTypeObject {
+		case ParamTypeObject:
 			objectParamSpecs = append(objectParamSpecs, p)
+		default:
+			stringParameterNames.Insert(p.Name)
 		}
 	}
 
-	errs = errs.Also(validateVariables(ctx, steps, "params", parameterNames))
+	errs = errs.Also(validateNameFormat(stringParameterNames.Insert(arrayParameterNames.List()...), objectParamSpecs))
+	errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames))
 	errs = errs.Also(validateArrayUsage(steps, "params", arrayParameterNames))
 	errs = errs.Also(validateObjectDefault(objectParamSpecs))
 	return errs.Also(validateObjectUsage(ctx, steps, objectParamSpecs))
@@ -498,31 +514,59 @@ 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)
-	invalidNames := []string{}
+	// 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 name format of all param types follows the rules
+func validateNameFormat(stringAndArrayParams sets.String, objectParams []ParamSpec) (errs *apis.FieldError) {
+	// checking string or array name format
+	// ----
+	invalidStringAndArrayNames := []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) {
-			invalidNames = append(invalidNames, name)
+	for _, name := range stringAndArrayParams.List() {
+		if !arrayOrStringVariableNameFormatRegex.MatchString(name) {
+			invalidStringAndArrayNames = append(invalidStringAndArrayNames, name)
 		}
 	}
 
-	if len(invalidNames) != 0 {
-		return &apis.FieldError{
-			Message: fmt.Sprintf("The format of following variable names is invalid. %s", invalidNames),
+	if len(invalidStringAndArrayNames) != 0 {
+		errs = errs.Also(&apis.FieldError{
+			Message: fmt.Sprintf("The format of following array and string variable names is invalid: %s", invalidStringAndArrayNames),
 			Paths:   []string{"params"},
-			Details: "Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)\nMust begin with a letter or an underscore (_)",
+			Details: "String/Array Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)\nMust begin with a letter or an underscore (_)",
+		})
+	}
+
+	// checking object name and key name format
+	// -----
+	invalidObjectNames := map[string][]string{}
+	for _, obj := range objectParams {
+		// check object param name
+		if !objectVariableNameFormatRegex.MatchString(obj.Name) {
+			invalidObjectNames[obj.Name] = []string{}
+		}
+
+		// check key names
+		for k := range obj.Properties {
+			if !objectVariableNameFormatRegex.MatchString(k) {
+				invalidObjectNames[obj.Name] = append(invalidObjectNames[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(invalidObjectNames) != 0 {
+		errs = errs.Also(&apis.FieldError{
+			Message: fmt.Sprintf("Object param name and key name format is invalid: %s", invalidObjectNames),
+			Paths:   []string{"params"},
+			Details: "Object Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_) \nMust begin with a letter or an underscore (_)",
+		})
 	}
+
 	return errs
 }
 

diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go
@@ -569,9 +569,29 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Steps: validSteps,
 		},
 		expectedError: apis.FieldError{
-			Message: fmt.Sprintf("The format of following variable names is invalid. %s", []string{"", "0ab", "a^b", "f oo"}),
+			Message: fmt.Sprintf("The format of following array and string variable names is invalid: %s", []string{"", "0ab", "a^b", "f oo"}),
 			Paths:   []string{"params"},
-			Details: "Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)\nMust begin with a letter or an underscore (_)",
+			Details: "String/Array 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{
+					"invalid.key1": {},
+					"mykey2":       {},
+				},
+			}},
+			Steps: validSteps,
+		},
+		expectedError: apis.FieldError{
+			Message: fmt.Sprintf("Object param name and key name format is invalid: %v", map[string][]string{
+				"invalid.name1": {"invalid.key1"},
+			}),
+			Paths:   []string{"params"},
+			Details: "Object Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_) \nMust begin with a letter or an underscore (_)",
 		},
 	}, {
 		name: "duplicated param names",