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

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 all commits
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
33 changes: 33 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,36 @@ 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
// Todo : The TaskResource struct lacks data to correctly infer the type of
// a PipelineResourceTypeStorage. While all the currently implemented types
// have the same attributes, this doesn't appear to be an explicit design
// choice. Future types could not fit this constraint. So we cannot safely
// make any assumptions about the attributes.
if prt == PipelineResourceTypeStorage {
r.Spec.Params = []Param{
{
Name: "type",
Value: string(PipelineResourceTypeGCS),
Copy link
Member

Choose a reason for hiding this comment

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

PipelineResourceTypeStorage could be something else than PipelineResourceTypeGCS though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I think this is the blocking issue at the moment. I left a comment above in the code which goes into more detail.

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.

Still not exactly sure how to go about solving this problem, but I will think about it some more and update this PR with a better solution once I find the time.

Copy link

Choose a reason for hiding this comment

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

It seems like there isn't enough information in a TaskResource struct to correctly infer the type of a PipelineResourceTypeStorage. The exact storage type only gets determined when a TaskRun is created I think, right? It doesn't seem to me like there's a way to validate storage when the Task is applied since its type parameter wont be declared until later.

Options I can see right now are:

  1. Leave the code as it is :- make an assumption about the storage type. Future storage types will need to include these keys to validate or will need to change this validation code.
  2. Augment the TaskResource with enough info to infer the correct storage type to validate against. This seems counter to the design of PipelineResourceTypeStorage - the dynamic binding of the storage's exact type is intentionally left until the TaskRun to allow different sources to feed through a pipeline, I think?
  3. Agree on a set of templatable params that all storage types must implement.
  4. Iterate over all known storage types and check whether the templated fields match at least one of those storage kinds.
  5. Abandon validating storage params in a Task template. Maybe do it in TaskRuns instead.

Have I misunderstood the implementation of TaskResource / TaskResourceBinding? Are there other options available to us here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your analysis seems correct to me and I was thinking about a few of those options as well.

I agree that option 2 seems to counter the current design, though it's possible I have a misunderstanding.

I think the rest of the options you mentioned are solid approaches.

In my mind, option 3 sounds like an ideal approach and a convenient characteristic for all types of PipelineResourceTypeStorage to share. However, this might not be realistic depending on the types of PipelineResourceTypeStorage we plan to support in the future.

Option 4 seems like something that could be built now with not much effort and provide immediate benefit. There could be some cases in the future where different
types of PipelineResourceTypeStorage don't share the same templatable params, in which case we would only be able to catch certain errors at TaskRun execution time.

Option 5 seems reasonable and provides similar benefits as option 4. We will catch most kinds of templating errors earlier on but still have certain errors at TaskRun execution time.

After some thought, if we feel we could reach a consensus on option 3 then that seems like the most robust approach.

If that's not something we think we can enforce, then option 4 or 5 sound great to me.

Copy link

Choose a reason for hiding this comment

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

Option 3 seems like a design that would be worth raising with the community - I doubt we could confidently come to a widely agreeable consensus in this github thread alone. So the process for that would be to create a github issue describing the problem along with a suggested resolution and design doc if the problem is gnarly enough (this one is, I think).

It may also be that we can push this specific problem back until there is more movement on Pipeline Resource Extensibility, which may end up setting the "validatable surface area" simply as a byproduct of the interfaces that it defines for extensions.

Having said all that it seems to me that we could move ahead with options 4 or 5 in this PR and work on option 3 out-of-band. If it were me I'd probably skip validating storage types completely, leave a comment explaining why, and create a github issue to describe the problem and possible solutions in detail.

Copy link

@ghost ghost May 2, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

Actually thinking about this some more, I reckon simply pushing the validation to the TaskRun (if at all possible, otherwise skipping for now) is totally fine. The whole point of the PipelineResourceTypeStorage is that it's a placeholder to be filled in later. The Task shouldn't need to really worry about validating it deeply. At least from my POV.

Copy link

@ghost ghost May 7, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

I'm now realizing that option 5, skipping storage params, is kind of tricky. It's not possible to easily express "ignore variables of this type" when doing the template validation atm. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, if the task shouldn't have to worry about validating it deeply, then it's possible that we do not really want this kind of patch and may just want to revisit this in a more broad way with a github issue and a design doc at some point.

Otherwise, it might be possible to go with a hybrid approach where we deeply validate all types of variables besides storage. Then for storage params, we just validate the top level variable (similar to how the validation works currently). But yeah, not sure if there is an easy way to express that as you said.

Been busy so I haven't had time to take a look, but I can take a look this weekend to see if i can come up with a nice way to solve it.

},
{
Name: "Location",
Value: "/",
},
}
}
resource, err := ResourceFromType(r)
if err != nil {
return nil, err
}
keys := []string{}
for k := range resource.Replacements() {
keys = append(keys, k)
}
return keys, nil
}
60 changes: 60 additions & 0 deletions pkg/apis/pipeline/v1alpha1/resource_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
Copyright 2018 The Knative Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package v1alpha1

import (
"testing"

"github.com/google/go-cmp/cmp"
)

func TestAttributesFromType(t *testing.T) {
tests := []struct {
name string
resourceType PipelineResourceType
expectedErr error
}{
{
name: "git resource type",
resourceType: PipelineResourceTypeGit,
expectedErr: nil,
},
{
name: "storage resource type",
resourceType: PipelineResourceTypeStorage,
expectedErr: nil,
},
{
name: "image resource type",
resourceType: PipelineResourceTypeImage,
expectedErr: nil,
},
{
name: "cluster resource type",
resourceType: PipelineResourceTypeCluster,
expectedErr: nil,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
_, err := AttributesFromType(tc.resourceType)
if d := cmp.Diff(err, tc.expectedErr); d != "" {
t.Errorf("AttributesFromType error did not match expected error %s", d)
}
})
}
}
71 changes: 52 additions & 19 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,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
102 changes: 100 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 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 All @@ -339,3 +378,62 @@ func TestTaskSpec_ValidateError(t *testing.T) {
})
}
}

func TestGetResourceVariables(t *testing.T) {
tests := []struct {
name string
pathPrefix string
resources []TaskResource
expectedVars map[string]struct{}
}{
{
name: "empty resources",
pathPrefix: "taskspec.outputs.resources.",
resources: []TaskResource{},
expectedVars: map[string]struct{}{},
},
{
name: "single resource",
pathPrefix: "taskspec.outputs.resources.",
resources: []TaskResource{validResource},
expectedVars: map[string]struct{}{
"source.name": struct{}{},
"source.type": struct{}{},
"source.url": struct{}{},
"source.revision": struct{}{},
},
},
{
name: "multiple resources",
pathPrefix: "taskspec.outputs.resources.",
resources: []TaskResource{
validResource,
TaskResource{
Name: "foo",
Type: PipelineResourceTypeImage,
},
},
expectedVars: map[string]struct{}{
"source.name": struct{}{},
"source.type": struct{}{},
"source.url": struct{}{},
"source.revision": struct{}{},
"foo.name": struct{}{},
"foo.type": struct{}{},
"foo.url": struct{}{},
"foo.digest": struct{}{},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
vars, err := getResourceVariables(tt.resources, tt.pathPrefix)
if err != nil {
t.Errorf("getResourceVariables() = %v", err)
}
if d := cmp.Diff(tt.expectedVars, vars); d != "" {
t.Errorf("getResourceVariables results diff -want, +got: %v", d)
}
})
}
}
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
17 changes: 17 additions & 0 deletions pkg/templating/templating_test.go
View file
Open in desktop
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