Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1057,7 +1057,7 @@ func (r *JumpstarterReconciler) createRouterDeployment(jumpstarter *operatorv1al
Type: corev1.SeccompProfileTypeRuntimeDefault,
},
},
ServiceAccountName: fmt.Sprintf("%s-controller-manager", jumpstarter.Name),
ServiceAccountName: fmt.Sprintf("%s-router-sa", jumpstarter.Name),
TopologySpreadConstraints: jumpstarter.Spec.Routers.TopologySpreadConstraints,
},
},
Expand Down
273 changes: 243 additions & 30 deletions controller/deploy/operator/internal/controller/jumpstarter/rbac.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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
Comment on lines 66 to +68
Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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.

// 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 {
Comment thread
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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Expand Down Expand 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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] In the delete-and-recreate path, if SetControllerReference fails here (after Delete already succeeded on line 253), the error is returned without a log entry explaining why the RoleBinding is now absent. Adding a brief log line could help debugging in the unlikely event of a scheme misconfiguration. Self-heals on next reconciliation either way.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 74a9e9e -- added log.Error when SetControllerReference fails after the old RoleBinding has already been deleted, making the transient absent state traceable in operator logs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in commit 74a9e9e -- log.Error now fires when SetControllerReference fails after the deletion, explaining why the RoleBinding is absent until next reconciliation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 74a9e9e -- added a log.Error line when SetControllerReference fails after the delete, so the absent RoleBinding state is logged before the error is returned.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 74a9e9e -- added a log.Error when SetControllerReference fails after the old RoleBinding has been deleted, making the transient absent state traceable in operator logs.

}
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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] Non-atomic delete-and-recreate for immutable RoleRef: if Delete succeeds on line 253 but Create fails on line 262, the RoleBinding will be absent until the next reconciliation loop. This is the standard K8s operator pattern for immutable fields and self-heals, so it is acceptable as-is. Just flagging for awareness in case you want to add a targeted integration test for this window.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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
Comment thread
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
Comment thread
raballew marked this conversation as resolved.
}
Comment thread
raballew marked this conversation as resolved.

Expand All @@ -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 {
Comment thread
raballew marked this conversation as resolved.
return &corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("%s-router-sa", jumpstarter.Name),
Comment thread
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{
Expand Down Expand Up @@ -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"},
},
},
Comment thread
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{
Expand Down
Loading
Loading