diff --git a/pkg/util/validation/validation.go b/pkg/util/validation/validation.go index 1a7d20da8..8e2c90eeb 100644 --- a/pkg/util/validation/validation.go +++ b/pkg/util/validation/validation.go @@ -23,7 +23,7 @@ func ValidatePropagationSpec(spec policyv1alpha1.PropagationSpec) field.ErrorLis return allErrs } -// ValidatePlacement validates a PropagationSpec before creation or update. +// ValidatePlacement validates a placement before creation or update. func ValidatePlacement(placement policyv1alpha1.Placement, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList @@ -37,7 +37,7 @@ func ValidatePlacement(placement policyv1alpha1.Placement, fldPath *field.Path) return allErrs } -// ValidateClusterAffinity validates a PropagationSpec before creation or update. +// ValidateClusterAffinity validates a clusterAffinity before creation or update. func ValidateClusterAffinity(affinity *policyv1alpha1.ClusterAffinity, fldPath *field.Path) field.ErrorList { if affinity == nil { return nil @@ -51,7 +51,7 @@ func ValidateClusterAffinity(affinity *policyv1alpha1.ClusterAffinity, fldPath * return allErrs } -// ValidateClusterAffinities validates a PropagationSpec before creation or update. +// ValidateClusterAffinities validates clusterAffinities before creation or update. func ValidateClusterAffinities(affinities []policyv1alpha1.ClusterAffinityTerm, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList @@ -68,7 +68,7 @@ func ValidateClusterAffinities(affinities []policyv1alpha1.ClusterAffinityTerm, return allErrs } -// ValidatePolicyFieldSelector tests if the fieldSelector of propagation policy is valid. +// ValidatePolicyFieldSelector tests if the fieldSelector of propagation policy or override policy is valid. func ValidatePolicyFieldSelector(fieldSelector *policyv1alpha1.FieldSelector) error { if fieldSelector == nil { return nil @@ -125,45 +125,33 @@ func ValidateSpreadConstraint(spreadConstraints []policyv1alpha1.SpreadConstrain } // ValidateOverrideSpec validates that the overrider specification is correctly defined. -func ValidateOverrideSpec(overrideSpec *policyv1alpha1.OverrideSpec) error { +func ValidateOverrideSpec(overrideSpec *policyv1alpha1.OverrideSpec) field.ErrorList { + var allErrs field.ErrorList if overrideSpec == nil { return nil } - - if overrideSpec.OverrideRules == nil { - return nil - } - + specPath := field.NewPath("spec") //nolint:staticcheck // disable `deprecation` check for backward compatibility. - if overrideSpec.TargetCluster != nil || !EmptyOverrides(overrideSpec.Overriders) { - return fmt.Errorf("overrideRules and (overriders or targetCluster) can't co-exist") + if overrideSpec.TargetCluster != nil { + allErrs = append(allErrs, ValidateClusterAffinity(overrideSpec.TargetCluster, specPath.Child("targetCluster"))...) } - - for overrideRuleIndex, rule := range overrideSpec.OverrideRules { - rulePath := field.NewPath("spec").Child("overrideRules").Index(overrideRuleIndex) - - // validates provided annotations. - for annotationIndex, annotation := range rule.Overriders.AnnotationsOverrider { - annotationPath := rulePath.Child("overriders").Child("annotationsOverrider").Index(annotationIndex) - if err := apivalidation.ValidateAnnotations(annotation.Value, annotationPath.Child("value")).ToAggregate(); err != nil { - return err - } - } - - // validates provided labels. - for labelIndex, label := range rule.Overriders.LabelsOverrider { - labelPath := rulePath.Child("overriders").Child("labelsOverrider").Index(labelIndex) - if err := metav1validation.ValidateLabels(label.Value, labelPath.Child("value")).ToAggregate(); err != nil { - return err - } - } + //nolint:staticcheck + // disable `deprecation` check for backward compatibility. + if overrideSpec.TargetCluster != nil && overrideSpec.OverrideRules != nil { + allErrs = append(allErrs, field.Invalid(specPath.Child("targetCluster"), overrideSpec.TargetCluster, "overrideRules and targetCluster can't co-exist")) } - return nil + //nolint:staticcheck + // disable `deprecation` check for backward compatibility. + if !emptyOverrides(overrideSpec.Overriders) && overrideSpec.OverrideRules != nil { + allErrs = append(allErrs, field.Invalid(specPath.Child("overriders"), overrideSpec.Overriders, "overrideRules and overriders can't co-exist")) + } + allErrs = append(allErrs, ValidateOverrideRules(overrideSpec.OverrideRules, specPath)...) + return allErrs } -// EmptyOverrides check if the overriders of override policy is empty -func EmptyOverrides(overriders policyv1alpha1.Overriders) bool { +// emptyOverrides check if the overriders of override policy is empty. +func emptyOverrides(overriders policyv1alpha1.Overriders) bool { if len(overriders.Plaintext) != 0 { return false } @@ -184,3 +172,27 @@ func EmptyOverrides(overriders policyv1alpha1.Overriders) bool { } return true } + +// ValidateOverrideRules validates the overrideRules of override policy. +func ValidateOverrideRules(overrideRules []policyv1alpha1.RuleWithCluster, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + for overrideRuleIndex, rule := range overrideRules { + rulePath := fldPath.Child("overrideRules").Index(overrideRuleIndex) + + // validates provided annotations. + for annotationIndex, annotation := range rule.Overriders.AnnotationsOverrider { + annotationPath := rulePath.Child("overriders").Child("annotationsOverrider").Index(annotationIndex) + allErrs = append(allErrs, apivalidation.ValidateAnnotations(annotation.Value, annotationPath.Child("value"))...) + } + + // validates provided labels. + for labelIndex, label := range rule.Overriders.LabelsOverrider { + labelPath := rulePath.Child("overriders").Child("labelsOverrider").Index(labelIndex) + allErrs = append(allErrs, metav1validation.ValidateLabels(label.Value, labelPath.Child("value"))...) + } + + // validates the targetCluster. + allErrs = append(allErrs, ValidateClusterAffinity(rule.TargetCluster, rulePath.Child("targetCluster"))...) + } + return allErrs +} diff --git a/pkg/util/validation/validation_test.go b/pkg/util/validation/validation_test.go index c3a05d7f9..fbc7d8f97 100644 --- a/pkg/util/validation/validation_test.go +++ b/pkg/util/validation/validation_test.go @@ -146,6 +146,72 @@ func TestValidateOverrideSpec(t *testing.T) { }, expectError: true, }, + { + name: "overrideSpec.targetCluster.fieldSelector has unsupported key", + overrideSpec: policyv1alpha1.OverrideSpec{ + TargetCluster: &policyv1alpha1.ClusterAffinity{ + FieldSelector: &policyv1alpha1.FieldSelector{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "foo", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"fooCloud"}, + }}}}, + }, + expectError: true, + }, + { + name: "overrideSpec.targetCluster.fieldSelector has unsupported operator", + overrideSpec: policyv1alpha1.OverrideSpec{ + TargetCluster: &policyv1alpha1.ClusterAffinity{ + FieldSelector: &policyv1alpha1.FieldSelector{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: util.ProviderField, + Operator: corev1.NodeSelectorOpGt, + Values: []string{"fooCloud"}, + }}}}, + }, + expectError: true, + }, + { + name: "overrideRules.[index].targetCluster.fieldSelector has unsupported key", + overrideSpec: policyv1alpha1.OverrideSpec{ + OverrideRules: []policyv1alpha1.RuleWithCluster{ + { + TargetCluster: &policyv1alpha1.ClusterAffinity{ + FieldSelector: &policyv1alpha1.FieldSelector{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "foo", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"fooCloud"}, + }}}, + }, + }, + }, + }, + expectError: true, + }, + { + name: "overrideRules.[index].targetCluster.fieldSelector has unsupported operator", + overrideSpec: policyv1alpha1.OverrideSpec{ + OverrideRules: []policyv1alpha1.RuleWithCluster{ + { + TargetCluster: &policyv1alpha1.ClusterAffinity{ + FieldSelector: &policyv1alpha1.FieldSelector{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: util.ProviderField, + Operator: corev1.NodeSelectorOpGt, + Values: []string{"fooCloud"}, + }}}, + }, + }, + }, + }, + expectError: true, + }, } for _, test := range tests { @@ -210,7 +276,7 @@ func TestEmptyOverrides(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := EmptyOverrides(tt.overriders); got != tt.want { + if got := emptyOverrides(tt.overriders); got != tt.want { t.Errorf("EmptyOverrides() = %v, want %v", got, tt.want) } }) diff --git a/pkg/webhook/clusteroverridepolicy/validating.go b/pkg/webhook/clusteroverridepolicy/validating.go index 65fb4cc87..8a3526191 100644 --- a/pkg/webhook/clusteroverridepolicy/validating.go +++ b/pkg/webhook/clusteroverridepolicy/validating.go @@ -31,9 +31,9 @@ func (v *ValidatingAdmission) Handle(ctx context.Context, req admission.Request) } klog.V(2).Infof("Validating ClusterOverridePolicy(%s) for request: %s", policy.Name, req.Operation) - if err := validation.ValidateOverrideSpec(&policy.Spec); err != nil { - klog.Error(err) - return admission.Denied(err.Error()) + if errs := validation.ValidateOverrideSpec(&policy.Spec); len(errs) != 0 { + klog.Error(errs) + return admission.Denied(errs.ToAggregate().Error()) } return admission.Allowed("") diff --git a/pkg/webhook/overridepolicy/validating.go b/pkg/webhook/overridepolicy/validating.go index d6938a4e7..99c81d5cf 100644 --- a/pkg/webhook/overridepolicy/validating.go +++ b/pkg/webhook/overridepolicy/validating.go @@ -31,9 +31,9 @@ func (v *ValidatingAdmission) Handle(ctx context.Context, req admission.Request) } klog.V(2).Infof("Validating OverridePolicy(%s/%s) for request: %s", policy.Namespace, policy.Name, req.Operation) - if err := validation.ValidateOverrideSpec(&policy.Spec); err != nil { - klog.Error(err) - return admission.Denied(err.Error()) + if errs := validation.ValidateOverrideSpec(&policy.Spec); len(errs) != 0 { + klog.Error(errs) + return admission.Denied(errs.ToAggregate().Error()) } return admission.Allowed("")