Add validtion when applying task modifiers 🧐

Task modifiers for PipelineResources add pre and post steps and
sometimes Volumes to a pod spec. Adding a step or a volume that already
exists will create a pod that cannot run so this commit adds validation:
- If a modifier tries to add a step that already exists (i.e. the name
  is the same) that's an error
- If a modifier tries to add a volume that already exists but the volume
  it references is the same, we assume that two resources need the same
  volume, so it's not an error, but we only add it wonce
- If a modifier tries to add a volume that already exists but the volume
  it references is different, that means two resources need different
  volumes but we cant add both since the names are the same, so that's
  an error

I encountered a problem with this while working on #1417 (which we
decided not to merge) where I tried to use two VolumeResources and there
was a bug where they both had the same name, so the pod was not
schedulable. I had to work backwards from the error that the pod was
invalid and thought it would be much more handy to get the error
earlier, so I added this.

pkg/apis/pipeline/v1alpha1/resource_types.go

@@ -17,6 +17,7 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"github.com/google/go-cmp/cmp"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
 	"golang.org/x/xerrors"
 	v1 "k8s.io/api/core/v1"
@@ -80,7 +81,7 @@ func (tm *InternalTaskModifier) GetStepsToPrepend() []Step {
 	return tm.StepsToPrepend
 }
 
-// GetStepsToPrepend returns a set of Steps to append to the Task.
+// GetStepsToAppend returns a set of Steps to append to the Task.
 func (tm *InternalTaskModifier) GetStepsToAppend() []Step {
 	return tm.StepsToAppend
 }
@@ -90,16 +91,55 @@ func (tm *InternalTaskModifier) GetVolumes() []v1.Volume {
 	return tm.Volumes
 }
 
+func checkStepNotAlreadyAdded(s Step, steps []Step) error {
+	for _, step := range steps {
+		if s.Name == step.Name {
+			return xerrors.Errorf("Step %s cannot be added again", step.Name)
+		}
+	}
+	return nil
+}
+
 // ApplyTaskModifier applies a modifier to the task by appending and prepending steps and volumes.
