Option to restrict informer cache to a namespace #136

hasbro17 · 2018-09-09T22:56:14Z

Fixes #124

A namespace option can be passed from the manager to the cache which restricts the ListWatch for all informers to the desired namespace.
This way a manager can be run with a Role instead of a ClusterRole.

hasbro17 · 2018-09-09T23:07:35Z

@DirectXMan12 PTAL
First time writing a test in Ginkgo so I'm not sure if the namespaced cache test case should be in it's own Describe block. Seemed more concise to add it as a sub-case to the Reader.

interma · 2018-09-10T07:09:57Z

pkg/manager/manager.go

@@ -128,7 +132,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
 	}

 	// Create the cache for the cached read client and registering informers
-	cache, err := options.newCache(config, cache.Options{Scheme: options.Scheme, Mapper: mapper, Resync: options.SyncPeriod})
+	cache, err := options.newCache(config, cache.Options{Scheme: options.Scheme, Mapper: mapper, Resync: options.SyncPeriod, Namespace: options.Namespace})


When I need to watch all namespace events, set nil to options.Namespace or set other value?
Thanks.

To watch all namespaces you set options.Namespaces = "" or metav1.NamespaceAll which is the same thing.
But since the default string value is empty you can just not set the Namespace option in cache.Options and it should watch all namespaces by default.

DirectXMan12 · 2018-09-10T17:23:24Z

pkg/cache/internal/informers_map.go

@@ -217,14 +223,14 @@ func (ip *InformersMap) newListWatch(gvk schema.GroupVersionKind) (*cache.ListWa
 	return &cache.ListWatch{
 		ListFunc: func(opts metav1.ListOptions) (runtime.Object, error) {
 			res := listObj.DeepCopyObject()
-			err := client.Get().Resource(mapping.Resource.Resource).VersionedParams(&opts, ip.paramCodec).Do().Into(res)
+			err := client.Get().Namespace(ip.namespace).Resource(mapping.Resource.Resource).VersionedParams(&opts, ip.paramCodec).Do().Into(res)


does this need to be NamespaceIfScoped(ip.namespace, ip.namespace == "")?

You could do without NamespaceIfScoped since the URL constructed ignores namespaces if namespace == "".
https://github.com/kubernetes/client-go/blob/master/rest/request.go#L424
But that behavior is implicit so I've added NamespaceIfScoped(ip.namespace, ip.namespace != "") to make the distinction more clear.

hasbro17 · 2018-09-13T20:40:27Z

@DirectXMan12 The request is explicitly namespace scoped now. PTAL.

hasbro17 · 2018-09-18T18:47:33Z

@droot can you please review this in case @DirectXMan12 is busy.

droot · 2018-09-20T17:28:13Z

@hasbro17 change looks good to me. @DirectXMan12 , would you like to take a final look ?

hasbro17 · 2018-09-20T22:20:32Z

@droot thanks. I think @DirectXMan12 should be back on Monday to take a look.
I'll rebase my changes in the meanwhile since #101 was merged.

hasbro17 · 2018-09-21T03:18:31Z

Rebased and added another namespaced cache test for an unstructured object.

droot · 2018-09-21T16:58:59Z

pkg/cache/cache_test.go

+				Expect(out.Items).Should(HaveLen(1))
+				Expect(out.Items[0].GetNamespace()).To(Equal(testNamespaceOne))
+			})
+


+1 for covering this.

droot · 2018-09-21T17:09:29Z

pkg/manager/manager.go

@@ -106,6 +106,10 @@ type Options struct {
 	// will use for holding the leader lock.
 	LeaderElectionID string

+	// Namespace if specified restricts the manager's cache to watch the desired namespace
+	// Defaults to all namespaces


May be make it more explicit:

watch objects in the desired namespace

Also one more q: if I specify the namespace here and later setup a watch for a cluster scoped resource (say Node), will I get error, what would be the UX ?
We should document that behavior here probably.

I tested it out by specifying the manager's namespace and added a controller to watch nodes:

$ ./main --namespace=default {"level":"info","ts":1537598131.6718707,"logger":"kubebuilder.controller","caller":"controller/controller.go:120","msg":"Starting EventSource","Controller":"node-controller","Source":{"Type":{"metadata":{"creationTimestamp":null},"spec":{},"status":{"daemonEndpoints":{"kubeletEndpoint":{"Port":0}},"nodeInfo":{"machineID":"","systemUUID":"","bootID":"","kernelVersion":"","osImage":"","containerRuntimeVersion":"","kubeletVersion":"","kubeProxyVersion":"","operatingSystem":"","architecture":""}}}}} E0921 23:35:31.674413 15574 reflector.go:205] sigs.k8s.io/controller-runtime/pkg/cache/internal/informers_map.go:126: Failed to list *v1.Node: the server could not find the requested resource E0921 23:35:32.680154 15574 reflector.go:205] sigs.k8s.io/controller-runtime/pkg/cache/internal/informers_map.go:126: Failed to list *v1.Node: the server could not find the requested resource

