From 20cecaee97f34f874a84902bf8dbe2bc359db9f0 Mon Sep 17 00:00:00 2001 From: Mitsuru Kariya Date: Thu, 27 Feb 2025 01:24:38 +0900 Subject: [PATCH] Add Watch to controller roles (#130405) * Add Watch to controller roles Starting from version 1.32, the client feature `WatchListClient` has been set to `true` in `kube-controller-manager`. (commit 06a15c5cf96131faaf44f93f1be228a013ae5c0d) As a result, when the `kube-controller-manager` executes the `List` method, it utilizes `Watch`. However, there are some existing controller roles that include `List` but do not include `Watch`. Therefore, when processes using these controller roles execute the `List` method, `Watch` is executed first, but due to permission errors, it falls back to `List`. This PR adds `Watch` to the controller roles that include `List` but do not include `Watch`. The affected roles are as follows (prefixed with `system:controller:`): - `cronjob-controller` - `endpoint-controller` - `endpointslice-controller` - `endpointslicemirroring-controller` - `horizontal-pod-autoscaler` - `node-controller` - `pod-garbage-collector` - `storage-version-migrator-controller` Signed-off-by: Mitsuru Kariya * Fix Fixture Data I apologize, the Fixture Data modifications were missed. Signed-off-by: Mitsuru Kariya * Add ControllerRoles Test Added a test to check that if a controller role includes `List`, it also includes `Watch`. Signed-off-by: Mitsuru Kariya * Fix typo Co-authored-by: Jordan Liggitt * Add Additional Tests Added tests to check that if NodeRules, ClusterRoles, and NamespaceRoles include `List`, it also include `Watch`. Signed-off-by: Mitsuru Kariya --------- Signed-off-by: Mitsuru Kariya Co-authored-by: Jordan Liggitt --- .../rbac/bootstrappolicy/controller_policy.go | 24 ++++++------ .../bootstrappolicy/controller_policy_test.go | 13 +++++++ .../rbac/bootstrappolicy/policy_test.go | 37 +++++++++++++++++++ .../testdata/controller-roles.yaml | 11 ++++++ 4 files changed, 73 insertions(+), 12 deletions(-) diff --git a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go index f23a3751291..1f7858b3011 100644 --- a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go +++ b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go @@ -95,7 +95,7 @@ func buildControllerRoles() ([]rbacv1.ClusterRole, []rbacv1.ClusterRoleBinding) rbacv1helpers.NewRule("get", "list", "watch", "create", "update", "delete", "patch").Groups(batchGroup).Resources("jobs").RuleOrDie(), rbacv1helpers.NewRule("update").Groups(batchGroup).Resources("cronjobs/status").RuleOrDie(), rbacv1helpers.NewRule("update").Groups(batchGroup).Resources("cronjobs/finalizers").RuleOrDie(), - rbacv1helpers.NewRule("list", "delete").Groups(legacyGroup).Resources("pods").RuleOrDie(), + rbacv1helpers.NewRule("list", "watch", "delete").Groups(legacyGroup).Resources("pods").RuleOrDie(), eventsRule(), }, }) @@ -146,7 +146,7 @@ func buildControllerRoles() ([]rbacv1.ClusterRole, []rbacv1.ClusterRoleBinding) ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "endpoint-controller"}, Rules: []rbacv1.PolicyRule{ rbacv1helpers.NewRule("get", "list", "watch").Groups(legacyGroup).Resources("services", "pods").RuleOrDie(), - rbacv1helpers.NewRule("get", "list", "create", "update", "delete").Groups(legacyGroup).Resources("endpoints").RuleOrDie(), + rbacv1helpers.NewRule("get", "list", "watch", "create", "update", "delete").Groups(legacyGroup).Resources("endpoints").RuleOrDie(), rbacv1helpers.NewRule("create").Groups(legacyGroup).Resources("endpoints/restricted").RuleOrDie(), eventsRule(), }, @@ -159,7 +159,7 @@ func buildControllerRoles() ([]rbacv1.ClusterRole, []rbacv1.ClusterRoleBinding) // The controller needs to be able to set a service's finalizers to be able to create an EndpointSlice // resource that is owned by the service and sets blockOwnerDeletion=true in its ownerRef. rbacv1helpers.NewRule("update").Groups(legacyGroup).Resources("services/finalizers").RuleOrDie(), - rbacv1helpers.NewRule("get", "list", "create", "update", "delete").Groups(discoveryGroup).Resources("endpointslices").RuleOrDie(), + rbacv1helpers.NewRule("get", "list", "watch", "create", "update", "delete").Groups(discoveryGroup).Resources("endpointslices").RuleOrDie(), eventsRule(), }, }) @@ -175,7 +175,7 @@ func buildControllerRoles() ([]rbacv1.ClusterRole, []rbacv1.ClusterRoleBinding) // resource that is owned by the endpoint and sets blockOwnerDeletion=true in its ownerRef. // see https://github.com/openshift/kubernetes/blob/8691466059314c3f7d6dcffcbb76d14596ca716c/pkg/controller/endpointslicemirroring/utils.go#L87-L88 rbacv1helpers.NewRule("update").Groups(legacyGroup).Resources("endpoints/finalizers").RuleOrDie(), - rbacv1helpers.NewRule("get", "list", "create", "update", "delete").Groups(discoveryGroup).Resources("endpointslices").RuleOrDie(), + rbacv1helpers.NewRule("get", "list", "watch", "create", "update", "delete").Groups(discoveryGroup).Resources("endpointslices").RuleOrDie(), eventsRule(), }, }) @@ -231,11 +231,11 @@ func buildControllerRoles() ([]rbacv1.ClusterRole, []rbacv1.ClusterRoleBinding) rbacv1helpers.NewRule("get", "list", "watch").Groups(autoscalingGroup).Resources("horizontalpodautoscalers").RuleOrDie(), rbacv1helpers.NewRule("update").Groups(autoscalingGroup).Resources("horizontalpodautoscalers/status").RuleOrDie(), rbacv1helpers.NewRule("get", "update").Groups("*").Resources("*/scale").RuleOrDie(), - rbacv1helpers.NewRule("list").Groups(legacyGroup).Resources("pods").RuleOrDie(), + rbacv1helpers.NewRule("list", "watch").Groups(legacyGroup).Resources("pods").RuleOrDie(), // allow listing resource, custom, and external metrics - rbacv1helpers.NewRule("list").Groups(resMetricsGroup).Resources("pods").RuleOrDie(), - rbacv1helpers.NewRule("get", "list").Groups(customMetricsGroup).Resources("*").RuleOrDie(), - rbacv1helpers.NewRule("get", "list").Groups(externalMetricsGroup).Resources("*").RuleOrDie(), + rbacv1helpers.NewRule("list", "watch").Groups(resMetricsGroup).Resources("pods").RuleOrDie(), + rbacv1helpers.NewRule("get", "list", "watch").Groups(customMetricsGroup).Resources("*").RuleOrDie(), + rbacv1helpers.NewRule("get", "list", "watch").Groups(externalMetricsGroup).Resources("*").RuleOrDie(), eventsRule(), }, }) @@ -261,11 +261,11 @@ func buildControllerRoles() ([]rbacv1.ClusterRole, []rbacv1.ClusterRoleBinding) role := rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "node-controller"}, Rules: []rbacv1.PolicyRule{ - rbacv1helpers.NewRule("get", "list", "update", "delete", "patch").Groups(legacyGroup).Resources("nodes").RuleOrDie(), + rbacv1helpers.NewRule("get", "list", "watch", "update", "delete", "patch").Groups(legacyGroup).Resources("nodes").RuleOrDie(), rbacv1helpers.NewRule("patch", "update").Groups(legacyGroup).Resources("nodes/status").RuleOrDie(), // used for pod deletion rbacv1helpers.NewRule("patch", "update").Groups(legacyGroup).Resources("pods/status").RuleOrDie(), - rbacv1helpers.NewRule("list", "get", "delete").Groups(legacyGroup).Resources("pods").RuleOrDie(), + rbacv1helpers.NewRule("list", "watch", "get", "delete").Groups(legacyGroup).Resources("pods").RuleOrDie(), eventsRule(), }, } @@ -295,7 +295,7 @@ func buildControllerRoles() ([]rbacv1.ClusterRole, []rbacv1.ClusterRoleBinding) ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "pod-garbage-collector"}, Rules: []rbacv1.PolicyRule{ rbacv1helpers.NewRule("list", "watch", "delete").Groups(legacyGroup).Resources("pods").RuleOrDie(), - rbacv1helpers.NewRule("get", "list").Groups(legacyGroup).Resources("nodes").RuleOrDie(), + rbacv1helpers.NewRule("get", "list", "watch").Groups(legacyGroup).Resources("nodes").RuleOrDie(), rbacv1helpers.NewRule("patch").Groups(legacyGroup).Resources("pods/status").RuleOrDie(), }, } @@ -507,7 +507,7 @@ func buildControllerRoles() ([]rbacv1.ClusterRole, []rbacv1.ClusterRoleBinding) // need list to get current RV for any resource // need patch for SSA of any resource // need create because SSA of a deleted resource will be interpreted as a create request, these always fail with a conflict error because UID is set - rbacv1helpers.NewRule("list", "create", "patch").Groups("*").Resources("*").RuleOrDie(), + rbacv1helpers.NewRule("list", "watch", "create", "patch").Groups("*").Resources("*").RuleOrDie(), rbacv1helpers.NewRule("update").Groups(storageVersionMigrationGroup).Resources("storageversionmigrations/status").RuleOrDie(), }, }) diff --git a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy_test.go b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy_test.go index 365b5c3d20d..21417d18d90 100644 --- a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy_test.go +++ b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy_test.go @@ -18,6 +18,7 @@ package bootstrappolicy import ( "reflect" + "slices" "testing" "k8s.io/apimachinery/pkg/api/meta" @@ -91,3 +92,15 @@ func TestControllerRoleLabel(t *testing.T) { } } } + +func TestControllerRoleVerbsConsistency(t *testing.T) { + roles := ControllerRoles() + for _, role := range roles { + for _, rule := range role.Rules { + verbs := rule.Verbs + if slices.Contains(verbs, "list") && !slices.Contains(verbs, "watch") { + t.Errorf("The ClusterRole %s has Verb `List` but does not have Verb `Watch`.", role.Name) + } + } + } +} diff --git a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy_test.go b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy_test.go index 235b9f32e4e..611afa886b6 100644 --- a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy_test.go +++ b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy_test.go @@ -20,6 +20,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "testing" "github.com/google/go-cmp/cmp" @@ -285,3 +286,39 @@ func TestClusterRoleLabel(t *testing.T) { } } } + +func TestNodeRuleVerbsConsistency(t *testing.T) { + rules := bootstrappolicy.NodeRules() + for _, rule := range rules { + verbs := rule.Verbs + if slices.Contains(verbs, "list") && !slices.Contains(verbs, "watch") { + t.Errorf("The NodeRule has Verb `List` but does not have Verb `Watch`.") + } + } +} + +func TestClusterRoleVerbsConsistency(t *testing.T) { + roles := bootstrappolicy.ClusterRoles() + for _, role := range roles { + for _, rule := range role.Rules { + verbs := rule.Verbs + if slices.Contains(verbs, "list") && !slices.Contains(verbs, "watch") { + t.Errorf("The ClusterRole %s has Verb `List` but does not have Verb `Watch`.", role.Name) + } + } + } +} + +func TestNamespaceRoleVerbsConsistency(t *testing.T) { + namespaceRoles := bootstrappolicy.NamespaceRoles() + for namespace, roles := range namespaceRoles { + for _, role := range roles { + for _, rule := range role.Rules { + verbs := rule.Verbs + if slices.Contains(verbs, "list") && !slices.Contains(verbs, "watch") { + t.Errorf("The Role %s/%s has Verb `List` but does not have Verb `Watch`.", namespace, role.Name) + } + } + } + } +} diff --git a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml index 8042564a725..ab5e6152d94 100644 --- a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml +++ b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml @@ -206,6 +206,7 @@ items: verbs: - delete - list + - watch - apiGroups: - "" - events.k8s.io @@ -466,6 +467,7 @@ items: - get - list - update + - watch - apiGroups: - "" resources: @@ -517,6 +519,7 @@ items: - get - list - update + - watch - apiGroups: - "" - events.k8s.io @@ -567,6 +570,7 @@ items: - get - list - update + - watch - apiGroups: - "" - events.k8s.io @@ -735,12 +739,14 @@ items: - pods verbs: - list + - watch - apiGroups: - metrics.k8s.io resources: - pods verbs: - list + - watch - apiGroups: - custom.metrics.k8s.io resources: @@ -748,6 +754,7 @@ items: verbs: - get - list + - watch - apiGroups: - external.metrics.k8s.io resources: @@ -755,6 +762,7 @@ items: verbs: - get - list + - watch - apiGroups: - "" - events.k8s.io @@ -896,6 +904,7 @@ items: - list - patch - update + - watch - apiGroups: - "" resources: @@ -918,6 +927,7 @@ items: - delete - get - list + - watch - apiGroups: - "" - events.k8s.io @@ -1040,6 +1050,7 @@ items: verbs: - get - list + - watch - apiGroups: - "" resources: