From 3093c9189368a3c61de6846ef03ed84c4bdbd547 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Fri, 24 Jan 2025 17:10:57 -0800 Subject: [PATCH] fix the resource webhook logic and fix related flaky e2e (#1020) Co-authored-by: Ryan Zhang --- pkg/utils/validator/resourceoverride.go | 4 +- .../resourceoverride_validating_webhook.go | 19 ++---- test/e2e/webhook_test.go | 58 ++++++++----------- 3 files changed, 32 insertions(+), 49 deletions(-) diff --git a/pkg/utils/validator/resourceoverride.go b/pkg/utils/validator/resourceoverride.go index 67d16fcf4..ede8f88ff 100644 --- 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 index b08ce9954..cd439a0c6 100644 --- 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 index 5580fe535..d4c1ab964 100644 --- 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{