As expected the ListWatcher fails to List the nodes because when the namespace is set it will form an incorrect request to list Nodes by adding the namespace in the REST path.
https://github.com/kubernetes-sigs/controller-runtime/pull/136/files#diff-fc36bd1640f2ed10bc9e4f4d24789edfR235

The behavior seems consistent with the need for namespacing the manager i.e you don't expect to Watch cluster-scoped resources if your permissions are restricted to a namespace.

I've updated the comment to document this behavior.

DirectXMan12

realized that we also should check if the resource is namespaced in the first place.

DirectXMan12 · 2018-09-24T18:19:02Z

pkg/cache/internal/informers_map.go

@@ -225,14 +232,14 @@ func createStructuredListWatch(gvk schema.GroupVersionKind, ip *specificInformer
 	return &cache.ListWatch{
 		ListFunc: func(opts metav1.ListOptions) (runtime.Object, error) {
 			res := listObj.DeepCopyObject()
-			err := client.Get().Resource(mapping.Resource.Resource).VersionedParams(&opts, ip.paramCodec).Do().Into(res)
+			err := client.Get().NamespaceIfScoped(ip.namespace, ip.namespace != "").Resource(mapping.Resource.Resource).VersionedParams(&opts, ip.paramCodec).Do().Into(res)


it occurs to me that there's one last wrinkle for this -- we need to also check if the resource is namespace-scoped to begin with using the rest mapping -- i.e. you should be able to watch nodes when watching pods in a specific namespace.

so this should be NamespaceIfScoped(ip.namespace, ip.namespace != "" && mapping.Scope == apimeta.RESTScopeNameRoot)

and below (in the watch)

Fixed. Minor nit being it's:

ip.namespace != "" && mapping.Scope != apimeta.RESTScopeNameRoot

DirectXMan12 · 2018-09-24T18:32:05Z

pkg/cache/internal/informers_map.go

@@ -252,12 +259,18 @@ func createUnstructuredListWatch(gvk schema.GroupVersionKind, ip *specificInform
 	// Create a new ListWatch for the obj
 	return &cache.ListWatch{
 		ListFunc: func(opts metav1.ListOptions) (runtime.Object, error) {
+			if ip.namespace != "" {


ditto here on the "is this namespaced"

A namespace option can be passed from the manager to the cache which restricts the ListWatch for all informers to the desired namespace. This way a manager can be run with a Role instead of a ClusterRole.

hasbro17 · 2018-09-25T17:31:26Z

@DirectXMan12 I've updated the ListWatchers to check if the resource is namespace scoped before making a namespaced request.
With this change ,even if the cache is namespaced, it can still be used to read cluster-scoped resources.

I've updated the tests with the additional case of ensuring a namespaced cache will still let you List a cluster-scoped resource(corev1.Namespace).
Also updated the manager's Namespace option comment to clarify this.

DirectXMan12 · 2018-09-25T18:04:32Z

/lgtm
/approve

k8s-ci-robot · 2018-09-25T18:04:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, hasbro17

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…rs-namespaced Option to restrict informer cache to a namespace

Use "dep" to import dependencies for generated project

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 9, 2018

k8s-ci-robot requested review from droot and pwittrock September 9, 2018 22:56

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 9, 2018

interma reviewed Sep 10, 2018

View reviewed changes

DirectXMan12 reviewed Sep 10, 2018

View reviewed changes

hasbro17 force-pushed the make-list-watchers-namespaced branch from cb979d2 to 9ddd976 Compare September 11, 2018 21:34

shawn-hurley mentioned this pull request Sep 19, 2018

Gopkg.*,pkg/generator/templates.go: update to kubernetes 1.11.2 operator-framework/operator-sdk#391

Closed

droot approved these changes Sep 20, 2018

View reviewed changes

hasbro17 force-pushed the make-list-watchers-namespaced branch from 9ddd976 to b976451 Compare September 21, 2018 03:16

droot reviewed Sep 21, 2018

View reviewed changes

hasbro17 force-pushed the make-list-watchers-namespaced branch from b976451 to 495a3d8 Compare September 22, 2018 06:58

DirectXMan12 suggested changes Sep 24, 2018

View reviewed changes

hasbro17 force-pushed the make-list-watchers-namespaced branch from 495a3d8 to 6e82d0f Compare September 25, 2018 17:18

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 25, 2018

Option to restrict informer cache to a namespace

ca13e86

A namespace option can be passed from the manager to the cache which restricts the ListWatch for all informers to the desired namespace. This way a manager can be run with a Role instead of a ClusterRole.

hasbro17 force-pushed the make-list-watchers-namespaced branch from 6e82d0f to ca13e86 Compare September 25, 2018 17:26

DirectXMan12 approved these changes Sep 25, 2018

View reviewed changes

k8s-ci-robot assigned DirectXMan12 Sep 25, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 25, 2018

k8s-ci-robot merged commit 53fc44b into kubernetes-sigs:master Sep 25, 2018

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#136 from hasbro17/make-list-watche…

b8b3c36

…rs-namespaced Option to restrict informer cache to a namespace

interma mentioned this pull request Dec 12, 2018

REQUEST: New membership for interma kubernetes/org#301

Closed

6 tasks

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #136 from droot/use-dep

ddb0309

Use "dep" to import dependencies for generated project

sjenning mentioned this pull request Apr 24, 2020

add static resource controller openshift/cloud-credential-operator#185

Closed

YangKeao mentioned this pull request Nov 24, 2020

Connect daemon through endpoint chaos-mesh/chaos-mesh#1206

Merged

This was referenced Apr 5, 2023

Prometheus plugin influxdata/telegraf#10928

Closed

inputs.prometheus require cluster level permissions even when scoped to a namespace influxdata/telegraf#12780

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to restrict informer cache to a namespace #136

Option to restrict informer cache to a namespace #136

hasbro17 commented Sep 9, 2018

hasbro17 commented Sep 9, 2018

interma Sep 10, 2018

hasbro17 Sep 11, 2018

DirectXMan12 Sep 10, 2018

hasbro17 Sep 11, 2018

hasbro17 commented Sep 13, 2018

hasbro17 commented Sep 18, 2018

droot commented Sep 20, 2018

hasbro17 commented Sep 20, 2018

hasbro17 commented Sep 21, 2018

droot Sep 21, 2018

droot Sep 21, 2018

hasbro17 Sep 22, 2018

DirectXMan12 left a comment

DirectXMan12 Sep 24, 2018

DirectXMan12 Sep 24, 2018

DirectXMan12 Sep 24, 2018

hasbro17 Sep 25, 2018

DirectXMan12 Sep 24, 2018

hasbro17 commented Sep 25, 2018

DirectXMan12 commented Sep 25, 2018

k8s-ci-robot commented Sep 25, 2018

Option to restrict informer cache to a namespace #136

Option to restrict informer cache to a namespace #136

Conversation

hasbro17 commented Sep 9, 2018

hasbro17 commented Sep 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasbro17 commented Sep 13, 2018

hasbro17 commented Sep 18, 2018

droot commented Sep 20, 2018

hasbro17 commented Sep 20, 2018

hasbro17 commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasbro17 commented Sep 25, 2018

DirectXMan12 commented Sep 25, 2018

k8s-ci-robot commented Sep 25, 2018