Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Clean up and namespace verification while removing owner ref and controller ref #2958

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

🐛 Clean up and namespace verification while removing owner ref and controller ref #2958

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3dbd85
clean up and fixed referSameObject function
HiranmoyChowdhury Sep 23, 2024
361186d
validating Namespace added while removing ownerReference and controll…
HiranmoyChowdhury Sep 24, 2024
14c9e2f
newOwnerRef to newControllerRef for removing controller
HiranmoyChowdhury Sep 24, 2024
6181163
newOwnerRef modified and some clean up
HiranmoyChowdhury Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 27 additions & 64 deletions pkg/controller/controllerutil/controllerutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/utils/ptr"

"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
)
Expand Down Expand Up @@ -82,25 +80,20 @@ func SetControllerReference(owner, controlled metav1.Object, scheme *runtime.Sch
if err != nil {
return err
}
ref := metav1.OwnerReference{
APIVersion: gvk.GroupVersion().String(),
Kind: gvk.Kind,
Name: owner.GetName(),
UID: owner.GetUID(),
BlockOwnerDeletion: ptr.To(true),
Controller: ptr.To(true),
}

ref := metav1.NewControllerRef(owner, gvk)

for _, opt := range opts {
opt(&ref)
opt(ref)
}

// Return early with an error if the object is already controlled.
if existing := metav1.GetControllerOf(controlled); existing != nil && !referSameObject(*existing, ref) {
if existing := metav1.GetControllerOf(controlled); existing != nil && !referSameObject(*existing, *ref) {
return newAlreadyOwnedError(controlled, *existing)
}

// Update owner references and return.
upsertOwnerRef(ref, controlled)
upsertOwnerRef(*ref, controlled)
return nil
}

Expand All @@ -122,18 +115,13 @@ func SetOwnerReference(owner, object metav1.Object, scheme *runtime.Scheme, opts
if err != nil {
return err
}
ref := metav1.OwnerReference{
APIVersion: gvk.GroupVersion().String(),
Kind: gvk.Kind,
UID: owner.GetUID(),
Name: owner.GetName(),
}
ref := NewOwnerRef(owner, gvk)
for _, opt := range opts {
opt(&ref)
opt(ref)
}

// Update owner references and return.
upsertOwnerRef(ref, object)
upsertOwnerRef(*ref, object)
return nil
}

Expand All @@ -154,11 +142,7 @@ func RemoveOwnerReference(owner, object metav1.Object, scheme *runtime.Scheme) e
return err
}

index := indexOwnerRef(owners, metav1.OwnerReference{
APIVersion: gvk.GroupVersion().String(),
Name: owner.GetName(),
Kind: gvk.Kind,
})
index := indexOwnerRef(owners, *NewOwnerRef(owner, gvk))
if index == -1 {
return fmt.Errorf("%T does not have an owner reference for %T", object, owner)
}
Expand All @@ -171,14 +155,7 @@ func RemoveOwnerReference(owner, object metav1.Object, scheme *runtime.Scheme) e
// HasControllerReference returns true if the object
// has an owner ref with controller equal to true
func HasControllerReference(object metav1.Object) bool {
owners := object.GetOwnerReferences()
for _, owner := range owners {
isTrue := owner.Controller
if owner.Controller != nil && *isTrue {
return true
}
}
return false
return metav1.GetControllerOfNoCopy(object) != nil
}

// HasOwnerReference returns true if the owners list contains an owner reference
Expand All @@ -188,20 +165,17 @@ func HasOwnerReference(ownerRefs []metav1.OwnerReference, obj client.Object, sch
if err != nil {
return false, err
}
idx := indexOwnerRef(ownerRefs, metav1.OwnerReference{
APIVersion: gvk.GroupVersion().String(),
Name: obj.GetName(),
Kind: gvk.Kind,
})
idx := indexOwnerRef(ownerRefs, *NewOwnerRef(obj, gvk))
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay. gonna change it soon

Copy link
Author

@HiranmoyChowdhury HiranmoyChowdhury Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@troy0820 WDYT about this: #2958 (comment)

return fmt.Errorf("%T owner is not the controller reference for %T", owner, object)
}

ro, ok := owner.(runtime.Object)
if !ok {
return fmt.Errorf("%T is not a runtime.Object, cannot call RemoveControllerReference", owner)
Expand All @@ -211,19 +185,7 @@ func RemoveControllerReference(owner, object metav1.Object, scheme *runtime.Sche
return err
}
ownerRefs := object.GetOwnerReferences()
index := indexOwnerRef(ownerRefs, metav1.OwnerReference{
APIVersion: gvk.GroupVersion().String(),
Name: owner.GetName(),
Kind: gvk.Kind,
})

if index == -1 {
return fmt.Errorf("%T does not have an controller reference for %T", object, owner)
}

if ownerRefs[index].Controller == nil || !*ownerRefs[index].Controller {
return fmt.Errorf("%T owner is not the controller reference for %T", owner, object)
}
index := indexOwnerRef(ownerRefs, *metav1.NewControllerRef(owner, gvk))

ownerRefs = append(ownerRefs[:index], ownerRefs[index+1:]...)
object.SetOwnerReferences(ownerRefs)
Expand All @@ -250,6 +212,16 @@ func indexOwnerRef(ownerReferences []metav1.OwnerReference, ref metav1.OwnerRefe
return -1
}

// NewOwnerRef creates an OwnerReference pointing to the given owner.
func NewOwnerRef(owner metav1.Object, gvk schema.GroupVersionKind) *metav1.OwnerReference {
return &metav1.OwnerReference{
APIVersion: gvk.GroupVersion().String(),
Kind: gvk.Kind,
Name: owner.GetName(),
UID: owner.GetUID(),
}
}

func validateOwner(owner, object metav1.Object) error {
ownerNs := owner.GetNamespace()
if ownerNs != "" {
Expand All @@ -266,16 +238,7 @@ func validateOwner(owner, object metav1.Object) error {

// Returns true if a and b point to the same object.
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold

No, this isn't correct as UIDs can change across backups and clusters. References should be compared like above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I am gonna change this and I guess rest are okay

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this will ensure owner by validating namespace while removing owner ref or controller ref

}

// OperationResult is the action result of a CreateOrUpdate call.
Expand Down
Loading