Added set and map structural validation for AllowedTopologies

pkg/apis/core/validation/validation.go

@@ -3087,30 +3087,55 @@ func ValidateNodeSelector(nodeSelector *core.NodeSelector, fldPath *field.Path)
 	return allErrs
 }
 
-// validateTopologySelectorLabelRequirement tests that the specified TopologySelectorLabelRequirement fields has valid data
-func validateTopologySelectorLabelRequirement(rq core.TopologySelectorLabelRequirement, fldPath *field.Path) field.ErrorList {
+// validateTopologySelectorLabelRequirement tests that the specified TopologySelectorLabelRequirement fields has valid data,
+// and constructs a set containing all of its Values.
+func validateTopologySelectorLabelRequirement(rq core.TopologySelectorLabelRequirement, fldPath *field.Path) (sets.String, field.ErrorList) {
 	allErrs := field.ErrorList{}
+	valueSet := make(sets.String)
+	valuesPath := fldPath.Child("values")
 	if len(rq.Values) == 0 {
-		allErrs = append(allErrs, field.Forbidden(fldPath.Child("values"), "must specify as least one value"))
+		allErrs = append(allErrs, field.Required(valuesPath, ""))
 	}
+
+	// Validate set property of Values field
+	for i, value := range rq.Values {
+		if valueSet.Has(value) {
+			allErrs = append(allErrs, field.Duplicate(valuesPath.Index(i), value))
+		}
+		valueSet.Insert(value)
+	}
+
 	allErrs = append(allErrs, unversionedvalidation.ValidateLabelName(rq.Key, fldPath.Child("key"))...)
 
-	return allErrs
+	return valueSet, allErrs
 }
 
-// ValidateTopologySelectorTerm tests that the specified topology selector term has valid data
-func ValidateTopologySelectorTerm(term core.TopologySelectorTerm, fldPath *field.Path) field.ErrorList {
+// ValidateTopologySelectorTerm tests that the specified topology selector term has valid data,
+// and constructs a map representing the term in raw form.
+func ValidateTopologySelectorTerm(term core.TopologySelectorTerm, fldPath *field.Path) (map[string]sets.String, field.ErrorList) {
 	allErrs := field.ErrorList{}
+	exprMap := make(map[string]sets.String)
+	exprPath := fldPath.Child("matchLabelExpressions")
 
 	if utilfeature.DefaultFeatureGate.Enabled(features.DynamicProvisioningScheduling) {
+		// Allow empty MatchLabelExpressions, in case this field becomes optional in the future.
+
 		for i, req := range term.MatchLabelExpressions {
-			allErrs = append(allErrs, validateTopologySelectorLabelRequirement(req, fldPath.Child("matchLabelExpressions").Index(i))...)
+			idxPath := exprPath.Index(i)
+			valueSet, exprErrs := validateTopologySelectorLabelRequirement(req, idxPath)
+			allErrs = append(allErrs, exprErrs...)
+
+			// Validate no duplicate keys exist.
+			if _, exists := exprMap[req.Key]; exists {
+				allErrs = append(allErrs, field.Duplicate(idxPath.Child("key"), req.Key))
+			}
+			exprMap[req.Key] = valueSet
 		}
 	} else if len(term.MatchLabelExpressions) != 0 {
 		allErrs = append(allErrs, field.Forbidden(fldPath, "field is disabled by feature-gate DynamicProvisioningScheduling"))
 	}
 
-	return allErrs
+	return exprMap, allErrs
 }
 
 // ValidateAvoidPodsInNodeAnnotations tests that the serialized AvoidPods in Node.Annotations has valid data

pkg/apis/storage/validation/BUILD

@@ -12,6 +12,7 @@ go_library(
     importpath = "k8s.io/kubernetes/pkg/apis/storage/validation",
     deps = [
         "//pkg/apis/core:go_default_library",
+        "//pkg/apis/core/helper:go_default_library",
         "//pkg/apis/core/validation:go_default_library",
         "//pkg/apis/storage:go_default_library",
         "//pkg/features:go_default_library",

pkg/apis/storage/validation/validation.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	api "k8s.io/kubernetes/pkg/apis/core"
+	"k8s.io/kubernetes/pkg/apis/core/helper"
 	apivalidation "k8s.io/kubernetes/pkg/apis/core/validation"
 	"k8s.io/kubernetes/pkg/apis/storage"
 	"k8s.io/kubernetes/pkg/features"
@@ -253,8 +254,20 @@ func validateAllowedTopologies(topologies []api.TopologySelectorTerm, fldPath *f
 		allErrs = append(allErrs, field.Forbidden(fldPath, "field is disabled by feature-gate DynamicProvisioningScheduling"))
 	}
 
+	rawTopologies := make([]map[string]sets.String, len(topologies))
 	for i, term := range topologies {
-		allErrs = append(allErrs, apivalidation.ValidateTopologySelectorTerm(term, fldPath.Index(i))...)
+		idxPath := fldPath.Index(i)
+		exprMap, termErrs := apivalidation.ValidateTopologySelectorTerm(term, fldPath.Index(i))
+		allErrs = append(allErrs, termErrs...)
+
+		// TODO (verult) consider improving runtime
+		for _, t := range rawTopologies {
+			if helper.Semantic.DeepEqual(exprMap, t) {
+				allErrs = append(allErrs, field.Duplicate(idxPath.Child("matchLabelExpressions"), ""))
+			}
+		}
+
+		rawTopologies = append(rawTopologies, exprMap)
 	}
 
 	return allErrs

pkg/apis/storage/validation/validation_test.go

@@ -644,6 +644,187 @@ func TestValidateAllowedTopologies(t *testing.T) {
 		},
 	}
 
+	topologyDupValues := []api.TopologySelectorTerm{
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node1", "node1"},
+				},
+			},
+		},
+	}
+
+	topologyMultiValues := []api.TopologySelectorTerm{
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node1", "node2"},
+				},
+			},
+		},
+	}
+
+	topologyEmptyMatchLabelExpressions := []api.TopologySelectorTerm{
+		{
+			MatchLabelExpressions: nil,
+		},
+	}
+
+	topologyDupKeys := []api.TopologySelectorTerm{
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node1"},
+				},
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node2"},
+				},
+			},
+		},
+	}
+
+	topologyMultiTerm := []api.TopologySelectorTerm{
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node1"},
+				},
+			},
+		},
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node2"},
+				},
+			},
+		},
+	}
+
+	topologyDupTermsIdentical := []api.TopologySelectorTerm{
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "failure-domain.beta.kubernetes.io/zone",
+					Values: []string{"zone1"},
+				},
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node1"},
+				},
+			},
+		},
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "failure-domain.beta.kubernetes.io/zone",
+					Values: []string{"zone1"},
+				},
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node1"},
+				},
+			},
+		},
+	}
+
+	topologyExprsOneSameOneDiff := []api.TopologySelectorTerm{
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "failure-domain.beta.kubernetes.io/zone",
+					Values: []string{"zone1"},
+				},
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node1"},
+				},
+			},
+		},
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "failure-domain.beta.kubernetes.io/zone",
+					Values: []string{"zone1"},
+				},
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node2"},
+				},
+			},
+		},
+	}
+
+	topologyValuesOneSameOneDiff := []api.TopologySelectorTerm{
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node1", "node2"},
+				},
+			},
+		},
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node1", "node3"},
+				},
+			},
+		},
+	}
+
+	topologyDupTermsDiffExprOrder := []api.TopologySelectorTerm{
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node1"},
+				},
+				{
+					Key:    "failure-domain.beta.kubernetes.io/zone",
+					Values: []string{"zone1"},
+				},
+			},
+		},
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "failure-domain.beta.kubernetes.io/zone",
+					Values: []string{"zone1"},
+				},
+				{
+					Key:    "kubernetes.io/hostname",
+					Values: []string{"node1"},
+				},
+			},
+		},
+	}
+
+	topologyDupTermsDiffValueOrder := []api.TopologySelectorTerm{
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "failure-domain.beta.kubernetes.io/zone",
+					Values: []string{"zone1", "zone2"},
+				},
+			},
+		},
+		{
+			MatchLabelExpressions: []api.TopologySelectorLabelRequirement{
+				{
+					Key:    "failure-domain.beta.kubernetes.io/zone",
+					Values: []string{"zone2", "zone1"},
+				},
+			},
+		},
+	}
+
 	cases := map[string]bindingTest{
 		"no topology": {
 			class:         makeClass(nil, nil),
@@ -661,10 +842,56 @@ func TestValidateAllowedTopologies(t *testing.T) {
 			class:         makeClass(nil, topologyLackOfValues),
 			shouldSucceed: false,
 		},
+		"duplicate TopologySelectorRequirement values": {
+			class:         makeClass(nil, topologyDupValues),
+			shouldSucceed: false,
+		},
+		"multiple TopologySelectorRequirement values": {
+			class:         makeClass(nil, topologyMultiValues),
+			shouldSucceed: true,
+		},
+		"empty MatchLabelExpressions": {
+			class:         makeClass(nil, topologyEmptyMatchLabelExpressions),
+			shouldSucceed: false,
+		},
+		"duplicate MatchLabelExpression keys": {
+			class:         makeClass(nil, topologyDupKeys),
+			shouldSucceed: false,
+		},
+		"duplicate MatchLabelExpression keys but across separate terms": {
+			class:         makeClass(nil, topologyMultiTerm),
+			shouldSucceed: true,
+		},
+		"duplicate AllowedTopologies terms - identical": {
+			class:         makeClass(nil, topologyDupTermsIdentical),
+			shouldSucceed: false,
+		},
+		"two AllowedTopologies terms, with a pair of the same MatchLabelExpressions and a pair of different ones": {
+			class:         makeClass(nil, topologyExprsOneSameOneDiff),
+			shouldSucceed: true,
+		},
+		"two AllowedTopologies terms, with a pair of the same Values and a pair of different ones": {
+			class:         makeClass(nil, topologyValuesOneSameOneDiff),
+			shouldSucceed: true,
+		},
+		"duplicate AllowedTopologies terms - different MatchLabelExpressions order": {
+			class:         makeClass(nil, topologyDupTermsDiffExprOrder),
+			shouldSucceed: false,
+		},
+		"duplicate AllowedTopologies terms - different TopologySelectorRequirement values order": {
+			class:         makeClass(nil, topologyDupTermsDiffValueOrder),
+			shouldSucceed: false,
+		},
+	}
+
+	// Disable VolumeScheduling so nil VolumeBindingMode doesn't fail to validate.
+	err := utilfeature.DefaultFeatureGate.Set("VolumeScheduling=false")
+	if err != nil {
+		t.Fatalf("Failed to disable feature gate for VolumeScheduling: %v", err)
 	}
 
 	// TODO: remove when feature gate not required
-	err := utilfeature.DefaultFeatureGate.Set("DynamicProvisioningScheduling=true")
+	err = utilfeature.DefaultFeatureGate.Set("DynamicProvisioningScheduling=true")
 	if err != nil {
 		t.Fatalf("Failed to enable feature gate for DynamicProvisioningScheduling: %v", err)
 	}