feat(helm): add pod-hardening and migration-Job knobs to the componentized chart - #35489
Conversation
|
|
Greptile SummaryThe PR adds pod-hardening options to the gateway, backend, UI, and migration Job while independently resolving the migration Job’s ServiceAccount.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the prior selector mismatch is prevented for gateway, backend, and UI across all immutable selector keys.
|
| Filename | Overview |
|---|---|
| helm/litellm/templates/_helpers.tpl | Adds migration ServiceAccount resolution and complete reserved-selector-label validation for all component Deployments. |
| helm/litellm/templates/gateway/deployment.yaml | Renders the new gateway hardening options and applies the selector-label validation that resolves the prior finding. |
| helm/litellm/templates/backend/deployment.yaml | Renders backend hardening options and uses the shared selector-label guard. |
| helm/litellm/templates/ui/deployment.yaml | Renders UI hardening options and uses the shared selector-label guard. |
| helm/litellm/templates/migrations-job.yaml | Adds independent ServiceAccount resolution, token control, pod labels, security contexts, volumes, and mounts to the migration Job. |
| helm/litellm/values.yaml | Defines empty-by-default values for the new component and migration Job controls and documents the ServiceAccount behavior. |
| helm/litellm/tests/pod_hardening_tests.yaml | Covers hardening rendering, zero-valued grace periods, sidecar templating, and all three reserved selector keys across the component Deployments. |
| helm/litellm/tests/migration_job_tests.yaml | Covers migration ServiceAccount branches, token mounting, hardening fields, volumes, and permissive Job-label overrides. |
Reviews (3): Last reviewed commit: "feat(helm): add pod-hardening and migrat..." | Re-trigger Greptile
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
4b9a37d to
c628a0f
Compare
|
@greptileai please re-review at c628a0f. The podLabels selector collision is fixed: a shared helper now fails at template time naming the key and component, instead of emitting a duplicate label key that the apiserver rejects. The migrations Job stays permissive, verified with a server-side dry-run, since its selector is controller-generated; it merges podLabels over the chart labels so an override replaces a key rather than duplicating it. |
…tized chart The componentized chart exposed no pod-hardening surface, so values that operators of hardened clusters routinely set (podSecurityContext, securityContext, extraContainers, podLabels, lifecycle, terminationGracePeriodSeconds) rendered nothing at all. Helm does not error on unknown values, so the deploy went green with none of the hardening applied. Adds those six knobs to gateway, backend, and ui, plus volumes, volumeMounts, podLabels, podSecurityContext, and securityContext on the migrations Job. Also fixes a first-install failure: the migrations Job is a pre-install/pre-upgrade hook, so borrowing the backend ServiceAccount name while the chart creates that account references an account that does not exist yet, and the Job pod is rejected as forbidden. The Job now resolves its own name through migrationJob.serviceAccountName, falling back to the namespace default account when the chart creates the backend one and keeping today's shared name otherwise.
c628a0f to
01b4def
Compare
|
@greptileai please re-review at 01b4def. Since your 5/5 at c628a0f this adds one thing: migrationJob.automountServiceAccountToken, defaulting to false, so the Job stops inheriting a projected API token when the ServiceAccount fallback sends it to the namespace default account while the operator disabled automounting on the backend. It is a value rather than a hardcoded literal so IAM database auth keeps an escape hatch. Three unit tests cover the default, an explicit true, and the backend-disabled configuration. |
TLDR
Problem this solves:
podSecurityContext,securityContext, sidecars,podLabelssilently render nothingHow it solves it:
Relevant issues
Fixes #34252
The ServiceAccount half is the componentized-chart analogue of #21344 and #20571, which fixed the same pre-install hook cycle in
helm/litellm-helm. This mirrors that chart's semantics and field names so operator values stay portable between the twoLinear ticket
Resolves LIT-4727
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
This is a deploy artifact, so the proof is tool-native. Before runs are at base commit
0a42f28850(extracted withgit archive), after runs are at4b9a37d86cfor sections 1 and 2 and at01b4def385for sections 3, 4 and 5. The only differences between those two are the podLabels selector guard in section 3 and the ServiceAccount token fix in section 41. The silent no-op
Same hardened values file rendered against both charts, counting occurrences in the output of
helm template lit ./helm/litellm -f hardened.yaml. The values file sets pod-level and container-level securityContext, an auth sidecar, podLabels, lifecycle hooks, and terminationGracePeriodSeconds on all three components plus the JobHelm does not error on unknown values, so before this PR that render is green with none of the hardening applied
2. The first-install failure
kind cluster, dedicated namespace, an in-cluster Postgres so the migration actually runs,
serviceAccounts.backend.create: true,gateway.enabled: false,backend.enabled: false,ui.enabled: false,migrationJob.backoffLimit: 0, migrations image tagv1.94.1Before, at
0a42f28850:The Job references the backend ServiceAccount, which is an ordinary Sync-phase resource, so during the
pre-installhook it does not exist yet and the pod is never created. ArgoCD mapshelm.sh/hook: pre-install,pre-upgradeto PreSync and hits the same cycleAfter, at
4b9a37d86c, same values file:The discriminator is explicit: before, the Job pod is never created at all and the install dies on a forbidden pod creation; after, the pod is created, runs as
defaultwith the requested hardening on it, and the migration applies cleanly against a real Postgres. The11sage onlit-litellm-backendis the created ServiceAccount arriving in the Sync phase, which is to say after the hook already needed it3. podLabels cannot quietly break the selector
The three selector keys are also emitted on the pod template, so a
podLabelsentry reusing one used to render a duplicate YAML key whose later value wins, leaving the pod template no longer matching the immutable selector.helm lint,helm template, andhelm unittestare all happy with that, and the apiserver is not. Server-side dry-run against a kind cluster, renderinggateway.podLabels: {app.kubernetes.io/component: not-gateway}from the chart at4b9a37d86c:The chart now refuses at template time, naming the key and the component, rather than dropping it silently:
The migrations Job is deliberately left permissive, and that was verified rather than assumed. Its selector is generated by the controller, so the same override is accepted:
The Job merges podLabels over the chart labels rather than appending them, so an override there replaces the key instead of rendering it twice. A duplicate key is worth avoiding even where the apiserver tolerates it: helm-unittest's parser refuses to load such a document at all (
mapping key "app.kubernetes.io/component" already defined), so appending would have left that path untestable4. The Job no longer inherits a ServiceAccount token it should not have
Resolving the Job's ServiceAccount independently changes which account it runs as, and that changes what it inherits. With
serviceAccounts.backend.create: trueandautomount: falsethe Job used to run as the created backend account, which the chart renders withautomountServiceAccountToken: false, so no token was projected. Falling back todefaultwould have handed it one, which is a regression this PR would have introduced in exactly the hardened configuration it exists to servemigrationJob.automountServiceAccountTokendefaults tofalseand closes it. The Job runsprisma migrate deployagainst Postgres and never calls the Kubernetes API, which is the same reasoning the chart already applies to the ui ServiceAccount. It is a value rather than a literal so IAM database auth has an escape hatch; that path should not need the default token mount, since EKS Pod Identity injects its own projected token volume and GKE Workload Identity goes through the metadata server, and an operator who finds otherwise can flip the key instead of forking the chart5. Gates
The two new suites were also run against the base chart to confirm they are mutation-sensitive: 22 of the 27 new tests fail at
0a42f28850and pass here. The 5 that pass on both sides are deliberate, and they are the ones asserting that nothing renders by default and that a ServiceAccount configuration that works today keeps resolving to the same nameType
🆕 New Feature
🐛 Bug Fix
Changes
Each of gateway, backend, and ui gains
podSecurityContext, container-levelsecurityContext,extraContainers,podLabels,lifecycle, andterminationGracePeriodSeconds. The migrations Job gainsvolumes,volumeMounts,podLabels,podSecurityContext, andsecurityContextEverything defaults to empty and renders nothing, so an existing release re-renders byte for byte. A hardened default was tempting for
securityContext, and rejected: a non-empty default would change the runtime of every deployment that upgrades into it, which is not something a chart should do silently.terminationGracePeriodSecondsfollows the same rule and inherits the Kubernetes default of 30 when unset, rather than the legacy chart's 90; an explicit0is honored instead of being swallowed as an empty value.extraContainersrenders throughtpl, matchinghelm/litellm-helm, so sidecar entries can reference chart values and release metadatapodLabelson a Deployment is validated against that Deployment's selector. The three selector keys are also emitted on the pod template, so reusing one would render a duplicate YAML key, the later value would win, and the apiserver would reject the release for a pod template that no longer matches its own selector. A shared helper fails at template time naming the offending key and component, which follows what the chart already does elsewhere for invalid value combinations. Silently dropping the key was the alternative and it is the same wart as ignoring an operator's ServiceAccount name, so it renders an error instead. The migrations Job stays permissive here on purpose, since a Job's selector is generated by the controller rather than declared; it merges podLabels over the chart labels so an override replaces a key rather than duplicating itThe migrations Job now resolves its ServiceAccount through a helper of its own.
migrationJob.serviceAccountNamealways wins; with it empty the Job falls back todefaultwhenserviceAccounts.backend.createis true, and otherwise keeps borrowing the backend name, which in that branch is either an existing account the operator supplied ordefault. Both of those already exist when the hook runs, so every configuration that works today resolves to exactly the name it resolves to nowResolving the Job's identity independently also changes what that pod inherits, so
migrationJob.automountServiceAccountTokencloses the gap it would otherwise open. WithserviceAccounts.backend.create: trueandautomount: falsethe Job used to run as the created backend account and get no projected token, and falling back todefaultwould have handed it one, in precisely the hardened configuration this PR serves. It defaults tofalse, since the Job talks to Postgres and never calls the Kubernetes API, which is the reasoning the chart already applies to the ui ServiceAccount. It is a value rather than a literal so that IAM database auth keeps an escape hatch, per section 4One upgrade caveat, documented next to the key in values.yaml: a release already running with
serviceAccounts.backend.create: trueused to hand the Job the created backend account on every upgrade, since by then it exists, and now hands itdefault. If that Job relies on IRSA or Workload Identity annotations, for instance underdatabase.writer.useIAMAuth, setmigrationJob.serviceAccountNameto an account that already existsreadOnlyRootFilesystem: trueon the Job is now settable, and running the Job that way also needs the migrations image from #35485, which bakes the prisma CLI and engines offline. On today's published image the Job still writes under$HOMEat startup, so the chart knobs alone do not deliver a read-only Job until that landsTests live in
helm/litellm/tests/pod_hardening_tests.yamlandhelm/litellm/tests/migration_job_tests.yaml, both picked up by the existinghelm unittest -f 'tests/*.yaml' helm/litellmstep in.github/workflows/helm_unit_test.yml. They assert rendered values rather than key presence, and cover every branch of the ServiceAccount helper: name set,createtrue,createfalse with an explicit name, and neitherFinal Attestation