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

✨ Add public functions to add/remove OwnerReferences without the referenced object #2991

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

✨ Add public functions to add/remove OwnerReferences without the referenced object #2991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 30 additions & 0 deletions pkg/controller/controllerutil/controllerutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,36 @@ func RemoveOwnerReference(owner, object metav1.Object, scheme *runtime.Scheme) e
return nil
}

// AppendOwnerReference is a helper method to make sure the given object contains a provided owner reference.
// This allows you to declare that owner has a dependency on the object without specifying it as a controller.
// If a reference already exists, it'll be overwritten with the newly provided version.
Comment on lines +171 to +173
Copy link
Member

Choose a reason for hiding this comment

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

This comment is exactly like SetOwnerReference. How is this any different besides not using the scheme to get the GVK and skipping validateOwner?

Copy link
Author

@keeganwitt keeganwitt Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I couldn't think of a way to use Get in sigs.ki8s.io/controller-runtime/pkg/client without know what struct I need in advance (ReplicaSet, Job, etc). If there's a way to do this, then perhaps this entire request is unneeded.

The difference between this function and SetOwnerReference is that it takes an OwnerReference rather than an Object to use as the new 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 also sidesteps the validation that the function provides when adding an owner reference. Not sure if want to do that or allow the appending of owner references to any object outside of the scope where the owner would be setting it. This seems to be very dangerous where the lifecycle is now beholden to an owner reference the owner object didn't explicitly set.

Copy link
Author

Choose a reason for hiding this comment

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

I don't follow this second concern. In my experience, it's not at all unusual for a controller to create a resource on behalf of something, and have the resource share the lifecycle of that thing. Is your concern only about resources created by other controllers?

func AppendOwnerReference(owner metav1.OwnerReference, object metav1.Object) {
upsertOwnerRef(owner, object)
}

// DropOwnerReference is a helper method to make sure the given object removes a provided owner reference.
// This allows you to remove the owner to establish a new owner of the object in a subsequent call.
func DropOwnerReference(owner metav1.OwnerReference, object metav1.Object) error {
ownerRefs := object.GetOwnerReferences()
index := indexOwnerRef(ownerRefs, metav1.OwnerReference{
APIVersion: owner.APIVersion,
Name: owner.Name,
Kind: owner.Kind,
})
Comment on lines +182 to +186
Copy link
Member

Choose a reason for hiding this comment

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

We usually do this with GVK because relying on this may not be stable. (The workaround exist because of issues around this). Getting the GVK requires the scheme. Adding the scheme makes this method the RemoveOwnerReference which already exists.

I am trying to understand the repeat logic around this functionality. What are we trying to circumvent utilizing this logic?

Copy link
Author

Choose a reason for hiding this comment

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

Could you elaborate on that instability? Is that documented somewhere?

Copy link
Author

@keeganwitt keeganwitt Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Would this concern be addressed with

groupVersion, err := schema.ParseGroupVersion(owner.APIVersion)
if err != nil {
	return err
}
ownerRefs := object.GetOwnerReferences()
index := indexOwnerRef(ownerRefs, metav1.OwnerReference{
	APIVersion: groupVersion.String(),
	Name:       owner.Name,
	Kind:       owner.Kind,
})

?

Copy link
Member

Choose a reason for hiding this comment

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

Discussed here: #2882 (comment)
Reference:
kubernetes/kubernetes#3030
kubernetes/kubernetes#80609

I am still trying to understand what you are trying to do because this looks very similar to what is already in controllerutil and conflating the two different functions where the logic is the same can cause confusion on what we are trying to achieve.

Copy link
Author

Choose a reason for hiding this comment

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

This function I added just so there was a pair of functions that work with OwnerReference instead of Object. I don't personally need the DropOwnerReference function.


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)
}

ownerRefs = append(ownerRefs[:index], ownerRefs[index+1:]...)
object.SetOwnerReferences(ownerRefs)
return nil
}

// HasControllerReference returns true if the object
// has an owner ref with controller equal to true
func HasControllerReference(object metav1.Object) bool {
Expand Down
178 changes: 178 additions & 0 deletions pkg/controller/controllerutil/controllerutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,184 @@ var _ = Describe("Controllerutil", func() {
})
})

