Repository navigation
Conversation
d207c4f to
512ef39
Compare
|
Both failures here are the ECR-public registry, not this change — could someone re-run when it settles?
I checked that it isn't mine before writing this off, since redis is the pod that failed:
Nothing to change on my side as far as I can tell. |
512ef39 to
4d82ddf
Compare
|
Rebased onto current The one red check, It retries three times and gives up. It's currently red on 4 of the 8 open PRs in this repo, so it looks like the ECR-public pull limit rather than anything in these changes. Re-verified the behaviour on the rebased tree rather than assuming it survived Seven, not eight, because Worth stating explicitly for review, since a pod-level Default |
| securityContext: {} | ||
| # @default -- See [values.yaml] | ||
| securityContext: | ||
| runAsNonRoot: true |
There was a problem hiding this comment.
This stops any pod with a root init container from starting. The chart's own initContainers example for the controller and server uses alpine:3, which has no USER, and upstream's custom tooling page puts the same kind of alpine init container on the repo-server. Neither sets a container securityContext, so with runAsNonRoot: true on the pod the kubelet refuses to start them, and a repo-server that ran before won't after the upgrade.
Every container the chart ships already sets runAsNonRoot itself, so at pod level it only reaches containers people add. Could the pod level keep just seccompProfile? I ran trivy with only that and KSV-0118 still goes from 7 to 0. If you'd rather keep runAsNonRoot, it's a breaking change, so it needs a major and a note under Changelog in README.md.gotmpl, like 10.0.0 has for global.networkPolicy.create (versioning).
There was a problem hiding this comment.
Thanks @tico24, Ill split it for now and push the breaking change separately.
re: trivy, maybe worth to add it as a CI check?
4d82ddf to
724b20e
Compare
|
Rebased again — Re-verified rather than assuming it survived: the full Seven and not eight because This is the third rebase on version collisions alone. Is there any appetite for a review pass while it is green? Happy to keep rebasing, but the chart releases roughly weekly and the |
|
runs-on: ubuntu-latest
...
uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0
with:
python-version: 3.9
Happy to open a separate one-liner pinning it to something available ( |
tico24
left a comment
There was a problem hiding this comment.
My review from Monday is still open: the inline on global.securityContext and the two asks in its body.
Yes please to the python pin as a separate PR.
Every component already ships a hardened containerSecurityContext -- runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false, all capabilities dropped and a RuntimeDefault seccomp profile -- but global.securityContext and redisSecretInit.securityContext were both empty, so the pod templates themselves carried no security context. Default both to runAsNonRoot: true and a RuntimeDefault seccomp profile. These are the same values upstream argo-cd sets on its own notifications-controller pod in manifests/install.yaml, so this brings the chart to parity rather than inventing a policy. runAsUser is deliberately not set: upstream only pins it for redis, which redis.securityContext in this chart already matches. This covers the applicationset, dex, notifications, repo-server, server and commit-server Deployments, the application-controller StatefulSet and the redis-secret-init Job. Signed-off-by: Ilia Lazebnik <ilia.lazebnik@gmail.com>
argo-helm now bumps chart versions after merge (argoproj#4176). This puts the version and artifacthub.io/changes in Chart.yaml back to main's, and moves this PR's changelog entry into charts/argo-cd/changes/set-a-pod-level-security-context-by-default.yaml with the same bump level. Signed-off-by: Tim Collins <tim@thecollins.team>
Keep only the RuntimeDefault seccomp profile at pod level. A pod-level runAsNonRoot reaches containers the chart does not ship, including the initContainers example in values.yaml and the alpine init container on the upstream custom tooling page, neither of which sets a container security context. Those pods would stop starting after an upgrade, which makes it a breaking change rather than a default worth shipping in a minor. The seccomp profile alone keeps the benefit: trivy's KSV-0118 goes from 7 findings to 0 either way, and KSV-0012 stays at 0 because every container the chart ships already sets runAsNonRoot itself. runAsNonRoot will follow in its own pull request with a major bump and an Upgrading note. Signed-off-by: Ilia Lazebnik <ilia.lazebnik@gmail.com>
bfdf78b to
01332e9
Compare
|
All three addressed, and thanks for the push to change fragments — that removes the version collision I'd been rebasing on. 1. I reproduced your trivy measurement before dropping it, on the rendered output:
So seccomp alone keeps the whole benefit here, exactly as you said. KSV-0012 stays at zero precisely because the shipped containers set 2. Checklist and version reference. Both fixed — the template checklist is back in the description and ticked, and the stale 3. The python pin — no PR needed. #4175 already did it: On adding trivy as a CI check — I think that's worth doing and I'm happy to take it, but as its own PR against the workflows rather than bundled here. Worth deciding the shape first: the obvious version is Verification notes for this push: README needs no regeneration — confirmed with the pinned |
tico24
left a comment
There was a problem hiding this comment.
The red chart-test is the kind preload failure that #4178 fixes, so this needs a rebase once that's in.
While you're there, could you fix two things in the description? runAsUser: 0 through global.securityContext doesn't override the whole block, because helm merges maps: with --set global.securityContext.runAsUser=0 the pod gets runAsUser: 0 plus seccompProfile, and global.securityContext: {} leaves seccompProfile in place. Only null clears it. The python lines are out of date too, since #4175 did the pin.
Pull request was converted to draft
Every Argo CD component already ships a hardened
containerSecurityContext—runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false, all capabilities dropped and aRuntimeDefaultseccomp profile — butglobal.securityContextandredisSecretInit.securityContextwere both empty, so the pod templates themselves carried no security context at all.This sets a
RuntimeDefaultseccomp profile at pod level for both.What it renders
Seven pod templates gain a pod-level context, and nothing else changes.
argocd-redisis untouched because it already ships its own.Why
runAsNonRootis no longer hereIt was in the first version of this PR. @tico24 pointed out that a pod-level
runAsNonRootreaches containers the chart does not ship: theinitContainersexample invalues.yamland thealpineinit container on upstream's custom tooling page both run as root and set no container security context, so the kubelet would refuse to start a repo-server that worked before the upgrade. That is a breaking change, not a default to slip into a minor.He was right, and my earlier reasoning here was incomplete — I had only checked the containers the chart itself ships, all of which do set
runAsNonRoot.Measured what dropping it costs, with trivy on the rendered output:
So nothing. KSV-0012 stays at zero because every shipped container already sets
runAsNonRootitself.runAsNonRootwill follow as its own pull request with a major bump and an Upgrading note, per versioning.Notes
runAsUseris deliberately left unset so each image's own UID still applies. Anyone settingrunAsUser: 0throughglobal.securityContextoverrides the whole block as before.Chart.yamlis untouched: the version andartifacthub.io/changesare left to the post-merge publish workflow from feat(github): release chart versions from change fragments #4176, and the entry lives incharts/argo-cd/changes/. Thanks for wiring that up — it also removes the version collision that had me rebasing this three times.helm-docs v1.9.1fromscripts/helm-docs.shthat the README needs no regeneration, and checked the change fragment against every rule inscripts/lint-changelog.shby hand, sincemapfileis unavailable on macOS bash.chart-testis coming as a separate PR as requested.Checklist:
bumpaccording to versioningchart-testfails on the Ubuntu 26.04 / python 3.9 issue, unrelated to this change; separate PR to follow.