Skip validation when propagating parameters

Prior to this, propagating parameters only skipped webhook validation for params defined in script. As a result, when users tried to propagate params to other fields like `args` or `command`, etc. an webhook validation was raised. This PR address this issue.
tektoncd · Jul 14, 2022 · d987b78 · d987b78
1 parent 6542823
commit d987b78
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 15 deletions.
diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go
@@ -571,31 +571,32 @@ func validateNameFormat(stringAndArrayParams sets.String, objectParams []ParamSp
 }
 
 func validateStepVariables(ctx context.Context, step Step, prefix string, vars sets.String) *apis.FieldError {
-	errs := validateTaskVariable(step.Name, prefix, vars).ViaField("name")
-	errs = errs.Also(validateTaskVariable(step.Image, prefix, vars).ViaField("image"))
-	errs = errs.Also(validateTaskVariable(step.WorkingDir, prefix, vars).ViaField("workingDir"))
-	if !(config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" && prefix == "params") {
-		errs = errs.Also(validateTaskVariable(step.Script, prefix, vars).ViaField("script"))
-	}
+	errs := validateTaskVariable(ctx, step.Name, prefix, vars).ViaField("name")
+	errs = errs.Also(validateTaskVariable(ctx, step.Image, prefix, vars).ViaField("image"))
+	errs = errs.Also(validateTaskVariable(ctx, step.WorkingDir, prefix, vars).ViaField("workingDir"))
+	errs = errs.Also(validateTaskVariable(ctx, step.Script, prefix, vars).ViaField("script"))
 	for i, cmd := range step.Command {
-		errs = errs.Also(validateTaskVariable(cmd, prefix, vars).ViaFieldIndex("command", i))
+		errs = errs.Also(validateTaskVariable(ctx, cmd, prefix, vars).ViaFieldIndex("command", i))
 	}
 	for i, arg := range step.Args {
-		errs = errs.Also(validateTaskVariable(arg, prefix, vars).ViaFieldIndex("args", i))
+		errs = errs.Also(validateTaskVariable(ctx, arg, prefix, vars).ViaFieldIndex("args", i))
 	}
 	for _, env := range step.Env {
-		errs = errs.Also(validateTaskVariable(env.Value, prefix, vars).ViaFieldKey("env", env.Name))
+		errs = errs.Also(validateTaskVariable(ctx, env.Value, prefix, vars).ViaFieldKey("env", env.Name))
 	}
 	for i, v := range step.VolumeMounts {
-		errs = errs.Also(validateTaskVariable(v.Name, prefix, vars).ViaField("name").ViaFieldIndex("volumeMount", i))
-		errs = errs.Also(validateTaskVariable(v.MountPath, prefix, vars).ViaField("MountPath").ViaFieldIndex("volumeMount", i))
-		errs = errs.Also(validateTaskVariable(v.SubPath, prefix, vars).ViaField("SubPath").ViaFieldIndex("volumeMount", i))
+		errs = errs.Also(validateTaskVariable(ctx, v.Name, prefix, vars).ViaField("name").ViaFieldIndex("volumeMount", i))
+		errs = errs.Also(validateTaskVariable(ctx, v.MountPath, prefix, vars).ViaField("MountPath").ViaFieldIndex("volumeMount", i))
+		errs = errs.Also(validateTaskVariable(ctx, v.SubPath, prefix, vars).ViaField("SubPath").ViaFieldIndex("volumeMount", i))
 	}
 	return errs
 }
 
