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.

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

fix: incorrect templating not producing error #765

Closed
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
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
}
71 changes: 52 additions & 19 deletions pkg/apis/pipeline/v1alpha1/task_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,67 +132,100 @@ 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{}{}
var err *apis.FieldError
// Keep track of input and output resources separately.
// This ensures we can validate against appropriate variables set.
inputVars := map[string]struct{}{}
outputVars := map[string]struct{}{}
if inputs != nil {
for _, r := range inputs.Resources {
resourceNames[r.Name] = struct{}{}
inputVars, err = getResourceVariables(inputs.Resources, "taskspec.inputs.resources.")
if err != nil {
return err
}
}
if outputs != nil {
for _, r := range outputs.Resources {
resourceNames[r.Name] = struct{}{}
outputVars, err = getResourceVariables(outputs.Resources, "taskspec.outputs.resources.")
if err != nil {
return err
}
}
err = validateVariables(steps, "resources", "inputs.", inputVars)
if err != nil {
return err
}
err = validateVariables(steps, "resources", "outputs.", outputVars)
if err != nil {
return err
}
return nil
}

func getResourceVariables(resources []TaskResource, pathPrefix string) (map[string]struct{}, *apis.FieldError) {
vars := map[string]struct{}{}
for _, r := range resources {
attrs, err := AttributesFromType(r.Type)
if err != nil {
return nil, &apis.FieldError{
Message: fmt.Sprintf("invalid resource type %s", r.Type),
Paths: []string{pathPrefix + 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
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 resource 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
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

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
17 changes: 17 additions & 0 deletions pkg/templating/templating_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,23 @@ func TestValidateVariables(t *testing.T) {
},
expectedError: nil,
},
{
name: "nested variable",
args: args{
input: "--flag=${inputs.resources.foo.url}",
prefix: "resources",
contextPrefix: "inputs.",
locationName: "step",
path: "taskspec.steps",
vars: map[string]struct{}{
"foo.name": {},
"foo.type": {},
"foo.url": {},
"foo.digest": {},
},
},
expectedError: nil,
},
{
name: "undefined variable",
args: args{
Expand Down