Fix the shell-less cron-suspend image and five audit findings - #54
Open
henzigo wants to merge 2 commits into
Open
Fix the shell-less cron-suspend image and five audit findings#54henzigo wants to merge 2 commits into
henzigo wants to merge 2 commits into
Conversation
The cron-suspend hook ran `/bin/sh -c ...` on rancher/kubectl:v1.33.13, an image that ships only /bin/kubectl. The container could never start, and with backoffLimit: 0 the Job failed immediately — so every pre-upgrade with cron.enabled (i.e. every continuous deploy) died at hook weight 0. The script's `2>/dev/null || true` tolerance genuinely needs a shell, so the fix is the image, not the script: alpine/k8s:1.33.13 keeps the same pinned kubectl version and adds /bin/sh (verified with `docker run --entrypoint /bin/sh`). Also in this commit: - app.s3Endpoint is now enforced by the default nginx vhost. Empty renders passed `nginx -t` (the proxy_pass URL contains variables, so it is validated per request) but every /content/* request failed with "invalid URL prefix". The guard lives in files/nginx/project-nginx.conf only, so projects with their own webserver.nginx.projectConfig are unaffected; the schema is untouched for the same reason. - shopsys-app kubeVersion >=1.27.0-0: hpa-webserver.yaml uses the ContainerResource metric type, enabled by default only since 1.27 (GA 1.30). - Documented the infra pull-secret ordering: the dockerregistry Secret comes from the app release, which installs after shopsys-infra — private infra images need registry.existingSecret pre-created out of band. - Dropped `ingress.kubernetes.io/ssl-redirect: "true"` from all three ingresses. Without the `nginx.` prefix ingress-nginx never read it; the https redirect comes from the configuration-snippet and the controller's TLS default. - Added the missing `app: storefront` metadata label to the storefront Deployment (pod template and selector already had it, untouched). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alpine/k8s ships a full toolchain (~1.2 GB). line/kubectl-kustomize is actively maintained, keeps all historical tags (unlike alpine/kubectl, which prunes pinned tags after ~a year) and is ~100 MB. Verified with docker: /bin/sh present, kubectl v1.34.3, shell tolerance works. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Six audit fixes. The headline is a hard blocker: every continuous deploy currently fails
at the first pre-upgrade hook.
Changes
1.
cron-suspendhook: the kubectl image has no shell (critical)rancher/kubectl:v1.33.13contains only/bin/kubectl— no/bin/sh, entrypoint/bin/kubectl. The hook runscommand: [/bin/sh, -c, ...], so the container can neverstart; with
backoffLimit: 0the Job fails immediately and everypre-upgradewithcron.enabled(i.e. every continuous deploy) dies at hook weight 0.The shell is genuinely required — the script relies on
2>/dev/null || truetolerance(the cron deployment may not exist after a failed first install, and a
kubectl waittimeout must not fail the deploy). Splitting it into shell-less
kubectlinvocationscannot preserve that, so the image was replaced, not the script:
Verified locally with docker:
Image choice (candidates compared by size, maintenance and tag retention):
line/kubectl-kustomize(LINE Corp) is actively maintained (last release 2026-08), is~100 MB uncompressed and has never pruned a tag (history back to 1.16/2020), so a pinned
tag cannot disappear. The bundled kustomize is unused but harmless; the tag encodes
<kubectl>-<kustomize>versions (kubectl 1.33 was skipped upstream, 1.34.3 is the closestto the previous pin). Rejected:
alpine/kubectl(95 MB but keeps only ~15 tags — pinnedtags vanish after about a year),
alpine/k8s(never prunes but ~1.2 GB), officialregistry.k8s.io/kubectl(distroless, no shell),bitnami/kubectl(moved to theunmaintained
bitnamilegacydistribution, now publishes onlylatest),d3fk/kubectl(versioned tags stale since 2024). A new unittest case pins
command[0] == /bin/shandthe image so this cannot regress silently.
2.
app.s3Endpointis now enforced by the default nginx vhost (high)With the default empty value the vhost renders
proxy_pass /<project>-<env>/web....nginx -tpasses (the URL contains variables, so it is only validated per request) butevery
/content/*request fails at runtime with "invalid URL prefix" → HTTP 500.A
failguard was added at the top ofcharts/shopsys-app/files/nginx/project-nginx.conf(rendered through
tpl). It is scoped to that file, so projects supplyingwebserver.nginx.projectConfigare not affected — which is also whyvalues.schema.jsonwas deliberately left alone.environments/base.yamlandcharts/shopsys-app/tests/values/required.yaml(the lint/unittest baseline) now set aplaceholder endpoint.
3.
kubeVersionvs the HPAContainerResourcemetric (low)hpa-webserver.yamluses metric typeContainerResource, enabled by default only sinceKubernetes 1.27 (GA 1.30), while the chart declared
>=1.23.0-0. Bumped shopsys-app to>=1.27.0-0; shopsys-infra is unchanged.4. Infra pull-secret chicken-and-egg (docs only, low)
The
dockerregistrySecret is created by the shopsys-app release, but shopsys-infrainstalls before it with
wait: true— a private redis/rabbitmq image would hang the veryfirst deploy in
ImagePullBackOff. Documented incharts/shopsys-infra/values.yamlanddocs/values.md, pointing at a pre-createdregistry.existingSecret. No template changes.5. Removed the no-op
ingress.kubernetes.io/ssl-redirect: "true"(low)It lacks the
nginx.prefix, so ingress-nginx ignored it entirely — the https redirect isactually done by the
configuration-snippetand by the controller's default ssl-redirectfor TLS ingresses. Removed from
ingress-domains.yaml,ingress-mcp.yamlandingress-rabbitmq.yaml(51 lines across the snapshots), with anotExistsregressionassertion. As a deliberate departure from the 1:1 port it is recorded as deviation 21
in
docs/migrating-from-shopsys-deployment.md.6. Storefront Deployment metadata label (low)
deployment-storefront.yamlwas the only Deployment missingapp: storefrontinmetadata.labels. Added under theshopsys.labelsinclude, matching the otherdeployments. The pod template and the (immutable) selector were not touched.
Notes
Golden snapshots regenerated in the same commit. The diff is exactly the three expected
kinds of change and nothing else:
Verification suite, all green:
helm unittest(both charts)notExists)./tests/run-golden-tests.shhelm lint(infra + app)kubeconform -strict(k8s 1.31.0)shellcheck(4 scripts)helmfile -e devel templateNegative tests for the s3Endpoint guard:
Out of scope by design (a separate PR covers the probes): ingress snippet
annotations / annotations-risk-level, hook and cron timeout tuning, http-auth htpasswd
determinism,
security.mcpdefaults, RabbitMQ probes.🤖 Generated with Claude Code