From 131d02792a961c067103ad285a914844dc56bf15 Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Mon, 21 Jan 2019 15:52:58 -0800 Subject: [PATCH] Add validation for declared resources in Pipeline Now that we're declaring Resources, we can do some extra validation on creation that used to need to wait until only runtime. Also generalized the logic for looking for missing and extra values - though this will probably make the error messages a bit less helpful. Part of #320 --- .../pipeline/v1alpha1/pipeline_validation.go | 48 +++++- .../v1alpha1/pipeline_validation_test.go | 138 +++++++++++++++++- .../resources/pipelinerunresolution.go | 11 +- pkg/reconciler/v1alpha1/taskrun/list/diff.go | 17 +++ .../v1alpha1/taskrun/list/diff_test.go | 27 ++++ .../v1alpha1/taskrun/validate_resources.go | 10 +- 6 files changed, 233 insertions(+), 18 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go index 463a4eb03ad..63ed6765c9e 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go @@ -19,6 +19,7 @@ package v1alpha1 import ( "fmt" + "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun/list" "github.com/knative/pkg/apis" "k8s.io/apimachinery/pkg/api/equality" ) @@ -32,6 +33,39 @@ func (p *Pipeline) Validate() *apis.FieldError { return nil } +func isOutput(task PipelineTask, resource string) bool { + for _, output := range task.Resources.Outputs { + if output.Resource == resource { + return true + } + } + return false +} + +func validateDeclaredResources(ps *PipelineSpec) error { + needed := []string{} + for _, t := range ps.Tasks { + if t.Resources != nil { + for _, input := range t.Resources.Inputs { + needed = append(needed, input.Resource) + } + for _, output := range t.Resources.Outputs { + needed = append(needed, output.Resource) + } + } + } + + provided := make([]string, 0, len(ps.Resources)) + for _, resource := range ps.Resources { + provided = append(provided, resource.Name) + } + err := list.IsSame(needed, provided) + if err != nil { + return fmt.Errorf("Pipeline declared resources didn't match usage in Tasks: %s", err) + } + return nil +} + // Validate checks that taskNames in the Pipeline are valid and that the graph // of Tasks expressed in the Pipeline makes sense. func (ps *PipelineSpec) Validate() *apis.FieldError { @@ -48,6 +82,12 @@ func (ps *PipelineSpec) Validate() *apis.FieldError { taskNames[t.Name] = struct{}{} } + // All declared resources should be used, and the Pipeline shouldn't try to use any resources + // that aren't declared + if err := validateDeclaredResources(ps); err != nil { + return apis.ErrInvalidValue(err.Error(), "spec.resources") + } + // providedBy should match future tasks // TODO(#168) when pipelines don't just execute linearly this will need to be more sophisticated for i, t := range ps.Tasks { @@ -56,17 +96,21 @@ func (ps *PipelineSpec) Validate() *apis.FieldError { for _, pb := range rd.ProvidedBy { if i == 0 { // First Task can't depend on anything before it (b/c there is nothing) - return apis.ErrInvalidKeyName(pb, fmt.Sprintf("spec.tasks.resources.%s", pb)) + return apis.ErrInvalidValue(pb, "spec.tasks.resources.inputs.providedBy") } found := false // Look for previous Task that satisfies constraint for j := i - 1; j >= 0; j-- { if ps.Tasks[j].Name == pb { + // The input resource must actually be an output of the providedBy tasks + if !isOutput(ps.Tasks[j], rd.Resource) { + return apis.ErrInvalidKeyName(pb, "spec.tasks.resources.inputs.providedBy") + } found = true } } if !found { - return apis.ErrInvalidKeyName(pb, fmt.Sprintf("spec.tasks.resources.%s", pb)) + return apis.ErrInvalidKeyName(pb, "spec.tasks.resources.inputs.providedBy") } } } diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go index 200e39de648..f793865e3f8 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go @@ -22,6 +22,7 @@ import ( func TestPipelineSpec_Validate_Error(t *testing.T) { type fields struct { + Resources []PipelineDeclaredResource Tasks []PipelineTask Generation int64 } @@ -42,10 +43,16 @@ func TestPipelineSpec_Validate_Error(t *testing.T) { { name: "providedby task doesnt exist", fields: fields{ + Resources: []PipelineDeclaredResource{{ + Name: "great-resource", + Type: "git", + }}, Tasks: []PipelineTask{{ Name: "foo", Resources: &PipelineTaskResources{ Inputs: []PipelineTaskInputResource{{ + Name: "the-resource", + Resource: "great-resource", ProvidedBy: []string{"bar"}, }}, }, @@ -55,15 +62,122 @@ func TestPipelineSpec_Validate_Error(t *testing.T) { { name: "providedby task is afterward", fields: fields{ + Resources: []PipelineDeclaredResource{{ + Name: "great-resource", + Type: "git", + }}, Tasks: []PipelineTask{{ Name: "foo", Resources: &PipelineTaskResources{ Inputs: []PipelineTaskInputResource{{ + Name: "the-resource", + Resource: "great-resource", ProvidedBy: []string{"bar"}, }}, }, }, { Name: "bar", + Resources: &PipelineTaskResources{ + Outputs: []PipelineTaskOutputResource{{ + Name: "the-resource", + Resource: "great-resource", + }}, + }, + }}, + }, + }, + { + name: "unused resources declared", + fields: fields{ + Resources: []PipelineDeclaredResource{{ + Name: "great-resource", + Type: "git", + }, { + Name: "extra-resource", + Type: "image", + }}, + Tasks: []PipelineTask{{ + Name: "foo", + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "the-resource", + Resource: "great-resource", + }}, + }, + }}, + }, + }, + { + name: "output resources missing from declaration", + fields: fields{ + Resources: []PipelineDeclaredResource{{ + Name: "great-resource", + Type: "git", + }}, + Tasks: []PipelineTask{{ + Name: "foo", + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "the-resource", + Resource: "great-resource", + }}, + Outputs: []PipelineTaskOutputResource{{ + Name: "the-magic-resource", + Resource: "missing-resource", + }}, + }, + }}, + }, + }, + { + name: "input resources missing from declaration", + fields: fields{ + Resources: []PipelineDeclaredResource{{ + Name: "great-resource", + Type: "git", + }}, + Tasks: []PipelineTask{{ + Name: "foo", + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "the-resource", + Resource: "missing-resource", + }}, + Outputs: []PipelineTaskOutputResource{{ + Name: "the-magic-resource", + Resource: "great-resource", + }}, + }, + }}, + }, + }, + { + name: "providedBy resource isn't output by task", + fields: fields{ + Resources: []PipelineDeclaredResource{{ + Name: "great-resource", + Type: "git", + }, { + Name: "wonderful-resource", + Type: "image", + }}, + Tasks: []PipelineTask{{ + Name: "bar", + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "some-workspace", + Resource: "great-resource", + }}, + }, + }, { + Name: "foo", + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "wow-image", + Resource: "wonderful-resource", + ProvidedBy: []string{"bar"}, + }}, + }, }}, }, }, @@ -71,6 +185,7 @@ func TestPipelineSpec_Validate_Error(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ps := &PipelineSpec{ + Resources: tt.fields.Resources, Tasks: tt.fields.Tasks, Generation: tt.fields.Generation, } @@ -83,6 +198,7 @@ func TestPipelineSpec_Validate_Error(t *testing.T) { func TestPipelineSpec_Validate_Valid(t *testing.T) { type fields struct { + Resources []PipelineDeclaredResource Tasks []PipelineTask Generation int64 } @@ -101,14 +217,33 @@ func TestPipelineSpec_Validate_Valid(t *testing.T) { }, }, { - name: "valid constraint tasks", + name: "valid resource declarations and usage", fields: fields{ + Resources: []PipelineDeclaredResource{{ + Name: "great-resource", + Type: "git", + }, { + Name: "wonderful-resource", + Type: "image", + }}, Tasks: []PipelineTask{{ Name: "bar", + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "some-workspace", + Resource: "great-resource", + }}, + Outputs: []PipelineTaskOutputResource{{ + Name: "some-image", + Resource: "wonderful-resource", + }}, + }, }, { Name: "foo", Resources: &PipelineTaskResources{ Inputs: []PipelineTaskInputResource{{ + Name: "wow-image", + Resource: "wonderful-resource", ProvidedBy: []string{"bar"}, }}, }, @@ -119,6 +254,7 @@ func TestPipelineSpec_Validate_Valid(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ps := &PipelineSpec{ + Resources: tt.fields.Resources, Tasks: tt.fields.Tasks, Generation: tt.fields.Generation, } diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go index f5a8a9fea19..3fd15c28d7e 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go @@ -87,7 +87,6 @@ type GetTaskRun func(name string) (*v1alpha1.TaskRun, error) func GetResourcesFromBindings(p *v1alpha1.Pipeline, pr *v1alpha1.PipelineRun) (map[string]v1alpha1.PipelineResourceRef, error) { resources := map[string]v1alpha1.PipelineResourceRef{} - // TODO: this is very similar to logic in validateResources - use the same logic? needed := make([]string, 0, len(p.Spec.Resources)) for _, resource := range p.Spec.Resources { needed = append(needed, resource.Name) @@ -96,13 +95,9 @@ func GetResourcesFromBindings(p *v1alpha1.Pipeline, pr *v1alpha1.PipelineRun) (m for _, resource := range pr.Spec.Resources { provided = append(provided, resource.Name) } - missing := list.DiffLeft(needed, provided) - if len(missing) > 0 { - return resources, fmt.Errorf("PipelineRun didn't bind Pipeline's declared resources: %s", missing) - } - extra := list.DiffLeft(provided, needed) - if len(extra) > 0 { - return resources, fmt.Errorf("Pipeline didn't declare these resources but they were bound in PipelineRun: %s", extra) + err := list.IsSame(needed, provided) + if err != nil { + return resources, fmt.Errorf("PipelineRun bound resources didn't match Pipeline: %s", err) } for _, resource := range pr.Spec.Resources { diff --git a/pkg/reconciler/v1alpha1/taskrun/list/diff.go b/pkg/reconciler/v1alpha1/taskrun/list/diff.go index 5ad5ea19d69..c87711e193b 100644 --- a/pkg/reconciler/v1alpha1/taskrun/list/diff.go +++ b/pkg/reconciler/v1alpha1/taskrun/list/diff.go @@ -16,6 +16,23 @@ limitations under the License. package list +import "fmt" + +// IsSame will return an error indicating if there are extra or missing strings +// between the needed and provided strings, or will return no error if the two +// contain the same values. +func IsSame(needed, provided []string) error { + missing := DiffLeft(needed, provided) + if len(missing) > 0 { + return fmt.Errorf("Didn't provide needed values: %s", missing) + } + extra := DiffLeft(provided, needed) + if len(extra) > 0 { + return fmt.Errorf("Provided extra values: %s", extra) + } + return nil +} + // DiffLeft will return all strings which are in the left slice of strings but // not in the right. func DiffLeft(left, right []string) []string { diff --git a/pkg/reconciler/v1alpha1/taskrun/list/diff_test.go b/pkg/reconciler/v1alpha1/taskrun/list/diff_test.go index 51adc259a80..208c4799dd6 100644 --- a/pkg/reconciler/v1alpha1/taskrun/list/diff_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/list/diff_test.go @@ -21,6 +21,33 @@ import ( "testing" ) +func TestIsSame_same(t *testing.T) { + needed := []string{"elsa", "anna", "olaf", "kristoff"} + provided := []string{"elsa", "anna", "olaf", "kristoff"} + err := IsSame(needed, provided) + if err != nil { + t.Errorf("Didn't expect error when everything needed has been provided") + } +} + +func TestIsSame_missing(t *testing.T) { + needed := []string{"elsa", "anna", "olaf", "kristoff"} + provided := []string{"elsa", "anna", "olaf"} + err := IsSame(needed, provided) + if err == nil { + t.Errorf("Expected error since `kristoff` should be missing") + } +} + +func TestIsSame_extra(t *testing.T) { + needed := []string{"elsa", "anna", "olaf"} + provided := []string{"elsa", "anna", "olaf", "kristoff"} + err := IsSame(needed, provided) + if err == nil { + t.Errorf("Expected error since `kristoff` should be extra") + } +} + func TestDiffLeft_same(t *testing.T) { left := []string{"elsa", "anna", "olaf", "kristoff"} right := []string{"elsa", "anna", "olaf", "kristoff"} diff --git a/pkg/reconciler/v1alpha1/taskrun/validate_resources.go b/pkg/reconciler/v1alpha1/taskrun/validate_resources.go index a353eb56313..7ef0326f419 100644 --- a/pkg/reconciler/v1alpha1/taskrun/validate_resources.go +++ b/pkg/reconciler/v1alpha1/taskrun/validate_resources.go @@ -47,13 +47,9 @@ func validateResources(neededResources []v1alpha1.TaskResource, providedResource 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 but they were provided anyway: %s", extra) + err := list.IsSame(needed, provided) + if err != nil { + return fmt.Errorf("TaskRun's declared resources didn't match usage in Task: %s", err) } for _, resource := range neededResources { r := providedResources[resource.Name]