Merge pull request #3193 from chaunceyjiang/targetCluster

feat: validates the fieldSelector of overridepolicy

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
 	}
-
+	specPath := field.NewPath("spec")
-	if overrideSpec.OverrideRules == nil {
-		return nil
-	}
-
 	//nolint:staticcheck
 	// disable `deprecation` check for backward compatibility.
-	if overrideSpec.TargetCluster != nil || !EmptyOverrides(overrideSpec.Overriders) {
+	if overrideSpec.TargetCluster != nil {
-		return fmt.Errorf("overrideRules and (overriders or targetCluster) can't co-exist")
+		allErrs = append(allErrs, ValidateClusterAffinity(overrideSpec.TargetCluster, specPath.Child("targetCluster"))...)
 	}
-
+	//nolint:staticcheck
-	for overrideRuleIndex, rule := range overrideSpec.OverrideRules {
+	// disable `deprecation` check for backward compatibility.
-		rulePath := field.NewPath("spec").Child("overrideRules").Index(overrideRuleIndex)
+	if overrideSpec.TargetCluster != nil && overrideSpec.OverrideRules != nil {
-
+		allErrs = append(allErrs, field.Invalid(specPath.Child("targetCluster"), overrideSpec.TargetCluster, "overrideRules and targetCluster can't co-exist"))
-		// 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
-			}
-		}
 	}
-	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
+// emptyOverrides check if the overriders of override policy is empty.
-func EmptyOverrides(overriders policyv1alpha1.Overriders) bool {
+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
+}

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)
 			}
 		})

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 {
+	if errs := validation.ValidateOverrideSpec(&policy.Spec); len(errs) != 0 {
-		klog.Error(err)
+		klog.Error(errs)
-		return admission.Denied(err.Error())
+		return admission.Denied(errs.ToAggregate().Error())
 	}
 
 	return admission.Allowed("")

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 {
+	if errs := validation.ValidateOverrideSpec(&policy.Spec); len(errs) != 0 {
-		klog.Error(err)
+		klog.Error(errs)
-		return admission.Denied(err.Error())
+		return admission.Denied(errs.ToAggregate().Error())
 	}
 
 	return admission.Allowed("")