🐛Remove owner passed in to RemoveControllerReference #2595
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
troy0820
force-pushed
the
troy0820/remove-controller-ref-bug
branch
2 times, most recently
from
5f0a3f4 to
34aaa05
Compare
k8s-ci-robot
added
size/S
troy0820
force-pushed
the
troy0820/remove-controller-ref-bug
branch
from
34aaa05 to
25bf123
Compare
troy0820
commented
Nov 24, 2023
Comment on lines
174
to
199
| 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 |
There was a problem hiding this comment.
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.
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
sbueringer
reviewed
Nov 27, 2023
k8s-ci-robot
added
size/M
troy0820
force-pushed
the
troy0820/remove-controller-ref-bug
branch
2 times, most recently
from
0ed0fc1 to
a2a0263
Compare
sbueringer
reviewed
Nov 27, 2023
| dep2 := &extensionsv1beta1.Deployment{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "foo-2", UID: "foo-uid-42"}, | ||
| } | ||
| Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
So it exists, but the owner we passed in would still error because it doesn't have the controller == true. Makes sense.
troy0820
force-pushed
the
troy0820/remove-controller-ref-bug
branch
from
a2a0263 to
dffeafa
Compare
|
/retest |
…er controller equals true Signed-off-by: Troy Connor <[email protected]>
troy0820
force-pushed
the
troy0820/remove-controller-ref-bug
branch
2 times, most recently
from
e204fb3 to
9c4611e
Compare
|
@troy0820 could you fix the typo in the PR title? |
troy0820
changed the title
alvaroaleman
approved these changes
Dec 4, 2023
k8s-ci-robot
added
the
lgtm
|
[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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes initial PR of #2509
Resolves #2594
/assign @sbueringer