-
Notifications
You must be signed in to change notification settings - Fork 20
fix: separate ServiceAccount for router workloads #433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5bfa810
b1a519a
64b1e1b
cdf6b75
b5390e9
0e314ec
47d520a
74a9e9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
|
|
||
|
|
@@ -61,6 +64,46 @@ func (r *JumpstarterReconciler) reconcileRBAC(ctx context.Context, jumpstarter * | |
| "namespace", existingSA.Namespace, | ||
| "operation", op) | ||
|
|
||
| // 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{} | ||
| existingRouterSA.Name = desiredRouterSA.Name | ||
| existingRouterSA.Namespace = desiredRouterSA.Namespace | ||
|
|
||
| op, err = controllerutil.CreateOrUpdate(ctx, r.Client, existingRouterSA, func() error { | ||
|
raballew marked this conversation as resolved.
|
||
| if existingRouterSA.CreationTimestamp.IsZero() { | ||
| existingRouterSA.Labels = desiredRouterSA.Labels | ||
| existingRouterSA.Annotations = desiredRouterSA.Annotations | ||
| 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 | ||
| 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) | ||
|
|
||
|
Comment on lines
64
to
109
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] The router SA reconciliation block (lines 64-109) follows an identical pattern to the controller SA block above (lines 23-62). Same goes for the two Role reconciliation blocks. Consider extracting a generic SA/Role reconciliation helper in a follow-up to reduce duplication. AI-generated, human reviewed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. A generic reconciliation helper would be a worthwhile follow-up refactor. Keeping it consistent with the existing pattern for now to avoid expanding the scope of this PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged. A generic reconciliation helper is a good follow-up refactor. Keeping consistent with the existing pattern for now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, the duplication is notable. A generic helper using Go generics or a shared closure is a good candidate for a follow-up refactoring PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed -- a generic reconciliation helper would reduce duplication. Keeping consistent with the existing pattern in this PR to minimize scope; worth tackling in a dedicated follow-up.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed -- extracting a generic SA/Role reconciliation helper would reduce duplication significantly. Tracking this as a follow-up refactor to keep the scope of this PR focused. |
||
|
|
@@ -106,51 +149,157 @@ 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) | ||
| if err := r.reconcileRoleBinding(ctx, jumpstarter, desiredRoleBinding); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| 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) | ||
| // 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) | ||
| } | ||
|
|
||
| // 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) | ||
| if !roleNeedsUpdate(existingRouterRole, desiredRouterRole) { | ||
| log.V(1).Info("Router Role is up to date, skipping update", | ||
| "name", existingRouterRole.Name, | ||
| "namespace", existingRouterRole.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) | ||
| 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 RoleBinding", | ||
| "name", desiredRoleBinding.Name, | ||
| "namespace", desiredRoleBinding.Namespace) | ||
| log.Error(err, "Failed to reconcile Router Role", | ||
| "name", desiredRouterRole.Name, | ||
| "namespace", desiredRouterRole.Namespace) | ||
| return err | ||
| } | ||
|
|
||
| log.Info("RoleBinding reconciled", | ||
| "name", existingRoleBinding.Name, | ||
| "namespace", existingRoleBinding.Namespace, | ||
| log.Info("Router Role reconciled", | ||
| "name", existingRouterRole.Name, | ||
| "namespace", existingRouterRole.Namespace, | ||
| "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 | ||
| } | ||
|
|
||
| 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) | ||
|
|
||
| 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 get RoleBinding", | ||
| "name", desired.Name, | ||
| "namespace", desired.Namespace) | ||
| return err | ||
| } | ||
|
|
||
| // 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 { | ||
| 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 | ||
|
Comment on lines
+259
to
+263
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] In the delete-and-recreate path, if AI-generated, human reviewed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch -- will add a log line before returning the error so the absent RoleBinding state is traceable in operator logs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in commit 74a9e9e -- added
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in commit 74a9e9e --
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in commit 74a9e9e -- added a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in commit 74a9e9e -- added a |
||
| } | ||
| 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 | ||
|
Comment on lines
+248
to
+275
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] Non-atomic delete-and-recreate for immutable RoleRef: if AI-generated, human reviewed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged. The non-atomic window between Delete and Create is inherent to the immutable RoleRef pattern in Kubernetes. As you noted, the next reconciliation loop will self-heal. The risk is minimal given that this only triggers if RoleRef actually changes (which it does not under normal operation). No code change needed.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged. As noted, this is the standard K8s operator pattern for immutable fields and self-heals on next reconciliation. No code change needed.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged. The self-healing behavior on next reconciliation makes this acceptable. Added a log.Error for the SetControllerReference failure case in commit 74a9e9e to improve traceability during the gap window.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged -- this is the standard K8s operator pattern and self-heals on the next reconciliation loop, so leaving as-is. Good to have it documented here for awareness.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged. As you noted, this is the standard K8s operator pattern and self-heals on next reconciliation. No change needed here. |
||
| } | ||
|
|
||
| // 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) | ||
| return nil | ||
|
raballew marked this conversation as resolved.
|
||
| } | ||
|
|
||
| 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 | ||
|
raballew marked this conversation as resolved.
|
||
| } | ||
|
raballew marked this conversation as resolved.
|
||
|
|
||
|
|
@@ -169,6 +318,21 @@ func (r *JumpstarterReconciler) createServiceAccount(jumpstarter *operatorv1alph | |
| } | ||
| } | ||
|
|
||
| // createRouterServiceAccount creates a dedicated service account for router workloads | ||
| func (r *JumpstarterReconciler) createRouterServiceAccount(jumpstarter *operatorv1alpha1.Jumpstarter) *corev1.ServiceAccount { | ||
|
raballew marked this conversation as resolved.
|
||
| return &corev1.ServiceAccount{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: fmt.Sprintf("%s-router-sa", jumpstarter.Name), | ||
|
raballew marked this conversation as resolved.
|
||
| Namespace: jumpstarter.Namespace, | ||
| Labels: map[string]string{ | ||
| "app": "jumpstarter-router", | ||
| "app.kubernetes.io/name": "jumpstarter-router", | ||
| "app.kubernetes.io/managed-by": "jumpstarter-operator", | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| // createRole creates a role with necessary permissions for the controller | ||
| func (r *JumpstarterReconciler) createRole(jumpstarter *operatorv1alpha1.Jumpstarter) *rbacv1.Role { | ||
| return &rbacv1.Role{ | ||
|
|
@@ -221,6 +385,55 @@ func (r *JumpstarterReconciler) createRole(jumpstarter *operatorv1alpha1.Jumpsta | |
| } | ||
| } | ||
|
|
||
| // 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{ | ||
| 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"}, | ||
| }, | ||
| }, | ||
|
raballew marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
|
||
| // 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{ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] The router SA intentionally skips
SetControllerReference, so it persists after the Jumpstarter CR is deleted. This matches the existing controller SA pattern, which is fine. Worth noting that on rollback to a pre-PR operator version the router SA will remain orphaned (harmless, but good to document in release notes).AI-generated, human reviewed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about the orphaned SA on rollback. The SA is harmless (no permissions without the corresponding Role/RoleBinding) but worth noting in release notes. Will include this in the release documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. The orphaned SA is harmless and will be documented in release notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about documenting the orphaned SA in release notes. The behavior is harmless (a ServiceAccount with no permissions) but worth calling out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. The orphaned router SA is harmless (no permissions without the accompanying Role/RoleBinding which are owned by the CR), but worth noting in release notes for operators who roll back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. The orphaned router SA on rollback is harmless but worth documenting. Will note this in release notes.