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.

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

Event filter should be triggered after checking OwnerReferences for Ownsed objects #2351

Closed
samox73 opened this issue May 25, 2023 · 3 comments

Comments

@samox73
Copy link

samox73 commented May 25, 2023

To my understanding, when creating a new controller with ctrl#Manager#NewControllerManagedBy and specifying that the controller owns a resource--e.g. Deployment--an object of said type only triggers the reconciliation loop, if its OwnerReferences field contains the id of the controller.

When using a predicate function to filter certain objects based on some logic--e.g. the object's namespace--the predicate is called before the objects owner reference is asserted. Let's take an example:

ctrl.NewControllerManagedBy(mgr).
For(&myCRD).
Owns(&appsv1.Deployment{}).
WithEventFilter(myFilter()).
Complete(r)

The predicate myFilter will be called on ALL Deployments regardless of the Deployment's owner reference, which might filter the request anyway since it may not have the controller set as its owner.

@samox73
Copy link
Author

samox73 commented May 25, 2023

I just realized that filtering objects based on some logic may only be a niche case of WithEventFilter. For the use-case of filtering requests based on the object's namespace, would it be more sensible to use Watches instead of Owns combined with an event filter?

@alvaroaleman
Copy link
Member

alvaroaleman commented May 25, 2023

For better or worse, the pipeline is source -> predicate -> handler. An argument can be made both for having predicate before and after handler, which is why we are not going to change it.

Using Watches and filtering in the Handler would allow you to do the filtering, based on the owner, yeah.

@samox73
Copy link
Author

samox73 commented May 26, 2023

Thanks for the clarification @alvaroaleman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants