Fix issue where workspace volume variable and pod volume conf differ

Prior to this commit the volumes that were generated for workspace bindings
did not have names that matched the values injected through
workspaces.<name>.volume variables. This occurred because we called
the GetVolumes workspace function twice during TaskRun Pod initialization and
each execution generated different random names.

This commit updates the TaskRun reconciler to generate the random volume
names only once and then pass those generated volumes to the two functions
that need them. Additionally the function generating those volumes is renamed
to make clear that volume creation is its purpose. A e2e test is added to
ensure that the workspace volume and variable value matches.

pkg/reconciler/taskrun/resources/apply.go

@@ -20,7 +20,7 @@ import (
 	"fmt"
 	"path/filepath"
 
-	"github.com/tektoncd/pipeline/pkg/workspace"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
@@ -112,7 +112,7 @@ func ApplyContexts(spec *v1beta1.TaskSpec, rtr *ResolvedTaskResources, tr *v1bet
 // ApplyWorkspaces applies the substitution from paths that the workspaces in declarations mounted to, the
 // volumes that bindings are realized with in the task spec and the PersistentVolumeClaim names for the
 // workspaces.
-func ApplyWorkspaces(spec *v1beta1.TaskSpec, declarations []v1beta1.WorkspaceDeclaration, bindings []v1beta1.WorkspaceBinding) *v1beta1.TaskSpec {
+func ApplyWorkspaces(spec *v1beta1.TaskSpec, declarations []v1beta1.WorkspaceDeclaration, bindings []v1beta1.WorkspaceBinding, vols map[string]corev1.Volume) *v1beta1.TaskSpec {
 	stringReplacements := map[string]string{}
 
 	bindNames := sets.NewString()
@@ -131,7 +131,6 @@ func ApplyWorkspaces(spec *v1beta1.TaskSpec, declarations []v1beta1.WorkspaceDec
 		}
 	}
 
-	vols := workspace.GetVolumes(bindings)
 	for name, vol := range vols {
 		stringReplacements[fmt.Sprintf("workspaces.%s.volume", name)] = vol.Name
 	}

pkg/reconciler/taskrun/resources/apply_test.go

@@ -25,6 +25,7 @@ import (
 	"github.com/tektoncd/pipeline/pkg/apis/resource"
 	resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1"
 	"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
+	"github.com/tektoncd/pipeline/pkg/workspace"
 	"github.com/tektoncd/pipeline/test/diff"
 	"github.com/tektoncd/pipeline/test/names"
 	corev1 "k8s.io/api/core/v1"
@@ -788,7 +789,8 @@ func TestApplyWorkspaces(t *testing.T) {
 		}}},
 	}} {
 		t.Run(tc.name, func(t *testing.T) {
-			got := resources.ApplyWorkspaces(tc.spec, tc.decls, tc.binds)
+			vols := workspace.CreateVolumes(tc.binds)
+			got := resources.ApplyWorkspaces(tc.spec, tc.decls, tc.binds, vols)
 			if d := cmp.Diff(tc.want, got); d != "" {
 				t.Errorf("TestApplyWorkspaces() got diff %s", diff.PrintWantGot(d))
 			}

pkg/reconciler/taskrun/taskrun.go

@@ -603,13 +603,16 @@ func (c *Reconciler) createPod(ctx context.Context, tr *v1beta1.TaskRun, rtr *re
 	ts = resources.ApplyResources(ts, inputResources, "inputs")
 	ts = resources.ApplyResources(ts, outputResources, "outputs")
 
+	// Get the randomized volume names assigned to workspace bindings
+	workspaceVolumes := workspace.CreateVolumes(tr.Spec.Workspaces)
+
 	// Apply workspace resource substitution
-	ts = resources.ApplyWorkspaces(ts, ts.Workspaces, tr.Spec.Workspaces)
+	ts = resources.ApplyWorkspaces(ts, ts.Workspaces, tr.Spec.Workspaces, workspaceVolumes)
 
 	// Apply task result substitution
 	ts = resources.ApplyTaskResults(ts)
 
-	ts, err = workspace.Apply(*ts, tr.Spec.Workspaces)
+	ts, err = workspace.Apply(*ts, tr.Spec.Workspaces, workspaceVolumes)
 	if err != nil {
 		logger.Errorf("Failed to create a pod for taskrun: %s due to workspace error %v", tr.Name, err)
 		return nil, err

pkg/workspace/apply.go

@@ -39,10 +39,11 @@ func (nvm nameVolumeMap) setVolumeSource(workspaceName string, volumeName string
 	}
 }
 
-// GetVolumes will return a dictionary where the keys are the names of the workspaces bound in
-// wb and the value is the Volume to use. If the same Volume is bound twice, the resulting volumes
-// will both have the same name to prevent the same Volume from being attached to a pod twice.
-func GetVolumes(wb []v1beta1.WorkspaceBinding) map[string]corev1.Volume {
+// CreateVolumes will return a dictionary where the keys are the names of the workspaces bound in
+// wb and the value is a newly-created Volume to use. If the same Volume is bound twice, the
+// resulting volumes will both have the same name to prevent the same Volume from being attached
+// to a pod twice. The names of the returned volumes will be a short random string starting "ws-".
+func CreateVolumes(wb []v1beta1.WorkspaceBinding) map[string]corev1.Volume {
 	pvcs := map[string]corev1.Volume{}
 	v := make(nameVolumeMap)
 	for _, w := range wb {
@@ -84,13 +85,12 @@ func getDeclaredWorkspace(name string, w []v1beta1.WorkspaceDeclaration) (*v1bet
 // Apply will update the StepTemplate and Volumes declaration in ts so that the workspaces
 // specified through wb combined with the declared workspaces in ts will be available for
 // all containers in the resulting pod.
-func Apply(ts v1beta1.TaskSpec, wb []v1beta1.WorkspaceBinding) (*v1beta1.TaskSpec, error) {
+func Apply(ts v1beta1.TaskSpec, wb []v1beta1.WorkspaceBinding, v map[string]corev1.Volume) (*v1beta1.TaskSpec, error) {
 	// If there are no bound workspaces, we don't need to do anything
 	if len(wb) == 0 {
 		return &ts, nil
 	}
 
-	v := GetVolumes(wb)
 	addedVolumes := sets.NewString()
 
 	// Initialize StepTemplate if it hasn't been already

pkg/workspace/apply_test.go

@@ -11,7 +11,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 )
 
-func TestGetVolumes(t *testing.T) {
+func TestCreateVolumes(t *testing.T) {
 	names.TestingSeed()
 	for _, tc := range []struct {
 		name            string
@@ -185,7 +185,7 @@ func TestGetVolumes(t *testing.T) {
 		},
 	}} {
 		t.Run(tc.name, func(t *testing.T) {
-			v := workspace.GetVolumes(tc.workspaces)
+			v := workspace.CreateVolumes(tc.workspaces)
 			if d := cmp.Diff(tc.expectedVolumes, v); d != "" {
 				t.Errorf("Didn't get expected volumes from bindings %s", diff.PrintWantGot(d))
 			}
@@ -511,7 +511,8 @@ func TestApply(t *testing.T) {
 		},
 	}} {
 		t.Run(tc.name, func(t *testing.T) {
-			ts, err := workspace.Apply(tc.ts, tc.workspaces)
+			vols := workspace.CreateVolumes(tc.workspaces)
+			ts, err := workspace.Apply(tc.ts, tc.workspaces, vols)
 			if err != nil {
 				t.Fatalf("Did not expect error but got %v", err)
 			}

test/workspace_test.go

@@ -235,3 +235,85 @@ func TestWorkspacePipelineRunMissingWorkspaceInvalid(t *testing.T) {
 		t.Fatalf("Failed to wait for PipelineRun %q to finish: %s", pipelineRunName, err)
 	}
 }
+
+func TestWorkspaceVolumeNameMatchesVolumeVariableReplacement(t *testing.T) {
+	c, namespace := setup(t)
+
+	taskName := "foo-task"
+	taskRunName := "foo-taskrun"
+
+	knativetest.CleanupOnInterrupt(func() { tearDown(t, c, namespace) }, t.Logf)
+	defer tearDown(t, c, namespace)
+
+	task := &v1beta1.Task{
+		ObjectMeta: metav1.ObjectMeta{Name: taskName, Namespace: namespace},
+		Spec: v1beta1.TaskSpec{
+			Steps: []v1beta1.Step{{Container: corev1.Container{
+				Name:    "foo",
+				Image:   "alpine",
+				Command: []string{"echo"},
+				Args:    []string{"$(workspaces.test.volume)"},
+			}}},
+			Workspaces: []v1beta1.WorkspaceDeclaration{{
+				Name:        "test",
+				Description: "test workspace",
+				MountPath:   "/workspace/test/file",
+				ReadOnly:    true,
+			}},
+		},
+	}
+	if _, err := c.TaskClient.Create(task); err != nil {
+		t.Fatalf("Failed to create Task: %s", err)
+	}
+
+	taskRun := &v1beta1.TaskRun{
+		ObjectMeta: metav1.ObjectMeta{Name: taskRunName, Namespace: namespace},
+		Spec: v1beta1.TaskRunSpec{
+			TaskRef:            &v1beta1.TaskRef{Name: taskName},
+			ServiceAccountName: "default",
+			Workspaces: []v1beta1.WorkspaceBinding{{
+				Name:     "test",
+				EmptyDir: &corev1.EmptyDirVolumeSource{},
+			}},
+		},
+	}
+	if _, err := c.TaskRunClient.Create(taskRun); err != nil {
+		t.Fatalf("Failed to create TaskRun: %s", err)
+	}
+
+	t.Logf("Waiting for TaskRun in namespace %s to finish", namespace)
+	if err := WaitForTaskRunState(c, taskRunName, TaskRunSucceed(taskRunName), "success"); err != nil {
+		t.Errorf("Error waiting for TaskRun to finish with error: %s", err)
+	}
+
+	tr, err := c.TaskRunClient.Get(taskRunName, metav1.GetOptions{})
+	if err != nil {
+		t.Errorf("Error retrieving taskrun: %s", err)
+	}
+	if tr.Status.PodName == "" {
+		t.Fatal("Error getting a PodName (empty)")
+	}
+	p, err := c.KubeClient.Kube.CoreV1().Pods(namespace).Get(tr.Status.PodName, metav1.GetOptions{})
+
+	if err != nil {
+		t.Fatalf("Error getting pod `%s` in namespace `%s`", tr.Status.PodName, namespace)
+	}
+
+	workspaceVariableValue := ""
+	for _, container := range p.Spec.Containers {
+		if container.Name == "step-foo" {
+			argsLen := len(container.Args)
+			workspaceVariableValue = container.Args[argsLen-1]
+			break
+		}
+	}
+
+	volumeNames := []string{}
+	for _, volume := range p.Spec.Volumes {
+		if volume.Name == workspaceVariableValue {
+			return
+		}
+		volumeNames = append(volumeNames, volume.Name)
+	}
+	t.Fatalf("Workspace volume variable %q does not match any volume name in Pod volumes list %#v", workspaceVariableValue, volumeNames)
+}