-func validateTaskVariable(value, prefix string, vars sets.String) *apis.FieldError {
-	return substitution.ValidateVariableP(value, prefix, vars)
+func validateTaskVariable(ctx context.Context, value, prefix string, vars sets.String) *apis.FieldError {
+	if !(config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" && prefix == "params") {
+		return substitution.ValidateVariableP(value, prefix, vars)
+	}
+	return nil
 }
 
 func validateTaskNoObjectReferenced(value, prefix string, objectNames sets.String) *apis.FieldError {

diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go
@@ -250,6 +250,26 @@ func TestTaskSpecValidate(t *testing.T) {
 				Image: "some-image",
 			},
 		},
+	}, {
+		name: "propagating params valid step with script",
+		fields: fields{
+			Steps: []v1beta1.Step{{
+				Image: "my-image",
+				Script: `
+				#!/usr/bin/env bash
+				$(params.message)`,
+			}},
+		},
+	}, {
+		name: "propagating params valid step with args",
+		fields: fields{
+			Steps: []v1beta1.Step{{
+				Name:    "mystep",
+				Image:   "my-image",
+				Command: []string{"$(params.command)"},
+				Args:    []string{"$(params.message)"},
+			}},
+		},
 	}, {
 		name: "valid step with script",
 		fields: fields{
@@ -450,6 +470,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 		name          string
 		fields        fields
 		expectedError apis.FieldError
+		api           string
 	}{{
 		name: "empty spec",
 		expectedError: apis.FieldError{
@@ -593,6 +614,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Paths:   []string{"params"},
 			Details: "Object Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_) \nMust begin with a letter or an underscore (_)",
 		},
+		api: "alpha",
 	}, {
 		name: "duplicated param names",
 		fields: fields{
@@ -697,6 +719,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: `"object" type does not match default value's type: "string"`,
 			Paths:   []string{"params.task.type", "params.task.default.type"},
 		},
+		api: "alpha",
 	}, {
 		name: "the spec of object type parameter misses the definition of properties",
 		fields: fields{
@@ -708,6 +731,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Steps: validSteps,
 		},
 		expectedError: *apis.ErrMissingField(fmt.Sprintf("params.task.properties")),
+		api:           "alpha",
 	}, {
 		name: "PropertySpec type is set with unsupported type",
 		fields: fields{
@@ -726,6 +750,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: fmt.Sprintf("The value type specified for these keys %v is invalid", []string{"key1"}),
 			Paths:   []string{"params.task.properties"},
 		},
+		api: "alpha",
 	}, {
 		name: "keys defined in properties are missed in default",
 		fields: fields{
@@ -747,6 +772,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: fmt.Sprintf("Required key(s) %s are missing in the value provider.", []string{"key2"}),
 			Paths:   []string{"myobjectParam.properties", "myobjectParam.default"},
 		},
+		api: "alpha",
 	}, {
 		name: "invalid step",
 		fields: fields{
@@ -961,6 +987,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: `variable type invalid in "$(params.gitrepo)"`,
 			Paths:   []string{"steps[0].image"},
 		},
+		api: "alpha",
 	}, {
 		name: "object star used in a string field",
 		fields: fields{
@@ -983,6 +1010,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: `variable type invalid in "$(params.gitrepo[*])"`,
 			Paths:   []string{"steps[0].image"},
 		},
+		api: "alpha",
 	}, {
 		name: "object used in a field that can accept array type",
 		fields: fields{
@@ -1005,6 +1033,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: `variable type invalid in "$(params.gitrepo)"`,
 			Paths:   []string{"steps[0].args[0]"},
 		},
+		api: "alpha",
 	}, {
 		name: "object star used in a field that can accept array type",
 		fields: fields{
@@ -1027,6 +1056,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: `variable type invalid in "$(params.gitrepo[*])"`,
 			Paths:   []string{"steps[0].args[0]"},
 		},
+		api: "alpha",
 	}, {
 		name: "Inexistent param variable in volumeMount with existing",
 		fields: fields{
@@ -1299,7 +1329,10 @@ func TestTaskSpecValidateError(t *testing.T) {
 				Workspaces:   tt.fields.Workspaces,
 				Results:      tt.fields.Results,
 			}
-			ctx := config.EnableAlphaAPIFields(context.Background())
+			ctx := context.Background()
+			if tt.api == "alpha" {
+				ctx = config.EnableAlphaAPIFields(context.Background())
+			}
 			ts.SetDefaults(ctx)
 			err := ts.Validate(ctx)
 			if err == nil {