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

[keeganwitt]

This solves #2989 by providing new public functions in controllerutil for adding and removing OwnerReferences (current public functions only accept Objects). This seemed like a better approach than making the current private methods it uses public.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: keeganwitt
Once this PR has been reviewed and has the lgtm label, please assign joelanford for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: keeganwitt / name: Keegan Witt (c9a1896)

[k8s-ci-robot]

Welcome @keeganwitt!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @keeganwitt. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[keeganwitt]

We should think carefully about the naming convention. Ideally, I would have used SetOwnerReference and RemoveOwnerReference for these new methods, and SetOwner and RemoveOwner for the existing methods, but that would be a breaking change.

[troy0820]

/hold

[troy0820]

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?

[keeganwitt]

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

[keeganwitt]

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

?

[troy0820]

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.

[keeganwitt]

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.

[troy0820]

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

[keeganwitt]

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.

[troy0820]

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.

[keeganwitt]

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?

[alvaroaleman]

What you are asking for here can be done with an if !sices.ContainsFunc(o.OwnerRefs, ownerRef) { o.OwerRefs = append(o.OwnerRefs, ownerRef}, respective slices.DeleteFunc(o.OwnerRefs, func(or metav1.OwnerRef) bool {...}).

Is it really necessary to add something like this? The main reason that I am not super convinced is that none of this is really specific to OwnerReferences, what we are adding here is basically generic slice functionality but only for owner refs.

[keeganwitt]

What you are asking for here can be done with an if !sices.ContainsFunc(o.OwnerRefs, ownerRef) { o.OwerRefs = append(o.OwnerRefs, ownerRef}, respective slices.DeleteFunc(o.OwnerRefs, func(or metav1.OwnerRef) bool {...}).

The ContainsFunc only takes a single predicate. How can you capture the logic of aGV.Group == bGV.Group && a.Kind == b.Kind && a.Name == b.Name with a single predicate?

Also, even if you could do this with generic slice functions, the logic mentioned above would still have to be copy/pasted because it's also not a public function.

[troy0820]

I can't speak for alvaroaleman but I do know the non exposed function upsertOwnerRef

1. Gets the ownerRefs
2. see's if the ownerRef is in the ownerReferences
3. if not, it appends it to the owner reference
4. sets the owner references

Which to me seems equivalent to what was provided in his comment. The functionality of this is not specific to owner references as it's more of a golang slices thing.

The ContainsFunc only takes a single predicate. How can you capture the logic of aGV.Group == bGV.Group && a.Kind == b.Kind && a.Name == b.Name with a single predicate?

That's what I was asking around sidestepping the logic that exists to check that with the original functions. The two issues I listed talks about why we have a workaround with GVK and not using that from the type directly.

[keeganwitt]

2. see's if the ownerRef is in the ownerReferences

This is the part that isn't just a generic Go slices thing. The definition of whether two references are the same is logic that is defined in this package. But I'd agree that the rest of the functionality of these functions isn't specific to owner references. I'd be equally happy with exposing a version of HasOwnerReference that takes an OwnerReference instead of Object, since the rest is simple slices manipulation, if that's a less objectionable path. The important thing was to be able to have the logic in my controller change if the logic in controller-runtime changed (e.g. now a new field is required to match to be equivalent).

That's what I was asking around sidestepping the logic that exists to check that with the original functions. The two issues I listed talks about why we have a workaround with GVK and not using that from the type directly.

Any suggestions for an alternative? If I want to use the existing functions, I have to do something like

var owner ctrlClient.Object
switch ownerReference.Kind {
case "ReplicaSet":
	owner = &appsv1.ReplicaSet{}
case "StatefulSet":
	owner = &appsv1.StatefulSet{}
case "DaemonSet":
	owner = &appsv1.DaemonSet{}
case "Job":
	owner = &batchv1.Job{}
default:
	return fmt.Errorf("unexpected owner reference kind %v", ownerReference.Kind)
}
c.client.Get(ctx, ctrlClient.ObjectKey{Namespace: c.pod.Namespace, Name: ownerReference.Name}, owner)
controllerutil.SetOwnerReference(owner, object, c.scheme)

And then hope I've accounted for all the kinds that might create a pod.

[keeganwitt]

The ContainsFunc only takes a single predicate. How can you capture the logic of aGV.Group == bGV.Group && a.Kind == b.Kind && a.Name == b.Name with a single predicate?

What you are asking for here can be done with an if !sices.ContainsFunc(o.OwnerRefs, ownerRef) { o.OwerRefs = append(o.OwnerRefs, ownerRef}, respective slices.DeleteFunc(o.OwnerRefs, func(or metav1.OwnerRef) bool {...}).

Ah, I think I see what you were thinking now. I was thinking I'd need something like Java's compareTo(), which takes two arguments. But I can actually refer to the variable outside the function, like

func addOwnerReferenceIfNotPresent(owner metav1.OwnerReference, object metav1.Object) {
	if !slices.ContainsFunc(object.GetOwnerReferences(), func(o metav1.OwnerReference) bool {
		aGV, err := schema.ParseGroupVersion(o.APIVersion)
		if err != nil {
			return false
		}
		bGV, err := schema.ParseGroupVersion(owner.APIVersion)
		if err != nil {
			return false
		}
		return aGV.Group == bGV.Group && o.Kind == owner.Kind && o.Name == owner.Name
	}) {
		object.SetOwnerReferences(append(object.GetOwnerReferences(), owner))
	}
}

[troy0820]

This is the part that isn't just a generic Go slices thing. The definition of whether two references are the same is logic that is defined in this package

You skip over that with the function you provide. You provided a function that exposes upsertOwnerRef which just does what @alvaroaleman suggested.

var owner ctrlClient.Object
switch ownerReference.Kind {
case "ReplicaSet":
	owner = &appsv1.ReplicaSet{}
case "StatefulSet":
	owner = &appsv1.StatefulSet{}
case "DaemonSet":
	owner = &appsv1.DaemonSet{}
case "Job":
	owner = &batchv1.Job{}
default:
	return fmt.Errorf("unexpected owner reference kind %v", ownerReference.Kind)
}
c.client.Get(ctx, ctrlClient.ObjectKey{Namespace: c.pod.Namespace, Name: ownerReference.Name}, owner)
controllerutil.SetOwnerReference(owner, object, c.scheme)

And then hope I've accounted for all the kinds that might create a pod.

I wouldn't expect that the controllerutil package do this as it is targeting specific kinds and not generalize as the package is currently. These things create pods but that doesn't mean I can't create a resource specific to my domain that creates a pod and be left with trying to extend this logic.

[keeganwitt]

I wouldn't expect that the controllerutil package do this as it is targeting specific kinds and not generalize as the package is currently. These things create pods but that doesn't mean I can't create a resource specific to my domain that creates a pod and be left with trying to extend this logic.

I was talking about in my controller, not controllerutil.

[keeganwitt]

You skip over that with the function you provide. You provided a function that exposes upsertOwnerRef which just does what @alvaroaleman suggested.

I was suggesting that we add a public version of indexOwnerRef instead of what I did in this PR.