From 86720cd5c7c6566f84b527fc4b8c3f9afe443072 Mon Sep 17 00:00:00 2001 From: Chuang Wang Date: Thu, 21 Apr 2022 20:48:50 -0700 Subject: [PATCH] Add validation against the usage of the entire object According to TEP-0075: When providing values for strings, Task and Pipeline authors can access individual attributes of an object param; but they cannot access the object as whole. --- pkg/apis/pipeline/v1beta1/task_validation.go | 44 +++++++++-- .../pipeline/v1beta1/task_validation_test.go | 62 +++++++++++++++ pkg/substitution/substitution.go | 41 ++++++++++ pkg/substitution/substitution_test.go | 79 +++++++++++++++++++ 4 files changed, 220 insertions(+), 6 deletions(-) diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index 6fbc2be274d..43478b8948c 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -361,11 +361,7 @@ func ValidateResourcesVariables(steps []Step, resources *TaskResources) *apis.Fi return validateVariables(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(steps []Step, params []ParamSpec) (errs *apis.FieldError) { objectParameterNames := sets.NewString() for _, p := range params { @@ -386,7 +382,7 @@ func validateObjectUsage(steps []Step, params []ParamSpec) (errs *apis.FieldErro errs = errs.Also(validateVariables(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 @@ -407,6 +403,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) @@ -492,6 +520,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 255e6f5c3c1..f5e83538560 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{ @@ -855,6 +873,50 @@ 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 unaccepted 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: "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: "object star used in unaccepted 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: "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 ba363a08823..b60591a349e 100644 --- a/pkg/substitution/substitution.go +++ b/pkg/substitution/substitution.go @@ -94,6 +94,23 @@ 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 { + if vs, present := extractEntireVariablesFromString(value, prefix); present { + 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 { @@ -169,6 +186,30 @@ func extractVariablesFromString(s, prefix string) ([]string, bool) { return vars, true } +func extractEntireVariablesFromString(s, prefix string) ([]string, bool) { + pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution, parameterSubstitution, parameterSubstitution) + re := regexp.MustCompile(pattern) + matches := re.FindAllStringSubmatch(s, -1) + if len(matches) == 0 { + return []string{}, false + } + 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, true +} + 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 ab80dceb4e0..8ea0dd7f154 100644 --- a/pkg/substitution/substitution_test.go +++ b/pkg/substitution/substitution_test.go @@ -174,6 +174,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{ @@ -193,6 +209,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) @@ -203,6 +230,58 @@ 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 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