🐛 Clean up and namespace verification while removing owner ref and controller ref #2958
Conversation
Signed-off-by: Hiranmoy Das Chowdhury <hiranmoy.das70@gmail.com>
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: HiranmoyChowdhury The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @HiranmoyChowdhury. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
|
As there can be only one Controller Reference, then we should have something which only removes the controllerReference. Here the function named "RemoveControllerReference" removes controller reference if that matched with the given object. This way should only applicable for OwnerReference I guess. Should I change this too or make a new function? |
HiranmoyChowdhury
changed the title
| func referSameObject(a, b metav1.OwnerReference) bool { | ||
| aGV, err := schema.ParseGroupVersion(a.APIVersion) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| bGV, err := schema.ParseGroupVersion(b.APIVersion) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return aGV.Group == bGV.Group && a.Kind == b.Kind && a.Name == b.Name | ||
| return a.UID == b.UID |
There was a problem hiding this comment.
/hold
No, this isn't correct as UIDs can change across backups and clusters. References should be compared like above
There was a problem hiding this comment.
Got it. I am gonna change this and I guess rest are okay
There was a problem hiding this comment.
Now this will ensure owner by validating namespace while removing owner ref or controller ref
k8s-ci-robot
added
the
do-not-merge/hold
| return idx != -1, nil | ||
| } | ||
|
|
||
| // RemoveControllerReference removes an owner reference where the controller | ||
| // equals true | ||
| func RemoveControllerReference(owner, object metav1.Object, scheme *runtime.Scheme) error { | ||
| if ok := HasControllerReference(object); !ok { | ||
| return fmt.Errorf("%T does not have a owner reference with controller equals true", object) | ||
| if !metav1.IsControlledBy(object, owner) { |
There was a problem hiding this comment.
This error is now misleading because if the object doesn’t have a controller reference the returned error will only indicate that the owner being passed is not the controller reference for the object.
The error before allowed you to recognize if the object you are passing in had one at all before trying to remove it with the owner passed in.
There was a problem hiding this comment.
#2595 (comment) Discussion for why the logic exists was from several issues with how to get around overwriting and errors that exist with controller reference/owner reference.
There was a problem hiding this comment.
okay. gonna change it soon
There was a problem hiding this comment.
@troy0820 WDYT about this: #2958 (comment)
…er reference and fix error messages for not having controller reference Signed-off-by: Hiranmoy Das Chowdhury <hiranmoy.das70@gmail.com>
HiranmoyChowdhury
changed the title
HiranmoyChowdhury
changed the title
Signed-off-by: Hiranmoy Das Chowdhury <hiranmoy.das70@gmail.com>
Signed-off-by: Hiranmoy Das Chowdhury <hiranmoy.das70@gmail.com>
k8s-ci-robot
added
size/L
Signed-off-by: Hiranmoy Das Chowdhury hiranmoy.das70@gmail.com
As we know for a resource there only one name can be allocated for every individual namespace. So, when we are removing ownerRef or controllerRef from an controlled object by passing the owner object, we are only checking the name+group+kind but not the namespace. To verify this I added namespace verification and also did some cleanup.