Cannot set namespace of informers created by informer factory

[chancez]

Currently the generated code outputs an interface like this:

// Interface provides access to all the informers in this group version.
type Interface interface {
	// Foos returns a FooInformer.
	Foos() FooInformer
	// Bars returns a BarInformer.
	Bars() BarInformer
	// FooBars returns a FooBarInformer.
	FooBars() FooBarInformer
}

These informers return an interface like this:

type FooInformer interface {
	Informer() cache.SharedIndexInformer
	Lister() v1alpha1.FooLister
}

However there is no way to specify you want an informer restricted to a particular namespace. The only alternative seems to be to use the method NewFooInformer constructor, but then I'm creating all my Informers and Listers myself.

[tamalsaha]

v1alpha1.FooLister should have a method list Foos(namspace string). That should do what you want.

[chancez]

@tamalsaha That's only for retrieving from the cache in the informer, it doesn't actually make the informer only watch/list from a particular namespace.

[tamalsaha]

it doesn't actually make the informer only watch/list from a particular namespace.

Ah! I did not realize that's what you wanted. Probably just write the code directly yourself. I personally find that less confusing. It is a bit of boilerplate but more readable then these factory methods with lots of args.

[chancez]

That's what I'm currently doing, but it would still be nice to be able to control the informer's namespace long term.

Theres a TON of boilerplate I have to do that the generated code completely eliminates, and it also provides a lot of nice methods for handling coordinating waiting for caches to sync, and doing shutdowns of the queues that I'm effectively just re-implementing myself. It would be preferable to use the generated code to avoid bugs and mistakes in doing this, and ensuring things are correctly setup (it's not exactly simple).

[chancez]

What do you think about maybe making the Informer interfaces being updated to accept a namespace as an argument?

[munnerz]

I've run into this as I'm developing controllers, and have had to develop my own pretty-flakey and overly verbose SharedInformerFactory that keys on GroupVersionKind & the namespace: https://github.com/jetstack-experimental/cert-manager/blob/master/pkg/util/kube/factory.go

It'd be great if we could get this into the generator. I had a go at implementing this a while back but didn't get too far, mostly on account of difficulties expressing it with the current structure of the interface. How do you imagine the new interfaces looking?

Further to this however, I'm now also building an Initializer into my application which requires setting the IncludeUninitialized field on the ListOptions. It'd be great if there were some way to tweak ListOptions via the generated SharedInformerFactory structures to save all the boilerplate it creates in downstream applications.

I'm happy to work on a PR for this if we can work out what it'll look like, and if we can get any word whether it might be merged.

/cc @sttts @timothysc @deads2k

[sttts]

/cc @ncdc

[munnerz]

So I've had another go implementing this - whilst on the surface it may seem simple, it's actually quite a complex change.

I opted to add namespace to the version interface (i.e. this one: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/sample-apiserver/pkg/client/informers/externalversions/wardle/v1alpha1/interface.go#L48) if the resource is a namespaced one. I then added a namespace field to the flunderInformer structure (again, only if the resource is namespaced).

This works quite nicely, and is relatively sane to use from a user perspective (looks like: factory.Core().V1().Pods("default").Informer()) but we then get to the tricky bit... the factory structure itself currently stores a cache of SharedIndexInformer, keyed with the reflect.Type of each informer. This needs extending to also key on namespace. For simplicity, I am thinking we could change the type of f.informers (here: https://github.com/kubernetes/client-go/blob/master/informers/factory.go#L50) to be map[string]cache.SharedIndexInformer, where the string component is some kind of serialised tuple of the namespace and type of the informer.

I'm not too sure what can be used here to 'key' the (string, reflect.Type) pair in a robust way that doesn't introduce collisions - does anyone have any advice here?

[munnerz]

The other option is we make the SharedInformerFactory itself be a namespaced SharedInformerFactory, but I think that limits its use, and could make working with cluster scoped resources more painful.

I also had a go at doing this with ListOptions, however have found that considerably harder as the reflector actually creates the v1.ListOptions that get passed to the WatchFunc and ListFunc in the SharedIndexInformer (specifically, it sets the ResourceVersion and TimeoutSeconds fields). This means we'll need to either 'merge' a user provided ListOptions with the incoming list options passed to the List/WatchFunc, or alternatively accept a function of type func(*v1.ListOptions) that can modify the passed in ListOptions in NewXYZInformer(client versioned.Interface, resyncPeriod time.Duration, indexers cache.Indexers) .

Whilst this method does work, I worry it makes using the informers over verbose. That said, I think setting the likes of ListOptions is only going to become more important over time with the emergence of API Initializers etc.

[chancez]

The other option is we make the SharedInformerFactory itself be a namespaced SharedInformerFactory, but I think that limits its use, and could make working with cluster scoped resources more painful.

I would have no objects to this, and would almost prefer it. This is needed for operators/controllers running with limited permissions, running in their own namespace. You can always pass in meta/v1.NamespaceAll in if you need something cluster wide, right?

[munnerz]

I've put in a pull request here to add a namespace argument to the NewSharedInformerFactory constructor. I'm currently generating my own informer factories for core types in order to use this with the existing types today, before the main codebase integrates these changes.