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

code-generator: add NewFilteredSharedInformerFactory function #54660

Merged
merged 3 commits into from Nov 9, 2017

Conversation

@munnerz
Member

munnerz commented Oct 26, 2017

What this PR does / why we need it:

Adds a namespace option to the SharedInformerFactory constructor. This is useful when building controllers that may need to scope themselves to a namespace due to RBAC constraints.

Workarounds for this involve losing type safety if a user wants to use it for core APIs as well as a SharedInformerFactory type interface, as we have to deal with plain SharedIndexInformers (example here: https://github.com/jetstack-experimental/cert-manager/blob/master/pkg/util/kube/factory.go)

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Fixes kubernetes/code-generator#9

Special notes for your reviewer:

This will require updating all uses of SharedInformerFactory throughout the codebase. I'm going to follow up with later commits in this PR with these changes, but wanted to get this here to get some feedback on the way it's implemented.

Release note:

NONE

/cc @sttts @nikhita @deads2k

@munnerz

This comment has been minimized.

Show comment
Hide comment
@munnerz

munnerz Oct 27, 2017

Member

/sig api-machinery

Member

munnerz commented Oct 27, 2017

/sig api-machinery

@cjwagner

This comment has been minimized.

Show comment
Hide comment
@cjwagner

cjwagner Oct 27, 2017

Member

/retest

Member

cjwagner commented Oct 27, 2017

/retest

@munnerz

This comment has been minimized.

Show comment
Hide comment
@munnerz

munnerz Oct 27, 2017

Member

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

Member

munnerz commented Oct 27, 2017

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

@chancez

This comment has been minimized.

Show comment
Hide comment
@chancez

chancez Oct 27, 2017

Member

@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.

Member

chancez commented Oct 27, 2017

@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.

@munnerz

This comment has been minimized.

Show comment
Hide comment
@munnerz

munnerz Oct 27, 2017

Member

@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)

Member

munnerz commented Oct 27, 2017

@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)

@chancez

This comment has been minimized.

Show comment
Hide comment
@chancez

chancez Oct 27, 2017

Member

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.

Member

chancez commented Oct 27, 2017

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.

@munnerz

This comment has been minimized.

Show comment
Hide comment
@munnerz

munnerz Oct 27, 2017

Member

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

Member

munnerz commented Oct 27, 2017

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

@chancez

This comment has been minimized.

Show comment
Hide comment
@chancez

chancez Oct 27, 2017

Member

@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.

Member

chancez commented Oct 27, 2017

@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.

@munnerz

This comment has been minimized.

Show comment
Hide comment
@munnerz

munnerz Oct 27, 2017

Member

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?

Member

munnerz commented Oct 27, 2017

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?

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Oct 27, 2017

Contributor

/cc @ncdc

Contributor

sttts commented Oct 27, 2017

/cc @ncdc

@k8s-ci-robot k8s-ci-robot requested a review from ncdc Oct 27, 2017

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Oct 27, 2017

Contributor

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.

Contributor

sttts commented Oct 27, 2017

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.

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t Oct 27, 2017

Member

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.

Member

wojtek-t commented Oct 27, 2017

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.

@munnerz

This comment has been minimized.

Show comment
Hide comment
@munnerz

munnerz Oct 27, 2017

Member

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.

Member

munnerz commented Oct 27, 2017

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.

@ncdc

This comment has been minimized.

Show comment
Hide comment
@ncdc

ncdc Oct 27, 2017

Member

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).

Member

ncdc commented Oct 27, 2017

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).

@chancez

This comment has been minimized.

Show comment
Hide comment
@chancez

chancez Oct 27, 2017

Member

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.

Member

chancez commented Oct 27, 2017

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.

@munnerz munnerz changed the title from WIP: add namespace option to SharedInformerFactory to code-generator: add NewFilteredSharedInformerFactory function Oct 31, 2017

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Nov 8, 2017

Contributor

As discussed on Slack with @munnerz the inconsistency is actually in the opposite direction, i.e. the listers have the same filters applied as the informer. IMO this is not inconsistent at all, but the expected behaviour. So this is fine with me.

Contributor

sttts commented Nov 8, 2017

As discussed on Slack with @munnerz the inconsistency is actually in the opposite direction, i.e. the listers have the same filters applied as the informer. IMO this is not inconsistent at all, but the expected behaviour. So this is fine with me.

}
// NewFilteredSharedInformerFactory constructs a new instance of sharedInformerFactory.
// Listers obtained via this SharedInformerFactory will be subject to the same filters

This comment has been minimized.

@sttts

sttts Nov 8, 2017

Contributor

👍

@sttts

sttts Nov 8, 2017

Contributor

👍

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Nov 8, 2017

Contributor

lgtm

Contributor

sttts commented Nov 8, 2017

lgtm

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Nov 8, 2017

Contributor

/lgtm

Contributor

sttts commented Nov 8, 2017

/lgtm

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Nov 8, 2017

Contributor

/approve no-issue

Contributor

sttts commented Nov 8, 2017

/approve no-issue

@munnerz

This comment has been minimized.

Show comment
Hide comment
@munnerz

munnerz Nov 8, 2017

Member

/retest

Member

munnerz commented Nov 8, 2017

/retest

@munnerz

This comment has been minimized.

Show comment
Hide comment
@munnerz

munnerz Nov 8, 2017

Member

/retest

Member

munnerz commented Nov 8, 2017

/retest

@munnerz

This comment has been minimized.

Show comment
Hide comment
@munnerz

munnerz Nov 8, 2017

Member

@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!

Member

munnerz commented Nov 8, 2017

@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!

munnerz added some commits Oct 26, 2017

code-generator: add NewFilteredSharedInformerFactory function
Refactor to not change New*Informer constructors

Separate namespace and ListOptions filter

@k8s-merge-robot k8s-merge-robot removed the lgtm label Nov 9, 2017

@munnerz

This comment has been minimized.

Show comment
Hide comment
@munnerz

munnerz Nov 9, 2017

Member

@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!

Member

munnerz commented Nov 9, 2017

@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!

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Nov 9, 2017

Contributor

/lgtm

Contributor

sttts commented Nov 9, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 9, 2017

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Nov 9, 2017

Contributor

@smarterclayton are you fine with this?

Contributor

sttts commented Nov 9, 2017

@smarterclayton are you fine with this?

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Nov 9, 2017

Contributor

/approve

Contributor

deads2k commented Nov 9, 2017

/approve

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Nov 9, 2017

Contributor

[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.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

Contributor

k8s-merge-robot commented Nov 9, 2017

[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.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Nov 9, 2017

Contributor

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.

Contributor

k8s-merge-robot commented Nov 9, 2017

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.

@k8s-merge-robot k8s-merge-robot merged commit bab312d into kubernetes:master Nov 9, 2017

12 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation munnerz authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Jenkins job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Jenkins job succeeded.
Details
pull-kubernetes-verify Jenkins job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment