fix(chart): inject the release-scoped pull-secret name for training pods (backend#2119)

.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

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

client/templates/_helpers.tpl

@@ -137,8 +137,44 @@ mysql-pvc
 {{ .Values.pvc.mysql | default "2Gi" }}
 {{- end }}
 
+{{/*
+  The pull Secret's name. `<release>-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
+  `<release>-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 }}

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"]

client/templates/docker-registry-secret.yaml

@@ -1,4 +1,4 @@
-{{- if include "tracebloc.useImagePullSecrets" . }}
+{{- if include "tracebloc.createRegistrySecret" . }}
 apiVersion: v1
 kind: Secret
 metadata:

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 }}

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
+# `<release>-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