-
Notifications
You must be signed in to change notification settings - Fork 23
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 -- this is a pre-existing pattern that this PR extends. A generic SA/Role reconciliation helper using Go generics would be a good follow-up to reduce duplication.
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 a pre-existing pattern. Extracting a generic reconciliation helper is a good follow-up once this PR lands. Keeping it consistent with the existing structure 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 -- extracting a generic reconciliation helper would be a good follow-up. Keeping the existing pattern for now to minimize this PR's scope.
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 reconciliation helper is a good follow-up. Keeping this PR focused on the ServiceAccount separation; will track the refactor separately.
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 a pre-existing pattern that this PR extends. A generic reconciliation helper would be a good follow-up. |
||
|
|
@@ -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. Addressed in commit 74a9e9e. A log line has been added in the delete-and-recreate path when SetControllerReference fails after deletion, explaining that the RoleBinding is absent until the next reconciliation.
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. 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. Good point. Fixed in 74a9e9e -- added a log line when fails after the RoleBinding has already been deleted, so operators can see why the RoleBinding is temporarily absent.
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. Already addressed -- see line 260-262 of rbac.go. The |
||
| } | ||
| 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. This is the standard K8s operator pattern and self-heals on the next reconciliation loop, as you noted. The window is brief and acceptable for this use case.
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 is inherent to the immutable-field pattern and self-heals on the next reconciliation loop. Adding a targeted integration test for this edge case is a reasonable follow-up but out of scope for 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. As you noted, this is the standard K8s operator pattern and self-heals on the next reconciliation loop. The window is very brief and acceptable for this use case.
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 is inherent to the immutable-field pattern and self-heals on next reconciliation. Keeping as-is.
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 via the next reconciliation loop is the standard K8s operator pattern here, and the window is acceptably small. Good to have it documented for awareness. |
||
| } | ||
|
|
||
| // 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 about the orphaned SA on rollback. This is harmless (matches the existing controller SA behavior) but worth mentioning in release notes. Noted.
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 RBAC bindings would reference it after rollback), but documenting this in release notes is a sensible suggestion.
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. This is harmless but worth noting. Will keep in mind for 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 the orphaned SA on rollback. It is harmless since the SA has no elevated permissions, but worth noting. Will mention in release notes if needed.
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 elevated permissions) but worth noting in release notes. This matches the existing controller SA behavior.