Repository navigation
Make runAsUser/fsGroup overridable per component - #1702
josephtate wants to merge 1 commit into
Conversation
The operator hardcodes `runAsUser: 700` and `fsGroup: 700` for the api, content, worker and web pods. A custom image whose pulp user runs under a different UID/GID has no way to adjust this without patching the operator. Add an optional `pod_security_context` field to the api, content, worker and web sections of the Pulp CR that lets users override the pod-level security attributes. When unset, the existing hardcoded values are kept unchanged. Fixes pulp#1492 Assisted By: Hermes Agent (claude-sonnet-5-5)
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: josephtate The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @josephtate. Thanks for your PR. I'm waiting for a pulp member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
…ID handling Containerfile.core was confirmed redundant against stock images/pulp-minimal/stable/Containerfile.core: identical plugin set (stock is actually slightly ahead), byte-for-byte identical /var/lib/pulp permission scheme, and a no-op rm -f /usr/local/bin/pulp-content guard present (and unnecessary) in both. Replaced it with a symlink to the canonical file so the custom-minimal build directory still documents which file is in use, with zero drift risk going forward. Containerfile.webserver: reverted the chmod g+w portability fix back to chown nginx:nginx (matching the exact ownership/permissions pulp-operator's production fork already enforces for this image, and in production right now), since the new pulp-operator pod_security_context CR field (pulp/pulp-operator#1702) replaces the need for the operator itself to hardcode runAsUser/fsGroup -- the image can go back to a fixed UID/GID and let the CR express it explicitly instead. [noissue]
Fixes #1492
Summary
Adds one optional field to the
api,content,workerandwebsections of the Pulp CR:pod_security_context*corev1.PodSecurityContextsecurityContextfor that componentUnset fields leave the generated Deployments unchanged (covered by tests), so existing installs do not roll on upgrade.
Use case
The operator hardcodes
runAsUser: 700andfsGroup: 700for the api, content, worker and web pods. A custompulp-webimage whosenginxuser is UID 101 (or any other non-700 value) fails with:because the hardcoded
fsGroup: 700overrides Kubernetes' own default-to-GID-0 behavior that the image'schmod g+w/group-writable convention depends on for portability. Nothing in the CR lets a user override these two values today; the only prior CR-driven exception is the web pod's automatic emptysecurityContexton OpenShift. This PR follows the same typed/overridable-field shape used forextra_volumes/service_account_namein #1701, instead of hardcoding a different fixed UID for the next person with a different image.Discarded approaches
fsGroupoverride keeps the defaultrunAsUser): the field is a single typed*corev1.PodSecurityContextthat replaces the whole struct, matching howaffinityalready works in this CRD. Merging would need per-field logic with no precedent elsewhere in the codebase, for a need not reported by the issue.runAsGroupwithoutrunAsUser, orrunAsUser: 0): the API server and container runtime already enforce these; adding our own validation would duplicate that for no new guarantee.Notes for reviewers
database/cache(Redis) pods, andPulpBackup/PulpRestoreare unchanged and keep their own hardcoded security context. Documented in the new docs page's INFO box.securityContextby default (the platform's SCC assigns UID/GID), same as before this PR; settingpod_security_contextoverrides that too (see Review section below — this did not work correctly in the first version of this patch).web'sdeploymentForPulpWebalready special-cased OpenShift before this PR (emptysecurityContextinstead of the hardcoded 700/700); the new override is checked first, OpenShift second, default last.Review of this patch
The first version of this patch was reviewed by an independent, read-only Claude Code session (claude-sonnet-5-5), prompted to be adversarial and to assume shortcuts had been taken. I checked each finding against the code before acting. The patch is a single commit that I amended, so this section describes what changed after review.
What I changed, and why
On OpenShift, api/content/worker silently discarded the override.
controllers/ocp/deployment.go'sdefaultsForOCPDeploymentruns after the commonDeploy()path and unconditionally overwroteSpec.Template.Spec.SecurityContextwith an emptyPodSecurityContext{}for api, content and worker, regardless of whatsetPodSecurityContexthad just set from the CR.web.go's own OpenShift branch checked the override first, so web behaved differently from the other three and only web actually worked as documented.defaultsForOCPDeploymentcheck the CR'spod_security_contextfield for that component first and skip the empty-context overwrite when it is set, matching web's existing priority (override, then OpenShift default, then hardcoded default).controllers/ocp/pod_security_context_test.gocovering both the default (unset) and override cases for all three components; reverting the fix fails the override test.The docs did not say the field replaces the context instead of merging it.
pod_security_context: {fsGroup: 2000}dropsrunAsUser: 700entirely; the first version of the docs page read as though unset subfields kept their default.The docs' "not available for" INFO box was garbled ("the
database,cachejobs (AdminPasswordJob...)" read as though "cache jobs" were a real thing). Rewrote it as a list, and added the OpenShift behavior note from item 1.What I did not change, and why
database/cachepods and theAdminPasswordJob/MigrationJob/SigningJob/PulpBackup/PulpRestorepods also hardcode a security contextpulp_web; only api/content/worker/web were needed to close it. Documented explicitly in the docs page's INFO box so users aren't surprised.reflect.ValueOf(pulp.Spec).FieldByName(...)) panics if called with apulpcoreTypethat isn'tApi/Content/WorkersetAffinity/setNodeSelector/setTolerationsin the same file; the call site is only everbuild()with one of those three types. Not a new risk introduced by this change.*corev1.PodSecurityContextpointer is assigned directly into the Deployment spec withoutDeepCopy()setAffinity, which aliases*corev1.Affinitythe same way. Nothing in the reconcile path mutates it after assignment. Deviating from the established pattern for only this one field would be inconsistent without a demonstrated bug.PodSecurityContextsubfield combinations (e.g.runAsUser: 0withrunAsNonRoot: true, a negative UID)extra_volumes/service_account_namevalidation decisions on this CRD.Tests
controllers/pod_security_context_test.go,controllers/ocp/pod_security_context_test.go,controllers/repo_manager/pod_security_context_test.go) run the realDeploypath for api, content, worker (common and OpenShift variants) and web. Each test was mutation-checked by reverting the corresponding wiring line and confirming the test fails.controllers/repo_manager/pod_security_context_crd_test.go) confirms the field round-trips for all four components without the API server pruning it, and that an unset field is absent.go build ./...,go vet,gofmt -l .,KUBEBUILDER_ASSETS=... go test ./...(the one pre-existing unrelated failure incontroller_test.go:1291reproduces identically onorigin/mainwithout this branch's changes),make bundleandoperator-sdk bundle validate, and.ci/scripts/validate_commit_message.pyall pass.Live cluster test
Ran the operator built from this branch against a throwaway kind cluster (Kubernetes v1.33.1,
quay.io/pulp/pulp-minimal:stableandquay.io/pulp/pulp-web:stable) with a Pulp CR settingpod_security_contextdifferently per component:runAsUser:101,fsGroup:101on api,runAsUser:102,fsGroup:102on content, a partial override (runAsUser:700,runAsGroup:2000, nofsGroup) on worker, andrunAsUser:101,fsGroup:101on web.Running;kubectl exec ... idconfirmeduid=101anduid=102respectively, matching the CR.Runningwith the partial override applied as specified;idconfirmeduid=700 gid=2000.mkdir() "/var/cache/nginx/client_temp" failed (13: Permission denied)), confirming the override reaches the container and matches the original report precisely.kubectl patch pulp ... -p '{"spec":{"web":{"pod_security_context":null}}}') rolled web back to the default700/700and the pod became healthy again, confirming the unset-field default is preserved and the change reconciles live.runAsGroupwithoutrunAsUseron worker initially hit a real container-runtime constraint (user group "N" is specified without user) before I corrected the test CR — that is the CRI enforcing its own requirement, not a bug in this change, and is now called out in the docs.Not covered by the live test: OpenShift's SCC-based behavior (no OpenShift cluster available; covered by the unit tests added in the review above instead).
AI disclosure
Per the AI policy: the code, tests, docs and this description were written with Hermes Agent running claude-sonnet-5-5 (Anthropic). The adversarial review above ran in Claude Code (claude-sonnet-5-5). The commit carries an
Assisted By:label. I reviewed the code and this description and can explain the change.