Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: incorrect templating not producing error #765

Closed
wants to merge 7 commits into from
Closed

fix: incorrect templating not producing error #765

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8f5dcbc
fix: incorrect templating not producing error
mikeykhalil Apr 17, 2019
e818db1
reduce duplicate code in task validation
mikeykhalil Apr 18, 2019
454f4c9
templating: add test to ensure nested variable support
mikeykhalil Apr 21, 2019
d8ef177
task validation: fix test name
mikeykhalil Apr 21, 2019
93e68d0
task validation: add comment to explain new behavior
mikeykhalil Apr 21, 2019
1b10032
tests: add test for AttributeFromType
mikeykhalil Apr 24, 2019
43eaa8b
resource types: leave comment to explain problem
mikeykhalil Apr 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions pkg/apis/pipeline/v1alpha1/resource_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,19 @@ func ResourceFromType(r *PipelineResource) (PipelineResourceInterface, error) {
}
return nil, fmt.Errorf("%s is an invalid or unimplemented PipelineResource", r.Spec.Type)
}

// AttributesFromType returns a list of available attributes that can be replaced
// through templating.
func AttributesFromType(prt PipelineResourceType) ([]string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could have some unit tests for AttributesFromType too :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad you recommended this! Ended up running into an issue where this function was not working for one of the types (PipelineResourceTypeStorage). I included a fairly kludgy solution and a comment to help illustrate the problem.

We have run into this unfortunate event where technically we cannot determine what attributes an object has strictly from the PipelineResourceType alone. This is because of the subtypes that exists for the PipelineResourceTypeStorage type. At the moment, both of the subtypes happen to have equivalent keys in their Replacements(). So technically we can just pick one of the subtypes and still return the proper attributes. However, we may want to reconsider the approach on how we obtain or store these attributes.

I do not have more time to look at this at the moment, but will find time soon to try to come up with a more elegant solution.

r := &PipelineResource{}
r.Spec.Type = prt
resource, err := ResourceFromType(r)
if err != nil {
return nil, err
}
keys := []string{}
for k := range resource.Replacements() {
keys = append(keys, k)
}
return keys, nil
}
77 changes: 60 additions & 17 deletions pkg/apis/pipeline/v1alpha1/task_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,67 +132,110 @@ func validateInputParameterVariables(steps []corev1.Container, inputs *Inputs) *
parameterNames[p.Name] = struct{}{}
}
}
return validateVariables(steps, "params", parameterNames)
return validateVariables(steps, "params", "inputs.", parameterNames)
}

func validateResourceVariables(steps []corev1.Container, inputs *Inputs, outputs *Outputs) *apis.FieldError {
resourceNames := map[string]struct{}{}
inputVars, err := getInputResourceVariables(inputs)
if err != nil {
return err
}
outputVars, err := getOutputResourceVariables(outputs)
if err != nil {
return err
}
err = validateVariables(steps, "resources", "inputs.", inputVars)
if err != nil {
return err
}
return validateVariables(steps, "resources", "outputs.", outputVars)
}

func getInputResourceVariables(inputs *Inputs) (map[string]struct{}, *apis.FieldError) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why you chose to use two separate functions for input & output here - it seems like it would be possible to write a single function to handle both with a signature like:

func getResourceVariables(resources []TaskResource, pathPrefix string)

I might be misreading but it looks like the only line of difference between getInputResourceVariables and getOutputResourceVariables is the path string in the returned apis.FieldError, which could be resolved using a pathPrefix arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're totally right! There was plenty of duplicate code there. I went ahead and made these changes. Thanks for the suggestion.

vars := map[string]struct{}{}
if inputs != nil {
for _, r := range inputs.Resources {
resourceNames[r.Name] = struct{}{}
attrs, err := AttributesFromType(r.Type)
if err != nil {
return nil, &apis.FieldError{
Message: fmt.Sprintf("invalid resource type %s", r.Type),
Paths: []string{"taskspec.inputs.resources." + r.Name},
Details: err.Error(),
}
}
for _, a := range attrs {
rv := r.Name + "." + a
vars[rv] = struct{}{}
}
}
}
return vars, nil
}

