diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index 771aaeed74c..0d26457eadd 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -374,11 +374,7 @@ func ValidateResourcesVariables(ctx context.Context, steps []Step, resources *Ta return validateVariables(ctx, steps, "resources.(?:inputs|outputs)", resourceNames) } -// TODO (@chuangw6): Make sure an object param is not used as a whole when providing values for strings. -// https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md#variable-replacement-with-object-params -// "When providing values for strings, Task and Pipeline authors can access -// individual attributes of an object param; they cannot access the object -// as whole (we could add support for this later)." +// validateObjectUsage validates the usage of individual attributes of an object param and the usage of the entire object func validateObjectUsage(ctx context.Context, steps []Step, params []ParamSpec) (errs *apis.FieldError) { objectParameterNames := sets.NewString() for _, p := range params { @@ -399,7 +395,7 @@ func validateObjectUsage(ctx context.Context, steps []Step, params []ParamSpec) errs = errs.Also(validateVariables(ctx, steps, fmt.Sprintf("params\\.%s", p.Name), objectKeys)) } - return errs + return errs.Also(validateObjectUsageAsWhole(steps, "params", objectParameterNames)) } // validate if object keys defined in properties are all provided in default @@ -420,6 +416,38 @@ func validateObjectKeysInDefault(defaultObject map[string]string, neededObjectKe return nil } +// validateObjectUsageAsWhole makes sure the object params are not used as whole when providing values for strings +// i.e. param.objectParam, param.objectParam[*] +func validateObjectUsageAsWhole(steps []Step, prefix string, vars sets.String) (errs *apis.FieldError) { + for idx, step := range steps { + errs = errs.Also(validateStepObjectUsageAsWhole(step, prefix, vars)).ViaFieldIndex("steps", idx) + } + return errs +} + +func validateStepObjectUsageAsWhole(step Step, prefix string, vars sets.String) *apis.FieldError { + errs := validateTaskNoObjectReferenced(step.Name, prefix, vars).ViaField("name") + errs = errs.Also(validateTaskNoObjectReferenced(step.Image, prefix, vars).ViaField("image")) + errs = errs.Also(validateTaskNoObjectReferenced(step.WorkingDir, prefix, vars).ViaField("workingDir")) + errs = errs.Also(validateTaskNoObjectReferenced(step.Script, prefix, vars).ViaField("script")) + for i, cmd := range step.Command { + errs = errs.Also(validateTaskNoObjectReferenced(cmd, prefix, vars).ViaFieldIndex("command", i)) + } + for i, arg := range step.Args { + errs = errs.Also(validateTaskNoObjectReferenced(arg, prefix, vars).ViaFieldIndex("args", i)) + + } + for _, env := range step.Env { + errs = errs.Also(validateTaskNoObjectReferenced(env.Value, prefix, vars).ViaFieldKey("env", env.Name)) + } + for i, v := range step.VolumeMounts { + errs = errs.Also(validateTaskNoObjectReferenced(v.Name, prefix, vars).ViaField("name").ViaFieldIndex("volumeMount", i)) + errs = errs.Also(validateTaskNoObjectReferenced(v.MountPath, prefix, vars).ViaField("mountPath").ViaFieldIndex("volumeMount", i)) + errs = errs.Also(validateTaskNoObjectReferenced(v.SubPath, prefix, vars).ViaField("subPath").ViaFieldIndex("volumeMount", i)) + } + return errs +} + func validateArrayUsage(steps []Step, prefix string, vars sets.String) (errs *apis.FieldError) { for idx, step := range steps { errs = errs.Also(validateStepArrayUsage(step, prefix, vars)).ViaFieldIndex("steps", idx) @@ -507,6 +535,10 @@ func validateTaskVariable(value, prefix string, vars sets.String) *apis.FieldErr return substitution.ValidateVariableP(value, prefix, vars) } +func validateTaskNoObjectReferenced(value, prefix string, objectNames sets.String) *apis.FieldError { + return substitution.ValidateEntireVariableProhibitedP(value, prefix, objectNames) +} + func validateTaskNoArrayReferenced(value, prefix string, arrayNames sets.String) *apis.FieldError { return substitution.ValidateVariableProhibitedP(value, prefix, arrayNames) } diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index c69e2604756..509a4922e84 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -193,6 +193,24 @@ func TestTaskSpecValidate(t *testing.T) { WorkingDir: "/foo/bar/src/", }}, }, + }, { + name: "valid object template variable", + fields: fields{ + Params: []v1beta1.ParamSpec{{ + Name: "gitrepo", + Type: v1beta1.ParamTypeObject, + Properties: map[string]v1beta1.PropertySpec{ + "url": {}, + "commit": {}, + }, + }}, + Steps: []v1beta1.Step{{ + Name: "do-the-clone", + Image: "some-git-image", + Args: []string{"-url=$(params.gitrepo.url)", "-commit=$(params.gitrepo.commit)"}, + WorkingDir: "/foo/bar/src/", + }}, + }, }, { name: "valid star array template variable", fields: fields{ @@ -901,6 +919,94 @@ func TestTaskSpecValidateError(t *testing.T) { Message: `variable is not properly isolated in "not isolated: $(params.baz[*])"`, Paths: []string{"steps[0].args[0]"}, }, + }, { + name: "object used in a string field", + fields: fields{ + Params: []v1beta1.ParamSpec{{ + Name: "gitrepo", + Type: v1beta1.ParamTypeObject, + Properties: map[string]v1beta1.PropertySpec{ + "url": {}, + "commit": {}, + }, + }}, + Steps: []v1beta1.Step{{ + Name: "do-the-clone", + Image: "$(params.gitrepo)", + Args: []string{"echo"}, + WorkingDir: "/foo/bar/src/", + }}, + }, + expectedError: apis.FieldError{ + Message: `variable type invalid in "$(params.gitrepo)"`, + Paths: []string{"steps[0].image"}, + }, + }, { + name: "object star used in a string field", + fields: fields{ + Params: []v1beta1.ParamSpec{{ + Name: "gitrepo", + Type: v1beta1.ParamTypeObject, + Properties: map[string]v1beta1.PropertySpec{ + "url": {}, + "commit": {}, + }, + }}, + Steps: []v1beta1.Step{{ + Name: "do-the-clone", + Image: "$(params.gitrepo[*])", + Args: []string{"echo"}, + WorkingDir: "/foo/bar/src/", + }}, + }, + expectedError: apis.FieldError{ + Message: `variable type invalid in "$(params.gitrepo[*])"`, + Paths: []string{"steps[0].image"}, + }, + }, { + name: "object used in a field that can accept array type", + fields: fields{ + Params: []v1beta1.ParamSpec{{ + Name: "gitrepo", + Type: v1beta1.ParamTypeObject, + Properties: map[string]v1beta1.PropertySpec{ + "url": {}, + "commit": {}, + }, + }}, + Steps: []v1beta1.Step{{ + Name: "do-the-clone", + Image: "myimage", + Args: []string{"$(params.gitrepo)"}, + WorkingDir: "/foo/bar/src/", + }}, + }, + expectedError: apis.FieldError{ + Message: `variable type invalid in "$(params.gitrepo)"`, + Paths: []string{"steps[0].args[0]"}, + }, + }, { + name: "object star used in a field that can accept array type", + fields: fields{ + Params: []v1beta1.ParamSpec{{ + Name: "gitrepo", + Type: v1beta1.ParamTypeObject, + Properties: map[string]v1beta1.PropertySpec{ + "url": {}, + "commit": {}, + }, + }}, + Steps: []v1beta1.Step{{ + Name: "do-the-clone", + Image: "some-git-image", + Args: []string{"$(params.gitrepo[*])"}, + WorkingDir: "/foo/bar/src/", + }}, + }, + expectedError: apis.FieldError{ + Message: `variable type invalid in "$(params.gitrepo[*])"`, + Paths: []string{"steps[0].args[0]"}, + }, }, { name: "Inexistent param variable in volumeMount with existing", fields: fields{ diff --git a/pkg/substitution/substitution.go b/pkg/substitution/substitution.go index e0d07c946f9..fe0abc6a106 100644 --- a/pkg/substitution/substitution.go +++ b/pkg/substitution/substitution.go @@ -109,6 +109,31 @@ func ValidateVariableProhibitedP(value, prefix string, vars sets.String) *apis.F return nil } +// ValidateEntireVariableProhibitedP verifies that values of object type are not used as whole. +func ValidateEntireVariableProhibitedP(value, prefix string, vars sets.String) *apis.FieldError { + vs, err := extractEntireVariablesFromString(value, prefix) + if err != nil { + return &apis.FieldError{ + Message: fmt.Sprintf("Fail to extract entire variable from string: %v", err), + // Empty path is required to make the `ViaField`, … work + Paths: []string{""}, + } + } + + for _, v := range vs { + v = strings.TrimSuffix(v, "[*]") + if vars.Has(v) { + return &apis.FieldError{ + Message: fmt.Sprintf("variable type invalid in %q", value), + // Empty path is required to make the `ViaField`, … work + Paths: []string{""}, + } + } + } + + return nil +} + // ValidateVariableIsolated verifies that variables matching the relevant string expressions are completely isolated if present. func ValidateVariableIsolated(name, value, prefix, locationName, path string, vars sets.String) *apis.FieldError { if vs, present, _ := extractVariablesFromString(value, prefix); present { @@ -180,38 +205,57 @@ func extractVariablesFromString(s, prefix string) ([]string, bool, string) { groups := matchGroups(match, re) for j, v := range []string{"var1", "var2", "var3"} { val := groups[v] - // if using the dot notation + // If using the dot notation, the number of dot-separated components is restricted up to 2. + // Valid Examples: + // - extract "aString" from .aString + // - extract "anObject" from .anObject.key + // Invalid Examples: + // - .foo.bar.baz.... if j == 0 && strings.Contains(val, ".") { - switch prefix { - case "params": - // params can only have a maximum of two components in the dot notation otherwise it needs to use the bracket notation. - if len(strings.Split(val, ".")) > 0 { - errString = fmt.Sprintf(`Invalid referencing of parameters in %s !!! You can only use the dots inside single or double quotes. eg. $(params["org.foo.blah"]) or $(params['org.foo.blah']) are valid references but NOT $params.org.foo.blah.`, s) - return vars, true, errString - } - case "resources.(?:inputs|outputs)": - // resources can only have a maximum of 4 components. - if len(strings.Split(val, ".")) > 2 { - errString = fmt.Sprintf(`Invalid referencing of parameters in %s !!! resources.* can only have 4 components (eg. resources.inputs.foo.bar). Found more than 4 components.`, s) - return vars, true, errString - } - vars[i] = strings.SplitN(val, ".", 2)[0] - default: - // for backwards compatibality - vars[i] = strings.SplitN(val, ".", 2)[0] + if len(strings.Split(val, ".")) > 2 { + errString = fmt.Sprintf(`Invalid referencing of parameters in "%s"! Only two dot-separated components after the prefix "%s" are allowed.`, s, prefix) + return vars, true, errString } + vars[i] = strings.SplitN(val, ".", 2)[0] break } - if groups[v] != "" { + if val != "" { vars[i] = val break } - } } return vars, true, errString } +func extractEntireVariablesFromString(s, prefix string) ([]string, error) { + pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution, parameterSubstitution, parameterSubstitution) + re, err := regexp.Compile(pattern) + if err != nil { + return nil, fmt.Errorf("Fail to parse regex pattern: %v", err) + } + + matches := re.FindAllStringSubmatch(s, -1) + if len(matches) == 0 { + return []string{}, nil + } + vars := make([]string, len(matches)) + for i, match := range matches { + groups := matchGroups(match, re) + // foo -> foo + // foo.bar -> foo.bar + // foo.bar.baz -> foo.bar.baz + for _, v := range []string{"var1", "var2", "var3"} { + val := groups[v] + if val != "" { + vars[i] = val + break + } + } + } + return vars, nil +} + func matchGroups(matches []string, pattern *regexp.Regexp) map[string]string { groups := make(map[string]string) for i, name := range pattern.SubexpNames()[1:] { diff --git a/pkg/substitution/substitution_test.go b/pkg/substitution/substitution_test.go index 1167349be55..5a7ef316ec9 100644 --- a/pkg/substitution/substitution_test.go +++ b/pkg/substitution/substitution_test.go @@ -18,6 +18,7 @@ limitations under the License. package substitution_test import ( + "fmt" "testing" "github.com/google/go-cmp/cmp" @@ -174,7 +175,7 @@ func TestValidateVariablePs(t *testing.T) { vars: sets.NewString("foo.bar.baz"), }, expectedError: &apis.FieldError{ - Message: `Invalid referencing of parameters in --flag=$(params.foo.bar.baz) !!! You can only use the dots inside single or double quotes. eg. $(params["org.foo.blah"]) or $(params['org.foo.blah']) are valid references but NOT $params.org.foo.blah.`, + Message: fmt.Sprintf(`Invalid referencing of parameters in "%s"! Only two dot-separated components after the prefix "%s" are allowed.`, "--flag=$(params.foo.bar.baz)", "params"), Paths: []string{""}, }, }, { @@ -193,7 +194,7 @@ func TestValidateVariablePs(t *testing.T) { vars: sets.NewString("foo.bar"), }, expectedError: &apis.FieldError{ - Message: `Invalid referencing of parameters in --flag=$(resources.inputs.foo.bar.baz) !!! resources.* can only have 4 components (eg. resources.inputs.foo.bar). Found more than 4 components.`, + Message: fmt.Sprintf(`Invalid referencing of parameters in "%s"! Only two dot-separated components after the prefix "%s" are allowed.`, "--flag=$(resources.inputs.foo.bar.baz)", "resources.(?:inputs|outputs)"), Paths: []string{""}, }, }, { @@ -220,6 +221,22 @@ func TestValidateVariablePs(t *testing.T) { vars: sets.NewString("baz", "foo"), }, expectedError: nil, + }, { + name: "valid usage of an individual attribute of an object param", + args: args{ + input: "--flag=$(params.objectParam.key1)", + prefix: "params.objectParam", + vars: sets.NewString("key1", "key2"), + }, + expectedError: nil, + }, { + name: "valid usage of multiple individual attributes of an object param", + args: args{ + input: "--flag=$(params.objectParam.key1) $(params.objectParam.key2)", + prefix: "params.objectParam", + vars: sets.NewString("key1", "key2"), + }, + expectedError: nil, }, { name: "different context and prefix", args: args{ @@ -239,6 +256,17 @@ func TestValidateVariablePs(t *testing.T) { Message: `non-existent variable in "--flag=$(inputs.params.baz)"`, Paths: []string{""}, }, + }, { + name: "undefined individual attributes of an object param", + args: args{ + input: "--flag=$(params.objectParam.key3)", + prefix: "params.objectParam", + vars: sets.NewString("key1", "key2"), + }, + expectedError: &apis.FieldError{ + Message: `non-existent variable in "--flag=$(params.objectParam.key3)"`, + Paths: []string{""}, + }, }} { t.Run(tc.name, func(t *testing.T) { got := substitution.ValidateVariableP(tc.args.input, tc.args.prefix, tc.args.vars) @@ -249,6 +277,69 @@ func TestValidateVariablePs(t *testing.T) { }) } } + +func TestValidateEntireVariableProhibitedP(t *testing.T) { + type args struct { + input string + prefix string + vars sets.String + } + for _, tc := range []struct { + name string + args args + expectedError *apis.FieldError + }{{ + name: "valid usage of an individual key of an object param", + args: args{ + input: "--flag=$(params.objectParam.key1)", + prefix: "params", + vars: sets.NewString("objectParam"), + }, + expectedError: nil, + }, { + name: "invalid regex", + args: args{ + input: "--flag=$(params.objectParam.key1)", + prefix: `???`, + vars: sets.NewString("objectParam"), + }, + expectedError: &apis.FieldError{ + Message: "Fail to extract entire variable from string: Fail to parse regex pattern: error parsing regexp: invalid nested repetition operator: `???`", + Paths: []string{""}, + }, + }, { + name: "invalid usage of an entire object param when providing values for strings", + args: args{ + input: "--flag=$(params.objectParam)", + prefix: "params", + vars: sets.NewString("objectParam"), + }, + expectedError: &apis.FieldError{ + Message: `variable type invalid in "--flag=$(params.objectParam)"`, + Paths: []string{""}, + }, + }, { + name: "invalid usage of an entire object param when providing values for strings", + args: args{ + input: "--flag=$(params.objectParam[*])", + prefix: "params", + vars: sets.NewString("objectParam"), + }, + expectedError: &apis.FieldError{ + Message: `variable type invalid in "--flag=$(params.objectParam[*])"`, + Paths: []string{""}, + }, + }} { + t.Run(tc.name, func(t *testing.T) { + got := substitution.ValidateEntireVariableProhibitedP(tc.args.input, tc.args.prefix, tc.args.vars) + + if d := cmp.Diff(got, tc.expectedError, cmp.AllowUnexported(apis.FieldError{})); d != "" { + t.Errorf("ValidateEntireVariableProhibitedP() error did not match expected error %s", diff.PrintWantGot(d)) + } + }) + } +} + func TestApplyReplacements(t *testing.T) { type args struct { input string