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.

Also modified `extractVariablesFromString` function a bit to recognize
`params.foo.bar` reference that was treated as invalid previously.
  • Loading branch information
@chuangw6 @tekton-robot
chuangw6 authored and tekton-robot committed Jun 13, 2022
1 parent d4c3786 commit c13edc2
Show file tree
Hide file tree
Showing 4 changed files with 301 additions and 28 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 @@ -375,11 +375,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 {
Expand All @@ -396,7 +392,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))
}

// validateObjectDefault validates the keys of all the object params within a
Expand Down Expand Up @@ -438,6 +434,38 @@ func validateObjectKeys(properties map[string]PropertySpec, propertiesProvider *
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 @@ -525,6 +553,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 @@ -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{
Expand Down
84 changes: 64 additions & 20 deletions pkg/substitution/substitution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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("extractEntireVariablesFromString failed : %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 {
Expand Down Expand Up @@ -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 <prefix>.aString
// - extract "anObject" from <prefix>.anObject.key
// Invalid Examples:
// - <prefix>.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:] {
Expand Down
Loading

0 comments on commit c13edc2

Please sign in to comment.