var _ = Describe("Controllerutil", func() {
Describe("AppendOwnerReference", func() {
It("should set ownerRef on an empty list", func() {
rs := &appsv1.ReplicaSet{}
dep := metav1.OwnerReference{
APIVersion: "v1",
Kind: "Deployment",
Name: "foo",
UID: "foo-uid",
}
controllerutil.AppendOwnerReference(dep, rs)
Expect(rs.OwnerReferences).To(ConsistOf(metav1.OwnerReference{
Name: "foo",
Kind: "Deployment",
APIVersion: "extensions/v1beta1",
UID: "foo-uid",
}))
})

It("should not duplicate owner references", func() {
rs := &appsv1.ReplicaSet{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: []metav1.OwnerReference{
{
Name: "foo",
Kind: "Deployment",
APIVersion: "extensions/v1beta1",
UID: "foo-uid",
},
},
},
}
dep := metav1.OwnerReference{
APIVersion: "v1",
Kind: "Deployment",
Name: "foo",
UID: "foo-uid",
}
controllerutil.AppendOwnerReference(dep, rs)
Expect(rs.OwnerReferences).To(ConsistOf(metav1.OwnerReference{
Name: "foo",
Kind: "Deployment",
APIVersion: "extensions/v1beta1",
UID: "foo-uid",
}))
})

It("should update the reference", func() {
rs := &appsv1.ReplicaSet{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: []metav1.OwnerReference{
{
Name: "foo",
Kind: "Deployment",
APIVersion: "extensions/v1alpha1",
UID: "foo-uid-1",
},
},
},
}
dep := metav1.OwnerReference{
APIVersion: "v1",
Kind: "Deployment",
Name: "foo",
UID: "foo-uid-2",
}
controllerutil.AppendOwnerReference(dep, rs)
Expect(rs.OwnerReferences).To(ConsistOf(metav1.OwnerReference{
Name: "foo",
Kind: "Deployment",
APIVersion: "extensions/v1beta1",
UID: "foo-uid-2",
}))
})

It("should remove the owner reference", func() {
rs := &appsv1.ReplicaSet{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: []metav1.OwnerReference{
{
Name: "foo",
Kind: "Deployment",
APIVersion: "extensions/v1alpha1",
UID: "foo-uid-1",
},
},
},
}
dep := metav1.OwnerReference{
APIVersion: "v1",
Kind: "Deployment",
Name: "foo",
UID: "foo-uid-2",
}
controllerutil.AppendOwnerReference(dep, rs)
Expect(rs.OwnerReferences).To(ConsistOf(metav1.OwnerReference{
Name: "foo",
Kind: "Deployment",
APIVersion: "extensions/v1beta1",
UID: "foo-uid-2",
}))
Expect(controllerutil.DropOwnerReference(dep, rs)).ToNot(HaveOccurred())
Expect(rs.GetOwnerReferences()).To(BeEmpty())
})

It("should error when trying to remove the reference that doesn't exist", func() {
rs := &appsv1.ReplicaSet{
ObjectMeta: metav1.ObjectMeta{},
}
dep := metav1.OwnerReference{
APIVersion: "v1",
Kind: "Deployment",
Name: "foo",
UID: "foo-uid-2",
}
Expect(controllerutil.DropOwnerReference(dep, rs)).To(HaveOccurred())
Expect(rs.GetOwnerReferences()).To(BeEmpty())
})

It("should error when trying to remove an owner that doesn't exist", func() {
rs := &appsv1.ReplicaSet{
ObjectMeta: metav1.ObjectMeta{},
}
dep := metav1.OwnerReference{
APIVersion: "v1",
Kind: "Deployment",
Name: "foo",
UID: "foo-uid-2",
}
dep2 := metav1.OwnerReference{
APIVersion: "v1",
Kind: "Deployment",
Name: "foo",
UID: "foo-uid-3",
}
controllerutil.AppendOwnerReference(dep, rs)
Expect(controllerutil.DropOwnerReference(dep2, rs)).To(HaveOccurred())
Expect(rs.GetOwnerReferences()).To(HaveLen(1))
})

It("should error when DropOwnerReference owner's controller is set to false", func() {
rs := &appsv1.ReplicaSet{
ObjectMeta: metav1.ObjectMeta{},
}
dep := metav1.OwnerReference{
APIVersion: "v1",
Kind: "Deployment",
Name: "foo",
UID: "foo-uid-2",
}
controllerutil.AppendOwnerReference(dep, rs)
Expect(controllerutil.DropOwnerReference(dep, rs)).To(HaveOccurred())
})

It("should error when DropOwnerReference passed in owner is not the owner", func() {
rs := &appsv1.ReplicaSet{
ObjectMeta: metav1.ObjectMeta{},
}
dep := metav1.OwnerReference{
APIVersion: "v1",
Kind: "Deployment",
Name: "foo",
UID: "foo-uid-2",
}
dep2 := metav1.OwnerReference{
APIVersion: "v1",
Kind: "Deployment",
Name: "foo-2",
UID: "foo-uid-42",
}
controllerutil.AppendOwnerReference(dep, rs)
controllerutil.AppendOwnerReference(dep2, rs)
Expect(controllerutil.DropOwnerReference(dep2, rs)).To(HaveOccurred())
Expect(rs.GetOwnerReferences()).To(HaveLen(2))
})
})
})

Describe("SetControllerReference", func() {
It("should set the OwnerReference if it can find the group version kind", func() {
rs := &appsv1.ReplicaSet{}
Expand Down