🐛Remove owner passed in to RemoveControllerReference #2595

troy0820 · 2023-11-24T15:12:36Z

Fixes initial PR of #2509

Resolves #2594

Add a check to find owner reference that equals the passed in owner and remove if controller = true

troy0820 · 2023-11-24T19:25:38Z

pkg/controller/controllerutil/controllerutil.go

+	ro, ok := owner.(runtime.Object)
+	if !ok {
+		return fmt.Errorf("%T is not a runtime.Object, cannot call RemoveControllerReference", owner)
+	}
+	gvk, err := apiutil.GVKForObject(ro, scheme)
+	if err != nil {
+		return err
+	}
+	ownerRefs := object.GetOwnerReferences()
+	index := indexOwnerRef(ownerRefs, metav1.OwnerReference{
+		APIVersion: gvk.GroupVersion().String(),
+		Name:       owner.GetName(),
+		Kind:       gvk.Kind,
+		Controller: ptr.To(true),
+	})
+	if index == -1 {
+		return fmt.Errorf("%T does not have an controller reference for %T", object, owner)
+	}
+	ownerRefs = append(ownerRefs[:index], ownerRefs[index+1:]...)
+	object.SetOwnerReferences(ownerRefs)
+	return nil


Not a fan of this replicated logic but didn't want to conflate the errors that may exist passing into another function. Also, finding the index happens twice but we need to find the index to verify that the owner is the controllerReference for the object.

This seems to make sure the only controllerReference is the owner that is passed in because

Only one ControllerReference (1:1) be present on an object but many OwnerReferences (1:N)

The index returned verifies that the above owner is in fact the controller reference on the object if the index is greater than -1.

troy0820 · 2023-11-27T01:44:05Z

/ok-to-test

pkg/controller/controllerutil/controllerutil.go

sbueringer · 2023-11-27T14:40:09Z

pkg/controller/controllerutil/controllerutil.go

@@ -171,7 +171,27 @@ func RemoveControllerReference(owner, object metav1.Object, scheme *runtime.Sche
 	if ok := HasControllerReference(object); !ok {


WDYT about adding additional test cases for the cases which weren't captured by the existing tests?

I can do that. My work here still doesn't show that the owner reference that exists is in fact the actual controller Reference that is being removed.

The test I added was to show that this errors when passing in the wrong owner. @sbueringer

sbueringer · 2023-11-27T18:14:08Z

pkg/controller/controllerutil/controllerutil_test.go

+			dep2 := &extensionsv1beta1.Deployment{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo-2", UID: "foo-uid-42"},
+			}
+			Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).ToNot(HaveOccurred())


Suggested change

Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).ToNot(HaveOccurred())

Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).ToNot(HaveOccurred())

Expect(controllerutil.SetOwnerReference(dep2, rs, scheme.Scheme)).ToNot(HaveOccurred())

Wondering if that would add useful coverage. Basically we error out because the owner we want to remove is only owner but not the controller

(which is exactly the case we missed before, I think)

So it exists, but the owner we passed in would still error because it doesn't have the controller == true. Makes sense.

troy0820 · 2023-11-27T18:59:00Z

/retest

…er controller equals true Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>

inteon · 2023-12-02T07:58:38Z

@troy0820 could you fix the typo in the PR title?

k8s-ci-robot · 2023-12-04T16:18:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, troy0820

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot assigned sbueringer Nov 24, 2023

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 24, 2023

k8s-ci-robot requested review from FillZpp and inteon November 24, 2023 15:12

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 24, 2023

troy0820 force-pushed the troy0820/remove-controller-ref-bug branch 2 times, most recently from 5f0a3f4 to 34aaa05 Compare November 24, 2023 19:17

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 24, 2023

troy0820 force-pushed the troy0820/remove-controller-ref-bug branch from 34aaa05 to 25bf123 Compare November 24, 2023 19:20

troy0820 commented Nov 24, 2023

View reviewed changes

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 27, 2023

sbueringer reviewed Nov 27, 2023

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 27, 2023

troy0820 force-pushed the troy0820/remove-controller-ref-bug branch 2 times, most recently from 0ed0fc1 to a2a0263 Compare November 27, 2023 17:26

troy0820 requested a review from sbueringer November 27, 2023 17:38

sbueringer reviewed Nov 27, 2023

View reviewed changes

troy0820 force-pushed the troy0820/remove-controller-ref-bug branch from a2a0263 to dffeafa Compare November 27, 2023 18:27

remove owner passed in to RemoveControlleReference only when that own…

9c4611e

…er controller equals true Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>

troy0820 force-pushed the troy0820/remove-controller-ref-bug branch 2 times, most recently from e204fb3 to 9c4611e Compare November 27, 2023 20:27

troy0820 changed the title ~~🐛Remove owner passed in to RemoveControlleReference~~ 🐛Remove owner passed in to RemoveControllerReference Dec 2, 2023

alvaroaleman approved these changes Dec 4, 2023

View reviewed changes

k8s-ci-robot assigned alvaroaleman Dec 4, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2023

k8s-ci-robot merged commit 1fe3341 into kubernetes-sigs:main Dec 4, 2023
10 checks passed

kcp-ci-bot mentioned this pull request Feb 29, 2024

✨ Bump to v1.17.2 kcp-dev/controller-runtime#47

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛Remove owner passed in to RemoveControllerReference #2595

🐛Remove owner passed in to RemoveControllerReference #2595

troy0820 commented Nov 24, 2023

troy0820 Nov 24, 2023 •

edited

troy0820 commented Nov 27, 2023

sbueringer Nov 27, 2023

troy0820 Nov 27, 2023

troy0820 Nov 27, 2023

sbueringer Nov 27, 2023 •

edited

troy0820 Nov 27, 2023

sbueringer Dec 18, 2023

troy0820 commented Nov 27, 2023

inteon commented Dec 2, 2023

k8s-ci-robot commented Dec 4, 2023

		@@ -171,7 +171,27 @@ func RemoveControllerReference(owner, object metav1.Object, scheme *runtime.Sche
		if ok := HasControllerReference(object); !ok {

	Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).ToNot(HaveOccurred())
	Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).ToNot(HaveOccurred())
	Expect(controllerutil.SetOwnerReference(dep2, rs, scheme.Scheme)).ToNot(HaveOccurred())

🐛Remove owner passed in to RemoveControllerReference #2595

🐛Remove owner passed in to RemoveControllerReference #2595

Conversation

troy0820 commented Nov 24, 2023

troy0820 Nov 24, 2023 • edited

Choose a reason for hiding this comment

troy0820 commented Nov 27, 2023

sbueringer Nov 27, 2023

Choose a reason for hiding this comment

troy0820 Nov 27, 2023

Choose a reason for hiding this comment

troy0820 Nov 27, 2023

Choose a reason for hiding this comment

sbueringer Nov 27, 2023 • edited

Choose a reason for hiding this comment

troy0820 Nov 27, 2023

Choose a reason for hiding this comment

sbueringer Dec 18, 2023

Choose a reason for hiding this comment

troy0820 commented Nov 27, 2023

inteon commented Dec 2, 2023

k8s-ci-robot commented Dec 4, 2023

troy0820 Nov 24, 2023 •

edited

sbueringer Nov 27, 2023 •

edited