feat(spec): session SA requires system:image-builder for registry push#1635
feat(spec): session SA requires system:image-builder for registry push#1635markturansky wants to merge 13 commits into
Conversation
Document that the CP must bind system:image-builder ClusterRole to
session-{id}-sa at provision time. Without this binding the session SA
only has image-puller rights (granted automatically by OpenShift) and
agents cannot push built images to the internal registry.
Verified against running cluster: session SA receives system:image-pullers
via the namespace default RoleBinding but UNAUTHORIZED on blob upload
without explicit system:image-builder binding.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
📝 WalkthroughWalkthroughThis PR updates the control-plane specification to document an explicit Kubernetes RBAC provisioning step. During session reconciliation, a RoleBinding grants the ChangesSession RBAC Provisioning Documentation
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/control-plane/control-plane.spec.md`:
- Around line 91-92: Update the spec wording to remove the claim that OpenShift
automatically creates a RoleBinding for image pulls and instead state that the
control-plane explicitly provisions pull access: mention that the reconciler
calls ensureImagePullAccess to create a namespace-scoped RoleBinding named
ambient-image-puller in r.cfg.RunnerImageNamespace which binds the
cluster-scoped ClusterRole system:image-puller to the group
system:serviceaccounts:<session-namespace>, thereby granting pull access to
service accounts in the session namespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 45496aba-468a-42d6-b88a-12fa0f1d5d0c
📒 Files selected for processing (1)
specs/control-plane/control-plane.spec.md
| The CP binds the `system:image-builder` ClusterRole to `session-{id}-sa` via a namespace-scoped RoleBinding at provision time. This grants the runner pod push access to the OpenShift internal image registry (`image-registry.openshift-image-registry.svc:5000`), enabling agents to build and push images using daemonless tools such as `crane`. Pull access is provided automatically to all SAs in the namespace by OpenShift via the `system:image-pullers` RoleBinding created at namespace initialization. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify OpenShift automatic image-puller RoleBinding provisioning at namespace init
# Check for automatic system:image-pullers RoleBinding in a test namespace
# Expected: OpenShift creates this automatically, or the claim is incorrect
echo "=== Checking for automatic system:image-pullers RoleBinding pattern ==="
rg -n -C3 'system:image-puller' --type=go --type=md
echo "=== Searching for OpenShift namespace initialization behavior ==="
rg -n -C5 'namespace.*init' --iglob '*openshift*' --iglob '*image*'
echo "=== Checking reconciler image pull provisioning logic ==="
rg -nP -A10 -B5 'ensureImagePullAccess|image-puller' components/ambient-control-plane/internal/reconciler/Repository: ambient-code/platform
Length of output: 10887
Fix RBAC wording: image pull access isn’t “automatic” via OpenShift system:image-pullers.
- Repo docs for OpenShift internal-registry pulls explicitly instruct granting
system:image-pullertosystem:serviceaccountswithoc policy add-role-to-group system:image-puller system:serviceaccounts --namespace=$NAMESPACE(no claim of an OpenShift-created RoleBinding at namespace init). - The control-plane reconciler instead explicitly creates a
RoleBindingnamedambient-image-pullerinr.cfg.RunnerImageNamespace, binding thesystem:image-pullerClusterRole tosystem:serviceaccounts:<session-namespace>viaensureImagePullAccess(so the spec should reflect CP/explicit provisioning and the correct ClusterRole name/scope).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/control-plane/control-plane.spec.md` around lines 91 - 92, Update the
spec wording to remove the claim that OpenShift automatically creates a
RoleBinding for image pulls and instead state that the control-plane explicitly
provisions pull access: mention that the reconciler calls ensureImagePullAccess
to create a namespace-scoped RoleBinding named ambient-image-puller in
r.cfg.RunnerImageNamespace which binds the cluster-scoped ClusterRole
system:image-puller to the group system:serviceaccounts:<session-namespace>,
thereby granting pull access to service accounts in the session namespace.
Review — LGTM ✅Spec-only change. Verified against Factual accuracy:
One minor note: The new item 6 in the provisioning list is written as a present-tense fact ("RoleBinding granting..."), but this is unimplemented. Per the living-doc convention in this spec, consider marking it Pre-existing (not blocking): Item 2 (BOT_TOKEN Secret) no longer created by
— amber |
Follow-up — SA name discrepancy (supersedes prior LGTM)After Engineer's ping I re-read The SA name in the spec does not match the actual implementation target.
saName := fmt.Sprintf("ambient-session-%s", sessionName)Pod also runs under this SA ( runnerPodSAName = fmt.Sprintf("ambient-session-%s", name)The spec says: "RoleBinding granting
Clarification needed: Which component is the implementation target?
Everything else from the prior LGTM stands (gap is real in both components, OpenShift behavior claims accurate, infrastructure exists). This is the only factual accuracy issue — worth resolving before merge so the spec doesn't mislead the implementer. — amber |
|
@markturansky — good catch on reading The spec is
func serviceAccountName(sessionID string) string {
return fmt.Sprintf("session-%s-sa", safeResourceName(sessionID))
}
func safeResourceName(sessionID string) string {
return strings.ToLower(sessionID[:min(len(sessionID), 40)])
}Verified live: The actual SA running in this session is
SA name in the spec is accurate. Ready for LGTM. |
Verdict updated — LGTM ✅Engineer's clarification resolves the discrepancy. The implementation target is
Everything from the original LGTM stands:
Ready to merge once the test plan reference to — amber |
Amber Review — PR #1635What I verified
Blocking: spec describes unimplemented behavior in present tenseThe spec header states:
Other unimplemented sections follow this discipline with Both describe desired state as present-tense fact. Fix: apply the same 6. 🔲 RoleBinding granting `system:image-builder` ClusterRole to `session-{id}-sa` (enables push to the OpenShift internal image registry) — plannedAnd prefix the prose paragraph with a status annotation so readers know it is desired state, not current behavior. Minor notePR test plan item "Reviewer confirms spec wording is accurate against Amber · |
1 participant
Summary
system:image-builderRoleBinding to the KubeReconciler provisioning list (item 6) — the CP must explicitly bind this ClusterRole tosession-{id}-saat session provision timesystem:image-pullers(OpenShift namespace init) but push requires the explicit RoleBinding; daemonless tools such ascranerely on thisMotivation
Verified live in the session environment:
session-{id}-sahassystem:image-pullers(granted automatically at namespace creation)image-registry.openshift-image-registry.svc:5000returnsUNAUTHORIZED(blob upload 401) becausesystem:image-builderis not boundcomponents/ambient-control-plane/internal/reconciler/kube_reconciler.go:provisionSession()callsensureNamespaceExists,ensureServiceAccount,ensurePod,ensureService— no image-push RBAC anywhereKubeClient.CreateRoleBindinginfrastructure already exists (used byensureImagePullAccess) — the implementation path is clearsession-{id}-samatchesserviceAccountName()inkube_reconciler.goand confirmed against the live cluster SATest plan
kube_reconciler.goon mainsystem:image-builderRoleBinding inkube_reconciler.go:provisionSession()🤖 Generated with Claude Code