-func ApplyTaskModifier(ts *TaskSpec, tm TaskModifier) {
+// If steps with the same name exist in ts an error will be returned. If identical Volumes have
+// been added, they will not be added again. If Volumes with the same name but different contents
+// have been added, an error will be returned.
+func ApplyTaskModifier(ts *TaskSpec, tm TaskModifier) error {
 	steps := tm.GetStepsToPrepend()
+	for _, step := range steps {
+		if err := checkStepNotAlreadyAdded(step, ts.Steps); err != nil {
+			return err
+		}
+	}
 	ts.Steps = append(steps, ts.Steps...)
 
 	steps = tm.GetStepsToAppend()
+	for _, step := range steps {
+		if err := checkStepNotAlreadyAdded(step, ts.Steps); err != nil {
+			return err
+		}
+	}
 	ts.Steps = append(ts.Steps, steps...)
 
 	volumes := tm.GetVolumes()
-	ts.Volumes = append(ts.Volumes, volumes...)
+	for _, volume := range volumes {
+		var alreadyAdded bool
+		for _, v := range ts.Volumes {
+			if volume.Name == v.Name {
+				// If a Volume with the same name but different contents has already been added, we can't add both
+				if d := cmp.Diff(volume, v); d != "" {
+					return xerrors.Errorf("Tried to add volume %s already added but with different contents", volume.Name)
+				}
+				// If an identical Volume has already been added, don't add it again
+				alreadyAdded = true
+			}
+		}
+		if !alreadyAdded {
+			ts.Volumes = append(ts.Volumes, volume)
+		}
+	}
+
+	return nil
 }
 
 // SecretParam indicates which secret can be used to populate a field of the resource

pkg/apis/pipeline/v1alpha1/resource_types_test.go

@@ -0,0 +1,144 @@
+/*
+Copyright 2019 The Tekton 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 express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1alpha1_test
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
+	corev1 "k8s.io/api/core/v1"
+)
+
+var (
+	prependStep = corev1.Container{
+		Name:    "prepend-step",
+		Image:   "dummy",
+		Command: []string{"doit"},
+		Args:    []string{"stuff", "things"},
+	}
+	appendStep = corev1.Container{
+		Name:    "append-step",
+		Image:   "dummy",
+		Command: []string{"doit"},
+		Args:    []string{"other stuff", "other things"},
+	}
+	volume = corev1.Volume{
+		Name: "magic-volume",
+		VolumeSource: corev1.VolumeSource{
+			PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ClaimName: "some-claim"},
+		},
+	}
+)
+
+type TestTaskModifier struct{}
+
+func (tm *TestTaskModifier) GetStepsToPrepend() []v1alpha1.Step {
+	return []v1alpha1.Step{{
+		Container: prependStep,
+	}}
+}
+
+func (tm *TestTaskModifier) GetStepsToAppend() []v1alpha1.Step {
+	return []v1alpha1.Step{{
+		Container: appendStep,
+	}}
+}
+
+func (tm *TestTaskModifier) GetVolumes() []corev1.Volume {
+	return []corev1.Volume{volume}
+}
+
+func TestApplyTaskModifier(t *testing.T) {
+	testcases := []struct {
+		name string
+		ts   v1alpha1.TaskSpec
+	}{{
+		name: "success",
+		ts:   v1alpha1.TaskSpec{},
+	}, {
+		name: "identical volume already added",
+		ts: v1alpha1.TaskSpec{
+			// Trying to add the same Volume that has already been added shouldn't be an error
+			// and it should not be added twice
+			Volumes: []corev1.Volume{volume},
+		},
+	}}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			if err := v1alpha1.ApplyTaskModifier(&tc.ts, &TestTaskModifier{}); err != nil {
+				t.Fatalf("Did not expect error modifying TaskSpec but got %v", err)
+			}
+
+			expectedTaskSpec := v1alpha1.TaskSpec{
+				Steps: []v1alpha1.Step{{
+					Container: prependStep,
+				}, {
+					Container: appendStep,
+				}},
+				Volumes: []corev1.Volume{
+					volume,
+				},
+			}
+
+			if d := cmp.Diff(expectedTaskSpec, tc.ts); d != "" {
+				t.Errorf("TaskSpec was not modified as expected (-want, +got): %s", d)
+			}
+		})
+	}
+}
+
+func TestApplyTaskModifier_AlreadyAdded(t *testing.T) {
+	testcases := []struct {
+		name string
+		ts   v1alpha1.TaskSpec
+	}{{
+		name: "prepend already added",
+		ts: v1alpha1.TaskSpec{
+			Steps: []v1alpha1.Step{{Container: prependStep}},
+		},
+	}, {
+		name: "append already added",
+		ts: v1alpha1.TaskSpec{
+			Steps: []v1alpha1.Step{{Container: appendStep}},
+		},
+	}, {
+		name: "both steps already added",
+		ts: v1alpha1.TaskSpec{
+			Steps: []v1alpha1.Step{{Container: prependStep}, {Container: appendStep}},
+		},
+	}, {
+		name: "both steps already added reverse order",
+		ts: v1alpha1.TaskSpec{
+			Steps: []v1alpha1.Step{{Container: appendStep}, {Container: prependStep}},
+		},
+	}, {
+		name: "volume with same name but diff content already added",
+		ts: v1alpha1.TaskSpec{
+			Volumes: []corev1.Volume{{
+				Name: "magic-volume",
+				VolumeSource: corev1.VolumeSource{
+					EmptyDir: &corev1.EmptyDirVolumeSource{},
+				},
+			}},
+		},
+	}}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			if err := v1alpha1.ApplyTaskModifier(&tc.ts, &TestTaskModifier{}); err == nil {
+				t.Errorf("Expected error when applying values already added but got none")
+			}
+		})
+	}
+}

pkg/reconciler/taskrun/resources/input_resources.go

@@ -113,7 +113,9 @@ func AddInputResource(
 			if err != nil {
 				return nil, err
 			}
-			v1alpha1.ApplyTaskModifier(taskSpec, modifier)
+			if err := v1alpha1.ApplyTaskModifier(taskSpec, modifier); err != nil {
+				return nil, xerrors.Errorf("Unabled to apply Resource %s: %w", boundResource.Name, err)
+			}
 		}
 	}
 

pkg/reconciler/taskrun/resources/output_resource.go

@@ -108,7 +108,9 @@ func AddOutputResources(
 		if err != nil {
 			return nil, err
 		}
-		v1alpha1.ApplyTaskModifier(taskSpec, modifier)
+		if err := v1alpha1.ApplyTaskModifier(taskSpec, modifier); err != nil {
+			return nil, xerrors.Errorf("Unabled to apply Resource %s: %w", boundResource.Name, err)
+		}
 
 		if as.GetType() == v1alpha1.ArtifactStoragePVCType {
 			if pvcName == "" {