fix: do not allow user to pick the same resource twice

[ryanzhang-oss]

Description of your changes

1. Add the check in the resource selector to block duplication
2. Add a few e2e that is missing from previous PR

I have:

• Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

[Copilot]

Pull Request Overview

This PR prevents users from selecting the same resource more than once by detecting duplicates and returning an error, and updates related tests to verify the behavior.

• Introduces duplicate resource detection logic in the resource selector to error out on repeated selections.
• Updates e2e tests and resource override tests to reflect the requirement of unique resource selections.
• Adjusts error handling in the controller to requeue after a delay when a duplicate is detected.

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
test/e2e/placement_selecting_resources_test.go	Adds a focused test case to verify that duplicate resource selection produces the expected status update.
test/e2e/placement_ro_test.go	Adjusts test messages and injects Placement references for resource override scenarios.
test/e2e/actuals_test.go	Improves error messaging by including cluster name details.
pkg/controllers/clusterresourceplacement/resource_selector_test.go	Updates tests to work with unstructured objects and maintain order for resource selection.
pkg/controllers/clusterresourceplacement/resource_selector.go	Implements duplicate detection in resource selection and refines resource sorting logic.
pkg/controllers/clusterresourceplacement/controller.go	Modifies error handling to requeue updates after a duplicate resource error is encountered.

Files not reviewed (1)

• Makefile: Language not supported

Comments suppressed due to low confidence (2)

pkg/controllers/clusterresourceplacement/resource_selector.go:105

• [nitpick] Enhance the duplicate resource error message to include details such as the resource's name and kind to improve debuggability for users.

if _, exist := resourceMap[ri]; exist {

pkg/controllers/clusterresourceplacement/resource_selector_test.go:924

• [nitpick] Consider adding a dedicated unit test case for gatherSelectedResource to verify that the function returns an error when duplicate resources are provided.

tests := map[string]struct {

[Copilot]

Pull Request Overview

This PR prevents users from selecting the same resource more than once by detecting duplicates in the resource selectors and updating the CRP status accordingly. Key changes include:

• Adding an end-to-end test to verify that duplicated resource selection is rejected.
• Updating tests in resourceOverride flows to include the Placement reference and improve error messages.
• Refactoring resource selection logic to return unstructured objects and check for duplicate resource identifiers.

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/e2e/placement_selecting_resources_test.go	Adds a new e2e test case for duplicated resource selection, expecting an invalid CRP status.
test/e2e/placement_ro_test.go	Updates tests by adding Placement refs and revising test descriptions for clarity in resourceOverride scenarios.
test/e2e/actuals_test.go	Improves error message details by including the member cluster name when a namespace is not removed.
pkg/controllers/clusterresourceplacement/resource_selector_test.go	Modifies tests to use unstructured objects instead of runtime.Object for consistency.
pkg/controllers/clusterresourceplacement/resource_selector.go	Refactors resource gathering to enforce uniqueness and sorts resources with simplified code.
pkg/controllers/clusterresourceplacement/controller.go	Adjusts the error handling by requeuing after a fixed delay to allow user correction.

Files not reviewed (1)

• Makefile: Language not supported

[kaito-pr-agent]

Title

fix: do not allow user to pick the same resource twice

---

User description

Description of your changes

Fixes #

I have:

• Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

---

PR Type

Enhancement

---

Description

• Added validation to prevent users from selecting the same resource twice in ClusterResourcePlacement.

• Updated test cases to include scenarios where resources are selected multiple times.

• Enhanced error handling to return a specific error when duplicate resources are detected.

• Modified the sorting logic to ensure stability across reconcile loops.

---

Changes walkthrough 📝

	Relevant files
Bug fix	3 files controller.go Handle duplicate resource selection                                            +2/-1      resource_selector.go Validate resource selection uniqueness                                      +26/-11  actuals_test.go Improve error message for work namespace removal                  +1/-1
Tests	3 files resource_selector_test.go Add test cases for duplicate resource selection                    +24/-24  placement_ro_test.go Add test cases for resourceOverride with multiple overrides +230/-3  placement_selecting_resources_test.go Add test case for picking N resources with duplicates        +65/-0
Formatting	1 files Makefile Bump staticcheck version                                                                  +1/-1

---

Need help?

Type /help how to ... in the comments thread for any questions about PR-Agent usage.

Check out the documentation for more information.

[kaito-pr-agent]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

**🎫 Ticket compliance analysis **

Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Bug The new code adds a delay of 5 minutes before requeuing, which might not be necessary. // no need to retry faster, the user needs to fix the resource selectors return ctrl.Result{RequeueAfter: 5 * time.Minute}, nil Potential Bug The new test cases for handling duplicate resources should include assertions to verify the expected behavior. It("should remove controller finalizers from CRP", func() { finalizerRemovedActual := allFinalizersExceptForCustomDeletionBlockerRemovedFromCRPActual(crpName) Eventually(finalizerRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove controller finalizers from CRP %s", crpName) }) }) var _ = Describe("When creating a pickN ClusterResourcePlacement with duplicated resources", Ordered, func() { crpName := fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess()) var existingNS corev1.Namespace BeforeAll(func() { By("creating work resources on hub cluster") createWorkResources() existingNS = appNamespace() By("Create a crp that selects the same resource twice") crp := &placementv1beta1.ClusterResourcePlacement{ ObjectMeta: metav1.ObjectMeta{ Name: crpName, // Add a custom finalizer; this would allow us to better observe // the behavior of the controllers. Finalizers: []string{customDeletionBlockerFinalizer}, }, Spec: placementv1beta1.ClusterResourcePlacementSpec{ ResourceSelectors: []placementv1beta1.ClusterResourceSelector{ { Group: corev1.GroupName, Version: "v1", Kind: "Namespace", Name: existingNS.Name, }, { Group: corev1.GroupName, Version: "v1", Kind: "Namespace", Name: existingNS.Name, }, }, }, } By(fmt.Sprintf("creating placement %s", crpName)) Expect(hubClient.Create(ctx, crp)).To(Succeed(), "Failed to create CRP %s", crpName) }) AfterAll(func() { By(fmt.Sprintf("garbage all things related to placement %s", crpName)) ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters) }) It("should update CRP status as expected", func() { Eventually(func() error { crp := &placementv1beta1.ClusterResourcePlacement{} if err := hubClient.Get(ctx, types.NamespacedName{Name: crpName}, crp); err != nil { return err } wantStatus := placementv1beta1.ClusterResourcePlacementStatus{ Conditions: []metav1.Condition{ { Status: metav1.ConditionFalse, Type: string(placementv1beta1.ClusterResourcePlacementScheduledConditionType), Reason: clusterresourceplacement.InvalidResourceSelectorsReason, ObservedGeneration: 1, }, }, } if diff := cmp.Diff(crp.Status, wantStatus, crpStatusCmpOptions...); diff != "" { return fmt.Errorf("CRP status diff (-got, +want): %s", diff) } return nil }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status as expected", crpName) }) })

[jwtty]

lgtm

[zhiying-lin]

there could be some api server errors returned from selectResourcesForPlacement func. Should we keep the existing backoff?

[zhiying-lin]

Suggested change

	klog.ErrorS(err, "user selected one resource more than once", "resource", ri, "placement", placement)
	klog.ErrorS(err, "User selected one resource more than once", "resource", ri, "placement", placement)

[zhiying-lin]

it may work as expected to test the override without configuring the CRP.

[zhiying-lin]

should we block this case via the validation webhook?