From 8f5dcbc98cd26e05361d2e4085dd3764dd931661 Mon Sep 17 00:00:00 2001 From: mikeykhalil Date: Tue, 16 Apr 2019 21:36:32 -0700 Subject: [PATCH] fix: incorrect templating not producing error --- pkg/apis/pipeline/v1alpha1/resource_types.go | 16 ++++ pkg/apis/pipeline/v1alpha1/task_validation.go | 77 +++++++++++++++---- .../pipeline/v1alpha1/task_validation_test.go | 43 ++++++++++- pkg/templating/templating.go | 5 +- 4 files changed, 118 insertions(+), 23 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/resource_types.go b/pkg/apis/pipeline/v1alpha1/resource_types.go index 5d680591204..359177de7d2 100644 --- a/pkg/apis/pipeline/v1alpha1/resource_types.go +++ b/pkg/apis/pipeline/v1alpha1/resource_types.go @@ -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) { + 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 +} diff --git a/pkg/apis/pipeline/v1alpha1/task_validation.go b/pkg/apis/pipeline/v1alpha1/task_validation.go index e40d57fa740..55466742b7e 100644 --- a/pkg/apis/pipeline/v1alpha1/task_validation.go +++ b/pkg/apis/pipeline/v1alpha1/task_validation.go @@ -132,58 +132,101 @@ 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) { + 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) { + 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 } } @@ -191,8 +234,8 @@ func validateVariables(steps []corev1.Container, prefix string, vars map[string] 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 { diff --git a/pkg/apis/pipeline/v1alpha1/task_validation_test.go b/pkg/apis/pipeline/v1alpha1/task_validation_test.go index 93ecd5b544d..b0fe1d9e250 100644 --- a/pkg/apis/pipeline/v1alpha1/task_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/task_validation_test.go @@ -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}", }}, }, }} @@ -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{ diff --git a/pkg/templating/templating.go b/pkg/templating/templating.go index c2ccb0c9500..2c1654cb602 100644 --- a/pkg/templating/templating.go +++ b/pkg/templating/templating.go @@ -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"] } return vars, true }