Skip to content

Make runAsUser/fsGroup overridable per component - #1702

Open
josephtate wants to merge 1 commit into
pulp:mainfrom
josephtate:override-pod-security-context
Open

josephtate wants to merge 1 commit into
pulp:mainfrom
josephtate:override-pod-security-context

Conversation

@josephtate

Copy link
Copy Markdown
Contributor

Fixes #1492

Summary

Adds one optional field to the api, content, worker and web sections of the Pulp CR:

Field Type Effect
pod_security_context *corev1.PodSecurityContext replaces the pod-level securityContext for that component

Unset fields leave the generated Deployments unchanged (covered by tests), so existing installs do not roll on upgrade.

spec:
  web:
    pod_security_context:
      runAsUser: 101
      fsGroup: 101

Use case

The operator hardcodes runAsUser: 700 and fsGroup: 700 for the api, content, worker and web pods. A custom pulp-web image whose nginx user is UID 101 (or any other non-700 value) fails with:

mkdir() "/var/cache/nginx/client_temp" failed (13: Permission denied)

because the hardcoded fsGroup: 700 overrides Kubernetes' own default-to-GID-0 behavior that the image's chmod 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 empty securityContext on OpenShift. This PR follows the same typed/overridable-field shape used for extra_volumes/service_account_name in #1701, instead of hardcoding a different fixed UID for the next person with a different image.

Discarded approaches

  • Changing the hardcoded default from 700 to some other fixed value (e.g. 101): just moves which UID/GID combination breaks for the next person with a different custom image. Making it overridable fixes the class of bug instead of one instance of it.
  • Per-field defaulting/merging (e.g. fsGroup override keeps the default runAsUser): the field is a single typed *corev1.PodSecurityContext that replaces the whole struct, matching how affinity already 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.
  • Operator-side validation of the field's subfields (e.g. rejecting runAsGroup without runAsUser, or runAsUser: 0): the API server and container runtime already enforce these; adding our own validation would duplicate that for no new guarantee.

Notes for reviewers

  • Jobs (migration, admin password reset, signing), the database/cache (Redis) pods, and PulpBackup/PulpRestore are unchanged and keep their own hardcoded security context. Documented in the new docs page's INFO box.
  • On OpenShift, api/content/worker get an empty securityContext by default (the platform's SCC assigns UID/GID), same as before this PR; setting pod_security_context overrides that too (see Review section below — this did not work correctly in the first version of this patch).
  • web's deploymentForPulpWeb already special-cased OpenShift before this PR (empty securityContext instead 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

  1. On OpenShift, api/content/worker silently discarded the override.

    • controllers/ocp/deployment.go's defaultsForOCPDeployment runs after the common Deploy() path and unconditionally overwrote Spec.Template.Spec.SecurityContext with an empty PodSecurityContext{} for api, content and worker, regardless of what setPodSecurityContext had 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.
    • Fixed by having defaultsForOCPDeployment check the CR's pod_security_context field 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).
    • Added controllers/ocp/pod_security_context_test.go covering both the default (unset) and override cases for all three components; reverting the fix fails the override test.
  2. The docs did not say the field replaces the context instead of merging it.

    • pod_security_context: {fsGroup: 2000} drops runAsUser: 700 entirely; the first version of the docs page read as though unset subfields kept their default.
    • Added an explicit INFO box stating this.
  3. The docs' "not available for" INFO box was garbled ("the database, cache jobs (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

Finding Reason not addressed here
database/cache pods and the AdminPasswordJob/MigrationJob/SigningJob/PulpBackup/PulpRestore pods also hardcode a security context Out of scope for #1492, which is specifically about pulp_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.
The reflection-based field lookup (reflect.ValueOf(pulp.Spec).FieldByName(...)) panics if called with a pulpcoreType that isn't Api/Content/Worker Matches the exact pre-existing pattern used by setAffinity/setNodeSelector/setTolerations in the same file; the call site is only ever build() with one of those three types. Not a new risk introduced by this change.
The CR's *corev1.PodSecurityContext pointer is assigned directly into the Deployment spec without DeepCopy() Same precedent as setAffinity, which aliases *corev1.Affinity the 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.
Validation of PodSecurityContext subfield combinations (e.g. runAsUser: 0 with runAsNonRoot: true, a negative UID) Left to the API server and container runtime, same precedent as #1701's extra_volumes/service_account_name validation decisions on this CRD.

Tests

  • Generator tests (controllers/pod_security_context_test.go, controllers/ocp/pod_security_context_test.go, controllers/repo_manager/pod_security_context_test.go) run the real Deploy path 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.
  • Envtest CRD validation (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 in controller_test.go:1291 reproduces identically on origin/main without this branch's changes), make bundle and operator-sdk bundle validate, and .ci/scripts/validate_commit_message.py all 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:stable and quay.io/pulp/pulp-web:stable) with a Pulp CR setting pod_security_context differently per component: runAsUser:101,fsGroup:101 on api, runAsUser:102,fsGroup:102 on content, a partial override (runAsUser:700,runAsGroup:2000, no fsGroup) on worker, and runAsUser:101,fsGroup:101 on web.

  • The api and content pods came up Running; kubectl exec ... id confirmed uid=101 and uid=102 respectively, matching the CR.
  • The worker pod came up Running with the partial override applied as specified; id confirmed uid=700 gid=2000.
  • The web pod with the UID 101 override reproduced the exact error from Wrong runAsUser UID for pulp_web container #1492 on the stock upstream image (mkdir() "/var/cache/nginx/client_temp" failed (13: Permission denied)), confirming the override reaches the container and matches the original report precisely.
  • Removing the override (kubectl patch pulp ... -p '{"spec":{"web":{"pod_security_context":null}}}') rolled web back to the default 700/700 and the pod became healthy again, confirming the unset-field default is preserved and the change reconciles live.
  • Setting runAsGroup without runAsUser on 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.

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)
@openshift-ci
openshift-ci Bot requested review from git-hyagi and ipanova November 7, 2026 03:16
@openshift-ci

openshift-ci Bot commented Nov 7, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: josephtate
Once this PR has been reviewed and has the lgtm label, please assign dkliban for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci

openshift-ci Bot commented Nov 7, 2026

Copy link
Copy Markdown

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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

josephtate added a commit to josephtate/pulp-oci-images that referenced this pull request Nov 9, 2026
…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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong runAsUser UID for pulp_web container

1 participant