fix the resource webhook logic and fix related flaky e2e (#1020)

Co-authored-by: Ryan Zhang <[email protected]>
Azure · Jan 25, 2025 · 3093c91 · 3093c91
1 parent 20de7b7
commit 3093c91
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 49 deletions.
diff --git a/pkg/utils/validator/resourceoverride.go b/pkg/utils/validator/resourceoverride.go
@@ -92,11 +92,11 @@ func validateOverridePolicy(policy *fleetv1alpha1.OverridePolicy) error {
 			for _, selector := range rule.ClusterSelector.ClusterSelectorTerms {
 				// Check that only label selector is supported
 				if selector.PropertySelector != nil || selector.PropertySorter != nil {
-					allErr = append(allErr, fmt.Errorf("invalid clusterSelector %v: only labelSelector is supported", selector))
+					allErr = append(allErr, fmt.Errorf("invalid clusterSelector %+v: only labelSelector is supported", selector))
 					continue
 				}
 				if selector.LabelSelector == nil {
-					allErr = append(allErr, fmt.Errorf("invalid clusterSelector %v: labelSelector is required", selector))
+					allErr = append(allErr, fmt.Errorf("invalid clusterSelector %+v: labelSelector is required", selector))
 				} else if err := validateLabelSelector(selector.LabelSelector, "cluster selector"); err != nil {
 					allErr = append(allErr, err)
 				}

diff --git a/pkg/webhook/resourceoverride/resourceoverride_validating_webhook.go b/pkg/webhook/resourceoverride/resourceoverride_validating_webhook.go
@@ -49,10 +49,11 @@ func (v *resourceOverrideValidator) Handle(ctx context.Context, req admission.Re
 		return admission.Errored(http.StatusBadRequest, err)
 	}
 
-	// List of resource overrides.
-	roList, err := listResourceOverride(ctx, v.client)
-	if err != nil {
-		return admission.Errored(http.StatusBadRequest, err)
+	// List all the resource overrides in the same namespace
+	roList := &fleetv1alpha1.ResourceOverrideList{}
+	if err := v.client.List(ctx, roList, client.InNamespace(ro.Namespace)); err != nil {
+		klog.ErrorS(err, "Failed to list resourceOverrides when validating")
+		return admission.Errored(http.StatusInternalServerError, fmt.Errorf("failed to list resourceOverrides, please retry the request: %w", err))
 	}
 
 	// Check if the override count limit has been reached, if there are at most 100 resource overrides.
@@ -67,13 +68,3 @@ func (v *resourceOverrideValidator) Handle(ctx context.Context, req admission.Re
 	}
 	return admission.Allowed("resourceOverride has valid fields")
 }
-
-// listResourceOverride returns a list of cluster resource overrides.
-func listResourceOverride(ctx context.Context, client client.Client) (*fleetv1alpha1.ResourceOverrideList, error) {
-	roList := &fleetv1alpha1.ResourceOverrideList{}
-	if err := client.List(ctx, roList); err != nil {
-		klog.ErrorS(err, "Failed to list resourceOverrides when validating")
-		return nil, fmt.Errorf("failed to list resourceOverrides, please retry the request: %w", err)
-	}
-	return roList, nil
-}
diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go
@@ -1197,47 +1197,39 @@ var _ = Describe("webhook tests for ResourceOverride UPDATE operations", Ordered
 	})
 
 	It("should deny update RO with invalid resource override", func() {
-		Eventually(func(g Gomega) error {
-			By("creating a new resource override")
-			ro1 := &placementv1alpha1.ResourceOverride{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      fmt.Sprintf("test-ro-%d", GinkgoParallelProcess()),
-					Namespace: roNamespace,
-				},
-				Spec: placementv1alpha1.ResourceOverrideSpec{
-					ResourceSelectors: []placementv1alpha1.ResourceSelector{
+		newSelector := placementv1alpha1.ResourceSelector{
+			Group:   "apps",
+			Kind:    "Deployment",
+			Version: "v1",
+			Name:    "test-deployment-x",
+		}
+		ro1 := &placementv1alpha1.ResourceOverride{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      fmt.Sprintf("test-ro-%d", GinkgoParallelProcess()),
+				Namespace: roNamespace,
+			},
+			Spec: placementv1alpha1.ResourceOverrideSpec{
+				Policy: &placementv1alpha1.OverridePolicy{
+					OverrideRules: []placementv1alpha1.OverrideRule{
 						{
-							Group:   "apps",
-							Kind:    "Deployment",
-							Version: "v1",
-							Name:    "test-deployment-1",
-						},
-					},
-					Policy: &placementv1alpha1.OverridePolicy{
-						OverrideRules: []placementv1alpha1.OverrideRule{
-							{
-								ClusterSelector: nil,
-								JSONPatchOverrides: []placementv1alpha1.JSONPatchOverride{
-									{
-										Operator: placementv1alpha1.JSONPatchOverrideOpRemove,
-										Path:     "/meta/labels/test-key",
-									},
+							ClusterSelector: nil,
+							JSONPatchOverrides: []placementv1alpha1.JSONPatchOverride{
+								{
+									Operator: placementv1alpha1.JSONPatchOverrideOpRemove,
+									Path:     "/meta/labels/test-key",
 								},
 							},
 						},
 					},
 				},
-			}
-			Expect(hubClient.Create(ctx, ro1)).To(Succeed(), "Failed to create RO %s", ro1.Name)
-
+			},
+		}
+		ro1.Spec.ResourceSelectors = append(ro1.Spec.ResourceSelectors, newSelector)
+		By("creating a new resource override")
+		Expect(hubClient.Create(ctx, ro1)).To(Succeed(), "Failed to create RO %s", ro1.Name)
+		Eventually(func(g Gomega) error {
 			var ro placementv1alpha1.ResourceOverride
 			g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: roName, Namespace: roNamespace}, &ro)).Should(Succeed())
-			newSelector := placementv1alpha1.ResourceSelector{
-				Group:   "apps",
-				Kind:    "Deployment",
-				Version: "v1",
-				Name:    fmt.Sprintf("test-deployment-%d", 1),
-			}
 			ro.Spec.ResourceSelectors = append(ro.Spec.ResourceSelectors, newSelector)
 			clusterSelectorTerm := placementv1beta1.ClusterSelectorTerm{
 				PropertySelector: &placementv1beta1.PropertySelector{