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

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")
 					}
 				}
 			}

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,22 +62,130 @@ 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"},
+						}},
+					},
 				}},
 			},
 		},
 	}
 	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,
 			}

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 {

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 {

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"}

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]