From 5bfa810f41c2ecdd786b72cf641fbe25738a8570 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Wed, 8 Apr 2026 15:36:19 +0000 Subject: [PATCH 1/8] fix: separate ServiceAccount for router workloads (zero RBAC) The controller and router previously shared the same `controller-manager` ServiceAccount, giving the router unnecessary cluster-wide secrets CRUD access. This creates a dedicated `router-sa` ServiceAccount with no RBAC bindings and `automountServiceAccountToken: false`, following the principle of least privilege. Fixes #351 Co-Authored-By: Claude Opus 4.6 --- .../additional-router-deployment.yaml | 3 +- .../templates/rbac/leader_election_role.yaml | 2 +- .../rbac/leader_election_role_binding.yaml | 2 +- .../templates/rbac/role_binding.yaml | 2 +- .../templates/rbac/service_account.yaml | 13 ++++- .../templates/router-deployment.yaml | 3 +- .../jumpstarter/jumpstarter_controller.go | 5 +- .../internal/controller/jumpstarter/rbac.go | 57 +++++++++++++++++++ 8 files changed, 79 insertions(+), 8 deletions(-) diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/additional-router-deployment.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/additional-router-deployment.yaml index 49cc02726..562457085 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/additional-router-deployment.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/additional-router-deployment.yaml @@ -106,6 +106,7 @@ spec: requests: cpu: 1000m memory: 256Mi - serviceAccountName: controller-manager + serviceAccountName: router-sa + automountServiceAccountToken: false terminationGracePeriodSeconds: 10 {{ end }} diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml index b0390bd15..0dd9975b5 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml @@ -3,7 +3,7 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: Role metadata: labels: - app.kubernetes.io/name: jumpstarter-router + app.kubernetes.io/name: jumpstarter-controller name: leader-election-role namespace: {{ default .Release.Namespace .Values.namespace }} rules: diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml index d60dc3c9f..cb24e1fab 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml @@ -2,7 +2,7 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: RoleBinding metadata: labels: - app.kubernetes.io/name: jumpstarter-router + app.kubernetes.io/name: jumpstarter-controller namespace: {{ default .Release.Namespace .Values.namespace }} name: leader-election-rolebinding roleRef: diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role_binding.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role_binding.yaml index 71d864b05..2cec5d38d 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role_binding.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role_binding.yaml @@ -2,7 +2,7 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: labels: - app.kubernetes.io/name: jumpstarter-router + app.kubernetes.io/name: jumpstarter-controller annotations: argocd.argoproj.io/sync-wave: "-1" name: jumpstarter-manager-rolebinding diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/service_account.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/service_account.yaml index 5359726af..86ec35683 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/service_account.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/service_account.yaml @@ -2,8 +2,19 @@ apiVersion: v1 kind: ServiceAccount metadata: labels: - app.kubernetes.io/name: jumpstarter-router + app.kubernetes.io/name: jumpstarter-controller annotations: argocd.argoproj.io/sync-wave: "-1" name: controller-manager namespace: {{ default .Release.Namespace .Values.namespace }} +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + labels: + app.kubernetes.io/name: jumpstarter-router + annotations: + argocd.argoproj.io/sync-wave: "-1" + name: router-sa + namespace: {{ default .Release.Namespace .Values.namespace }} +automountServiceAccountToken: false diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml index fa9978bf3..323243724 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml @@ -105,5 +105,6 @@ spec: secret: secretName: {{ .Values.grpc.tls.routerCertSecret }} {{- end }} - serviceAccountName: controller-manager + serviceAccountName: router-sa + automountServiceAccountToken: false terminationGracePeriodSeconds: 10 diff --git a/controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go b/controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go index 03e1d3123..c24c7aaa2 100644 --- a/controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go +++ b/controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go @@ -1057,8 +1057,9 @@ func (r *JumpstarterReconciler) createRouterDeployment(jumpstarter *operatorv1al Type: corev1.SeccompProfileTypeRuntimeDefault, }, }, - ServiceAccountName: fmt.Sprintf("%s-controller-manager", jumpstarter.Name), - TopologySpreadConstraints: jumpstarter.Spec.Routers.TopologySpreadConstraints, + ServiceAccountName: fmt.Sprintf("%s-router-sa", jumpstarter.Name), + AutomountServiceAccountToken: boolPtr(false), + TopologySpreadConstraints: jumpstarter.Spec.Routers.TopologySpreadConstraints, }, }, }, diff --git a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go index e03336181..52c2352a6 100644 --- a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go +++ b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go @@ -61,6 +61,46 @@ func (r *JumpstarterReconciler) reconcileRBAC(ctx context.Context, jumpstarter * "namespace", existingSA.Namespace, "operation", op) + // Router ServiceAccount (zero RBAC, no token automount) + desiredRouterSA := r.createRouterServiceAccount(jumpstarter) + + existingRouterSA := &corev1.ServiceAccount{} + existingRouterSA.Name = desiredRouterSA.Name + existingRouterSA.Namespace = desiredRouterSA.Namespace + + op, err = controllerutil.CreateOrUpdate(ctx, r.Client, existingRouterSA, func() error { + if existingRouterSA.CreationTimestamp.IsZero() { + existingRouterSA.Labels = desiredRouterSA.Labels + existingRouterSA.Annotations = desiredRouterSA.Annotations + existingRouterSA.AutomountServiceAccountToken = desiredRouterSA.AutomountServiceAccountToken + return nil + } + + if !serviceAccountNeedsUpdate(existingRouterSA, desiredRouterSA) { + log.V(1).Info("Router ServiceAccount is up to date, skipping update", + "name", existingRouterSA.Name, + "namespace", existingRouterSA.Namespace) + return nil + } + + existingRouterSA.Labels = desiredRouterSA.Labels + existingRouterSA.Annotations = desiredRouterSA.Annotations + existingRouterSA.AutomountServiceAccountToken = desiredRouterSA.AutomountServiceAccountToken + return nil + }) + + if err != nil { + log.Error(err, "Failed to reconcile Router ServiceAccount", + "name", desiredRouterSA.Name, + "namespace", desiredRouterSA.Namespace) + return err + } + + log.Info("Router ServiceAccount reconciled", + "name", existingRouterSA.Name, + "namespace", existingRouterSA.Namespace, + "operation", op) + // Role desiredRole := r.createRole(jumpstarter) @@ -169,6 +209,23 @@ func (r *JumpstarterReconciler) createServiceAccount(jumpstarter *operatorv1alph } } +// createRouterServiceAccount creates a service account for the router with no RBAC permissions +func (r *JumpstarterReconciler) createRouterServiceAccount(jumpstarter *operatorv1alpha1.Jumpstarter) *corev1.ServiceAccount { + automount := false + return &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-router-sa", jumpstarter.Name), + Namespace: jumpstarter.Namespace, + Labels: map[string]string{ + "app": "jumpstarter-router", + "app.kubernetes.io/name": "jumpstarter-router", + "app.kubernetes.io/managed-by": "jumpstarter-operator", + }, + }, + AutomountServiceAccountToken: &automount, + } +} + // createRole creates a role with necessary permissions for the controller func (r *JumpstarterReconciler) createRole(jumpstarter *operatorv1alpha1.Jumpstarter) *rbacv1.Role { return &rbacv1.Role{ From b1a519abde55758bf9b50dfc52eac43d794bce03 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Wed, 8 Apr 2026 16:38:02 +0000 Subject: [PATCH 2/8] fix: revert Helm chart changes per maintainer feedback Helm charts are deprecated; only the operator deployment path should be modified for the separate router ServiceAccount. Co-Authored-By: Claude Opus 4.6 --- .../templates/additional-router-deployment.yaml | 3 +-- .../templates/rbac/leader_election_role.yaml | 2 +- .../rbac/leader_election_role_binding.yaml | 2 +- .../templates/rbac/role_binding.yaml | 2 +- .../templates/rbac/service_account.yaml | 13 +------------ .../templates/router-deployment.yaml | 3 +-- 6 files changed, 6 insertions(+), 19 deletions(-) diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/additional-router-deployment.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/additional-router-deployment.yaml index 562457085..49cc02726 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/additional-router-deployment.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/additional-router-deployment.yaml @@ -106,7 +106,6 @@ spec: requests: cpu: 1000m memory: 256Mi - serviceAccountName: router-sa - automountServiceAccountToken: false + serviceAccountName: controller-manager terminationGracePeriodSeconds: 10 {{ end }} diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml index 0dd9975b5..b0390bd15 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml @@ -3,7 +3,7 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: Role metadata: labels: - app.kubernetes.io/name: jumpstarter-controller + app.kubernetes.io/name: jumpstarter-router name: leader-election-role namespace: {{ default .Release.Namespace .Values.namespace }} rules: diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml index cb24e1fab..d60dc3c9f 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml @@ -2,7 +2,7 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: RoleBinding metadata: labels: - app.kubernetes.io/name: jumpstarter-controller + app.kubernetes.io/name: jumpstarter-router namespace: {{ default .Release.Namespace .Values.namespace }} name: leader-election-rolebinding roleRef: diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role_binding.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role_binding.yaml index 2cec5d38d..71d864b05 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role_binding.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/role_binding.yaml @@ -2,7 +2,7 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: labels: - app.kubernetes.io/name: jumpstarter-controller + app.kubernetes.io/name: jumpstarter-router annotations: argocd.argoproj.io/sync-wave: "-1" name: jumpstarter-manager-rolebinding diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/service_account.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/service_account.yaml index 86ec35683..5359726af 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/service_account.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/service_account.yaml @@ -1,20 +1,9 @@ apiVersion: v1 kind: ServiceAccount -metadata: - labels: - app.kubernetes.io/name: jumpstarter-controller - annotations: - argocd.argoproj.io/sync-wave: "-1" - name: controller-manager - namespace: {{ default .Release.Namespace .Values.namespace }} ---- -apiVersion: v1 -kind: ServiceAccount metadata: labels: app.kubernetes.io/name: jumpstarter-router annotations: argocd.argoproj.io/sync-wave: "-1" - name: router-sa + name: controller-manager namespace: {{ default .Release.Namespace .Values.namespace }} -automountServiceAccountToken: false diff --git a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml index 323243724..fa9978bf3 100644 --- a/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml +++ b/controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml @@ -105,6 +105,5 @@ spec: secret: secretName: {{ .Values.grpc.tls.routerCertSecret }} {{- end }} - serviceAccountName: router-sa - automountServiceAccountToken: false + serviceAccountName: controller-manager terminationGracePeriodSeconds: 10 From 64b1e1bb5ec66a189f75bd926122ca220953da15 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Wed, 8 Apr 2026 17:18:12 +0000 Subject: [PATCH 3/8] fix: allow router SA to access K8s API for config loading The router process needs Kubernetes API access at startup to load its configuration from a ConfigMap (via ctrl.GetConfigOrDie() and LoadRouterConfiguration). Setting AutomountServiceAccountToken: false on both the ServiceAccount and pod spec prevented the router from authenticating, causing the pod to crash and never become ready (180s timeout in CI). Changes: - Remove AutomountServiceAccountToken: false from router ServiceAccount and pod spec so the token is mounted - Add a minimal router Role granting read-only access to configmaps and secrets (the only resources the router needs) - Add a RoleBinding to bind the router Role to the router ServiceAccount This maintains the security goal of separating the router SA from the controller SA while granting only the minimum permissions needed. Co-Authored-By: Claude Opus 4.6 --- .../jumpstarter/jumpstarter_controller.go | 5 +- .../internal/controller/jumpstarter/rbac.go | 142 +++++++++++++++++- 2 files changed, 139 insertions(+), 8 deletions(-) diff --git a/controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go b/controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go index c24c7aaa2..bd5abc237 100644 --- a/controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go +++ b/controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go @@ -1057,9 +1057,8 @@ func (r *JumpstarterReconciler) createRouterDeployment(jumpstarter *operatorv1al Type: corev1.SeccompProfileTypeRuntimeDefault, }, }, - ServiceAccountName: fmt.Sprintf("%s-router-sa", jumpstarter.Name), - AutomountServiceAccountToken: boolPtr(false), - TopologySpreadConstraints: jumpstarter.Spec.Routers.TopologySpreadConstraints, + ServiceAccountName: fmt.Sprintf("%s-router-sa", jumpstarter.Name), + TopologySpreadConstraints: jumpstarter.Spec.Routers.TopologySpreadConstraints, }, }, }, diff --git a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go index 52c2352a6..0d37d9846 100644 --- a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go +++ b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go @@ -72,7 +72,6 @@ func (r *JumpstarterReconciler) reconcileRBAC(ctx context.Context, jumpstarter * if existingRouterSA.CreationTimestamp.IsZero() { existingRouterSA.Labels = desiredRouterSA.Labels existingRouterSA.Annotations = desiredRouterSA.Annotations - existingRouterSA.AutomountServiceAccountToken = desiredRouterSA.AutomountServiceAccountToken return nil } @@ -85,7 +84,6 @@ func (r *JumpstarterReconciler) reconcileRBAC(ctx context.Context, jumpstarter * existingRouterSA.Labels = desiredRouterSA.Labels existingRouterSA.Annotations = desiredRouterSA.Annotations - existingRouterSA.AutomountServiceAccountToken = desiredRouterSA.AutomountServiceAccountToken return nil }) @@ -191,6 +189,88 @@ func (r *JumpstarterReconciler) reconcileRBAC(ctx context.Context, jumpstarter * "namespace", existingRoleBinding.Namespace, "operation", op) + // Router Role (minimal permissions: read configmaps) + desiredRouterRole := r.createRouterRole(jumpstarter) + + existingRouterRole := &rbacv1.Role{} + existingRouterRole.Name = desiredRouterRole.Name + existingRouterRole.Namespace = desiredRouterRole.Namespace + + op, err = controllerutil.CreateOrUpdate(ctx, r.Client, existingRouterRole, func() error { + if existingRouterRole.CreationTimestamp.IsZero() { + existingRouterRole.Labels = desiredRouterRole.Labels + existingRouterRole.Annotations = desiredRouterRole.Annotations + existingRouterRole.Rules = desiredRouterRole.Rules + return controllerutil.SetControllerReference(jumpstarter, existingRouterRole, r.Scheme) + } + + if !roleNeedsUpdate(existingRouterRole, desiredRouterRole) { + log.V(1).Info("Router Role is up to date, skipping update", + "name", existingRouterRole.Name, + "namespace", existingRouterRole.Namespace) + return nil + } + + existingRouterRole.Labels = desiredRouterRole.Labels + existingRouterRole.Annotations = desiredRouterRole.Annotations + existingRouterRole.Rules = desiredRouterRole.Rules + return controllerutil.SetControllerReference(jumpstarter, existingRouterRole, r.Scheme) + }) + + if err != nil { + log.Error(err, "Failed to reconcile Router Role", + "name", desiredRouterRole.Name, + "namespace", desiredRouterRole.Namespace) + return err + } + + log.Info("Router Role reconciled", + "name", existingRouterRole.Name, + "namespace", existingRouterRole.Namespace, + "operation", op) + + // Router RoleBinding + desiredRouterRoleBinding := r.createRouterRoleBinding(jumpstarter) + + existingRouterRoleBinding := &rbacv1.RoleBinding{} + existingRouterRoleBinding.Name = desiredRouterRoleBinding.Name + existingRouterRoleBinding.Namespace = desiredRouterRoleBinding.Namespace + + op, err = controllerutil.CreateOrUpdate(ctx, r.Client, existingRouterRoleBinding, func() error { + if existingRouterRoleBinding.CreationTimestamp.IsZero() { + existingRouterRoleBinding.Labels = desiredRouterRoleBinding.Labels + existingRouterRoleBinding.Annotations = desiredRouterRoleBinding.Annotations + existingRouterRoleBinding.Subjects = desiredRouterRoleBinding.Subjects + existingRouterRoleBinding.RoleRef = desiredRouterRoleBinding.RoleRef + return controllerutil.SetControllerReference(jumpstarter, existingRouterRoleBinding, r.Scheme) + } + + if !roleBindingNeedsUpdate(existingRouterRoleBinding, desiredRouterRoleBinding) { + log.V(1).Info("Router RoleBinding is up to date, skipping update", + "name", existingRouterRoleBinding.Name, + "namespace", existingRouterRoleBinding.Namespace) + return nil + } + + existingRouterRoleBinding.Labels = desiredRouterRoleBinding.Labels + existingRouterRoleBinding.Annotations = desiredRouterRoleBinding.Annotations + existingRouterRoleBinding.Subjects = desiredRouterRoleBinding.Subjects + existingRouterRoleBinding.RoleRef = desiredRouterRoleBinding.RoleRef + return controllerutil.SetControllerReference(jumpstarter, existingRouterRoleBinding, r.Scheme) + }) + + if err != nil { + log.Error(err, "Failed to reconcile Router RoleBinding", + "name", desiredRouterRoleBinding.Name, + "namespace", desiredRouterRoleBinding.Namespace) + return err + } + + log.Info("Router RoleBinding reconciled", + "name", existingRouterRoleBinding.Name, + "namespace", existingRouterRoleBinding.Namespace, + "operation", op) + return nil } @@ -209,9 +289,8 @@ func (r *JumpstarterReconciler) createServiceAccount(jumpstarter *operatorv1alph } } -// createRouterServiceAccount creates a service account for the router with no RBAC permissions +// createRouterServiceAccount creates a service account for the router with minimal RBAC permissions func (r *JumpstarterReconciler) createRouterServiceAccount(jumpstarter *operatorv1alpha1.Jumpstarter) *corev1.ServiceAccount { - automount := false return &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("%s-router-sa", jumpstarter.Name), @@ -222,7 +301,6 @@ func (r *JumpstarterReconciler) createRouterServiceAccount(jumpstarter *operator "app.kubernetes.io/managed-by": "jumpstarter-operator", }, }, - AutomountServiceAccountToken: &automount, } } @@ -278,6 +356,60 @@ func (r *JumpstarterReconciler) createRole(jumpstarter *operatorv1alpha1.Jumpsta } } +// createRouterRole creates a role with minimal permissions for the router (read configmaps and secrets) +func (r *JumpstarterReconciler) createRouterRole(jumpstarter *operatorv1alpha1.Jumpstarter) *rbacv1.Role { + return &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-router-role", jumpstarter.Name), + Namespace: jumpstarter.Namespace, + Labels: map[string]string{ + "app": "jumpstarter-router", + "app.kubernetes.io/name": "jumpstarter-router", + "app.kubernetes.io/managed-by": "jumpstarter-operator", + }, + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"configmaps"}, + Verbs: []string{"get", "list", "watch"}, + }, + { + APIGroups: []string{""}, + Resources: []string{"secrets"}, + Verbs: []string{"get", "list", "watch"}, + }, + }, + } +} + +// createRouterRoleBinding creates a role binding for the router service account +func (r *JumpstarterReconciler) createRouterRoleBinding(jumpstarter *operatorv1alpha1.Jumpstarter) *rbacv1.RoleBinding { + return &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-router-rolebinding", jumpstarter.Name), + Namespace: jumpstarter.Namespace, + Labels: map[string]string{ + "app": "jumpstarter-router", + "app.kubernetes.io/name": "jumpstarter-router", + "app.kubernetes.io/managed-by": "jumpstarter-operator", + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: fmt.Sprintf("%s-router-role", jumpstarter.Name), + }, + Subjects: []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: fmt.Sprintf("%s-router-sa", jumpstarter.Name), + Namespace: jumpstarter.Namespace, + }, + }, + } +} + // createRoleBinding creates a role binding for the controller func (r *JumpstarterReconciler) createRoleBinding(jumpstarter *operatorv1alpha1.Jumpstarter) *rbacv1.RoleBinding { return &rbacv1.RoleBinding{ From cdf6b75ca7c1612bb6b964cceceb40e012daf71b Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Wed, 8 Apr 2026 17:26:03 +0000 Subject: [PATCH 4/8] fix: remove unnecessary secrets permission from router Role The router only reads a ConfigMap via LoadRouterConfiguration() and does not access any secrets. Remove the secrets PolicyRule from the router Role per review feedback. Co-Authored-By: Claude Opus 4.6 --- .../operator/internal/controller/jumpstarter/rbac.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go index 0d37d9846..d31b40d44 100644 --- a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go +++ b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go @@ -356,7 +356,7 @@ func (r *JumpstarterReconciler) createRole(jumpstarter *operatorv1alpha1.Jumpsta } } -// createRouterRole creates a role with minimal permissions for the router (read configmaps and secrets) +// createRouterRole creates a role with minimal permissions for the router (read configmaps) func (r *JumpstarterReconciler) createRouterRole(jumpstarter *operatorv1alpha1.Jumpstarter) *rbacv1.Role { return &rbacv1.Role{ ObjectMeta: metav1.ObjectMeta{ @@ -374,11 +374,6 @@ func (r *JumpstarterReconciler) createRouterRole(jumpstarter *operatorv1alpha1.J Resources: []string{"configmaps"}, Verbs: []string{"get", "list", "watch"}, }, - { - APIGroups: []string{""}, - Resources: []string{"secrets"}, - Verbs: []string{"get", "list", "watch"}, - }, }, } } From b5390e99525280bf3c822669386c66598a44e626 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Mon, 13 Apr 2026 19:32:33 +0000 Subject: [PATCH 5/8] fix: address review nits on router SA comments - Fix misleading "zero RBAC, no token automount" comment to "uses dedicated minimal Role" - Add missing comment explaining why SetControllerReference is not called on router SA - Fix createRouterServiceAccount doc comment to not reference RBAC permissions Co-Authored-By: Claude Opus 4.6 --- .../deploy/operator/internal/controller/jumpstarter/rbac.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go index d31b40d44..ccf996fd5 100644 --- a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go +++ b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go @@ -61,7 +61,9 @@ func (r *JumpstarterReconciler) reconcileRBAC(ctx context.Context, jumpstarter * "namespace", existingSA.Namespace, "operation", op) - // Router ServiceAccount (zero RBAC, no token automount) + // Router ServiceAccount (uses dedicated minimal Role) + // Note: We intentionally do NOT set controller reference on ServiceAccount to prevent + // it from being garbage collected when the Jumpstarter CR is deleted desiredRouterSA := r.createRouterServiceAccount(jumpstarter) existingRouterSA := &corev1.ServiceAccount{} @@ -289,7 +291,7 @@ func (r *JumpstarterReconciler) createServiceAccount(jumpstarter *operatorv1alph } } -// createRouterServiceAccount creates a service account for the router with minimal RBAC permissions +// createRouterServiceAccount creates a dedicated service account for router workloads func (r *JumpstarterReconciler) createRouterServiceAccount(jumpstarter *operatorv1alpha1.Jumpstarter) *corev1.ServiceAccount { return &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ From 0e314ec7c659d0ef4e675e3853138d1d2e0feaa7 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Tue, 14 Apr 2026 08:56:21 +0000 Subject: [PATCH 6/8] fix: handle immutable RoleRef in RoleBinding reconciliation Kubernetes considers RoleRef immutable after a RoleBinding is created. Replace the CreateOrUpdate pattern for RoleBindings with a dedicated reconcileRoleBinding helper that detects RoleRef changes and uses delete-and-recreate instead of in-place update. This applies to both the controller and router RoleBindings. Addresses review feedback from @raballew. Co-Authored-By: Claude Opus 4.6 --- .../internal/controller/jumpstarter/rbac.go | 172 ++++++++++-------- 1 file changed, 100 insertions(+), 72 deletions(-) diff --git a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go index ccf996fd5..65bfe0e18 100644 --- a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go +++ b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go @@ -6,7 +6,10 @@ import ( corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -146,51 +149,12 @@ func (r *JumpstarterReconciler) reconcileRBAC(ctx context.Context, jumpstarter * "operation", op) // RoleBinding + // Note: RoleRef is immutable in Kubernetes. If it changes, we must delete and recreate. desiredRoleBinding := r.createRoleBinding(jumpstarter) - - existingRoleBinding := &rbacv1.RoleBinding{} - existingRoleBinding.Name = desiredRoleBinding.Name - existingRoleBinding.Namespace = desiredRoleBinding.Namespace - - op, err = controllerutil.CreateOrUpdate(ctx, r.Client, existingRoleBinding, func() error { - // Check if this is a new role binding or an existing one - if existingRoleBinding.CreationTimestamp.IsZero() { - // RoleBinding is being created, copy all fields from desired - existingRoleBinding.Labels = desiredRoleBinding.Labels - existingRoleBinding.Annotations = desiredRoleBinding.Annotations - existingRoleBinding.Subjects = desiredRoleBinding.Subjects - existingRoleBinding.RoleRef = desiredRoleBinding.RoleRef - return controllerutil.SetControllerReference(jumpstarter, existingRoleBinding, r.Scheme) - } - - // RoleBinding exists, check if update is needed - if !roleBindingNeedsUpdate(existingRoleBinding, desiredRoleBinding) { - log.V(1).Info("RoleBinding is up to date, skipping update", - "name", existingRoleBinding.Name, - "namespace", existingRoleBinding.Namespace) - return nil - } - - // Update needed - apply changes - existingRoleBinding.Labels = desiredRoleBinding.Labels - existingRoleBinding.Annotations = desiredRoleBinding.Annotations - existingRoleBinding.Subjects = desiredRoleBinding.Subjects - existingRoleBinding.RoleRef = desiredRoleBinding.RoleRef - return controllerutil.SetControllerReference(jumpstarter, existingRoleBinding, r.Scheme) - }) - - if err != nil { - log.Error(err, "Failed to reconcile RoleBinding", - "name", desiredRoleBinding.Name, - "namespace", desiredRoleBinding.Namespace) + if err := r.reconcileRoleBinding(ctx, jumpstarter, desiredRoleBinding); err != nil { return err } - log.Info("RoleBinding reconciled", - "name", existingRoleBinding.Name, - "namespace", existingRoleBinding.Namespace, - "operation", op) - // Router Role (minimal permissions: read configmaps) desiredRouterRole := r.createRouterRole(jumpstarter) @@ -232,47 +196,111 @@ func (r *JumpstarterReconciler) reconcileRBAC(ctx context.Context, jumpstarter * "operation", op) // Router RoleBinding + // Note: RoleRef is immutable in Kubernetes. If it changes, we must delete and recreate. desiredRouterRoleBinding := r.createRouterRoleBinding(jumpstarter) + if err := r.reconcileRoleBinding(ctx, jumpstarter, desiredRouterRoleBinding); err != nil { + return err + } - existingRouterRoleBinding := &rbacv1.RoleBinding{} - existingRouterRoleBinding.Name = desiredRouterRoleBinding.Name - existingRouterRoleBinding.Namespace = desiredRouterRoleBinding.Namespace - - op, err = controllerutil.CreateOrUpdate(ctx, r.Client, existingRouterRoleBinding, func() error { - if existingRouterRoleBinding.CreationTimestamp.IsZero() { - existingRouterRoleBinding.Labels = desiredRouterRoleBinding.Labels - existingRouterRoleBinding.Annotations = desiredRouterRoleBinding.Annotations - existingRouterRoleBinding.Subjects = desiredRouterRoleBinding.Subjects - existingRouterRoleBinding.RoleRef = desiredRouterRoleBinding.RoleRef - return controllerutil.SetControllerReference(jumpstarter, existingRouterRoleBinding, r.Scheme) - } + return nil +} - if !roleBindingNeedsUpdate(existingRouterRoleBinding, desiredRouterRoleBinding) { - log.V(1).Info("Router RoleBinding is up to date, skipping update", - "name", existingRouterRoleBinding.Name, - "namespace", existingRouterRoleBinding.Namespace) - return nil - } +// reconcileRoleBinding reconciles a RoleBinding, handling the immutable RoleRef field. +// Kubernetes does not allow updating RoleRef on an existing RoleBinding. If the desired +// RoleRef differs from the existing one, this function deletes the old RoleBinding and +// creates a new one. For all other fields, it uses a standard get-and-update pattern. +func (r *JumpstarterReconciler) reconcileRoleBinding( + ctx context.Context, + jumpstarter *operatorv1alpha1.Jumpstarter, + desired *rbacv1.RoleBinding, +) error { + log := logf.FromContext(ctx) - existingRouterRoleBinding.Labels = desiredRouterRoleBinding.Labels - existingRouterRoleBinding.Annotations = desiredRouterRoleBinding.Annotations - existingRouterRoleBinding.Subjects = desiredRouterRoleBinding.Subjects - existingRouterRoleBinding.RoleRef = desiredRouterRoleBinding.RoleRef - return controllerutil.SetControllerReference(jumpstarter, existingRouterRoleBinding, r.Scheme) - }) + existing := &rbacv1.RoleBinding{} + key := client.ObjectKeyFromObject(desired) + err := r.Client.Get(ctx, key, existing) + + if apierrors.IsNotFound(err) { + // RoleBinding does not exist, create it + if err := controllerutil.SetControllerReference(jumpstarter, desired, r.Scheme); err != nil { + return err + } + if err := r.Client.Create(ctx, desired); err != nil { + log.Error(err, "Failed to create RoleBinding", + "name", desired.Name, + "namespace", desired.Namespace) + return err + } + log.Info("RoleBinding reconciled", + "name", desired.Name, + "namespace", desired.Namespace, + "operation", "created") + return nil + } if err != nil { - log.Error(err, "Failed to reconcile Router RoleBinding", - "name", desiredRouterRoleBinding.Name, - "namespace", desiredRouterRoleBinding.Namespace) + log.Error(err, "Failed to get RoleBinding", + "name", desired.Name, + "namespace", desired.Namespace) return err } - log.Info("Router RoleBinding reconciled", - "name", existingRouterRoleBinding.Name, - "namespace", existingRouterRoleBinding.Namespace, - "operation", op) + // RoleRef is immutable -- if it differs we must delete and recreate + if !equality.Semantic.DeepEqual(existing.RoleRef, desired.RoleRef) { + log.Info("RoleBinding RoleRef changed, deleting and recreating", + "name", existing.Name, + "namespace", existing.Namespace) + if err := r.Client.Delete(ctx, existing); err != nil { + log.Error(err, "Failed to delete RoleBinding for recreation", + "name", existing.Name, + "namespace", existing.Namespace) + return err + } + if err := controllerutil.SetControllerReference(jumpstarter, desired, r.Scheme); err != nil { + return err + } + if err := r.Client.Create(ctx, desired); err != nil { + log.Error(err, "Failed to recreate RoleBinding", + "name", desired.Name, + "namespace", desired.Namespace) + return err + } + log.Info("RoleBinding reconciled", + "name", desired.Name, + "namespace", desired.Namespace, + "operation", "recreated") + return nil + } + + // RoleRef unchanged -- update other fields if needed + if !roleBindingNeedsUpdate(existing, desired) { + log.V(1).Info("RoleBinding is up to date, skipping update", + "name", existing.Name, + "namespace", existing.Namespace) + log.Info("RoleBinding reconciled", + "name", existing.Name, + "namespace", existing.Namespace, + "operation", "unchanged") + return nil + } + + existing.Labels = desired.Labels + existing.Annotations = desired.Annotations + existing.Subjects = desired.Subjects + if err := controllerutil.SetControllerReference(jumpstarter, existing, r.Scheme); err != nil { + return err + } + if err := r.Client.Update(ctx, existing); err != nil { + log.Error(err, "Failed to update RoleBinding", + "name", existing.Name, + "namespace", existing.Namespace) + return err + } + log.Info("RoleBinding reconciled", + "name", existing.Name, + "namespace", existing.Namespace, + "operation", "updated") return nil } From 47d520aea79280659d540aab23c541847defb387 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Wed, 15 Apr 2026 11:46:01 +0000 Subject: [PATCH 7/8] fix: add RBAC unit tests and reduce log noise in reconcileRoleBinding Add unit tests for router RBAC factory functions (createRouterServiceAccount, createRouterRole, createRouterRoleBinding) and reconcileRoleBinding covering all four code paths: create, delete-and-recreate, update, and no-op. Remove redundant Info-level log in the "unchanged" path of reconcileRoleBinding to reduce log noise during steady-state reconciliation. The V(1) debug log is sufficient. Co-Authored-By: Claude Opus 4.6 --- .../internal/controller/jumpstarter/rbac.go | 4 - .../controller/jumpstarter/rbac_test.go | 282 ++++++++++++++++++ 2 files changed, 282 insertions(+), 4 deletions(-) create mode 100644 controller/deploy/operator/internal/controller/jumpstarter/rbac_test.go diff --git a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go index 65bfe0e18..bb3dcdcb6 100644 --- a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go +++ b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go @@ -277,10 +277,6 @@ func (r *JumpstarterReconciler) reconcileRoleBinding( log.V(1).Info("RoleBinding is up to date, skipping update", "name", existing.Name, "namespace", existing.Namespace) - log.Info("RoleBinding reconciled", - "name", existing.Name, - "namespace", existing.Namespace, - "operation", "unchanged") return nil } diff --git a/controller/deploy/operator/internal/controller/jumpstarter/rbac_test.go b/controller/deploy/operator/internal/controller/jumpstarter/rbac_test.go new file mode 100644 index 000000000..03a9ef291 --- /dev/null +++ b/controller/deploy/operator/internal/controller/jumpstarter/rbac_test.go @@ -0,0 +1,282 @@ +/* +Copyright 2025. The Jumpstarter Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package jumpstarter + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + operatorv1alpha1 "github.com/jumpstarter-dev/jumpstarter-controller/deploy/operator/api/v1alpha1" +) + +var _ = Describe("RBAC factory functions", func() { + var ( + r *JumpstarterReconciler + js *operatorv1alpha1.Jumpstarter + ) + + BeforeEach(func() { + r = &JumpstarterReconciler{} + js = &operatorv1alpha1.Jumpstarter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + } + }) + + Describe("createRouterServiceAccount", func() { + It("should use the correct name format", func() { + sa := r.createRouterServiceAccount(js) + Expect(sa.Name).To(Equal("test-router-sa")) + Expect(sa.Namespace).To(Equal("default")) + }) + + It("should set router labels", func() { + sa := r.createRouterServiceAccount(js) + Expect(sa.Labels).To(HaveKeyWithValue("app", "jumpstarter-router")) + Expect(sa.Labels).To(HaveKeyWithValue("app.kubernetes.io/name", "jumpstarter-router")) + Expect(sa.Labels).To(HaveKeyWithValue("app.kubernetes.io/managed-by", "jumpstarter-operator")) + }) + }) + + Describe("createRouterRole", func() { + It("should use the correct name format", func() { + role := r.createRouterRole(js) + Expect(role.Name).To(Equal("test-router-role")) + Expect(role.Namespace).To(Equal("default")) + }) + + It("should set router labels", func() { + role := r.createRouterRole(js) + Expect(role.Labels).To(HaveKeyWithValue("app", "jumpstarter-router")) + Expect(role.Labels).To(HaveKeyWithValue("app.kubernetes.io/name", "jumpstarter-router")) + Expect(role.Labels).To(HaveKeyWithValue("app.kubernetes.io/managed-by", "jumpstarter-operator")) + }) + + It("should grant read-only access to configmaps only", func() { + role := r.createRouterRole(js) + Expect(role.Rules).To(HaveLen(1)) + Expect(role.Rules[0].APIGroups).To(Equal([]string{""})) + Expect(role.Rules[0].Resources).To(Equal([]string{"configmaps"})) + Expect(role.Rules[0].Verbs).To(ConsistOf("get", "list", "watch")) + }) + + It("should not grant access to secrets", func() { + role := r.createRouterRole(js) + for _, rule := range role.Rules { + Expect(rule.Resources).NotTo(ContainElement("secrets")) + } + }) + }) + + Describe("createRouterRoleBinding", func() { + It("should use the correct name format", func() { + rb := r.createRouterRoleBinding(js) + Expect(rb.Name).To(Equal("test-router-rolebinding")) + Expect(rb.Namespace).To(Equal("default")) + }) + + It("should set router labels", func() { + rb := r.createRouterRoleBinding(js) + Expect(rb.Labels).To(HaveKeyWithValue("app", "jumpstarter-router")) + Expect(rb.Labels).To(HaveKeyWithValue("app.kubernetes.io/name", "jumpstarter-router")) + Expect(rb.Labels).To(HaveKeyWithValue("app.kubernetes.io/managed-by", "jumpstarter-operator")) + }) + + It("should reference the router role", func() { + rb := r.createRouterRoleBinding(js) + Expect(rb.RoleRef.APIGroup).To(Equal("rbac.authorization.k8s.io")) + Expect(rb.RoleRef.Kind).To(Equal("Role")) + Expect(rb.RoleRef.Name).To(Equal("test-router-role")) + }) + + It("should bind to the router service account", func() { + rb := r.createRouterRoleBinding(js) + Expect(rb.Subjects).To(HaveLen(1)) + Expect(rb.Subjects[0].Kind).To(Equal("ServiceAccount")) + Expect(rb.Subjects[0].Name).To(Equal("test-router-sa")) + Expect(rb.Subjects[0].Namespace).To(Equal("default")) + }) + }) +}) + +var _ = Describe("reconcileRoleBinding", func() { + var ( + r *JumpstarterReconciler + js *operatorv1alpha1.Jumpstarter + ctx context.Context + ) + + BeforeEach(func() { + r = &JumpstarterReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + } + ctx = context.Background() + + js = &operatorv1alpha1.Jumpstarter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rbac-test", + Namespace: "default", + }, + } + // Create the Jumpstarter CR so that SetControllerReference works + err := k8sClient.Get(ctx, types.NamespacedName{Name: js.Name, Namespace: js.Namespace}, &operatorv1alpha1.Jumpstarter{}) + if errors.IsNotFound(err) { + js.Spec = operatorv1alpha1.JumpstarterSpec{ + BaseDomain: "example.com", + Controller: operatorv1alpha1.ControllerConfig{ + Image: "quay.io/jumpstarter/jumpstarter:latest", + Replicas: 1, + }, + Routers: operatorv1alpha1.RoutersConfig{ + Image: "quay.io/jumpstarter/jumpstarter:latest", + Replicas: 1, + }, + } + Expect(k8sClient.Create(ctx, js)).To(Succeed()) + } + // Re-fetch to get the server-assigned UID + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: js.Name, Namespace: js.Namespace}, js)).To(Succeed()) + }) + + AfterEach(func() { + // Clean up the RoleBinding if it exists + rb := &rbacv1.RoleBinding{} + err := k8sClient.Get(ctx, types.NamespacedName{Name: "rbac-test-router-rolebinding", Namespace: "default"}, rb) + if err == nil { + Expect(k8sClient.Delete(ctx, rb)).To(Succeed()) + } + // Clean up the Jumpstarter CR + resource := &operatorv1alpha1.Jumpstarter{} + err = k8sClient.Get(ctx, types.NamespacedName{Name: js.Name, Namespace: js.Namespace}, resource) + if err == nil { + Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + } + }) + + It("should create a RoleBinding when it does not exist", func() { + desired := r.createRouterRoleBinding(js) + err := r.reconcileRoleBinding(ctx, js, desired) + Expect(err).NotTo(HaveOccurred()) + + // Verify it was created + created := &rbacv1.RoleBinding{} + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: "rbac-test-router-rolebinding", + Namespace: "default", + }, created) + Expect(err).NotTo(HaveOccurred()) + Expect(created.RoleRef.Name).To(Equal("rbac-test-router-role")) + Expect(created.Subjects).To(HaveLen(1)) + Expect(created.Subjects[0].Name).To(Equal("rbac-test-router-sa")) + }) + + It("should be a no-op when the RoleBinding already matches", func() { + // Create it first + desired := r.createRouterRoleBinding(js) + err := r.reconcileRoleBinding(ctx, js, desired) + Expect(err).NotTo(HaveOccurred()) + + // Reconcile again with same desired state -- should be a no-op + desired2 := r.createRouterRoleBinding(js) + err = r.reconcileRoleBinding(ctx, js, desired2) + Expect(err).NotTo(HaveOccurred()) + + // Verify it still exists and is unchanged + existing := &rbacv1.RoleBinding{} + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: "rbac-test-router-rolebinding", + Namespace: "default", + }, existing) + Expect(err).NotTo(HaveOccurred()) + Expect(existing.RoleRef.Name).To(Equal("rbac-test-router-role")) + }) + + It("should update Subjects when they change but RoleRef is unchanged", func() { + // Create it first + desired := r.createRouterRoleBinding(js) + err := r.reconcileRoleBinding(ctx, js, desired) + Expect(err).NotTo(HaveOccurred()) + + // Now reconcile with updated Subjects but same RoleRef + desired2 := r.createRouterRoleBinding(js) + desired2.Subjects = []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: "updated-sa", + Namespace: "default", + }, + } + err = r.reconcileRoleBinding(ctx, js, desired2) + Expect(err).NotTo(HaveOccurred()) + + // Verify the Subjects were updated + existing := &rbacv1.RoleBinding{} + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: "rbac-test-router-rolebinding", + Namespace: "default", + }, existing) + Expect(err).NotTo(HaveOccurred()) + Expect(existing.Subjects).To(HaveLen(1)) + Expect(existing.Subjects[0].Name).To(Equal("updated-sa")) + Expect(existing.RoleRef.Name).To(Equal("rbac-test-router-role")) + }) + + It("should delete and recreate when RoleRef changes", func() { + // Create it first + desired := r.createRouterRoleBinding(js) + err := r.reconcileRoleBinding(ctx, js, desired) + Expect(err).NotTo(HaveOccurred()) + + // Get the original UID to verify it was recreated + original := &rbacv1.RoleBinding{} + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: "rbac-test-router-rolebinding", + Namespace: "default", + }, original) + Expect(err).NotTo(HaveOccurred()) + originalUID := original.UID + + // Reconcile with a different RoleRef + desired2 := r.createRouterRoleBinding(js) + desired2.RoleRef = rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: "different-role", + } + err = r.reconcileRoleBinding(ctx, js, desired2) + Expect(err).NotTo(HaveOccurred()) + + // Verify it was recreated with the new RoleRef and a new UID + recreated := &rbacv1.RoleBinding{} + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: "rbac-test-router-rolebinding", + Namespace: "default", + }, recreated) + Expect(err).NotTo(HaveOccurred()) + Expect(recreated.RoleRef.Name).To(Equal("different-role")) + Expect(recreated.UID).NotTo(Equal(originalUID)) + }) +}) From 74a9e9e9a11a846d47256ad41303e41f21c7f1ed Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Wed, 15 Apr 2026 20:40:23 +0000 Subject: [PATCH 8/8] fix: add log for absent RoleBinding and assert ResourceVersion in no-op test - Add log.Error when SetControllerReference fails after RoleBinding deletion, making the absent RoleBinding state traceable in operator logs - Assert ResourceVersion is unchanged in the no-op reconciliation test to prove no unnecessary API write occurred Co-Authored-By: Claude Opus 4.6 --- .../operator/internal/controller/jumpstarter/rbac.go | 3 +++ .../internal/controller/jumpstarter/rbac_test.go | 12 +++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go index bb3dcdcb6..b301fede2 100644 --- a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go +++ b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go @@ -257,6 +257,9 @@ func (r *JumpstarterReconciler) reconcileRoleBinding( return err } if err := controllerutil.SetControllerReference(jumpstarter, desired, r.Scheme); err != nil { + log.Error(err, "Failed to set controller reference after RoleBinding deletion; RoleBinding is absent until next reconciliation", + "name", desired.Name, + "namespace", desired.Namespace) return err } if err := r.Client.Create(ctx, desired); err != nil { diff --git a/controller/deploy/operator/internal/controller/jumpstarter/rbac_test.go b/controller/deploy/operator/internal/controller/jumpstarter/rbac_test.go index 03a9ef291..a20e84cd3 100644 --- a/controller/deploy/operator/internal/controller/jumpstarter/rbac_test.go +++ b/controller/deploy/operator/internal/controller/jumpstarter/rbac_test.go @@ -199,12 +199,21 @@ var _ = Describe("reconcileRoleBinding", func() { err := r.reconcileRoleBinding(ctx, js, desired) Expect(err).NotTo(HaveOccurred()) + // Capture ResourceVersion before the no-op reconciliation + before := &rbacv1.RoleBinding{} + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: "rbac-test-router-rolebinding", + Namespace: "default", + }, before) + Expect(err).NotTo(HaveOccurred()) + rvBefore := before.ResourceVersion + // Reconcile again with same desired state -- should be a no-op desired2 := r.createRouterRoleBinding(js) err = r.reconcileRoleBinding(ctx, js, desired2) Expect(err).NotTo(HaveOccurred()) - // Verify it still exists and is unchanged + // Verify it still exists, is unchanged, and no unnecessary API write occurred existing := &rbacv1.RoleBinding{} err = k8sClient.Get(ctx, types.NamespacedName{ Name: "rbac-test-router-rolebinding", @@ -212,6 +221,7 @@ var _ = Describe("reconcileRoleBinding", func() { }, existing) Expect(err).NotTo(HaveOccurred()) Expect(existing.RoleRef.Name).To(Equal("rbac-test-router-role")) + Expect(existing.ResourceVersion).To(Equal(rvBefore), "ResourceVersion should be unchanged when no update is needed") }) It("should update Subjects when they change but RoleRef is unchanged", func() {