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

Question: defining optional informers #2309

Closed
benmoss opened this issue Oct 7, 2021 · 6 comments
Closed

Question: defining optional informers #2309

benmoss opened this issue Oct 7, 2021 · 6 comments

Comments

@benmoss
Copy link
Member

benmoss commented Oct 7, 2021

/kind question

Expected Behavior

I'd like to define a controller that can work with 3rd party resources if they are installed in the cluster, but fallback to different or no behavior if they are not installed.

Actual Behavior

Right now using sharedmain / EnableInformersOrDie my controller gets stuck starting informers because the types are not resolvable.

Additional Info

#2210 might be a solution, talked a little with @n3wscott about this.

An alternate solution might be to just have a way of starting an informer but declaring that it's okay to fail, and have some way to signal that failure to the controller so it can fallback to alternate behavior.

Another solution would be to push starting informers into user-land code so you could only try to start them if some check passes (ie if resourceInstalled { startInformer } )

@benmoss
Copy link
Member Author

benmoss commented Oct 7, 2021

cc @mattmoor

@mattmoor
Copy link
Member

mattmoor commented Oct 7, 2021

A workaround is to graph the shared informer factory and only get/start the informer if the resource is installed.

I'd rather not make the .Get() API any more complex than that, the point of those paths is that the dependency graph conveys what you require. Using the injected informer factory directly seems like a reasonable way to make this optional (that's what .Get() uses anyways).

Does that work?

@benmoss
Copy link
Member Author

benmoss commented Oct 7, 2021

graph the shared informer factory

Not sure I understand what this would mean. Is opting out of sharedmain a baseline assumption here?

@mattmoor
Copy link
Member

No, the webhook goes through sharedmain and it has a weak dependency on the secret lister here for exactly this reason:

pkg/webhook/webhook.go

Lines 133 to 138 in 9179f78

// Injection is too aggressive for this case because by simply linking this
// library we force consumers to have secret access. If we require that one
// of the admission controllers' informers *also* require the secret
// informer, then we can fetch the shared informer factory here and produce
// a new secret informer from it.
secretInformer := kubeinformerfactory.Get(ctx).Core().V1().Secrets()

If you access the informer this way, then you can make the informer optional, and guard it with some sort of environment check. If you'd want to make it so the resulting controller.Impl can be nil, then you might need some small changes to sharedmain, but I don't think it would take much.

@benmoss
Copy link
Member Author

benmoss commented Oct 11, 2021

/close

thanks! this makes sense

@knative-prow-robot
Copy link
Contributor

@benmoss: Closing this issue.

In response to this:

/close

thanks! this makes sense

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/test-infra repository.

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

No branches or pull requests

3 participants