[Mirror] Kubernetes/securitycontexts

[chaoran-chen]

This is a mirror of #7033 to run the CI. Please continue discussions in the original PR, I'll try to keep this in sync for CI and preview.

🚀 Preview: Add preview label to enable

[claude]

Claude finished @chaoran-chen's task in 4m 24s —— View job

---

Review

• Gather context (read diff + changed files)
• Review templates & helper
• Review values.yaml / values.schema.json
• Post review

Solid, well-scoped change. The helper mirrors the existing loculus.resources pattern closely, both values default to {} so it's backwards-compatible, and coverage of the pod/container specs is complete: I checked every workload template and every image: line, and each one gets both includes. All five loculus.configProcessor callers were updated for the new Values dict key. The schema keys match the component names actually passed by the templates, with no typos.

Seven inline comments. The ones worth acting on:

1. No CI coverage (values.yaml) — with both defaults {}, nothing in the repo renders a single securityContext. Setting a real default in values_e2e_and_dev.yaml would both smoke-test the templates and answer whether the images actually tolerate runAsNonRoot/readOnlyRootFilesystem.
2. Docs won't render the new options (values.schema.json) — missing the groups key that SchemaDocs.astro filters on.
3. Init containers share the app container's key (_ingest-pod-spec.tpl) — the resources helper already distinguishes ingest-init; the security context doesn't.
4. Two dead schema keys (values.schema.json) — podSecurityContext.silo-importer and podSecurityContext.config-processor validate but are never read.

One gap not attributable to the original work: raw-reads-processing-deployment.yaml landed on main after this branch started and has podScheduling but no security context includes. It's the only workload missing them — worth adding while you're in here so the merge doesn't leave a hole.

I could not run helm lint / helm template or prettier --check in this environment (commands blocked), so the review is static only. The schema looks prettier-formatted by eye, but per kubernetes/AGENTS.md it's worth confirming locally.
• branch kubernetes/securitycontexts

[claude]

These two properties are missing a groups key, so they won't show up in the generated docs. SchemaDocs.astro:70 only emits a row when definition.groups includes the requested group — the sibling podScheduling (line 1415) has "groups": ["general"] for exactly this reason.

Suggested change

	"podSecurityContext": {
	"$ref": "#/definitions/podSecurityContextSpec",
	"description": "Pod-level security context values keyed by component name. See https://kubernetes.io/docs/tasks/configure-pod-container/security-context/."
	},
	"containerSecurityContext": {
	"$ref": "#/definitions/containerSecurityContextSpec",
	"description": "Container-level security context values keyed by component name. See https://kubernetes.io/docs/tasks/configure-pod-container/security-context/."
	},
	"podSecurityContext": {
	"groups": ["general"],
	"$ref": "#/definitions/podSecurityContextSpec",
	"description": "Pod-level security context values keyed by component name. See https://kubernetes.io/docs/tasks/configure-pod-container/security-context/."
	},
	"containerSecurityContext": {
	"groups": ["general"],
	"$ref": "#/definitions/containerSecurityContextSpec",
	"description": "Container-level security context values keyed by component name. See https://kubernetes.io/docs/tasks/configure-pod-container/security-context/."
	},

Note that a "type": "object" may also be needed alongside for the Type column to render.

[claude]

silo-importer and config-processor are never their own pods — silo-importer is a second container in the silo pod (silo-deployment.yaml:82) and config-processor is an init container injected into the backend/website/keycloak/lapis/silo pods. loculus.podSecurityContext is never called with either key, so setting podSecurityContext.silo-importer or podSecurityContext.config-processor validates fine and then silently does nothing.

Suggest dropping both from podSecurityContextSpec (they're correctly kept in containerSecurityContextSpec).

[claude]

Missing trailing newline at EOF.

Also, the two defines are byte-identical apart from the values key. Worth collapsing into one helper to avoid the two drifting apart:

{{- define "loculus.securityContext" -}}
{{- $component := index . 0 -}}
{{- $scopeMap := index . 1 -}}
{{- if $scopeMap -}}
{{- $sc := or (index $scopeMap $component) (index $scopeMap "default") -}}
...

and call it as (list "backend" $.Values.podSecurityContext). Not blocking — the current shape does mirror loculus.resources, so keeping it is defensible.

[claude]

Two things about the default fallback:

1. No way to opt a component out. Once default is set, every component gets it and the only override is a full replacement. There's no value that disables the context for a single component — podSecurityContext.backend: {} renders securityContext: {} (harmless but not "unset"), and null fails schema validation since the $ref requires an object. If someone needs one workload (e.g. minio, database) to run without the hardened default, they currently can't express it. Consider documenting this or accepting null/false as an explicit disable.

2. Full replace vs. merge. values.yaml documents this, but a mergeOverwrite of default with the component entry would be much less error-prone for the common "same base, one field differs" case. If full-replace is deliberate, it's worth a one-line comment here in the template too, since this is where a future reader will look.

Neither is a bug, but (1) is the one likely to be hit in practice.

[claude]

Init containers reuse the app container's key. Here loculus.resources already distinguishes ingest-init from ingest, but the security context lumps version-check (busybox), wait-for-no-other-ingest (alpine/kubectl) and the snakemake ingest container under a single ingest key. Same pattern in keycloak-deployment.yaml:38 (keycloak-theme-prep), ena-submission-deployment.yaml:37 (flyway), taxonomy-deployment.yaml:30 (download-taxonomy-db).

That matters because the headline hardening knobs (runAsUser, runAsNonRoot, readOnlyRootFilesystem) frequently need to differ between an init container and the app it prepares — e.g. flyway and the ena-submission image don't necessarily run as the same UID. Right now there's no way to express that without dropping the setting for both.

Suggest reusing the keys the resources helper already defines (ingest-init) and adding equivalents for the other init containers, at least for those where a divergence is plausible.

[claude]

Indentation of this line changed from column 0 to 4 while adding the "Values" key. Harmless (the {{- trims it and nindent 8 sets the real indent), but it now matches neither keycloak-deployment.yaml:29 (column 0) nor silo-deployment.yaml:36 / lapis-deployment.yaml:30 (column 8). Same in loculus-website.yaml:28. Worth reverting to keep the diff to the functional change.

[claude]

This makes Values a required key of the loculus.configProcessor dict — a caller that omits it fails with nil pointer evaluating interface {}.containerSecurityContext. All five current callers were updated, so this is correct today, but the contract is now implicit.

Worth documenting the expected dict keys in a comment above the define (the way _ingest-pod-spec.tpl:6-12 does), so the next person adding a caller doesn't have to discover it from a render failure.

[claude]

Both default to {}, so this PR is a no-op at render time for every values file in the repo — CI will exercise the {{- end }} branch of both helpers and nothing else. A typo in a nindent level or a bad key would ship undetected.

Consider setting a non-trivial containerSecurityContext.default (and at least one component override) in values_e2e_and_dev.yaml so the e2e cluster actually renders and schedules pods with these fields. That also validates the interesting real-world question — whether the images tolerate runAsNonRoot / readOnlyRootFilesystem — which the current default of {} leaves entirely untested.