diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 72919778229..d1af9e9fabc 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3105,30 +3105,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 diff --git a/pkg/apis/storage/validation/BUILD b/pkg/apis/storage/validation/BUILD index e8570dfba9a..90dd3fe6824 100644 --- a/pkg/apis/storage/validation/BUILD +++ b/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", diff --git a/pkg/apis/storage/validation/validation.go b/pkg/apis/storage/validation/validation.go index 1c29df8f42d..411cba3058d 100644 --- a/pkg/apis/storage/validation/validation.go +++ b/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 diff --git a/pkg/apis/storage/validation/validation_test.go b/pkg/apis/storage/validation/validation_test.go index 06ed6c7441a..4ddbaae0178 100644 --- a/pkg/apis/storage/validation/validation_test.go +++ b/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) }