Update workload selector validation

pkg/api/pod/util.go

@@ -760,7 +760,6 @@ func SeccompAnnotationForField(field *api.SeccompProfile) string {
 	return ""
 }
 
-
 func hasInvalidLabelValueInAffinitySelector(spec *api.PodSpec) bool {
 	if spec.Affinity != nil {
 		if spec.Affinity.PodAffinity != nil {

pkg/apis/admissionregistration/validation/validation.go

@@ -688,13 +688,15 @@ func validateMatchResources(mc *admissionregistration.MatchResources, fldPath *f
 	if mc.NamespaceSelector == nil {
 		allErrors = append(allErrors, field.Required(fldPath.Child("namespaceSelector"), ""))
 	} else {
-		allErrors = append(allErrors, metav1validation.ValidateLabelSelector(mc.NamespaceSelector, fldPath.Child("namespaceSelector"))...)
+		// validate selector strictly, this type was released after issue #99139 was resolved
+		allErrors = append(allErrors, metav1validation.ValidateLabelSelector(mc.NamespaceSelector, metav1validation.LabelSelectorValidationOptions{}, fldPath.Child("namespaceSelector"))...)
 	}
 
 	if mc.ObjectSelector == nil {
 		allErrors = append(allErrors, field.Required(fldPath.Child("labelSelector"), ""))
 	} else {
-		allErrors = append(allErrors, metav1validation.ValidateLabelSelector(mc.ObjectSelector, fldPath.Child("labelSelector"))...)
+		// validate selector strictly, this type was released after issue #99139 was resolved
+		allErrors = append(allErrors, metav1validation.ValidateLabelSelector(mc.ObjectSelector, metav1validation.LabelSelectorValidationOptions{}, fldPath.Child("labelSelector"))...)
 	}
 
 	for i, namedRuleWithOperations := range mc.ResourceRules {

pkg/apis/apps/validation/validation.go

@@ -90,7 +90,6 @@ func ValidatePersistentVolumeClaimRetentionPolicy(policy *apps.StatefulSetPersis
 // ValidateStatefulSetSpec tests if required fields in the StatefulSet spec are set.
 func ValidateStatefulSetSpec(spec *apps.StatefulSetSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList {
 	allErrs := field.ErrorList{}
-	labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector}
 
 	switch spec.PodManagementPolicy {
 	case "":
@@ -133,7 +132,8 @@ func ValidateStatefulSetSpec(spec *apps.StatefulSetSpec, fldPath *field.Path, op
 	if spec.Selector == nil {
 		allErrs = append(allErrs, field.Required(fldPath.Child("selector"), ""))
 	} else {
-		allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOpts, fldPath.Child("selector"))...)
+		// validate selector strictly, spec.selector was always required to pass LabelSelectorAsSelector below
+		allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false}, fldPath.Child("selector"))...)
 		if len(spec.Selector.MatchLabels)+len(spec.Selector.MatchExpressions) == 0 {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), spec.Selector, "empty selector is invalid for statefulset"))
 		}
@@ -538,12 +538,12 @@ func ValidateRollback(rollback *apps.RollbackConfig, fldPath *field.Path) field.
 func ValidateDeploymentSpec(spec *apps.DeploymentSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList {
 	allErrs := field.ErrorList{}
 	allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(spec.Replicas), fldPath.Child("replicas"))...)
-	labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector}
 
 	if spec.Selector == nil {
 		allErrs = append(allErrs, field.Required(fldPath.Child("selector"), ""))
 	} else {
-		allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOpts, fldPath.Child("selector"))...)
+		// validate selector strictly, spec.selector was always required to pass LabelSelectorAsSelector below
+		allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false}, fldPath.Child("selector"))...)
 		if len(spec.Selector.MatchLabels)+len(spec.Selector.MatchExpressions) == 0 {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), spec.Selector, "empty selector is invalid for deployment"))
 		}
@@ -701,12 +701,12 @@ func ValidateReplicaSetSpec(spec *apps.ReplicaSetSpec, fldPath *field.Path, opts
 
 	allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(spec.Replicas), fldPath.Child("replicas"))...)
 	allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(spec.MinReadySeconds), fldPath.Child("minReadySeconds"))...)
