try to use userNamespaces to avoid having privileged workspaces#182
Merged
Conversation
📝 WalkthroughWalkthroughAdded a dedicated build ServiceAccount ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OperatorCtrl as Operator Ctrl
participant K8sAPI as Kubernetes API
participant TokenSvc as SA Token
participant BuildSystem as Tekton/Shipwright
participant Registry
User->>OperatorCtrl: reconcile OperatorConfig (create build SA & workspace SCC)
OperatorCtrl->>K8sAPI: create ServiceAccount `ado-build`
OperatorCtrl->>K8sAPI: create/update workspace SCC (priv/non-priv variants)
OperatorCtrl->>K8sAPI: read SCC -> determine UserNamespacesSupported
OperatorCtrl-->>User: update OperatorConfig.Status.UserNamespacesSupported
User->>BuildSystem: trigger build (PipelineRun / BuildRun)
BuildSystem->>K8sAPI: request token for registry using SA `ado-build`
K8sAPI->>TokenSvc: ServiceAccounts(namespace).CreateToken(ado-build)
TokenSvc-->>BuildSystem: token
BuildSystem->>Registry: push/pull using token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/operatorconfig/resources.go (1)
908-967:⚠️ Potential issue | 🟠 MajorAdd
ado-buildregistry-editor RoleBinding.The migration to
ado-buildServiceAccount for internal-registry tokens (ininternal/buildapi/server.go) is incomplete. Thepipeline-registry-editorRoleBinding inconfig/rbac/pipeline_registry_rolebinding.yamlstill targets only thepipelineServiceAccount, but the code now creates tokens forado-build. Without an equivalentregistry-editorbinding forado-build, internal-registry authentication will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/operatorconfig/resources.go` around lines 908 - 967, The code creates the ado-build ServiceAccount (automotivev1alpha1.BuildServiceAccountName) but does not create a corresponding registry-editor RoleBinding, so internal-registry tokens for ado-build will fail; add a new function (e.g., buildBuildRegistryRoleBinding(namespace string) *rbacv1.RoleBinding) that constructs a RoleBinding which references the same registry-editor Role/ClusterRole used by the existing pipeline registry RoleBinding (the role name from config/rbac/pipeline_registry_rolebinding.yaml) and includes a Subject for automotivev1alpha1.BuildServiceAccountName in the given namespace; return this RoleBinding from reconciliation where other build RBs (e.g., buildBuildSCCRoleBinding) are created so ado-build receives the registry-editor permissions.
🧹 Nitpick comments (1)
internal/controller/operatorconfig/resources.go (1)
25-26: Rename the leftoverpipelineSCCBindingNamesymbol.The value is build-specific now, but the identifier still points readers at the old model and makes the new helpers harder to follow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/operatorconfig/resources.go` around lines 25 - 26, The constant pipelineSCCBindingName is misnamed for build-specific usage; rename it to a clearer identifier (e.g., buildSCCBindingName or buildPrivilegedSCCBindingName) and update all references to it across the package (including any helper functions that consume pipelineSCCBindingName and any tests or docs), leaving sccPrivilegedRoleName as-is; ensure you update imports/usages and run unit tests to confirm no missing symbol errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/workspace/controller.go`:
- Around line 232-239: The container capabilities currently only append to the
runtime defaults (using caps/Capabilities.Add), so the container still runs with
the full default capability set; update the pod spec where the Capabilities are
set (the caps slice and the usage of Capabilities.Add) to explicitly drop all
defaults first by setting Capabilities.Drop = ["ALL"] before setting
Capabilities.Add = caps (and keep the conditional append of "SYS_ADMIN" for
configuredImage); apply the same Drop addition to the other instance referenced
around the 320-324 region so the allowlist is enforced.
---
Outside diff comments:
In `@internal/controller/operatorconfig/resources.go`:
- Around line 908-967: The code creates the ado-build ServiceAccount
(automotivev1alpha1.BuildServiceAccountName) but does not create a corresponding
registry-editor RoleBinding, so internal-registry tokens for ado-build will
fail; add a new function (e.g., buildBuildRegistryRoleBinding(namespace string)
*rbacv1.RoleBinding) that constructs a RoleBinding which references the same
registry-editor Role/ClusterRole used by the existing pipeline registry
RoleBinding (the role name from config/rbac/pipeline_registry_rolebinding.yaml)
and includes a Subject for automotivev1alpha1.BuildServiceAccountName in the
given namespace; return this RoleBinding from reconciliation where other build
RBs (e.g., buildBuildSCCRoleBinding) are created so ado-build receives the
registry-editor permissions.
---
Nitpick comments:
In `@internal/controller/operatorconfig/resources.go`:
- Around line 25-26: The constant pipelineSCCBindingName is misnamed for
build-specific usage; rename it to a clearer identifier (e.g.,
buildSCCBindingName or buildPrivilegedSCCBindingName) and update all references
to it across the package (including any helper functions that consume
pipelineSCCBindingName and any tests or docs), leaving sccPrivilegedRoleName
as-is; ensure you update imports/usages and run unit tests to confirm no missing
symbol errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: defc076a-b25f-4169-b860-a6d216b6316f
📒 Files selected for processing (9)
api/v1alpha1/operatorconfig_types.gointernal/buildapi/server.gointernal/common/tasks/tasks.gointernal/controller/containerbuild/controller.gointernal/controller/imagebuild/controller.gointernal/controller/imagereseal/controller.gointernal/controller/operatorconfig/controller.gointernal/controller/operatorconfig/resources.gointernal/controller/workspace/controller.go
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/workspace/controller.go`:
- Around line 235-255: The fallback branch that sets secCtx.Privileged when
userNamespaces is false currently grants privileged mode to any workspace image;
change it so Privileged is only applied for the trusted toolchain image: in the
else branch of the userNamespaces check, test if image == configuredImage and
only then set secCtx = &corev1.SecurityContext{Privileged: ptr.To(true),
AllowPrivilegeEscalation: ptr.To(true)}; otherwise create a non-privileged
SecurityContext (e.g. secCtx = &corev1.SecurityContext{AllowPrivilegeEscalation:
ptr.To(true)}) so untrusted user-specified images cannot run privileged. Ensure
you update uses of secCtx accordingly (symbols: userNamespaces, operatorConfig,
image, configuredImage, secCtx, SecurityContext, Privileged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3675624f-9356-44e3-80cf-18ef843ba9e2
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (4)
api/v1alpha1/operatorconfig_types.gointernal/controller/operatorconfig/controller.gointernal/controller/operatorconfig/resources.gointernal/controller/workspace/controller.go
on OCP 4.20+ we don't have to use privileged pods to run nested podman Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/operatorconfig_types.go`:
- Around line 583-585: The new UserNamespacesSupported bool field added to the
OperatorConfig status type (UserNamespacesSupported) is not present in the
generated deepcopy implementations; regenerate the API artifacts so
zz_generated.deepcopy.go and CRD manifests include this field by running the
project codegen: regenerate deepcopy/clients/listers/informers by running the
generation targets (e.g., run make generate manifests) and commit the updated
generated files so the deepcopy methods for the OperatorConfig status include
UserNamespacesSupported.
In `@internal/controller/workspace/controller.go`:
- Around line 232-235: The current logic computes userNamespaces :=
operatorConfig != nil && operatorConfig.Status.UserNamespacesSupported which
conflates “not yet observed” and “explicitly unsupported”; change
ensurePod()/the workspace controller to treat operatorConfig.Status as unknown
until status.ObservedGeneration >= operatorConfig.Generation (or equivalent
reconciliation marker) and requeue the workspace reconcile when the
OperatorConfig status is not yet observed, so you only decide HostUsers /
container security context (and the userNamespaces boolean) after the
OperatorConfig status has caught up; alternatively introduce an explicit
tri-state (unknown/true/false) for userNamespaces and ensure the code path at
both occurrences (the userNamespaces assignment and the logic at lines
referenced) defers to requeue when unknown instead of taking the fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43669371-f40a-4b68-8c65-bfb1246ecc26
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (4)
api/v1alpha1/operatorconfig_types.gointernal/controller/operatorconfig/controller.gointernal/controller/operatorconfig/resources.gointernal/controller/workspace/controller.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/operatorconfig/controller.go
- internal/controller/operatorconfig/resources.go
bkhizgiy
approved these changes
Mar 26, 2026
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On OCP 4.20 we can use userNamespace and avoid using privileged workspace pods, older version will defer to privileged for now
Summary by CodeRabbit
New Features
Improvements