Add support for repeated PVC-claim but using subPath in AA-validation

The validation for compatibility with the Affinity Assistant does not
support the same PVC repeated, but using different subPaths.

This patch adds support for this case and tests for the validation.

Co-authored-by: Scott <sbws@google.com>

Author: 2 people

pkg/reconciler/taskrun/taskrun.go

@@ -302,10 +302,12 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1
 		return nil, nil, controller.NewPermanentError(err)
 	}
 
-	if err := validateWorkspaceCompatibilityWithAffinityAssistant(tr); err != nil {
-		logger.Errorf("TaskRun %q workspaces are invalid: %v", tr.Name, err)
-		tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err)
-		return nil, nil, controller.NewPermanentError(err)
+	if _, usesAssistant := tr.Annotations[workspace.AnnotationAffinityAssistantName]; usesAssistant {
+		if err := workspace.ValidateOnlyOnePVCIsUsed(tr.Spec.Workspaces); err != nil {
+			logger.Errorf("TaskRun %q workspaces incompatible with Affinity Assistant: %v", tr.Name, err)
+			tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err)
+			return nil, nil, controller.NewPermanentError(err)
+		}
 	}
 
 	// Initialize the cloud events if at least a CloudEventResource is defined
@@ -775,26 +777,3 @@ func storeTaskSpec(ctx context.Context, tr *v1beta1.TaskRun, ts *v1beta1.TaskSpe
 	}
 	return nil
 }
-
-// validateWorkspaceCompatibilityWithAffinityAssistant validates the TaskRun's compatibility
-// with the Affinity Assistant - if associated with an Affinity Assistant.
-// No more than one PVC-backed workspace can be used for a TaskRun that is associated with an
-// Affinity Assistant.
-func validateWorkspaceCompatibilityWithAffinityAssistant(tr *v1beta1.TaskRun) error {
-	_, isAssociatedWithAnAffinityAssistant := tr.Annotations[workspace.AnnotationAffinityAssistantName]
-	if !isAssociatedWithAnAffinityAssistant {
-		return nil
-	}
-
-	pvcWorkspaces := 0
-	for _, w := range tr.Spec.Workspaces {
-		if w.PersistentVolumeClaim != nil || w.VolumeClaimTemplate != nil {
-			pvcWorkspaces++
-		}
-	}
-
-	if pvcWorkspaces > 1 {
-		return fmt.Errorf("TaskRun mounts more than one PersistentVolumeClaim - that is forbidden when the Affinity Assistant is enabled")
-	}
-	return nil
-}

pkg/workspace/validate.go

@@ -48,3 +48,25 @@ func ValidateBindings(w []v1beta1.WorkspaceDeclaration, wb []v1beta1.WorkspaceBi
 	}
 	return nil
 }
+
+// ValidateOnlyOnePVCIsUsed checks that a list of WorkspaceBinding uses only one
+// persistent volume claim.
+//
+// This is only useful to validate that WorkspaceBindings in TaskRuns are compatible
+// with affinity rules enforced by the AffinityAssistant.
+func ValidateOnlyOnePVCIsUsed(wb []v1beta1.WorkspaceBinding) error {
+	workspaceVolumes := make(map[string]bool)
+	for _, w := range wb {
+		if w.PersistentVolumeClaim != nil {
+			workspaceVolumes[w.PersistentVolumeClaim.ClaimName] = true
+		}
+		if w.VolumeClaimTemplate != nil {
+			workspaceVolumes[w.Name] = true
+		}
+	}
+
+	if len(workspaceVolumes) > 1 {
+		return fmt.Errorf("more than one PersistentVolumeClaim is bound")
+	}
+	return nil
+}

pkg/workspace/validate_test.go

@@ -17,9 +17,11 @@ limitations under the License.
 package workspace
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
+	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	corev1 "k8s.io/api/core/v1"
 )
 
@@ -131,3 +133,89 @@ func TestValidateBindingsInvalid(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateOnlyOnePVCIsUsed_Valid(t *testing.T) {
+	for _, tc := range []struct {
+		name     string
+		bindings []v1beta1.WorkspaceBinding
+	}{{
+		name:     "an error is not returned when no bindings are given",
+		bindings: []v1beta1.WorkspaceBinding{},
+	}, {
+		name: "an error is not returned when volume claims are not used",
+		bindings: []v1beta1.WorkspaceBinding{{
+			EmptyDir: &corev1.EmptyDirVolumeSource{},
+		}, {
+			Secret: &corev1.SecretVolumeSource{},
+		}},
+	}, {
+		name: "an error is not returned when one PV claim is used in two bindings",
+		bindings: []v1beta1.WorkspaceBinding{{
+			PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
+				ClaimName: "foo",
+			},
+		}, {
+			PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
+				ClaimName: "foo",
+			},
+		}},
+	}, {
+		name: "an error is not returned when one PV claim is used in two bindings with different subpaths",
+		bindings: []v1beta1.WorkspaceBinding{{
+			SubPath: "/pathA",
+			PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
+				ClaimName: "foo",
+			},
+		}, {
+			SubPath: "/pathB",
+			PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
+				ClaimName: "foo",
+			},
+		}},
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			if err := ValidateOnlyOnePVCIsUsed(tc.bindings); err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+		})
+	}
+}
+
+func TestValidateOnlyOnePVCIsUsed_Invalid(t *testing.T) {
+	validationError := errors.New("more than one PersistentVolumeClaim is bound")
+	for _, tc := range []struct {
+		name     string
+		bindings []v1beta1.WorkspaceBinding
+		wantErr  error
+	}{{
+		name: "an error is returned when two different PV claims are used",
+		bindings: []v1beta1.WorkspaceBinding{{
+			PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
+				ClaimName: "foo",
+			},
+		}, {
+			PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
+				ClaimName: "bar",
+			},
+		}},
+		wantErr: validationError,
+	}, {
+		name: "an error is returned when a PVC and volume claim template are mixed",
+		bindings: []v1beta1.WorkspaceBinding{{
+			PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
+				ClaimName: "foo",
+			},
+		}, {
+			Name:                "bar",
+			VolumeClaimTemplate: &corev1.PersistentVolumeClaim{},
+		}},
+		wantErr: validationError,
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			err := ValidateOnlyOnePVCIsUsed(tc.bindings)
+			if err == nil || (tc.wantErr.Error() != err.Error()) {
+				t.Errorf("expected %v received %v", tc.wantErr, err)
+			}
+		})
+	}
+}