From ae11c7deb1b07803ad1bdfbbe32ddd85c2dad617 Mon Sep 17 00:00:00 2001 From: AxeZhan Date: Wed, 23 Oct 2024 17:28:27 +0800 Subject: [PATCH] DisallowInvalidLabelValueInNodeSelector --- pkg/api/node/util.go | 16 ++- pkg/api/persistentvolume/util.go | 2 +- pkg/api/pod/util.go | 12 ++ pkg/api/pod/util_test.go | 74 +++++++++++ pkg/api/pod/warnings.go | 4 +- pkg/api/pod/warnings_test.go | 56 +++++++++ pkg/apis/core/helper/helpers.go | 15 +++ pkg/apis/core/helper/helpers_test.go | 45 +++++++ pkg/apis/core/validation/validation.go | 24 ++-- pkg/apis/core/validation/validation_test.go | 131 +++++++++++++++++++- 10 files changed, 366 insertions(+), 13 deletions(-) diff --git a/pkg/api/node/util.go b/pkg/api/node/util.go index daf5ee0cf62..a221642f624 100644 --- a/pkg/api/node/util.go +++ b/pkg/api/node/util.go @@ -20,6 +20,7 @@ import ( "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/node" @@ -91,7 +92,7 @@ func GetWarningsForNodeSelector(nodeSelector *metav1.LabelSelector, fieldPath *f } // GetWarningsForNodeSelectorTerm checks match expressions of node selector term -func GetWarningsForNodeSelectorTerm(nodeSelectorTerm api.NodeSelectorTerm, fieldPath *field.Path) []string { +func GetWarningsForNodeSelectorTerm(nodeSelectorTerm api.NodeSelectorTerm, checkLabelValue bool, fieldPath *field.Path) []string { var warnings []string // use of deprecated node labels in matchLabelExpressions for i, expression := range nodeSelectorTerm.MatchExpressions { @@ -106,6 +107,19 @@ func GetWarningsForNodeSelectorTerm(nodeSelectorTerm api.NodeSelectorTerm, field ), ) } + if checkLabelValue { + for index, value := range expression.Values { + for _, msg := range validation.IsValidLabelValue(value) { + warnings = append(warnings, + fmt.Sprintf( + "%s: %s is invalid, %s", + fieldPath.Child("matchExpressions").Index(i).Child("values").Index(index), + value, + msg, + )) + } + } + } } return warnings } diff --git a/pkg/api/persistentvolume/util.go b/pkg/api/persistentvolume/util.go index 0b4dfbfb21b..b0a98e57b49 100644 --- a/pkg/api/persistentvolume/util.go +++ b/pkg/api/persistentvolume/util.go @@ -76,7 +76,7 @@ func warningsForPersistentVolumeSpecAndMeta(fieldPath *field.Path, pvSpec *api.P termFldPath := fieldPath.Child("spec", "nodeAffinity", "required", "nodeSelectorTerms") // use of deprecated node labels in node affinity for i, term := range pvSpec.NodeAffinity.Required.NodeSelectorTerms { - warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term, termFldPath.Index(i))...) + warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term, false, termFldPath.Index(i))...) } } // If we are on deprecated volume plugin diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 621438091b2..7cae53daad2 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -385,6 +385,7 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po AllowNonLocalProjectedTokenPath: false, AllowPodLifecycleSleepActionZeroValue: utilfeature.DefaultFeatureGate.Enabled(features.PodLifecycleSleepActionAllowZero), PodLevelResourcesEnabled: utilfeature.DefaultFeatureGate.Enabled(features.PodLevelResources), + AllowInvalidLabelValueInRequiredNodeAffinity: false, } // If old spec uses relaxed validation or enabled the RelaxedEnvironmentVariableValidation feature gate, @@ -399,6 +400,7 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po opts.AllowIndivisibleHugePagesValues = usesIndivisibleHugePagesValues(oldPodSpec) opts.AllowInvalidLabelValueInSelector = hasInvalidLabelValueInAffinitySelector(oldPodSpec) + opts.AllowInvalidLabelValueInRequiredNodeAffinity = hasInvalidLabelValueInRequiredNodeAffinity(oldPodSpec) // if old spec has invalid labelSelector in topologySpreadConstraint, we must allow it opts.AllowInvalidTopologySpreadConstraintLabelSelector = hasInvalidTopologySpreadConstraintLabelSelector(oldPodSpec) // if old spec has an invalid projected token volume path, we must allow it @@ -1271,6 +1273,16 @@ func IsRestartableInitContainer(initContainer *api.Container) bool { return *initContainer.RestartPolicy == api.ContainerRestartPolicyAlways } +func hasInvalidLabelValueInRequiredNodeAffinity(spec *api.PodSpec) bool { + if spec == nil || + spec.Affinity == nil || + spec.Affinity.NodeAffinity == nil || + spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution == nil { + return false + } + return helper.HasInvalidLabelValueInNodeSelectorTerms(spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms) +} + func MarkPodProposedForResize(oldPod, newPod *api.Pod) { if len(newPod.Spec.Containers) != len(oldPod.Spec.Containers) { // Update is invalid: ignore changes and let validation handle it diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 2f53c8fdc6e..97736682408 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -4204,3 +4204,77 @@ func TestValidateAllowSidecarResizePolicy(t *testing.T) { }) } } + +func TestValidateInvalidLabelValueInNodeSelectorOption(t *testing.T) { + testCases := []struct { + name string + oldPodSpec *api.PodSpec + wantOption bool + }{ + { + name: "Create", + wantOption: false, + }, + { + name: "UpdateInvalidLabelSelector", + oldPodSpec: &api.PodSpec{ + Affinity: &api.Affinity{ + NodeAffinity: &api.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{{ + MatchExpressions: []api.NodeSelectorRequirement{{ + Key: "foo", + Operator: api.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + }, + }, + }, + }, + wantOption: true, + }, + { + name: "UpdateValidLabelSelector", + oldPodSpec: &api.PodSpec{ + Affinity: &api.Affinity{ + NodeAffinity: &api.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{{ + MatchExpressions: []api.NodeSelectorRequirement{{ + Key: "foo", + Operator: api.NodeSelectorOpIn, + Values: []string{"bar"}, + }}, + }}, + }, + }, + }, + }, + wantOption: false, + }, + { + name: "UpdateEmptyLabelSelector", + oldPodSpec: &api.PodSpec{ + Affinity: &api.Affinity{ + NodeAffinity: &api.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{}, + }, + }, + }, + }, + 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.AllowInvalidLabelValueInRequiredNodeAffinity { + t.Errorf("Got AllowInvalidLabelValueInRequiredNodeAffinity=%t, want %t", gotOptions.AllowInvalidLabelValueInRequiredNodeAffinity, tc.wantOption) + } + }) + } +} diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index 84fb25a64d0..7ef6e99bdd7 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -96,12 +96,12 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta if n.RequiredDuringSchedulingIgnoredDuringExecution != nil { termFldPath := fieldPath.Child("spec", "affinity", "nodeAffinity", "requiredDuringSchedulingIgnoredDuringExecution", "nodeSelectorTerms") for i, term := range n.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms { - warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term, termFldPath.Index(i))...) + warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term, false, termFldPath.Index(i))...) } } preferredFldPath := fieldPath.Child("spec", "affinity", "nodeAffinity", "preferredDuringSchedulingIgnoredDuringExecution") for i, term := range n.PreferredDuringSchedulingIgnoredDuringExecution { - warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term.Preference, preferredFldPath.Index(i).Child("preference"))...) + warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term.Preference, true, preferredFldPath.Index(i).Child("preference"))...) } } for i, t := range podSpec.TopologySpreadConstraints { diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index b285a4b9f09..bea1b1df62a 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -1711,6 +1711,62 @@ func TestWarnings(t *testing.T) { `spec.containers[0].ports[1]: duplicate port name "test" with spec.initContainers[0].ports[0], services and probes that select ports by name will use spec.initContainers[0].ports[0]`, }, }, + { + name: "creating pod with invalid value in nodeaffinity", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Affinity: &api.Affinity{NodeAffinity: &api.NodeAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []api.PreferredSchedulingTerm{{ + Weight: 10, + Preference: api.NodeSelectorTerm{ + MatchExpressions: []api.NodeSelectorRequirement{{ + Key: "foo", + Operator: api.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }, + }}, + }}, + }}, + expected: []string{ + `spec.affinity.nodeAffinity.preferredDuringSchedulingIgnoredDuringExecution[0].preference.matchExpressions[0].values[0]: -1 is invalid, a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')`, + }, + }, + { + name: "updating pod with invalid value in nodeaffinity", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Affinity: &api.Affinity{NodeAffinity: &api.NodeAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []api.PreferredSchedulingTerm{{ + Weight: 10, + Preference: api.NodeSelectorTerm{ + MatchExpressions: []api.NodeSelectorRequirement{{ + Key: "foo", + Operator: api.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }, + }}, + }}, + SchedulingGates: []api.PodSchedulingGate{{Name: "foo"}}, + }}, + oldTemplate: &api.PodTemplateSpec{Spec: api.PodSpec{ + Affinity: &api.Affinity{NodeAffinity: &api.NodeAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []api.PreferredSchedulingTerm{{ + Weight: 10, + Preference: api.NodeSelectorTerm{ + MatchExpressions: []api.NodeSelectorRequirement{{ + Key: "foo", + Operator: api.NodeSelectorOpIn, + Values: []string{"bar"}, + }}, + }, + }}, + }}, + SchedulingGates: []api.PodSchedulingGate{{Name: "foo"}}, + }}, + expected: []string{ + `spec.affinity.nodeAffinity.preferredDuringSchedulingIgnoredDuringExecution[0].preference.matchExpressions[0].values[0]: -1 is invalid, a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')`, + }, + }, } for _, tc := range testcases { diff --git a/pkg/apis/core/helper/helpers.go b/pkg/apis/core/helper/helpers.go index 0724e6378bf..01913653083 100644 --- a/pkg/apis/core/helper/helpers.go +++ b/pkg/apis/core/helper/helpers.go @@ -500,3 +500,18 @@ func validFirstDigit(str string) bool { } return str[0] == '-' || (str[0] == '0' && str == "0") || (str[0] >= '1' && str[0] <= '9') } + +// HasInvalidLabelValueInNodeSelectorTerms checks if there's an invalid label value +// in one NodeSelectorTerm's MatchExpression values +func HasInvalidLabelValueInNodeSelectorTerms(terms []core.NodeSelectorTerm) bool { + for _, term := range terms { + for _, expression := range term.MatchExpressions { + for _, value := range expression.Values { + if len(validation.IsValidLabelValue(value)) > 0 { + return true + } + } + } + } + return false +} diff --git a/pkg/apis/core/helper/helpers_test.go b/pkg/apis/core/helper/helpers_test.go index 5f48b05b244..f920cb38cc4 100644 --- a/pkg/apis/core/helper/helpers_test.go +++ b/pkg/apis/core/helper/helpers_test.go @@ -369,3 +369,48 @@ func TestIsServiceIPSet(t *testing.T) { }) } } + +func TestHasInvalidLabelValueInNodeSelectorTerms(t *testing.T) { + testCases := []struct { + name string + terms []core.NodeSelectorTerm + expect bool + }{ + { + name: "valid values", + terms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "foo", + Operator: core.NodeSelectorOpIn, + Values: []string{"far"}, + }}, + }}, + expect: false, + }, + { + name: "empty terms", + terms: []core.NodeSelectorTerm{}, + expect: false, + }, + { + name: "invalid label value", + terms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "foo", + Operator: core.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + expect: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := HasInvalidLabelValueInNodeSelectorTerms(tc.terms) + if got != tc.expect { + t.Errorf("exepct %v, got %v", tc.expect, got) + } + }) + } +} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 694896ee750..89baa3d592e 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -1777,6 +1777,8 @@ var allowedTemplateObjectMetaFields = map[string]bool{ type PersistentVolumeSpecValidationOptions struct { // Allow users to modify the class of volume attributes EnableVolumeAttributesClass bool + // Allow invalid label-value in RequiredNodeSelector + AllowInvalidLabelValueInRequiredNodeAffinity bool } // ValidatePersistentVolumeName checks that a name is appropriate for a @@ -1798,11 +1800,17 @@ var supportedVolumeModes = sets.New(core.PersistentVolumeBlock, core.PersistentV func ValidationOptionsForPersistentVolume(pv, oldPv *core.PersistentVolume) PersistentVolumeSpecValidationOptions { opts := PersistentVolumeSpecValidationOptions{ - EnableVolumeAttributesClass: utilfeature.DefaultMutableFeatureGate.Enabled(features.VolumeAttributesClass), + EnableVolumeAttributesClass: utilfeature.DefaultMutableFeatureGate.Enabled(features.VolumeAttributesClass), + AllowInvalidLabelValueInRequiredNodeAffinity: false, } if oldPv != nil && oldPv.Spec.VolumeAttributesClassName != nil { opts.EnableVolumeAttributesClass = true } + if oldPv != nil && oldPv.Spec.NodeAffinity != nil && + oldPv.Spec.NodeAffinity.Required != nil { + terms := oldPv.Spec.NodeAffinity.Required.NodeSelectorTerms + opts.AllowInvalidLabelValueInRequiredNodeAffinity = helper.HasInvalidLabelValueInNodeSelectorTerms(terms) + } return opts } @@ -1874,7 +1882,7 @@ func ValidatePersistentVolumeSpec(pvSpec *core.PersistentVolumeSpec, pvName stri if validateInlinePersistentVolumeSpec { allErrs = append(allErrs, field.Forbidden(fldPath.Child("nodeAffinity"), "may not be specified in the context of inline volumes")) } else { - nodeAffinitySpecified, errs = validateVolumeNodeAffinity(pvSpec.NodeAffinity, fldPath.Child("nodeAffinity")) + nodeAffinitySpecified, errs = validateVolumeNodeAffinity(pvSpec.NodeAffinity, opts, fldPath.Child("nodeAffinity")) allErrs = append(allErrs, errs...) } } @@ -3865,7 +3873,7 @@ func validateAffinity(affinity *core.Affinity, opts PodValidationOptions, fldPat if affinity != nil { if affinity.NodeAffinity != nil { - allErrs = append(allErrs, validateNodeAffinity(affinity.NodeAffinity, fldPath.Child("nodeAffinity"))...) + allErrs = append(allErrs, validateNodeAffinity(affinity.NodeAffinity, opts, fldPath.Child("nodeAffinity"))...) } if affinity.PodAffinity != nil { allErrs = append(allErrs, validatePodAffinity(affinity.PodAffinity, opts.AllowInvalidLabelValueInSelector, fldPath.Child("podAffinity"))...) @@ -4053,6 +4061,8 @@ type PodValidationOptions struct { PodLevelResourcesEnabled bool // Allow sidecar containers resize policy for backward compatibility AllowSidecarResizePolicy bool + // Allow invalid label-value in RequiredNodeSelector + AllowInvalidLabelValueInRequiredNodeAffinity bool } // validatePodMetadataAndSpec tests if required fields in the pod.metadata and pod.spec are set, @@ -4730,14 +4740,14 @@ func validatePodAntiAffinity(podAntiAffinity *core.PodAntiAffinity, allowInvalid } // validateNodeAffinity tests that the specified nodeAffinity fields have valid data -func validateNodeAffinity(na *core.NodeAffinity, fldPath *field.Path) field.ErrorList { +func validateNodeAffinity(na *core.NodeAffinity, opts PodValidationOptions, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} // TODO: Uncomment the next three lines once RequiredDuringSchedulingRequiredDuringExecution is implemented. // if na.RequiredDuringSchedulingRequiredDuringExecution != nil { // allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingRequiredDuringExecution, fldPath.Child("requiredDuringSchedulingRequiredDuringExecution"))...) // } if na.RequiredDuringSchedulingIgnoredDuringExecution != nil { - allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingIgnoredDuringExecution, true /* TODO: opts.AllowInvalidLabelValueInRequiredNodeAffinity */, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...) + allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingIgnoredDuringExecution, opts.AllowInvalidLabelValueInRequiredNodeAffinity, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...) } if len(na.PreferredDuringSchedulingIgnoredDuringExecution) > 0 { allErrs = append(allErrs, ValidatePreferredSchedulingTerms(na.PreferredDuringSchedulingIgnoredDuringExecution, fldPath.Child("preferredDuringSchedulingIgnoredDuringExecution"))...) @@ -7783,7 +7793,7 @@ func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field. // returns: // - true if volumeNodeAffinity is set // - errorList if there are validation errors -func validateVolumeNodeAffinity(nodeAffinity *core.VolumeNodeAffinity, fldPath *field.Path) (bool, field.ErrorList) { +func validateVolumeNodeAffinity(nodeAffinity *core.VolumeNodeAffinity, opts PersistentVolumeSpecValidationOptions, fldPath *field.Path) (bool, field.ErrorList) { allErrs := field.ErrorList{} if nodeAffinity == nil { @@ -7791,7 +7801,7 @@ func validateVolumeNodeAffinity(nodeAffinity *core.VolumeNodeAffinity, fldPath * } if nodeAffinity.Required != nil { - allErrs = append(allErrs, ValidateNodeSelector(nodeAffinity.Required, true /* TODO: opts.AllowInvalidLabelValueInRequiredNodeAffinity */, fldPath.Child("required"))...) + allErrs = append(allErrs, ValidateNodeSelector(nodeAffinity.Required, opts.AllowInvalidLabelValueInRequiredNodeAffinity, fldPath.Child("required"))...) } else { allErrs = append(allErrs, field.Required(fldPath.Child("required"), "must specify required node constraints")) } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 7c70af236e8..76e3a1dfb18 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -704,9 +704,28 @@ func TestValidatePersistentVolumeSpec(t *testing.T) { MountOptions: []string{"soft", "read-write"}, }, }, + "invalid-node-affinity": { + isExpectedFailure: true, + isInlineSpec: false, + pvSpec: &core.PersistentVolumeSpec{ + NodeAffinity: &core.VolumeNodeAffinity{ + Required: &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "foo", + Operator: core.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + }, + }, + }, + }, } for name, scenario := range scenarios { - opts := PersistentVolumeSpecValidationOptions{} + opts := ValidationOptionsForPersistentVolume(&core.PersistentVolume{ + Spec: *scenario.pvSpec.DeepCopy(), + }, nil) errs := ValidatePersistentVolumeSpec(scenario.pvSpec, "", scenario.isInlineSpec, field.NewPath("field"), opts) if len(errs) == 0 && scenario.isExpectedFailure { t.Errorf("Unexpected success for scenario: %s", name) @@ -998,6 +1017,24 @@ func TestValidationOptionsForPersistentVolume(t *testing.T) { enableVolumeAttributesClass: false, expectValidationOpts: PersistentVolumeSpecValidationOptions{EnableVolumeAttributesClass: true}, }, + "old pv has invalid label-value in node affinity": { + oldPv: &core.PersistentVolume{ + Spec: core.PersistentVolumeSpec{ + NodeAffinity: &core.VolumeNodeAffinity{ + Required: &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "foo", + Operator: core.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + }, + }, + }, + }, + expectValidationOpts: PersistentVolumeSpecValidationOptions{AllowInvalidLabelValueInRequiredNodeAffinity: true}, + }, } for name, tc := range tests { @@ -1464,6 +1501,11 @@ func TestValidateVolumeNodeAffinityUpdate(t *testing.T) { oldPV: testVolumeWithNodeAffinity(simpleVolumeNodeAffinity("foo", "bar")), newPV: testVolumeWithNodeAffinity(nil), }, + "old affinity already has invalid label-value": { + isExpectedFailure: false, + oldPV: testVolumeWithNodeAffinity(simpleVolumeNodeAffinity(v1.LabelInstanceType, "-1")), + newPV: testVolumeWithNodeAffinity(simpleVolumeNodeAffinity(v1.LabelInstanceTypeStable, "-1")), + }, } for name, scenario := range scenarios { @@ -12242,6 +12284,23 @@ func TestValidatePod(t *testing.T) { podtest.SetAnnotations(map[string]string{core.PodDeletionCost: "+10"}), ), }, + "invalid required node affinity, value of NodeSelectorRequirement should be a valid label value": { + expectedError: "spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].values[0]: Invalid value: \"-1\": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')", + spec: *podtest.MakePod("123", + podtest.SetAffinity(&core.Affinity{ + NodeAffinity: &core.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "foo", + Operator: core.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + }}, + }), + ), + }, } for k, v := range errorCases { @@ -12316,6 +12375,7 @@ func TestValidatePodUpdate(t *testing.T) { old core.Pod new core.Pod err string + opts PodValidationOptions }{ {new: *podtest.MakePod(""), old: *podtest.MakePod(""), err: "", test: "nothing"}, { new: *podtest.MakePod("foo"), @@ -13669,6 +13729,73 @@ func TestValidatePodUpdate(t *testing.T) { err: "pod updates may not change fields other than", test: "the podAntiAffinity cannot be updated on gated pods", }, + { + old: *podtest.MakePod("foo", + podtest.SetAffinity(&core.Affinity{ + NodeAffinity: &core.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "expr", + Operator: core.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + }}, + }), + podtest.SetSchedulingGates(core.PodSchedulingGate{Name: "baz"}), + ), + new: *podtest.MakePod("foo", + podtest.SetAffinity(&core.Affinity{ + NodeAffinity: &core.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "expr", + Operator: core.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + }}, + }), + ), + test: "allow update pod if old pod already has invalid label-value in node affinity", + opts: PodValidationOptions{AllowInvalidLabelValueInRequiredNodeAffinity: true}, + }, + { + old: *podtest.MakePod("foo", + podtest.SetAffinity(&core.Affinity{ + NodeAffinity: &core.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "expr", + Operator: core.NodeSelectorOpIn, + Values: []string{"bar"}, + }}, + }}, + }}, + }), + podtest.SetSchedulingGates(core.PodSchedulingGate{Name: "baz"}), + ), + new: *podtest.MakePod("foo", + podtest.SetAffinity(&core.Affinity{ + NodeAffinity: &core.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "expr", + Operator: core.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + }}, + }), + podtest.SetSchedulingGates(core.PodSchedulingGate{Name: "baz"}), + ), + err: `a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.'`, + test: "not allow update node affinity to an invalid label-value", + }, { new: *podtest.MakePod("pod", podtest.SetContainers(podtest.MakeContainer("container", @@ -13730,7 +13857,7 @@ func TestValidatePodUpdate(t *testing.T) { test.old.Spec.RestartPolicy = "Always" } - errs := ValidatePodUpdate(&test.new, &test.old, PodValidationOptions{}) + errs := ValidatePodUpdate(&test.new, &test.old, test.opts) if test.err == "" { if len(errs) != 0 { t.Errorf("unexpected invalid: %s (%+v)\nA: %+v\nB: %+v", test.test, errs, test.new, test.old)