diff --git a/.github/workflows/standard-checks.yml b/.github/workflows/standard-checks.yml index b3a16d63..8e86dcc8 100644 --- a/.github/workflows/standard-checks.yml +++ b/.github/workflows/standard-checks.yml @@ -71,6 +71,11 @@ jobs: runs-on: ubuntu-latest # See the lint job: no job in this file had a timeout before #753. # bats itself runs in ~2 min; the headroom is for the apt install. + # + # The helm install added for #751 does not move this: setup-helm downloads a + # single ~15 MB binary from get.helm.sh and is measured in seconds, and the + # worst case documented on the bats step below (390 s of bounded apt retries + # + ~155 s of bats = ~9 min) leaves ~6 min of headroom inside the 15. timeout-minutes: 15 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -114,5 +119,24 @@ jobs: echo "::error::could not install bats in 3 bounded attempts - runner-to-mirror connectivity, not this PR. Re-run this job." exit 1 + # scripts/tests/chart-pull-secret.bats renders the REAL chart, so this + # required job needs helm or those cases self-skip and a wrong + # IMAGE_PULL_SECRET_NAME or Secret name ships green (Bugbot on #751 -- the + # tests were written, added to the suite, and silently skipped by the only + # job that runs the suite). + # + # Same sha-pinned action and same v3.15.4 pin as helm-ci.yaml and + # drift-checks.yaml, for the reason helm-ci states: helm is absent from the + # runner image, and an unpinned one silently changes which assertions run. + # The bats file's own version gate keys off this pin. + - name: Set up Helm + uses: azure/setup-helm@1a275c3b69536ee54be43f2070a358922e12c8d4 # v4.3.1 + with: + version: v3.15.4 + - name: Run bats (bash unit suite, mocked) + # CI=true is already set by GitHub; chart-pull-secret.bats reads it to + # turn "helm is missing" from a skip into a FAILURE. A skip is the right + # answer on a laptop without helm and the wrong one in a required gate, + # where it is indistinguishable from a pass. run: bats scripts/tests/*.bats diff --git a/client/Chart.yaml b/client/Chart.yaml index f1415c68..34748d1c 100644 --- a/client/Chart.yaml +++ b/client/Chart.yaml @@ -2,8 +2,8 @@ apiVersion: v2 name: client description: A unified Helm chart for tracebloc on AKS, EKS, bare-metal, and OpenShift type: application -version: 1.9.47 -appVersion: "1.9.47" +version: 1.9.48 +appVersion: "1.9.48" keywords: - tracebloc - kubernetes diff --git a/client/templates/_helpers.tpl b/client/templates/_helpers.tpl index 7ce2e381..da3e3b28 100644 --- a/client/templates/_helpers.tpl +++ b/client/templates/_helpers.tpl @@ -137,8 +137,44 @@ mysql-pvc {{ .Values.pvc.mysql | default "2Gi" }} {{- end }} +{{/* + The pull Secret's name. `-regcred` when the chart makes it -- release + scoped, because two releases in one namespace must not share a Secret -- or + the operator's own name when they brought their own. + + Every imagePullSecrets block in the chart and the IMAGE_PULL_SECRET_NAME that + jobs-manager stamps onto training pods both read THIS, so the name cannot + disagree between the pod that creates the Secret and the pod that uses it. +*/}} +{{/* + The name the CHART creates, whatever `existingSecret` says. Not the same + question as `registrySecretName`, and RBAC is where the difference bites + (Bugbot on client#751). + + The auto-upgrade Role in the GPU device-plugin namespace -- `kube-system` by + default -- pins `get`/`update`/`patch`/`delete` to a `resourceNames` list so + Helm can reconcile the mirrored pull Secret. Resolving that list through + `registrySecretName` would hand the auto-upgrade ServiceAccount read AND + delete on the OPERATOR's own dockerconfigjson in kube-system -- a privilege + over a Secret this chart does not own and was never asked to touch. + + It also keeps the migration honest in the other direction: switching a release + from `create: true` to `existingSecret` leaves the previously mirrored + `-regcred` behind, and Helm has to be able to delete it. A list that + followed `existingSecret` would stop naming the orphan, so the delete would + 403 and stall auto-upgrade on every later tick. +*/}} +{{- define "tracebloc.createdRegistrySecretName" -}} +{{ .Release.Name }}-regcred +{{- end }} + {{- define "tracebloc.registrySecretName" -}} +{{- $reg := .Values.dockerRegistry | default dict -}} +{{- if $reg.existingSecret -}} +{{ $reg.existingSecret }} +{{- else -}} {{ .Release.Name }}-regcred +{{- end -}} {{- end }} {{/* @@ -413,9 +449,47 @@ Always {{- end -}} {{- end -}} -{{/* Whether to create registry secret and add imagePullSecrets. Only when dockerRegistry is present and create is true; omit dockerRegistry or set create: false for public images. */}} +{{/* + Whether this release HAS a registry pull Secret -- whoever made it. True when + the chart creates one (`dockerRegistry.create: true`) or when the operator + points at one they made themselves (`dockerRegistry.existingSecret`). Omit + `dockerRegistry` entirely, or set `create: false` with no `existingSecret`, + for public images: nothing renders and every pull is anonymous by declaration. + + `existingSecret` exists because of a config this chart never supported and + never noticed (Asad on client#751): `create: false` plus a Secret hand-made in + the namespace and literally named `regcred`. That worked for exactly one pod + -- the training pod -- because client-runtime's `job.yaml` carried `regcred` + as a hardcoded literal, while every chart-rendered pod got no imagePullSecrets + at all. So those installs were half-authenticated by accident, and the moment + backend#2119 replaces that literal with the injected name they would fall to + an anonymous pull with nothing louder than an INFO log -- the same silent + downgrade this PR exists to remove, moved rather than fixed. + + This is the ONE helper every consumer consults, which is why the + contradiction check lives here: it cannot be bypassed by adding a template. +*/}} {{- define "tracebloc.useImagePullSecrets" -}} -{{- if and .Values.dockerRegistry (default false .Values.dockerRegistry.create) -}} +{{- $reg := .Values.dockerRegistry | default dict -}} +{{- if and (default false $reg.create) $reg.existingSecret -}} +{{- fail (printf "dockerRegistry.create is true AND dockerRegistry.existingSecret is %q. Those contradict: one asks the chart to build the Secret from the credentials in values, the other says one already exists. Pick one -- drop `create` to use your own Secret, or drop `existingSecret` to let the chart build it." $reg.existingSecret) -}} +{{- end -}} +{{- if or (default false $reg.create) $reg.existingSecret -}} +true +{{- end -}} +{{- end }} + +{{/* + Whether the CHART renders the Secret, as opposed to merely referencing one. + Split out from `useImagePullSecrets` because those two questions had the same + answer until `existingSecret` existed and now do not: gating the Secret + template on "a Secret exists" would make the chart overwrite the operator's + hand-made one with credentials from values it does not have -- turning a + supported config into a broken pull on the first upgrade. +*/}} +{{- define "tracebloc.createRegistrySecret" -}} +{{- $reg := .Values.dockerRegistry | default dict -}} +{{- if default false $reg.create -}} true {{- end -}} {{- end }} diff --git a/client/templates/auto-upgrade-rbac.yaml b/client/templates/auto-upgrade-rbac.yaml index 7417808e..6e2f3401 100644 --- a/client/templates/auto-upgrade-rbac.yaml +++ b/client/templates/auto-upgrade-rbac.yaml @@ -317,12 +317,21 @@ rules: - {{ include "tracebloc.gpuDevicePluginName" "amd" | quote }} verbs: ["update", "patch", "delete"] # docker-registry-secret.yaml — the mirrored dockerconfigjson pull Secret. + # + # resourceNames names the secret the CHART creates, never the operator's own + # (`dockerRegistry.existingSecret`). Resolving it through + # `tracebloc.registrySecretName` would grant this ServiceAccount get + DELETE + # on a bring-your-own dockerconfigjson living in kube-system — rights over a + # Secret the chart does not own (Bugbot on client#751). Naming the chart's own + # secret also keeps `create: true` -> `existingSecret` migrations working: the + # orphaned mirror is still deletable, where a list that followed + # `existingSecret` would 403 on it and stall every later upgrade tick. - apiGroups: [""] resources: ["secrets"] verbs: ["create"] - apiGroups: [""] resources: ["secrets"] - resourceNames: [{{ include "tracebloc.registrySecretName" . | quote }}] + resourceNames: [{{ include "tracebloc.createdRegistrySecretName" . | quote }}] verbs: ["get", "update", "patch", "delete"] # This Role and its RoleBinding, so the SA can reconcile its own grant. - apiGroups: ["rbac.authorization.k8s.io"] diff --git a/client/templates/docker-registry-secret.yaml b/client/templates/docker-registry-secret.yaml index 31e458ae..b802b5be 100644 --- a/client/templates/docker-registry-secret.yaml +++ b/client/templates/docker-registry-secret.yaml @@ -1,4 +1,4 @@ -{{- if include "tracebloc.useImagePullSecrets" . }} +{{- if include "tracebloc.createRegistrySecret" . }} apiVersion: v1 kind: Secret metadata: diff --git a/client/templates/jobs-manager-deployment.yaml b/client/templates/jobs-manager-deployment.yaml index 592585e6..5cf76536 100644 --- a/client/templates/jobs-manager-deployment.yaml +++ b/client/templates/jobs-manager-deployment.yaml @@ -415,6 +415,28 @@ spec: # --reset-then-reuse-values upgrades. - name: JOB_IMAGE_HOST value: {{ printf "%s/" (dig "imageRegistry" "docker.io" (.Values.global | default dict) | default "docker.io") | quote }} + {{- if include "tracebloc.useImagePullSecrets" . }} + # IMAGE_PULL_SECRET_NAME is the pull Secret jobs-manager puts on the + # TRAINING pods it spawns (backend#2119). It has to be injected rather + # than written into client-runtime's job.yaml, because the name is + # release-dependent: a literal in that repo cannot be right for every + # release, and the one it carried ("regcred", unqualified) was right for + # none. Every training pod named a Secret the kubelet could not retrieve + # and fell back to an ANONYMOUS pull of the ~2.7 GB training image — so + # on a mirrored install (JOB_IMAGE_HOST above) the training pod was the + # one pod in the release NOT using the credential the operator supplied. + # + # Rendered from the same helper as every imagePullSecrets block in this + # chart, and gated on the same condition that creates the Secret, so the + # three cannot disagree. Absent when no registry is configured: the + # runtime reads that as "pull publicly" and sets no imagePullSecrets at + # all, which is the honest declaration of a public pull. + # + # Skew-safe both ways: a client-runtime older than backend#2119 ignores + # this var, and a newer one treats its absence as "public". + - name: IMAGE_PULL_SECRET_NAME + value: {{ include "tracebloc.registrySecretName" . | quote }} + {{- end }} - name: CLIENT_ENV value: {{ include "tracebloc.clientEnv" . | quote }} # Where the resource-monitor DaemonSet lives, for the heartbeat version @@ -472,7 +494,24 @@ spec: - name: SINGLE_NODE value: {{ if hasKey .Values.env "SINGLE_NODE" }}{{ .Values.env.SINGLE_NODE | quote }}{{ else }}{{ (default dict .Values.hostPath).enabled | default false | quote }}{{ end }} {{- range $key, $value := .Values.env }} - {{- if and (ne $key "CLIENT_ENV") (ne $key "RESOURCE_REQUESTS") (ne $key "RESOURCE_LIMITS") (ne $key "GPU_REQUESTS") (ne $key "GPU_LIMITS") (ne $key "RUNTIME_CLASS_NAME") (ne $key "SINGLE_NODE") (ne $key "CLIENT_ID") (not (has $key $proxyKeys)) $value }} + {{- /* IMAGE_PULL_SECRET_NAME is excluded so `env.IMAGE_PULL_SECRET_NAME` + cannot render it twice with the passthrough copy winning (Asad on + client#751). The consequence is sharper than JOB_IMAGE_HOST's, which + has the same shape and is deliberately left alone: a slow pull is + recoverable, whereas client-runtime treats a pull-secret name that + does not resolve as PERMANENT and dead-letters the experiment. The + supported way to name your own Secret is `dockerRegistry.existingSecret`, + which every consumer reads through one helper; an env override would + be a second reader of the same rule. + + Excluded from pods-monitor's list below too, so a stray value is + inert everywhere rather than only where it would have done harm. + pods-monitor never renders this var and never reads it -- job.yaml + is read by jobs_manager.py, which runs in THIS container -- so + passing it through there put an ignored value in an unrelated + process's environment and nothing more. Nobody can depend on that: + the variable does not exist before this change. */}} + {{- if and (ne $key "CLIENT_ENV") (ne $key "RESOURCE_REQUESTS") (ne $key "RESOURCE_LIMITS") (ne $key "GPU_REQUESTS") (ne $key "GPU_LIMITS") (ne $key "RUNTIME_CLASS_NAME") (ne $key "SINGLE_NODE") (ne $key "CLIENT_ID") (ne $key "IMAGE_PULL_SECRET_NAME") (not (has $key $proxyKeys)) $value }} - name: {{ $key }} value: {{ $value | quote }} {{- end }} @@ -551,7 +590,7 @@ spec: - name: SINGLE_NODE value: {{ if hasKey .Values.env "SINGLE_NODE" }}{{ .Values.env.SINGLE_NODE | quote }}{{ else }}{{ (default dict .Values.hostPath).enabled | default false | quote }}{{ end }} {{- range $key, $value := .Values.env }} - {{- if and (ne $key "CLIENT_ENV") (ne $key "RESOURCE_REQUESTS") (ne $key "RESOURCE_LIMITS") (ne $key "GPU_REQUESTS") (ne $key "GPU_LIMITS") (ne $key "RUNTIME_CLASS_NAME") (ne $key "SINGLE_NODE") (ne $key "CLIENT_ID") (not (has $key $proxyKeys)) $value }} + {{- if and (ne $key "CLIENT_ENV") (ne $key "RESOURCE_REQUESTS") (ne $key "RESOURCE_LIMITS") (ne $key "GPU_REQUESTS") (ne $key "GPU_LIMITS") (ne $key "RUNTIME_CLASS_NAME") (ne $key "SINGLE_NODE") (ne $key "CLIENT_ID") (ne $key "IMAGE_PULL_SECRET_NAME") (not (has $key $proxyKeys)) $value }} - name: {{ $key }} value: {{ $value | quote }} {{- end }} diff --git a/client/tests/image_pull_secret_test.yaml b/client/tests/image_pull_secret_test.yaml new file mode 100644 index 00000000..7c17a3af --- /dev/null +++ b/client/tests/image_pull_secret_test.yaml @@ -0,0 +1,151 @@ +suite: the pull Secret jobs-manager stamps on training pods (backend#2119) +# client-runtime's job.yaml carried `imagePullSecrets: [{name: regcred}]` as a +# LITERAL. This chart has never created a Secret by that name -- it renders +# `-regcred` -- so the reference resolved on NO release: every training +# pod named a Secret the kubelet could not retrieve and fell back to an +# ANONYMOUS pull of the ~2.7GB training image. On a mirrored install the +# training pod was the one pod in the release NOT using the operator's +# credential. +# +# Two invariants, the shape global_image_registry_test.yaml uses (@saadqbal on +# client#751 asked for this coverage; the whole fix is one `{{- if }}`, which is +# exactly the kind of thing that gets deleted by accident): +# 1. a pull Secret exists -> the var is injected AND equals the Secret's own +# name, from the same helper every imagePullSecrets block reads +# 2. no registry configured -> the var is ABSENT, not empty. client-runtime +# reads absence as "pull publicly"; an empty value would make it set a +# pull secret nothing created, i.e. the original defect inverted. +# +# `dockerRegistry.existingSecret` is covered too, because it is the path where +# the name is NOT derivable from the release name -- the case a literal could +# never have got right. +templates: + - templates/jobs-manager-deployment.yaml + - templates/docker-registry-secret.yaml +release: + name: t + namespace: tracebloc +set: + clientId: "test-id" + clientPassword: "test" +tests: + # --------------------------------------------------------------------------- + # 1. A pull Secret exists -> injected, and it names the real Secret + # --------------------------------------------------------------------------- + - it: injects the chart-created Secret's name when the chart creates one + template: templates/jobs-manager-deployment.yaml + set: + dockerRegistry: + create: true + server: https://index.docker.io/v1/ + username: u + password: p + email: e@example.com + asserts: + - contains: + path: spec.template.spec.containers[0].env + content: + name: IMAGE_PULL_SECRET_NAME + value: t-regcred + + - it: names the same Secret the release actually pulls with + # The invariant that was broken: the injected name must be a Secret that + # EXISTS, not a plausible string. Asserted against the pod's own + # imagePullSecrets so the two cannot drift apart. + template: templates/jobs-manager-deployment.yaml + set: + dockerRegistry: + create: true + server: https://index.docker.io/v1/ + username: u + password: p + email: e@example.com + asserts: + - contains: + path: spec.template.spec.imagePullSecrets + content: + name: t-regcred + + - it: renders that Secret in the release namespace + template: templates/docker-registry-secret.yaml + documentIndex: 0 + set: + dockerRegistry: + create: true + server: https://index.docker.io/v1/ + username: u + password: p + email: e@example.com + asserts: + - equal: + path: metadata.name + value: t-regcred + - equal: + path: metadata.namespace + value: tracebloc + - equal: + path: type + value: kubernetes.io/dockerconfigjson + + - it: injects the operator's own name when they bring their own Secret + # The case a literal could never have got right, and the reason the name is + # injected rather than written into client-runtime's job.yaml. + template: templates/jobs-manager-deployment.yaml + set: + dockerRegistry: + existingSecret: regcred + asserts: + - contains: + path: spec.template.spec.containers[0].env + content: + name: IMAGE_PULL_SECRET_NAME + value: regcred + + - it: renders NO Secret when the operator brought their own + # Gating the Secret template on "a pull secret exists" instead of "the chart + # creates it" would have the chart overwrite the operator's Secret using + # credentials it does not have. + template: templates/docker-registry-secret.yaml + set: + dockerRegistry: + existingSecret: regcred + asserts: + - hasDocuments: + count: 0 + + # --------------------------------------------------------------------------- + # 2. No registry configured -> ABSENT, not empty + # --------------------------------------------------------------------------- + - it: omits the variable entirely when no registry is configured + template: templates/jobs-manager-deployment.yaml + asserts: + - notContains: + path: spec.template.spec.containers[0].env + content: + name: IMAGE_PULL_SECRET_NAME + value: t-regcred + - notContains: + path: spec.template.spec.containers[0].env + content: + name: IMAGE_PULL_SECRET_NAME + value: "" + + - it: omits it under create false as well, which is a public pull + template: templates/jobs-manager-deployment.yaml + set: + dockerRegistry: + create: false + asserts: + - notContains: + path: spec.template.spec.containers[0].env + content: + name: IMAGE_PULL_SECRET_NAME + value: t-regcred + + - it: renders no Secret and no imagePullSecrets on a public pull + # Absence has to be REAL: client-runtime reads it as "pull publicly", so a + # dangling reference here is the original defect in the other direction. + template: templates/jobs-manager-deployment.yaml + asserts: + - isNull: + path: spec.template.spec.imagePullSecrets diff --git a/client/values.schema.json b/client/values.schema.json index 09bbbe08..df1b3b71 100644 --- a/client/values.schema.json +++ b/client/values.schema.json @@ -1205,7 +1205,7 @@ "object", "null" ], - "description": "Optional. Omit entirely or set null for public images (no secret or imagePullSecrets). Only create when set and create is true.", + "description": "Optional. Omit entirely or set null for public images (no secret or imagePullSecrets). Either create: true with the credentials below, or existingSecret naming a Secret you made yourself -- never both.", "properties": { "create": { "type": "boolean", @@ -1224,9 +1224,24 @@ }, "email": { "type": "string" + }, + "existingSecret": { + "type": "string", + "minLength": 1, + "description": "Name of a kubernetes.io/dockerconfigjson Secret you created yourself. Mutually exclusive with create: the chart references this Secret and never writes it, because it has no credentials to write it with. You must create it in every namespace that pulls -- the release namespace, plus nodeAgents.namespace.name while resourceMonitor is true, plus gpu.devicePlugin.namespace while that is enabled -- since the chart cannot mirror a Secret whose contents it does not have." } }, "allOf": [ + { + "if": { + "required": ["existingSecret"] + }, + "then": { + "properties": { + "create": { "const": false } + } + } + }, { "if": { "properties": { diff --git a/client/values.yaml b/client/values.yaml index 91123940..4fa55433 100644 --- a/client/values.yaml +++ b/client/values.yaml @@ -817,15 +817,50 @@ podTokenSigningSecret: "" # expires. Default 7 days. podTokenTtlSeconds: 604800 -# -- Docker registry credentials (optional; only used when dockerRegistry is set and create is true) -# Omit dockerRegistry entirely, or set create: false, for public images (no imagePullSecrets). -# When create is true, secret name is {{ .Release.Name }}-regcred. +# -- Docker registry credentials (optional). Two supported shapes, and they are +# mutually exclusive -- setting both fails the render rather than picking one. +# +# 1. LET THE CHART BUILD IT: `create: true` plus the credentials below. The +# Secret is named `{{ .Release.Name }}-regcred` (release-scoped, so two +# releases in one namespace cannot share it) and is mirrored automatically +# into the node-agents and GPU device-plugin namespaces when those are +# enabled and differ from the release namespace. +# +# 2. BRING YOUR OWN: `existingSecret: ` naming a `kubernetes.io/ +# dockerconfigjson` Secret you created yourself. Use this when registry +# credentials must not live in values. The chart references it and never +# writes it -- it has no credentials to write it with. +# +# YOU must create that Secret in every namespace that pulls: the release +# namespace, plus `nodeAgents.namespace.name` (default +# "tracebloc-node-agents") while `resourceMonitor` is true, plus +# `gpu.devicePlugin.namespace` (default "kube-system") while the device +# plugin is enabled. The chart cannot mirror a Secret whose contents it does +# not have. If one is missing, those pods report ImagePullBackOff -- loud and +# diagnosable, unlike the failure this option exists to prevent. +# +# Omit `dockerRegistry` entirely, or set `create: false` with no +# `existingSecret`, for public images: no imagePullSecrets are rendered +# anywhere, and jobs-manager stamps none onto training pods either. That is the +# honest declaration of an anonymous pull. +# +# `existingSecret` was added for a config that used to half-work by accident +# (client#751): `create: false` plus a hand-made Secret literally named +# `regcred`. Chart-rendered pods pulled anonymously while TRAINING pods +# authenticated, because client-runtime's job.yaml carried `regcred` as a +# hardcoded literal. backend#2119 replaces that literal with the name below, so +# those installs need to say what they are using. Set `existingSecret: regcred` +# to keep working, and note that it now covers your control-plane pods too. +# # dockerRegistry: # create: true # server: https://index.docker.io/v1/ # username: "" # password: "" # email: "" +# +# dockerRegistry: +# existingSecret: regcred # -- Self-upgrade CronJob (closes tracebloc/client#69). # Runs `helm upgrade --reuse-values` against this release on a schedule, so diff --git a/scripts/tests/chart-pull-secret.bats b/scripts/tests/chart-pull-secret.bats new file mode 100644 index 00000000..a149b37f --- /dev/null +++ b/scripts/tests/chart-pull-secret.bats @@ -0,0 +1,375 @@ +#!/usr/bin/env bats +# The pull Secret every pod in the release pulls with, and the name jobs-manager +# stamps onto the TRAINING pods it spawns (backend#2119). +# +# WHY THESE EXIST +# client-runtime's job.yaml carried `imagePullSecrets: [{name: regcred}]` as a +# literal. This chart has never created a Secret by that name — it renders +# `-regcred` — so the reference resolved on NO release: every training +# pod named a Secret the kubelet could not retrieve and fell back to an +# ANONYMOUS pull of the ~2.7GB training image. That defect survived because the +# chart had no rendering test for the pull-secret paths at all; the first version +# of the fix was verified by rendering it once by hand (client#751). +# +# These render the REAL chart with helm. A stubbed render would test the stub, +# and the whole question here is what helm emits for five different value +# shapes — including the two that must emit NOTHING, which is where a +# hand-check is least reliable. + +CHART="" +VALUES="" +NS="tracebloc" + +# A missing tool is a SKIP on a laptop and a FAILURE in CI. +# +# The first version of this file skipped unconditionally, and the only job that +# runs `scripts/tests/*.bats` — the required `Unit tests` job — had no helm. So +# all fourteen cases were silent skips: a wrong IMAGE_PULL_SECRET_NAME or Secret +# name would have shipped green past tests written to catch exactly that (Bugbot +# on client#751). In a required gate a skip is indistinguishable from a pass, +# which is the whole inert-verification class this repo keeps finding. +# +# `CI` is set to `true` by GitHub Actions on every runner. +require_tool() { + command -v "$1" >/dev/null && return 0 + if [ "${CI:-}" = "true" ]; then + echo "::error::$1 is missing in CI, so these chart-render assertions would" >&2 + echo "::error::be skipped rather than run. Install it in the job (see" >&2 + echo "::error::standard-checks.yml 'Set up Helm') instead of accepting a green skip." >&2 + return 1 + fi + skip "$1 not installed (local run)" +} + +setup() { + # Set here rather than in setup_file: exports from setup_file do not reach the + # test body in every bats version, and an empty $CHART makes helm fail with + # "non-absolute URLs" — eleven confusing reds instead of one clear skip. + CHART="${BATS_TEST_DIRNAME}/../../client" + VALUES="${CHART}/ci/bm-values.yaml" + require_tool helm || return 1 + require_tool python3 || return 1 + [ -d "$CHART" ] || { + echo "chart directory not found at $CHART" >&2 + return 1 + } + [ -f "$VALUES" ] || { + echo "CI values file not found at $VALUES" >&2 + return 1 + } +} + +# Render, or fail the test with helm's own message rather than an empty file. +render() { + run helm template myrel "$CHART" -f "$VALUES" --namespace "$NS" "$@" + [ "$status" -eq 0 ] || { + echo "helm template failed: $output" >&2 + return 1 + } + printf '%s\n' "$output" +} + +# IMAGE_PULL_SECRET_NAME values in one container of the jobs-manager Deployment. +# Parsed as YAML, not grepped: the two containers each have their own env list, +# and a grep cannot tell which one it is looking at — the exact confusion that +# made the first count of this variable read 2 when the api container had 1. +env_values_in() { + python3 -c ' +import sys, yaml +want_container, want_key = sys.argv[1], sys.argv[2] +for doc in yaml.safe_load_all(sys.stdin.read()): + if not doc or doc.get("kind") != "Deployment": + continue + if "jobs-manager" not in doc["metadata"]["name"]: + continue + for c in doc["spec"]["template"]["spec"]["containers"]: + if c["name"] != want_container: + continue + for e in (c.get("env") or []): + if e["name"] == want_key: + print(e.get("value")) +' "$1" "$2" +} + +dockerconfig_secrets() { + python3 -c ' +import sys, yaml +for doc in yaml.safe_load_all(sys.stdin.read()): + if doc and doc.get("type") == "kubernetes.io/dockerconfigjson": + print(doc["metadata"]["name"], doc["metadata"]["namespace"]) +' +} + +pull_secret_refs() { + python3 -c ' +import sys, yaml +names = set() +def walk(node): + if isinstance(node, dict): + for k, v in node.items(): + if k == "imagePullSecrets" and isinstance(v, list): + names.update(x.get("name") for x in v if isinstance(x, dict)) + walk(v) + elif isinstance(node, list): + for v in node: + walk(v) +for doc in yaml.safe_load_all(sys.stdin.read()): + walk(doc) +for n in sorted(x for x in names if x): + print(n) +' +} + +created() { + render --set dockerRegistry.create=true \ + --set dockerRegistry.server=https://index.docker.io/v1/ \ + --set dockerRegistry.username=u \ + --set dockerRegistry.password=p \ + --set dockerRegistry.email=e@x.io "$@" +} + +# ---- the chart builds the Secret ------------------------------------------- + +@test "create: the injected name is the Secret the chart actually renders" { + out="$(created)" || return 1 + # The property that was broken: the name jobs-manager stamps on training pods + # must be a Secret that EXISTS in this render, not a plausible string. + [ "$(printf '%s\n' "$out" | env_values_in api IMAGE_PULL_SECRET_NAME)" = "myrel-regcred" ] || return 1 + printf '%s\n' "$out" | dockerconfig_secrets | grep -qx "myrel-regcred ${NS}" || return 1 +} + +@test "create: every imagePullSecrets block names that same Secret" { + out="$(created)" || return 1 + refs="$(printf '%s\n' "$out" | pull_secret_refs)" + [ -n "$refs" ] || return 1 + # One distinct name across the whole release. A second name here is the + # original defect in a new place: a pod referring to a Secret nobody made. + [ "$(printf '%s\n' "$refs" | sort -u | wc -l | tr -d ' ')" = "1" ] || return 1 + [ "$(printf '%s\n' "$refs" | sort -u)" = "myrel-regcred" ] || return 1 +} + +# ---- the operator brings their own ----------------------------------------- + +@test "existingSecret: the name is the operator's, everywhere" { + out="$(render --set dockerRegistry.existingSecret=regcred)" || return 1 + [ "$(printf '%s\n' "$out" | env_values_in api IMAGE_PULL_SECRET_NAME)" = "regcred" ] || return 1 + [ "$(printf '%s\n' "$out" | pull_secret_refs | sort -u)" = "regcred" ] || return 1 +} + +@test "existingSecret: the chart renders NO Secret, so it cannot overwrite it" { + # The trap this split exists for. Gating the Secret template on "a pull + # secret exists" instead of "the chart creates it" would have the chart write + # the operator's Secret from credentials it does not have — turning a + # supported config into a broken pull on the first upgrade. + out="$(render --set dockerRegistry.existingSecret=regcred)" || return 1 + [ -z "$(printf '%s\n' "$out" | dockerconfig_secrets)" ] || return 1 +} + +# ---- public: the absence has to be real ------------------------------------ + +@test "public: no name is injected and no imagePullSecrets are rendered" { + # Absence is the declaration of an anonymous pull, and client-runtime reads + # it that way. If the variable appeared here with an empty or stale value the + # runtime would set a pull secret nothing created — the defect, inverted. + out="$(render)" || return 1 + [ -z "$(printf '%s\n' "$out" | env_values_in api IMAGE_PULL_SECRET_NAME)" ] || return 1 + [ -z "$(printf '%s\n' "$out" | pull_secret_refs)" ] || return 1 + [ -z "$(printf '%s\n' "$out" | dockerconfig_secrets)" ] || return 1 +} + +@test "create: false with no existingSecret is public, not half-configured" { + out="$(render --set dockerRegistry.create=false)" || return 1 + [ -z "$(printf '%s\n' "$out" | env_values_in api IMAGE_PULL_SECRET_NAME)" ] || return 1 + [ -z "$(printf '%s\n' "$out" | pull_secret_refs)" ] || return 1 +} + +# ---- the contradiction is refused at BOTH layers --------------------------- + +@test "create plus existingSecret is refused by the values schema" { + run helm template myrel "$CHART" -f "$VALUES" --namespace "$NS" \ + --set dockerRegistry.create=true \ + --set dockerRegistry.server=https://index.docker.io/v1/ \ + --set dockerRegistry.username=u --set dockerRegistry.password=p \ + --set dockerRegistry.email=e@x.io \ + --set dockerRegistry.existingSecret=regcred + [ "$status" -ne 0 ] || return 1 + # Anchored on text that is IDENTICAL across helm generations, because the + # per-rule wording is not. Measured on both, for the same values: + # + # 3.15.4 (the CI pin) + # - dockerRegistry.create: dockerRegistry.create does not match: false + # 4.1.1 (a current local build) + # - at '/dockerRegistry/create': value must be false + # + # The first version of this asserted `*"must be false"*` and went red in the + # required Unit tests job the moment helm was installed there — a + # version-specific string in an assertion, which is the same defect class as + # a hand-copied constant (Bugbot on client#751, whose conclusion was right; + # its stated cause, case-sensitivity on "Must be false", is not what 3.15.4 + # actually prints). + # + # The header line proves the SCHEMA layer refused rather than the template, + # which is the distinction this test exists to make; `create` and `false` + # together pin WHICH rule fired, so an unrelated schema failure — a missing + # required value, say — cannot satisfy it. + [[ "$output" == *"meet the specifications of the schema"* ]] || return 1 + printf '%s\n' "$output" | grep -qi "create" || return 1 + printf '%s\n' "$output" | grep -qi "false" || return 1 +} + +@test "create plus existingSecret is refused by the template when the schema is skipped" { + # --skip-schema-validation exists, so the schema is a declaration and not a + # gate. The template refusal is the gate, and it says which two values + # collide rather than failing somewhere downstream on a name. + # + # The flag landed in helm 3.16 and CI pins v3.15.4, so this case self-skips + # there — the same treatment and the same reason as the helper-backstop cases + # in chart-env-vocabulary.sh. It runs for anyone local on 3.16+, and bumping + # the CI pin turns it on with no change here. The SCHEMA half of this pair + # (the test above) runs on every version, so the contradiction is never + # unguarded; this case only proves the second layer. + helm template --help 2>&1 | grep -q -- "--skip-schema-validation" || { + skip "helm $(helm version --short 2>/dev/null) predates --skip-schema-validation (3.16)" + } + run helm template myrel "$CHART" -f "$VALUES" --namespace "$NS" \ + --skip-schema-validation \ + --set dockerRegistry.create=true \ + --set dockerRegistry.server=https://index.docker.io/v1/ \ + --set dockerRegistry.username=u --set dockerRegistry.password=p \ + --set dockerRegistry.email=e@x.io \ + --set dockerRegistry.existingSecret=regcred + [ "$status" -ne 0 ] || return 1 + [[ "$output" == *"contradict"* ]] || return 1 +} + +# ---- the passthrough cannot shadow a computed value ------------------------ + +@test "env.IMAGE_PULL_SECRET_NAME cannot shadow the computed name" { + # `.Values.env` is copied into both containers wholesale, so a computed var + # that is not excluded renders TWICE and the passthrough copy wins. For most + # vars that is a slow pull; for this one client-runtime treats a name that + # does not resolve as permanent and dead-letters the experiment, so a + # shadowed value costs the run (Asad on client#751). + out="$(render --set dockerRegistry.existingSecret=regcred \ + --set env.IMAGE_PULL_SECRET_NAME=shadowed)" || return 1 + [ "$(printf '%s\n' "$out" | env_values_in api IMAGE_PULL_SECRET_NAME)" = "regcred" ] || return 1 + # And inert in the container that never reads it, rather than sitting there. + [ -z "$(printf '%s\n' "$out" | env_values_in pods-monitor-container IMAGE_PULL_SECRET_NAME)" ] || return 1 +} + +@test "no container renders any env key twice" { + # The general form of the finding above, so the next computed var added + # without an exclusion is caught here rather than in a review. Asserted + # across every workload in the chart, not just jobs-manager. + out="$(created --set env.IMAGE_PULL_SECRET_NAME=shadowed \ + --set env.SOMETHING_HARMLESS=ok)" || return 1 + dupes="$(printf '%s\n' "$out" | python3 -c ' +import sys, yaml, collections +bad = [] +for doc in yaml.safe_load_all(sys.stdin.read()): + if not doc or not isinstance(doc, dict): + continue + kind = doc.get("kind") + if kind not in ("Deployment", "StatefulSet", "DaemonSet", "Job", "CronJob", "Pod"): + continue + spec = doc.get("spec") or {} + pod = (((spec.get("jobTemplate") or {}).get("spec") or {}).get("template") + or spec.get("template") or {}) + podspec = (pod.get("spec") if pod else None) or (spec if kind == "Pod" else {}) + for group in ("initContainers", "containers"): + for c in (podspec.get(group) or []): + names = [e["name"] for e in (c.get("env") or [])] + for name, n in collections.Counter(names).items(): + if n > 1: + where = kind + "/" + doc["metadata"]["name"] + ":" + c["name"] + bad.append(where + ":" + name + " x" + str(n)) +print("\n".join(sorted(bad))) +')" + if [ -n "$dupes" ]; then + echo "env keys rendered more than once in one container:" >&2 + echo "$dupes" >&2 + return 1 + fi +} + +@test "the duplicate-env check can actually see a duplicate" { + # Otherwise the test above passes on a parser that found no containers at + # all, which is indistinguishable from a clean render (BUGBOT.md: an inert + # check and real coverage look identical in a log). JOB_IMAGE_HOST has the + # same unexcluded shape and is deliberately left that way — a slow pull is + # recoverable — so it is the honest live proof that the parser reaches real + # env lists. + out="$(created --set env.JOB_IMAGE_HOST=shadowed)" || return 1 + count="$(printf '%s\n' "$out" | env_values_in api JOB_IMAGE_HOST | wc -l | tr -d ' ')" + [ "$count" = "2" ] || { + echo "expected JOB_IMAGE_HOST to render twice (it has no exclusion); got $count." >&2 + echo "If it was excluded on purpose, replace this canary with another one —" >&2 + echo "without a live duplicate, the check above proves nothing." >&2 + return 1 + } +} + +# ---- RBAC must never name the operator's Secret ----------------------------- + +rbac_secret_names() { + # resourceNames on every secrets rule, with the Role's namespace. + python3 -c ' +import sys, yaml +for doc in yaml.safe_load_all(sys.stdin.read()): + if not doc or doc.get("kind") not in ("Role", "ClusterRole"): + continue + ns = (doc.get("metadata") or {}).get("namespace", "-") + for rule in (doc.get("rules") or []): + if "secrets" not in (rule.get("resources") or []): + continue + for name in (rule.get("resourceNames") or []): + print(ns, name, ",".join(sorted(rule.get("verbs") or []))) +' +} + +@test "existingSecret: no Role grants rights on the operator's own Secret" { + # The auto-upgrade Role in the GPU device-plugin namespace (kube-system by + # default) pins get/update/patch/DELETE to a resourceNames list. Resolving it + # through the release-wide name helper handed that ServiceAccount delete on a + # bring-your-own dockerconfigjson the chart does not own (Bugbot, client#751). + out="$(render --set dockerRegistry.existingSecret=regcred \ + --set gpu.devicePlugin.enabled=true \ + --set gpu.devicePlugin.vendor=nvidia)" || return 1 + names="$(printf '%s\n' "$out" | rbac_secret_names)" || return 1 + [ -n "$names" ] || { + echo "no secrets resourceNames found at all — the parser is not reaching" >&2 + echo "the Roles, so this test would pass on any chart" >&2 + return 1 + } + if printf '%s\n' "$names" | awk '{print $2}' | grep -qx "regcred"; then + echo "a Role names the operator's Secret:" >&2 + printf '%s\n' "$names" >&2 + return 1 + fi +} + +@test "existingSecret: the chart's own former mirror stays deletable" { + # The other direction of the same rule. Switching a release from create: true + # to existingSecret orphans the mirrored -regcred, and Helm must be + # able to delete it; a resourceNames list that followed existingSecret would + # 403 on the orphan and stall every later auto-upgrade tick. + out="$(render --set dockerRegistry.existingSecret=regcred \ + --set gpu.devicePlugin.enabled=true \ + --set gpu.devicePlugin.vendor=nvidia)" || return 1 + printf '%s\n' "$out" | rbac_secret_names \ + | grep -E "^\S+ myrel-regcred " | grep -q "delete" || { + echo "no rule can delete myrel-regcred, so a create->existingSecret" >&2 + echo "migration leaves an orphan nothing is allowed to remove:" >&2 + printf '%s\n' "$out" | rbac_secret_names >&2 + return 1 + } +} + +@test "create: the RBAC name still matches the Secret that is rendered" { + # Splitting the two names must not desynchronise the ordinary path: the name + # RBAC pins and the name the Secret carries have to be the same string. + out="$(created --set gpu.devicePlugin.enabled=true \ + --set gpu.devicePlugin.vendor=nvidia)" || return 1 + printf '%s\n' "$out" | dockerconfig_secrets | grep -q "^myrel-regcred " || return 1 + printf '%s\n' "$out" | rbac_secret_names | grep -qE "^\S+ myrel-regcred " || return 1 +}