From a7967073969e200ca82eca17ea999de899a66184 Mon Sep 17 00:00:00 2001 From: maao Date: Thu, 11 Aug 2022 21:21:06 +0800 Subject: [PATCH] Validate labelSelector in topologySpreadConstraints Signed-off-by: maao --- pkg/api/pod/util.go | 20 +++++-- pkg/api/pod/util_test.go | 60 +++++++++++++++++++++ pkg/apis/core/validation/validation.go | 9 +++- pkg/apis/core/validation/validation_test.go | 46 +++++++++++++++- 4 files changed, 129 insertions(+), 6 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index c99a8e34f30..dfd5229146f 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -21,6 +21,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + metavalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" utilfeature "k8s.io/apiserver/pkg/util/feature" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/helper" @@ -402,6 +403,17 @@ func haveSameExpandedDNSConfig(podSpec, oldPodSpec *api.PodSpec) bool { return true } +// hasInvalidTopologySpreadConstraintLabelSelector return true if spec.TopologySpreadConstraints have any entry with invalid labelSelector +func hasInvalidTopologySpreadConstraintLabelSelector(spec *api.PodSpec) bool { + for _, constraint := range spec.TopologySpreadConstraints { + errs := metavalidation.ValidateLabelSelector(constraint.LabelSelector, metavalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false}, nil) + if len(errs) != 0 { + return true + } + } + return false +} + // GetValidationOptionsFromPodSpecAndMeta returns validation options based on pod specs and metadata func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, podMeta, oldPodMeta *metav1.ObjectMeta) apivalidation.PodValidationOptions { // default pod validation options based on feature gate @@ -412,8 +424,9 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po // Do not allow pod spec to use non-integer multiple of huge page unit size default AllowIndivisibleHugePagesValues: false, // Allow pod spec with expanded DNS configuration - AllowExpandedDNSConfig: utilfeature.DefaultFeatureGate.Enabled(features.ExpandedDNSConfig) || haveSameExpandedDNSConfig(podSpec, oldPodSpec), - AllowInvalidLabelValueInSelector: false, + AllowExpandedDNSConfig: utilfeature.DefaultFeatureGate.Enabled(features.ExpandedDNSConfig) || haveSameExpandedDNSConfig(podSpec, oldPodSpec), + AllowInvalidLabelValueInSelector: false, + AllowInvalidTopologySpreadConstraintLabelSelector: false, } if oldPodSpec != nil { @@ -431,7 +444,8 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po opts.AllowIndivisibleHugePagesValues = usesIndivisibleHugePagesValues(oldPodSpec) opts.AllowInvalidLabelValueInSelector = hasInvalidLabelValueInAffinitySelector(oldPodSpec) - + // if old spec has invalid labelSelector in topologySpreadConstraint, we must allow it + opts.AllowInvalidTopologySpreadConstraintLabelSelector = hasInvalidTopologySpreadConstraintLabelSelector(oldPodSpec) } if oldPodMeta != nil && !opts.AllowInvalidPodDeletionCost { // This is an update, so validate only if the existing object was valid. diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 0271e1a5271..17ae3fe78ad 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -2176,3 +2176,63 @@ func TestDropSchedulingGates(t *testing.T) { } } } + +func TestValidateTopologySpreadConstraintLabelSelectorOption(t *testing.T) { + testCases := []struct { + name string + oldPodSpec *api.PodSpec + wantOption bool + }{ + { + name: "Create", + wantOption: false, + }, + { + name: "UpdateInvalidLabelSelector", + oldPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "foo"}, + }, + }, + }, + }, + wantOption: true, + }, + { + name: "UpdateValidLabelSelector", + oldPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo": "foo"}, + }, + }, + }, + }, + wantOption: false, + }, + { + name: "UpdateEmptyLabelSelector", + oldPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + { + LabelSelector: nil, + }, + }, + }, + wantOption: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Pod meta doesn't impact the outcome. + gotOptions := GetValidationOptionsFromPodSpecAndMeta(&api.PodSpec{}, tc.oldPodSpec, nil, nil) + if tc.wantOption != gotOptions.AllowInvalidTopologySpreadConstraintLabelSelector { + t.Errorf("Got AllowInvalidLabelValueInSelector=%t, want %t", gotOptions.AllowInvalidTopologySpreadConstraintLabelSelector, tc.wantOption) + } + }) + } +} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index d53339863b3..656a846735f 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3654,6 +3654,8 @@ type PodValidationOptions struct { AllowIndivisibleHugePagesValues bool // Allow more DNSSearchPaths and longer DNSSearchListChars AllowExpandedDNSConfig bool + // Allow invalid topologySpreadConstraint labelSelector for backward compatibility + AllowInvalidTopologySpreadConstraintLabelSelector bool } // validatePodMetadataAndSpec tests if required fields in the pod.metadata and pod.spec are set, @@ -3759,7 +3761,7 @@ func ValidatePodSpec(spec *core.PodSpec, podMeta *metav1.ObjectMeta, fldPath *fi allErrs = append(allErrs, validatePodDNSConfig(spec.DNSConfig, &spec.DNSPolicy, fldPath.Child("dnsConfig"), opts)...) allErrs = append(allErrs, validateReadinessGates(spec.ReadinessGates, fldPath.Child("readinessGates"))...) allErrs = append(allErrs, validateSchedulingGates(spec.SchedulingGates, fldPath.Child("schedulingGates"))...) - allErrs = append(allErrs, validateTopologySpreadConstraints(spec.TopologySpreadConstraints, fldPath.Child("topologySpreadConstraints"))...) + allErrs = append(allErrs, validateTopologySpreadConstraints(spec.TopologySpreadConstraints, fldPath.Child("topologySpreadConstraints"), opts)...) allErrs = append(allErrs, validateWindowsHostProcessPod(spec, fldPath)...) allErrs = append(allErrs, validateHostUsers(spec, fldPath)...) if len(spec.ServiceAccountName) > 0 { @@ -6742,7 +6744,7 @@ var ( ) // validateTopologySpreadConstraints validates given TopologySpreadConstraints. -func validateTopologySpreadConstraints(constraints []core.TopologySpreadConstraint, fldPath *field.Path) field.ErrorList { +func validateTopologySpreadConstraints(constraints []core.TopologySpreadConstraint, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} for i, constraint := range constraints { @@ -6768,6 +6770,9 @@ func validateTopologySpreadConstraints(constraints []core.TopologySpreadConstrai allErrs = append(allErrs, err) } allErrs = append(allErrs, validateMatchLabelKeys(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...) + if !opts.AllowInvalidTopologySpreadConstraintLabelSelector { + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(constraint.LabelSelector, unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false}, subFldPath.Child("labelSelector"))...) + } } return allErrs diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index bf669a7fbf3..b5939bf5c77 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -19976,6 +19976,7 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { fieldPathMatchLabelKeys := subFldPath0.Child("matchLabelKeys") nodeAffinityField := subFldPath0.Child("nodeAffinityPolicy") nodeTaintsField := subFldPath0.Child("nodeTaintsPolicy") + labelSelectorField := subFldPath0.Child("labelSelector") unknown := core.NodeInclusionPolicy("Unknown") ignore := core.NodeInclusionPolicyIgnore honor := core.NodeInclusionPolicyHonor @@ -19984,6 +19985,7 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { name string constraints []core.TopologySpreadConstraint wantFieldErrors field.ErrorList + opts PodValidationOptions }{ { name: "all required fields ok", @@ -20185,11 +20187,53 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { }, wantFieldErrors: []*field.Error{field.Invalid(fieldPathMatchLabelKeys.Index(0), "foo", "exists in both matchLabelKeys and labelSelector")}, }, + { + name: "invalid matchLabels set on labelSelector when AllowInvalidTopologySpreadConstraintLabelSelector is false", + constraints: []core.TopologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "k8s.io/zone", + WhenUnsatisfiable: core.DoNotSchedule, + MinDomains: nil, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "foo"}}, + }, + }, + wantFieldErrors: []*field.Error{field.Invalid(labelSelectorField.Child("matchLabels"), "NoUppercaseOrSpecialCharsLike=Equals", "name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')")}, + opts: PodValidationOptions{AllowInvalidTopologySpreadConstraintLabelSelector: false}, + }, + { + name: "invalid matchLabels set on labelSelector when AllowInvalidTopologySpreadConstraintLabelSelector is true", + constraints: []core.TopologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "k8s.io/zone", + WhenUnsatisfiable: core.DoNotSchedule, + MinDomains: nil, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "foo"}}, + }, + }, + wantFieldErrors: []*field.Error{}, + opts: PodValidationOptions{AllowInvalidTopologySpreadConstraintLabelSelector: true}, + }, + { + name: "valid matchLabels set on labelSelector when AllowInvalidTopologySpreadConstraintLabelSelector is false", + constraints: []core.TopologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "k8s.io/zone", + WhenUnsatisfiable: core.DoNotSchedule, + MinDomains: nil, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "foo"}}, + }, + }, + wantFieldErrors: []*field.Error{}, + opts: PodValidationOptions{AllowInvalidTopologySpreadConstraintLabelSelector: false}, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - errs := validateTopologySpreadConstraints(tc.constraints, fieldPath) + errs := validateTopologySpreadConstraints(tc.constraints, fieldPath, tc.opts) if diff := cmp.Diff(tc.wantFieldErrors, errs); diff != "" { t.Errorf("unexpected field errors (-want, +got):\n%s", diff) }