-	labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector}
 
 	if spec.Selector == nil {
 		allErrs = append(allErrs, field.Required(fldPath.Child("selector"), ""))
 	} else {
-		allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOpts, fldPath.Child("selector"))...)
+		// validate selector strictly, spec.selector was always required to pass LabelSelectorAsSelector below
+		allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false}, fldPath.Child("selector"))...)
 		if len(spec.Selector.MatchLabels)+len(spec.Selector.MatchExpressions) == 0 {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), spec.Selector, "empty selector is invalid for deployment"))
 		}

pkg/apis/batch/validation/validation.go

@@ -146,12 +146,12 @@ func hasJobTrackingAnnotation(job *batch.Job) bool {
 // ValidateJobSpec validates a JobSpec and returns an ErrorList with any errors.
 func ValidateJobSpec(spec *batch.JobSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList {
 	allErrs := validateJobSpec(spec, fldPath, opts)
-	labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{
-		AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector,
-	}
 	if spec.Selector == nil {
 		allErrs = append(allErrs, field.Required(fldPath.Child("selector"), ""))
 	} else {
+		labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{
+			AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector,
+		}
 		allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOpts, fldPath.Child("selector"))...)
 	}
 

pkg/apis/core/validation/validation.go

@@ -2113,13 +2113,13 @@ func validateDataSource(dataSource *core.TypedLocalObjectReference, fldPath *fie
 // ValidatePersistentVolumeClaimSpec validates a PersistentVolumeClaimSpec
 func ValidatePersistentVolumeClaimSpec(spec *core.PersistentVolumeClaimSpec, fldPath *field.Path, opts PersistentVolumeClaimSpecValidationOptions) field.ErrorList {
 	allErrs := field.ErrorList{}
-	labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{
-		AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector,
-	}
 	if len(spec.AccessModes) == 0 {
 		allErrs = append(allErrs, field.Required(fldPath.Child("accessModes"), "at least 1 access mode is required"))
 	}
 	if spec.Selector != nil {
+		labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{
+			AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector,
+		}
 		allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOpts, fldPath.Child("selector"))...)
 	}
 

pkg/apis/networking/validation/validation.go

@@ -194,10 +194,10 @@ func ValidationOptionsForNetworking(new, old *networking.NetworkPolicy) NetworkP
 	opts := NetworkPolicyValidationOptions{
 		AllowInvalidLabelValueInSelector: false,
 	}
-	labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{
-		AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector,
-	}
 	if old != nil {
+		labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{
+			AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector,
+		}
 		if len(unversionedvalidation.ValidateLabelSelector(&old.Spec.PodSelector, labelSelectorValidationOpts, nil)) > 0 {
 			opts.AllowInvalidLabelValueInSelector = true
 		}

pkg/registry/apps/daemonset/strategy.go

@@ -22,6 +22,7 @@ import (
 	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apivalidation "k8s.io/apimachinery/pkg/api/validation"
+	metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -145,6 +146,8 @@ func (daemonSetStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Ob
 	oldDaemonSet := old.(*apps.DaemonSet)
 
 	opts := pod.GetValidationOptionsFromPodTemplate(&newDaemonSet.Spec.Template, &oldDaemonSet.Spec.Template)
+	opts.AllowInvalidLabelValueInSelector = opts.AllowInvalidLabelValueInSelector || metav1validation.LabelSelectorHasInvalidLabelValue(oldDaemonSet.Spec.Selector)
+
 	allErrs := validation.ValidateDaemonSet(obj.(*apps.DaemonSet), opts)
 	allErrs = append(allErrs, validation.ValidateDaemonSetUpdate(newDaemonSet, oldDaemonSet, opts)...)
 

pkg/registry/apps/daemonset/strategy_test.go

@@ -105,6 +105,19 @@ func TestSelectorImmutability(t *testing.T) {
 	}
 }
 
+func TestValidateToleratingBadLabels(t *testing.T) {
+	oldObj := newDaemonSetWithSelectorLabels(map[string]string{"a": "b"}, 1)
+	oldObj.Spec.Selector.MatchExpressions = []metav1.LabelSelectorRequirement{{Key: "key", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"bad value"}}}
+	newObj := newDaemonSetWithSelectorLabels(map[string]string{"a": "b"}, 1)
+	newObj.Spec.Selector.MatchExpressions = []metav1.LabelSelectorRequirement{{Key: "key", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"bad value"}}}
+
+	context := genericapirequest.NewContext()
+	errorList := daemonSetStrategy{}.ValidateUpdate(context, newObj, oldObj)
+	if len(errorList) > 0 {
+		t.Errorf("Unexpected error list with no-op update of bad object: %v", errorList)
+	}
+}
+
 func newDaemonSetWithSelectorLabels(selectorLabels map[string]string, templateGeneration int64) *apps.DaemonSet {
 	return &apps.DaemonSet{
 		ObjectMeta: metav1.ObjectMeta{

pkg/registry/batch/job/strategy.go

@@ -24,6 +24,7 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -169,6 +170,8 @@ func validationOptionsForJob(newJob, oldJob *batch.Job) validation.JobValidation
 		AllowTrackingAnnotation: true,
 	}
 	if oldJob != nil {
+		opts.AllowInvalidLabelValueInSelector = opts.AllowInvalidLabelValueInSelector || metav1validation.LabelSelectorHasInvalidLabelValue(oldJob.Spec.Selector)
+
 		// Because we don't support the tracking with finalizers for already
 		// existing jobs, we allow the annotation only if the Job already had it,
 		// regardless of the feature gate.
@@ -261,10 +264,6 @@ func (jobStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object)
 	oldJob := old.(*batch.Job)
 
 	opts := validationOptionsForJob(job, oldJob)
-	tempErrs := validation.ValidateJob(oldJob, opts)
-	if len(tempErrs) > 0 {
-		opts.AllowInvalidLabelValueInSelector = false
-	}
 	validationErrorList := validation.ValidateJob(job, opts)
 	updateErrorList := validation.ValidateJobUpdate(job, oldJob, opts)
 	return append(validationErrorList, updateErrorList...)

pkg/registry/batch/job/strategy_test.go

@@ -449,6 +449,31 @@ func TestJobStrategy(t *testing.T) {
 	}
 }
 
+func TestValidateToleratingBadLabels(t *testing.T) {
+	invalidSelector := getValidLabelSelector()
+	invalidSelector.MatchExpressions = []metav1.LabelSelectorRequirement{{Key: "key", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"bad value"}}}
+
+	validPodTemplateSpec := getValidPodTemplateSpecForSelector(getValidLabelSelector())
+	job := &batch.Job{
+		ObjectMeta: getValidObjectMeta(0),
+		Spec: batch.JobSpec{
+			Selector:       invalidSelector,
+			ManualSelector: pointer.BoolPtr(true),
+			Template:       validPodTemplateSpec,
+		},
+	}
+	job.ResourceVersion = "1"
+
+	oldObj := job.DeepCopy()
+	newObj := job.DeepCopy()
+
+	context := genericapirequest.NewContext()
+	errorList := Strategy.ValidateUpdate(context, newObj, oldObj)
+	if len(errorList) > 0 {
+		t.Errorf("Unexpected error list with no-op update of bad object: %v", errorList)
+	}
+}
+
 func TestJobStrategyValidateUpdate(t *testing.T) {
 	ctx := genericapirequest.NewDefaultContext()
 	validSelector := &metav1.LabelSelector{

pkg/registry/policy/poddisruptionbudget/strategy.go

@@ -111,7 +111,7 @@ func (podDisruptionBudgetStrategy) AllowCreateOnUpdate() bool {
 // ValidateUpdate is the default update validation for an end user.
 func (podDisruptionBudgetStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
 	opts := validation.PodDisruptionBudgetValidationOptions{
-		AllowInvalidLabelValueInSelector: allowValidateLabelValueInLabelSelector(old.(*policy.PodDisruptionBudget)),
+		AllowInvalidLabelValueInSelector: hasInvalidLabelValueInLabelSelector(old.(*policy.PodDisruptionBudget)),
 	}
 	return validation.ValidatePodDisruptionBudget(obj.(*policy.PodDisruptionBudget), opts)
 }
@@ -175,9 +175,9 @@ func (podDisruptionBudgetStatusStrategy) WarningsOnUpdate(ctx context.Context, o
 	return nil
 }
 
-func allowValidateLabelValueInLabelSelector(pdb *policy.PodDisruptionBudget) bool {
+func hasInvalidLabelValueInLabelSelector(pdb *policy.PodDisruptionBudget) bool {
-	labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false}
 	if pdb.Spec.Selector != nil {
+		labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false}
 		return len(metav1validation.ValidateLabelSelector(pdb.Spec.Selector, labelSelectorValidationOptions, nil)) > 0
 	}
 	return false

pkg/registry/rbac/clusterrole/strategy.go

@@ -91,7 +91,7 @@ func (strategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) fie
 	newObj := obj.(*rbac.ClusterRole)
 	oldObj := old.(*rbac.ClusterRole)
 	opts := validation.ClusterRoleValidationOptions{
-		AllowInvalidLabelValueInSelector: allowValidateLabelValueInLabelSelector(oldObj),
+		AllowInvalidLabelValueInSelector: hasInvalidLabelValueInLabelSelector(oldObj),
 	}
 	errorList := validation.ValidateClusterRole(newObj, opts)
 	return append(errorList, validation.ValidateClusterRoleUpdate(newObj, old.(*rbac.ClusterRole), opts)...)
@@ -111,9 +111,9 @@ func (strategy) AllowUnconditionalUpdate() bool {
 	return true
 }
 
-func allowValidateLabelValueInLabelSelector(role *rbac.ClusterRole) bool {
+func hasInvalidLabelValueInLabelSelector(role *rbac.ClusterRole) bool {
-	labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false}
 	if role.AggregationRule != nil {
+		labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false}
 		for _, selector := range role.AggregationRule.ClusterRoleSelectors {
 			if len(metav1validation.ValidateLabelSelector(&selector, labelSelectorValidationOptions, nil)) > 0 {
 				return true

pkg/registry/storage/csistoragecapacity/strategy.go

@@ -79,7 +79,7 @@ func (csiStorageCapacityStrategy) ValidateUpdate(ctx context.Context, obj, old r
 	newCSIStorageCapacityObj := obj.(*storage.CSIStorageCapacity)
 	oldCSIStorageCapacityObj := old.(*storage.CSIStorageCapacity)
 	opts := validation.CSIStorageCapacityValidateOptions{
-		AllowInvalidLabelValueInSelector: allowValidateLabelValueInLabelSelector(oldCSIStorageCapacityObj),
+		AllowInvalidLabelValueInSelector: hasInvalidLabelValueInLabelSelector(oldCSIStorageCapacityObj),
 	}
 	errorList := validation.ValidateCSIStorageCapacity(newCSIStorageCapacityObj, opts)
 	return append(errorList, validation.ValidateCSIStorageCapacityUpdate(newCSIStorageCapacityObj, oldCSIStorageCapacityObj)...)
@@ -94,7 +94,7 @@ func (csiStorageCapacityStrategy) AllowUnconditionalUpdate() bool {
 	return false
 }
 
-func allowValidateLabelValueInLabelSelector(capacity *storage.CSIStorageCapacity) bool {
+func hasInvalidLabelValueInLabelSelector(capacity *storage.CSIStorageCapacity) bool {
 	labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false}
 	return len(metav1validation.ValidateLabelSelector(capacity.NodeTopology, labelSelectorValidationOptions, nil)) > 0
 }

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go

@@ -34,6 +34,25 @@ type LabelSelectorValidationOptions struct {
 	AllowInvalidLabelValueInSelector bool
 }
 
+// LabelSelectorHasInvalidLabelValue returns true if the given selector contains an invalid label value in a match expression.
+// This is useful for determining whether AllowInvalidLabelValueInSelector should be set to true when validating an update
+// based on existing persisted invalid values.
+func LabelSelectorHasInvalidLabelValue(ps *metav1.LabelSelector) bool {
+	if ps == nil {
+		return false
+	}
+	for _, e := range ps.MatchExpressions {
+		for _, v := range e.Values {
+			if len(validation.IsValidLabelValue(v)) > 0 {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// ValidateLabelSelector validate the LabelSelector according to the opts and returns any validation errors.
+// opts.AllowInvalidLabelValueInSelector is only expected to be set to true when required for backwards compatibility with existing invalid data.
 func ValidateLabelSelector(ps *metav1.LabelSelector, opts LabelSelectorValidationOptions, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	if ps == nil {
@@ -46,6 +65,8 @@ func ValidateLabelSelector(ps *metav1.LabelSelector, opts LabelSelectorValidatio
 	return allErrs
 }
 
+// ValidateLabelSelectorRequirement validate the requirement according to the opts and returns any validation errors.
+// opts.AllowInvalidLabelValueInSelector is only expected to be set to true when required for backwards compatibility with existing invalid data.
 func ValidateLabelSelectorRequirement(sr metav1.LabelSelectorRequirement, opts LabelSelectorValidationOptions, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	switch sr.Operator {

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go

@@ -428,7 +428,6 @@ func TestValidateConditions(t *testing.T) {
 }
 
 func TestLabelSelectorMatchExpression(t *testing.T) {
-	// Success case
 	testCases := []struct {
 		name            string
 		labelSelector   *metav1.LabelSelector