✨ add EnqueueRequestForAnnotation enqueues Requests based on the presence of an annotation to watch resources

[camilamacedo86]

Story
As an operator developer, it would be nice to be able to watch dependent resources based on the presence of an annotation in the dependent resource.

Description

While owner references are the idiomatic way to manage dependent resources, they don't solve all use cases. Owner references have restrictions that limit which objects can own other objects.

An annotation-based watch handler does not have these restrictions. However, they don't provide the same feature set either.

The watch handler can be used to trigger reconciliation of a parent resource based on changes to dependent resources that include an annotation. The purpose of this handler is to support cross-scope ownership relationships that are not supported by native owner references.

Since this is a niche feature that should only be used in specific circumstances. Note that for example, owner references are used for garbage collection. If a resource's owner is deleted, the resource will also be deleted. Because of this, annotation references need to be paired with a finalizer so that the operator can manually garbage collect.

Example

	// Watch ClusterRoles that has the following annotations:
	// ...
	// annotations:
	//    watch.kubebuilder.io/owner-namespaced-name:my-namespace/my-replicaset
	//    watch.kubebuilder.io/owner-type:apps.ReplicaSet
	// ...
	// It will enqueue a Request to the primary-resource-namespace when some change occurs in a ClusterRole resource with these
	// annotations
	if err := c.Watch(&source.Kind{
		// Watch cluster roles
		Type: &rbacv1.ClusterRole{}},

		// Enqueue ReplicaSet reconcile requests using the
		// namespacedName annotation value in the request.
		&handler.EnqueueRequestForAnnotation{schema.GroupKind{Group:"ReplicaSet", Kind:"apps"}}); err != nil {
		entryLog.Error(err, "unable to watch Cluster Role")
		os.Exit(1)
	}

Closes: kubernetes-sigs/kubebuilder#888

[camilamacedo86]

/assign @vincepri

[vincepri]

Can't this be a predicate on the watch?

[camilamacedo86]

Hi @vincepri,

Can't this be a predicate on the watch?

Thank you for your input. However, to do a predicate on the watch we still needing the ownership. Am I right? Note that:

The watch handler can be used to trigger reconciliation of a parent resource based on changes to dependent resources that include an annotation. The purpose of this handler is to support cross-scope ownership relationships that are not supported by native owner references.

I added more info in the first comment. Please, let me know if it makes more sense now and if you `re ok with.

[joelanford]

@camilamacedo86 The Closes link in the description points to the wrong repo. I think you meant to use kubernetes-sigs/kubebuilder#888

[vincepri]

However, to do a predicate on the watch we still needing the ownership. Am I right?

If you get a controller.Controller back when you build your cluster, you can use controller.Watch directly without the need for ownership.

For example:

• In Cluster API, we have a Machine and a Cluster objects
• The machine controller watches Clusters https://github.com/kubernetes-sigs/cluster-api/blob/master/controllers/machine_controller.go#L94-L96
• Which calls this function https://github.com/kubernetes-sigs/cluster-api/blob/cb8f8e40d3a013a9cf8328f7a3df5553501f7baf/util/util.go#L542-L554 that calls controller.Watch with predicates

[joelanford]

@vincepri EnqueueRequestForAnnotation is basically a specific implementation of an EnqueueRequestsFromMapFunc. If I'm understanding correctly, a predicate could filter out events for objects that don't have the expected annotations, and the handler map function would convert the annotation to a request using the namespacedName annotation value.

So the answer is, yes, this could be done with existing handler and predicate features.

The question we need to answer is whether the problem this handler sets out to solve is widespread enough that the controller-runtime community would find enough value in it to justify its addition here.

This solution isn't perfect though.

• It requires developers to code their reconcilers to add these annotations correctly on child resources (this is theoretically no more difficult than adding an owner reference though).
• It requires developers to add a finalizer on the primary resource if they need to cleanup these child resources since garbage collection doesn't work in all cases.
• Barring some ugly hacking around of annotations, it only allows a resource to be "owned" by one other object (owner references are a list, annotations are a map)

[camilamacedo86]

/assign @mengqiy

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: camilamacedo86
To complete the pull request process, please assign mengqiy
You can assign the PR to them by writing /assign @mengqiy in a comment when ready.

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

[camilamacedo86]

/assign @mengqiy

[alvaroaleman]

I have a bit of a feeling that this change is a bit of a workaround for multiple problems:
a) Writing custom map functions is too hard and not very intuitive
b) OnwerRefs do not support cross-namespace referencing

For a), I think this is a totally solvable problem, it just seems that no one bothered to do that yet (We had a bunch of downstream code for this in a different project but didn't upstream it)

For b), I think we should first have a discussion with sig-apimachinery if this is something that just isn't possible today or something that will never be possible. If the latter, we should spend a bit of time on thinking about nice api that replaces ownerRefs for that case. One problem I see for example with the current approach is that at most one owner can be declared. Also, I am not a huge fan of putting the burden of remembering to set finalizers and cleaning up objects if their parents are gone (AKA: build a custom garbage collector controller) onto the users of c-r.

[camilamacedo86]

Hi @alvaroaleman,

So, if I understood you are ok with move with it but you also would like to see customization/rfe in the garbage collector controller in order to NOT be required to work with finalizers. I mean, allow it also remove the chields based on the annotations. Am I right? Could we work to do this improvement in a follow-up? WDYT?

[alvaroaleman]

So, if I understood you are ok with move with

Sorry I don't understand what you mean by that?

But no, I am not a fan of this right now because:

• It isn't clear if this is a problem that should be solved downstream or upstream
• There has been no design doc about this and the current design is lacking

[camilamacedo86]

HI @alvaroaleman,

There has been no design doc about this and the current design is lacking

Could you please let me know where we can add a design doc about this? Also, when you say that the current design is lacking are you speaking about the need to use the finalizer always? So, if we are able to delete the child resources by using annotations it would be solved? Am I right?

It isn't clear if this is a problem that should be solved downstream or upstream

Over it, what in your point of view what is missing for we are able to clarify it?

As you said:

For a), I think this is a totally solvable problem, it just seems that no one bothered to do that yet (We had a bunch of downstream code for this in a different project but didn't upstream it)
It isn't clear if this is a problem that should be solved downstream or upstream

So would not it be a valid reason to incorporate to upstream?

[alvaroaleman]

I think before we start implementing something that is only needed because upstream does not support a certain feature we should:

• Find out why upstream does not support it
• Check if that reason still applies to upstream/if it can/should be done upstream
• Check if that reason applies to downstream

The reason is apparently that:

liggitt  20 minutes ago
namespaces are the traditional acl boundary. extending ownership across that boundary was intentionally avoided in the current design

alvaroaleman  16 minutes ago
Or is the problem that this might be abused to get info about the existence of objects in other namespaces?

liggitt  15 minutes ago
to get info or to block owner deletion

Source: https://kubernetes.slack.com/archives/C0EG7JC6T/p1587388988464000

The get info part definitely applies to us, the block owner part might apply, depending on how ppl implement garbage collection (Which they shouldn't need to do in the first place).

Can you please start a discussion with upstream sig-apimachinery if there is anything that can be done to support an usecase like this in a safe manner?

/hold

[camilamacedo86]

We are closing this one. Based on the feedback shows that it would not be generic enough to upstream.

[mcristina422]

I was thinking about this PR when I needed to write a controller that reconciled on k8s ServiceAccount types and the secrets that back them.

The Secret it refers to has no OwnerRef but there are annotations that refer to the "owning" ServiceAccount

kubernetes.io/service-account.name and kubernetes.io/service-account.uid