From 955b42ca917d442e387d46447aad90605cc9b0f7 Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Fri, 7 Dec 2018 11:24:02 -0800 Subject: [PATCH] Validate that extra params + resources aren't provided MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While working on creating an end to end test for #168, @dlorenc and I had a very hard time determining why our pipeline wasn't working as expected - it turned out that we were using the wrong resource name in our `providedBy` clauses. This is the first of a couple follow up commits to add more validation: this one makes sure that extra resources are not provided (e.g. if you typo-ed a param with a default value, you may never know your param was being ignored). Some minor changes: * Combined 3 test cases in the taskrun reconciler into one test (testing templating with default params). * Reorganized logic in some reconciler test failures so that `Fatalf` won't prevent the dev from seeing informative failure info (specifically the condition tends to have a reason that explains why the case is failing) (Shout out to @vdemeester - I updated the reconciler tests both before and after his test refactoring, and it was nearly impossible to understand the tests before his builders were added: the builders made it waaaaay easier! 🙏 Follow up on #124 --- .../v1alpha1/pipelinerun/pipelinerun_test.go | 4 +- pkg/reconciler/v1alpha1/taskrun/list/diff.go | 35 ++++ .../v1alpha1/taskrun/list/diff_test.go | 52 ++++++ .../resources/taskresourceresolution_test.go | 8 +- .../v1alpha1/taskrun/taskrun_test.go | 91 +++------- pkg/reconciler/v1alpha1/taskrun/validate.go | 70 -------- .../v1alpha1/taskrun/validate_resources.go | 111 ++++++++++++ .../taskrun/validate_resources_test.go | 161 ++++++++++++++++++ .../v1alpha1/taskrun/validate_test.go | 104 ----------- test/builder/task.go | 12 +- test/builder/task_test.go | 9 + 11 files changed, 406 insertions(+), 251 deletions(-) create mode 100644 pkg/reconciler/v1alpha1/taskrun/list/diff.go create mode 100644 pkg/reconciler/v1alpha1/taskrun/list/diff_test.go delete mode 100644 pkg/reconciler/v1alpha1/taskrun/validate.go create mode 100644 pkg/reconciler/v1alpha1/taskrun/validate_resources.go create mode 100644 pkg/reconciler/v1alpha1/taskrun/validate_resources_test.go delete mode 100644 pkg/reconciler/v1alpha1/taskrun/validate_test.go diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go index ed3b8bec8b8..b145b103624 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go @@ -105,7 +105,7 @@ func TestReconcile(t *testing.T) { tb.Task("unit-test-task", "foo", tb.TaskSpec( tb.TaskInputs( tb.InputsResource("workspace", v1alpha1.PipelineResourceTypeGit), - tb.InputsParam("foo"), tb.InputsParam("bar"), + tb.InputsParam("foo"), tb.InputsParam("bar"), tb.InputsParam("templatedparam"), ), tb.TaskOutputs( tb.OutputsResource("image-to-use", v1alpha1.PipelineResourceTypeImage), @@ -120,7 +120,7 @@ func TestReconcile(t *testing.T) { tb.ClusterTask("unit-test-cluster-task", tb.ClusterTaskSpec( tb.TaskInputs( tb.InputsResource("workspace", v1alpha1.PipelineResourceTypeGit), - tb.InputsParam("foo"), tb.InputsParam("bar"), + tb.InputsParam("foo"), tb.InputsParam("bar"), tb.InputsParam("templatedparam"), ), tb.TaskOutputs( tb.OutputsResource("image-to-use", v1alpha1.PipelineResourceTypeImage), diff --git a/pkg/reconciler/v1alpha1/taskrun/list/diff.go b/pkg/reconciler/v1alpha1/taskrun/list/diff.go new file mode 100644 index 00000000000..34a798b2d11 --- /dev/null +++ b/pkg/reconciler/v1alpha1/taskrun/list/diff.go @@ -0,0 +1,35 @@ +/* +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 extress or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package list + +// DiffLeft will return all strings which are in the left list of stirngs but +// not in the right. +func DiffLeft(left, right []string) []string { + extra := []string{} + for _, s := range left { + found := false + for _, s2 := range right { + if s == s2 { + found = true + } + } + if !found { + extra = append(extra, s) + } + } + return extra +} diff --git a/pkg/reconciler/v1alpha1/taskrun/list/diff_test.go b/pkg/reconciler/v1alpha1/taskrun/list/diff_test.go new file mode 100644 index 00000000000..51adc259a80 --- /dev/null +++ b/pkg/reconciler/v1alpha1/taskrun/list/diff_test.go @@ -0,0 +1,52 @@ +/* +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 extress or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package list + +import ( + "reflect" + "testing" +) + +func TestDiffLeft_same(t *testing.T) { + left := []string{"elsa", "anna", "olaf", "kristoff"} + right := []string{"elsa", "anna", "olaf", "kristoff"} + extraLeft := DiffLeft(left, right) + + if !reflect.DeepEqual(extraLeft, []string{}) { + t.Errorf("Didn't expect extra strings in left list but got %v", extraLeft) + } +} + +func TestDiffLeft_extraLeft(t *testing.T) { + left := []string{"elsa", "anna", "olaf", "kristoff", "hans"} + right := []string{"elsa", "anna", "olaf", "kristoff"} + extraLeft := DiffLeft(left, right) + + if !reflect.DeepEqual(extraLeft, []string{"hans"}) { + t.Errorf("Should have identified extra string in left list but got %v", extraLeft) + } +} + +func TestDiffLeft_extraRight(t *testing.T) { + left := []string{"elsa", "anna", "olaf", "kristoff"} + right := []string{"elsa", "anna", "olaf", "kristoff", "hans"} + extraLeft := DiffLeft(left, right) + + if !reflect.DeepEqual(extraLeft, []string{}) { + t.Errorf("Shouldn't have noticed extra item in right list but got %v", extraLeft) + } +} diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution_test.go index b7b8f39f101..727f9d500c6 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution_test.go @@ -51,7 +51,7 @@ func TestResolveTaskRun(t *testing.T) { }} taskName := "orchestrate" - taskSpec := &v1alpha1.TaskSpec{ + taskSpec := v1alpha1.TaskSpec{ Steps: []corev1.Container{{ Name: "step1", }}} @@ -80,7 +80,7 @@ func TestResolveTaskRun(t *testing.T) { return r, nil } - rtr, err := ResolveTaskResources(taskSpec, taskName, inputs, outputs, gr) + rtr, err := ResolveTaskResources(&taskSpec, taskName, inputs, outputs, gr) if err != nil { t.Fatalf("Did not expect error trying to resolve TaskRun: %s", err) } @@ -166,14 +166,14 @@ func TestResolveTaskRun_missingInput(t *testing.T) { } func TestResolveTaskRun_noResources(t *testing.T) { - taskSpec := &v1alpha1.TaskSpec{ + taskSpec := v1alpha1.TaskSpec{ Steps: []corev1.Container{{ Name: "step1", }}} gr := func(n string) (*v1alpha1.PipelineResource, error) { return &v1alpha1.PipelineResource{}, nil } - rtr, err := ResolveTaskResources(taskSpec, "orchestrate", []v1alpha1.TaskResourceBinding{}, []v1alpha1.TaskResourceBinding{}, gr) + rtr, err := ResolveTaskResources(&taskSpec, "orchestrate", []v1alpha1.TaskResourceBinding{}, []v1alpha1.TaskResourceBinding{}, gr) if err != nil { t.Fatalf("Did not expect error trying to resolve TaskRun: %s", err) } diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go index c673468c786..fb64bd77684 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go @@ -79,25 +79,22 @@ var ( saTask = tb.Task("test-with-sa", "foo", tb.TaskSpec(tb.Step("sa-step", "foo", tb.Command("/mycmd")))) templatedTask = tb.Task("test-task-with-templating", "foo", tb.TaskSpec( - tb.TaskInputs(tb.InputsResource("workspace", v1alpha1.PipelineResourceTypeGit)), + tb.TaskInputs( + tb.InputsResource("workspace", v1alpha1.PipelineResourceTypeGit), + tb.InputsParam("myarg"), tb.InputsParam("myarghasdefault", tb.ParamDefault("dont see me")), + tb.InputsParam("myarghasdefault2", tb.ParamDefault("thedefault")), + ), tb.TaskOutputs(tb.OutputsResource("myimage", v1alpha1.PipelineResourceTypeImage)), tb.Step("mycontainer", "myimage", tb.Command("/mycmd"), tb.Args( "--my-arg=${inputs.params.myarg}", + "--my-arg-with-default=${inputs.params.myarghasdefault}", + "--my-arg-with-default2=${inputs.params.myarghasdefault2}", "--my-additional-arg=${outputs.resources.myimage.url}", )), tb.Step("myothercontainer", "myotherimage", tb.Command("/mycmd"), tb.Args( "--my-other-arg=${inputs.resources.workspace.url}", )), )) - defaultTemplatedTask = tb.Task("test-task-with-default-templating", "foo", tb.TaskSpec( - tb.TaskInputs(tb.InputsParam("myarg", tb.ParamDefault("mydefault"))), - tb.Step("mycontainer", "myimage", tb.Command("/mycmd"), tb.Args( - "--my-arg=${inputs.params.myarg}", - )), - tb.Step("myothercontainer", "myotherimage", tb.Command("/mycmd"), tb.Args( - "--my-other-arg=${inputs.resources.git-resource.url}", - )), - )) gitResource = tb.PipelineResource("git-resource", "foo", tb.PipelineResourceSpec( v1alpha1.PipelineResourceTypeGit, tb.PipelineResourceSpecParam("URL", "https://foo.git"), @@ -185,23 +182,11 @@ func TestReconcile(t *testing.T) { tb.TaskRunTaskRef(templatedTask.Name, tb.TaskRefAPIVersion("a1")), tb.TaskRunInputs( tb.TaskRunInputsParam("myarg", "foo"), + tb.TaskRunInputsParam("myarghasdefault", "bar"), tb.TaskRunInputsResource("workspace", tb.ResourceBindingRef(gitResource.Name)), ), tb.TaskRunOutputs(tb.TaskRunOutputsResource("myimage", tb.ResourceBindingRef("image-resource"))), )) - taskRunOverrivdesDefaultTemplating := tb.TaskRun("test-taskrun-overrides-default-templating", "foo", tb.TaskRunSpec( - tb.TaskRunTaskRef(defaultTemplatedTask.Name, tb.TaskRefAPIVersion("a1")), - tb.TaskRunInputs( - tb.TaskRunInputsParam("myarg", "foo"), - tb.TaskRunInputsResource(gitResource.Name), - ), - )) - taskRunDefaultTemplating := tb.TaskRun("test-taskrun-default-templating", "foo", tb.TaskRunSpec( - tb.TaskRunTaskRef(defaultTemplatedTask.Name, tb.TaskRefAPIVersion("a1")), - tb.TaskRunInputs( - tb.TaskRunInputsResource(gitResource.Name), - ), - )) taskRunInputOutput := tb.TaskRun("test-taskrun-input-output", "foo", tb.TaskRunOwnerReference("PipelineRun", "test"), tb.TaskRunSpec( @@ -241,13 +226,12 @@ func TestReconcile(t *testing.T) { ) taskruns := []*v1alpha1.TaskRun{ taskRunSuccess, taskRunWithSaSuccess, - taskRunTemplating, taskRunOverrivdesDefaultTemplating, taskRunDefaultTemplating, - taskRunInputOutput, taskRunWithTaskSpec, taskRunWithClusterTask, + taskRunTemplating, taskRunInputOutput, taskRunWithTaskSpec, taskRunWithClusterTask, } d := test.Data{ TaskRuns: taskruns, - Tasks: []*v1alpha1.Task{simpleTask, saTask, templatedTask, defaultTemplatedTask, outputTask}, + Tasks: []*v1alpha1.Task{simpleTask, saTask, templatedTask, outputTask}, ClusterTasks: []*v1alpha1.ClusterTask{clustertask}, PipelineResources: []*v1alpha1.PipelineResource{gitResource, anotherGitResource, imageResource}, } @@ -283,7 +267,7 @@ func TestReconcile(t *testing.T) { tb.BuildSource("git-resource", tb.BuildSourceGit("https://foo.git", "master")), entrypointCopyStep, tb.BuildStep("mycontainer", "myimage", tb.Command(entrypointLocation), - tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-arg=foo","--my-additional-arg=gcr.io/kristoff/sven"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`), + tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-arg=foo","--my-arg-with-default=bar","--my-arg-with-default2=thedefault","--my-additional-arg=gcr.io/kristoff/sven"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`), tb.VolumeMount(toolsMount), ), tb.BuildStep("myothercontainer", "myotherimage", tb.Command(entrypointLocation), @@ -292,36 +276,6 @@ func TestReconcile(t *testing.T) { ), tb.BuildVolume(getToolsVolume(taskRunTemplating.Name)), ), - }, { - name: "input-overrides-default-params", - taskRun: taskRunOverrivdesDefaultTemplating, - wantBuildSpec: tb.BuildSpec( - entrypointCopyStep, - tb.BuildStep("mycontainer", "myimage", tb.Command(entrypointLocation), - tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-arg=foo"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`), - tb.VolumeMount(toolsMount), - ), - tb.BuildStep("myothercontainer", "myotherimage", tb.Command(entrypointLocation), - tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-other-arg=https://foo.git"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`), - tb.VolumeMount(toolsMount), - ), - tb.BuildVolume(getToolsVolume(taskRunOverrivdesDefaultTemplating.Name)), - ), - }, { - name: "default-params", - taskRun: taskRunDefaultTemplating, - wantBuildSpec: tb.BuildSpec( - entrypointCopyStep, - tb.BuildStep("mycontainer", "myimage", tb.Command(entrypointLocation), - tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-arg=mydefault"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`), - tb.VolumeMount(toolsMount), - ), - tb.BuildStep("myothercontainer", "myotherimage", tb.Command(entrypointLocation), - tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-other-arg=https://foo.git"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`), - tb.VolumeMount(toolsMount), - ), - tb.BuildVolume(getToolsVolume(taskRunDefaultTemplating.Name)), - ), }, { name: "wrap-steps", taskRun: taskRunInputOutput, @@ -397,15 +351,23 @@ func TestReconcile(t *testing.T) { if err != nil { t.Errorf("Invalid resource key: %v", err) } + tr, err := clients.Pipeline.PipelineV1alpha1().TaskRuns(namespace).Get(name, metav1.GetOptions{}) if err != nil { t.Fatalf("getting updated taskrun: %v", err) } + condition := tr.Status.GetCondition(duckv1alpha1.ConditionSucceeded) + if condition == nil || condition.Status != corev1.ConditionUnknown { + t.Errorf("Expected invalid TaskRun to have in progress status, but had %v", condition) + } + if condition != nil && condition.Reason != reasonRunning { + t.Errorf("Expected reason %q but was %s", reasonRunning, condition.Reason) + } + if tr.Status.PodName == "" { t.Fatalf("Reconcile didn't set pod name") } - // check error pod, err := clients.Kube.CoreV1().Pods(tr.Namespace).Get(tr.Status.PodName, metav1.GetOptions{}) if err != nil { t.Fatalf("Failed to fetch build pod: %v", err) @@ -420,26 +382,15 @@ func TestReconcile(t *testing.T) { if d := cmp.Diff(pod.Spec, wantPod.Spec); d != "" { t.Errorf("pod spec doesn't match, diff: %s", d) } - - // This TaskRun is in progress now and the status should reflect that. - condition := tr.Status.GetCondition(duckv1alpha1.ConditionSucceeded) - if condition == nil || condition.Status != corev1.ConditionUnknown { - t.Errorf("Expected invalid TaskRun to have in progress status, but had %v", condition) - } - if condition != nil && condition.Reason != reasonRunning { - t.Errorf("Expected reason %q but was %s", reasonRunning, condition.Reason) - } - if len(clients.Kube.Actions()) == 0 { t.Fatalf("Expected actions to be logged in the kubeclient, got none") } - // 3. check that PVC was created + pvc, err := clients.Kube.CoreV1().PersistentVolumeClaims(namespace).Get(name, metav1.GetOptions{}) if err != nil { t.Errorf("Failed to fetch build: %v", err) } - // get related TaskRun to populate expected PVC expectedVolume := getExpectedPVC(tr) if d := cmp.Diff(pvc.Name, expectedVolume.Name); d != "" { t.Errorf("pvc doesn't match, diff: %s", d) diff --git a/pkg/reconciler/v1alpha1/taskrun/validate.go b/pkg/reconciler/v1alpha1/taskrun/validate.go deleted file mode 100644 index 4a3d20cb982..00000000000 --- a/pkg/reconciler/v1alpha1/taskrun/validate.go +++ /dev/null @@ -1,70 +0,0 @@ -/* -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 extress or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package taskrun - -import ( - "fmt" - - "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" - "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun/resources" -) - -// ValidateResolvedTaskResources validates task inputs, params and output matches taskrun -func ValidateResolvedTaskResources(params []v1alpha1.Param, rtr *resources.ResolvedTaskResources) error { - // stores params to validate with task params - paramsMapping := map[string]string{} - - for _, param := range params { - paramsMapping[param.Name] = "" - } - - if rtr.TaskSpec != nil { - if rtr.TaskSpec.Inputs != nil { - for _, inputResource := range rtr.TaskSpec.Inputs.Resources { - r, ok := rtr.Inputs[inputResource.Name] - if !ok { - return fmt.Errorf("input resource %q not provided for task %q", inputResource.Name, rtr.TaskName) - } - // Validate the type of resource match - if inputResource.Type != r.Spec.Type { - return fmt.Errorf("input resource %q for task %q should be type %q but was %q", inputResource.Name, rtr.TaskName, r.Spec.Type, inputResource.Type) - } - } - for _, inputResourceParam := range rtr.TaskSpec.Inputs.Params { - if _, ok := paramsMapping[inputResourceParam.Name]; !ok { - if inputResourceParam.Default == "" { - return fmt.Errorf("input param %q not provided for task %q", inputResourceParam.Name, rtr.TaskName) - } - } - } - } - - if rtr.TaskSpec.Outputs != nil { - for _, outputResource := range rtr.TaskSpec.Outputs.Resources { - r, ok := rtr.Outputs[outputResource.Name] - if !ok { - return fmt.Errorf("output resource %q not provided for task %q", outputResource.Name, rtr.TaskName) - } - // Validate the type of resource match - if outputResource.Type != r.Spec.Type { - return fmt.Errorf("output resource %q for task %q should be type %q but was %q", outputResource.Name, rtr.TaskName, r.Spec.Type, outputResource.Type) - } - } - } - } - return nil -} diff --git a/pkg/reconciler/v1alpha1/taskrun/validate_resources.go b/pkg/reconciler/v1alpha1/taskrun/validate_resources.go new file mode 100644 index 00000000000..1203d9011da --- /dev/null +++ b/pkg/reconciler/v1alpha1/taskrun/validate_resources.go @@ -0,0 +1,111 @@ +/* +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 extress or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package taskrun + +import ( + "fmt" + + "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" + "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun/list" + "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun/resources" +) + +func validateInputResources(inputs *v1alpha1.Inputs, providedResources map[string]*v1alpha1.PipelineResource) error { + if inputs != nil { + return validateResources(inputs.Resources, providedResources) + } + return validateResources([]v1alpha1.TaskResource{}, providedResources) +} + +func validateOutputResources(outputs *v1alpha1.Outputs, providedResources map[string]*v1alpha1.PipelineResource) error { + if outputs != nil { + return validateResources(outputs.Resources, providedResources) + } + return validateResources([]v1alpha1.TaskResource{}, providedResources) +} + +func validateResources(neededResources []v1alpha1.TaskResource, providedResources map[string]*v1alpha1.PipelineResource) error { + needed := make([]string, 0, len(neededResources)) + for _, resource := range neededResources { + needed = append(needed, resource.Name) + } + provided := make([]string, 0, len(providedResources)) + for resource := range providedResources { + provided = append(provided, resource) + } + missing := list.DiffLeft(needed, provided) + if len(missing) > 0 { + return fmt.Errorf("missing resources: %s", missing) + } + extra := list.DiffLeft(provided, needed) + if len(extra) > 0 { + return fmt.Errorf("didn't need these resources by they were provided anyway: %s", extra) + } + for _, resource := range neededResources { + r := providedResources[resource.Name] + if resource.Type != r.Spec.Type { + return fmt.Errorf("resource %q should be type %q but was %q", resource.Name, r.Spec.Type, resource.Type) + } + } + return nil +} + +func validateParams(inputs *v1alpha1.Inputs, params []v1alpha1.Param) error { + neededParams := []string{} + if inputs != nil { + neededParams = make([]string, 0, len(inputs.Params)) + for _, inputResourceParam := range inputs.Params { + neededParams = append(neededParams, inputResourceParam.Name) + } + } + providedParams := make([]string, 0, len(params)) + for _, param := range params { + providedParams = append(providedParams, param.Name) + } + missingParams := list.DiffLeft(neededParams, providedParams) + missingParamsNoDefaults := []string{} + for _, param := range missingParams { + for _, inputResourceParam := range inputs.Params { + if inputResourceParam.Name == param && inputResourceParam.Default == "" { + missingParamsNoDefaults = append(missingParamsNoDefaults, param) + } + } + } + if len(missingParamsNoDefaults) > 0 { + return fmt.Errorf("missing values for these params which have no default values: %s", missingParamsNoDefaults) + } + extraParams := list.DiffLeft(providedParams, neededParams) + if len(extraParams) != 0 { + return fmt.Errorf("didn't need these params but they were provided anyway: %s", extraParams) + } + return nil +} + +// ValidateResolvedTaskResources validates task inputs, params and output matches taskrun +func ValidateResolvedTaskResources(params []v1alpha1.Param, rtr *resources.ResolvedTaskResources) error { + if err := validateParams(rtr.TaskSpec.Inputs, params); err != nil { + return fmt.Errorf("invalid input params: %v", err) + } + if err := validateInputResources(rtr.TaskSpec.Inputs, rtr.Inputs); err != nil { + return fmt.Errorf("invalid input resources: %v", err) + } + if err := validateOutputResources(rtr.TaskSpec.Outputs, rtr.Outputs); err != nil { + return fmt.Errorf("invalid output resources: %v", err) + } + + return nil +} diff --git a/pkg/reconciler/v1alpha1/taskrun/validate_resources_test.go b/pkg/reconciler/v1alpha1/taskrun/validate_resources_test.go new file mode 100644 index 00000000000..139ced9be8d --- /dev/null +++ b/pkg/reconciler/v1alpha1/taskrun/validate_resources_test.go @@ -0,0 +1,161 @@ +package taskrun_test + +import ( + "testing" + + "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" + "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun" + "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun/resources" + tb "github.com/knative/build-pipeline/test/builder" + corev1 "k8s.io/api/core/v1" +) + +var ( + validBuildSteps = []corev1.Container{{ + Name: "mystep", + Image: "myimage", + Command: []string{"mycmd"}, + }} + validBuildStepFn = tb.Step("mystep", "myimage", tb.Command("mycmd")) +) + +func TestValidateResolvedTaskResources_ValidResources(t *testing.T) { + rtr := tb.ResolvedTaskResources( + tb.ResolvedTaskResourcesTaskSpec( + validBuildStepFn, + tb.TaskInputs(tb.InputsResource("resource-to-build", v1alpha1.PipelineResourceTypeGit)), + tb.TaskOutputs(tb.OutputsResource("resource-to-provide", v1alpha1.PipelineResourceTypeImage)), + ), + tb.ResolvedTaskResourcesInputs("resource-to-build", tb.PipelineResource("example-resource", "foo", + tb.PipelineResourceSpec(v1alpha1.PipelineResourceTypeGit, + tb.PipelineResourceSpecParam("foo", "bar"), + ))), + tb.ResolvedTaskResourcesOutputs("resource-to-provide", tb.PipelineResource("example-image", "bar", + tb.PipelineResourceSpec(v1alpha1.PipelineResourceTypeImage)), + )) + if err := taskrun.ValidateResolvedTaskResources([]v1alpha1.Param{}, rtr); err != nil { + t.Fatalf("Did not expect to see error when validating valid resolved TaskRun but saw %v", err) + } +} + +func TestValidateResolvedTaskResources_ValidParams(t *testing.T) { + rtr := tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( + validBuildStepFn, + tb.TaskInputs(tb.InputsParam("foo"), tb.InputsParam("bar")), + )) + p := []v1alpha1.Param{{ + Name: "foo", + Value: "somethinggood", + }, { + Name: "bar", + Value: "somethinggood", + }} + if err := taskrun.ValidateResolvedTaskResources(p, rtr); err != nil { + t.Fatalf("Did not expect to see error when validating TaskRun with correct params but saw %v", err) + } +} + +func TestValidateResolvedTaskResources_InvalidParams(t *testing.T) { + tcs := []struct { + name string + rtr *resources.ResolvedTaskResources + params []v1alpha1.Param + }{{ + name: "missing-params", + rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( + validBuildStepFn, + tb.TaskInputs(tb.InputsParam("foo")), + )), + params: []v1alpha1.Param{{ + Name: "foobar", + Value: "somethingfun", + }}, + }, { + name: "missing-params", + rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( + validBuildStepFn, + tb.TaskInputs(tb.InputsParam("foo")), + )), + params: []v1alpha1.Param{{ + Name: "foo", + Value: "i am a real param", + }, { + Name: "extra", + Value: "i am an extra param", + }}, + }} + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + if err := taskrun.ValidateResolvedTaskResources(tc.params, tc.rtr); err == nil { + t.Errorf("Expected to see error when validating invalid resolved TaskRun with wrong params but saw none") + } + }) + } +} + +func TestValidateResolvedTaskResources_InvalidResources(t *testing.T) { + r := tb.PipelineResource("git-test-resource", "foo", tb.PipelineResourceSpec( + v1alpha1.PipelineResourceTypeGit, + tb.PipelineResourceSpecParam("foo", "bar"), + )) + tcs := []struct { + name string + rtr *resources.ResolvedTaskResources + }{{ + name: "bad-inputkey", + rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( + tb.TaskInputs(tb.InputsResource("testinput", v1alpha1.PipelineResourceTypeGit)), + ), tb.ResolvedTaskResourcesInputs("wrong-resource-name", r)), + }, { + name: "bad-outputkey", + rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( + tb.TaskOutputs(tb.OutputsResource("testoutput", v1alpha1.PipelineResourceTypeGit)), + ), tb.ResolvedTaskResourcesOutputs("wrong-resource-name", r)), + }, { + name: "input-resource-mismatch", + rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( + tb.TaskInputs(tb.InputsResource("testimageinput", v1alpha1.PipelineResourceTypeImage)), + ), tb.ResolvedTaskResourcesInputs("testimageinput", r)), + }, { + name: "output-resource-mismatch", + rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( + tb.TaskOutputs(tb.OutputsResource("testimageoutput", v1alpha1.PipelineResourceTypeImage)), + ), tb.ResolvedTaskResourcesOutputs("testimageoutput", r)), + }, { + name: "extra-input-resource", + rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( + tb.TaskInputs(tb.InputsResource("testoutput", v1alpha1.PipelineResourceTypeGit))), + tb.ResolvedTaskResourcesInputs("testoutput", r), + tb.ResolvedTaskResourcesInputs("someextrainput", r), + ), + }, { + name: "extra-output-resource", + rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( + tb.TaskOutputs(tb.OutputsResource("testoutput", v1alpha1.PipelineResourceTypeGit))), + tb.ResolvedTaskResourcesOutputs("testoutput", r), + tb.ResolvedTaskResourcesOutputs("someextraoutput", r), + ), + }, { + name: "extra-input-resource-none-required", + rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( + tb.TaskOutputs(tb.OutputsResource("testoutput", v1alpha1.PipelineResourceTypeGit))), + tb.ResolvedTaskResourcesOutputs("testoutput", r), + tb.ResolvedTaskResourcesInputs("someextrainput", r), + ), + }, { + name: "extra-output-resource-none-required", + rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( + tb.TaskInputs(tb.InputsResource("testinput", v1alpha1.PipelineResourceTypeGit))), + tb.ResolvedTaskResourcesInputs("testinput", r), + tb.ResolvedTaskResourcesOutputs("someextraoutput", r), + ), + }} + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + if err := taskrun.ValidateResolvedTaskResources([]v1alpha1.Param{}, tc.rtr); err == nil { + t.Errorf("Expected to see error when validating invalid resolved TaskRun but saw none") + } + }) + } +} diff --git a/pkg/reconciler/v1alpha1/taskrun/validate_test.go b/pkg/reconciler/v1alpha1/taskrun/validate_test.go deleted file mode 100644 index f0a4769417f..00000000000 --- a/pkg/reconciler/v1alpha1/taskrun/validate_test.go +++ /dev/null @@ -1,104 +0,0 @@ -package taskrun_test - -import ( - "testing" - - "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" - "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun" - "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun/resources" - tb "github.com/knative/build-pipeline/test/builder" - corev1 "k8s.io/api/core/v1" -) - -var ( - validBuildSteps = []corev1.Container{{ - Name: "mystep", - Image: "myimage", - Command: []string{"mycmd"}, - }} - validBuildStepFn = tb.Step("mystep", "myimage", tb.Command("mycmd")) -) - -func TestValidateResolvedTaskResources_ValidResources(t *testing.T) { - rtr := tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( - validBuildStepFn, - tb.TaskInputs(tb.InputsResource("resource-to-build", v1alpha1.PipelineResourceTypeGit)), - ), tb.ResolvedTaskResourcesInputs("resource-to-build", tb.PipelineResource("example-resource", "foo", - tb.PipelineResourceSpec(v1alpha1.PipelineResourceTypeGit, - tb.PipelineResourceSpecParam("foo", "bar"), - )), - )) - if err := taskrun.ValidateResolvedTaskResources([]v1alpha1.Param{}, rtr); err != nil { - t.Fatalf("Did not expect to see error when validating valid resolved TaskRun but saw %v", err) - } -} - -func TestValidateResolvedTaskResources_ValidParams(t *testing.T) { - rtr := tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( - validBuildStepFn, - tb.TaskInputs(tb.InputsParam("foo"), tb.InputsParam("bar")), - )) - p := []v1alpha1.Param{{ - Name: "foo", - Value: "somethinggood", - }, { - Name: "bar", - Value: "somethinggood", - }} - if err := taskrun.ValidateResolvedTaskResources(p, rtr); err != nil { - t.Fatalf("Did not expect to see error when validating TaskRun with correct params but saw %v", err) - } -} - -func TestValidateResolvedTaskResources_InvalidParams(t *testing.T) { - rtr := tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( - validBuildStepFn, - tb.TaskInputs(tb.InputsParam("foo"), tb.InputsParam("bar")), - )) - p := []v1alpha1.Param{{ - Name: "foobar", - Value: "somethingfun", - }} - if err := taskrun.ValidateResolvedTaskResources(p, rtr); err == nil { - t.Errorf("Expected to see error when validating invalid resolved TaskRun with wrong params but saw none") - } -} - -func TestValidateResolvedTaskResources_InvalidResources(t *testing.T) { - r := tb.PipelineResource("git-test-resource", "foo", tb.PipelineResourceSpec( - v1alpha1.PipelineResourceTypeGit, - tb.PipelineResourceSpecParam("foo", "bar"), - )) - tcs := []struct { - name string - rtr *resources.ResolvedTaskResources - }{{ - name: "bad-inputkey", - rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( - tb.TaskInputs(tb.InputsResource("testinput", v1alpha1.PipelineResourceTypeGit)), - ), tb.ResolvedTaskResourcesInputs("wrong-resource-name", r)), - }, { - name: "bad-outputkey", - rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( - tb.TaskOutputs(tb.OutputsResource("testoutput", v1alpha1.PipelineResourceTypeGit)), - ), tb.ResolvedTaskResourcesInputs("wrong-resource-name", r)), - }, { - name: "input-resource-mismatch", - rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( - tb.TaskInputs(tb.InputsResource("testimageinput", v1alpha1.PipelineResourceTypeImage)), - ), tb.ResolvedTaskResourcesInputs("testimageinput", r)), - }, { - name: "output-resource-mismatch", - rtr: tb.ResolvedTaskResources(tb.ResolvedTaskResourcesTaskSpec( - tb.TaskOutputs(tb.OutputsResource("testimageoutput", v1alpha1.PipelineResourceTypeImage)), - ), tb.ResolvedTaskResourcesInputs("testimageoutput", r)), - }} - - for _, tc := range tcs { - t.Run(tc.name, func(t *testing.T) { - if err := taskrun.ValidateResolvedTaskResources([]v1alpha1.Param{}, tc.rtr); err == nil { - t.Errorf("Expected to see error when validating invalid resolved TaskRun but saw none") - } - }) - } -} diff --git a/test/builder/task.go b/test/builder/task.go index ce3049f7a14..fb804f8f32d 100644 --- a/test/builder/task.go +++ b/test/builder/task.go @@ -494,7 +494,7 @@ func ResolvedTaskResourcesTaskSpec(ops ...TaskSpecOp) ResolvedTaskResourcesOp { } } -// ResolvedTaskResourcesInputs adds a PipelineResource, with specified name, to the ResolvedTaskResources. +// ResolvedTaskResourcesInputs adds an input PipelineResource, with specified name, to the ResolvedTaskResources. func ResolvedTaskResourcesInputs(name string, resource *v1alpha1.PipelineResource) ResolvedTaskResourcesOp { return func(r *resources.ResolvedTaskResources) { if r.Inputs == nil { @@ -503,3 +503,13 @@ func ResolvedTaskResourcesInputs(name string, resource *v1alpha1.PipelineResourc r.Inputs[name] = resource } } + +// ResolvedTaskResourcesOutputs adds an output PipelineResource, with specified name, to the ResolvedTaskResources. +func ResolvedTaskResourcesOutputs(name string, resource *v1alpha1.PipelineResource) ResolvedTaskResourcesOp { + return func(r *resources.ResolvedTaskResources) { + if r.Outputs == nil { + r.Outputs = map[string]*v1alpha1.PipelineResource{} + } + r.Outputs[name] = resource + } +} diff --git a/test/builder/task_test.go b/test/builder/task_test.go index b0a051cac30..c0c7b63ea8b 100644 --- a/test/builder/task_test.go +++ b/test/builder/task_test.go @@ -230,6 +230,7 @@ func TestResolvedTaskResources(t *testing.T) { tb.Step("step", "image", tb.Command("/mycmd")), ), tb.ResolvedTaskResourcesInputs("foo", tb.PipelineResource("bar", "baz")), + tb.ResolvedTaskResourcesOutputs("qux", tb.PipelineResource("quux", "quuz")), ) expectedResolvedTaskResources := &resources.ResolvedTaskResources{ TaskSpec: &v1alpha1.TaskSpec{ @@ -247,6 +248,14 @@ func TestResolvedTaskResources(t *testing.T) { }, }, }, + Outputs: map[string]*v1alpha1.PipelineResource{ + "qux": &v1alpha1.PipelineResource{ + ObjectMeta: metav1.ObjectMeta{ + Name: "quux", + Namespace: "quuz", + }, + }, + }, } if d := cmp.Diff(expectedResolvedTaskResources, resolvedTaskResources); d != "" { t.Fatalf("ResolvedTaskResources diff -want, +got: %v", d)