From 2dbca50af35b8fb38bc7960d4d2434a0468bbe29 Mon Sep 17 00:00:00 2001 From: Soumya Ghosh Dastidar Date: Tue, 2 May 2023 09:34:59 +0530 Subject: [PATCH 1/5] feat: respect rbac for resource exclusions Signed-off-by: Soumya Ghosh Dastidar --- pkg/cache/cluster.go | 75 ++++++++++++++++++++++++++++++++++++++----- pkg/cache/settings.go | 7 ++++ 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/pkg/cache/cluster.go b/pkg/cache/cluster.go index d6e888f22..7ec482d59 100644 --- a/pkg/cache/cluster.go +++ b/pkg/cache/cluster.go @@ -11,6 +11,7 @@ import ( "github.com/go-logr/logr" "golang.org/x/sync/semaphore" + authorizationv1 "k8s.io/api/authorization/v1" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -22,6 +23,8 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/dynamic" + "k8s.io/client-go/kubernetes" + authType1 "k8s.io/client-go/kubernetes/typed/authorization/v1" "k8s.io/client-go/rest" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/pager" @@ -208,6 +211,8 @@ type clusterCache struct { eventHandlers map[uint64]OnEventHandler openAPISchema openapi.Resources gvkParser *managedfields.GvkParser + + respectRBAC bool } type clusterCacheSync struct { @@ -462,6 +467,10 @@ func (c *clusterCache) startMissingWatches() error { if err != nil { return err } + clientset, err := kubernetes.NewForConfig(c.config) + if err != nil { + return err + } namespacedResources := make(map[schema.GroupKind]bool) for i := range apis { api := apis[i] @@ -470,10 +479,14 @@ func (c *clusterCache) startMissingWatches() error { ctx, cancel := context.WithCancel(context.Background()) c.apisMeta[api.GroupKind] = &apiMeta{namespaced: api.Meta.Namespaced, watchCancel: cancel} - err = c.processApi(client, api, func(resClient dynamic.ResourceInterface, ns string) error { + keepResource, err := c.processApi(ctx, clientset.AuthorizationV1().SelfSubjectAccessReviews(), client, api, func(resClient dynamic.ResourceInterface, ns string) error { go c.watchEvents(ctx, api, resClient, ns, "") return nil }) + if !keepResource { + delete(c.apisMeta, api.GroupKind) + delete(namespacedResources, api.GroupKind) + } if err != nil { return err } @@ -683,23 +696,58 @@ func (c *clusterCache) watchEvents(ctx context.Context, api kube.APIResourceInfo }) } -func (c *clusterCache) processApi(client dynamic.Interface, api kube.APIResourceInfo, callback func(resClient dynamic.ResourceInterface, ns string) error) error { +// processApi checks if resource can be accessed using current rbac (when respectRBAC is enabled) if no returns false else tries to execute the callback +func (c *clusterCache) processApi(ctx context.Context, reviewInterface authType1.SelfSubjectAccessReviewInterface, client dynamic.Interface, api kube.APIResourceInfo, callback func(resClient dynamic.ResourceInterface, ns string) error) (keep bool, err error) { resClient := client.Resource(api.GroupVersionResource) + sar := &authorizationv1.SelfSubjectAccessReview{ + Spec: authorizationv1.SelfSubjectAccessReviewSpec{ + ResourceAttributes: &authorizationv1.ResourceAttributes{ + Namespace: "*", + Verb: "list", + Resource: api.GroupVersionResource.Resource, + }, + }, + } + var ( + resp *authorizationv1.SelfSubjectAccessReview + ) switch { // if manage whole cluster or resource is cluster level and cluster resources enabled case len(c.namespaces) == 0 || !api.Meta.Namespaced && c.clusterResources: - return callback(resClient, "") + if c.respectRBAC { + resp, err = reviewInterface.Create(ctx, sar, metav1.CreateOptions{}) + if err != nil { + return true, err + } + } + if !c.respectRBAC || (resp != nil && resp.Status.Allowed) { + return true, callback(resClient, "") + } + // unsupported, remove from watch list + return false, nil // if manage some namespaces and resource is namespaced case len(c.namespaces) != 0 && api.Meta.Namespaced: for _, ns := range c.namespaces { - err := callback(resClient.Namespace(ns), ns) - if err != nil { - return err + if c.respectRBAC { + sar.Spec.ResourceAttributes.Namespace = ns + resp, err = reviewInterface.Create(ctx, sar, metav1.CreateOptions{}) + if err != nil { + return true, err + } + } + if !c.respectRBAC || (resp != nil && resp.Status.Allowed) { + err := callback(resClient, "") + if err != nil { + return true, err + } + } else { + // unsupported, remove from watch list + return false, nil } } } - return nil + return true, nil } func (c *clusterCache) sync() error { @@ -748,6 +796,10 @@ func (c *clusterCache) sync() error { if err != nil { return err } + clientset, err := kubernetes.NewForConfig(config) + if err != nil { + return err + } lock := sync.Mutex{} err = kube.RunAllAsync(len(apis), func(i int) error { api := apis[i] @@ -759,7 +811,7 @@ func (c *clusterCache) sync() error { c.namespacedResources[api.GroupKind] = api.Meta.Namespaced lock.Unlock() - return c.processApi(client, api, func(resClient dynamic.ResourceInterface, ns string) error { + keepResource, err := c.processApi(ctx, clientset.AuthorizationV1().SelfSubjectAccessReviews(), client, api, func(resClient dynamic.ResourceInterface, ns string) error { resourceVersion, err := c.listResources(ctx, resClient, func(listPager *pager.ListPager) error { return listPager.EachListItem(context.Background(), metav1.ListOptions{}, func(obj runtime.Object) error { if un, ok := obj.(*unstructured.Unstructured); !ok { @@ -780,6 +832,13 @@ func (c *clusterCache) sync() error { return nil }) + if !keepResource { + lock.Lock() + delete(c.apisMeta, api.GroupKind) + delete(c.namespacedResources, api.GroupKind) + lock.Unlock() + } + return err }) if err != nil { diff --git a/pkg/cache/settings.go b/pkg/cache/settings.go index 1c87cdee1..0d3d69c29 100644 --- a/pkg/cache/settings.go +++ b/pkg/cache/settings.go @@ -158,3 +158,10 @@ func SetRetryOptions(maxRetries int32, useBackoff bool, retryFunc ListRetryFunc) cache.listRetryFunc = retryFunc } } + +// SetRespectRBAC allows to set whether to respect the controller rbac in list/watches +func SetRespectRBAC(respectRBA bool) UpdateSettingsFunc { + return func(cache *clusterCache) { + cache.respectRBAC = respectRBA + } +} From 60236d63045a0a7cf1f5253055a53658a368d0d8 Mon Sep 17 00:00:00 2001 From: Soumya Ghosh Dastidar Date: Wed, 10 May 2023 19:28:37 +0530 Subject: [PATCH 2/5] feat: use list call to check for permissions Signed-off-by: Soumya Ghosh Dastidar --- pkg/cache/cluster.go | 136 +++++++++++++++---------------------------- 1 file changed, 48 insertions(+), 88 deletions(-) diff --git a/pkg/cache/cluster.go b/pkg/cache/cluster.go index 7ec482d59..6207cac94 100644 --- a/pkg/cache/cluster.go +++ b/pkg/cache/cluster.go @@ -11,9 +11,9 @@ import ( "github.com/go-logr/logr" "golang.org/x/sync/semaphore" - authorizationv1 "k8s.io/api/authorization/v1" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/api/errors" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -23,8 +23,6 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/dynamic" - "k8s.io/client-go/kubernetes" - authType1 "k8s.io/client-go/kubernetes/typed/authorization/v1" "k8s.io/client-go/rest" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/pager" @@ -467,10 +465,6 @@ func (c *clusterCache) startMissingWatches() error { if err != nil { return err } - clientset, err := kubernetes.NewForConfig(c.config) - if err != nil { - return err - } namespacedResources := make(map[schema.GroupKind]bool) for i := range apis { api := apis[i] @@ -479,14 +473,16 @@ func (c *clusterCache) startMissingWatches() error { ctx, cancel := context.WithCancel(context.Background()) c.apisMeta[api.GroupKind] = &apiMeta{namespaced: api.Meta.Namespaced, watchCancel: cancel} - keepResource, err := c.processApi(ctx, clientset.AuthorizationV1().SelfSubjectAccessReviews(), client, api, func(resClient dynamic.ResourceInterface, ns string) error { - go c.watchEvents(ctx, api, resClient, ns, "") + err := c.processApi(client, api, func(resClient dynamic.ResourceInterface, ns string) error { + resourceVersion, err := c.loadInitialState(ctx, api, resClient, ns) + if err != nil && c.respectRBAC && (k8sErrors.IsForbidden(err) || k8sErrors.IsUnauthorized(err)) { + delete(c.apisMeta, api.GroupKind) + delete(namespacedResources, api.GroupKind) + return nil + } + go c.watchEvents(ctx, api, resClient, ns, resourceVersion) return nil }) - if !keepResource { - delete(c.apisMeta, api.GroupKind) - delete(namespacedResources, api.GroupKind) - } if err != nil { return err } @@ -543,6 +539,29 @@ func (c *clusterCache) listResources(ctx context.Context, resClient dynamic.Reso return resourceVersion, callback(listPager) } +func (c *clusterCache) loadInitialState(ctx context.Context, api kube.APIResourceInfo, resClient dynamic.ResourceInterface, ns string) (string, error) { + return c.listResources(ctx, resClient, func(listPager *pager.ListPager) error { + var items []*Resource + err := listPager.EachListItem(ctx, metav1.ListOptions{}, func(obj runtime.Object) error { + if un, ok := obj.(*unstructured.Unstructured); !ok { + return fmt.Errorf("object %s/%s has an unexpected type", un.GroupVersionKind().String(), un.GetName()) + } else { + items = append(items, c.newResource(un)) + } + return nil + }) + + if err != nil { + return fmt.Errorf("failed to load initial state of resource %s: %w", api.GroupKind.String(), err) + } + + return runSynced(&c.lock, func() error { + c.replaceResourceCache(api.GroupKind, items, ns) + return nil + }) + }) +} + func (c *clusterCache) watchEvents(ctx context.Context, api kube.APIResourceInfo, resClient dynamic.ResourceInterface, ns string, resourceVersion string) { kube.RetryUntilSucceed(ctx, watchResourcesRetryTimeout, fmt.Sprintf("watch %s on %s", api.GroupKind, c.config.Host), c.log, func() (err error) { defer func() { @@ -553,27 +572,7 @@ func (c *clusterCache) watchEvents(ctx context.Context, api kube.APIResourceInfo // load API initial state if no resource version provided if resourceVersion == "" { - resourceVersion, err = c.listResources(ctx, resClient, func(listPager *pager.ListPager) error { - var items []*Resource - err := listPager.EachListItem(ctx, metav1.ListOptions{}, func(obj runtime.Object) error { - if un, ok := obj.(*unstructured.Unstructured); !ok { - return fmt.Errorf("object %s/%s has an unexpected type", un.GroupVersionKind().String(), un.GetName()) - } else { - items = append(items, c.newResource(un)) - } - return nil - }) - - if err != nil { - return fmt.Errorf("failed to load initial state of resource %s: %v", api.GroupKind.String(), err) - } - - return runSynced(&c.lock, func() error { - c.replaceResourceCache(api.GroupKind, items, ns) - return nil - }) - }) - + resourceVersion, err = c.loadInitialState(ctx, api, resClient, ns) if err != nil { return err } @@ -696,58 +695,23 @@ func (c *clusterCache) watchEvents(ctx context.Context, api kube.APIResourceInfo }) } -// processApi checks if resource can be accessed using current rbac (when respectRBAC is enabled) if no returns false else tries to execute the callback -func (c *clusterCache) processApi(ctx context.Context, reviewInterface authType1.SelfSubjectAccessReviewInterface, client dynamic.Interface, api kube.APIResourceInfo, callback func(resClient dynamic.ResourceInterface, ns string) error) (keep bool, err error) { +func (c *clusterCache) processApi(client dynamic.Interface, api kube.APIResourceInfo, callback func(resClient dynamic.ResourceInterface, ns string) error) error { resClient := client.Resource(api.GroupVersionResource) - sar := &authorizationv1.SelfSubjectAccessReview{ - Spec: authorizationv1.SelfSubjectAccessReviewSpec{ - ResourceAttributes: &authorizationv1.ResourceAttributes{ - Namespace: "*", - Verb: "list", - Resource: api.GroupVersionResource.Resource, - }, - }, - } - var ( - resp *authorizationv1.SelfSubjectAccessReview - ) switch { // if manage whole cluster or resource is cluster level and cluster resources enabled case len(c.namespaces) == 0 || !api.Meta.Namespaced && c.clusterResources: - if c.respectRBAC { - resp, err = reviewInterface.Create(ctx, sar, metav1.CreateOptions{}) - if err != nil { - return true, err - } - } - if !c.respectRBAC || (resp != nil && resp.Status.Allowed) { - return true, callback(resClient, "") - } - // unsupported, remove from watch list - return false, nil + return callback(resClient, "") // if manage some namespaces and resource is namespaced case len(c.namespaces) != 0 && api.Meta.Namespaced: for _, ns := range c.namespaces { - if c.respectRBAC { - sar.Spec.ResourceAttributes.Namespace = ns - resp, err = reviewInterface.Create(ctx, sar, metav1.CreateOptions{}) - if err != nil { - return true, err - } - } - if !c.respectRBAC || (resp != nil && resp.Status.Allowed) { - err := callback(resClient, "") - if err != nil { - return true, err - } - } else { - // unsupported, remove from watch list - return false, nil + err := callback(resClient.Namespace(ns), ns) + if err != nil { + return err } } } - return true, nil + return nil } func (c *clusterCache) sync() error { @@ -796,10 +760,6 @@ func (c *clusterCache) sync() error { if err != nil { return err } - clientset, err := kubernetes.NewForConfig(config) - if err != nil { - return err - } lock := sync.Mutex{} err = kube.RunAllAsync(len(apis), func(i int) error { api := apis[i] @@ -811,7 +771,7 @@ func (c *clusterCache) sync() error { c.namespacedResources[api.GroupKind] = api.Meta.Namespaced lock.Unlock() - keepResource, err := c.processApi(ctx, clientset.AuthorizationV1().SelfSubjectAccessReviews(), client, api, func(resClient dynamic.ResourceInterface, ns string) error { + return c.processApi(client, api, func(resClient dynamic.ResourceInterface, ns string) error { resourceVersion, err := c.listResources(ctx, resClient, func(listPager *pager.ListPager) error { return listPager.EachListItem(context.Background(), metav1.ListOptions{}, func(obj runtime.Object) error { if un, ok := obj.(*unstructured.Unstructured); !ok { @@ -825,20 +785,20 @@ func (c *clusterCache) sync() error { }) }) if err != nil { - return fmt.Errorf("failed to load initial state of resource %s: %v", api.GroupKind.String(), err) + if c.respectRBAC && (k8sErrors.IsForbidden(err) || k8sErrors.IsUnauthorized(err)) { + lock.Lock() + delete(c.apisMeta, api.GroupKind) + delete(c.namespacedResources, api.GroupKind) + lock.Unlock() + return nil + } + return fmt.Errorf("failed to load initial state of resource %s: %w", api.GroupKind.String(), err) } go c.watchEvents(ctx, api, resClient, ns, resourceVersion) return nil }) - if !keepResource { - lock.Lock() - delete(c.apisMeta, api.GroupKind) - delete(c.namespacedResources, api.GroupKind) - lock.Unlock() - } - return err }) if err != nil { From 56c89ca1b7cd2f8c02ed51e439c4f22e1bf9f38e Mon Sep 17 00:00:00 2001 From: Soumya Ghosh Dastidar Date: Thu, 6 Jul 2023 19:13:28 +0530 Subject: [PATCH 3/5] feat: updated implementation to handle different levels of rbac check Signed-off-by: Soumya Ghosh Dastidar --- pkg/cache/cluster.go | 111 +++++++++++++++++++++++++++++++++++++----- pkg/cache/settings.go | 9 +++- 2 files changed, 107 insertions(+), 13 deletions(-) diff --git a/pkg/cache/cluster.go b/pkg/cache/cluster.go index 42ae330b8..15e32410e 100644 --- a/pkg/cache/cluster.go +++ b/pkg/cache/cluster.go @@ -11,6 +11,7 @@ import ( "github.com/go-logr/logr" "golang.org/x/sync/semaphore" + authorizationv1 "k8s.io/api/authorization/v1" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/api/errors" k8sErrors "k8s.io/apimachinery/pkg/api/errors" @@ -23,6 +24,8 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/dynamic" + "k8s.io/client-go/kubernetes" + authType1 "k8s.io/client-go/kubernetes/typed/authorization/v1" "k8s.io/client-go/rest" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/pager" @@ -56,6 +59,15 @@ const ( defaultListSemaphoreWeight = 50 ) +const ( + // RespectRbacDisabled default value for respectRbac + RespectRbacDisabled = iota + // RespectRbacNormal checks only api response for forbidden/unauthorized errors + RespectRbacNormal + // RespectRbacStrict checks both api response for forbidden/unauthorized errors and SelfSubjectAccessReview + RespectRbacStrict +) + type apiMeta struct { namespaced bool watchCancel context.CancelFunc @@ -210,7 +222,7 @@ type clusterCache struct { openAPISchema openapi.Resources gvkParser *managedfields.GvkParser - respectRBAC bool + respectRBAC int } type clusterCacheSync struct { @@ -465,6 +477,10 @@ func (c *clusterCache) startMissingWatches() error { if err != nil { return err } + clientset, err := kubernetes.NewForConfig(c.config) + if err != nil { + return err + } namespacedResources := make(map[schema.GroupKind]bool) for i := range apis { api := apis[i] @@ -475,10 +491,21 @@ func (c *clusterCache) startMissingWatches() error { err := c.processApi(client, api, func(resClient dynamic.ResourceInterface, ns string) error { resourceVersion, err := c.loadInitialState(ctx, api, resClient, ns) - if err != nil && c.respectRBAC && (k8sErrors.IsForbidden(err) || k8sErrors.IsUnauthorized(err)) { - delete(c.apisMeta, api.GroupKind) - delete(namespacedResources, api.GroupKind) - return nil + if err != nil && c.respectRBAC != RespectRbacDisabled && (k8sErrors.IsForbidden(err) || k8sErrors.IsUnauthorized(err)) { + keep := false + if c.respectRBAC == RespectRbacStrict { + k, permErr := c.checkPermission(ctx, clientset.AuthorizationV1().SelfSubjectAccessReviews(), api) + if permErr != nil { + return fmt.Errorf("failed to check permissions for resource %s: %w, original error=%v", api.GroupKind.String(), permErr, err.Error()) + } + keep = k + } + // if we are not allowed to list the resource, remove it from the watch list + if !keep { + delete(c.apisMeta, api.GroupKind) + delete(namespacedResources, api.GroupKind) + return nil + } } go c.watchEvents(ctx, api, resClient, ns, resourceVersion) return nil @@ -573,6 +600,9 @@ func (c *clusterCache) watchEvents(ctx context.Context, api kube.APIResourceInfo // load API initial state if no resource version provided if resourceVersion == "" { resourceVersion, err = c.loadInitialState(ctx, api, resClient, ns) + if err != nil { + return err + } } w, err := watchutil.NewRetryWatcher(resourceVersion, &cache.ListWatch{ @@ -711,6 +741,50 @@ func (c *clusterCache) processApi(client dynamic.Interface, api kube.APIResource return nil } +// checkPermission runs a self subject access review to check if the controller has permissions to list the resource +func (c *clusterCache) checkPermission(ctx context.Context, reviewInterface authType1.SelfSubjectAccessReviewInterface, api kube.APIResourceInfo) (keep bool, err error) { + sar := &authorizationv1.SelfSubjectAccessReview{ + Spec: authorizationv1.SelfSubjectAccessReviewSpec{ + ResourceAttributes: &authorizationv1.ResourceAttributes{ + Namespace: "*", + Verb: "list", // uses list verb to check for permissions + Resource: api.GroupVersionResource.Resource, + }, + }, + } + + switch { + // if manage whole cluster or resource is cluster level and cluster resources enabled + case len(c.namespaces) == 0 || !api.Meta.Namespaced && c.clusterResources: + resp, err := reviewInterface.Create(ctx, sar, metav1.CreateOptions{}) + if err != nil { + return true, err + } + if resp != nil && resp.Status.Allowed { + return true, nil + } + // unsupported, remove from watch list + return false, nil + // if manage some namespaces and resource is namespaced + case len(c.namespaces) != 0 && api.Meta.Namespaced: + for _, ns := range c.namespaces { + sar.Spec.ResourceAttributes.Namespace = ns + resp, err := reviewInterface.Create(ctx, sar, metav1.CreateOptions{}) + if err != nil { + return true, err + } + if resp != nil && resp.Status.Allowed { + return true, nil + } + // unsupported, remove from watch list + return false, nil + } + } + // checkPermission follows the same logic of determining namespace/cluster resource as the processApi function + // so if neither of the cases match it means the controller will not watch for it so it is safe to return true. + return true, nil +} + func (c *clusterCache) sync() error { c.log.Info("Start syncing cluster") @@ -757,6 +831,10 @@ func (c *clusterCache) sync() error { if err != nil { return err } + clientset, err := kubernetes.NewForConfig(config) + if err != nil { + return err + } lock := sync.Mutex{} err = kube.RunAllAsync(len(apis), func(i int) error { api := apis[i] @@ -782,12 +860,23 @@ func (c *clusterCache) sync() error { }) }) if err != nil { - if c.respectRBAC && (k8sErrors.IsForbidden(err) || k8sErrors.IsUnauthorized(err)) { - lock.Lock() - delete(c.apisMeta, api.GroupKind) - delete(c.namespacedResources, api.GroupKind) - lock.Unlock() - return nil + if c.respectRBAC != RespectRbacDisabled && (k8sErrors.IsForbidden(err) || k8sErrors.IsUnauthorized(err)) { + keep := false + if c.respectRBAC == RespectRbacStrict { + k, permErr := c.checkPermission(ctx, clientset.AuthorizationV1().SelfSubjectAccessReviews(), api) + if permErr != nil { + return fmt.Errorf("failed to check permissions for resource %s: %w, original error=%v", api.GroupKind.String(), permErr, err.Error()) + } + keep = k + } + // if we are not allowed to list the resource, remove it from the watch list + if !keep { + lock.Lock() + delete(c.apisMeta, api.GroupKind) + delete(c.namespacedResources, api.GroupKind) + lock.Unlock() + return nil + } } return fmt.Errorf("failed to load initial state of resource %s: %w", api.GroupKind.String(), err) } diff --git a/pkg/cache/settings.go b/pkg/cache/settings.go index 0d3d69c29..a7194d0ca 100644 --- a/pkg/cache/settings.go +++ b/pkg/cache/settings.go @@ -160,8 +160,13 @@ func SetRetryOptions(maxRetries int32, useBackoff bool, retryFunc ListRetryFunc) } // SetRespectRBAC allows to set whether to respect the controller rbac in list/watches -func SetRespectRBAC(respectRBA bool) UpdateSettingsFunc { +func SetRespectRBAC(respectRBAC int) UpdateSettingsFunc { return func(cache *clusterCache) { - cache.respectRBAC = respectRBA + // if invalid value is provided disable respect rbac + if respectRBAC < RespectRbacDisabled || respectRBAC > RespectRbacStrict { + cache.respectRBAC = RespectRbacDisabled + } else { + cache.respectRBAC = respectRBAC + } } } From e103a6aea6090aa6e30d9ab8860d36edc8bc3faa Mon Sep 17 00:00:00 2001 From: Soumya Ghosh Dastidar Date: Thu, 6 Jul 2023 23:31:01 +0530 Subject: [PATCH 4/5] feat: fixed linter error Signed-off-by: Soumya Ghosh Dastidar --- pkg/cache/cluster.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/cache/cluster.go b/pkg/cache/cluster.go index 15e32410e..1b820a678 100644 --- a/pkg/cache/cluster.go +++ b/pkg/cache/cluster.go @@ -775,9 +775,10 @@ func (c *clusterCache) checkPermission(ctx context.Context, reviewInterface auth } if resp != nil && resp.Status.Allowed { return true, nil + } else { + // unsupported, remove from watch list + return false, nil } - // unsupported, remove from watch list - return false, nil } } // checkPermission follows the same logic of determining namespace/cluster resource as the processApi function From 9b1f75b56d12a0736dda813682007c8f797779c0 Mon Sep 17 00:00:00 2001 From: Soumya Ghosh Dastidar Date: Mon, 10 Jul 2023 20:56:59 +0530 Subject: [PATCH 5/5] feat: resolve review comments Signed-off-by: Soumya Ghosh Dastidar --- pkg/cache/cluster.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/pkg/cache/cluster.go b/pkg/cache/cluster.go index 1b820a678..7e15f89f8 100644 --- a/pkg/cache/cluster.go +++ b/pkg/cache/cluster.go @@ -491,7 +491,7 @@ func (c *clusterCache) startMissingWatches() error { err := c.processApi(client, api, func(resClient dynamic.ResourceInterface, ns string) error { resourceVersion, err := c.loadInitialState(ctx, api, resClient, ns) - if err != nil && c.respectRBAC != RespectRbacDisabled && (k8sErrors.IsForbidden(err) || k8sErrors.IsUnauthorized(err)) { + if err != nil && c.isRestrictedResource(err) { keep := false if c.respectRBAC == RespectRbacStrict { k, permErr := c.checkPermission(ctx, clientset.AuthorizationV1().SelfSubjectAccessReviews(), api) @@ -726,7 +726,7 @@ func (c *clusterCache) processApi(client dynamic.Interface, api kube.APIResource resClient := client.Resource(api.GroupVersionResource) switch { // if manage whole cluster or resource is cluster level and cluster resources enabled - case len(c.namespaces) == 0 || !api.Meta.Namespaced && c.clusterResources: + case len(c.namespaces) == 0 || (!api.Meta.Namespaced && c.clusterResources): return callback(resClient, "") // if manage some namespaces and resource is namespaced case len(c.namespaces) != 0 && api.Meta.Namespaced: @@ -741,6 +741,11 @@ func (c *clusterCache) processApi(client dynamic.Interface, api kube.APIResource return nil } +// isRestrictedResource checks if the kube api call is unauthorized or forbidden +func (c *clusterCache) isRestrictedResource(err error) bool { + return c.respectRBAC != RespectRbacDisabled && (k8sErrors.IsForbidden(err) || k8sErrors.IsUnauthorized(err)) +} + // checkPermission runs a self subject access review to check if the controller has permissions to list the resource func (c *clusterCache) checkPermission(ctx context.Context, reviewInterface authType1.SelfSubjectAccessReviewInterface, api kube.APIResourceInfo) (keep bool, err error) { sar := &authorizationv1.SelfSubjectAccessReview{ @@ -755,10 +760,10 @@ func (c *clusterCache) checkPermission(ctx context.Context, reviewInterface auth switch { // if manage whole cluster or resource is cluster level and cluster resources enabled - case len(c.namespaces) == 0 || !api.Meta.Namespaced && c.clusterResources: + case len(c.namespaces) == 0 || (!api.Meta.Namespaced && c.clusterResources): resp, err := reviewInterface.Create(ctx, sar, metav1.CreateOptions{}) if err != nil { - return true, err + return false, err } if resp != nil && resp.Status.Allowed { return true, nil @@ -771,7 +776,7 @@ func (c *clusterCache) checkPermission(ctx context.Context, reviewInterface auth sar.Spec.ResourceAttributes.Namespace = ns resp, err := reviewInterface.Create(ctx, sar, metav1.CreateOptions{}) if err != nil { - return true, err + return false, err } if resp != nil && resp.Status.Allowed { return true, nil @@ -861,7 +866,7 @@ func (c *clusterCache) sync() error { }) }) if err != nil { - if c.respectRBAC != RespectRbacDisabled && (k8sErrors.IsForbidden(err) || k8sErrors.IsUnauthorized(err)) { + if c.isRestrictedResource(err) { keep := false if c.respectRBAC == RespectRbacStrict { k, permErr := c.checkPermission(ctx, clientset.AuthorizationV1().SelfSubjectAccessReviews(), api)