/sig api-machinery

/retest

This may be better to implement as a NewNamespacedSharedInformerFactory function instead of modifying the existing signature to avoid having to touch so many files.

@munnerz If it's a function, that's not so bad, but I wouldn't want to see a new method added to the interface if thats what you mean.

@chancez nope, I mean adding something like:

func NewNamespacedSharedInformerFactory(client {{.clientSetInterface|raw}}, namespace string, defaultResync {{.timeDuration|raw}}) SharedInformerFactory {

and leaving the existing NewSharedInformerFactory function as-is (not changing any interfaces, just add a new constructor method to the package)

Edit: I misunderstood

That would work. That seems like it might interact weirdly with the lister though, which expects a namespace argument which could be different namespace from the one the informer is using.

I've managed to resolve that by placing the non exported constructor method onto the typed interface. The namespace is plumbed through each group and version interface, along with the factory through each group and versions New method.

I've put an example of informers generated with this PR here for you to look at: https://github.com/munnerz/cert-manager/blob/namespaced-informers/third_party/informers/core/v1/configmap.go#L64

You can see here how the namespace is plumbed through to the typed informers: https://github.com/munnerz/cert-manager/blob/namespaced-informers/third_party/informers/factory.go#L121

@munnerz Yah, I realized it would work, but what about the Lister from the informer? We can now pass the namespace to the FooNamespaceLister that may be different from the informer's namespace.

Hmm, that's a good point. I don't see how we can work around this then with the current design of the SharedIndexInformer interface/Lister interface/version informer interface.

If a user were to attempt to get or list objects in another namespace, it would just return not found. Perhaps this, combined with it being an additional constructor function (and so not-so invasive), means we could include this?

/cc @ncdc

This somehow contradicts the goal of shared informers as this scales with the number of namespaces. We could have a filter method on the existing shared informers which returns an informer filtered by namespace.

I agree with @sttts - I don't see any good usecase for that. If you have anything specific on your mind I would like to see it.

So my use-case for being able to specify a namespace comes from the need to run my application in an environment that constrains the controller to only run in a single namespace. This is a request I've had numerous times, and it usually comes from people running a multi-tenant kubernetes cluster.

Another issue I have, that is similarly related, is that I want to be able to modify the ListOptions used on the List & Watch requests. This is especially important with the addition of Initializers into Kubernetes, as I need a way to set IncludeUninitialized: true. With the current design, I need to create my own implementation of a factory so that I can add this extra field onto the ListOptions. Building on @sttts suggestion, perhaps some way to filter/hook the ListOptions would work - but I'm not sure how we could do this in a clean way.

I think this is probably something that more and more users are going to ask for as people start building out initializers. For now I will probably just fork the informer-gen and create my own informers for core API types, built against k8s.io/api/k8s.io/client-go. If this is in fact the best way it should be done, we should probably document this on a "build an initializer" page or something similar.

Let's take a look at what functions the shared informer factory performs:

1. It creates shared informers on the fly the first time they're referenced in your code
2. It keeps track of which ones you've referenced prior to starting the factory, so it can start them all
3. It allows you to wait for all of its informers' caches to sync
4. It allows you to request a generic informer based on a GroupVersionResource

All of the generated shared informer constructors take in a namespace parameter (assuming the resource is namespace-scoped). If all you want is a set of shared informers for a single namespace, you can get that today, albeit without using the factory. You can construct each informer by hand, passing in the desired namespace. You can start them as needed. And you can construct the associated lister for each shared informer. BUT, this is stuff you'd have to do by hand, that the factory does for you automatically (assuming you want all namespaces).

Having said all that, I don't think that what @munnerz is asking for is unreasonable, as I've seen multiple people asking for the ability to have a shared informer factory specific to a single namespace. I'd prefer to see it as a new constructor instead of a modification to the existing one (NewNamespacedSharedInformerFactory, as described above).

My use case is the same. My controller is supposed to run with minimal permissions in it's own namespace (no access to non-namespaced resources) and this would greatly reduce the amount of boilerplate i have to write.

Additionally, I've already ha.d issues where I forgot to add a new informer to my function that waits for caches to sync for all my informers. The autogenerated factories have methods that automatically handle this, and would help prevent mistakes like that.

Also I think I'm okay with listers being a bit odd with WRT namespaces, they already are weird with namespaces, even when not using the factories.

lgtm

/lgtm

/approve no-issue

/retest

@lavalamp @sttts tests are now passing again after problems with Prow and a few flakes.

If you could take a final look over and approve I'd greatly appreciate it!

@lavalamp @sttts more conflicts came up which I have once again resolved. Looks like this is going to happen a lot as this PR touches pkg/client!

Hopefully test flakes won't hold this up too much again 😄 but this will need another /lgtm since rebasing!

/lgtm

@smarterclayton are you fine with this?

/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, munnerz, sttts

Associated issue: 9

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

Automatic merge from submit-queue (batch tested with PRs 55403, 54660, 55165). If you want to cherry-pick this change to another branch, please follow the instructions here.