Skip to content

Commit

Permalink
Add validation against the usage of the entire object
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@chuangw6
chuangw6 committed May 20, 2022
1 parent 2cff700 commit 73681ce
Show file tree
Hide file tree
Showing 4 changed files with 264 additions and 6 deletions.
44 changes: 38 additions & 6 deletions pkg/apis/pipeline/v1beta1/task_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down
106 changes: 106 additions & 0 deletions pkg/apis/pipeline/v1beta1/task_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -855,6 +873,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{
Expand Down
41 changes: 41 additions & 0 deletions pkg/substitution/substitution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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:] {
Expand Down
79 changes: 79 additions & 0 deletions pkg/substitution/substitution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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)
Expand All @@ -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
Expand Down

0 comments on commit 73681ce

Please sign in to comment.