func getOutputResourceVariables(outputs *Outputs) (map[string]struct{}, *apis.FieldError) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's worth having unit tests for getOutputResourceVariables and getInputResourceVariables specifically - that might also help address @sbwsg 's feedback about knowing exactly what changed, b/c we'd be able to see it in the test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changed slightly and has been reduced to one function, getResourceVariables. Your comment is still completely valid though. I have an idea for a test that could be constructive for getResourceVariables, but I want to put a bit more thought into it so that it's not tightly coupled with the current structure of a given resource type. Will circle back to this tomorrow.

vars := map[string]struct{}{}
if outputs != nil {
for _, r := range outputs.Resources {
resourceNames[r.Name] = struct{}{}
attrs, err := AttributesFromType(r.Type)
if err != nil {
return nil, &apis.FieldError{
Message: fmt.Sprintf("invalid resource type %s", r.Type),
Paths: []string{"taskspec.outputs.resources." + r.Name},
Details: err.Error(),
}
}
for _, a := range attrs {
rv := r.Name + "." + a
vars[rv] = struct{}{}
}
}
}
return validateVariables(steps, "resources", resourceNames)
return vars, nil
}

func validateVariables(steps []corev1.Container, prefix string, vars map[string]struct{}) *apis.FieldError {
func validateVariables(steps []corev1.Container, prefix, contextPrefix string, vars map[string]struct{}) *apis.FieldError {
for _, step := range steps {
if err := validateTaskVariable("name", step.Name, prefix, vars); err != nil {
if err := validateTaskVariable("name", step.Name, prefix, contextPrefix, vars); err != nil {
return err
}
if err := validateTaskVariable("image", step.Image, prefix, vars); err != nil {
if err := validateTaskVariable("image", step.Image, prefix, contextPrefix, vars); err != nil {
return err
}
if err := validateTaskVariable("workingDir", step.WorkingDir, prefix, vars); err != nil {
if err := validateTaskVariable("workingDir", step.WorkingDir, prefix, contextPrefix, vars); err != nil {
return err
}
for i, cmd := range step.Command {
if err := validateTaskVariable(fmt.Sprintf("command[%d]", i), cmd, prefix, vars); err != nil {
if err := validateTaskVariable(fmt.Sprintf("command[%d]", i), cmd, prefix, contextPrefix, vars); err != nil {
return err
}
}
for i, arg := range step.Args {
if err := validateTaskVariable(fmt.Sprintf("arg[%d]", i), arg, prefix, vars); err != nil {
if err := validateTaskVariable(fmt.Sprintf("arg[%d]", i), arg, prefix, contextPrefix, vars); err != nil {
return err
}
}
for _, env := range step.Env {
if err := validateTaskVariable(fmt.Sprintf("env[%s]", env.Name), env.Value, prefix, vars); err != nil {
if err := validateTaskVariable(fmt.Sprintf("env[%s]", env.Name), env.Value, prefix, contextPrefix, vars); err != nil {
return err
}
}
for i, v := range step.VolumeMounts {
if err := validateTaskVariable(fmt.Sprintf("volumeMount[%d].Name", i), v.Name, prefix, vars); err != nil {
if err := validateTaskVariable(fmt.Sprintf("volumeMount[%d].Name", i), v.Name, prefix, contextPrefix, vars); err != nil {
return err
}
if err := validateTaskVariable(fmt.Sprintf("volumeMount[%d].MountPath", i), v.MountPath, prefix, vars); err != nil {
if err := validateTaskVariable(fmt.Sprintf("volumeMount[%d].MountPath", i), v.MountPath, prefix, contextPrefix, vars); err != nil {
return err
}
if err := validateTaskVariable(fmt.Sprintf("volumeMount[%d].SubPath", i), v.SubPath, prefix, vars); err != nil {
if err := validateTaskVariable(fmt.Sprintf("volumeMount[%d].SubPath", i), v.SubPath, prefix, contextPrefix, vars); err != nil {
return err
}
}
}
return nil
}

func validateTaskVariable(name, value, prefix string, vars map[string]struct{}) *apis.FieldError {
return templating.ValidateVariable(name, value, prefix, "(?:inputs|outputs).", "step", "taskspec.steps", vars)
func validateTaskVariable(name, value, prefix, contextPrefix string, vars map[string]struct{}) *apis.FieldError {
return templating.ValidateVariable(name, value, prefix, contextPrefix, "step", "taskspec.steps", vars)
}

func checkForDuplicates(resources []TaskResource, path string) *apis.FieldError {
Expand Down
43 changes: 41 additions & 2 deletions pkg/apis/pipeline/v1alpha1/task_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func TestTaskSpec_Validate(t *testing.T) {
Name: "mystep",
Image: "${inputs.resources.foo.url}",
Args: []string{"--flag=${inputs.params.baz} && ${input.params.foo-is-baz}"},
WorkingDir: "/foo/bar/${outputs.resources.source}",
WorkingDir: "/foo/bar/${outputs.resources.source.name}",
mikeykhalil marked this conversation as resolved.
Show resolved Hide resolved
}},
},
}}
Expand Down Expand Up @@ -321,7 +321,46 @@ func TestTaskSpec_ValidateError(t *testing.T) {
Message: `non-existent variable in "${inputs.params.foo} && ${inputs.params.inexistent}" for step arg[0]`,
Paths: []string{"taskspec.steps.arg[0]"},
},
}}
}, {
name: "invalid resource variable usage",
fields: fields{
Inputs: &Inputs{
Resources: []TaskResource{{
Name: "foo",
Type: PipelineResourceTypeImage,
}},
},
BuildSteps: []corev1.Container{{
Name: "mystep",
Image: "myimage",
Args: []string{"${inputs.resources.foo}"},
}},
},
expectedError: apis.FieldError{
Message: `non-existent variable in "${inputs.resources.foo}" for step arg[0]`,
Paths: []string{"taskspec.steps.arg[0]"},
},
},
{
name: "inexistent output param variable",
fields: fields{
Inputs: &Inputs{
Resources: []TaskResource{{
Name: "foo",
Type: PipelineResourceTypeImage,
}},
},
BuildSteps: []corev1.Container{{
Name: "mystep",
Image: "myimage",
Args: []string{"${outputs.resources.foo.url}"},
}},
},
expectedError: apis.FieldError{
Message: `non-existent variable in "${outputs.resources.foo.url}" for step arg[0]`,
Paths: []string{"taskspec.steps.arg[0]"},
},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ts := &TaskSpec{
Expand Down
5 changes: 1 addition & 4 deletions pkg/templating/templating.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,7 @@ func extractVariablesFromString(s, prefix string) ([]string, bool) {
vars := make([]string, len(matches))
for i, match := range matches {
groups := matchGroups(match, re)
// foo -> foo
// foo.bar -> foo
// foo.bar.baz -> foo
vars[i] = strings.SplitN(groups["var"], ".", 2)[0]
vars[i] = groups["var"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time figuring out if the logic has remained the same here or has been changed to make an intentional difference to behaviour. Would you mind just quickly describing what this change is?

Copy link
Contributor Author

@mikeykhalil mikeykhalil Apr 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I intentionally made this change.

I added a new test to the templating package which hopefully provides additional insight as to why this was needed. This new test would fail without this change.

strings.SplitN(groups["var"], ".", 2)[0] was just returning the first split of a variable. So if groups["var"] was set to foo.bar.baz, it would just return foo.

Since the vars map passed into ValidateVariable can contain nested variables now, like fooImage.url, I believe we want to return the entire variable string to validate against.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK got it, thankyou, I understand the original issue and this fix more clearly now!

}
return vars, true
}
Expand Down