From a6a225623b0e359b32dec34cb83ad4ad6d5c4f49 Mon Sep 17 00:00:00 2001
From: Michelle Au <msau@google.com>
Date: Tue, 20 Feb 2018 11:42:46 -0800
Subject: [PATCH] Disallow setting both alpha and beta PV nodeAffinity Allow
 setting PV nodeAffinity if previously unset

---
 pkg/apis/core/validation/validation.go      | 15 +++++++++++--
 pkg/apis/core/validation/validation_test.go | 25 ++++++++++++++++++++-
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go
index 0a4bd397538..fd183027b1c 100644
--- a/pkg/apis/core/validation/validation.go
+++ b/pkg/apis/core/validation/validation.go
@@ -1501,9 +1501,14 @@ func ValidatePersistentVolume(pv *core.PersistentVolume) field.ErrorList {
 	allErrs = append(allErrs, errs...)
 
 	volumeNodeAffinitySpecified, errs := validateVolumeNodeAffinity(pv.Spec.NodeAffinity, specPath.Child("nodeAffinity"))
-	nodeAffinitySpecified = nodeAffinitySpecified || volumeNodeAffinitySpecified
 	allErrs = append(allErrs, errs...)
 
+	if nodeAffinitySpecified && volumeNodeAffinitySpecified {
+		allErrs = append(allErrs, field.Forbidden(specPath.Child("nodeAffinity"), "may not specify both alpha nodeAffinity annotation and nodeAffinity field"))
+	}
+
+	nodeAffinitySpecified = nodeAffinitySpecified || volumeNodeAffinitySpecified
+
 	numVolumes := 0
 	if pv.Spec.HostPath != nil {
 		if numVolumes > 0 {
@@ -1733,7 +1738,10 @@ func ValidatePersistentVolumeUpdate(newPv, oldPv *core.PersistentVolume) field.E
 	}
 
 	if utilfeature.DefaultFeatureGate.Enabled(features.VolumeScheduling) {
-		allErrs = append(allErrs, ValidateImmutableField(newPv.Spec.NodeAffinity, oldPv.Spec.NodeAffinity, field.NewPath("nodeAffinity"))...)
+		// Allow setting NodeAffinity if oldPv NodeAffinity was not set
+		if oldPv.Spec.NodeAffinity != nil {
+			allErrs = append(allErrs, ValidateImmutableField(newPv.Spec.NodeAffinity, oldPv.Spec.NodeAffinity, field.NewPath("nodeAffinity"))...)
+		}
 	}
 
 	return allErrs
@@ -4964,6 +4972,9 @@ func validateStorageNodeAffinityAnnotation(annotations map[string]string, fldPat
 }
 
 // validateVolumeNodeAffinity tests that the PersistentVolume.NodeAffinity has valid data
+// returns:
+// - true if volumeNodeAffinity is set
+// - errorList if there are validation errors
 func validateVolumeNodeAffinity(nodeAffinity *core.VolumeNodeAffinity, fldPath *field.Path) (bool, field.ErrorList) {
 	allErrs := field.ErrorList{}
 
diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go
index 15f9d8a40d9..460805569da 100644
--- a/pkg/apis/core/validation/validation_test.go
+++ b/pkg/apis/core/validation/validation_test.go
@@ -586,6 +586,29 @@ func TestValidateLocalVolumes(t *testing.T) {
 				},
 				testLocalVolume("/foo", nil)),
 		},
+		"alpha and beta local volume": {
+			isExpectedFailure: true,
+			volume: testVolumeWithAlphaNodeAffinity(
+				t,
+				"invalid-alpha-beta-local-volume",
+				"",
+				&core.NodeAffinity{
+					RequiredDuringSchedulingIgnoredDuringExecution: &core.NodeSelector{
+						NodeSelectorTerms: []core.NodeSelectorTerm{
+							{
+								MatchExpressions: []core.NodeSelectorRequirement{
+									{
+										Key:      "test-label-key",
+										Operator: core.NodeSelectorOpIn,
+										Values:   []string{"test-label-value"},
+									},
+								},
+							},
+						},
+					},
+				},
+				testLocalVolume("/foo", simpleVolumeNodeAffinity("foo", "bar"))),
+		},
 		"valid local volume": {
 			isExpectedFailure: false,
 			volume: testVolume("valid-local-volume", "",
@@ -741,7 +764,7 @@ func TestValidateVolumeNodeAffinityUpdate(t *testing.T) {
 			newPV:             testVolumeWithNodeAffinity(simpleVolumeNodeAffinity("foo", "bar2")),
 		},
 		"nil-to-obj": {
-			isExpectedFailure: true,
+			isExpectedFailure: false,
 			oldPV:             testVolumeWithNodeAffinity(nil),
 			newPV:             testVolumeWithNodeAffinity(simpleVolumeNodeAffinity("foo", "bar")),
 		},