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]