✨ 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[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: keeganwitt / name: Keegan Witt (c9a18962ff187f353967321cb9fe05944cca1fce)

[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. :smiley:

[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

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

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale