From 0843c4dfcab93dd242044e51d6a2a21dc73d233e Mon Sep 17 00:00:00 2001 From: Manjusaka Date: Thu, 13 Jan 2022 22:10:10 +0800 Subject: [PATCH 1/2] Add extra value validation for matchExpression field in LabelSelector --- pkg/api/pod/util.go | 40 ++++++- .../validation/validation.go | 62 ++++++++++- pkg/apis/apps/validation/validation.go | 12 ++- pkg/apis/batch/validation/validation.go | 6 +- pkg/apis/core/validation/validation.go | 69 ++++++++---- pkg/apis/networking/validation/validation.go | 52 +++++++-- .../networking/validation/validation_test.go | 8 +- pkg/apis/policy/validation/validation.go | 16 ++- pkg/apis/policy/validation/validation_test.go | 8 +- pkg/apis/rbac/validation/validation.go | 14 ++- pkg/apis/rbac/validation/validation_test.go | 2 +- pkg/apis/storage/validation/validation.go | 9 +- .../storage/validation/validation_test.go | 2 +- pkg/registry/batch/job/strategy.go | 4 + .../networking/networkpolicy/strategy.go | 8 +- .../policy/poddisruptionbudget/strategy.go | 19 +++- pkg/registry/rbac/clusterrole/strategy.go | 26 ++++- .../storage/csistoragecapacity/strategy.go | 19 +++- .../pkg/apis/meta/v1/validation/validation.go | 19 +++- .../meta/v1/validation/validation_test.go | 102 ++++++++++++++++++ 20 files changed, 419 insertions(+), 78 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 1bdbd4523eb..00c52f6c47f 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -412,7 +412,8 @@ 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), + AllowExpandedDNSConfig: utilfeature.DefaultFeatureGate.Enabled(features.ExpandedDNSConfig) || haveSameExpandedDNSConfig(podSpec, oldPodSpec), + AllowInvalidLabelValueInSelector: false, } if oldPodSpec != nil { @@ -429,6 +430,8 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po // if old spec used non-integer multiple of huge page unit size, we must allow it opts.AllowIndivisibleHugePagesValues = usesIndivisibleHugePagesValues(oldPodSpec) + opts.AllowInvalidLabelValueInSelector = hasInvalidLabelValueInAffinitySelector(oldPodSpec) + } if oldPodMeta != nil && !opts.AllowInvalidPodDeletionCost { // This is an update, so validate only if the existing object was valid. @@ -756,3 +759,38 @@ func SeccompAnnotationForField(field *api.SeccompProfile) string { // type is specified return "" } + + +func hasInvalidLabelValueInAffinitySelector(spec *api.PodSpec) bool { + if spec.Affinity != nil { + if spec.Affinity.PodAffinity != nil { + for _, term := range spec.Affinity.PodAffinity.RequiredDuringSchedulingIgnoredDuringExecution { + allErrs := apivalidation.ValidatePodAffinityTermSelector(term, false, nil) + if len(allErrs) != 0 { + return true + } + } + for _, term := range spec.Affinity.PodAffinity.PreferredDuringSchedulingIgnoredDuringExecution { + allErrs := apivalidation.ValidatePodAffinityTermSelector(term.PodAffinityTerm, false, nil) + if len(allErrs) != 0 { + return true + } + } + } + if spec.Affinity.PodAntiAffinity != nil { + for _, term := range spec.Affinity.PodAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution { + allErrs := apivalidation.ValidatePodAffinityTermSelector(term, false, nil) + if len(allErrs) != 0 { + return true + } + } + for _, term := range spec.Affinity.PodAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution { + allErrs := apivalidation.ValidatePodAffinityTermSelector(term.PodAffinityTerm, false, nil) + if len(allErrs) != 0 { + return true + } + } + } + } + return false +} diff --git a/pkg/apis/admissionregistration/validation/validation.go b/pkg/apis/admissionregistration/validation/validation.go index 6a07b0b5f48..56effdf01e0 100644 --- a/pkg/apis/admissionregistration/validation/validation.go +++ b/pkg/apis/admissionregistration/validation/validation.go @@ -210,6 +210,7 @@ func ValidateValidatingWebhookConfiguration(e *admissionregistration.ValidatingW requireNoSideEffects: true, requireRecognizedAdmissionReviewVersion: true, requireUniqueWebhookNames: true, + allowInvalidLabelValueInSelector: false, }) } @@ -236,6 +237,7 @@ func ValidateMutatingWebhookConfiguration(e *admissionregistration.MutatingWebho requireNoSideEffects: true, requireRecognizedAdmissionReviewVersion: true, requireUniqueWebhookNames: true, + allowInvalidLabelValueInSelector: false, }) } @@ -243,10 +245,12 @@ type validationOptions struct { requireNoSideEffects bool requireRecognizedAdmissionReviewVersion bool requireUniqueWebhookNames bool + allowInvalidLabelValueInSelector bool } func validateMutatingWebhookConfiguration(e *admissionregistration.MutatingWebhookConfiguration, opts validationOptions) field.ErrorList { allErrors := genericvalidation.ValidateObjectMeta(&e.ObjectMeta, false, genericvalidation.NameIsDNSSubdomain, field.NewPath("metadata")) + hookNames := sets.NewString() for i, hook := range e.Webhooks { allErrors = append(allErrors, validateMutatingWebhook(&hook, opts, field.NewPath("webhooks").Index(i))...) @@ -266,6 +270,9 @@ func validateValidatingWebhook(hook *admissionregistration.ValidatingWebhook, op var allErrors field.ErrorList // hook.Name must be fully qualified allErrors = append(allErrors, utilvalidation.IsFullyQualifiedName(fldPath.Child("name"), hook.Name)...) + labelSelectorValidationOpts := metav1validation.LabelSelectorValidationOptions{ + AllowInvalidLabelValueInSelector: opts.allowInvalidLabelValueInSelector, + } for i, rule := range hook.Rules { allErrors = append(allErrors, validateRuleWithOperations(&rule, fldPath.Child("rules").Index(i))...) @@ -291,11 +298,11 @@ func validateValidatingWebhook(hook *admissionregistration.ValidatingWebhook, op } if hook.NamespaceSelector != nil { - allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.NamespaceSelector, fldPath.Child("namespaceSelector"))...) + allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.NamespaceSelector, labelSelectorValidationOpts, fldPath.Child("namespaceSelector"))...) } if hook.ObjectSelector != nil { - allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.ObjectSelector, fldPath.Child("objectSelector"))...) + allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.ObjectSelector, labelSelectorValidationOpts, fldPath.Child("objectSelector"))...) } cc := hook.ClientConfig @@ -314,6 +321,9 @@ func validateMutatingWebhook(hook *admissionregistration.MutatingWebhook, opts v var allErrors field.ErrorList // hook.Name must be fully qualified allErrors = append(allErrors, utilvalidation.IsFullyQualifiedName(fldPath.Child("name"), hook.Name)...) + labelSelectorValidationOpts := metav1validation.LabelSelectorValidationOptions{ + AllowInvalidLabelValueInSelector: opts.allowInvalidLabelValueInSelector, + } for i, rule := range hook.Rules { allErrors = append(allErrors, validateRuleWithOperations(&rule, fldPath.Child("rules").Index(i))...) @@ -339,10 +349,10 @@ func validateMutatingWebhook(hook *admissionregistration.MutatingWebhook, opts v } if hook.NamespaceSelector != nil { - allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.NamespaceSelector, fldPath.Child("namespaceSelector"))...) + allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.NamespaceSelector, labelSelectorValidationOpts, fldPath.Child("namespaceSelector"))...) } if hook.ObjectSelector != nil { - allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.ObjectSelector, fldPath.Child("objectSelector"))...) + allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.ObjectSelector, labelSelectorValidationOpts, fldPath.Child("objectSelector"))...) } if hook.ReinvocationPolicy != nil && !supportedReinvocationPolicies.Has(string(*hook.ReinvocationPolicy)) { allErrors = append(allErrors, field.NotSupported(fldPath.Child("reinvocationPolicy"), *hook.ReinvocationPolicy, supportedReinvocationPolicies.List())) @@ -508,12 +518,55 @@ func validatingHasNoSideEffects(webhooks []admissionregistration.ValidatingWebho return true } +// validatingWebhookAllowInvalidLabelValueInSelector returns true if all webhooksallow invalid label value in selector +func validatingWebhookHasInvalidLabelValueInSelector(webhooks []admissionregistration.ValidatingWebhook) bool { + labelSelectorValidationOpts := metav1validation.LabelSelectorValidationOptions{ + AllowInvalidLabelValueInSelector: false, + } + + for _, hook := range webhooks { + if hook.NamespaceSelector != nil { + if len(metav1validation.ValidateLabelSelector(hook.NamespaceSelector, labelSelectorValidationOpts, nil)) > 0 { + return true + } + } + if hook.ObjectSelector != nil { + if len(metav1validation.ValidateLabelSelector(hook.ObjectSelector, labelSelectorValidationOpts, nil)) > 0 { + return true + } + } + } + return false +} + +// mutatingWebhookAllowInvalidLabelValueInSelector returns true if all webhooks allow invalid label value in selector +func mutatingWebhookHasInvalidLabelValueInSelector(webhooks []admissionregistration.MutatingWebhook) bool { + labelSelectorValidationOpts := metav1validation.LabelSelectorValidationOptions{ + AllowInvalidLabelValueInSelector: false, + } + + for _, hook := range webhooks { + if hook.NamespaceSelector != nil { + if len(metav1validation.ValidateLabelSelector(hook.NamespaceSelector, labelSelectorValidationOpts, nil)) > 0 { + return true + } + } + if hook.ObjectSelector != nil { + if len(metav1validation.ValidateLabelSelector(hook.ObjectSelector, labelSelectorValidationOpts, nil)) > 0 { + return true + } + } + } + return false +} + // ValidateValidatingWebhookConfigurationUpdate validates update of validating webhook configuration func ValidateValidatingWebhookConfigurationUpdate(newC, oldC *admissionregistration.ValidatingWebhookConfiguration) field.ErrorList { return validateValidatingWebhookConfiguration(newC, validationOptions{ requireNoSideEffects: validatingHasNoSideEffects(oldC.Webhooks), requireRecognizedAdmissionReviewVersion: validatingHasAcceptedAdmissionReviewVersions(oldC.Webhooks), requireUniqueWebhookNames: validatingHasUniqueWebhookNames(oldC.Webhooks), + allowInvalidLabelValueInSelector: validatingWebhookHasInvalidLabelValueInSelector(oldC.Webhooks), }) } @@ -523,6 +576,7 @@ func ValidateMutatingWebhookConfigurationUpdate(newC, oldC *admissionregistratio requireNoSideEffects: mutatingHasNoSideEffects(oldC.Webhooks), requireRecognizedAdmissionReviewVersion: mutatingHasAcceptedAdmissionReviewVersions(oldC.Webhooks), requireUniqueWebhookNames: mutatingHasUniqueWebhookNames(oldC.Webhooks), + allowInvalidLabelValueInSelector: mutatingWebhookHasInvalidLabelValueInSelector(oldC.Webhooks), }) } diff --git a/pkg/apis/apps/validation/validation.go b/pkg/apis/apps/validation/validation.go index 89e644e07ce..48aa4d1980d 100644 --- a/pkg/apis/apps/validation/validation.go +++ b/pkg/apis/apps/validation/validation.go @@ -90,6 +90,7 @@ 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 "": @@ -132,7 +133,7 @@ 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, fldPath.Child("selector"))...) + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOpts, 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")) } @@ -336,8 +337,9 @@ func ValidateDaemonSetStatusUpdate(ds, oldDS *apps.DaemonSet) field.ErrorList { // ValidateDaemonSetSpec tests if required fields in the DaemonSetSpec are set. func ValidateDaemonSetSpec(spec *apps.DaemonSetSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} + labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector} - allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, fldPath.Child("selector"))...) + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOpts, fldPath.Child("selector"))...) selector, err := metav1.LabelSelectorAsSelector(spec.Selector) if err == nil && !selector.Matches(labels.Set(spec.Template.Labels)) { @@ -536,11 +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, fldPath.Child("selector"))...) + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOpts, 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")) } @@ -698,11 +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, fldPath.Child("selector"))...) + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOpts, 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")) } diff --git a/pkg/apis/batch/validation/validation.go b/pkg/apis/batch/validation/validation.go index 6ebb1a4fefd..8227d8bc3e4 100644 --- a/pkg/apis/batch/validation/validation.go +++ b/pkg/apis/batch/validation/validation.go @@ -146,11 +146,13 @@ 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 { - allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, fldPath.Child("selector"))...) + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOpts, fldPath.Child("selector"))...) } // Whether manually or automatically generated, the selector of the job must match the pods it will produce. diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index d658925937c..909d8fe0889 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -2029,6 +2029,8 @@ type PersistentVolumeClaimSpecValidationOptions struct { EnableRecoverFromExpansionFailure bool // Allow assigning StorageClass to unbound PVCs retroactively EnableRetroactiveDefaultStorageClass bool + // Allow to validate the label value of the label selector + AllowInvalidLabelValueInSelector bool } func ValidationOptionsForPersistentVolumeClaim(pvc, oldPvc *core.PersistentVolumeClaim) PersistentVolumeClaimSpecValidationOptions { @@ -2036,11 +2038,19 @@ func ValidationOptionsForPersistentVolumeClaim(pvc, oldPvc *core.PersistentVolum AllowReadWriteOncePod: utilfeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod), EnableRecoverFromExpansionFailure: utilfeature.DefaultFeatureGate.Enabled(features.RecoverVolumeExpansionFailure), EnableRetroactiveDefaultStorageClass: utilfeature.DefaultFeatureGate.Enabled(features.RetroactiveDefaultStorageClass), + AllowInvalidLabelValueInSelector: false, } if oldPvc == nil { // If there's no old PVC, use the options based solely on feature enablement return opts } + labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{ + AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector, + } + if len(unversionedvalidation.ValidateLabelSelector(oldPvc.Spec.Selector, labelSelectorValidationOpts, nil)) > 0 { + // If the old object had an invalid label selector, continue to allow it in the new object + opts.AllowInvalidLabelValueInSelector = true + } if helper.ContainsAccessMode(oldPvc.Spec.AccessModes, core.ReadWriteOncePod) { // If the old object allowed "ReadWriteOncePod", continue to allow it in the new object @@ -2051,12 +2061,20 @@ func ValidationOptionsForPersistentVolumeClaim(pvc, oldPvc *core.PersistentVolum func ValidationOptionsForPersistentVolumeClaimTemplate(claimTemplate, oldClaimTemplate *core.PersistentVolumeClaimTemplate) PersistentVolumeClaimSpecValidationOptions { opts := PersistentVolumeClaimSpecValidationOptions{ - AllowReadWriteOncePod: utilfeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod), + AllowReadWriteOncePod: utilfeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod), + AllowInvalidLabelValueInSelector: false, } if oldClaimTemplate == nil { // If there's no old PVC template, use the options based solely on feature enablement return opts } + labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{ + AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector, + } + if len(unversionedvalidation.ValidateLabelSelector(oldClaimTemplate.Spec.Selector, labelSelectorValidationOpts, nil)) > 0 { + // If the old object had an invalid label selector, continue to allow it in the new object + opts.AllowInvalidLabelValueInSelector = true + } if helper.ContainsAccessMode(oldClaimTemplate.Spec.AccessModes, core.ReadWriteOncePod) { // If the old object allowed "ReadWriteOncePod", continue to allow it in the new object opts.AllowReadWriteOncePod = true @@ -2095,11 +2113,14 @@ 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 { - allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, fldPath.Child("selector"))...) + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOpts, fldPath.Child("selector"))...) } expandedSupportedAccessModes := sets.StringKeySet(supportedAccessModes) @@ -3371,7 +3392,7 @@ func validateImagePullSecrets(imagePullSecrets []core.LocalObjectReference, fldP } // validateAffinity checks if given affinities are valid -func validateAffinity(affinity *core.Affinity, fldPath *field.Path) field.ErrorList { +func validateAffinity(affinity *core.Affinity, opts PodValidationOptions, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if affinity != nil { @@ -3379,10 +3400,10 @@ func validateAffinity(affinity *core.Affinity, fldPath *field.Path) field.ErrorL allErrs = append(allErrs, validateNodeAffinity(affinity.NodeAffinity, fldPath.Child("nodeAffinity"))...) } if affinity.PodAffinity != nil { - allErrs = append(allErrs, validatePodAffinity(affinity.PodAffinity, fldPath.Child("podAffinity"))...) + allErrs = append(allErrs, validatePodAffinity(affinity.PodAffinity, opts.AllowInvalidLabelValueInSelector, fldPath.Child("podAffinity"))...) } if affinity.PodAntiAffinity != nil { - allErrs = append(allErrs, validatePodAntiAffinity(affinity.PodAntiAffinity, fldPath.Child("podAntiAffinity"))...) + allErrs = append(allErrs, validatePodAntiAffinity(affinity.PodAntiAffinity, opts.AllowInvalidLabelValueInSelector, fldPath.Child("podAntiAffinity"))...) } } @@ -3543,6 +3564,8 @@ type PodValidationOptions struct { AllowDownwardAPIHugePages bool // Allow invalid pod-deletion-cost annotation value for backward compatibility. AllowInvalidPodDeletionCost bool + // Allow invalid label-value in LabelSelector + AllowInvalidLabelValueInSelector bool // Allow pod spec to use non-integer multiple of huge page unit size AllowIndivisibleHugePagesValues bool // Allow more DNSSearchPaths and longer DNSSearchListChars @@ -3646,7 +3669,7 @@ func ValidatePodSpec(spec *core.PodSpec, podMeta *metav1.ObjectMeta, fldPath *fi allErrs = append(allErrs, unversionedvalidation.ValidateLabels(spec.NodeSelector, fldPath.Child("nodeSelector"))...) allErrs = append(allErrs, ValidatePodSecurityContext(spec.SecurityContext, spec, fldPath, fldPath.Child("securityContext"), opts)...) allErrs = append(allErrs, validateImagePullSecrets(spec.ImagePullSecrets, fldPath.Child("imagePullSecrets"))...) - allErrs = append(allErrs, validateAffinity(spec.Affinity, fldPath.Child("affinity"))...) + allErrs = append(allErrs, validateAffinity(spec.Affinity, opts, fldPath.Child("affinity"))...) 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"))...) @@ -4010,12 +4033,10 @@ func ValidatePreferredSchedulingTerms(terms []core.PreferredSchedulingTerm, fldP } // validatePodAffinityTerm tests that the specified podAffinityTerm fields have valid data -func validatePodAffinityTerm(podAffinityTerm core.PodAffinityTerm, fldPath *field.Path) field.ErrorList { +func validatePodAffinityTerm(podAffinityTerm core.PodAffinityTerm, allowInvalidLabelValueInSelector bool, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(podAffinityTerm.LabelSelector, fldPath.Child("labelSelector"))...) - allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(podAffinityTerm.NamespaceSelector, fldPath.Child("namespaceSelector"))...) - + allErrs = append(allErrs, ValidatePodAffinityTermSelector(podAffinityTerm, allowInvalidLabelValueInSelector, fldPath)...) for _, name := range podAffinityTerm.Namespaces { for _, msg := range ValidateNamespaceName(name, false) { allErrs = append(allErrs, field.Invalid(fldPath.Child("namespace"), name, msg)) @@ -4028,28 +4049,28 @@ func validatePodAffinityTerm(podAffinityTerm core.PodAffinityTerm, fldPath *fiel } // validatePodAffinityTerms tests that the specified podAffinityTerms fields have valid data -func validatePodAffinityTerms(podAffinityTerms []core.PodAffinityTerm, fldPath *field.Path) field.ErrorList { +func validatePodAffinityTerms(podAffinityTerms []core.PodAffinityTerm, allowInvalidLabelValueInSelector bool, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for i, podAffinityTerm := range podAffinityTerms { - allErrs = append(allErrs, validatePodAffinityTerm(podAffinityTerm, fldPath.Index(i))...) + allErrs = append(allErrs, validatePodAffinityTerm(podAffinityTerm, allowInvalidLabelValueInSelector, fldPath.Index(i))...) } return allErrs } // validateWeightedPodAffinityTerms tests that the specified weightedPodAffinityTerms fields have valid data -func validateWeightedPodAffinityTerms(weightedPodAffinityTerms []core.WeightedPodAffinityTerm, fldPath *field.Path) field.ErrorList { +func validateWeightedPodAffinityTerms(weightedPodAffinityTerms []core.WeightedPodAffinityTerm, allowInvalidLabelValueInSelector bool, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for j, weightedTerm := range weightedPodAffinityTerms { if weightedTerm.Weight <= 0 || weightedTerm.Weight > 100 { allErrs = append(allErrs, field.Invalid(fldPath.Index(j).Child("weight"), weightedTerm.Weight, "must be in the range 1-100")) } - allErrs = append(allErrs, validatePodAffinityTerm(weightedTerm.PodAffinityTerm, fldPath.Index(j).Child("podAffinityTerm"))...) + allErrs = append(allErrs, validatePodAffinityTerm(weightedTerm.PodAffinityTerm, allowInvalidLabelValueInSelector, fldPath.Index(j).Child("podAffinityTerm"))...) } return allErrs } // validatePodAntiAffinity tests that the specified podAntiAffinity fields have valid data -func validatePodAntiAffinity(podAntiAffinity *core.PodAntiAffinity, fldPath *field.Path) field.ErrorList { +func validatePodAntiAffinity(podAntiAffinity *core.PodAntiAffinity, allowInvalidLabelValueInSelector bool, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} // TODO:Uncomment below code once RequiredDuringSchedulingRequiredDuringExecution is implemented. // if podAntiAffinity.RequiredDuringSchedulingRequiredDuringExecution != nil { @@ -4057,11 +4078,11 @@ func validatePodAntiAffinity(podAntiAffinity *core.PodAntiAffinity, fldPath *fie // fldPath.Child("requiredDuringSchedulingRequiredDuringExecution"))...) // } if podAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil { - allErrs = append(allErrs, validatePodAffinityTerms(podAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution, + allErrs = append(allErrs, validatePodAffinityTerms(podAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution, allowInvalidLabelValueInSelector, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...) } if podAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution != nil { - allErrs = append(allErrs, validateWeightedPodAffinityTerms(podAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution, + allErrs = append(allErrs, validateWeightedPodAffinityTerms(podAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution, allowInvalidLabelValueInSelector, fldPath.Child("preferredDuringSchedulingIgnoredDuringExecution"))...) } return allErrs @@ -4084,7 +4105,7 @@ func validateNodeAffinity(na *core.NodeAffinity, fldPath *field.Path) field.Erro } // validatePodAffinity tests that the specified podAffinity fields have valid data -func validatePodAffinity(podAffinity *core.PodAffinity, fldPath *field.Path) field.ErrorList { +func validatePodAffinity(podAffinity *core.PodAffinity, allowInvalidLabelValueInSelector bool, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} // TODO:Uncomment below code once RequiredDuringSchedulingRequiredDuringExecution is implemented. // if podAffinity.RequiredDuringSchedulingRequiredDuringExecution != nil { @@ -4092,11 +4113,11 @@ func validatePodAffinity(podAffinity *core.PodAffinity, fldPath *field.Path) fie // fldPath.Child("requiredDuringSchedulingRequiredDuringExecution"))...) // } if podAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil { - allErrs = append(allErrs, validatePodAffinityTerms(podAffinity.RequiredDuringSchedulingIgnoredDuringExecution, + allErrs = append(allErrs, validatePodAffinityTerms(podAffinity.RequiredDuringSchedulingIgnoredDuringExecution, allowInvalidLabelValueInSelector, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...) } if podAffinity.PreferredDuringSchedulingIgnoredDuringExecution != nil { - allErrs = append(allErrs, validateWeightedPodAffinityTerms(podAffinity.PreferredDuringSchedulingIgnoredDuringExecution, + allErrs = append(allErrs, validateWeightedPodAffinityTerms(podAffinity.PreferredDuringSchedulingIgnoredDuringExecution, allowInvalidLabelValueInSelector, fldPath.Child("preferredDuringSchedulingIgnoredDuringExecution"))...) } return allErrs @@ -7022,3 +7043,11 @@ func sameLoadBalancerClass(oldService, service *core.Service) bool { } return *oldService.Spec.LoadBalancerClass == *service.Spec.LoadBalancerClass } + +func ValidatePodAffinityTermSelector(podAffinityTerm core.PodAffinityTerm, allowInvalidLabelValueInSelector bool, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + labelSelectorValidationOptions := unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: allowInvalidLabelValueInSelector} + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(podAffinityTerm.LabelSelector, labelSelectorValidationOptions, fldPath.Child("labelSelector"))...) + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(podAffinityTerm.NamespaceSelector, labelSelectorValidationOptions, fldPath.Child("namespaceSelector"))...) + return allErrs +} diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index 4238a3af76c..7fcba85fb82 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/pkg/apis/networking/validation/validation.go @@ -55,6 +55,10 @@ var ( ) ) +type NetworkPolicyValidationOptions struct { + AllowInvalidLabelValueInSelector bool +} + // ValidateNetworkPolicyName can be used to check whether the given networkpolicy // name is valid. func ValidateNetworkPolicyName(name string, prefix bool) []string { @@ -98,17 +102,20 @@ func ValidateNetworkPolicyPort(port *networking.NetworkPolicyPort, portPath *fie } // ValidateNetworkPolicyPeer validates a NetworkPolicyPeer -func ValidateNetworkPolicyPeer(peer *networking.NetworkPolicyPeer, peerPath *field.Path) field.ErrorList { +func ValidateNetworkPolicyPeer(peer *networking.NetworkPolicyPeer, opts NetworkPolicyValidationOptions, peerPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} numPeers := 0 + labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{ + AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector, + } if peer.PodSelector != nil { numPeers++ - allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(peer.PodSelector, peerPath.Child("podSelector"))...) + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(peer.PodSelector, labelSelectorValidationOpts, peerPath.Child("podSelector"))...) } if peer.NamespaceSelector != nil { numPeers++ - allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(peer.NamespaceSelector, peerPath.Child("namespaceSelector"))...) + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(peer.NamespaceSelector, labelSelectorValidationOpts, peerPath.Child("namespaceSelector"))...) } if peer.IPBlock != nil { numPeers++ @@ -125,9 +132,16 @@ func ValidateNetworkPolicyPeer(peer *networking.NetworkPolicyPeer, peerPath *fie } // ValidateNetworkPolicySpec tests if required fields in the networkpolicy spec are set. -func ValidateNetworkPolicySpec(spec *networking.NetworkPolicySpec, fldPath *field.Path) field.ErrorList { +func ValidateNetworkPolicySpec(spec *networking.NetworkPolicySpec, opts NetworkPolicyValidationOptions, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(&spec.PodSelector, fldPath.Child("podSelector"))...) + labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{ + AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector, + } + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector( + &spec.PodSelector, + labelSelectorValidationOpts, + fldPath.Child("podSelector"), + )...) // Validate ingress rules. for i, ingress := range spec.Ingress { @@ -138,7 +152,7 @@ func ValidateNetworkPolicySpec(spec *networking.NetworkPolicySpec, fldPath *fiel } for i, from := range ingress.From { fromPath := ingressPath.Child("from").Index(i) - allErrs = append(allErrs, ValidateNetworkPolicyPeer(&from, fromPath)...) + allErrs = append(allErrs, ValidateNetworkPolicyPeer(&from, opts, fromPath)...) } } // Validate egress rules @@ -150,7 +164,7 @@ func ValidateNetworkPolicySpec(spec *networking.NetworkPolicySpec, fldPath *fiel } for i, to := range egress.To { toPath := egressPath.Child("to").Index(i) - allErrs = append(allErrs, ValidateNetworkPolicyPeer(&to, toPath)...) + allErrs = append(allErrs, ValidateNetworkPolicyPeer(&to, opts, toPath)...) } } // Validate PolicyTypes @@ -169,17 +183,33 @@ func ValidateNetworkPolicySpec(spec *networking.NetworkPolicySpec, fldPath *fiel } // ValidateNetworkPolicy validates a networkpolicy. -func ValidateNetworkPolicy(np *networking.NetworkPolicy) field.ErrorList { +func ValidateNetworkPolicy(np *networking.NetworkPolicy, opts NetworkPolicyValidationOptions) field.ErrorList { allErrs := apivalidation.ValidateObjectMeta(&np.ObjectMeta, true, ValidateNetworkPolicyName, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateNetworkPolicySpec(&np.Spec, field.NewPath("spec"))...) + allErrs = append(allErrs, ValidateNetworkPolicySpec(&np.Spec, opts, field.NewPath("spec"))...) return allErrs } +// ValidationOptionsForNetworking generates NetworkPolicyValidationOptions for Networking +func ValidationOptionsForNetworking(new, old *networking.NetworkPolicy) NetworkPolicyValidationOptions { + opts := NetworkPolicyValidationOptions{ + AllowInvalidLabelValueInSelector: false, + } + labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{ + AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector, + } + if old != nil { + if len(unversionedvalidation.ValidateLabelSelector(&old.Spec.PodSelector, labelSelectorValidationOpts, nil)) > 0 { + opts.AllowInvalidLabelValueInSelector = true + } + } + return opts +} + // ValidateNetworkPolicyUpdate tests if an update to a NetworkPolicy is valid. -func ValidateNetworkPolicyUpdate(update, old *networking.NetworkPolicy) field.ErrorList { +func ValidateNetworkPolicyUpdate(update, old *networking.NetworkPolicy, opts NetworkPolicyValidationOptions) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&update.ObjectMeta, &old.ObjectMeta, field.NewPath("metadata"))...) - allErrs = append(allErrs, ValidateNetworkPolicySpec(&update.Spec, field.NewPath("spec"))...) + allErrs = append(allErrs, ValidateNetworkPolicySpec(&update.Spec, opts, field.NewPath("spec"))...) return allErrs } diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index c88f70b1135..2b116292e63 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/pkg/apis/networking/validation/validation_test.go @@ -251,7 +251,7 @@ func TestValidateNetworkPolicy(t *testing.T) { // Success cases are expected to pass validation. for k, v := range successCases { - if errs := ValidateNetworkPolicy(v); len(errs) != 0 { + if errs := ValidateNetworkPolicy(v, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) != 0 { t.Errorf("Expected success for the success validation test number %d, got %v", k, errs) } } @@ -368,7 +368,7 @@ func TestValidateNetworkPolicy(t *testing.T) { // Error cases are not expected to pass validation. for testName, networkPolicy := range errorCases { - if errs := ValidateNetworkPolicy(networkPolicy); len(errs) == 0 { + if errs := ValidateNetworkPolicy(networkPolicy, NetworkPolicyValidationOptions{AllowInvalidLabelValueInSelector: true}); len(errs) == 0 { t.Errorf("Expected failure for test: %s", testName) } } @@ -423,7 +423,7 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { for testName, successCase := range successCases { successCase.old.ObjectMeta.ResourceVersion = "1" successCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old); len(errs) != 0 { + if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{false}); len(errs) != 0 { t.Errorf("expected success (%s): %v", testName, errs) } } @@ -450,7 +450,7 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) { for testName, errorCase := range errorCases { errorCase.old.ObjectMeta.ResourceVersion = "1" errorCase.update.ObjectMeta.ResourceVersion = "1" - if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old); len(errs) == 0 { + if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{false}); len(errs) == 0 { t.Errorf("expected failure: %s", testName) } } diff --git a/pkg/apis/policy/validation/validation.go b/pkg/apis/policy/validation/validation.go index 57a0d46ea62..fb529b3d6bc 100644 --- a/pkg/apis/policy/validation/validation.go +++ b/pkg/apis/policy/validation/validation.go @@ -30,7 +30,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" appsvalidation "k8s.io/kubernetes/pkg/apis/apps/validation" - core "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/apis/core" apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" "k8s.io/kubernetes/pkg/apis/policy" ) @@ -44,16 +44,20 @@ const ( seccompAllowedProfilesAnnotationKey = "seccomp.security.alpha.kubernetes.io/allowedProfileNames" ) +type PodDisruptionBudgetValidationOptions struct { + AllowInvalidLabelValueInSelector bool +} + // ValidatePodDisruptionBudget validates a PodDisruptionBudget and returns an ErrorList // with any errors. -func ValidatePodDisruptionBudget(pdb *policy.PodDisruptionBudget) field.ErrorList { - allErrs := ValidatePodDisruptionBudgetSpec(pdb.Spec, field.NewPath("spec")) +func ValidatePodDisruptionBudget(pdb *policy.PodDisruptionBudget, opts PodDisruptionBudgetValidationOptions) field.ErrorList { + allErrs := ValidatePodDisruptionBudgetSpec(pdb.Spec, opts, field.NewPath("spec")) return allErrs } // ValidatePodDisruptionBudgetSpec validates a PodDisruptionBudgetSpec and returns an ErrorList // with any errors. -func ValidatePodDisruptionBudgetSpec(spec policy.PodDisruptionBudgetSpec, fldPath *field.Path) field.ErrorList { +func ValidatePodDisruptionBudgetSpec(spec policy.PodDisruptionBudgetSpec, opts PodDisruptionBudgetValidationOptions, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if spec.MinAvailable != nil && spec.MaxUnavailable != nil { @@ -70,7 +74,9 @@ func ValidatePodDisruptionBudgetSpec(spec policy.PodDisruptionBudgetSpec, fldPat allErrs = append(allErrs, appsvalidation.IsNotMoreThan100Percent(*spec.MaxUnavailable, fldPath.Child("maxUnavailable"))...) } - allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, fldPath.Child("selector"))...) + labelSelectorValidationOptions := unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector} + + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOptions, fldPath.Child("selector"))...) return allErrs } diff --git a/pkg/apis/policy/validation/validation_test.go b/pkg/apis/policy/validation/validation_test.go index 546836daed8..0902328d4f3 100644 --- a/pkg/apis/policy/validation/validation_test.go +++ b/pkg/apis/policy/validation/validation_test.go @@ -41,7 +41,7 @@ func TestValidatePodDisruptionBudgetSpec(t *testing.T) { MinAvailable: &minAvailable, MaxUnavailable: &maxUnavailable, } - errs := ValidatePodDisruptionBudgetSpec(spec, field.NewPath("foo")) + errs := ValidatePodDisruptionBudgetSpec(spec, PodDisruptionBudgetValidationOptions{true}, field.NewPath("foo")) if len(errs) == 0 { t.Errorf("unexpected success for %v", spec) } @@ -60,7 +60,7 @@ func TestValidateMinAvailablePodDisruptionBudgetSpec(t *testing.T) { spec := policy.PodDisruptionBudgetSpec{ MinAvailable: &c, } - errs := ValidatePodDisruptionBudgetSpec(spec, field.NewPath("foo")) + errs := ValidatePodDisruptionBudgetSpec(spec, PodDisruptionBudgetValidationOptions{true}, field.NewPath("foo")) if len(errs) != 0 { t.Errorf("unexpected failure %v for %v", errs, spec) } @@ -77,7 +77,7 @@ func TestValidateMinAvailablePodDisruptionBudgetSpec(t *testing.T) { spec := policy.PodDisruptionBudgetSpec{ MinAvailable: &c, } - errs := ValidatePodDisruptionBudgetSpec(spec, field.NewPath("foo")) + errs := ValidatePodDisruptionBudgetSpec(spec, PodDisruptionBudgetValidationOptions{true}, field.NewPath("foo")) if len(errs) == 0 { t.Errorf("unexpected success for %v", spec) } @@ -92,7 +92,7 @@ func TestValidateMinAvailablePodAndMaxUnavailableDisruptionBudgetSpec(t *testing MinAvailable: &c1, MaxUnavailable: &c2, } - errs := ValidatePodDisruptionBudgetSpec(spec, field.NewPath("foo")) + errs := ValidatePodDisruptionBudgetSpec(spec, PodDisruptionBudgetValidationOptions{true}, field.NewPath("foo")) if len(errs) == 0 { t.Errorf("unexpected success for %v", spec) } diff --git a/pkg/apis/rbac/validation/validation.go b/pkg/apis/rbac/validation/validation.go index 1cc6f680c05..b65ac19f72e 100644 --- a/pkg/apis/rbac/validation/validation.go +++ b/pkg/apis/rbac/validation/validation.go @@ -55,7 +55,11 @@ func ValidateRoleUpdate(role *rbac.Role, oldRole *rbac.Role) field.ErrorList { return allErrs } -func ValidateClusterRole(role *rbac.ClusterRole) field.ErrorList { +type ClusterRoleValidationOptions struct { + AllowInvalidLabelValueInSelector bool +} + +func ValidateClusterRole(role *rbac.ClusterRole, opts ClusterRoleValidationOptions) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, validation.ValidateObjectMeta(&role.ObjectMeta, false, ValidateRBACName, field.NewPath("metadata"))...) @@ -65,13 +69,15 @@ func ValidateClusterRole(role *rbac.ClusterRole) field.ErrorList { } } + labelSelectorValidationOptions := unversionedvalidation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector} + if role.AggregationRule != nil { if len(role.AggregationRule.ClusterRoleSelectors) == 0 { allErrs = append(allErrs, field.Required(field.NewPath("aggregationRule", "clusterRoleSelectors"), "at least one clusterRoleSelector required if aggregationRule is non-nil")) } for i, selector := range role.AggregationRule.ClusterRoleSelectors { fieldPath := field.NewPath("aggregationRule", "clusterRoleSelectors").Index(i) - allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(&selector, fieldPath)...) + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(&selector, labelSelectorValidationOptions, fieldPath)...) selector, err := metav1.LabelSelectorAsSelector(&selector) if err != nil { @@ -86,8 +92,8 @@ func ValidateClusterRole(role *rbac.ClusterRole) field.ErrorList { return nil } -func ValidateClusterRoleUpdate(role *rbac.ClusterRole, oldRole *rbac.ClusterRole) field.ErrorList { - allErrs := ValidateClusterRole(role) +func ValidateClusterRoleUpdate(role *rbac.ClusterRole, oldRole *rbac.ClusterRole, opts ClusterRoleValidationOptions) field.ErrorList { + allErrs := ValidateClusterRole(role, opts) allErrs = append(allErrs, validation.ValidateObjectMetaUpdate(&role.ObjectMeta, &oldRole.ObjectMeta, field.NewPath("metadata"))...) return allErrs diff --git a/pkg/apis/rbac/validation/validation_test.go b/pkg/apis/rbac/validation/validation_test.go index d460c4d424d..76a9ece8142 100644 --- a/pkg/apis/rbac/validation/validation_test.go +++ b/pkg/apis/rbac/validation/validation_test.go @@ -330,7 +330,7 @@ type ValidateClusterRoleTest struct { } func (v ValidateClusterRoleTest) test(t *testing.T) { - errs := ValidateClusterRole(&v.role) + errs := ValidateClusterRole(&v.role, ClusterRoleValidationOptions{false}) if len(errs) == 0 { if v.wantErr { t.Fatal("expected validation error") diff --git a/pkg/apis/storage/validation/validation.go b/pkg/apis/storage/validation/validation.go index 4497f1e5513..73392232289 100644 --- a/pkg/apis/storage/validation/validation.go +++ b/pkg/apis/storage/validation/validation.go @@ -537,10 +537,15 @@ func validateVolumeLifecycleModes(modes []storage.VolumeLifecycleMode, fldPath * // CSIStorageCapacity object. var ValidateStorageCapacityName = apimachineryvalidation.NameIsDNSSubdomain +type CSIStorageCapacityValidateOptions struct { + AllowInvalidLabelValueInSelector bool +} + // ValidateCSIStorageCapacity validates a CSIStorageCapacity. -func ValidateCSIStorageCapacity(capacity *storage.CSIStorageCapacity) field.ErrorList { +func ValidateCSIStorageCapacity(capacity *storage.CSIStorageCapacity, opts CSIStorageCapacityValidateOptions) field.ErrorList { allErrs := apivalidation.ValidateObjectMeta(&capacity.ObjectMeta, true, ValidateStorageCapacityName, field.NewPath("metadata")) - allErrs = append(allErrs, metav1validation.ValidateLabelSelector(capacity.NodeTopology, field.NewPath("nodeTopology"))...) + labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector} + allErrs = append(allErrs, metav1validation.ValidateLabelSelector(capacity.NodeTopology, labelSelectorValidationOptions, field.NewPath("nodeTopology"))...) for _, msg := range apivalidation.ValidateClassName(capacity.StorageClassName, false) { allErrs = append(allErrs, field.Invalid(field.NewPath("storageClassName"), capacity.StorageClassName, msg)) } diff --git a/pkg/apis/storage/validation/validation_test.go b/pkg/apis/storage/validation/validation_test.go index 2c21554569f..9aa457a4da5 100644 --- a/pkg/apis/storage/validation/validation_test.go +++ b/pkg/apis/storage/validation/validation_test.go @@ -2179,7 +2179,7 @@ func TestValidateCSIStorageCapacity(t *testing.T) { for name, scenario := range scenarios { t.Run(name, func(t *testing.T) { - errs := ValidateCSIStorageCapacity(scenario.capacity) + errs := ValidateCSIStorageCapacity(scenario.capacity, CSIStorageCapacityValidateOptions{false}) if len(errs) == 0 && scenario.isExpectedFailure { t.Errorf("Unexpected success") } diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index 51c8b6e58a3..b7f38920124 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/pkg/registry/batch/job/strategy.go @@ -261,6 +261,10 @@ 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...) diff --git a/pkg/registry/networking/networkpolicy/strategy.go b/pkg/registry/networking/networkpolicy/strategy.go index 71d9b260301..692ce54168d 100644 --- a/pkg/registry/networking/networkpolicy/strategy.go +++ b/pkg/registry/networking/networkpolicy/strategy.go @@ -96,7 +96,8 @@ func (networkPolicyStrategy) PrepareForUpdate(ctx context.Context, obj, old runt // Validate validates a new NetworkPolicy. func (networkPolicyStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { networkPolicy := obj.(*networking.NetworkPolicy) - return validation.ValidateNetworkPolicy(networkPolicy) + ops := validation.ValidationOptionsForNetworking(networkPolicy, nil) + return validation.ValidateNetworkPolicy(networkPolicy, ops) } // WarningsOnCreate returns warnings for the creation of the given object. @@ -114,8 +115,9 @@ func (networkPolicyStrategy) AllowCreateOnUpdate() bool { // ValidateUpdate is the default update validation for an end user. func (networkPolicyStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { - validationErrorList := validation.ValidateNetworkPolicy(obj.(*networking.NetworkPolicy)) - updateErrorList := validation.ValidateNetworkPolicyUpdate(obj.(*networking.NetworkPolicy), old.(*networking.NetworkPolicy)) + opts := validation.ValidationOptionsForNetworking(obj.(*networking.NetworkPolicy), old.(*networking.NetworkPolicy)) + validationErrorList := validation.ValidateNetworkPolicy(obj.(*networking.NetworkPolicy), opts) + updateErrorList := validation.ValidateNetworkPolicyUpdate(obj.(*networking.NetworkPolicy), old.(*networking.NetworkPolicy), opts) return append(validationErrorList, updateErrorList...) } diff --git a/pkg/registry/policy/poddisruptionbudget/strategy.go b/pkg/registry/policy/poddisruptionbudget/strategy.go index d5fc502c5e8..bf89a2d20ad 100644 --- a/pkg/registry/policy/poddisruptionbudget/strategy.go +++ b/pkg/registry/policy/poddisruptionbudget/strategy.go @@ -20,6 +20,7 @@ import ( "context" apiequality "k8s.io/apimachinery/pkg/api/equality" + 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" @@ -87,7 +88,10 @@ func (podDisruptionBudgetStrategy) PrepareForUpdate(ctx context.Context, obj, ol // Validate validates a new PodDisruptionBudget. func (podDisruptionBudgetStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { podDisruptionBudget := obj.(*policy.PodDisruptionBudget) - return validation.ValidatePodDisruptionBudget(podDisruptionBudget) + opts := validation.PodDisruptionBudgetValidationOptions{ + AllowInvalidLabelValueInSelector: false, + } + return validation.ValidatePodDisruptionBudget(podDisruptionBudget, opts) } // WarningsOnCreate returns warnings for the creation of the given object. @@ -106,7 +110,10 @@ 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 { - return validation.ValidatePodDisruptionBudget(obj.(*policy.PodDisruptionBudget)) + opts := validation.PodDisruptionBudgetValidationOptions{ + AllowInvalidLabelValueInSelector: allowValidateLabelValueInLabelSelector(old.(*policy.PodDisruptionBudget)), + } + return validation.ValidatePodDisruptionBudget(obj.(*policy.PodDisruptionBudget), opts) } // WarningsOnUpdate returns warnings for the given update. @@ -167,3 +174,11 @@ func (podDisruptionBudgetStatusStrategy) ValidateUpdate(ctx context.Context, obj func (podDisruptionBudgetStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { return nil } + +func allowValidateLabelValueInLabelSelector(pdb *policy.PodDisruptionBudget) bool { + labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false} + if pdb.Spec.Selector != nil { + return len(metav1validation.ValidateLabelSelector(pdb.Spec.Selector, labelSelectorValidationOptions, nil)) > 0 + } + return false +} diff --git a/pkg/registry/rbac/clusterrole/strategy.go b/pkg/registry/rbac/clusterrole/strategy.go index ca917c3e73f..60e4a806c53 100644 --- a/pkg/registry/rbac/clusterrole/strategy.go +++ b/pkg/registry/rbac/clusterrole/strategy.go @@ -19,6 +19,7 @@ package clusterrole import ( "context" + metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/registry/rest" @@ -71,7 +72,10 @@ func (strategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { // Validate validates a new ClusterRole. Validation must check for a correct signature. func (strategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { clusterRole := obj.(*rbac.ClusterRole) - return validation.ValidateClusterRole(clusterRole) + opts := validation.ClusterRoleValidationOptions{ + AllowInvalidLabelValueInSelector: false, + } + return validation.ValidateClusterRole(clusterRole, opts) } // WarningsOnCreate returns warnings for the creation of the given object. @@ -85,8 +89,12 @@ func (strategy) Canonicalize(obj runtime.Object) { // ValidateUpdate is the default update validation for an end user. func (strategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { newObj := obj.(*rbac.ClusterRole) - errorList := validation.ValidateClusterRole(newObj) - return append(errorList, validation.ValidateClusterRoleUpdate(newObj, old.(*rbac.ClusterRole))...) + oldObj := old.(*rbac.ClusterRole) + opts := validation.ClusterRoleValidationOptions{ + AllowInvalidLabelValueInSelector: allowValidateLabelValueInLabelSelector(oldObj), + } + errorList := validation.ValidateClusterRole(newObj, opts) + return append(errorList, validation.ValidateClusterRoleUpdate(newObj, old.(*rbac.ClusterRole), opts)...) } // WarningsOnUpdate returns warnings for the given update. @@ -102,3 +110,15 @@ func (strategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) [ func (strategy) AllowUnconditionalUpdate() bool { return true } + +func allowValidateLabelValueInLabelSelector(role *rbac.ClusterRole) bool { + labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false} + if role.AggregationRule != nil { + for _, selector := range role.AggregationRule.ClusterRoleSelectors { + if len(metav1validation.ValidateLabelSelector(&selector, labelSelectorValidationOptions, nil)) > 0 { + return true + } + } + } + return false +} diff --git a/pkg/registry/storage/csistoragecapacity/strategy.go b/pkg/registry/storage/csistoragecapacity/strategy.go index 559b6bba04e..d59a5c38b23 100644 --- a/pkg/registry/storage/csistoragecapacity/strategy.go +++ b/pkg/registry/storage/csistoragecapacity/strategy.go @@ -19,6 +19,7 @@ package csistoragecapacity import ( "context" + metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/storage/names" @@ -48,9 +49,11 @@ func (csiStorageCapacityStrategy) PrepareForCreate(ctx context.Context, obj runt func (csiStorageCapacityStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { csiStorageCapacity := obj.(*storage.CSIStorageCapacity) - - errs := validation.ValidateCSIStorageCapacity(csiStorageCapacity) - errs = append(errs, validation.ValidateCSIStorageCapacity(csiStorageCapacity)...) + opts := validation.CSIStorageCapacityValidateOptions{ + AllowInvalidLabelValueInSelector: false, + } + errs := validation.ValidateCSIStorageCapacity(csiStorageCapacity, opts) + errs = append(errs, validation.ValidateCSIStorageCapacity(csiStorageCapacity, opts)...) return errs } @@ -75,7 +78,10 @@ func (csiStorageCapacityStrategy) PrepareForUpdate(ctx context.Context, obj, old func (csiStorageCapacityStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { newCSIStorageCapacityObj := obj.(*storage.CSIStorageCapacity) oldCSIStorageCapacityObj := old.(*storage.CSIStorageCapacity) - errorList := validation.ValidateCSIStorageCapacity(newCSIStorageCapacityObj) + opts := validation.CSIStorageCapacityValidateOptions{ + AllowInvalidLabelValueInSelector: allowValidateLabelValueInLabelSelector(oldCSIStorageCapacityObj), + } + errorList := validation.ValidateCSIStorageCapacity(newCSIStorageCapacityObj, opts) return append(errorList, validation.ValidateCSIStorageCapacityUpdate(newCSIStorageCapacityObj, oldCSIStorageCapacityObj)...) } @@ -87,3 +93,8 @@ func (csiStorageCapacityStrategy) WarningsOnUpdate(ctx context.Context, obj, old func (csiStorageCapacityStrategy) AllowUnconditionalUpdate() bool { return false } + +func allowValidateLabelValueInLabelSelector(capacity *storage.CSIStorageCapacity) bool { + labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false} + return len(metav1validation.ValidateLabelSelector(capacity.NodeTopology, labelSelectorValidationOptions, nil)) > 0 +} diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go index 4c09898b8b9..89df038bc53 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go @@ -28,19 +28,25 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) -func ValidateLabelSelector(ps *metav1.LabelSelector, fldPath *field.Path) field.ErrorList { +// LabelSelectorValidationOptions is a struct that can be passed to ValidateLabelSelector to record the validate options +type LabelSelectorValidationOptions struct { + // Allow invalid label value in selector + AllowInvalidLabelValueInSelector bool +} + +func ValidateLabelSelector(ps *metav1.LabelSelector, opts LabelSelectorValidationOptions, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if ps == nil { return allErrs } allErrs = append(allErrs, ValidateLabels(ps.MatchLabels, fldPath.Child("matchLabels"))...) for i, expr := range ps.MatchExpressions { - allErrs = append(allErrs, ValidateLabelSelectorRequirement(expr, fldPath.Child("matchExpressions").Index(i))...) + allErrs = append(allErrs, ValidateLabelSelectorRequirement(expr, opts, fldPath.Child("matchExpressions").Index(i))...) } return allErrs } -func ValidateLabelSelectorRequirement(sr metav1.LabelSelectorRequirement, fldPath *field.Path) field.ErrorList { +func ValidateLabelSelectorRequirement(sr metav1.LabelSelectorRequirement, opts LabelSelectorValidationOptions, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} switch sr.Operator { case metav1.LabelSelectorOpIn, metav1.LabelSelectorOpNotIn: @@ -55,6 +61,13 @@ func ValidateLabelSelectorRequirement(sr metav1.LabelSelectorRequirement, fldPat allErrs = append(allErrs, field.Invalid(fldPath.Child("operator"), sr.Operator, "not a valid selector operator")) } allErrs = append(allErrs, ValidateLabelName(sr.Key, fldPath.Child("key"))...) + if !opts.AllowInvalidLabelValueInSelector { + for valueIndex, value := range sr.Values { + for _, msg := range validation.IsValidLabelValue(value) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("values").Index(valueIndex), value, msg)) + } + } + } return allErrs } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go index 93c3284f7cf..f81e520d805 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go @@ -427,6 +427,99 @@ func TestValidateConditions(t *testing.T) { } } +func TestLabelSelectorMatchExpression(t *testing.T) { + // Success case + testCases := []struct { + name string + labelSelector *metav1.LabelSelector + wantErrorNumber int + validateErrs func(t *testing.T, errs field.ErrorList) + }{ + { + name: "Valid LabelSelector", + labelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "key", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"value"}, + }, + }, + }, + wantErrorNumber: 0, + validateErrs: nil, + }, + { + name: "MatchExpression's key name isn't valid", + labelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "-key", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"value"}, + }, + }, + }, + wantErrorNumber: 1, + validateErrs: func(t *testing.T, errs field.ErrorList) { + errMessage := "name part must consist of alphanumeric characters" + if !partStringInErrorMessage(errs, errMessage) { + t.Errorf("missing %q in\n%v", errMessage, errorsAsString(errs)) + } + }, + }, + { + name: "MatchExpression's operator isn't valid", + labelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "key", + Operator: "abc", + Values: []string{"value"}, + }, + }, + }, + wantErrorNumber: 1, + validateErrs: func(t *testing.T, errs field.ErrorList) { + errMessage := "not a valid selector operator" + if !partStringInErrorMessage(errs, errMessage) { + t.Errorf("missing %q in\n%v", errMessage, errorsAsString(errs)) + } + }, + }, + { + name: "MatchExpression's value name isn't valid", + labelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "key", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"-value"}, + }, + }, + }, + wantErrorNumber: 1, + validateErrs: func(t *testing.T, errs field.ErrorList) { + errMessage := "a valid label must be an empty string or consist of" + if !partStringInErrorMessage(errs, errMessage) { + t.Errorf("missing %q in\n%v", errMessage, errorsAsString(errs)) + } + }, + }, + } + for index, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + allErrs := ValidateLabelSelector(testCase.labelSelector, LabelSelectorValidationOptions{false}, field.NewPath("labelSelector")) + if len(allErrs) != testCase.wantErrorNumber { + t.Errorf("case[%d]: expected failure", index) + } + if len(allErrs) >= 1 && testCase.validateErrs != nil { + testCase.validateErrs(t, allErrs) + } + }) + } +} + func hasError(errs field.ErrorList, needle string) bool { for _, curr := range errs { if curr.Error() == needle { @@ -445,6 +538,15 @@ func hasPrefixError(errs field.ErrorList, prefix string) bool { return false } +func partStringInErrorMessage(errs field.ErrorList, prefix string) bool { + for _, curr := range errs { + if strings.Contains(curr.Error(), prefix) { + return true + } + } + return false +} + func errorsAsString(errs field.ErrorList) string { messages := []string{} for _, curr := range errs { From fc69084bf1fd32e55256795a6db7a74d75679d8b Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Mon, 7 Nov 2022 13:03:27 -0500 Subject: [PATCH 2/2] Update workload selector validation --- pkg/api/pod/util.go | 1 - .../validation/validation.go | 6 +++-- pkg/apis/apps/validation/validation.go | 12 ++++----- pkg/apis/batch/validation/validation.go | 6 ++--- pkg/apis/core/validation/validation.go | 6 ++--- pkg/apis/networking/validation/validation.go | 6 ++--- pkg/registry/apps/daemonset/strategy.go | 3 +++ pkg/registry/apps/daemonset/strategy_test.go | 13 ++++++++++ pkg/registry/batch/job/strategy.go | 7 +++--- pkg/registry/batch/job/strategy_test.go | 25 +++++++++++++++++++ .../policy/poddisruptionbudget/strategy.go | 6 ++--- pkg/registry/rbac/clusterrole/strategy.go | 6 ++--- .../storage/csistoragecapacity/strategy.go | 4 +-- .../pkg/apis/meta/v1/validation/validation.go | 21 ++++++++++++++++ .../meta/v1/validation/validation_test.go | 1 - 15 files changed, 92 insertions(+), 31 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 00c52f6c47f..61d74bf177c 100644 --- a/pkg/api/pod/util.go +++ b/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 { diff --git a/pkg/apis/admissionregistration/validation/validation.go b/pkg/apis/admissionregistration/validation/validation.go index 56effdf01e0..32d14cc2253 100644 --- a/pkg/apis/admissionregistration/validation/validation.go +++ b/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 { diff --git a/pkg/apis/apps/validation/validation.go b/pkg/apis/apps/validation/validation.go index 48aa4d1980d..7dd539b944a 100644 --- a/pkg/apis/apps/validation/validation.go +++ b/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")) } diff --git a/pkg/apis/batch/validation/validation.go b/pkg/apis/batch/validation/validation.go index 8227d8bc3e4..2efb83bb064 100644 --- a/pkg/apis/batch/validation/validation.go +++ b/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"))...) } diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 909d8fe0889..91635d83dca 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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"))...) } diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index 7fcba85fb82..ca9bbaafa0c 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/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 } diff --git a/pkg/registry/apps/daemonset/strategy.go b/pkg/registry/apps/daemonset/strategy.go index 8eca081dd76..32665566fa0 100644 --- a/pkg/registry/apps/daemonset/strategy.go +++ b/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)...) diff --git a/pkg/registry/apps/daemonset/strategy_test.go b/pkg/registry/apps/daemonset/strategy_test.go index cc8929eaec7..21e42cd25af 100644 --- a/pkg/registry/apps/daemonset/strategy_test.go +++ b/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{ diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index b7f38920124..ceae5bb6879 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/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...) diff --git a/pkg/registry/batch/job/strategy_test.go b/pkg/registry/batch/job/strategy_test.go index 60141c8cd98..68d4225406a 100644 --- a/pkg/registry/batch/job/strategy_test.go +++ b/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{ diff --git a/pkg/registry/policy/poddisruptionbudget/strategy.go b/pkg/registry/policy/poddisruptionbudget/strategy.go index bf89a2d20ad..9be2b55ffd6 100644 --- a/pkg/registry/policy/poddisruptionbudget/strategy.go +++ b/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 { - labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false} +func hasInvalidLabelValueInLabelSelector(pdb *policy.PodDisruptionBudget) bool { if pdb.Spec.Selector != nil { + labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false} return len(metav1validation.ValidateLabelSelector(pdb.Spec.Selector, labelSelectorValidationOptions, nil)) > 0 } return false diff --git a/pkg/registry/rbac/clusterrole/strategy.go b/pkg/registry/rbac/clusterrole/strategy.go index 60e4a806c53..ab5d6fcda06 100644 --- a/pkg/registry/rbac/clusterrole/strategy.go +++ b/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 { - labelSelectorValidationOptions := metav1validation.LabelSelectorValidationOptions{AllowInvalidLabelValueInSelector: false} +func hasInvalidLabelValueInLabelSelector(role *rbac.ClusterRole) bool { 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 diff --git a/pkg/registry/storage/csistoragecapacity/strategy.go b/pkg/registry/storage/csistoragecapacity/strategy.go index d59a5c38b23..ec865dacf93 100644 --- a/pkg/registry/storage/csistoragecapacity/strategy.go +++ b/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 } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go index 89df038bc53..a0f709ad862 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go +++ b/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 { diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go index f81e520d805..60b0cd3dbad 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go +++ b/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