From 90995f3a4cbdb1adff2d706491408b18551a7255 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Sun, 30 Aug 2026 12:27:54 +0200 Subject: [PATCH 01/22] feat(chart): fullnameOverride, with the completeness guard that makes it safe (backend#2626) backend#2621 built this helper, proved the default render byte-identical, and REVERTED IT. The helper is the easy half. The release name appears ~174 times across these templates and is at least six different things, only two of which may follow an override -- and a partial routing produces `prod-auto-upgrade` beside `myrel-jobs-manager`, which is harder to reason about than a release that is merely badly named. So the guard is the deliverable and the helper rides along. WHAT MOVED (39 sites, 15 files): names of resources this chart creates, and two things that REFERENCE those names -- * `image-refresh-cronjob.yaml` DEPLOYMENT_NAME. It names a Deployment this chart creates; leaving it behind points `kubectl set image` at a workload that no longer exists. Not in the ticket's list. * `telemetry-collector-configmap.yaml`'s filelog glob, `{ns}_{release}-*/{container}/*.log`. It matches POD DIRECTORIES, and pods are named after the DaemonSet. Route the DaemonSet without the glob and the Collector runs, reports healthy, and collects nothing -- the exact failure that file already warns about. Also not in the ticket's list. WHAT STAYED: `app.kubernetes.io/instance` (Helm convention), the 27 `meta.helm.sh/release-name` annotations (Helm's bookkeeping), the 3 RELEASE_NAME/RELEASE envs (a HELM IDENTITY -- `helm status`, `helm rollback`), and 4 on-disk paths (a LOCATION: renaming orphans a tenant's data). THE TICKET PREDICTED THE BUG AND I WROTE IT ANYWAY. "A `sed` over `.Release.Name` catches it on the first pass; mine did." My first routing pass anchored the env-var exception with `$` against a CONCATENATED context string, so it never matched and the RELEASE_NAME env was routed -- backend#2620 re-introduced by the fix for backend#2621, exactly as written. Two more followed: `$.Release.Name` substituted as a plain string clipped `$.Release.Namespace` to `...$)space`, and routing ran over the helper's own body so `tracebloc.fullname` called itself until helm died. The third is now structural rather than guarded: the helper is inserted AFTER routing, so its body is never a candidate. The guard, `scripts/tests/fullname-override-completeness.sh` plus its assertions module, over EVERY platform profile in `client/ci/*-values.yaml`: 1. NO-OP override == release name renders identically to unset 1b. VERBATIM with the override unset, resource names carry the release name WHOLE 2. MOVED with a distinctive override, no resource name still carries the release name -- misses reported BY NAME 3. STAYED every exception still carries it, checked with the right predicate per class Assertions 2 and 3 are a pair: without 3, assertion 2 is satisfied by renaming Helm's own bookkeeping. PROFILES MATTER, and a single render hid it: `bm-values.yaml` sets `hostPath.enabled`, and the hostPath PVs and dataset directory exist in no other profile. One ad-hoc render checked ONE of the four release-scoped paths and called itself satisfied. ASSERTION 1b EXISTS BECAUSE A MUTATION SURVIVED. Assertion 1 diffs two renders that both pass through the helper, so `| trunc N` cancels out on both sides and is structurally invisible to it. The mutation was also inert for a three-letter release name. The release name used by the guard is now 38 characters, and 1b reads the default render directly. Non-determinism is MEASURED, not listed: secrets.yaml mints credentials with `randAlphaNum`, so the guard renders twice with identical inputs and excludes whatever differs. A hand-kept key list goes stale, and the template's variable names do not even map to the rendered keys (`$podTokenSecret` renders as `POD_TOKEN_SIGNING_SECRET`). Wired into `DRIFT_GUARDS`, which the REQUIRED `Source-of-truth drift` job runs -- a guard in a non-required job is advice. Evidence -------- helm unittest ./client (helm 3.15.4, CI's pin) -> 36 suites, 631 tests, OK 18 name-sensitive shell gates -> all pass helm lint / shellcheck -S warning -x -> clean the guard, 4 profiles -> 32 assertions OK mutations, each anchor asserted to have applied: route the RELEASE_NAME env -> caught (helm-identity) un-route one resource name -> caught (by name) route the dataset directory -> caught (on-disk path) trunc 20 on the default -> caught (verbatim) upper on the default -> caught (verbatim) restored -> green Co-Authored-By: Claude Opus 5 --- Makefile | 2 +- client/Chart.yaml | 4 +- client/templates/_helpers.tpl | 77 +++++-- .../templates/egress-enforcement-check.yaml | 2 +- client/templates/egress-proxy-configmap.yaml | 2 +- client/templates/egress-proxy-deployment.yaml | 4 +- .../templates/egress-reachability-check.yaml | 2 +- client/templates/image-refresh-cronjob.yaml | 2 +- .../templates/ingestion-authz-configmap.yaml | 2 +- client/templates/jobs-manager-deployment.yaml | 4 +- client/templates/jobs-manager-pdb.yaml | 2 +- client/templates/network-policy-training.yaml | 2 +- client/templates/resource-monitor-rbac.yaml | 6 +- client/templates/resource-monitor-scc.yaml | 10 +- .../templates/storage-assertions-check.yaml | 2 +- .../telemetry-collector-configmap.yaml | 2 +- .../templates/telemetry-collector-status.yaml | 2 +- client/values.schema.json | 4 + client/values.yaml | 21 ++ .../tests/fullname-override-completeness.sh | 113 ++++++++++ scripts/tests/fullname_override_assertions.py | 199 ++++++++++++++++++ 21 files changed, 422 insertions(+), 42 deletions(-) create mode 100755 scripts/tests/fullname-override-completeness.sh create mode 100644 scripts/tests/fullname_override_assertions.py diff --git a/Makefile b/Makefile index a6970c1c..12376965 100644 --- a/Makefile +++ b/Makefile @@ -350,7 +350,7 @@ lint-warnings: # # `|`-separated because each guard is a multi-word command. One entry per guard, # and this is the only place they are written down. -DRIFT_GUARDS := scripts/gen-manifest.sh --check|scripts/check-facts.sh --check|bash scripts/check-style.sh|bash scripts/tests/check-drift.sh|bash scripts/tests/env-vocabulary-agreement.sh|bash scripts/tests/telemetry-vocabulary-agreement.sh|bash scripts/tests/k3s-components-agreement.sh|bash scripts/tests/collector-class-a-agreement.sh|bash scripts/tests/openshift-scc-coverage.sh|bash scripts/tests/collector-offsets-persisted.sh|bash scripts/tests/node-agents-tenancy.sh|bash scripts/tests/node-agents-pull-secret.sh|bash scripts/tests/telemetry-token-agreement.sh|bash scripts/tests/node-agents-namespace-safety.sh|bash scripts/tests/automount-token-explicit.sh|bash scripts/tests/collector-redaction-floor.sh|bash scripts/tests/collector-redaction-derived.sh|bash scripts/tests/telemetry-token-bootstrap.sh|bash scripts/tests/node-jsonpath-agreement.sh|bash scripts/tests/preflight-not-privilege-gated.sh|bash scripts/tests/kubelet-arg-map-safety.sh|bash scripts/tests/cronjob-failures-are-readable.sh|bash scripts/tests/release-name-equals-namespace.sh|bash scripts/tests/helm-unittest-error-assertions.sh|bash scripts/tests/helm-unittest-gated.sh|bash scripts/tests/mirror-enumeration-complete.sh|bash scripts/tests/reparent-requires-rotation.sh|bash scripts/tests/edgeuser-drop-readiness-verdicts.sh|bash scripts/tests/client-credentials-have-a-secret-tier.sh|bash scripts/tests/gate-byenv-resolution.sh|bash scripts/tests/gpu-limit-semantics-documented.sh|bash scripts/tests/gate-default-prose-agreement.sh|bash scripts/tests/gate-default-prose-mutations.sh|bash scripts/tests/guards-survive-spaced-paths.sh +DRIFT_GUARDS := scripts/gen-manifest.sh --check|scripts/check-facts.sh --check|bash scripts/check-style.sh|bash scripts/tests/check-drift.sh|bash scripts/tests/env-vocabulary-agreement.sh|bash scripts/tests/telemetry-vocabulary-agreement.sh|bash scripts/tests/k3s-components-agreement.sh|bash scripts/tests/collector-class-a-agreement.sh|bash scripts/tests/openshift-scc-coverage.sh|bash scripts/tests/collector-offsets-persisted.sh|bash scripts/tests/node-agents-tenancy.sh|bash scripts/tests/node-agents-pull-secret.sh|bash scripts/tests/telemetry-token-agreement.sh|bash scripts/tests/node-agents-namespace-safety.sh|bash scripts/tests/automount-token-explicit.sh|bash scripts/tests/collector-redaction-floor.sh|bash scripts/tests/collector-redaction-derived.sh|bash scripts/tests/telemetry-token-bootstrap.sh|bash scripts/tests/node-jsonpath-agreement.sh|bash scripts/tests/preflight-not-privilege-gated.sh|bash scripts/tests/kubelet-arg-map-safety.sh|bash scripts/tests/cronjob-failures-are-readable.sh|bash scripts/tests/release-name-equals-namespace.sh|bash scripts/tests/helm-unittest-error-assertions.sh|bash scripts/tests/helm-unittest-gated.sh|bash scripts/tests/mirror-enumeration-complete.sh|bash scripts/tests/reparent-requires-rotation.sh|bash scripts/tests/edgeuser-drop-readiness-verdicts.sh|bash scripts/tests/client-credentials-have-a-secret-tier.sh|bash scripts/tests/gate-byenv-resolution.sh|bash scripts/tests/gpu-limit-semantics-documented.sh|bash scripts/tests/gate-default-prose-agreement.sh|bash scripts/tests/gate-default-prose-mutations.sh|bash scripts/tests/guards-survive-spaced-paths.sh|bash scripts/tests/fullname-override-completeness.sh # EXPORTED, not interpolated. The recipe reads $$DRIFT_GUARDS from the # environment; it used to do `guards='$(DRIFT_GUARDS)'`, which Make expands diff --git a/client/Chart.yaml b/client/Chart.yaml index f8d48d67..b29dac40 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.86 -appVersion: "1.9.86" +version: 1.9.87 +appVersion: "1.9.87" keywords: - tracebloc - kubernetes diff --git a/client/templates/_helpers.tpl b/client/templates/_helpers.tpl index 116bc6a7..9f567889 100644 --- a/client/templates/_helpers.tpl +++ b/client/templates/_helpers.tpl @@ -17,8 +17,51 @@ app.kubernetes.io/name: {{ .Chart.Name }} app.kubernetes.io/instance: {{ .Release.Name }} {{- end }} +{{/* + tracebloc.fullname — the prefix every resource name this chart creates is built + from. backend#2626. + + DEFAULTS TO `.Release.Name` VERBATIM, and verbatim is load-bearing: no `trunc`, + no `trimSuffix`, no normalisation. Those are defensible in a fresh chart and + wrong here, because the whole migration-safety argument is that an UNSET + override renders byte-identical to the chart before this existed. A `trunc 63` + firing only for release names over 63 characters is a behaviour change hiding + behind a default nobody exercises until it breaks an install. + + WHAT MAY USE IT is not a style question -- backend#2621 was reverted over + exactly this. The release name appears ~174 times across these templates and is + at least six different things: + + MAY follow the override -- names of resources THIS CHART CREATES, and + anything referencing one of those names (an env naming a Deployment to + restart; a log glob matching pod directories, because pod directories are + named after the DaemonSet). + + MUST NOT follow it: + * `app.kubernetes.io/instance` Helm convention: it IS the release + * `meta.helm.sh/release-name` Helm's own ownership bookkeeping + * `RELEASE_NAME` / `RELEASE` env a HELM IDENTITY -- `helm status`, + `helm rollback`. Rename it and + auto-upgrade hunts a release that does + not exist and fails every tick: that is + backend#2620, re-introduced by the fix + for backend#2621. + * on-disk paths a LOCATION, not a name. Renaming + orphans a tenant's data. + * `.Release.Namespace` unrelated + + `scripts/tests/fullname-override-completeness.sh` keeps that table true: it + renders with a distinctive override and fails on any resource name still + carrying the release name, and in the same pass on any exception that STOPPED + carrying it. Both halves are required -- without the second, the guard is + satisfied by breaking auto-upgrade. +*/}} +{{- define "tracebloc.fullname" -}} +{{- default .Release.Name .Values.fullnameOverride -}} +{{- end -}} + {{- define "tracebloc.secretName" -}} -{{ .Release.Name }}-secrets +{{ include "tracebloc.fullname" . }}-secrets {{- end }} {{/* @@ -54,7 +97,7 @@ tracebloc.io/seal-check-name: {{ .name | quote }} {{- end }} {{- define "tracebloc.serviceAccountName" -}} -{{ .Release.Name }}-jobs-manager +{{ include "tracebloc.fullname" . }}-jobs-manager {{- end }} {{/* @@ -80,11 +123,11 @@ tracebloc.io/seal-check-name: {{ .name | quote }} See the v1.2.0 release notes / tenant-d-prod migration case study. */}} {{- define "tracebloc.resourceMonitorName" -}} -{{ .Release.Name }}-resource-monitor +{{ include "tracebloc.fullname" . }}-resource-monitor {{- end }} {{- define "tracebloc.rbacName" -}} -{{ .Release.Name }}-jobs-manager-rbac +{{ include "tracebloc.fullname" . }}-jobs-manager-rbac {{- end }} {{- define "tracebloc.clientDataPvc" -}} @@ -92,7 +135,7 @@ client-pvc {{- end }} {{- define "tracebloc.clientDataPvName" -}} -{{ .Release.Name }}-data-pv +{{ include "tracebloc.fullname" . }}-data-pv {{- end }} {{- define "tracebloc.clientDataStorage" -}} @@ -118,7 +161,7 @@ client-logs-pvc {{- end }} {{- define "tracebloc.clientLogsPvName" -}} -{{ .Release.Name }}-logs-pv +{{ include "tracebloc.fullname" . }}-logs-pv {{- end }} {{- define "tracebloc.clientLogsStorage" -}} @@ -130,7 +173,7 @@ mysql-pvc {{- end }} {{- define "tracebloc.mysqlPvName" -}} -{{ .Release.Name }}-mysql-pv +{{ include "tracebloc.fullname" . }}-mysql-pv {{- end }} {{- define "tracebloc.mysqlStorage" -}} @@ -165,7 +208,7 @@ mysql-pvc 403 and stall auto-upgrade on every later tick. */}} {{- define "tracebloc.createdRegistrySecretName" -}} -{{ .Release.Name }}-regcred +{{ include "tracebloc.fullname" . }}-regcred {{- end }} {{- define "tracebloc.registrySecretName" -}} @@ -173,7 +216,7 @@ mysql-pvc {{- if $reg.existingSecret -}} {{ $reg.existingSecret }} {{- else -}} -{{ .Release.Name }}-regcred +{{ include "tracebloc.fullname" . }}-regcred {{- end -}} {{- end }} @@ -212,7 +255,7 @@ nvidia-device-plugin-daemonset the SA by name, and the CronJob mounts the ConfigMap by name. */}} {{- define "tracebloc.autoUpgradeName" -}} -{{ .Release.Name }}-auto-upgrade +{{ include "tracebloc.fullname" . }}-auto-upgrade {{- end }} {{/* @@ -225,7 +268,7 @@ nvidia-device-plugin-daemonset cluster-admin), and customers may reasonably disable one but not the other. */}} {{- define "tracebloc.imageRefreshName" -}} -{{ .Release.Name }}-image-refresh +{{ include "tracebloc.fullname" . }}-image-refresh {{- end }} {{/* @@ -248,7 +291,7 @@ nvidia-device-plugin-daemonset grant. */}} {{- define "tracebloc.imageRefreshNodeAgentsName" -}} -{{ .Release.Name }}-image-refresh-node-agents +{{ include "tracebloc.fullname" . }}-image-refresh-node-agents {{- end }} {{/* @@ -257,7 +300,7 @@ nvidia-device-plugin-daemonset release namespace. */}} {{- define "tracebloc.rbacNodeAgentsName" -}} -{{ .Release.Name }}-jobs-manager-node-agents +{{ include "tracebloc.fullname" . }}-jobs-manager-node-agents {{- end }} {{/* @@ -280,7 +323,7 @@ nvidia-device-plugin-daemonset left for its own PR; a contract test pins the two sides here in the meantime. */}} {{- define "tracebloc.requestsProxyName" -}} -{{ .Release.Name }}-requests-proxy +{{ include "tracebloc.fullname" . }}-requests-proxy {{- end }} {{/* @@ -443,7 +486,7 @@ Always */}} {{- define "tracebloc.storageClassName" -}} {{- if .Values.storageClass.create -}} -{{ .Release.Name }}-storage-class +{{ include "tracebloc.fullname" . }}-storage-class {{- else -}} {{ .Values.storageClass.name }} {{- end -}} @@ -1000,7 +1043,7 @@ can be kept above the configured helm timeout. two releases on one cluster do not collide in the shared node-agents namespace. */}} {{- define "tracebloc.telemetryCollectorName" -}} -{{- printf "%s-telemetry-collector" .Release.Name | trunc 63 | trimSuffix "-" -}} +{{- printf "%s-telemetry-collector" (include "tracebloc.fullname" .) | trunc 63 | trimSuffix "-" -}} {{- end -}} {{/* @@ -1054,7 +1097,7 @@ tracebloc-telemetry-token {{- $tc := default (dict) .Values.telemetryCollector -}} {{- $name := (default (dict) $tc.tokenSecret).name | default "" -}} {{- if or (eq $name "") (eq $name (include "tracebloc.telemetryTokenLegacyName" .)) -}} -{{- printf "%s-telemetry-token" .Release.Name -}} +{{- printf "%s-telemetry-token" (include "tracebloc.fullname" .) -}} {{- else -}} {{- $name -}} {{- end -}} diff --git a/client/templates/egress-enforcement-check.yaml b/client/templates/egress-enforcement-check.yaml index 557dbeab..0816da34 100644 --- a/client/templates/egress-enforcement-check.yaml +++ b/client/templates/egress-enforcement-check.yaml @@ -17,7 +17,7 @@ apiVersion: batch/v1 kind: Job metadata: - name: {{ .Release.Name }}-egress-enforcement-check + name: {{ include "tracebloc.fullname" . }}-egress-enforcement-check namespace: {{ .Release.Namespace }} labels: {{- include "tracebloc.labels" . | nindent 4 }} diff --git a/client/templates/egress-proxy-configmap.yaml b/client/templates/egress-proxy-configmap.yaml index 57244068..d98463ea 100644 --- a/client/templates/egress-proxy-configmap.yaml +++ b/client/templates/egress-proxy-configmap.yaml @@ -11,7 +11,7 @@ apiVersion: v1 kind: ConfigMap metadata: - name: {{ .Release.Name }}-egress-proxy + name: {{ include "tracebloc.fullname" . }}-egress-proxy namespace: {{ .Release.Namespace }} labels: {{- include "tracebloc.labels" . | nindent 4 }} diff --git a/client/templates/egress-proxy-deployment.yaml b/client/templates/egress-proxy-deployment.yaml index d80469de..792d96ab 100644 --- a/client/templates/egress-proxy-deployment.yaml +++ b/client/templates/egress-proxy-deployment.yaml @@ -17,7 +17,7 @@ apiVersion: apps/v1 kind: Deployment metadata: - name: {{ .Release.Name }}-egress-proxy + name: {{ include "tracebloc.fullname" . }}-egress-proxy namespace: {{ .Release.Namespace }} labels: {{- include "tracebloc.labels" . | nindent 4 }} @@ -82,7 +82,7 @@ spec: volumes: - name: squid-config configMap: - name: {{ .Release.Name }}-egress-proxy + name: {{ include "tracebloc.fullname" . }}-egress-proxy - name: tmp emptyDir: {} - name: var-run diff --git a/client/templates/egress-reachability-check.yaml b/client/templates/egress-reachability-check.yaml index dc69ce0f..e5ae6f1e 100644 --- a/client/templates/egress-reachability-check.yaml +++ b/client/templates/egress-reachability-check.yaml @@ -31,7 +31,7 @@ apiVersion: batch/v1 kind: Job metadata: - name: {{ .Release.Name }}-egress-reachability-check + name: {{ include "tracebloc.fullname" . }}-egress-reachability-check namespace: {{ .Release.Namespace }} labels: {{- include "tracebloc.labels" . | nindent 4 }} diff --git a/client/templates/image-refresh-cronjob.yaml b/client/templates/image-refresh-cronjob.yaml index 70bfb100..2b371ebe 100644 --- a/client/templates/image-refresh-cronjob.yaml +++ b/client/templates/image-refresh-cronjob.yaml @@ -789,7 +789,7 @@ spec: - name: RELEASE_NAMESPACE value: {{ .Release.Namespace | quote }} - name: DEPLOYMENT_NAME - value: {{ printf "%s-jobs-manager" .Release.Name | quote }} + value: {{ printf "%s-jobs-manager" (include "tracebloc.fullname" .) | quote }} # #569: the two additional workloads reconciled by `set image`. - name: REQUESTS_PROXY_DEPLOYMENT value: {{ include "tracebloc.requestsProxyName" . | quote }} diff --git a/client/templates/ingestion-authz-configmap.yaml b/client/templates/ingestion-authz-configmap.yaml index 24b02a37..cdd78db7 100644 --- a/client/templates/ingestion-authz-configmap.yaml +++ b/client/templates/ingestion-authz-configmap.yaml @@ -10,7 +10,7 @@ metadata: # to a list of table-name prefixes. Omit `namespace` to default to the # release's namespace — the common case where the ingestor subchart is # installed alongside the tracebloc client. - name: {{ .Release.Name }}-ingestion-authz + name: {{ include "tracebloc.fullname" . }}-ingestion-authz namespace: {{ .Release.Namespace }} labels: {{- include "tracebloc.labels" . | nindent 4 }} diff --git a/client/templates/jobs-manager-deployment.yaml b/client/templates/jobs-manager-deployment.yaml index e3659fea..e17e8eb7 100644 --- a/client/templates/jobs-manager-deployment.yaml +++ b/client/templates/jobs-manager-deployment.yaml @@ -6,7 +6,7 @@ apiVersion: apps/v1 kind: Deployment metadata: - name: {{ .Release.Name }}-jobs-manager + name: {{ include "tracebloc.fullname" . }}-jobs-manager namespace: {{ .Release.Namespace }} labels: {{- include "tracebloc.labels" . | nindent 4 }} @@ -797,5 +797,5 @@ spec: claimName: {{ include "tracebloc.clientLogsPvc" . }} - name: ingestion-authz configMap: - name: {{ .Release.Name }}-ingestion-authz + name: {{ include "tracebloc.fullname" . }}-ingestion-authz restartPolicy: Always diff --git a/client/templates/jobs-manager-pdb.yaml b/client/templates/jobs-manager-pdb.yaml index 80b769ad..8be6c3de 100644 --- a/client/templates/jobs-manager-pdb.yaml +++ b/client/templates/jobs-manager-pdb.yaml @@ -37,7 +37,7 @@ apiVersion: policy/v1 kind: PodDisruptionBudget metadata: - name: {{ .Release.Name }}-jobs-manager + name: {{ include "tracebloc.fullname" . }}-jobs-manager namespace: {{ .Release.Namespace }} labels: {{- include "tracebloc.labels" . | nindent 4 }} diff --git a/client/templates/network-policy-training.yaml b/client/templates/network-policy-training.yaml index 4ad11315..08b350da 100644 --- a/client/templates/network-policy-training.yaml +++ b/client/templates/network-policy-training.yaml @@ -34,7 +34,7 @@ apiVersion: networking.k8s.io/v1 kind: NetworkPolicy metadata: - name: {{ .Release.Name }}-training-egress + name: {{ include "tracebloc.fullname" . }}-training-egress namespace: {{ .Release.Namespace }} labels: {{- include "tracebloc.labels" . | nindent 4 }} diff --git a/client/templates/resource-monitor-rbac.yaml b/client/templates/resource-monitor-rbac.yaml index 54d9eaaa..da0339dc 100644 --- a/client/templates/resource-monitor-rbac.yaml +++ b/client/templates/resource-monitor-rbac.yaml @@ -33,7 +33,7 @@ metadata: apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: - name: tracebloc-resource-monitor-{{ .Release.Name }} + name: tracebloc-resource-monitor-{{ include "tracebloc.fullname" . }} annotations: meta.helm.sh/release-name: {{ .Release.Name }} meta.helm.sh/release-namespace: {{ .Release.Namespace }} @@ -79,7 +79,7 @@ rules: apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: - name: tracebloc-resource-monitor-{{ .Release.Name }} + name: tracebloc-resource-monitor-{{ include "tracebloc.fullname" . }} annotations: meta.helm.sh/release-name: {{ .Release.Name }} meta.helm.sh/release-namespace: {{ .Release.Namespace }} @@ -89,7 +89,7 @@ metadata: roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole - name: tracebloc-resource-monitor-{{ .Release.Name }} + name: tracebloc-resource-monitor-{{ include "tracebloc.fullname" . }} subjects: - kind: ServiceAccount name: {{ include "tracebloc.resourceMonitorName" . }} diff --git a/client/templates/resource-monitor-scc.yaml b/client/templates/resource-monitor-scc.yaml index 1850edb7..f9fba66c 100644 --- a/client/templates/resource-monitor-scc.yaml +++ b/client/templates/resource-monitor-scc.yaml @@ -3,7 +3,7 @@ apiVersion: security.openshift.io/v1 kind: SecurityContextConstraints metadata: - name: tracebloc-resource-monitor-{{ .Release.Name }} + name: tracebloc-resource-monitor-{{ include "tracebloc.fullname" . }} annotations: meta.helm.sh/release-name: {{ .Release.Name }} meta.helm.sh/release-namespace: {{ .Release.Namespace }} @@ -50,7 +50,7 @@ groups: [] apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: - name: tracebloc-resource-monitor-scc-{{ .Release.Name }} + name: tracebloc-resource-monitor-scc-{{ include "tracebloc.fullname" . }} annotations: meta.helm.sh/release-name: {{ .Release.Name }} meta.helm.sh/release-namespace: {{ .Release.Namespace }} @@ -60,13 +60,13 @@ metadata: rules: - apiGroups: ["security.openshift.io"] resources: ["securitycontextconstraints"] - resourceNames: ["tracebloc-resource-monitor-{{ .Release.Name }}"] + resourceNames: ["tracebloc-resource-monitor-{{ include "tracebloc.fullname" . }}"] verbs: ["use"] --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: - name: tracebloc-resource-monitor-scc-{{ .Release.Name }} + name: tracebloc-resource-monitor-scc-{{ include "tracebloc.fullname" . }} annotations: meta.helm.sh/release-name: {{ .Release.Name }} meta.helm.sh/release-namespace: {{ .Release.Namespace }} @@ -76,7 +76,7 @@ metadata: roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole - name: tracebloc-resource-monitor-scc-{{ .Release.Name }} + name: tracebloc-resource-monitor-scc-{{ include "tracebloc.fullname" . }} subjects: - kind: ServiceAccount name: {{ include "tracebloc.resourceMonitorName" . }} diff --git a/client/templates/storage-assertions-check.yaml b/client/templates/storage-assertions-check.yaml index 14deb21f..8bb1b1f5 100644 --- a/client/templates/storage-assertions-check.yaml +++ b/client/templates/storage-assertions-check.yaml @@ -49,7 +49,7 @@ at install/upgrade time, so it can never block them or the auto-upgrade. Set sealCheck.storageAssertions.enabled=false to disable. */ -}} -{{- $name := printf "%s-storage-assertions-check" .Release.Name }} +{{- $name := printf "%s-storage-assertions-check" (include "tracebloc.fullname" .) }} {{- $timeout := dig "timeoutSeconds" 120 $cfg }} {{- $img := default dict $cfg.image }} {{- $pvScan := ne .Values.clusterScope false }} diff --git a/client/templates/telemetry-collector-configmap.yaml b/client/templates/telemetry-collector-configmap.yaml index c2466da7..e9331aca 100644 --- a/client/templates/telemetry-collector-configmap.yaml +++ b/client/templates/telemetry-collector-configmap.yaml @@ -121,7 +121,7 @@ data: cycle here.) */}} {{- range $tc.classANodeAgentContainers }} - {{- $includes = append $includes (printf "%s/%s_%s-*/%s/*.log" $logs $.Values.nodeAgents.namespace.name $.Release.Name .) }} + {{- $includes = append $includes (printf "%s/%s_%s-*/%s/*.log" $logs $.Values.nodeAgents.namespace.name (include "tracebloc.fullname" $) .) }} {{- end }} {{- /* EACH LIST, NOT THE COMBINED ONE. The first version of this guard checked diff --git a/client/templates/telemetry-collector-status.yaml b/client/templates/telemetry-collector-status.yaml index e529683e..5f0d9d77 100644 --- a/client/templates/telemetry-collector-status.yaml +++ b/client/templates/telemetry-collector-status.yaml @@ -33,7 +33,7 @@ apiVersion: v1 kind: ConfigMap metadata: - name: {{ printf "%s-telemetry-status" .Release.Name }} + name: {{ printf "%s-telemetry-status" (include "tracebloc.fullname" .) }} namespace: {{ .Values.nodeAgents.namespace.name }} labels: {{- include "tracebloc.labels" . | nindent 4 }} diff --git a/client/values.schema.json b/client/values.schema.json index e5e6c472..9b2a92cb 100644 --- a/client/values.schema.json +++ b/client/values.schema.json @@ -1396,6 +1396,10 @@ "type": "boolean" } }, + "fullnameOverride": { + "type": "string", + "description": "Rename the resources this chart creates without uninstall + reinstall (backend#2626). Unset resolves to .Release.Name verbatim, so leaving it alone is byte-identical to before it existed. Renames RESOURCES, not the Helm release: app.kubernetes.io/instance, meta.helm.sh/release-name and the RELEASE_NAME env keep the release name, and release-scoped on-disk paths keep it too because a path is a location, not a name. Changing it on an existing install is a migration (Helm renames as delete + create), not a config tweak." + }, "telemetryCollector": { "type": [ "object", diff --git a/client/values.yaml b/client/values.yaml index ac23f490..5a493bc7 100644 --- a/client/values.yaml +++ b/client/values.yaml @@ -472,6 +472,27 @@ egressProxy: cpu: "500m" memory: "256Mi" +# -- Rename the resources this chart creates, without uninstall + reinstall. +# backend#2626. +# +# UNSET BY DEFAULT, and the default resolves to `.Release.Name` VERBATIM, so +# leaving it alone renders byte-identical to the chart before this existed. That +# is asserted, not asserted-in-prose: `scripts/tests/fullname-override-completeness.sh` +# renders with the override set to the release name and diffs against unset. +# +# IT RENAMES RESOURCES, NOT THE RELEASE. The Helm release keeps its name -- +# `helm list`, `helm status` and `helm rollback` are unaffected, and so are +# `app.kubernetes.io/instance`, Helm's `meta.helm.sh/release-name` bookkeeping, +# and the RELEASE_NAME env auto-upgrade rolls back with (backend#2620). +# +# IT DOES NOT MOVE DATA. Release-scoped on-disk paths -- the dataset directory, +# the hostPath PVs, the Collector's queue -- keep the RELEASE name, because a +# path is a location: renaming it orphans a tenant's data instead of moving it. +# +# Changing it on an existing install renames every resource, which Helm performs +# as delete + create. Treat it as a migration, not a config tweak. +# fullnameOverride: "" + # -- Edge telemetry Collector (RFC-BACKEND-1872 D6/D7, backend#1906). # An OpenTelemetry Collector DaemonSet that reads Class A container stdout with a # `filelog` receiver and forwards it to the tracebloc backend's ingest endpoint. diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh new file mode 100755 index 00000000..5d39f2ef --- /dev/null +++ b/scripts/tests/fullname-override-completeness.sh @@ -0,0 +1,113 @@ +#!/usr/bin/env bash +# fullnameOverride completeness — backend#2626. +# +# `fullnameOverride` renames the resources this chart creates, so a badly-named +# release is fixable without uninstall + reinstall. backend#2621 built the +# helper, proved the default render byte-identical, and REVERTED IT — because the +# helper is the easy half and completeness is the hard one. The release name +# appears ~174 times across these templates and is at least six different things; +# routing some and not others yields `prod-auto-upgrade` beside +# `myrel-jobs-manager`, which is harder to reason about than a badly-named +# release. +# +# So THIS GUARD is the deliverable, not the helper. Three assertions, and 2 and 3 +# are a PAIR — either alone is satisfiable by breaking the other: +# +# 1. NO-OP Setting the override to the release name renders identically to +# leaving it unset. Pins that the default is `.Release.Name` +# verbatim, which is the whole migration-safety argument. +# 2. MOVED With a distinctive override, NO resource name still carries the +# release name. Misses are reported BY NAME. +# 3. STAYED With that same override every exception STILL carries the release +# name. Without this half, (2) is trivially satisfied by renaming +# everything — including the env `helm rollback` reads, which is +# backend#2620 re-introduced by the fix for backend#2621. +# +# EVERY PLATFORM PROFILE, because the platform decides which templates render at +# all: `bm-values.yaml` sets `hostPath.enabled`, and the hostPath PVs plus the +# dataset directory — the on-disk paths this guard most needs to protect — exist +# in no other profile. An earlier single-profile version checked ONE of the four +# release-scoped paths and reported itself satisfied. +# +# Run: scripts/tests/fullname-override-completeness.sh +set -euo pipefail +cd "$(dirname "$0")/../.." + +# DELIBERATELY LONG, and that is not cosmetic. Assertion 1 diffs two renders +# that BOTH pass through the helper, so a transformation applied uniformly -- +# `| trunc N` on the default -- cancels out and is invisible to it. With a short +# release name it is invisible to assertion 1b as well: a mutation adding +# `trunc 3` to the helper SURVIVED the first version of this guard, because the +# release name was `rel`, exactly three characters, so the mutation was inert. +# 38 characters makes any truncation below that observable. (Helm caps release +# names at 53.) +RELEASE="relnamelongenoughtocatchatruncation38" +NS="tracebloc" +OVERRIDE="zzoverride" + +profiles=(client/ci/*-values.yaml) +if [ ! -e "${profiles[0]}" ]; then + echo "[ERROR] no client/ci/*-values.yaml found — this guard would check nothing." + echo " Cannot tell is not OK." + exit 1 +fi + +echo "== fullnameOverride completeness ==" +tmp=$(mktemp -d); trap 'rm -rf "$tmp"' EXIT +failures=0 + +for VALUES in "${profiles[@]}"; do + prof=$(basename "$VALUES" -values.yaml) + echo "-- profile: $prof" + + render() { + helm template "$RELEASE" ./client --namespace "$NS" \ + --set clientId=x --set clientPassword=p -f "$VALUES" "$@" + } + render > "$tmp/a.yaml" + render > "$tmp/b.yaml" + render --set fullnameOverride="$RELEASE" > "$tmp/explicit.yaml" + render --set fullnameOverride="$OVERRIDE" > "$tmp/override.yaml" + + # --- 1. NO-OP ------------------------------------------------------------- + # NON-DETERMINISM IS MEASURED, NOT LISTED. secrets.yaml mints credentials with + # `randAlphaNum` when nothing supplies them, so two renders of identical inputs + # already differ. A hand-kept key list goes stale the day a credential is + # added — and the template's variable names do not even map to the rendered + # data keys (`$podTokenSecret` renders as `POD_TOKEN_SIGNING_SECRET`). So ask + # the chart: any line differing between two IDENTICAL renders cannot be + # evidence about naming. + nondet=$(diff "$tmp/a.yaml" "$tmp/b.yaml" 2>/dev/null | grep -E '^[<>]' \ + | sed -E 's/^[<>][[:space:]]*//; s/:.*$//; s/^[[:space:]]+//' \ + | sort -u | grep -E '^[A-Za-z_][A-Za-z0-9_]*$' || true) + if [ -n "$nondet" ]; then + pat=$(printf '%s\n' "$nondet" | paste -sd'|' -) + strip() { grep -vE "^[[:space:]]*($pat):" "$1"; } + else + strip() { cat "$1"; } + fi + + if diff <(strip "$tmp/a.yaml") <(strip "$tmp/explicit.yaml") > "$tmp/noop.diff"; then + echo " [OK] override == release name renders identically to unset" + else + echo " [ERROR] setting fullnameOverride to the release name CHANGED the render." + echo " The default is not '.Release.Name' verbatim, so an existing" + echo " install would be renamed by an upgrade that set nothing." + head -30 "$tmp/noop.diff" + failures=$((failures + 1)) + fi + + # --- 2 and 3 -------------------------------------------------------------- + if RELEASE="$RELEASE" NS="$NS" OVERRIDE="$OVERRIDE" \ + python3 scripts/tests/fullname_override_assertions.py "$tmp/override.yaml" "$tmp/a.yaml"; then + : + else + failures=$((failures + 1)) + fi +done + +if [ "$failures" -ne 0 ]; then + echo "[ERROR] fullnameOverride is incomplete in $failures profile check(s)" + exit 1 +fi +echo "[OK] fullnameOverride routes every resource name, and no exception followed it" diff --git a/scripts/tests/fullname_override_assertions.py b/scripts/tests/fullname_override_assertions.py new file mode 100644 index 00000000..f962bc03 --- /dev/null +++ b/scripts/tests/fullname_override_assertions.py @@ -0,0 +1,199 @@ +"""Assertions 2 and 3 of the fullnameOverride guard — backend#2626. + +Split out of `fullname-override-completeness.sh` rather than embedded as a +heredoc so it can be read, linted and reasoned about; the shell half only +renders and loops profiles. + +Reads ONE rendered manifest, produced with `fullnameOverride` set to something +distinctive, and answers two opposite questions about it: + + MOVED no resource name may still carry the release name + STAYED every exception must still carry it + +Both are required. Checking only the first is satisfied by renaming Helm's own +bookkeeping and the env var `helm rollback` reads. +""" + +from __future__ import annotations + +import os +import re +import sys + +import yaml + + +def walk(node, path=""): + """Every scalar in the document, with a dotted path to it.""" + if isinstance(node, dict): + for k, v in node.items(): + yield from walk(v, f"{path}.{k}") + elif isinstance(node, list): + for i, v in enumerate(node): + yield from walk(v, f"{path}[{i}]") + else: + yield path, node + + +def main() -> int: + rel = os.environ["RELEASE"] + ns = os.environ["NS"] + ovr = os.environ["OVERRIDE"] + docs = [d for d in yaml.safe_load_all(open(sys.argv[1], encoding="utf-8")) if d] + default_docs = [d for d in yaml.safe_load_all(open(sys.argv[2], encoding="utf-8")) if d] + if not docs: + print(" [ERROR] the render produced no documents; nothing was checked.") + return 1 + + fail = False + # Word-boundary, so an unrelated word merely CONTAINING those letters is not + # a hit while the release token itself is. + tok = re.compile(rf"(^|[^A-Za-z0-9]){re.escape(rel)}($|[^A-Za-z0-9])") + + # --- VERBATIM ----------------------------------------------------------- + # The default must be `.Release.Name` UNCHANGED. Assertion 1 in the shell + # cannot see this: it diffs two renders that both pass through the helper, so + # `| trunc N` applied to the default cancels out on both sides. Measured -- + # a mutation adding `trunc 3` to the helper survived that diff. + # + # So look at the default render directly and require the release name to + # appear WHOLE in the names built from it. A truncating or normalising helper + # fails here even though it renders self-consistently. + verbatim = [ + f"{d.get('kind')}/{(d.get('metadata') or {}).get('name')}" + for d in default_docs + if isinstance((d.get("metadata") or {}).get("name"), str) + and tok.search(d["metadata"]["name"]) + ] + if not verbatim: + fail = True + print( + f" [ERROR] with the override UNSET, no resource name contains the release " + f"name {rel!r} whole. The default is not '.Release.Name' verbatim — a " + f"truncating or normalising helper renames every existing install." + ) + else: + print(f" [OK] {len(verbatim)} resource name(s) carry the release name verbatim when unset") + + # --- MOVED -------------------------------------------------------------- + missed = [ + f"{d.get('kind')}/{(d.get('metadata') or {}).get('name')}" + for d in docs + if isinstance((d.get("metadata") or {}).get("name"), str) + and tok.search(d["metadata"]["name"]) + ] + if missed: + fail = True + print( + f" [ERROR] {len(missed)} resource name(s) still carry the release name " + f"{rel!r} under fullnameOverride={ovr!r} — each is an unrouted site:" + ) + for m in sorted(missed): + print(f" {m}") + else: + print(f" [OK] no resource name carries the release name ({len(docs)} documents)") + + # --- STAYED: values that ARE the release name --------------------------- + exact = { + "app.kubernetes.io/instance label": ([], rel), + "meta.helm.sh/release-name annotation": ([], rel), + "RELEASE_NAME / RELEASE env (a Helm identity)": ([], rel), + "RELEASE_NAMESPACE env": ([], ns), + } + for d in docs: + where = f"{d.get('kind')}/{(d.get('metadata') or {}).get('name')}" + for path, val in walk(d): + if not isinstance(val, str): + continue + if path.endswith(".app.kubernetes.io/instance"): + exact["app.kubernetes.io/instance label"][0].append((where, path, val)) + elif path.endswith(".meta.helm.sh/release-name"): + exact["meta.helm.sh/release-name annotation"][0].append((where, path, val)) + # env vars are name/value SIBLINGS, so the key is not on the value's path + for path, val in walk(d): + if not path.endswith(".name") or not isinstance(val, str): + continue + key = { + "RELEASE_NAME": "RELEASE_NAME / RELEASE env (a Helm identity)", + "RELEASE": "RELEASE_NAME / RELEASE env (a Helm identity)", + "RELEASE_NAMESPACE": "RELEASE_NAMESPACE env", + }.get(val) + if not key: + continue + sibling = path.rsplit(".name", 1)[0] + ".value" + for p2, v2 in walk(d): + if p2 == sibling: + exact[key][0].append((where, val, v2)) + + for label, (found, expected) in exact.items(): + if not found: + # CANNOT TELL IS A FINDING. An empty list agrees with every + # expectation, so a selector that stops matching would read as a pass. + print( + f" [ERROR] found NO {label} to check — the selector matches nothing, " + f"so this assertion proves nothing." + ) + fail = True + continue + bad = [f for f in found if f[2] != expected] + if bad: + fail = True + print( + f" [ERROR] {len(bad)} {label} value(s) followed the override; they " + f"must stay {expected!r}:" + ) + for where, p, v in sorted(bad)[:10]: + print(f" {where} {p} = {v!r}") + else: + print(f" [OK] {len(found)} {label} value(s) still {expected!r}") + + # --- STAYED: on-disk paths --------------------------------------------- + # A DIFFERENT PREDICATE, and the difference is the point. The checks above + # compare for equality because a label or a Helm-identity env IS the release + # name. A path is not: `/proc` and `/var/log/pods` never referenced the + # release, and `/var/lib/tracebloc//telemetry` merely contains it. + # Demanding equality reported those unscoped paths as regressions — the first + # cut of this guard did exactly that. The real property is that no path may + # FOLLOW the override, because a path is a location: renaming it orphans a + # tenant's data rather than moving it. + paths = [ + (f"{d.get('kind')}/{(d.get('metadata') or {}).get('name')}", path, val) + for d in docs + for path, val in walk(d) + if isinstance(val, str) + and (path.endswith(".hostPath.path") or (path.endswith(".path") and val.startswith("/"))) + ] + if not paths: + print(" [ERROR] found NO on-disk paths to check — selector matches nothing.") + fail = True + else: + followed = [p for p in paths if ovr in p[2]] + scoped = [p for p in paths if tok.search(p[2])] + if followed: + fail = True + print( + f" [ERROR] {len(followed)} on-disk path(s) followed the override. A " + f"path is a LOCATION, not a name — renaming it orphans a tenant's " + f"data rather than moving it:" + ) + for where, p, v in sorted(followed): + print(f" {where} {p} = {v!r}") + elif not scoped: + # Vacuity guard: "none followed the override" is also true of a chart + # that stopped scoping paths by release at all. + print( + " [ERROR] no on-disk path is release-scoped, so 'none followed the " + "override' proves nothing. Did the release-scoped paths disappear?" + ) + fail = True + else: + print( + f" [OK] {len(scoped)} release-scoped path(s) kept the release name; " + f"{len(paths) - len(scoped)} unscoped path(s) untouched" + ) + + return 1 if fail else 0 + + +if __name__ == "__main__": + sys.exit(main()) From 6aecef53b8ebe4468e16cba2c3505c9c987e768e Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 11:00:37 +0200 Subject: [PATCH 02/22] fix(chart): the guard walks every scalar, NOTES is routed and checked (backend#2626) All four review points, and the first two were the same defect. THE GUARD DID NOT COVER THE SITES THIS PR SINGLED OUT. MOVED read doc-root `metadata.name` only, while STAYED enumerated the exception classes separately - so the two halves disagreed about what the exceptions were and every name-REFERENCE site fell through the gap. Un-routing DEPLOYMENT_NAME (which `kubectl set image` targets) or the Collector filelog glob (which globs pod directories) rendered both broken and left the guard green on all four profiles. Now ONE classifier, TWO callers: `classify()` walks every string scalar and labels each release-name hit with the exception that licenses it, or None. MOVED is "nothing unlicensed"; STAYED is "every class non-empty and correct". Adding a class cannot weaken MOVED without adding an obligation to STAYED. 1339-1422 scalars per profile, against a handful of metadata.name before. NOTES.txt: L6, L24 and L27 routed. It was half-routed seven lines apart - L6 printed the release name while L13 printed the override, in the first thing an operator reads. Assertion 4 now checks it, and needed its own render: `helm template` omits NOTES, `--show-only templates/NOTES.txt` answers "could not find template", and BOTH `--dry-run` and `--dry-run=client` need a cluster on 3.15.4 - with a reachable one, ownership validation against real objects made the verdict depend on whose kubeconfig ran it. So it renders a chart copy in which NOTES is an ordinary template, with --debug (helm refuses to emit output it cannot parse as YAML, and NOTES carries ANSI escapes) and KUBECONFIG=/dev/null. PyYAML: a named refusal with EXIT 2, distinct from the 1 that means "the chart is incomplete" - a missing dependency used to print a traceback and then "[ERROR] fullnameOverride is incomplete in 4 profile check(s)", sending the reader hunting un-routed names that do not exist. And `.py` is now in pyyaml-preflight.bats`s filter: `extract_python` finds python embedded in shell, so the first sidecar in this tree escaped the class rule entirely and every future one would have too. values.schema.json: DNS-1123 pattern + maxLength 53, the same constraints Helm enforces on the value this replaces. Verified - a bad value now fails at template time by name instead of object-by-object at the API server mid-install. VERIFIED: make drift 35/35 guards green on all four profiles; CI=true bats scripts/tests/*.bats 1568 passing, 0 failures; shellcheck -S warning -x clean. Mutation-proved eight ways, each anchor asserted applied - and two mutations exposed weaknesses in my own fix: a broadened path allowlist swallowed unrouted sites out of MOVED (the path class was the only one a non-path could satisfy by accident - it now asserts its members are paths), and deleting assertion 4`s "no NOTES supplied" refusal changed nothing because the loop always supplies it (there is now a self-check that invokes the assertions without it and requires a refusal). Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 --- client/templates/NOTES.txt | 6 +- client/values.schema.json | 4 +- .../tests/fullname-override-completeness.sh | 102 +++++- scripts/tests/fullname_override_assertions.py | 338 ++++++++++++------ scripts/tests/pyyaml-preflight.bats | 27 +- 5 files changed, 359 insertions(+), 118 deletions(-) diff --git a/client/templates/NOTES.txt b/client/templates/NOTES.txt index 0197da7e..d941b10e 100644 --- a/client/templates/NOTES.txt +++ b/client/templates/NOTES.txt @@ -3,7 +3,7 @@ {{ "\033[1;36m" }}━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━{{ "\033[0m" }} {{ "\033[1;35m" }}Components:{{ "\033[0m" }} - - {{ "\033[1;34m" }}Jobs Manager:{{ "\033[0m" }} {{ "\033[0;33m" }}{{ .Release.Name }}-jobs-manager{{ "\033[0m" }} + - {{ "\033[1;34m" }}Jobs Manager:{{ "\033[0m" }} {{ "\033[0;33m" }}{{ include "tracebloc.fullname" . }}-jobs-manager{{ "\033[0m" }} - {{ "\033[1;34m" }}MySQL Host:{{ "\033[0m" }} {{ "\033[0;33m" }}mysql-client{{ "\033[0m" }} {{- if ne .Values.resourceMonitor false }} - {{ "\033[1;34m" }}Resource Monitor (DS):{{ "\033[0m" }} {{ "\033[0;33m" }}{{ include "tracebloc.resourceMonitorName" . }}{{ "\033[0m" }} @@ -21,10 +21,10 @@ {{ "\033[1;34m" }}Storage:{{ "\033[0m" }} {{ "\033[0;33m" }}dynamic PVC ({{ include "tracebloc.storageClassName" . }}){{ "\033[0m" }} {{- end }} {{- if .Values.openshift.scc.enabled }} - {{ "\033[1;34m" }}OpenShift SCC:{{ "\033[0m" }} {{ "\033[0;33m" }}tracebloc-resource-monitor-{{ .Release.Name }}{{ "\033[0m" }} + {{ "\033[1;34m" }}OpenShift SCC:{{ "\033[0m" }} {{ "\033[0;33m" }}tracebloc-resource-monitor-{{ include "tracebloc.fullname" . }}{{ "\033[0m" }} {{- end }} {{- if .Values.autoUpgrade.enabled }} - {{ "\033[1;34m" }}Auto-upgrade:{{ "\033[0m" }} {{ "\033[1;32m" }}ON{{ "\033[0m" }} {{ "\033[0;90m" }}(CronJob {{ .Release.Name }}-auto-upgrade, schedule "{{ .Values.autoUpgrade.schedule }}", repo {{ .Values.autoUpgrade.repoUrl }}){{ "\033[0m" }} + {{ "\033[1;34m" }}Auto-upgrade:{{ "\033[0m" }} {{ "\033[1;32m" }}ON{{ "\033[0m" }} {{ "\033[0;90m" }}(CronJob {{ include "tracebloc.autoUpgradeName" . }}, schedule "{{ .Values.autoUpgrade.schedule }}", repo {{ .Values.autoUpgrade.repoUrl }}){{ "\033[0m" }} {{- else }} {{ "\033[1;34m" }}Auto-upgrade:{{ "\033[0m" }} {{ "\033[1;33m" }}OFF{{ "\033[0m" }} {{ "\033[0;90m" }}(release pinned to chart {{ .Chart.Version }}; set autoUpgrade.enabled=true to receive future fixes automatically){{ "\033[0m" }} {{- end }} diff --git a/client/values.schema.json b/client/values.schema.json index 9b2a92cb..194c7bb1 100644 --- a/client/values.schema.json +++ b/client/values.schema.json @@ -1398,7 +1398,9 @@ }, "fullnameOverride": { "type": "string", - "description": "Rename the resources this chart creates without uninstall + reinstall (backend#2626). Unset resolves to .Release.Name verbatim, so leaving it alone is byte-identical to before it existed. Renames RESOURCES, not the Helm release: app.kubernetes.io/instance, meta.helm.sh/release-name and the RELEASE_NAME env keep the release name, and release-scoped on-disk paths keep it too because a path is a location, not a name. Changing it on an existing install is a migration (Helm renames as delete + create), not a config tweak." + "pattern": "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$", + "maxLength": 53, + "description": "Rename the resources this chart creates without uninstall + reinstall (backend#2626). Unset resolves to .Release.Name verbatim, so leaving it alone is byte-identical to before it existed. Renames RESOURCES, not the Helm release: app.kubernetes.io/instance, meta.helm.sh/release-name and the RELEASE_NAME env keep the release name, and release-scoped on-disk paths keep it too because a path is a location, not a name. Changing it on an existing install is a migration (Helm renames as delete + create), not a config tweak. THE PATTERN AND LENGTH ARE THE ONES HELM ENFORCES ON THE VALUE THIS REPLACES: a release name is a DNS-1123 label capped at 53 characters, so without them this key was a strictly weaker gate than the thing it substitutes for. A value like Bad_Name! templated clean and then failed object-by-object at the API server, mid-install, leaving a partly-created release. Declared here so it fails at template time, by name (reviewer, backend#2626)." }, "telemetryCollector": { "type": [ diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh index 5d39f2ef..79df22bb 100755 --- a/scripts/tests/fullname-override-completeness.sh +++ b/scripts/tests/fullname-override-completeness.sh @@ -22,6 +22,17 @@ # name. Without this half, (2) is trivially satisfied by renaming # everything — including the env `helm rollback` reads, which is # backend#2620 re-introduced by the fix for backend#2621. +# 4. NOTES The install message names no stale release. Its own render, because +# `helm template` does not emit NOTES.txt at all. +# +# (2) AND (3) SHARE ONE CLASSIFIER, and that is what closes the gap the first +# version had: (2) read doc-root `metadata.name` only, so every name-REFERENCE +# site — the `DEPLOYMENT_NAME` env `kubectl set image` targets, the Collector's +# filelog glob — could be un-routed with the guard staying green on all four +# profiles. It now walks every string scalar and asks `classify()` which +# exception licenses each release-name hit; (2) is "nothing unlicensed", (3) is +# "every class non-empty and correct". Adding a class cannot weaken (2) without +# also adding an obligation to (3). # # EVERY PLATFORM PROFILE, because the platform decides which templates render at # all: `bm-values.yaml` sets `hostPath.enabled`, and the hostPath PVs plus the @@ -56,6 +67,49 @@ echo "== fullnameOverride completeness ==" tmp=$(mktemp -d); trap 'rm -rf "$tmp"' EXIT failures=0 +# NOTES NEEDS ITS OWN RENDER, AND EVERY OBVIOUS ROUTE IS CLOSED. Measured on the +# CI-pinned helm v3.15.4: +# +# helm template … omits NOTES entirely +# helm template … --show-only templates/NOTES.txt "could not find template" +# (NOTES is not in the +# manifest set) +# helm install … --dry-run "Kubernetes cluster +# unreachable" +# helm install … --dry-run=client ALSO needs a cluster on +# 3.15.4, and with a reachable +# one it fails ownership +# validation against real +# objects -- so the guard's +# verdict would depend on +# whose kubeconfig ran it +# +# So: render a COPY of the chart in which NOTES.txt is an ordinary template. That +# keeps the real template engine and the real values, needs no cluster, and gives +# the same answer on a laptop and on CI. `--debug` is required because NOTES +# carries ANSI escapes and helm refuses to emit output it cannot parse as YAML +# ("control characters are not allowed"); `--debug` renders it anyway. +# KUBECONFIG=/dev/null is belt-and-braces: it makes the hermeticity a property of +# the command rather than of the machine. +probe="$tmp/notes-probe" +mkdir -p "$probe" +cp -R client "$probe/chart" +mv "$probe/chart/templates/NOTES.txt" "$probe/chart/templates/zz-notes-probe.txt" + +# EXIT CODE 1 IS EXPECTED HERE, and swallowing it is deliberate rather than lazy. +# `--debug` prints the render AND still exits non-zero, because helm considers +# unparseable output an error even when asked to emit it anyway. Under +# `set -euo pipefail` that killed the whole guard silently after the first +# profile. So the exit code is discarded and EMPTINESS is the failure signal +# instead — checked by the caller, which is the honest test of "did we get a +# render": a non-zero exit here means nothing, an empty file means we checked +# nothing. +render_notes() { + KUBECONFIG=/dev/null helm template "$RELEASE" "$probe/chart" --namespace "$NS" \ + --set clientId=x --set clientPassword=p -f "$VALUES" "$@" \ + --show-only templates/zz-notes-probe.txt --debug 2>/dev/null || true +} + for VALUES in "${profiles[@]}"; do prof=$(basename "$VALUES" -values.yaml) echo "-- profile: $prof" @@ -69,6 +123,16 @@ for VALUES in "${profiles[@]}"; do render --set fullnameOverride="$RELEASE" > "$tmp/explicit.yaml" render --set fullnameOverride="$OVERRIDE" > "$tmp/override.yaml" + notes() { render_notes "$@"; } + notes > "$tmp/notes-default.txt" + notes --set fullnameOverride="$OVERRIDE" > "$tmp/notes-override.txt" + for f in notes-default notes-override; do + if ! [ -s "$tmp/$f.txt" ]; then + echo " [ERROR] rendering NOTES produced nothing ($f). Cannot tell is not OK." + failures=$((failures + 1)) + fi + done + # --- 1. NO-OP ------------------------------------------------------------- # NON-DETERMINISM IS MEASURED, NOT LISTED. secrets.yaml mints credentials with # `randAlphaNum` when nothing supplies them, so two renders of identical inputs @@ -98,14 +162,42 @@ for VALUES in "${profiles[@]}"; do fi # --- 2 and 3 -------------------------------------------------------------- - if RELEASE="$RELEASE" NS="$NS" OVERRIDE="$OVERRIDE" \ - python3 scripts/tests/fullname_override_assertions.py "$tmp/override.yaml" "$tmp/a.yaml"; then - : - else - failures=$((failures + 1)) + # Exit 2 is "could not check" (PyYAML absent), NOT "the chart is incomplete" — + # reported separately so a missing dependency never reads as a chart defect. + set +e + RELEASE="$RELEASE" NS="$NS" OVERRIDE="$OVERRIDE" \ + python3 scripts/tests/fullname_override_assertions.py \ + "$tmp/override.yaml" "$tmp/a.yaml" \ + "$tmp/notes-override.txt" "$tmp/notes-default.txt" + rc=$? + set -e + if [ "$rc" -eq 2 ]; then + echo "[ERROR] the guard could not run (see above). This is NOT a verdict on the chart." + exit 2 fi + [ "$rc" -eq 0 ] || failures=$((failures + 1)) done +# --- 5. THE GUARD REFUSES TO RUN HALF OF ITSELF ----------------------------- +# A self-check, because assertion 4's "no rendered NOTES was passed" branch is +# unreachable from the loop above (which always passes both files) — and an +# unreachable refusal is one nobody notices has stopped refusing. Measured: +# deleting that branch left the whole guard green. +# +# So invoke the assertions directly with the NOTES arguments MISSING and require +# a non-zero exit. If this ever passes, a future caller could quietly drop the +# NOTES render and the guard would report three assertions as four. +if RELEASE="$RELEASE" NS="$NS" OVERRIDE="$OVERRIDE" \ + python3 scripts/tests/fullname_override_assertions.py \ + "$tmp/override.yaml" "$tmp/a.yaml" >/dev/null 2>&1; then + echo "[ERROR] the assertions PASSED with no rendered NOTES supplied, so a caller" + echo " that drops the NOTES render would get a green guard that checked" + echo " three things while documenting four." + failures=$((failures + 1)) +else + echo "-- self-check: the assertions refuse to run without the NOTES render [OK]" +fi + if [ "$failures" -ne 0 ]; then echo "[ERROR] fullnameOverride is incomplete in $failures profile check(s)" exit 1 diff --git a/scripts/tests/fullname_override_assertions.py b/scripts/tests/fullname_override_assertions.py index f962bc03..965d7ec2 100644 --- a/scripts/tests/fullname_override_assertions.py +++ b/scripts/tests/fullname_override_assertions.py @@ -1,17 +1,33 @@ -"""Assertions 2 and 3 of the fullnameOverride guard — backend#2626. +"""Assertions 2, 3 and 4 of the fullnameOverride guard — backend#2626. Split out of `fullname-override-completeness.sh` rather than embedded as a heredoc so it can be read, linted and reasoned about; the shell half only renders and loops profiles. -Reads ONE rendered manifest, produced with `fullnameOverride` set to something -distinctive, and answers two opposite questions about it: +Reads the rendered manifests — produced with `fullnameOverride` set to something +distinctive — and answers two opposite questions about the SAME set of scalars: - MOVED no resource name may still carry the release name - STAYED every exception must still carry it + MOVED nothing may still carry the release name + STAYED every declared exception must still carry it Both are required. Checking only the first is satisfied by renaming Helm's own bookkeeping and the env var `helm rollback` reads. + +ONE CLASSIFIER, TWO CALLERS, and this is the structural point rather than a +refactor. The first version answered MOVED by looking at doc-root +`metadata.name` and nothing else, while STAYED enumerated the exception classes +separately. So the two halves disagreed about what the exceptions were, and +every name-REFERENCE site fell through the gap: un-routing `DEPLOYMENT_NAME` +(which `kubectl set image` targets) or the Collector's filelog glob (which globs +pod directories) rendered both broken and left the guard green on all four +profiles — the two sites this PR's own description called out as the ones a +`metadata.name` sweep misses (Asad + Bugbot, review of backend#2626). + +Now `classify()` walks EVERY string scalar and labels the ones carrying the +release name with the exception class that licenses them, or `None`. MOVED is +"nothing classified `None`"; STAYED is "every class is non-empty and correct". +Neither can be satisfied by breaking the other, and neither can be satisfied by +a site the other never looked at. """ from __future__ import annotations @@ -20,7 +36,45 @@ import re import sys -import yaml +# PREFLIGHT, NOT A BARE IMPORT. This runs on CI runners and on operators' +# laptops, and a runner with `python3` but no PyYAML used to die as a +# `ModuleNotFoundError` traceback -- after which the shell half printed +# "[ERROR] fullnameOverride is incomplete in 4 profile check(s)", so a MISSING +# DEPENDENCY reported itself as a chart defect and sent the reader hunting +# un-routed names that do not exist (Asad + Bugbot, review of backend#2626). +# +# Exit 2, distinct from the 1 that means "the chart is incomplete": the caller +# and a human both need "could not check" to be a different answer from "checked +# and found a problem". `scripts/tests/pyyaml-preflight.bats` asserts every +# sidecar in this tree carries this shape. +try: + import yaml +except ModuleNotFoundError: + sys.stderr.write( + "[ERROR] this guard needs PyYAML and the interpreter does not have it.\n" + " Install it: python3 -m pip install pyyaml\n" + " NOT a chart defect -- nothing about fullnameOverride was\n" + " checked, which is a different answer from 'checked and clean'.\n" + ) + raise SystemExit(2) + +#: Env var names that ARE a Helm identity and must keep the release name. +#: `helm rollback` reads these; renaming them is backend#2620. +RELEASE_ENV = {"RELEASE_NAME", "RELEASE", "RELEASE_NAMESPACE"} + +#: Attribute paths whose value IS the release name, by Helm's own convention. +RELEASE_IDENTITY_SUFFIXES = ( + ".app.kubernetes.io/instance", + ".meta.helm.sh/release-name", +) + +# The exception classes, as labels. Kept as constants because both the +# classifier and the per-class expectations below key on them, and a typo in one +# place would silently create a class nothing checks. +CLS_INSTANCE = "app.kubernetes.io/instance label" +CLS_ANNOTATION = "meta.helm.sh/release-name annotation" +CLS_ENV = "RELEASE_NAME / RELEASE / RELEASE_NAMESPACE env (a Helm identity)" +CLS_PATH = "on-disk path (a location, not a name)" def walk(node, path=""): @@ -35,12 +89,54 @@ def walk(node, path=""): yield path, node +def env_value_paths(doc): + """`{path-of-an-env-value: ENV_NAME}` for every env entry in `doc`. + + Env vars are `name`/`value` SIBLINGS, so the variable's name is not on its + value's path and cannot be recovered from the path alone. Computed once per + document and shared by both callers rather than re-derived in each. + """ + out = {} + for path, val in walk(doc): + if path.endswith(".name") and isinstance(val, str): + out[path.rsplit(".name", 1)[0] + ".value"] = val + return out + + +def classify(doc, path, val, rel, ns, envs): + """Which exception licenses `val` carrying the release name — or `None`. + + `None` means "an unrouted site". Every branch here is a class STAYED then + checks for the right VALUE, so adding a class cannot weaken MOVED without + also adding an obligation. + """ + if any(path.endswith(s) for s in RELEASE_IDENTITY_SUFFIXES): + return CLS_INSTANCE if path.endswith(RELEASE_IDENTITY_SUFFIXES[0]) else CLS_ANNOTATION + if path in envs and envs[path] in RELEASE_ENV: + return CLS_ENV + # A PATH IS A LOCATION. `/var/lib/tracebloc//telemetry` merely + # contains the release; renaming it orphans a tenant's data rather than + # moving it. Deliberately narrow: only a value that IS a filesystem path, at + # a key that declares itself one. A release-scoped path embedded in a + # ConfigMap blob is NOT covered, which is what makes the Collector's filelog + # glob catchable. + if path.endswith(".hostPath.path") or (path.endswith(".path") and val.startswith("/")): + return CLS_PATH + return None + + +def strip_ansi(text): + return re.sub(r"\033\[[0-9;]*m", "", text) + + def main() -> int: rel = os.environ["RELEASE"] ns = os.environ["NS"] ovr = os.environ["OVERRIDE"] docs = [d for d in yaml.safe_load_all(open(sys.argv[1], encoding="utf-8")) if d] default_docs = [d for d in yaml.safe_load_all(open(sys.argv[2], encoding="utf-8")) if d] + notes_override = sys.argv[3] if len(sys.argv) > 3 else "" + notes_default = sys.argv[4] if len(sys.argv) > 4 else "" if not docs: print(" [ERROR] the render produced no documents; nothing was checked.") return 1 @@ -55,10 +151,6 @@ def main() -> int: # cannot see this: it diffs two renders that both pass through the helper, so # `| trunc N` applied to the default cancels out on both sides. Measured -- # a mutation adding `trunc 3` to the helper survived that diff. - # - # So look at the default render directly and require the release name to - # appear WHOLE in the names built from it. A truncating or normalising helper - # fails here even though it renders self-consistently. verbatim = [ f"{d.get('kind')}/{(d.get('metadata') or {}).get('name')}" for d in default_docs @@ -75,122 +167,158 @@ def main() -> int: else: print(f" [OK] {len(verbatim)} resource name(s) carry the release name verbatim when unset") - # --- MOVED -------------------------------------------------------------- - missed = [ - f"{d.get('kind')}/{(d.get('metadata') or {}).get('name')}" - for d in docs - if isinstance((d.get("metadata") or {}).get("name"), str) - and tok.search(d["metadata"]["name"]) - ] - if missed: - fail = True - print( - f" [ERROR] {len(missed)} resource name(s) still carry the release name " - f"{rel!r} under fullnameOverride={ovr!r} — each is an unrouted site:" - ) - for m in sorted(missed): - print(f" {m}") - else: - print(f" [OK] no resource name carries the release name ({len(docs)} documents)") - - # --- STAYED: values that ARE the release name --------------------------- - exact = { - "app.kubernetes.io/instance label": ([], rel), - "meta.helm.sh/release-name annotation": ([], rel), - "RELEASE_NAME / RELEASE env (a Helm identity)": ([], rel), - "RELEASE_NAMESPACE env": ([], ns), - } + # --- classify every token-bearing scalar, once --------------------------- + licensed = {CLS_INSTANCE: [], CLS_ANNOTATION: [], CLS_ENV: [], CLS_PATH: []} + unrouted = [] + scanned = 0 for d in docs: where = f"{d.get('kind')}/{(d.get('metadata') or {}).get('name')}" + envs = env_value_paths(d) for path, val in walk(d): if not isinstance(val, str): continue - if path.endswith(".app.kubernetes.io/instance"): - exact["app.kubernetes.io/instance label"][0].append((where, path, val)) - elif path.endswith(".meta.helm.sh/release-name"): - exact["meta.helm.sh/release-name annotation"][0].append((where, path, val)) - # env vars are name/value SIBLINGS, so the key is not on the value's path - for path, val in walk(d): - if not path.endswith(".name") or not isinstance(val, str): - continue - key = { - "RELEASE_NAME": "RELEASE_NAME / RELEASE env (a Helm identity)", - "RELEASE": "RELEASE_NAME / RELEASE env (a Helm identity)", - "RELEASE_NAMESPACE": "RELEASE_NAMESPACE env", - }.get(val) - if not key: + scanned += 1 + if not tok.search(val): continue - sibling = path.rsplit(".name", 1)[0] + ".value" - for p2, v2 in walk(d): - if p2 == sibling: - exact[key][0].append((where, val, v2)) + cls = classify(d, path, val, rel, ns, envs) + if cls is None: + unrouted.append((where, path, val)) + else: + licensed[cls].append((where, path, val, envs.get(path, ""))) + + # FAIL CLOSED. Zero scalars scanned agrees with every assertion below, and a + # walk that stopped walking is indistinguishable from a clean chart. + if scanned == 0: + print(" [ERROR] walked 0 string scalars — the walk sees nothing, so nothing was checked.") + return 1 - for label, (found, expected) in exact.items(): + # --- MOVED -------------------------------------------------------------- + if unrouted: + fail = True + print( + f" [ERROR] {len(unrouted)} value(s) still carry the release name {rel!r} " + f"under fullnameOverride={ovr!r}, and no exception licenses them — each is " + f"an unrouted site:" + ) + for where, path, val in sorted(unrouted)[:20]: + snippet = val if len(val) <= 90 else val[:87] + "..." + print(f" {where} {path} = {snippet!r}") + if len(unrouted) > 20: + print(f" ... and {len(unrouted) - 20} more") + else: + print( + f" [OK] no unrouted value carries the release name " + f"({scanned} string scalars across {len(docs)} documents)" + ) + + # --- STAYED ------------------------------------------------------------- + expected = {CLS_INSTANCE: rel, CLS_ANNOTATION: rel, CLS_ENV: None, CLS_PATH: None} + for cls, found in licensed.items(): if not found: # CANNOT TELL IS A FINDING. An empty list agrees with every - # expectation, so a selector that stops matching would read as a pass. + # expectation, so a class that stops matching would read as a pass. print( - f" [ERROR] found NO {label} to check — the selector matches nothing, " - f"so this assertion proves nothing." + f" [ERROR] found NO {cls} carrying the release name — the class matches " + f"nothing, so its half of this assertion proves nothing." ) fail = True continue - bad = [f for f in found if f[2] != expected] + if cls is CLS_PATH: + # A DIFFERENT PREDICATE, and the difference is the point: a path is + # not the release name, it merely contains it. The property is that + # it did not FOLLOW the override. + # + # FIRST, THOUGH: every member must actually BE a path. This class is + # the only one whose obligation a non-path can satisfy by accident — + # the other three demand the value equal a release identity, so a + # mis-classified value fails there, while "did not follow the + # override" is true of any unrouted name. So a broadened allowlist + # here would silently swallow unrouted sites out of MOVED and report + # nothing. Measured: a mutation returning CLS_PATH for every + # unclassified value left the guard green. + notpath = [f for f in found if not f[2].startswith("/")] + if notpath: + fail = True + print( + f" [ERROR] {len(notpath)} value(s) classified as an on-disk path " + f"are not paths, so MOVED is not seeing them:" + ) + for where, path, val, _ in sorted(notpath)[:10]: + print(f" {where} {path} = {val!r}") + followed = [f for f in found if ovr in f[2]] + if followed: + fail = True + print(f" [ERROR] {len(followed)} {cls} followed the override:") + for where, path, val, _ in sorted(followed): + print(f" {where} {path} = {val!r}") + else: + print(f" [OK] {len(found)} release-scoped path(s) kept the release name") + continue + want = expected[cls] + if cls is CLS_ENV: + bad = [f for f in found if f[2] not in (rel, ns)] + else: + bad = [f for f in found if f[2] != want] if bad: fail = True - print( - f" [ERROR] {len(bad)} {label} value(s) followed the override; they " - f"must stay {expected!r}:" - ) - for where, p, v in sorted(bad)[:10]: - print(f" {where} {p} = {v!r}") + print(f" [ERROR] {len(bad)} {cls} value(s) are not the release identity:") + for where, path, val, name in sorted(bad)[:10]: + print(f" {where} {name or path} = {val!r}") else: - print(f" [OK] {len(found)} {label} value(s) still {expected!r}") - - # --- STAYED: on-disk paths --------------------------------------------- - # A DIFFERENT PREDICATE, and the difference is the point. The checks above - # compare for equality because a label or a Helm-identity env IS the release - # name. A path is not: `/proc` and `/var/log/pods` never referenced the - # release, and `/var/lib/tracebloc//telemetry` merely contains it. - # Demanding equality reported those unscoped paths as regressions — the first - # cut of this guard did exactly that. The real property is that no path may - # FOLLOW the override, because a path is a location: renaming it orphans a - # tenant's data rather than moving it. - paths = [ - (f"{d.get('kind')}/{(d.get('metadata') or {}).get('name')}", path, val) - for d in docs - for path, val in walk(d) - if isinstance(val, str) - and (path.endswith(".hostPath.path") or (path.endswith(".path") and val.startswith("/"))) - ] - if not paths: - print(" [ERROR] found NO on-disk paths to check — selector matches nothing.") - fail = True - else: - followed = [p for p in paths if ovr in p[2]] - scoped = [p for p in paths if tok.search(p[2])] - if followed: - fail = True - print( - f" [ERROR] {len(followed)} on-disk path(s) followed the override. A " - f"path is a LOCATION, not a name — renaming it orphans a tenant's " - f"data rather than moving it:" - ) - for where, p, v in sorted(followed): - print(f" {where} {p} = {v!r}") - elif not scoped: - # Vacuity guard: "none followed the override" is also true of a chart - # that stopped scoping paths by release at all. + print(f" [OK] {len(found)} {cls} value(s) still carry the release identity") + + # --- NOTES -------------------------------------------------------------- + # A SEPARATE RENDER, because `helm template` does not emit NOTES.txt at all + # (measured on the CI-pinned v3.15.4: `--show-only templates/NOTES.txt` + # answers "could not find template"). The shell half therefore renders it + # with `helm install --dry-run=client`, which works with no cluster. + # + # WHY IT IS WORTH A FOURTH ASSERTION. NOTES is the first thing anyone sees + # after an install, and it was half-routed seven lines apart: L6 printed + # `rel-jobs-manager` while L13 printed `zzoverride-jobs-manager`, under one + # override. That is exactly the mixed render this guard's own rationale calls + # worse than a badly-named release, in the one output an operator reads + # (Asad, review of backend#2626). + if notes_override and notes_default: + try: + over_txt = strip_ansi(open(notes_override, encoding="utf-8").read()) + def_txt = strip_ansi(open(notes_default, encoding="utf-8").read()) + except OSError as exc: + print(f" [ERROR] could not read the rendered NOTES ({exc}) — not checked.") + return 1 + if not tok.search(def_txt): + # Vacuity guard: if the DEFAULT notes never mention the release name, + # "the override notes do not" is true of an empty file. print( - " [ERROR] no on-disk path is release-scoped, so 'none followed the " - "override' proves nothing. Did the release-scoped paths disappear?" + " [ERROR] the default NOTES never mentions the release name, so the " + "override check below proves nothing. Did NOTES stop naming resources?" ) fail = True - else: + # The instance-label line is the one legitimate carrier, and it is + # allowlisted by its own text rather than by a path — free text has no + # path. Narrow on purpose: only a line that is showing the operator the + # selector `app.kubernetes.io/instance=`. + offenders = [ + ln.strip() + for ln in over_txt.splitlines() + if tok.search(ln) and "app.kubernetes.io/instance=" not in ln + ] + if offenders: + fail = True print( - f" [OK] {len(scoped)} release-scoped path(s) kept the release name; " - f"{len(paths) - len(scoped)} unscoped path(s) untouched" + f" [ERROR] {len(offenders)} NOTES line(s) print the release name under " + f"fullnameOverride={ovr!r}, so the install message names resources that " + f"do not exist:" ) + for ln in offenders[:10]: + print(f" {ln[:110]}") + else: + print(" [OK] NOTES prints no stale release name under the override") + else: + # NOT SILENTLY SKIPPED. A guard that quietly checks three things when it + # documents four is the shape this whole file exists to prevent. + print(" [ERROR] no rendered NOTES was passed, so NOTES was not checked at all.") + fail = True return 1 if fail else 0 diff --git a/scripts/tests/pyyaml-preflight.bats b/scripts/tests/pyyaml-preflight.bats index b6c9da12..90d5bdc1 100644 --- a/scripts/tests/pyyaml-preflight.bats +++ b/scripts/tests/pyyaml-preflight.bats @@ -137,18 +137,37 @@ def snippet_ok(src): return imports <= guarded guards, offenders = [], [] +# `.py` IS IN THE FILTER, and it was the gap that let this class re-open. +# `extract_python` finds python EMBEDDED in shell — heredocs and `python3 -c`. +# The moment a guard's python moves into a standalone sidecar, there is no +# heredoc to find, and a filter of `.sh`/`.bats` never opens the file at all: the +# first `.py` sidecar in this tree (`fullname_override_assertions.py`, +# backend#2626) escaped the rule entirely, which is exactly the shape of "a check +# that is not connected to what it claims to check" this suite exists to stop. +# Every future sidecar would have got the same free pass. +# +# A sidecar needs no extraction — the whole file IS the snippet — so it is fed to +# `snippet_ok` directly. for name in sorted(os.listdir(tests_dir)): - if not (name.endswith(".sh") or name.endswith(".bats")): + if not (name.endswith(".sh") or name.endswith(".bats") or name.endswith(".py")): continue if name == self_name: # this enforcer is not itself a guard-under-test continue - lines = open(os.path.join(tests_dir, name), encoding="utf-8", errors="replace").read().splitlines() - yaml_snips = [s for s in extract_python(lines) if IMPORT_YAML.search(s)] + body = open(os.path.join(tests_dir, name), encoding="utf-8", errors="replace").read() + lines = body.splitlines() + if name.endswith(".py"): + yaml_snips = [body] if IMPORT_YAML.search(body) else [] + else: + yaml_snips = [s for s in extract_python(lines) if IMPORT_YAML.search(s)] raw_import = any(IMPORT_YAML.match(l) for l in lines) if not (raw_import or yaml_snips): # not a yaml-importing guard continue guards.append(name) - if has_shell_gate(lines): # refused at the shell level, before python — accept + # A SIDECAR HAS NO SHELL TO GATE IT. `has_shell_gate` looks for a refusal in + # the surrounding script; in a `.py` file the same text would be a comment or + # a string, so honouring it here would exempt exactly the files this filter + # was extended to cover. + if not name.endswith(".py") and has_shell_gate(lines): continue if not yaml_snips: # import present in the file but not captured — cannot verify offenders.append(name); continue From ea6568dcba7bdcb863aa293c6722062a73176d2c Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 11:26:29 +0200 Subject: [PATCH 03/22] fix(chart): refuse a fullnameOverride rename of a LIVE release (backend#2626) Bugbot High, and it is a data-loss-shaped bug rather than a naming one. fullnameOverride now routes tracebloc.secretName. That is correct for an INSTALL and unsafe for a RENAME: setting the override on a release that already exists moves the Secret name, so the lookup at the top of secrets.yaml misses and four of the six credentials fall to their tier-3 randAlphaNum and are MINTED FRESH. The MySQL PVC is keep-ed and still holds the old ones: helm upgrade -> STATUS: deployed mysql -> ERROR 1045 (28000): Access denied A successful upgrade that leaves the database unopenable, with no warning at any layer. The completeness guard this PR adds could not see it -- every rendered NAME follows the override exactly as designed; what did not follow was the DATA. secrets.yaml now looks for the Secret under the name it would have had WITHOUT the override. Present, plus an override that differs, means a live release is being renamed, and it fails with the migrate-deliberately remedy. A fresh install with an override, and a release that always had one, both see no old-name Secret and are untouched. MEASURED against a live cluster, not reasoned about -- k3d + `--dry-run=server`, which unlike `helm template` actually performs lookup: override + pre-existing un-overridden Secret -> REFUSED, by the named message override, no such Secret (fresh install) -> silent no override at all -> silent the dangerous case re-run after the controls -> still REFUSED The assertions go in client-credentials-have-a-secret-tier.sh rather than the chart suite, for that file own structural reason: this is a lookup, so helm-unittest renders it away. Three assertions -- the refusal exists, it derives the UN-overridden name from .Release.Name, and it gates on the two names differing. The middle one is the correctness of the whole thing: keyed on the overridden name it would compare a name against itself, never fire, and still read as a guard. Collapsed-run floor raised 13 -> 16. 3 mutations, all reddening, every anchor asserted applied. 631 chart tests pass, drift 35/35, completeness guard green, shellcheck clean. Co-Authored-By: Claude Opus 4.8 --- client/templates/secrets.yaml | 35 +++++++++++++++ .../client-credentials-have-a-secret-tier.sh | 45 ++++++++++++++++++- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/client/templates/secrets.yaml b/client/templates/secrets.yaml index a8446c68..5dbda9cf 100644 --- a/client/templates/secrets.yaml +++ b/client/templates/secrets.yaml @@ -6,6 +6,41 @@ */ -}} {{- $secretName := include "tracebloc.secretName" . -}} {{- $existingSecret := (lookup "v1" "Secret" .Release.Namespace $secretName) -}} +{{- /* + REFUSE A RENAME OF A LIVE RELEASE (Bugbot, High, on client#911). + + `fullnameOverride` now routes `tracebloc.secretName`, which is correct for an + INSTALL and unsafe for a RENAME. Setting the override on a release that already + exists moves the Secret's name, so the `lookup` above misses, and four of the six + credentials below fall to their tier-3 `randAlphaNum` and are MINTED FRESH. The + MySQL PVC is `keep`'d and still holds the old ones, so: + + helm upgrade -> STATUS: deployed + mysql -> ERROR 1045 (28000): Access denied + + A successful upgrade that leaves the database unopenable, with no warning at any + layer. The completeness guard this PR adds cannot see it: every rendered name + follows the override exactly as intended -- the bug is that the DATA did not. + + Detected by looking for the Secret under the name it would have had WITHOUT the + override. Present + an override that differs = a live release being renamed. + + fresh install with an override old name absent -> no refusal + release always installed with one old name absent -> no refusal + release renamed mid-life old name PRESENT -> refused, by name + + DRY-RUN, stated rather than discovered: `lookup` is empty under `helm template`, + `--dry-run` and every client-side renderer, so this refusal is inert there for the + same reason tier 2 above is. It is a live-cluster guard and cannot be otherwise -- + the question it asks is "what is already in this namespace". The chart-test tier + therefore cannot exercise it; `scripts/tests/` covers the reachable half. +*/ -}} +{{- $unoverriddenSecretName := printf "%s-secrets" .Release.Name -}} +{{- if ne $secretName $unoverriddenSecretName -}} +{{- if (lookup "v1" "Secret" .Release.Namespace $unoverriddenSecretName) -}} +{{- fail (printf "fullnameOverride is set to %q, but release %q already has a Secret named %q in namespace %q. Renaming a LIVE release would move the Secret name, so the credentials below would be re-minted while the kept MySQL PVC still holds the old ones -- the upgrade would succeed and the database would refuse every login. fullnameOverride is an INSTALL-TIME choice. To adopt it on this release, migrate the data deliberately: back up, uninstall, reinstall under the new name and restore. To keep this release as it is, drop fullnameOverride." .Values.fullnameOverride .Release.Name $unoverriddenSecretName .Release.Namespace) -}} +{{- end -}} +{{- end -}} {{- /* Platform client credentials (backend#2571). Issued by the backend, NOT generated here — so tier 3 is a hard failure rather than `randAlphaNum`, diff --git a/scripts/tests/client-credentials-have-a-secret-tier.sh b/scripts/tests/client-credentials-have-a-secret-tier.sh index 67eed09f..6597b12e 100755 --- a/scripts/tests/client-credentials-have-a-secret-tier.sh +++ b/scripts/tests/client-credentials-have-a-secret-tier.sh @@ -157,12 +157,53 @@ for cred in clientId clientPassword; do done +# --- the RENAME refusal, same unreachable class (Bugbot, High, on client#911) --- +# +# `fullnameOverride` routes tracebloc.secretName, which is right for an install and +# unsafe for a rename: the lookup above misses under the new name, four credentials +# fall to their randAlphaNum tier and are minted fresh, and the kept MySQL PVC still +# holds the old ones. `helm upgrade` reports deployed and the database refuses every +# login. secrets.yaml refuses that case. +# +# It belongs in THIS file rather than the chart suite for the same structural reason +# as everything above: it is a `lookup`, so helm-unittest renders it away. Verified +# against a live cluster instead (k3d, `--dry-run=server`, 2026-08-31): refused with +# the named message when the un-overridden Secret exists, silent on a fresh install +# with an override, and silent with no override at all. +code_all="$(grep -v '^[[:space:]]*#' "$TPL" 2>/dev/null || true)" + +# 1. the refusal exists at all. +if ! grep -qF 'fullnameOverride is set to' <<<"$code_all"; then + fail "secrets.yaml no longer refuses a rename of a live release. Setting + fullnameOverride on an existing release re-mints the generated credentials while + the kept MySQL PVC holds the old ones: upgrade succeeds, database refuses login." +fi +ok + +# 2. it keys on the UN-OVERRIDDEN name. This is the whole correctness of it: keying +# the second lookup on tracebloc.secretName -- the overridden name -- would make +# it compare a name against itself and never fire, while reading as a guard. +if ! grep -qE 'printf "%s-secrets" \.Release\.Name' <<<"$code_all"; then + fail "the rename refusal no longer derives the UN-overridden Secret name from + .Release.Name. Keyed on the overridden name it can never fire, because that is + the name it is being compared against." +fi +ok + +# 3. it only fires when the two names DIFFER, so an ordinary release -- where they +# are equal -- is never refused. Without this the guard would break every install. +if ! grep -qE 'ne \$secretName \$unoverriddenSecretName' <<<"$code_all"; then + fail "the rename refusal no longer gates on the two names differing, so it would + fire on releases that set no override at all." +fi +ok + if [ "$fails" -ne 0 ]; then echo "client-credentials-have-a-secret-tier: $fails failure(s) across $checks assertion(s)" >&2 exit 1 fi -if [ "$checks" -lt 13 ]; then - echo "client-credentials-have-a-secret-tier: only $checks assertion(s) ran; expected 13+. +if [ "$checks" -lt 16 ]; then + echo "client-credentials-have-a-secret-tier: only $checks assertion(s) ran; expected 16+. A collapsed run must not report success (rule 3)." >&2 exit 1 fi From fae5441b6c87e0439306091c2a1e2ae4f04bfeba Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 11:54:14 +0200 Subject: [PATCH 04/22] fix(chart): key the re-mint refusal on the persisted data, not on a name (backend#2626) Co-Authored-By: Claude Opus 4.8 --- client/templates/secrets.yaml | 64 +++++++++++++----- .../client-credentials-have-a-secret-tier.sh | 66 ++++++++++++------- 2 files changed, 93 insertions(+), 37 deletions(-) diff --git a/client/templates/secrets.yaml b/client/templates/secrets.yaml index 5dbda9cf..96ed178c 100644 --- a/client/templates/secrets.yaml +++ b/client/templates/secrets.yaml @@ -22,24 +22,58 @@ layer. The completeness guard this PR adds cannot see it: every rendered name follows the override exactly as intended -- the bug is that the DATA did not. - Detected by looking for the Secret under the name it would have had WITHOUT the - override. Present + an override that differs = a live release being renamed. + KEYED ON THE PERSISTED DATA, NOT ON A NAME (Arturo + Bugbot, re-review of + ea6568dc). The first version of this guard looked for the Secret under the name + the release would have had WITHOUT the override, and that name is a PROXY for + the thing that causes the lockout. It caught one of three rename directions: - fresh install with an override old name absent -> no refusal - release always installed with one old name absent -> no refusal - release renamed mid-life old name PRESENT -> refused, by name + none -> override A live Secret is `-secrets` -> caught + override A -> B live Secret is `A-secrets`, so the + `-secrets` probe missed -> MISSED + override A -> none the two names are equal, so the + `ne` gate skipped the body entirely -> MISSED - DRY-RUN, stated rather than discovered: `lookup` is empty under `helm template`, - `--dry-run` and every client-side renderer, so this refusal is inert there for the - same reason tier 2 above is. It is a live-cluster guard and cannot be otherwise -- - the question it asks is "what is already in this namespace". The chart-test tier - therefore cannot exercise it; `scripts/tests/` covers the reachable half. + Both missed directions re-mint against the same kept `mysql-pvc` and produce the + same silent lockout, so the guard read as closed while two thirds of the class + was open. + + THE INVARIANT IS ABOUT STATE: persisted MySQL credentials exist, and the Secret + under the CURRENT EFFECTIVE name does not. `tracebloc.mysqlPvc` is the constant + `mysql-pvc` -- it never follows the override and carries + `helm.sh/resource-policy: keep` -- so its presence IS "there is a database here + whose credentials are already baked in". `$existingSecret` above is already the + current effective name. Two facts, no name arithmetic, and every direction falls + out of it: + + first install no PVC -> no refusal + fresh install WITH an override no PVC -> no refusal + ordinary upgrade PVC + Secret present -> no refusal + none -> A, A -> B, A -> none PVC present, Secret gone -> REFUSED + reinstall over a kept PVC PVC present, Secret gone -> REFUSED + + That last row is a case name-keying could not reach at all, and Arturo named it: + uninstall drops the Secret and keeps the PVC, so a reinstall under any name + re-mints against retained data. + + IT ALSO FIRES ON A DELETED SECRET, deliberately. An operator who removes the + Secret expecting regeneration is walking into the same lockout; refusing names it + before the fact instead of after. + + THE REMEDY IS CHEAP, which is why it leads the message. The credentials live in + the Secret and the data lives in `mysql-pvc`, whose name never moves -- so a + rename needs the Secret copied to the new effective name and nothing else. The + old advice (back up, uninstall, reinstall, restore) was heavier than the problem. + + DRY-RUN, stated rather than discovered: both `lookup`s are empty under `helm + template`, `--dry-run` and every client-side renderer, so this refusal is inert + there for the same reason tier 2 above is. It is a live-cluster guard and cannot + be otherwise -- the question it asks is "what is already in this namespace". The + chart-test tier therefore cannot exercise it; `scripts/tests/` covers the + reachable half. */ -}} -{{- $unoverriddenSecretName := printf "%s-secrets" .Release.Name -}} -{{- if ne $secretName $unoverriddenSecretName -}} -{{- if (lookup "v1" "Secret" .Release.Namespace $unoverriddenSecretName) -}} -{{- fail (printf "fullnameOverride is set to %q, but release %q already has a Secret named %q in namespace %q. Renaming a LIVE release would move the Secret name, so the credentials below would be re-minted while the kept MySQL PVC still holds the old ones -- the upgrade would succeed and the database would refuse every login. fullnameOverride is an INSTALL-TIME choice. To adopt it on this release, migrate the data deliberately: back up, uninstall, reinstall under the new name and restore. To keep this release as it is, drop fullnameOverride." .Values.fullnameOverride .Release.Name $unoverriddenSecretName .Release.Namespace) -}} -{{- end -}} +{{- $mysqlDataPresent := (lookup "v1" "PersistentVolumeClaim" .Release.Namespace (include "tracebloc.mysqlPvc" .)) -}} +{{- if and $mysqlDataPresent (not $existingSecret) -}} +{{- fail (printf "release %q in namespace %q already has MySQL data (PersistentVolumeClaim %q, retained by helm.sh/resource-policy: keep), but there is no Secret named %q -- the name this render resolves to. The credentials below would therefore be re-minted while that database still holds the old ones: the upgrade would report deployed and MySQL would refuse every login. FIX: copy the existing credentials to the name this render wants, then re-run -- kubectl -n %s get secret -o json | jq '.metadata.name=\"%s\" | del(.metadata.uid,.metadata.resourceVersion,.metadata.creationTimestamp,.metadata.ownerReferences)' | kubectl apply -f - . The MySQL PVC is named %q and never follows fullnameOverride, so nothing else has to move. ALTERNATIVELY: put fullnameOverride back to the value this release was last rendered with, so the existing Secret matches again." .Release.Name .Release.Namespace (include "tracebloc.mysqlPvc" .) $secretName .Release.Namespace $secretName (include "tracebloc.mysqlPvc" .)) -}} {{- end -}} {{- /* Platform client credentials (backend#2571). Issued by the backend, NOT diff --git a/scripts/tests/client-credentials-have-a-secret-tier.sh b/scripts/tests/client-credentials-have-a-secret-tier.sh index 6597b12e..bda277ec 100755 --- a/scripts/tests/client-credentials-have-a-secret-tier.sh +++ b/scripts/tests/client-credentials-have-a-secret-tier.sh @@ -166,35 +166,57 @@ done # login. secrets.yaml refuses that case. # # It belongs in THIS file rather than the chart suite for the same structural reason -# as everything above: it is a `lookup`, so helm-unittest renders it away. Verified -# against a live cluster instead (k3d, `--dry-run=server`, 2026-08-31): refused with -# the named message when the un-overridden Secret exists, silent on a fresh install -# with an override, and silent with no override at all. +# as everything above: it is a `lookup`, so helm-unittest renders it away. +# +# THE FIRST VERSION OF THESE ASSERTIONS PINNED THE WRONG DESIGN, and that is worth +# recording because they passed while two thirds of the class was open. They asserted +# the refusal "keys on the UN-OVERRIDDEN name" and called that "the whole correctness +# of it". It was not: `-secrets` is a PROXY for the lockout, and it missed +# override A -> B (the probe looks for a name that was never live) and override A -> +# none (the `ne` gate makes the body unreachable). Both re-mint against the same kept +# PVC. A test that pins a proxy cements it -- Arturo's re-review of ea6568dc caught +# exactly that, and it is why assertion 4 below now forbids the name key outright. code_all="$(grep -v '^[[:space:]]*#' "$TPL" 2>/dev/null || true)" # 1. the refusal exists at all. -if ! grep -qF 'fullnameOverride is set to' <<<"$code_all"; then - fail "secrets.yaml no longer refuses a rename of a live release. Setting - fullnameOverride on an existing release re-mints the generated credentials while - the kept MySQL PVC holds the old ones: upgrade succeeds, database refuses login." +if ! grep -qF 'already has MySQL data' <<<"$code_all"; then + fail "secrets.yaml no longer refuses to re-mint credentials over a live database. + Any render that resolves to a Secret name the namespace does not have -- adding, + changing or dropping fullnameOverride, or reinstalling over a kept PVC -- re-mints + the generated credentials while the retained MySQL PVC holds the old ones: + upgrade succeeds, database refuses every login." +fi +ok + +# 2. it probes the PERSISTED DATA. This is the correctness of it: the MySQL PVC is +# `mysql-pvc`, a constant that never follows the override and is retained by +# resource-policy: keep, so its presence is what "there is already a database +# here" means. Keying on any release-derived NAME instead is what missed two of +# the three rename directions. +if ! grep -qE 'lookup "v1" "PersistentVolumeClaim" \.Release\.Namespace \(include "tracebloc\.mysqlPvc"' <<<"$code_all"; then + fail "the refusal no longer probes the MySQL PVC, so it is back to inferring the + lockout from a name. A name-keyed probe cannot see override A -> override B (it + looks for a name that was never live) or a dropped override (the names are equal + and the gate never opens), and both re-mint against retained data." fi ok -# 2. it keys on the UN-OVERRIDDEN name. This is the whole correctness of it: keying -# the second lookup on tracebloc.secretName -- the overridden name -- would make -# it compare a name against itself and never fire, while reading as a guard. -if ! grep -qE 'printf "%s-secrets" \.Release\.Name' <<<"$code_all"; then - fail "the rename refusal no longer derives the UN-overridden Secret name from - .Release.Name. Keyed on the overridden name it can never fire, because that is - the name it is being compared against." +# 3. it gates on the CURRENT EFFECTIVE Secret being absent, so an ordinary upgrade -- +# PVC and Secret both present -- is never refused. Without this the guard would +# fire on every upgrade of every release. +if ! grep -qE 'if and \$mysqlDataPresent \(not \$existingSecret\)' <<<"$code_all"; then + fail "the refusal no longer gates on the current effective Secret being ABSENT, so + it would fire on ordinary upgrades where nothing is being renamed at all." fi ok -# 3. it only fires when the two names DIFFER, so an ordinary release -- where they -# are equal -- is never refused. Without this the guard would break every install. -if ! grep -qE 'ne \$secretName \$unoverriddenSecretName' <<<"$code_all"; then - fail "the rename refusal no longer gates on the two names differing, so it would - fire on releases that set no override at all." +# 4. THE OLD PROXY MUST STAY GONE. Re-introducing the name comparison reopens the two +# directions it could not see, and it would do so while every other assertion here +# still passed -- which is precisely how ea6568dc shipped looking complete. +if grep -qE '\$unoverriddenSecretName|ne \$secretName' <<<"$code_all"; then + fail "the refusal is keyed on a release-derived Secret NAME again. That proxy misses + override A -> override B and a dropped override; the invariant is 'persisted MySQL + data exists and the Secret under the current effective name does not'." fi ok @@ -202,8 +224,8 @@ if [ "$fails" -ne 0 ]; then echo "client-credentials-have-a-secret-tier: $fails failure(s) across $checks assertion(s)" >&2 exit 1 fi -if [ "$checks" -lt 16 ]; then - echo "client-credentials-have-a-secret-tier: only $checks assertion(s) ran; expected 16+. +if [ "$checks" -lt 17 ]; then + echo "client-credentials-have-a-secret-tier: only $checks assertion(s) ran; expected 17+. A collapsed run must not report success (rule 3)." >&2 exit 1 fi From a043e5230bbdb850c36e3c7c44bc33357881ce0a Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 11:54:15 +0200 Subject: [PATCH 05/22] test(chart): hold the CLASS behind the Secret-lookup finding, not just the instance (backend#2626) fae5441 fixed the instance, and better than my own attempt twice over. I had un-routed `tracebloc.secretName` so a rename could not lose the credentials - UNTESTED, because `lookup` is inert under every client-side renderer I have. ea6568d refused the dangerous case and MEASURED it with `--dry-run=server`, the only way to exercise a lookup; fae5441 then re-keyed that refusal on the persisted DATA after Arturo showed name-keying caught one of three rename directions and missed reinstall-over-a-kept-PVC entirely. That work is on the branch. Mine is discarded rather than layered on: with the Secret un-routed, the refusal would compare a name against itself and fire on every override-set upgrade. What nothing covers is the CLASS. A Secret `lookup` keyed on a name that follows the override misses on a rename, and a missed lookup is not an error - it silently takes the last resolution tier. This chart has TWO such sites: secrets.yaml a `fail` reached by the miss (fae5441, held by client-credentials-have-a-secret-tier.sh) tracebloc.telemetryTokenPresent safe ONLY because it ORs a lookup on the legacy fixed name - and nothing asserted that So the second member was one edit from the same silent shape with no check. Assertion 5 requires every routed Secret lookup to carry one of the two mitigations in its own file. Which mitigation a site needs is not this assertion`s call - it is a text-level check and could not have established what `--dry-run=server` did. It only requires that one is still there. THE REFUSAL IS DETECTED ON THE INVARIANT, NOT THE ARITHMETIC, and that is the lesson from writing this against a moving target. My first detector matched `printf "%s-secrets" .Release.Name` - the shape the refusal had while it keyed on the un-overridden name. fae5441 improved the refusal and my detector would have reported it as NO refusal. A detector that breaks when the thing it guards gets better is worse than none: it pushes back toward the shape it was written against. It now keys on the property that must hold however the other half is computed - a miss on THAT lookup reaches a `fail`. Which names follow the override is DERIVED transitively, closing `include` over the helper definitions, so a helper added tomorrow that wraps `fullname` is covered without anyone remembering. Fails closed three ways: a define parser that matches nothing, zero routed sites in a chart known to have two, and zero Secret lookups at all. Mutation-proved four ways, each anchor asserted applied: the data-keyed refusal removed -> flagged the refusal no longer negates that lookup -> flagged telemetry legacy-name fallback removed -> flagged (the unguarded member) routed-var detection stops resolving -> fail closed VERIFIED on helm v3.15.4 (the CI pin): make drift 35/35; helm unittest 631/631 in 36 suites; client-credentials-have-a-secret-tier 17 assertions OK; shellcheck clean. No chart files touched, so no version bump. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 --- .../tests/fullname-override-completeness.sh | 16 ++ scripts/tests/fullname_override_assertions.py | 206 ++++++++++++++++++ 2 files changed, 222 insertions(+) diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh index 79df22bb..73e3a5fe 100755 --- a/scripts/tests/fullname-override-completeness.sh +++ b/scripts/tests/fullname-override-completeness.sh @@ -24,6 +24,22 @@ # backend#2620 re-introduced by the fix for backend#2621. # 4. NOTES The install message names no stale release. Its own render, because # `helm template` does not emit NOTES.txt at all. +# 5. LOOKUPS Every Secret `lookup` keyed on a name that FOLLOWS the override +# carries a mitigation. THE CLASS behind Bugbot's High on this PR: a +# lookup that misses is not an error, it silently takes the last +# resolution tier -- for `secrets.yaml` that meant minting a new +# password while the fixed-name `mysql-pvc` datadir kept the old, so +# the upgrade succeeded and the database refused every login. +# +# THE INSTANCE IS FIXED IN `secrets.yaml` (a refusal keyed on the +# un-overridden name, measured against a live cluster with +# `--dry-run=server`, and held by +# `client-credentials-have-a-secret-tier.sh`). This assertion holds +# the CLASS, which has a second member nothing checked: +# `telemetryTokenPresent` is safe only because it ORs a lookup on the +# legacy fixed name. Two mitigation shapes are accepted -- a refusal +# or a fallback lookup -- and which one a site needs is not this +# assertion'"'"'s call to make; it only requires that one is still there. # # (2) AND (3) SHARE ONE CLASSIFIER, and that is what closes the gap the first # version had: (2) read doc-root `metadata.name` only, so every name-REFERENCE diff --git a/scripts/tests/fullname_override_assertions.py b/scripts/tests/fullname_override_assertions.py index 965d7ec2..3821422b 100644 --- a/scripts/tests/fullname_override_assertions.py +++ b/scripts/tests/fullname_override_assertions.py @@ -125,6 +125,205 @@ def classify(doc, path, val, rel, ns, envs): return None +# -------------------------------------------------------------------------- +# Assertion 5 — the CLASS behind the credential-Secret finding +# -------------------------------------------------------------------------- +# +# THE INSTANCE IS FIXED ELSEWHERE, AND THIS IS THE CLASS. `secrets.yaml` resolves +# credentials as "explicit, else THE VALUE IN THE LIVE SECRET, else randAlphaNum", +# and the middle tier is a `lookup` keyed on a name that follows the override. A +# lookup that misses is not an error -- it silently takes the last tier, which for +# a credential means minting a new password while the fixed-name `mysql-pvc` +# datadir keeps the old one. That was Bugbot's High on this PR, and it is fixed in +# `secrets.yaml` by refusing a rename of a live release. +# +# But it is a CLASS, and this chart has TWO members: +# +# secrets.yaml mitigated by a `fail` keyed on the +# un-overridden name (that refusal) +# tracebloc.telemetryTokenPresent mitigated by OR-ing a lookup on the LEGACY +# fixed name -- and nothing asserted that +# +# So the second member was one edit away from the same silent shape, with no +# check. This holds both: a Secret lookup keyed on a name that follows the +# override must carry one of the two mitigations, in its own file. +# +# WHICH NAMES FOLLOW THE OVERRIDE IS DERIVED, INCLUDING THROUGH HELPERS -- closed +# over `include` across the template sources rather than listed here, so a helper +# added tomorrow that wraps `fullname` is covered without anyone remembering. +# +# IT DOES NOT SECOND-GUESS WHICH MITIGATION IS RIGHT. `secrets.yaml`'s refusal was +# measured against a live cluster with `--dry-run=server`, which is the only way to +# exercise a `lookup` at all; this assertion is a text-level check and could not +# have established that. It only requires that a mitigation is still there. + +TEMPLATES = "client/templates" +_DEFINE = re.compile(r'{{-?\s*define\s+"([^"]+)"\s*-?}}(.*?){{-?\s*end\s*-?}}', re.S) +_INCLUDE = re.compile(r'include\s+"([^"]+)"') +_SECRET_LOOKUP = re.compile(r'lookup\s+"v1"\s+"Secret"\s+\S+\s+([^\n)]*\)?)') + + +def helpers_following_the_override(sources): + """Helper names whose rendered value contains `tracebloc.fullname`. + + Transitive: a helper that includes a helper that includes `fullname` follows + the override too. Closed by iteration rather than recursion so a cyclic + include cannot hang the guard. + """ + bodies = {} + for text in sources: + for name, body in _DEFINE.findall(text): + bodies[name] = body + following = {"tracebloc.fullname"} + changed = True + while changed: + changed = False + for name, body in bodies.items(): + if name in following: + continue + if any(inc in following for inc in _INCLUDE.findall(body)): + following.add(name) + changed = True + return following + + +def _mitigations(text, routed_vars): + """Which mitigation shapes this file carries, as a set of labels. + + TWO SHAPES, BOTH REAL, and neither is a substitute for judgement about which + one a given site needs: + + "refusal" a MISS on the routed lookup leads to `fail` — however the other + half of the condition is computed + "fallback" a second lookup in the same expression, on a name that does not + follow the override — `telemetryTokenPresent`'s legacy name + + THE REFUSAL IS DETECTED ON THE INVARIANT, NOT ON THE ARITHMETIC. The first cut + matched `printf "%s-secrets" .Release.Name` — the shape the refusal happened to + have when it keyed on the UN-OVERRIDDEN NAME. Arturo then showed that name + keying caught only one of three rename directions and missed reinstall-over-a- + kept-PVC entirely, so the refusal was re-keyed on the persisted DATA (the + retained `mysql-pvc`) — a strictly better guard that my detector would have + reported as no guard at all. + + A detector that breaks when the thing it guards is IMPROVED is worse than + none: it pushes back toward the shape it happened to be written against. So it + now keys on the property that has to hold however the other half is computed — + a miss on THIS lookup must reach a `fail`: + + $existingSecret := (lookup "v1" "Secret" … $secretName) + … + if and $mysqlDataPresent (not $existingSecret) <- the miss + fail … <- the refusal + + `routed_vars` are the variables assigned from a routed Secret lookup, so the + negation has to be of THAT lookup's result rather than of any variable. + """ + out = set() + lines = text.splitlines() + for var in routed_vars: + neg = re.compile(rf"(not|empty)\s+\${re.escape(var)}\b") + for i, line in enumerate(lines): + if not neg.search(line): + continue + # `fail` within the guarded block. A small window rather than a full + # parse: the refusal is the first statement of the branch in every + # spelling this chart uses, and a wider window would start accepting + # an unrelated `fail` further down the file. + if any("fail " in nxt or "fail(" in nxt for nxt in lines[i : i + 4]): + out.add("refusal") + for line in lines: + if line.split("#", 1)[0].count("lookup ") >= 2: + out.add("fallback") + return out + + +def assert_lookup_keys(): + """`(ok, messages)` — every Secret lookup on a routed name is mitigated.""" + files = sorted( + os.path.join(TEMPLATES, f) + for f in os.listdir(TEMPLATES) + if f.endswith(".yaml") or f.endswith(".tpl") + ) + if not files: + return False, [f" [ERROR] no templates found under {TEMPLATES} — nothing checked."] + sources = {f: open(f, encoding="utf-8").read() for f in files} + following = helpers_following_the_override(sources.values()) + if len(following) < 2: + # FAIL CLOSED: `fullname` alone means the define regex matched nothing, so + # every name would read as "does not follow the override" and this + # assertion would pass vacuously. + return False, [ + " [ERROR] resolved no helpers as following the override, so every " + "lookup would read as safe. The define parser matched nothing." + ] + + msgs, lookups, routed_sites = [], 0, 0 + for path, text in sources.items(): + vars_ = dict( + re.findall(r'\$(\w+)\s*:?=\s*\(?\s*include\s+"([^"]+)"', text) + ) + # Variables assigned from a Secret lookup whose NAME follows the + # override — the ones a refusal has to negate. + routed_vars = set() + for m in re.finditer( + r'\$(\w+)\s*:?=\s*\(?\s*lookup\s+"v1"\s+"Secret"\s+\S+\s+([^\n)]*\)?)', text + ): + expr = m.group(2) + referenced = set(_INCLUDE.findall(expr)) + for v in re.findall(r"\$(\w+)", expr): + if v in vars_: + referenced.add(vars_[v]) + if referenced & following: + routed_vars.add(m.group(1)) + have = _mitigations(text, routed_vars) + for line in text.splitlines(): + code = line.split("#", 1)[0] + if "lookup " not in code or '"Secret"' not in code: + continue + lookups += 1 + routed = [] + for expr in _SECRET_LOOKUP.findall(code): + referenced = set(_INCLUDE.findall(expr)) + for v in re.findall(r"\$(\w+)", expr): + if v in vars_: + referenced.add(vars_[v]) + if referenced & following: + routed.append(expr.strip()) + if not routed: + continue + routed_sites += 1 + if not have: + msgs.append( + f" [ERROR] {path}: a Secret lookup keys on a name that follows " + f"fullnameOverride ({', '.join(routed)}) and the file carries " + f"NEITHER mitigation — no `fail` reached by a MISS on that " + f"lookup, and no fallback lookup on a name the override cannot " + f"move. A missed lookup is not an error: it silently takes the " + f"last resolution tier, which for a credential means minting a " + f"new one while the kept datadir holds the old." + ) + if not lookups: + return False, [ + " [ERROR] found ZERO Secret lookups in the templates — the matcher " + "sees nothing, so this assertion proves nothing." + ] + if not routed_sites: + # Also a finding: this chart HAS routed lookups, so zero means the + # "follows the override" resolution stopped resolving. + return False, [ + " [ERROR] found ZERO Secret lookups keyed on a routed name, but this " + "chart has at least two. The name resolution has stopped working, so " + "every site would read as safe." + ] + if msgs: + return False, msgs + return True, [ + f" [OK] all {routed_sites} routed Secret lookup(s) of {lookups} carry a " + f"mitigation (refusal or fallback)" + ] + + def strip_ansi(text): return re.sub(r"\033\[[0-9;]*m", "", text) @@ -320,6 +519,13 @@ def main() -> int: print(" [ERROR] no rendered NOTES was passed, so NOTES was not checked at all.") fail = True + # --- 5. the class behind the credential-Secret finding ------------------- + ok, msgs = assert_lookup_keys() + for m in msgs: + print(m) + if not ok: + fail = True + return 1 if fail else 0 From 41eb4bb28cbac633af8bafbba60c765b2b8d96f4 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 12:37:39 +0200 Subject: [PATCH 06/22] fix(installer): read the client Secret under fullnameOverride, and parse helper bodies whole (backend#2626) Two Bugbot findings on a043e52, both Medium, both real. 1. THE INSTALLER MISSES AN OVERRIDDEN SECRET NAME. `tracebloc.secretName` follows `fullnameOverride`, so on a release installed with one the Secret is `-secrets`. `detect_installed_client` and `Get-InstalledClientInfo` read `-secrets`, find nothing, and a live client whose id lives only in the Secret (the backend#2571 shape the chart now recommends) reads as UNIDENTIFIABLE - so `diagnose` and `upgrade` treat it as having no id. The override is already in the values both callers have open, so the effective prefix costs one more read of the same file rather than a second API call. Absent -> the release name, which is the chart`s own `default .Release.Name .Values.fullnameOverride`. Four tests, two per language, and the CONTROL is the load-bearing half: without it the fix is satisfied by always using the override key, which would break every ordinary release. Mutation-proved in both directions, in both languages - reverting to the release name fails the override case, always using the override fails the control. 2. MY DEFINE PARSER STOPPED AT THE FIRST `end`. `{{- define "x" -}}(.*?){{- end -}}` is non-greedy, so a helper containing an inner `if`/`range` closed at the INNER end and its tail was silently dropped. Measured: 28 of 55 helper bodies truncated. IT CHANGED NO ANSWER TODAY - no helper`s `tracebloc.fullname` reference happens to sit in a dropped tail, so the routed-helper closure came out at 21 either way. That is why it was worth fixing rather than noting: the guard`s coverage depended on WHERE in a helper an include happened to sit, and one edit moving an include below an `if` would silently un-route it - after which a routed Secret lookup reads as unrouted and assertion 5 stops requiring a mitigation. Replaced with a balanced parse over the `{{ }}` ACTIONS, so an `end` inside a string or comment cannot close a block early. A permanent self-test feeds it the exact shape the regex dropped - a helper whose `fullname` reference sits after an inner `if`/`end` - because the chart contains no such helper today and a check only exercisable by a bug already present arrives too late. Both halves asserted: the routed probe must be seen, the unrouted probe must not. VERIFIED on helm v3.15.4 (the CI pin): make drift 35/35 (manifest.sha256 regenerated - the two installer files are hashed there); helm unittest 631/631; Pester 776 passed / 0 failed / 15 skipped; the two new bats cases green; shellcheck -S warning -x clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 --- scripts/install-k8s.ps1 | 16 ++- scripts/lib/install-client-helm.sh | 17 ++- scripts/manifest.sha256 | 4 +- scripts/tests/fullname_override_assertions.py | 100 +++++++++++++++++- scripts/tests/install-client-helm.bats | 53 ++++++++++ scripts/tests/install-k8s.Tests.ps1 | 48 +++++++++ 6 files changed, 230 insertions(+), 8 deletions(-) diff --git a/scripts/install-k8s.ps1 b/scripts/install-k8s.ps1 index 1d7df6f0..75754be0 100644 --- a/scripts/install-k8s.ps1 +++ b/scripts/install-k8s.ps1 @@ -5226,9 +5226,23 @@ function Get-InstalledClientInfo { # is a client we cannot NAME -> unidentifiable, so the guard fails # closed rather than waving through an install that re-points the # machine. Bash parity: detect_installed_client / _client_id_from_secret. + # + # THE SECRET'S NAME IS NOT ALWAYS THE RELEASE NAME (Bugbot, Medium, on + # client#911). `tracebloc.secretName` follows `fullnameOverride`, so on + # a release installed with one, `-secrets` does not exist and + # this fallback reads nothing -- a live client whose id lives only in + # the Secret then reads as UNIDENTIFIABLE. The override is in the + # values already parsed above; absent -> the release name, which is + # the chart's own `default .Release.Name .Values.fullnameOverride`. + # Bash parity: detect_installed_client's `_fno` read. $id = "" if ($null -ne $vals -and $null -ne $vals.clientId) { $id = "$($vals.clientId)".Trim() } - if (-not $id) { $id = Get-ClientIdFromSecret -Release $rel.name -Namespace $rel.namespace } + $prefix = $rel.name + if ($null -ne $vals -and $null -ne $vals.fullnameOverride) { + $fno = "$($vals.fullnameOverride)".Trim() + if ($fno) { $prefix = $fno } + } + if (-not $id) { $id = Get-ClientIdFromSecret -Release $prefix -Namespace $rel.namespace } if ($id) { $existingId = $id; $existingNs = $rel.namespace; $existingName = $rel.name; break } # No trailing `continue` here. It is the last statement of the loop # body, so it buys nothing -- and PowerShell reported it escaping as an diff --git a/scripts/lib/install-client-helm.sh b/scripts/lib/install-client-helm.sh index 20d69b00..8e1c7bcb 100644 --- a/scripts/lib/install-client-helm.sh +++ b/scripts/lib/install-client-helm.sh @@ -753,7 +753,7 @@ detect_installed_client() { INSTALLED_CLIENT_ID=""; INSTALLED_CLIENT_NS=""; INSTALLED_CLIENT_UNKNOWN=0 # No helm => nothing helm-installed here; a genuine (documented) "no client". has helm || return 0 - local _gvf _rel _ns _id _list _unreadable=0 + local _gvf _rel _ns _id _fno _list _unreadable=0 # A mktemp failure is an environment error, NOT proof of "no client here" — flag # UNKNOWN so the guards fail closed rather than skip. Fall back to a path in a # dir we own (never a predictable world-writable /tmp path under sudo) before @@ -785,7 +785,20 @@ detect_installed_client() { # clientId in its values, and reading that as "not a match" let the # one-client guard wave through an install that re-points the machine. # Fall back to where the id now lives. - [[ -z "$_id" ]] && _id="$(_client_id_from_secret "$_rel" "$_ns")" + # + # THE SECRET'S NAME IS NOT ALWAYS THE RELEASE NAME (Bugbot, Medium, on + # client#911). `tracebloc.secretName` follows `fullnameOverride`, so on a + # release installed with one, `-secrets` does not exist and this + # fallback reads nothing -- a live client with its id only in the Secret + # then reads as UNIDENTIFIABLE, and `diagnose`/`upgrade` treat it as having + # no id. + # + # The override is in the values file already open above, so the effective + # prefix costs one more read of the same file rather than a second API + # call. Empty or absent -> the release name, which is exactly the chart's + # own `default .Release.Name .Values.fullnameOverride`. + _fno="$(_extract_yaml_value "$_gvf" fullnameOverride)" + [[ -z "$_id" ]] && _id="$(_client_id_from_secret "${_fno:-$_rel}" "$_ns")" [[ -n "$_id" ]] && { INSTALLED_CLIENT_ID="$_id"; INSTALLED_CLIENT_NS="$_ns"; break; } # A client-chart release with no id in EITHER place is a client we cannot # NAME, not an absent one. Record it and keep scanning; if nothing else diff --git a/scripts/manifest.sha256 b/scripts/manifest.sha256 index 93bf6bd1..da3f45e1 100644 --- a/scripts/manifest.sha256 +++ b/scripts/manifest.sha256 @@ -9,12 +9,12 @@ b569eec2d8ffb9673da287a2a59d249a7dbc7236c98ab6a5062136bcc69a942c scripts/lib/gp 58cfe1ff05074c58ff6698984c2b906b5f2540b951d0a95d1d959d18538ceef6 scripts/lib/setup-linux.sh 4d1993695202beef1db0c87ebda7fe9481e1f8b4ea85bbb7ca02e07964d19c3b scripts/lib/cluster.sh 84ed9d9b3ab4633bfaf07b256c066ed43f96a0025ec6b1a34db23fdef75f0f62 scripts/lib/gpu-plugins.sh -7f21ee43cb5fcc5077aa6e2409a790ac83d9d39edafa5e5bb1d5ab0ffb384444 scripts/lib/install-client-helm.sh +ef8e6a05fb586a301b6ab11d3d8365d563437bbd9bd39fc381c4f86cd379c827 scripts/lib/install-client-helm.sh 615a8294b54f8745787b5ec39f8c9d479e4734a62d8a57500dbe273a2931f5bb scripts/lib/install-cli.sh ea2bbd9948ee9e31e93271e235c630ced50d746e51a5629b5622041d8a39df07 scripts/lib/provision.sh fe5d98000fcc1aab4169dba657b726f295fb18df607ded8d01ffcd0ec64260a2 scripts/lib/assess.sh 6b3164415dcd793880b972fb61897179d58097f7d288e3904f6f09f858417a15 scripts/lib/probe.sh e36c0fc3568aa5054ee6c853ced0791ad45951724fff059cff63847f5bc870e2 scripts/lib/summary.sh 1b5e0fb3c9b100672dc3f547b677524f98f0a9ed28d8dc7de0f619d2687cfc52 scripts/lib/diagnose.sh -3b52b0bdbd5f7ad9971dfea743960c474a48dd4a32473a3eee5a35839ad3c619 scripts/install-k8s.ps1 +ec8096aec5c2b65dbd317644e262fcb743ec22a434ed25b15c21085521e2cf24 scripts/install-k8s.ps1 fc6eeeb4e4114ce74566992519a90320c52434d604218a30ba8ef2c3fa266ca5 scripts/lib/telemetry.ps1 diff --git a/scripts/tests/fullname_override_assertions.py b/scripts/tests/fullname_override_assertions.py index 3821422b..13178e9f 100644 --- a/scripts/tests/fullname_override_assertions.py +++ b/scripts/tests/fullname_override_assertions.py @@ -158,9 +158,51 @@ def classify(doc, path, val, rel, ns, envs): # have established that. It only requires that a mitigation is still there. TEMPLATES = "client/templates" -_DEFINE = re.compile(r'{{-?\s*define\s+"([^"]+)"\s*-?}}(.*?){{-?\s*end\s*-?}}', re.S) _INCLUDE = re.compile(r'include\s+"([^"]+)"') _SECRET_LOOKUP = re.compile(r'lookup\s+"v1"\s+"Secret"\s+\S+\s+([^\n)]*\)?)') +#: Every `{{ … }}` action, in order. Used to find a define's BALANCED end. +_ACTION = re.compile(r"{{-?\s*(.*?)\s*-?}}", re.S) +#: Actions that open a block and therefore need their own `end`. +_OPENS = re.compile(r"^(if|range|with|block|define)\b") + + +def define_bodies(text): + """`{helper: body}`, each body closed at its BALANCED `end`. + + NOT A REGEX, and the regex it replaces was a real defect (Bugbot, Medium). + `{{- define "x" -}}(.*?){{- end -}}` is non-greedy, so a helper containing an + inner `if`/`range`/`with` closes at the INNER `end` and its tail is silently + dropped. Measured on this chart: 28 of 55 helper bodies were truncated. + + It changed no answer TODAY — no helper's `tracebloc.fullname` reference + happens to sit in a dropped tail, so the routed-helper closure came out at 21 + either way. That is exactly why it was worth fixing rather than noting: the + guard's coverage depended on WHERE in a helper an include happened to sit, and + one edit moving an include below an `if` would have silently un-routed it — + after which a routed Secret lookup reads as unrouted and assertion 5 stops + requiring a mitigation. A check that passes because it is not connected to + what it claims to check. + + Depth is counted over the ACTIONS rather than over the text, so an `end` + inside a quoted string or a comment cannot close a block early. + """ + out, stack = {}, [] + for m in _ACTION.finditer(text): + action = m.group(1) + opened = re.match(r'define\s+"([^"]+)"', action) + if opened: + stack.append([opened.group(1), m.end(), 1]) + continue + if not stack: + continue + if _OPENS.match(action): + stack[-1][2] += 1 + elif re.match(r"^end\b", action): + stack[-1][2] -= 1 + if stack[-1][2] == 0: + name, start, _ = stack.pop() + out[name] = text[start : m.start()] + return out def helpers_following_the_override(sources): @@ -172,8 +214,7 @@ def helpers_following_the_override(sources): """ bodies = {} for text in sources: - for name, body in _DEFINE.findall(text): - bodies[name] = body + bodies.update(define_bodies(text)) following = {"tracebloc.fullname"} changed = True while changed: @@ -238,6 +279,51 @@ def _mitigations(text, routed_vars): return out +#: A helper whose `fullname` reference sits AFTER an inner `if`/`end` — the exact +#: shape the non-greedy regex dropped. Written down here rather than hunted for in +#: the chart, because the chart does not currently contain one: the defect was +#: LATENT, and a check that can only be exercised by a bug already present is a +#: check that arrives too late (CLAUDE.md rule 6 — derive the input domain, do not +#: wait for the input). +_SELFTEST_TEMPLATE = """ +{{- define "probe.routed" -}} +{{- if .Values.something -}} +irrelevant +{{- end -}} +{{ include "tracebloc.fullname" . }}-probe +{{- end -}} +{{- define "probe.unrouted" -}} +{{- if .Values.something -}} +irrelevant +{{- end -}} +a-constant-name +{{- end -}} +""" + + +def selftest_the_parser(): + """`(ok, messages)` — the define parser reads a body past an inner `end`. + + Two halves, and the second is what stops this passing vacuously: a parser that + marked EVERYTHING routed would satisfy the first assertion and fail this one. + """ + following = helpers_following_the_override([_SELFTEST_TEMPLATE]) + if "probe.routed" not in following: + return False, [ + " [ERROR] the define parser does not read a helper body past an inner " + "`end`, so a helper whose `fullname` reference sits below an `if` reads " + "as UNROUTED — after which a routed Secret lookup needs no mitigation " + "and assertion 5 passes on a chart that is not safe." + ] + if "probe.unrouted" in following: + return False, [ + " [ERROR] the define parser marked a helper with no `fullname` " + "reference as routed, so 'follows the override' means nothing and the " + "check above cannot distinguish a safe site from an unsafe one." + ] + return True, [" [OK] the define parser reads helper bodies past an inner `end`"] + + def assert_lookup_keys(): """`(ok, messages)` — every Secret lookup on a routed name is mitigated.""" files = sorted( @@ -520,6 +606,14 @@ def main() -> int: fail = True # --- 5. the class behind the credential-Secret finding ------------------- + # The parser this rests on is self-tested first: if it cannot read a helper + # body, everything below reads as safe. + ok, msgs = selftest_the_parser() + for m in msgs: + print(m) + if not ok: + fail = True + ok, msgs = assert_lookup_keys() for m in msgs: print(m) diff --git a/scripts/tests/install-client-helm.bats b/scripts/tests/install-client-helm.bats index dc02c4d6..30c7f4e4 100644 --- a/scripts/tests/install-client-helm.bats +++ b/scripts/tests/install-client-helm.bats @@ -934,6 +934,59 @@ _no_clientid_release_ctx() { [ "${INSTALLED_CLIENT_UNKNOWN:-0}" = 0 ] || return 1 } +# THE SECRET'S NAME IS NOT ALWAYS THE RELEASE NAME (Bugbot, Medium, on #911). +# `tracebloc.secretName` follows `fullnameOverride`, so on a release installed +# with one the Secret is `-secrets`. Reading `-secrets` finds +# nothing, and a live client whose id lives only in the Secret then reads as +# UNIDENTIFIABLE -- `diagnose` and `upgrade` treat it as having no id. +# +# BOTH DIRECTIONS ARE ASSERTED. Without the second, the first is satisfied by a +# fallback that always uses the override key even when it is absent, which would +# break every ordinary release; without the first, nothing catches the bug. +_override_release_ctx() { + HOST_DATA_DIR="$BATS_TEST_TMPDIR/data"; mkdir -p "$HOST_DATA_DIR" + helm() { + if [ "$1" = list ]; then + printf '%s\n' 'NAME NAMESPACE REVISION UPDATED STATUS CHART APP VERSION' \ + 'liverel munich 1 2026-01-01 deployed client-1.9.87 1.9.87' + return 0 + fi + # Values carry the override and NO clientId -- the #2571 shape on a renamed + # release, which is what this PR makes reachable. + if [ "$1" = get ] && [ "$2" = values ]; then + printf 'fullnameOverride: zzoverride\nstorageClass: {}\n'; return 0 + fi + return 0 + } +} + +@test "detect_installed_client: the Secret is read under fullnameOverride, not the release name (#911)" { + _override_release_ctx + has() { [ "$1" = helm ] || [ "$1" = kubectl ]; } + kubectl() { + [ "$1" = -n ] && [ "$2" = munich ] || return 1 + # THE ASSERTION: the overridden name, not `liverel-secrets`. + [ "$5" = "zzoverride-secrets" ] || return 1 + printf 'dXVpZC1mcm9tLXNlY3JldA==' + } + detect_installed_client + [ "$INSTALLED_CLIENT_ID" = "uuid-from-secret" ] || return 1 + [ "$INSTALLED_CLIENT_NS" = "munich" ] || return 1 + [ "${INSTALLED_CLIENT_UNKNOWN:-0}" = 0 ] || return 1 +} + +@test "detect_installed_client: with NO override the Secret is still the release name (#911 control)" { + _no_clientid_release_ctx + has() { [ "$1" = helm ] || [ "$1" = kubectl ]; } + kubectl() { + [ "$5" = "liverel-secrets" ] || return 1 + printf 'dXVpZC1mcm9tLXNlY3JldA==' + } + detect_installed_client + [ "$INSTALLED_CLIENT_ID" = "uuid-from-secret" ] || return 1 + [ "${INSTALLED_CLIENT_UNKNOWN:-0}" = 0 ] || return 1 +} + @test "detect_installed_client: no clientId in values AND no readable Secret -> UNKNOWN, never 'no client'" { _no_clientid_release_ctx # kubectl absent: the id cannot be read from either place. The release still diff --git a/scripts/tests/install-k8s.Tests.ps1 b/scripts/tests/install-k8s.Tests.ps1 index 022d7907..9387e2bf 100644 --- a/scripts/tests/install-k8s.Tests.ps1 +++ b/scripts/tests/install-k8s.Tests.ps1 @@ -3484,6 +3484,54 @@ Describe "Get-InstalledClientInfo API gating (Bugbot)" { $info.ListUnknown | Should -BeFalse Should -Invoke helm -ParameterFilter { $args -contains "list" } } + + # THE SECRET'S NAME IS NOT ALWAYS THE RELEASE NAME (Bugbot, Medium, on + # client#911). `tracebloc.secretName` follows `fullnameOverride`, so on a + # release installed with one the Secret is `-secrets`. Reading + # `-secrets` finds nothing, and a live client whose id lives only in + # the Secret reads as UNIDENTIFIABLE -- `diagnose` and `upgrade` then treat it + # as having no id. Bash parity: install-client-helm.bats' two #911 cases. + It "reads the Secret under fullnameOverride, not the release name (client#911)" { + Mock kubectl { + $global:LASTEXITCODE = 0 + # THE ASSERTION lives in the mock: only the overridden name answers. + if ($args -contains "zzoverride-secrets") { return "dXVpZC1mcm9tLXNlY3JldA==" } + return "" + } + Mock helm { + if ($args -contains "list") { '[{"name":"rel","namespace":"tracebloc","chart":"client-1.9.87"}]'; $global:LASTEXITCODE = 0; return } + # Values carry the override and NO clientId -- the backend#2571 shape on a + # renamed release, which is what this PR makes reachable. + if ($args -contains "get") { '{"fullnameOverride":"zzoverride"}'; $global:LASTEXITCODE = 0; return } + $global:LASTEXITCODE = 0 + } + $info = Get-InstalledClientInfo + $info.Id | Should -Be "uuid-from-secret" + $info.ListUnknown | Should -BeFalse + } + + It "with NO override still reads the release name (client#911 control)" { + # Without this the first case is satisfied by always using the override key, + # which would break every ordinary release. + # EXIT 0 THROUGHOUT, and that matters: `Get-InstalledClientInfo` opens with a + # bounded kubectl probe and degrades to ListUnknown when it fails. A mock that + # returns non-zero for "anything but my Secret" fails that probe too, so the + # test would pass or fail for the wrong reason. A non-matching Secret read + # returns EMPTY instead, which `Get-ClientIdFromSecret` already treats as + # "couldn't read". + Mock kubectl { + $global:LASTEXITCODE = 0 + if ($args -contains "rel-secrets") { return "dXVpZC1mcm9tLXNlY3JldA==" } + return "" + } + Mock helm { + if ($args -contains "list") { '[{"name":"rel","namespace":"tracebloc","chart":"client-1.9.87"}]'; $global:LASTEXITCODE = 0; return } + if ($args -contains "get") { '{"storageClass":{}}'; $global:LASTEXITCODE = 0; return } + $global:LASTEXITCODE = 0 + } + $info = Get-InstalledClientInfo + $info.Id | Should -Be "uuid-from-secret" + } } Describe "UNC-safe background jobs (#409)" { From f585de1a3d0f7fcd2b79384d9b38035fe2a7421b Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 13:09:29 +0200 Subject: [PATCH 07/22] fix(chart): assert the SPECIFIC refusal, gate the interpreter, and correct the prose the refusal falsified (backend#2626) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asad`s five remaining items, and two of them were defects in my own guard. 1. THE SELF-CHECK WAS SATISFIED BY ANY NON-ZERO EXIT. Written as `if …; then error; else OK; fi`, so with python3 absent (rc 127) the sidecar never ran and it printed [OK]. The one assertion whose stated purpose is "an unreachable refusal is one nobody notices has stopped refusing" reported green for a reason unrelated to what it checks - the bare `assertRaises(Exception)` shape, in shell (CLAUDE.md rule 10). It now requires EXIT 1 specifically, and treats any other code as a cannot-tell finding. 2. NO INTERPRETER GATE, where 23 siblings have one. python3 absent gave rc 127, missed the exit-2 branch, and ended on "fullnameOverride is incomplete in 4 profile check(s)" - a missing tool reported as a chart defect, the exact misdiagnosis the module half was fixed for. Added in the siblings` `fail_closed` idiom, for python3 AND helm. Measured: absent python3 now exits 2 with "THIS IS A MISSING TOOL, NOT A VERDICT ON THE CHART". And that work exposed a third: my PyYAML guard caught `ModuleNotFoundError`, which is a SUBCLASS of `ImportError` - so it missed the parent, and a plain `ImportError` is exactly what `pyyaml-preflight.bats` injects to simulate an absent PyYAML. Under the repo`s own simulation the guard gave a TRACEBACK and rc 1 while the bats class rule stayed green, because it reads the AST and accepts either name. Widened to `ImportError`, which also covers the real-world broken-install case. Verified: rc 2 with the named refusal. 3. `fullnameOverride: ""` was schema-INVALID. The pattern required at least one character, so the chart`s own commented example at values.yaml would have been rejected if uncommented verbatim, while the template treats "" as unset (Arturo). Pattern now accepts the empty string; Bad_Name and a 54-char value still fail at template time. 4. values.yaml said changing it on an existing install is "a migration, not a config tweak". It is REFUSED outright. That was the most misleading line in the file - it invited planning a migration the chart will not start. 5. values.yaml said the chart "ALWAYS emits `-secrets`", with pre-create commands using that name. False under an override, and the consequence after the PVC re-key is a tier-3 hard fail on clientId for an operator who followed the doc correctly. It now says the name follows the override and gives a command to read it off the render rather than guess. 6. THE REFUSAL BLOCKED A DOCUMENTED RECOVERY PATH. docs/MIGRATIONS.md Option C is uninstall -> clear claimRef -> re-create PVCs -> install. Only the PVCs carry `resource-policy: keep`, so uninstall deletes the Secret and the re-install renders PVC-present + Secret-absent -> refused, with a primary remedy ("copy the credentials") that has nothing left to copy from. Option C now saves the Secret in a step 0 and restores it under the new effective name in a step 4, and the refusal message names `kubectl delete pvc mysql-pvc` as the accept-data-loss path so the failure is escapable from the failure itself. NOT CLAIMED: the `--dry-run=server` matrix was run against ea6568dc, before the PVC re-key. Asad flagged that and he is right - it is not measured for this version and is not recorded as such. VERIFIED on helm v3.15.4: make drift 36/36; helm unittest 640/640; client-credentials-have-a-secret-tier 17 assertions; pyyaml-preflight 3/3; chart-pull-secret green; shellcheck clean; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 --- client/templates/secrets.yaml | 2 +- client/values.schema.json | 4 +- client/values.yaml | 31 ++++++++++-- docs/MIGRATIONS.md | 48 ++++++++++++++++++ .../tests/fullname-override-completeness.sh | 49 ++++++++++++++++--- scripts/tests/fullname_override_assertions.py | 14 +++++- 6 files changed, 135 insertions(+), 13 deletions(-) diff --git a/client/templates/secrets.yaml b/client/templates/secrets.yaml index 96ed178c..bf8dc500 100644 --- a/client/templates/secrets.yaml +++ b/client/templates/secrets.yaml @@ -73,7 +73,7 @@ */ -}} {{- $mysqlDataPresent := (lookup "v1" "PersistentVolumeClaim" .Release.Namespace (include "tracebloc.mysqlPvc" .)) -}} {{- if and $mysqlDataPresent (not $existingSecret) -}} -{{- fail (printf "release %q in namespace %q already has MySQL data (PersistentVolumeClaim %q, retained by helm.sh/resource-policy: keep), but there is no Secret named %q -- the name this render resolves to. The credentials below would therefore be re-minted while that database still holds the old ones: the upgrade would report deployed and MySQL would refuse every login. FIX: copy the existing credentials to the name this render wants, then re-run -- kubectl -n %s get secret -o json | jq '.metadata.name=\"%s\" | del(.metadata.uid,.metadata.resourceVersion,.metadata.creationTimestamp,.metadata.ownerReferences)' | kubectl apply -f - . The MySQL PVC is named %q and never follows fullnameOverride, so nothing else has to move. ALTERNATIVELY: put fullnameOverride back to the value this release was last rendered with, so the existing Secret matches again." .Release.Name .Release.Namespace (include "tracebloc.mysqlPvc" .) $secretName .Release.Namespace $secretName (include "tracebloc.mysqlPvc" .)) -}} +{{- fail (printf "release %q in namespace %q already has MySQL data (PersistentVolumeClaim %q, retained by helm.sh/resource-policy: keep), but there is no Secret named %q -- the name this render resolves to. The credentials below would therefore be re-minted while that database still holds the old ones: the upgrade would report deployed and MySQL would refuse every login. FIX: copy the existing credentials to the name this render wants, then re-run -- kubectl -n %s get secret -o json | jq '.metadata.name=\"%s\" | del(.metadata.uid,.metadata.resourceVersion,.metadata.creationTimestamp,.metadata.ownerReferences)' | kubectl apply -f - . The MySQL PVC is named %q and never follows fullnameOverride, so nothing else has to move. ALTERNATIVELY: put fullnameOverride back to the value this release was last rendered with, so the existing Secret matches again. IF YOU ARE DELIBERATELY STARTING FROM AN EMPTY DATABASE and there is no Secret to copy -- the uninstall-and-recover flow in docs/MIGRATIONS.md Option C, where the uninstall deleted it -- then delete the datadir and let this render mint fresh credentials: kubectl -n %s delete pvc %s . That is the accept-data-loss path and it is the only other way past this refusal." .Release.Name .Release.Namespace (include "tracebloc.mysqlPvc" .) $secretName .Release.Namespace $secretName (include "tracebloc.mysqlPvc" .) .Release.Namespace (include "tracebloc.mysqlPvc" .)) -}} {{- end -}} {{- /* Platform client credentials (backend#2571). Issued by the backend, NOT diff --git a/client/values.schema.json b/client/values.schema.json index 9ef22786..4a39f6bb 100644 --- a/client/values.schema.json +++ b/client/values.schema.json @@ -1450,9 +1450,9 @@ }, "fullnameOverride": { "type": "string", - "pattern": "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$", + "pattern": "^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$", "maxLength": 53, - "description": "Rename the resources this chart creates without uninstall + reinstall (backend#2626). Unset resolves to .Release.Name verbatim, so leaving it alone is byte-identical to before it existed. Renames RESOURCES, not the Helm release: app.kubernetes.io/instance, meta.helm.sh/release-name and the RELEASE_NAME env keep the release name, and release-scoped on-disk paths keep it too because a path is a location, not a name. Changing it on an existing install is a migration (Helm renames as delete + create), not a config tweak. THE PATTERN AND LENGTH ARE THE ONES HELM ENFORCES ON THE VALUE THIS REPLACES: a release name is a DNS-1123 label capped at 53 characters, so without them this key was a strictly weaker gate than the thing it substitutes for. A value like Bad_Name! templated clean and then failed object-by-object at the API server, mid-install, leaving a partly-created release. Declared here so it fails at template time, by name (reviewer, backend#2626)." + "description": "Rename the resources this chart creates without uninstall + reinstall (backend#2626). Unset resolves to .Release.Name verbatim, so leaving it alone is byte-identical to before it existed. Renames RESOURCES, not the Helm release: app.kubernetes.io/instance, meta.helm.sh/release-name and the RELEASE_NAME env keep the release name, and release-scoped on-disk paths keep it too because a path is a location, not a name. Changing it on an existing install is a migration (Helm renames as delete + create), not a config tweak. THE PATTERN AND LENGTH ARE THE ONES HELM ENFORCES ON THE VALUE THIS REPLACES: a release name is a DNS-1123 label capped at 53 characters, so without them this key was a strictly weaker gate than the thing it substitutes for. A value like Bad_Name! templated clean and then failed object-by-object at the API server, mid-install, leaving a partly-created release. Declared here so it fails at template time, by name (reviewer, backend#2626). THE EMPTY STRING IS ACCEPTED EXPLICITLY: the template treats it as unset (default .Release.Name .Values.fullnameOverride) and values.yaml ships `fullnameOverride: \"\"` as its commented example, so a pattern requiring at least one character would reject the chart's own documented line if uncommented verbatim (Arturo). CHANGING IT ON A LIVE RELEASE IS REFUSED at template time by secrets.yaml, not merely discouraged - earlier prose here called it a migration, which invited planning one the chart will not start." }, "telemetryCollector": { "type": [ diff --git a/client/values.yaml b/client/values.yaml index 8aa2261c..a89b2980 100644 --- a/client/values.yaml +++ b/client/values.yaml @@ -510,8 +510,24 @@ egressProxy: # the hostPath PVs, the Collector's queue -- keep the RELEASE name, because a # path is a location: renaming it orphans a tenant's data instead of moving it. # -# Changing it on an existing install renames every resource, which Helm performs -# as delete + create. Treat it as a migration, not a config tweak. +# CHANGING IT ON A LIVE RELEASE IS REFUSED, not merely discouraged. Earlier +# revisions of this comment called it "a migration, not a config tweak", which +# invited planning a migration the chart will not let you start. `secrets.yaml` +# now hard-fails at template time when the MySQL PVC is present and no Secret +# exists under the name this render resolves to -- which is every direction of a +# rename (unset->A, A->B, A->none) and also a reinstall over a retained PVC. The +# refusal names the one-command remedy: copy the Secret to the name this render +# wants. Without it, four credentials would be re-minted while the kept +# `mysql-pvc` datadir still holds the old ones, the upgrade would report +# `deployed`, and MySQL would refuse every login. +# +# So: choose it at INSTALL time. On a live release, read the refusal message. +# +# The commented example below is the EMPTY STRING, which the template treats as +# unset (`default .Release.Name .Values.fullnameOverride`) and which +# values.schema.json therefore accepts explicitly -- an earlier revision of the +# schema pattern required at least one character and would have rejected this +# very line if uncommented verbatim (Arturo, review of backend#2626). # fullnameOverride: "" # -- Edge telemetry Collector (RFC-BACKEND-1872 D6/D7, backend#1906). @@ -1116,7 +1132,16 @@ podDisruptionBudget: # already Helm-managed, so there is nothing else to do. # # BEFORE THE FIRST INSTALL, the Secret has to be adopted, not just created. This -# chart ALWAYS emits `-secrets`, so a plain `kubectl create secret` +# chart emits `-secrets` -- or `-secrets` if you set +# that (see `fullnameOverride` above; `tracebloc.secretName` follows it). USE THE +# NAME THIS RENDER RESOLVES TO in the three commands below, substituting your +# override for `` if you have one. Get it from the chart rather than +# guessing: +# +# helm template ./client -n --show-only templates/secrets.yaml \ +# [-f your-values.yaml] | grep -m1 " name:" +# +# Naming the wrong one is not a cosmetic error: a plain `kubectl create secret` # leaves an object Helm does not own and `helm install` aborts with # `invalid ownership metadata` rather than reading it (Bugbot, #859). Helm adopts # a pre-existing object that already carries its three ownership fields, so add diff --git a/docs/MIGRATIONS.md b/docs/MIGRATIONS.md index 1a743401..fe84fe3a 100644 --- a/docs/MIGRATIONS.md +++ b/docs/MIGRATIONS.md @@ -70,6 +70,35 @@ If you find yourself reaching for Option B, you actually want Option A or Option If neither A nor B works and the underlying storage is `Retain`, the data survives PVC deletion. You'll need to rebuild the PVCs manually after uninstall: +**Save the credentials Secret FIRST.** The PVCs carry +`helm.sh/resource-policy: keep`; the Secret does **not**, so `helm uninstall` +deletes it while the MySQL datadir survives on the retained PV. The re-install +would then find a datadir with no matching credentials, and since backend#2626 it +**refuses at template time** rather than silently re-minting them: + +> already has MySQL data (PersistentVolumeClaim "mysql-pvc" …), but there is no +> Secret named … + +That refusal is correct — without it the install would report `deployed` and MySQL +would refuse every login — but in this flow there is nothing left to copy from +unless you saved it in step 0. So: + +```bash +# 0. SAVE THE SECRET. Do this before the uninstall; there is no recovering it +# afterwards, and the re-install in step 4 will refuse without it. +# Get the name this release actually rendered (it follows fullnameOverride if +# that is set) rather than assuming -secrets: +SECRET=$(kubectl -n get secret -l app.kubernetes.io/instance= \ + -o jsonpath='{.items[?(@.type=="Opaque")].metadata.name}' | tr ' ' '\n' \ + | grep -- '-secrets$' | head -1) +kubectl -n get secret "$SECRET" -o json > /tmp/tracebloc-secret-backup.json +``` + +If you are deliberately starting from an empty database, delete the datadir PVC +instead and let the chart mint fresh credentials — +`kubectl -n delete pvc mysql-pvc`. That is the accept-data-loss path, and it +is the only other way past the refusal. + ```bash # 1. Uninstall (PVCs deleted; PVs go to Released; underlying EFS/EBS intact) helm uninstall -n @@ -101,6 +130,25 @@ spec: EOF ``` +```bash +# 4. Restore the Secret under the name the NEW release will render, then install. +# Strip the fields the API server owns, and re-stamp Helm's ownership for the +# new release so `helm install` adopts it instead of aborting on +# `invalid ownership metadata`. +jq --arg n "-secrets" --arg ns "" --arg rel "" ' + .metadata.name = $n + | .metadata.namespace = $ns + | .metadata.labels["app.kubernetes.io/managed-by"] = "Helm" + | .metadata.annotations["meta.helm.sh/release-name"] = $rel + | .metadata.annotations["meta.helm.sh/release-namespace"] = $ns + | del(.metadata.uid, .metadata.resourceVersion, .metadata.creationTimestamp, + .metadata.ownerReferences, .metadata.managedFields)' \ + /tmp/tracebloc-secret-backup.json | kubectl apply -f - +``` + +If the new release uses `fullnameOverride`, use `-secrets` as the name +in step 4 — that is what the chart will look for. + Only safe if the PV's `reclaimPolicy` is `Retain`. With `Delete` you lose data the moment the PV goes Released. --- diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh index 73e3a5fe..3fb587fe 100755 --- a/scripts/tests/fullname-override-completeness.sh +++ b/scripts/tests/fullname-override-completeness.sh @@ -60,6 +60,21 @@ set -euo pipefail cd "$(dirname "$0")/../.." +# PREFLIGHT, IN THE SAME SHAPE AS 23 SIBLINGS IN THIS DIRECTORY, and exit 2 is +# the point rather than the tidiness (Asad, review of backend#2626). +# +# The assertions sidecar already answers "PyYAML is missing" with exit 2 and a +# named refusal. The INTERPRETER half had no gate at all, and it lands worse: +# python3 absent gives rc 127, which misses the `exit 2` branch below entirely, +# increments `failures` once per profile, and the run ends on "fullnameOverride +# is incomplete in 4 profile check(s)" -- a MISSING TOOL reported as a chart +# defect, which is exactly the misdiagnosis the module half was fixed for. +fail_closed() { printf '[ERROR] %s\n' "$1" >&2; exit 2; } +command -v python3 >/dev/null 2>&1 || fail_closed \ + "python3 is required for this check -- assertions 2-5 are a python sidecar and it was not found on PATH. Install python3 or add it to 'make setup'. THIS IS A MISSING TOOL, NOT A VERDICT ON THE CHART: nothing about fullnameOverride was checked." +command -v helm >/dev/null 2>&1 || fail_closed \ + "helm is required for this check -- every assertion reads a rendered manifest and it was not found on PATH. THIS IS A MISSING TOOL, NOT A VERDICT ON THE CHART." + # DELIBERATELY LONG, and that is not cosmetic. Assertion 1 diffs two renders # that BOTH pass through the helper, so a transformation applied uniformly -- # `| trunc N` on the default -- cancels out and is invisible to it. With a short @@ -201,17 +216,39 @@ done # deleting that branch left the whole guard green. # # So invoke the assertions directly with the NOTES arguments MISSING and require -# a non-zero exit. If this ever passes, a future caller could quietly drop the -# NOTES render and the guard would report three assertions as four. -if RELEASE="$RELEASE" NS="$NS" OVERRIDE="$OVERRIDE" \ - python3 scripts/tests/fullname_override_assertions.py \ - "$tmp/override.yaml" "$tmp/a.yaml" >/dev/null 2>&1; then +# THE SPECIFIC exit code that means "checked, and found a problem". +# +# EXIT 1, NOT MERELY NON-ZERO, and the difference was a real defect (Asad, +# review of backend#2626). Written as `if …; then error; else OK; fi`, ANY +# non-zero satisfied the `else` -- so with python3 absent the sidecar never ran, +# rc was 127, and this printed `[OK]`. The one assertion whose stated purpose is +# "an unreachable refusal is one nobody notices has stopped refusing" reported +# green for a reason unrelated to what it checks. That is the bare +# `assertRaises(Exception)` shape, in shell. +# +# 1 = the assertions ran and refused. 2 = could not run (no PyYAML). 127 = no +# interpreter. Only the first is the thing being asserted, and the preflight +# above now makes 127 unreachable anyway -- belt and braces, because this +# assertion must not be satisfiable by the absence of the thing it invokes. +set +e +RELEASE="$RELEASE" NS="$NS" OVERRIDE="$OVERRIDE" \ + python3 scripts/tests/fullname_override_assertions.py \ + "$tmp/override.yaml" "$tmp/a.yaml" >/dev/null 2>&1 +selfcheck_rc=$? +set -e +if [ "$selfcheck_rc" -eq 1 ]; then + echo "-- self-check: the assertions refuse to run without the NOTES render [OK]" +elif [ "$selfcheck_rc" -eq 0 ]; then echo "[ERROR] the assertions PASSED with no rendered NOTES supplied, so a caller" echo " that drops the NOTES render would get a green guard that checked" echo " three things while documenting four." failures=$((failures + 1)) else - echo "-- self-check: the assertions refuse to run without the NOTES render [OK]" + echo "[ERROR] the self-check could not establish anything: the assertions exited" + echo " $selfcheck_rc, which is neither 0 (passed -- a defect) nor 1 (refused" + echo " -- correct). Exit 2 means the sidecar could not run at all, and any" + echo " other code means it did not get that far. Cannot tell is not OK." + failures=$((failures + 1)) fi if [ "$failures" -ne 0 ]; then diff --git a/scripts/tests/fullname_override_assertions.py b/scripts/tests/fullname_override_assertions.py index 13178e9f..8468974a 100644 --- a/scripts/tests/fullname_override_assertions.py +++ b/scripts/tests/fullname_override_assertions.py @@ -49,7 +49,19 @@ # sidecar in this tree carries this shape. try: import yaml -except ModuleNotFoundError: +except ImportError: + # `ImportError`, NOT `ModuleNotFoundError`, and the distinction is not + # pedantic. `ModuleNotFoundError` is a SUBCLASS of `ImportError`, so catching + # the subclass misses the parent -- and a plain `ImportError` is exactly what + # `scripts/tests/pyyaml-preflight.bats` injects to simulate an absent PyYAML + # (`raise ImportError("simulated: PyYAML not installed")`). Measured: under + # that simulation the narrow form gave a TRACEBACK and rc 1, i.e. the very + # failure this block exists to prevent, while the bats class-rule stayed + # green because it reads the AST and accepts either name. + # + # It is also the real-world case: a PyYAML installed but broken (a partial + # wheel, a C-extension mismatch) raises `ImportError`, not + # `ModuleNotFoundError`. Catching the parent covers both. sys.stderr.write( "[ERROR] this guard needs PyYAML and the interpreter does not have it.\n" " Install it: python3 -m pip install pyyaml\n" From d59664f0e3684e3cbaff612a58c5c00f2a810e0d Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 13:26:22 +0200 Subject: [PATCH 08/22] fix(chart): accept a renamed release`s token Secret, and scope the path-class check to the chart (backend#2626) Three Bugbot findings on f585de1. One demoted with evidence, two fixed. 1. HIGH, DEMOTED: "Path class fails on cloud profiles". It does not, at head. Measured on all four CI profiles: aks 1, bm 4, eks 1, oc 1 - the cloud profiles each render ONE release-scoped path, the Collector`s queue directory `/var/lib/tracebloc//telemetry`, because `telemetryCollector` is on by chart DEFAULT and no client/ci profile sets it. The required drift guard is green and does not refuse a complete chart. The HAZARD is real though, and the finding named the right one with the wrong scope: the gate is tri-state since backend#1906, so a profile that disabled the Collector AND hostPath would have no release-scoped path, and a PER-PROFILE emptiness check would then refuse a complete chart. So the emptiness assertion moved from per-profile to ACROSS profiles - a profile with none is a `[note]`, a CHART with none anywhere is still an error, because "no path followed the override" is equally true of a chart that stopped scoping paths by release. Proved both ways: a 5th profile disabling both keeps the guard green (the false positive that would otherwise have fired), and neutering the path class reddens it. 2. MEDIUM, FIXED: the token lookup missed a renamed release`s Secret. `telemetryTokenPresent` accepted the override-following name and the legacy FIXED name, but not `-telemetry-token` - so on a renamed release `telemetryCollectorState` hard-FAILED for an operator who had explicitly enabled the Collector, naming two names that were never going to match while the token sat there under a third. Accepted now, and named in the refusal. Accepted rather than refused, which is deliberately the opposite call from the credentials Secret: there a name miss means silently minting a password against a datadir holding the old one. The token is server-side and re-derivable (jobs-manager writes it, backend#2274), so finding the existing one is safe and is what the operator meant. Nothing pinned the accepted-name set, so nothing would have caught it going away again. `telemetry-token-agreement.sh` now asserts an AGREEMENT rather than a list: every name the lookup accepts must be reported by the refusal. Placeholders are COUNTED against arguments, because the first cut checked that the name appeared on the line - and the format string and its args share one line, so deleting a `%q` left the substring matching while printf silently dropped the argument. Caught by mutation-proving; the counting lives in a `.py` sidecar, which is inside pyyaml-preflight`s class rule now that it covers `.py`. 3. MEDIUM, ALREADY FILED: the installer still builds `{namespace}-jobs-manager` in six places. That is backend#2888, filed with the measurement - six reconstructing sites, two that already DISCOVER by pattern and are override-safe, and the structural fix (select on `app.kubernetes.io/instance`). It is a change to the installer`s naming model, not a patch, and it should land before fullnameOverride is recommended to operators. VERIFIED on helm v3.15.4: make drift 36/36; helm unittest 640/640; telemetry-token-agreement green and mutation-proved 4 ways; pyyaml-preflight 3/3; shellcheck clean; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 --- client/templates/_helpers.tpl | 26 ++++++- .../tests/fullname-override-completeness.sh | 36 +++++++++- scripts/tests/fullname_override_assertions.py | 28 +++++++- scripts/tests/telemetry-token-agreement.sh | 69 +++++++++++++++++++ .../tests/telemetry_token_refusal_arity.py | 69 +++++++++++++++++++ 5 files changed, 222 insertions(+), 6 deletions(-) create mode 100644 scripts/tests/telemetry_token_refusal_arity.py diff --git a/client/templates/_helpers.tpl b/client/templates/_helpers.tpl index 9f567889..2d3e5db0 100644 --- a/client/templates/_helpers.tpl +++ b/client/templates/_helpers.tpl @@ -1116,12 +1116,34 @@ tracebloc-telemetry-token until jobs-manager next re-authenticates, and refusing it would wedge exactly the edge that is already collecting. */}} +{{/* + The token Secret's name BEFORE `fullnameOverride` was set — i.e. what a release + installed without one is still carrying (Bugbot, Medium, on client#911). + + `telemetryTokenSecretName` follows the override, so on a renamed release the + lookup below missed the live Secret and `telemetryCollectorState` hard-FAILED + for an operator who had explicitly enabled the Collector: the token exists, it + is simply under `-telemetry-token`. The legacy fallback did not cover + it either — that is a different, FIXED name (`tracebloc-telemetry-token`), not + the release-scoped one. + + Accepted rather than refused, and that is deliberately the opposite call from + the credentials Secret. There, a name miss means SILENTLY MINTING a new + password against a datadir that holds the old one, so refusing is the only safe + answer. Here the token is server-side and re-derivable — jobs-manager writes it + (backend#2274) — so finding the existing one is both safe and what the operator + meant. Same reasoning as the legacy name this sits beside. +*/}} +{{- define "tracebloc.telemetryTokenPreOverrideName" -}} +{{- printf "%s-telemetry-token" .Release.Name -}} +{{- end -}} + {{- define "tracebloc.telemetryTokenPresent" -}} {{- if not (lookup "v1" "Namespace" "" "kube-system") -}} unknown {{- else -}} {{- $ns := .Values.nodeAgents.namespace.name -}} -{{- if or (lookup "v1" "Secret" $ns (include "tracebloc.telemetryTokenSecretName" .)) (lookup "v1" "Secret" $ns (include "tracebloc.telemetryTokenLegacyName" .)) -}} +{{- if or (lookup "v1" "Secret" $ns (include "tracebloc.telemetryTokenSecretName" .)) (lookup "v1" "Secret" $ns (include "tracebloc.telemetryTokenLegacyName" .)) (lookup "v1" "Secret" $ns (include "tracebloc.telemetryTokenPreOverrideName" .)) -}} yes {{- else -}} no @@ -1190,7 +1212,7 @@ no {{- if kindIs "bool" $tc.enabled -}} {{- if $tc.enabled -}} {{- if eq (include "tracebloc.telemetryTokenPresent" .) "no" -}} -{{- fail (printf "telemetryCollector.enabled is true but its token Secret does not exist in namespace %q — looked for %q, and during migration the legacy %q. The Collector's exporter authenticates with it, and jobs-manager writes it (backend#2274). Create it, or set telemetryCollector.enabled: false — enabling without it buys a DaemonSet that spools to every node's disk and delivers nothing." .Values.nodeAgents.namespace.name (include "tracebloc.telemetryTokenSecretName" .) (include "tracebloc.telemetryTokenLegacyName" .)) -}} +{{- fail (printf "telemetryCollector.enabled is true but its token Secret does not exist in namespace %q — looked for %q, the legacy %q, and the pre-fullnameOverride %q. The Collector's exporter authenticates with it, and jobs-manager writes it (backend#2274). Create it, or set telemetryCollector.enabled: false — enabling without it buys a DaemonSet that spools to every node's disk and delivers nothing." .Values.nodeAgents.namespace.name (include "tracebloc.telemetryTokenSecretName" .) (include "tracebloc.telemetryTokenLegacyName" .) (include "tracebloc.telemetryTokenPreOverrideName" .)) -}} {{- end -}} enabled {{- else -}} diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh index 3fb587fe..f51865a2 100755 --- a/scripts/tests/fullname-override-completeness.sh +++ b/scripts/tests/fullname-override-completeness.sh @@ -97,6 +97,9 @@ fi echo "== fullnameOverride completeness ==" tmp=$(mktemp -d); trap 'rm -rf "$tmp"' EXIT failures=0 +# Accumulators for the cross-profile release-scoped-path assertion after the loop. +path_profiles=0 +path_counts="" # NOTES NEEDS ITS OWN RENDER, AND EVERY OBVIOUS ROUTE IS CLOSED. Measured on the # CI-pinned helm v3.15.4: @@ -199,9 +202,16 @@ for VALUES in "${profiles[@]}"; do RELEASE="$RELEASE" NS="$NS" OVERRIDE="$OVERRIDE" \ python3 scripts/tests/fullname_override_assertions.py \ "$tmp/override.yaml" "$tmp/a.yaml" \ - "$tmp/notes-override.txt" "$tmp/notes-default.txt" - rc=$? + "$tmp/notes-override.txt" "$tmp/notes-default.txt" | tee "$tmp/out.txt" + rc=${PIPESTATUS[0]} set -e + # The release-scoped-path count, accumulated for the cross-profile assertion + # below. `|| true` because grep exits 1 on no match, which under pipefail would + # abort the loop -- and "the sidecar printed no count" is itself a finding the + # assertion after the loop reports, not one to die on here. + pc=$(grep -E '^PATHCLASS [0-9]+$' "$tmp/out.txt" | awk '{print $2}' | head -1 || true) + path_counts="${path_counts}${prof}=${pc:-none} " + case "${pc:-0}" in ''|0) ;; *) path_profiles=$((path_profiles + 1)) ;; esac if [ "$rc" -eq 2 ]; then echo "[ERROR] the guard could not run (see above). This is NOT a verdict on the chart." exit 2 @@ -251,6 +261,28 @@ else failures=$((failures + 1)) fi +# --- 6. RELEASE-SCOPED PATHS EXIST SOMEWHERE --------------------------------- +# The chart-level half of assertion 3's path class. Per profile, an empty class is +# legitimate: on a cloud profile the only release-scoped path is the Collector's +# queue directory, and a profile that disabled the Collector would have none. Per +# CHART it is not: "no path followed the override" is also true of a chart that +# stopped scoping paths by release at all, which is the failure the class exists +# to catch (Bugbot, High -- the finding named the right hazard and the wrong +# scope; measured at head, all four profiles have at least one). +# +# Counts are printed either way so a reader can see WHICH profile contributed. +echo "-- release-scoped paths per profile: ${path_counts% }" +if [ "$path_profiles" -eq 0 ]; then + echo "[ERROR] NO profile rendered a release-scoped on-disk path, so 'no path" + echo " followed the override' proves nothing — it is equally true of a" + echo " chart that stopped scoping paths by release. Either the paths" + echo " disappeared or every profile now disables both hostPath and the" + echo " Collector, and both are findings." + failures=$((failures + 1)) +else + echo "-- release-scoped paths present in $path_profiles profile(s) [OK]" +fi + if [ "$failures" -ne 0 ]; then echo "[ERROR] fullnameOverride is incomplete in $failures profile check(s)" exit 1 diff --git a/scripts/tests/fullname_override_assertions.py b/scripts/tests/fullname_override_assertions.py index 8468974a..aa716ce0 100644 --- a/scripts/tests/fullname_override_assertions.py +++ b/scripts/tests/fullname_override_assertions.py @@ -512,8 +512,28 @@ def main() -> int: expected = {CLS_INSTANCE: rel, CLS_ANNOTATION: rel, CLS_ENV: None, CLS_PATH: None} for cls, found in licensed.items(): if not found: - # CANNOT TELL IS A FINDING. An empty list agrees with every - # expectation, so a class that stops matching would read as a pass. + # CANNOT TELL IS A FINDING — but for the PATH class the scope of that + # finding is the CHART, not the profile (Bugbot, High; demoted after + # measuring). The only release-scoped path on a cloud profile is the + # Collector's queue directory, which renders because + # `telemetryCollector` is on by chart DEFAULT and no `client/ci` + # profile sets it. A profile that disabled it — entirely plausible now + # the gate is tri-state (backend#1906) — would have no release-scoped + # path at all, and a per-profile check would then refuse a complete + # chart. + # + # MEASURED AT HEAD, so the finding as filed does not reproduce: aks 1, + # bm 4, eks 1, oc 1. The concern is real and the failure is not, so the + # emptiness is REPORTED here and asserted once ACROSS profiles by the + # shell, which is the only layer that sees all four. + if cls is CLS_PATH: + print( + f" [note] no {cls} in this profile — legitimate when neither " + f"hostPath nor the Collector renders. Asserted across profiles, " + f"not here." + ) + print("PATHCLASS 0") + continue print( f" [ERROR] found NO {cls} carrying the release name — the class matches " f"nothing, so its half of this assertion proves nothing." @@ -550,6 +570,10 @@ def main() -> int: print(f" {where} {path} = {val!r}") else: print(f" [OK] {len(found)} release-scoped path(s) kept the release name") + # A MACHINE-READABLE COUNT for the cross-profile assertion in the + # shell. Printed on both branches so a profile that HAS paths and a + # profile that has none are distinguishable there. + print(f"PATHCLASS {len(found)}") continue want = expected[cls] if cls is CLS_ENV: diff --git a/scripts/tests/telemetry-token-agreement.sh b/scripts/tests/telemetry-token-agreement.sh index 63d70833..45afe9d5 100755 --- a/scripts/tests/telemetry-token-agreement.sh +++ b/scripts/tests/telemetry-token-agreement.sh @@ -237,4 +237,73 @@ echo " release $rel_a -> $name_a" echo " release $rel_b -> $name_b" echo " ok: two releases resolve to distinct, release-scoped Secrets" +# --- the ACCEPTED-NAME SET, and the message that reports it ----------------- +# +# `telemetryTokenPresent` ORs several lookups, and `telemetryCollectorState` +# hard-FAILS when none hits. Three names are accepted: +# +# telemetryTokenSecretName follows fullnameOverride — the current one +# telemetryTokenLegacyName the fixed pre-#2274 name — mid-migration +# telemetryTokenPreOverrideName -telemetry-token — a RENAMED release +# +# The third was missing and the omission was invisible: on a release with +# fullnameOverride set, the token exists under the release-scoped name, the lookup +# missed it, and an operator who had explicitly enabled the Collector got a hard +# refusal naming two names that were never going to match (Bugbot, Medium, on +# client#911). Nothing pinned the set, so nothing would have caught it going away +# again. +# +# ASSERTED AS AN AGREEMENT, not as a list: every name the lookup accepts must +# also be NAMED IN THE REFUSAL. That is the property a human depends on — a +# message that omits a name it searched sends the reader to create a Secret that +# already exists under another name — and it is derived from the template on both +# sides, so adding a fourth name without mentioning it fails here. +helpers="client/templates/_helpers.tpl" +[ -r "$helpers" ] || { echo "[ERROR] cannot read $helpers — refusing to report agreement" >&2; exit 2; } + +accepted="$(grep -oE 'include "tracebloc\.telemetryToken[A-Za-z]*Name"' "$helpers" \ + | sed -E 's/.*"tracebloc\.(telemetryToken[A-Za-z]*Name)".*/\1/' | sort -u)" +[ -n "$accepted" ] || { echo "[ERROR] found ZERO telemetry-token name helpers — the matcher sees nothing" >&2; exit 2; } + +# The `or (lookup …)` chain, on one line by construction. +chain="$(grep -E 'if or \(lookup "v1" "Secret"' "$helpers" || true)" +[ -n "$chain" ] || { echo "[ERROR] could not find the telemetryTokenPresent lookup chain" >&2; exit 2; } +# The refusal, which must report every name the chain searched. +msg="$(grep -E 'telemetryCollector\.enabled is true but its token Secret' "$helpers" || true)" +[ -n "$msg" ] || { echo "[ERROR] could not find the token refusal message" >&2; exit 2; } + +# PLACEHOLDERS COUNTED AGAINST ARGUMENTS, not "the name appears on the line". +# The first cut did the latter, and the `fail (printf "…" args)` call has the name +# helpers in its ARGUMENT list on the same line as the format string — so deleting +# a `%q` from the message left the include in the args, the substring still +# matched, and the check stayed green while the refusal reported one name fewer +# than it searched. Found by mutation-proving. +# +# An argument with no placeholder is silently dropped by printf, which is exactly +# the failure being guarded: a name searched and not reported. +set_fail=0 +mismatch="$(python3 scripts/tests/telemetry_token_refusal_arity.py "$helpers")" +if [ -n "$mismatch" ]; then + echo "[ERROR] $mismatch" >&2 + set_fail=1 +else + echo " ok: the refusal reports every argument it is given" +fi +for n in $accepted; do + case "$chain" in *"$n"*) ;; *) continue ;; esac # not in the chain -> not our business + case "$msg" in + *"$n"*) echo " accepted and passed to the refusal: $n" ;; + *) echo "[ERROR] the lookup accepts $n but the refusal is not even given it" >&2 + set_fail=1 ;; + esac +done +case "$chain" in + *telemetryTokenPreOverrideName*) echo " ok: a renamed release's pre-override token name is still accepted" ;; + *) echo "[ERROR] telemetryTokenPresent no longer accepts the pre-fullnameOverride name" >&2 + echo " (-telemetry-token). A renamed release with the Collector" >&2 + echo " explicitly enabled will hard-fail with the token sitting right there." >&2 + set_fail=1 ;; +esac +[ "$set_fail" -eq 0 ] || { echo "telemetry token agreement: FAILED" >&2; exit 1; } + echo "telemetry token agreement: green" diff --git a/scripts/tests/telemetry_token_refusal_arity.py b/scripts/tests/telemetry_token_refusal_arity.py new file mode 100644 index 00000000..6e2898ca --- /dev/null +++ b/scripts/tests/telemetry_token_refusal_arity.py @@ -0,0 +1,69 @@ +"""Every argument the token refusal is given has a placeholder to print it. + +Split out of `telemetry-token-agreement.sh` rather than inlined as a heredoc, +because a `.py` sidecar is readable and lintable — and because +`pyyaml-preflight.bats` now covers `.py` files in this directory, so a sidecar +here is inside the class rule rather than outside it (backend#2626). + +WHAT THIS CATCHES. `telemetryCollectorState` refuses with +`fail (printf "" )`, and the args include one `include` per Secret +name the lookup accepted. printf SILENTLY DROPS an argument with no placeholder, +so removing a `%q` from the format string leaves the name searched and unreported +— and the operator is told to create a Secret that may already exist under a name +the message never mentions. + +WHY NOT A SUBSTRING CHECK. That was the first cut, and it was vacuous: the format +string and its arguments are on ONE line, so `case $msg in *telemetryTokenName*` +matched the ARGUMENT even after the placeholder was deleted. Counting is the only +form that separates "given" from "printed". + +Prints a reason on mismatch and NOTHING on agreement, so the caller can treat any +output as the finding. Exits 0 either way: the caller decides, and a non-zero here +would be indistinguishable from "the interpreter is missing". +""" + +from __future__ import annotations + +import re +import sys + +MARKER = "telemetryCollector.enabled is true but its token Secret" +#: An argument that supplies a value: a helper include, a values read, or a +#: release field. Counted rather than named, so a fourth accepted name is covered. +ARGUMENT = re.compile(r'\(include "|\.Values\.|\.Release\.') +PLACEHOLDER = re.compile(r"%[qsvd]") + + +def check(path: str) -> str: + """`""` when the refusal can print everything it is given, else the reason.""" + try: + text = open(path, encoding="utf-8").read() + except OSError as exc: + return f"cannot read {path} ({exc}) — refusing to report agreement" + line = next((l for l in text.splitlines() if MARKER in l), "") + if not line: + return "could not isolate the token refusal line; this check proves nothing" + m = re.search(r'printf\s+"((?:[^"\\]|\\.)*)"(.*)$', line) + if not m: + return "could not parse the refusal's printf format string" + fmt, args = m.group(1), m.group(2) + placeholders = len(PLACEHOLDER.findall(fmt)) + supplied = len(ARGUMENT.findall(args)) + if not placeholders or not supplied: + return ( + f"parsed {placeholders} placeholder(s) and {supplied} argument(s) — one of " + "them is zero, so the comparison would be vacuous" + ) + if placeholders != supplied: + return ( + f"the refusal has {placeholders} placeholder(s) for {supplied} argument(s). " + "printf drops the extras, so a Secret name the lookup searched is not " + "reported — and the operator is told to create one that may already exist" + ) + return "" + + +if __name__ == "__main__": + reason = check(sys.argv[1]) + if reason: + print(reason) From 1559def112605ac1eb8b034981cb06ba135edf2c Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 13:49:06 +0200 Subject: [PATCH 09/22] =?UTF-8?q?chore(chart):=20bump=20to=201.9.91=20?= =?UTF-8?q?=E2=80=94=20develop=20reached=201.9.89=20while=20this=20PR=20wa?= =?UTF-8?q?s=20open?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- client/Chart.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/Chart.yaml b/client/Chart.yaml index 65e49efd..d79f4213 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.89 -appVersion: "1.9.89" +version: 1.9.91 +appVersion: "1.9.91" keywords: - tracebloc - kubernetes From cfd674597af2bd15ac2c3940e7692768ad5b3ddb Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 14:15:35 +0200 Subject: [PATCH 10/22] fix(chart): make the token refusal escapable after an override-to-override rename (backend#2626) Bugbot, Medium: `telemetryTokenPreOverrideName` covers `-telemetry-token`, so unset->A and mid-migration are handled, but A->B leaves the token under A`s name and the render hard-fails naming three names none of which will match. Correct, and the fix is NOT a fourth candidate name. WHY NOT ENUMERATE. Measured across the three consumers: the Collector`s volume (telemetry-collector-daemonset.yaml:197), the RBAC`s resourceNames (telemetry-token-rbac.yaml:102) and jobs-manager`s writer env (jobs-manager-deployment.yaml:351) ALL resolve `tracebloc.telemetryTokenSecretName` and nothing else. So a token discovered under any other name is one nothing is permitted to read - enumerating the namespace would turn a loud refusal into a green render with a Collector that cannot mount its token. That is strictly worse than the hard fail. What makes the hard fail acceptable is that it is ESCAPABLE, and it was not. The message now names the rename as the likely cause and gives the two ways out, neither of which is a reinstall: * copy the Secret to the name this render wants (the command is in the message) * leave telemetryCollector.enabled unset for one upgrade and let jobs-manager re-mint under the new name on its next re-authentication The second is safe because the Collector`s mount is `optional: true` and the daemonset says why in terms - "buffers until the token arrives" rather than "CrashLoopBackOff on every node" - so the window costs buffered telemetry, not node health. NO NEW PLACEHOLDERS, so telemetry-token-agreement.sh`s placeholder/argument arity check still holds; verified green. One design question left for the reviewer rather than decided here: whether the gate should skip the refusal entirely on a LIVE release, keying on the persisted MySQL PVC the way the credentials refusal now does. That covers every rename direction with no name arithmetic, and it costs the fresh-install protection on live releases. It changes the gate`s invariant, so it is the reviewer`s call and the evidence is on the thread. VERIFIED on helm v3.15.4: make drift 38/38; helm unittest 641/641; telemetry-token-agreement green; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 --- client/templates/_helpers.tpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/templates/_helpers.tpl b/client/templates/_helpers.tpl index 2d3e5db0..8fb734ec 100644 --- a/client/templates/_helpers.tpl +++ b/client/templates/_helpers.tpl @@ -1212,7 +1212,7 @@ no {{- if kindIs "bool" $tc.enabled -}} {{- if $tc.enabled -}} {{- if eq (include "tracebloc.telemetryTokenPresent" .) "no" -}} -{{- fail (printf "telemetryCollector.enabled is true but its token Secret does not exist in namespace %q — looked for %q, the legacy %q, and the pre-fullnameOverride %q. The Collector's exporter authenticates with it, and jobs-manager writes it (backend#2274). Create it, or set telemetryCollector.enabled: false — enabling without it buys a DaemonSet that spools to every node's disk and delivers nothing." .Values.nodeAgents.namespace.name (include "tracebloc.telemetryTokenSecretName" .) (include "tracebloc.telemetryTokenLegacyName" .) (include "tracebloc.telemetryTokenPreOverrideName" .)) -}} +{{- fail (printf "telemetryCollector.enabled is true but its token Secret does not exist in namespace %q — looked for %q, the legacy %q, and the pre-fullnameOverride %q. The Collector's exporter authenticates with it, and jobs-manager writes it (backend#2274). IF YOU JUST CHANGED fullnameOverride FROM ONE VALUE TO ANOTHER, the token is under the PREVIOUS override's name and this render cannot guess it: nothing records what the last one was, and enumerating the namespace would be worse than guessing, because the Collector's volume and the RBAC's resourceNames both name ONLY the first Secret above, so a token found under any other name is one nothing is permitted to read. Two ways forward, and neither is a reinstall: copy the existing Secret to the first name above in that namespace (kubectl get secret -o json | jq '.metadata.name=\"\" | del(.metadata.uid,.metadata.resourceVersion,.metadata.creationTimestamp,.metadata.ownerReferences)' | kubectl apply -f -), or leave telemetryCollector.enabled unset for one upgrade and let jobs-manager re-mint it under the new name on its next re-authentication — the Collector's mount is optional, so it waits and buffers rather than crash-looping. Otherwise create it, or set telemetryCollector.enabled: false — enabling without it buys a DaemonSet that spools to every node's disk and delivers nothing." .Values.nodeAgents.namespace.name (include "tracebloc.telemetryTokenSecretName" .) (include "tracebloc.telemetryTokenLegacyName" .) (include "tracebloc.telemetryTokenPreOverrideName" .)) -}} {{- end -}} enabled {{- else -}} From 9eb3c7e7a6f4d799ab088d7e7e0068f467e3bb45 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 15:24:18 +0200 Subject: [PATCH 11/22] fix(guard): make the fallback mitigation the property, not the arity (backend#2626) Co-Authored-By: Claude Opus 4.8 --- client/values.yaml | 9 ++++ scripts/install-k8s.ps1 | 12 ++++- scripts/lib/common.sh | 18 ++++++-- scripts/lib/summary.sh | 6 ++- .../tests/fullname-override-completeness.sh | 25 ++++++++++- scripts/tests/fullname_override_assertions.py | 44 ++++++++++++++++--- 6 files changed, 100 insertions(+), 14 deletions(-) diff --git a/client/values.yaml b/client/values.yaml index 5fe3342d..90440ed7 100644 --- a/client/values.yaml +++ b/client/values.yaml @@ -506,6 +506,15 @@ egressProxy: # `app.kubernetes.io/instance`, Helm's `meta.helm.sh/release-name` bookkeeping, # and the RELEASE_NAME env auto-upgrade rolls back with (backend#2620). # +# THE INSTALLER DOES NOT YET UNDERSTAND IT — backend#2888. `helm` renders the +# overridden names correctly, but the shell and PowerShell installers still derive +# the client's workload names as `-jobs-manager` / `-requests-proxy` +# (`scripts/lib/common.sh` `_client_workload_deployments`, `scripts/install-k8s.ps1` +# `Get-ClientDeploymentNames`). So on an overridden release the #420 fast-path +# health check reads UNHEALTHY and a re-run reinstalls over a working client +# instead of no-opping. Set this via `helm upgrade` directly until #2888 lands, +# or accept that the installer's idempotence check is wrong for this release. +# # IT DOES NOT MOVE DATA. Release-scoped on-disk paths -- the dataset directory, # the hostPath PVs, the Collector's queue -- keep the RELEASE name, because a # path is a location: renaming it orphans a tenant's data instead of moving it. diff --git a/scripts/install-k8s.ps1 b/scripts/install-k8s.ps1 index 67fc7e1f..65f42428 100644 --- a/scripts/install-k8s.ps1 +++ b/scripts/install-k8s.ps1 @@ -1386,8 +1386,16 @@ function Get-ClusterRunState { return (Get-ClusterRunStateFromList -Json $out -Name $CLUSTER_NAME) } -# The client's three workload deployments in a namespace. Single source of truth for -# both the readiness gate and the fast-path health check (#420). +# The client's three workload deployments in a namespace. One source for the two +# POWERSHELL consumers (the readiness gate and the #420 fast-path health check) -- +# NOT for the installer as a whole: `scripts/lib/common.sh` holds an independent +# `_client_workload_deployments` with the same list, and nothing checks the two +# agree. The previous "single source of truth" here named a guarantee that a +# second implementation makes impossible (@saadqbal on client#911). +# +# Neither copy resolves `fullnameOverride`: both assume `-` prefixes, +# so an overridden release reads UNHEALTHY on the fast path and a re-run +# reinstalls over a working client. Tracked as backend#2888. function Get-ClientDeploymentNames { param([string]$Namespace) return @("mysql-client", "$Namespace-jobs-manager", "$Namespace-requests-proxy") diff --git a/scripts/lib/common.sh b/scripts/lib/common.sh index 35f4de96..ddaf8457 100644 --- a/scripts/lib/common.sh +++ b/scripts/lib/common.sh @@ -533,11 +533,21 @@ _chart_version() { } # The client's core workload Deployments in namespace $1 — the set whose -# readiness DEFINES "the client is up". SINGLE SOURCE OF TRUTH: both +# readiness DEFINES "the client is up". One source for the two BASH consumers: # wait_for_client_ready (summary.sh, the post-install readiness gate) and the -# installer's stop-and-check gate (assess.sh) consume this, so the two can never -# drift on what "ready" / "healthy" means. Echoes one Deployment name per line; -# `mysql-client` is fixed, the other two are release-namespace-prefixed. +# installer's stop-and-check gate (assess.sh). It is NOT one source across tiers — +# `scripts/install-k8s.ps1` carries its own `Get-ClientDeploymentNames`, and +# nothing checks the two agree. +# +# `mysql-client` is fixed. The other two are prefixed by the release's RESOLVED +# NAME, which equals the namespace only while `fullnameOverride` is unset — the +# earlier "release-namespace-prefixed" here stated the default as the rule, and +# that is what made these sites easy to miss when the override landed +# (@saadqbal on client#911). Under an override the chart renders +# `-jobs-manager`, this function still looks for +# `-jobs-manager`, and the readiness gate therefore fails on a healthy +# client. Tracked as backend#2888; deliberately NOT fixed here, because the fix +# has to land in both tiers at once or they drift further apart. _client_workload_deployments() { local ns="${1:-${TB_NAMESPACE:-default}}" printf '%s\n' "mysql-client" "${ns}-jobs-manager" "${ns}-requests-proxy" diff --git a/scripts/lib/summary.sh b/scripts/lib/summary.sh index b7f527db..c0e58709 100755 --- a/scripts/lib/summary.sh +++ b/scripts/lib/summary.sh @@ -35,8 +35,10 @@ READY_TIMEOUT="${READY_TIMEOUT:-600}" wait_for_client_ready() { local ns="${TB_NAMESPACE:-default}" # The workloads that must be Ready are shared with the installer's stop-and-check - # gate (assess.sh) via _client_workload_deployments — single source of truth, so - # the readiness gate and the gate's "healthy" test can't drift. + # gate (assess.sh) via _client_workload_deployments, so those two cannot drift. + # NOT shared with the PowerShell installer, which holds its own copy — and + # neither copy resolves `fullnameOverride` (backend#2888), so an overridden + # release reads as not-Ready here while being perfectly healthy. local deploys=() _d while IFS= read -r _d; do [[ -n "$_d" ]] && deploys+=("$_d"); done < <(_client_workload_deployments "$ns") local deadline=$(( $(date +%s) + READY_TIMEOUT )) diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh index f51865a2..25f42540 100755 --- a/scripts/tests/fullname-override-completeness.sh +++ b/scripts/tests/fullname-override-completeness.sh @@ -99,6 +99,7 @@ tmp=$(mktemp -d); trap 'rm -rf "$tmp"' EXIT failures=0 # Accumulators for the cross-profile release-scoped-path assertion after the loop. path_profiles=0 +pathclass_missing=0 path_counts="" # NOTES NEEDS ITS OWN RENDER, AND EVERY OBVIOUS ROUTE IS CLOSED. Measured on the @@ -212,6 +213,22 @@ for VALUES in "${profiles[@]}"; do pc=$(grep -E '^PATHCLASS [0-9]+$' "$tmp/out.txt" | awk '{print $2}' | head -1 || true) path_counts="${path_counts}${prof}=${pc:-none} " case "${pc:-0}" in ''|0) ;; *) path_profiles=$((path_profiles + 1)) ;; esac + # NO COUNT MEANS NO ANSWER, NOT AN ANSWER OF ZERO (@saadqbal on client#911). + # A profile that never PRINTED `PATHCLASS` leaves `pc` empty, `path_profiles` + # unincremented, and assertion 6 below then reports "NO profile rendered a + # release-scoped on-disk path" — a claim about the CHART derived from the fact + # that we never looked. Two ways to reach it, and both are real: + # + # * a `python3` on PATH that exits 127 (stale pyenv shim, dangling symlink) — + # the sidecar produces nothing at all; + # * the sidecar's own `scanned == 0` refusal, which returns 1 BEFORE the + # PATHCLASS print, so a genuine "the walk sees nothing" finding collects a + # fabricated path-class one on top of it. + # + # Gating on the exit status would fix only the first: `scanned == 0` exits 1, + # which is a legitimate finding code. The property that actually licenses + # assertion 6 is that the count was PRODUCED, so that is what is tracked. + [ -n "${pc:-}" ] || pathclass_missing=$((pathclass_missing + 1)) if [ "$rc" -eq 2 ]; then echo "[ERROR] the guard could not run (see above). This is NOT a verdict on the chart." exit 2 @@ -272,7 +289,13 @@ fi # # Counts are printed either way so a reader can see WHICH profile contributed. echo "-- release-scoped paths per profile: ${path_counts% }" -if [ "$path_profiles" -eq 0 ]; then +if [ "$pathclass_missing" -ne 0 ]; then + echo "-- release-scoped path class: NOT CHECKED — $pathclass_missing profile(s)" + echo " produced no PATHCLASS count, so this assertion has no input. Not a" + echo " verdict on the chart in either direction; fix the finding(s) above and" + echo " re-run. (Reporting it as 'NO profile rendered a path' would be a claim" + echo " about the chart derived from never having looked.)" +elif [ "$path_profiles" -eq 0 ]; then echo "[ERROR] NO profile rendered a release-scoped on-disk path, so 'no path" echo " followed the override' proves nothing — it is equally true of a" echo " chart that stopped scoping paths by release. Either the paths" diff --git a/scripts/tests/fullname_override_assertions.py b/scripts/tests/fullname_override_assertions.py index aa716ce0..dc5ba354 100644 --- a/scripts/tests/fullname_override_assertions.py +++ b/scripts/tests/fullname_override_assertions.py @@ -249,7 +249,11 @@ def _mitigations(text, routed_vars): "refusal" a MISS on the routed lookup leads to `fail` — however the other half of the condition is computed "fallback" a second lookup in the same expression, on a name that does not - follow the override — `telemetryTokenPresent`'s legacy name + follow the override — `telemetryTokenPresent`'s legacy name. + NOT computed here: it is a property of the LINE, so it is + established per site by `_fallback_on_line` rather than once per + file. A file-wide answer let one qualifying line license every + routed lookup in it (client#911). THE REFUSAL IS DETECTED ON THE INVARIANT, NOT ON THE ARITHMETIC. The first cut matched `printf "%s-secrets" .Release.Name` — the shape the refusal happened to @@ -285,12 +289,39 @@ def _mitigations(text, routed_vars): # an unrelated `fail` further down the file. if any("fail " in nxt or "fail(" in nxt for nxt in lines[i : i + 4]): out.add("refusal") - for line in lines: - if line.split("#", 1)[0].count("lookup ") >= 2: - out.add("fallback") return out +def _fallback_on_line(code, following, vars_): + """True iff THIS line carries a Secret lookup the override cannot move. + + THE PROPERTY, NOT A PROXY FOR IT (@saadqbal on client#911). This used to be + `count("lookup ") >= 2` over every line in the FILE, and the result was reused + for every routed lookup in that file. Both halves were wrong, and the docstring + above already described the check this now performs — "a second lookup in the + same expression, on a name that does not follow the override" — while the code + asserted only that two `lookup ` substrings existed SOMEWHERE. + + Measured: routing all three telemetry-token names through `fullnameOverride` + leaves every probe missing on a rename, and the old detector still reported + "all 2 routed Secret lookup(s) of 2 carry a mitigation" and exited 0. It + counted the arity of the fallback and never its point — a fallback that also + follows the override is not a fallback, it is the same miss twice. + + So the question asked here is the one that matters: does at least one lookup on + this line key on a name the override CANNOT move? `.Release.Name` and a literal + both qualify; anything reaching `tracebloc.fullname` does not. + """ + for expr in _SECRET_LOOKUP.findall(code): + referenced = set(_INCLUDE.findall(expr)) + for v in re.findall(r"\$(\w+)", expr): + if v in vars_: + referenced.add(vars_[v]) + if not (referenced & following): + return True + return False + + #: A helper whose `fullname` reference sits AFTER an inner `if`/`end` — the exact #: shape the non-greedy regex dropped. Written down here rather than hunted for in #: the chart, because the chart does not currently contain one: the defect was @@ -391,7 +422,10 @@ def assert_lookup_keys(): if not routed: continue routed_sites += 1 - if not have: + # Per SITE, not per file: the refusal is a property of the file (it + # keys on routed_vars), the fallback is a property of this line. + site_have = have | ({"fallback"} if _fallback_on_line(code, following, vars_) else set()) + if not site_have: msgs.append( f" [ERROR] {path}: a Secret lookup keys on a name that follows " f"fullnameOverride ({', '.join(routed)}) and the file carries " From 7d0ea112296ea4cf0bbf7441fb8b36affed0e211 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 15:34:53 +0200 Subject: [PATCH 12/22] chore(manifest): regenerate after the guard-mitigation change (backend#2626) `make drift` was red on `gen-manifest.sh --check`: 9eb3c7e changed scripts/lib/summary.sh and scripts/install-k8s.ps1 without regenerating scripts/manifest.sha256, so the R8 static-analysis gate saw two stale digests. Manifest-only. Verified: make drift 39/39, helm unittest 641/641, bats 69/69 over the manifest/hygiene/style suites. Co-Authored-By: Claude Opus 4.8 --- scripts/manifest.sha256 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/manifest.sha256 b/scripts/manifest.sha256 index dd0ff0a9..68b46e30 100644 --- a/scripts/manifest.sha256 +++ b/scripts/manifest.sha256 @@ -1,5 +1,5 @@ 753502a4e76694bdc333eb2a0c898e4802cf6cce30f8968e95c95491472c3db8 scripts/install-k8s.sh -c260a2a4ce634de912a11a3a40484d825da449201a31a1a362473294ff95860e scripts/lib/common.sh +58c0b8a3409975cc97e391bc2854cb2fd340849dd4570f926821840e95015f12 scripts/lib/common.sh d773c8a11fa80d6880b5a104e2159de1dc9aaa35234ad6a764e4e1c49f601aec scripts/lib/telemetry.sh 3d15a215c460935a8ab4ef2a0991e378e2ff57c2c9d1a4550941d5cdb902e19f scripts/lib/preflight.sh a61f5bac3786a3283b5fa08fea9e522f7cea5f7fa44799a2b38b5caea42469d8 scripts/lib/detect-gpu.sh @@ -14,7 +14,7 @@ b569eec2d8ffb9673da287a2a59d249a7dbc7236c98ab6a5062136bcc69a942c scripts/lib/gp ea2bbd9948ee9e31e93271e235c630ced50d746e51a5629b5622041d8a39df07 scripts/lib/provision.sh fc6426e454f5c113748c32ce025cc1dce52d465cc4ca97850cd25ad3cf045c24 scripts/lib/assess.sh 6b3164415dcd793880b972fb61897179d58097f7d288e3904f6f09f858417a15 scripts/lib/probe.sh -e36c0fc3568aa5054ee6c853ced0791ad45951724fff059cff63847f5bc870e2 scripts/lib/summary.sh +3d66cce74afd7894bba313a7468d0e6c5e98a1f1aead4d2569791274d70dea9d scripts/lib/summary.sh 1b5e0fb3c9b100672dc3f547b677524f98f0a9ed28d8dc7de0f619d2687cfc52 scripts/lib/diagnose.sh -ad1fdec084f547917148d68d2e5a0b943faee84c120d9a417e59432c83dadb5a scripts/install-k8s.ps1 +d89e464cc5bf050ec50dfe64084f49cbbab6f7a8f143e98a0e959bde8bf79c5f scripts/install-k8s.ps1 fc6eeeb4e4114ce74566992519a90320c52434d604218a30ba8ef2c3fa266ca5 scripts/lib/telemetry.ps1 From 91e95f483934c2f12b40931cbac8ffe434aeaf51 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 16:10:37 +0200 Subject: [PATCH 13/22] fix(chart): the rename remedy is incomplete on hostPath, and said otherwise (backend#2626) Co-Authored-By: Claude Opus 4.8 --- client/templates/secrets.yaml | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/client/templates/secrets.yaml b/client/templates/secrets.yaml index 7d0e5497..c74f81c5 100644 --- a/client/templates/secrets.yaml +++ b/client/templates/secrets.yaml @@ -72,8 +72,24 @@ reachable half. */ -}} {{- $mysqlDataPresent := (lookup "v1" "PersistentVolumeClaim" .Release.Namespace (include "tracebloc.mysqlPvc" .)) -}} +{{- /* + "NOTHING ELSE HAS TO MOVE" IS ONLY TRUE OFF hostPath (Bugbot, High). + The PVC name is a literal and is `keep`-annotated, so it survives a rename -- + that half of the remedy is sound. The hostPath PVs are neither: their names go + through `tracebloc.fullname` (`mysqlPvName` / `clientDataPvName` / + `clientLogsPvName`) and only the PVCs carry `helm.sh/resource-policy: keep`. + So on a bare-metal install the copy-the-Secret remedy is INCOMPLETE: the + upgrade deletes `-mysql-pv` / `-data-pv` / `-logs-pv` and creates them + under the new name, while the retained PVCs stay Bound to the objects that + just went away. Telling an operator "nothing else has to move" there sends + them into exactly that. +*/ -}} +{{- $pvCaveat := "" -}} +{{- if .Values.hostPath.enabled -}} +{{- $pvCaveat = printf " NOTE, and this install is one: with hostPath enabled the PersistentVolumes %q, %q and %q DO follow fullnameOverride, and unlike the PVCs they carry no helm.sh/resource-policy: keep -- so copying the Secret is NOT sufficient on its own. The upgrade would delete those PVs and create them under the new name while the retained PVCs stay Bound to the deleted ones. Either keep the name (the ALTERNATIVELY path below), or re-point the PVCs at the new PVs by hand before re-running." (include "tracebloc.mysqlPvName" .) (include "tracebloc.clientDataPvName" .) (include "tracebloc.clientLogsPvName" .) -}} +{{- end -}} {{- if and $mysqlDataPresent (not $existingSecret) -}} -{{- fail (printf "release %q in namespace %q already has MySQL data (PersistentVolumeClaim %q, retained by helm.sh/resource-policy: keep), but there is no Secret named %q -- the name this render resolves to. The credentials below would therefore be re-minted while that database still holds the old ones: the upgrade would report deployed and MySQL would refuse every login. FIX: copy the existing credentials to the name this render wants, then re-run -- kubectl -n %s get secret -o json | jq '.metadata.name=\"%s\" | del(.metadata.uid,.metadata.resourceVersion,.metadata.creationTimestamp,.metadata.ownerReferences)' | kubectl apply -f - . The MySQL PVC is named %q and never follows fullnameOverride, so nothing else has to move. ALTERNATIVELY: put fullnameOverride back to the value this release was last rendered with, so the existing Secret matches again. IF YOU ARE DELIBERATELY STARTING FROM AN EMPTY DATABASE and there is no Secret to copy -- the uninstall-and-recover flow in docs/MIGRATIONS.md Option C, where the uninstall deleted it -- then delete the datadir and let this render mint fresh credentials: kubectl -n %s delete pvc %s . That is the accept-data-loss path and it is the only other way past this refusal." .Release.Name .Release.Namespace (include "tracebloc.mysqlPvc" .) $secretName .Release.Namespace $secretName (include "tracebloc.mysqlPvc" .) .Release.Namespace (include "tracebloc.mysqlPvc" .)) -}} +{{- fail (printf "release %q in namespace %q already has MySQL data (PersistentVolumeClaim %q, retained by helm.sh/resource-policy: keep), but there is no Secret named %q -- the name this render resolves to. The credentials below would therefore be re-minted while that database still holds the old ones: the upgrade would report deployed and MySQL would refuse every login. FIX: copy the existing credentials to the name this render wants, then re-run -- kubectl -n %s get secret -o json | jq '.metadata.name=\"%s\" | del(.metadata.uid,.metadata.resourceVersion,.metadata.creationTimestamp,.metadata.ownerReferences)' | kubectl apply -f - . The MySQL PVC is named %q and never follows fullnameOverride, so the DATABASE does not have to move.%s ALTERNATIVELY: put fullnameOverride back to the value this release was last rendered with, so the existing Secret matches again. IF YOU ARE DELIBERATELY STARTING FROM AN EMPTY DATABASE and there is no Secret to copy -- the uninstall-and-recover flow in docs/MIGRATIONS.md Option C, where the uninstall deleted it -- then delete the datadir and let this render mint fresh credentials: kubectl -n %s delete pvc %s . That is the accept-data-loss path and it is the only other way past this refusal." .Release.Name .Release.Namespace (include "tracebloc.mysqlPvc" .) $secretName .Release.Namespace $secretName (include "tracebloc.mysqlPvc" .) $pvCaveat .Release.Namespace (include "tracebloc.mysqlPvc" .)) -}} {{- end -}} {{- /* Platform client credentials (backend#2571). Issued by the backend, NOT From 9cd5f0f9ba2aba22cecec719500ddb592c4e7a46 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 16:31:37 +0200 Subject: [PATCH 14/22] fix(guard): a sidecar exit outside {0,1} is could-not-run, not a chart defect (backend#2626) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last half of @saadqbal`s interpreter finding. 9eb3c7e/head fixed the fabricated path-class claim - assertion 6 now reports NOT CHECKED rather than "NO profile rendered a release-scoped on-disk path" - but the final verdict still blamed the chart: $ PATH= bash scripts/tests/fullname-override-completeness.sh [ERROR] fullnameOverride is incomplete in 5 profile check(s) # rc 1 `if [ "$rc" -eq 2 ]` handled exactly 2, and `[ "$rc" -eq 0 ] || failures++` swept every other code into "the chart is incomplete" - 127 from a stale pyenv shim or a dangling symlink, 126 from a non-executable interpreter, 137 from an OOM kill. Now classified: 0 clean, 1 a finding, ANYTHING ELSE exits 2 with the named refusal. $ PATH= … [ERROR] the assertions exited 127, which is not a verdict … NOTHING ABOUT fullnameOverride WAS CHECKED. # rc 2 AND THE PREFLIGHT I ADDED CANNOT COVER THIS, which the comment now says because it read as though it could: `command -v python3` answers "is there something on PATH by that name", not "does it run". A shim that execs a deleted interpreter passes it and exits 127 here. Mutation-proved: folding 127/126 back into `failures` restores the false "incomplete in N profile check(s)" claim and rc 1; the classification restores rc 2 and zero such claims. VERIFIED at head on helm v3.15.4: make drift 39/39; helm unittest 641/641; shellcheck -S warning -x clean; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 --- .../tests/fullname-override-completeness.sh | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh index 25f42540..86295c75 100755 --- a/scripts/tests/fullname-override-completeness.sh +++ b/scripts/tests/fullname-override-completeness.sh @@ -229,11 +229,32 @@ for VALUES in "${profiles[@]}"; do # which is a legitimate finding code. The property that actually licenses # assertion 6 is that the count was PRODUCED, so that is what is tracked. [ -n "${pc:-}" ] || pathclass_missing=$((pathclass_missing + 1)) - if [ "$rc" -eq 2 ]; then - echo "[ERROR] the guard could not run (see above). This is NOT a verdict on the chart." - exit 2 - fi - [ "$rc" -eq 0 ] || failures=$((failures + 1)) + # EXIT CODES THE SIDECAR OWNS: 0 clean, 1 a real finding, 2 could-not-run. + # ANYTHING ELSE MEANS IT DID NOT RUN, and lumping those into `failures` was the + # half of @saadqbal's interpreter finding that survived the fix above: a + # `python3` that EXISTS and exits 127 -- a stale pyenv shim, a dangling symlink, + # a 126 from a non-executable interpreter, a 137 from an OOM kill -- produced no + # output, no PATHCLASS, and a final line reading "fullnameOverride is incomplete + # in N profile check(s)". A broken tool reported as a chart defect, which is the + # exact misdiagnosis the module-level PyYAML refusal and the interpreter + # preflight were both added to remove. + # + # THE `command -v python3` PREFLIGHT CANNOT COVER THIS, and it reads as though + # it does: `command -v` answers "is there something on PATH by that name", not + # "does it run". A shim that execs a deleted interpreter passes it and then + # exits 127 here. + case "$rc" in + 0) ;; + 1) failures=$((failures + 1)) ;; + *) + echo "[ERROR] the assertions exited $rc, which is not a verdict: 0 is clean," + echo " 1 is a finding, 2 is could-not-run. Anything else means the" + echo " sidecar did not run -- a python3 that exists but exits non-zero" + echo " (a stale shim), a non-executable interpreter, or a killed" + echo " process. NOTHING ABOUT fullnameOverride WAS CHECKED." + exit 2 + ;; + esac done # --- 5. THE GUARD REFUSES TO RUN HALF OF ITSELF ----------------------------- From 9de2d982d31f932cc92a404ac4863aebd0496ae7 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 16:37:50 +0200 Subject: [PATCH 15/22] fix(guard): "not checked" is not a pass, and the fallback detector gets a fixture (backend#2626) Two from @saadqbal, and they are the same sentence twice: an absence is not an answer. 1. The pathclass_missing branch printed NOT CHECKED and fell through to exit 0. The two causes it enumerates both raise `failures` elsewhere, so the branch looked safe -- but a third does not: rename the PATHCLASS marker in the sidecar, a pure refactor with every assertion intact, and assertion 6 stops asserting while DRIFT_GUARDS reports a pass. Now `could_not_run` -> exit 2, the vocabulary the interpreter loop already owns; a confirmed finding still outranks it, so a real defect is reported as one and not as "cannot tell". Mutation-proved with the anchor asserted: PATHCLASS -> PATH_CLASS took the guard from rc=0 to rc=2. 2. _fallback_on_line was pinned by nothing -- regress it to the file-wide `count("lookup ") >= 2` and the real chart still renders green, because the chart does not contain the shape that separates the two. Written-down fixture, same reasoning as _SELFTEST_TEMPLATE: two lookups both routed (the half the old form gets wrong), a literal-keyed real fallback, a single routed lookup, and the $var indirection. Mutation-proved: the old form reddens the first and the $var case. `make drift`: all 39 guards green. Co-Authored-By: Claude Opus 4.8 --- .../tests/fullname-override-completeness.sh | 25 ++++++ scripts/tests/fullname_override_assertions.py | 78 +++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh index 86295c75..277e71dc 100755 --- a/scripts/tests/fullname-override-completeness.sh +++ b/scripts/tests/fullname-override-completeness.sh @@ -100,6 +100,7 @@ failures=0 # Accumulators for the cross-profile release-scoped-path assertion after the loop. path_profiles=0 pathclass_missing=0 +could_not_run=0 path_counts="" # NOTES NEEDS ITS OWN RENDER, AND EVERY OBVIOUS ROUTE IS CLOSED. Measured on the @@ -316,6 +317,20 @@ if [ "$pathclass_missing" -ne 0 ]; then echo " verdict on the chart in either direction; fix the finding(s) above and" echo " re-run. (Reporting it as 'NO profile rendered a path' would be a claim" echo " about the chart derived from never having looked.)" + # AND IT MUST NOT EXIT 0 (@saadqbal, review of backend#2626). Printing this and + # falling through left assertion 6 asserting nothing while `DRIFT_GUARDS` + # reported a pass. The two causes enumerated above both raise `failures` + # elsewhere, so the branch LOOKED safe -- but a third does not: rename the + # `PATHCLASS` marker in the sidecar, a pure refactor with every assertion + # intact, and the guard prints NOT CHECKED and exits 0 everywhere. A required + # drift guard going vacuous in silence, on the class whose whole point is that + # renaming a path orphans a tenant's data. + # + # `could_not_run`, not `failures`: this is the vocabulary the interpreter loop + # above already owns -- 0 clean, 1 a real finding, 2 could-not-run -- and + # counting it as a finding would report a chart defect that was never observed, + # the same misdiagnosis the exit-code case was just fixed to stop making. + could_not_run=1 elif [ "$path_profiles" -eq 0 ]; then echo "[ERROR] NO profile rendered a release-scoped on-disk path, so 'no path" echo " followed the override' proves nothing — it is equally true of a" @@ -331,4 +346,14 @@ if [ "$failures" -ne 0 ]; then echo "[ERROR] fullnameOverride is incomplete in $failures profile check(s)" exit 1 fi +# A CONFIRMED FINDING OUTRANKS "could not tell", which is why this sits below the +# block above rather than replacing it: a run that both found a defect AND failed +# to check something should report the defect, the actionable half. With no +# finding, "could not tell" is the verdict -- and it is a non-zero one. +if [ "$could_not_run" -ne 0 ]; then + echo "[ERROR] one or more assertions had no input, so this run did not check" + echo " what it is required to check. That is not a pass: see the" + echo " NOT CHECKED line(s) above for which, and why." + exit 2 +fi echo "[OK] fullnameOverride routes every resource name, and no exception followed it" diff --git a/scripts/tests/fullname_override_assertions.py b/scripts/tests/fullname_override_assertions.py index dc5ba354..1da15b60 100644 --- a/scripts/tests/fullname_override_assertions.py +++ b/scripts/tests/fullname_override_assertions.py @@ -344,6 +344,78 @@ def _fallback_on_line(code, following, vars_): """ +#: THREE LINES THAT PIN `_fallback_on_line` TO ITS STATED PROPERTY (@saadqbal, +#: review of client#911). The detector it replaced -- `count("lookup ") >= 2` over +#: the whole FILE -- was quietly wrong for five rounds and nothing in the tree +#: could tell: regress to it and the real chart still renders green, because the +#: chart happens not to contain the shape that separates them. Same reasoning as +#: `_SELFTEST_TEMPLATE` above: a check exercisable only once the bug is present +#: arrives too late, so the input is written down rather than waited for. +#: +#: `_BOTH_ROUTED` is the one that does the pinning. Two lookups on one line, both +#: keyed on names the override moves -- so on a rename both probes miss together +#: and the "fallback" mitigates nothing. The old form counted two `lookup `s and +#: called it mitigated; the property says it is not. +_SELFTEST_BOTH_ROUTED = ( + '{{- $s := (lookup "v1" "Secret" .Release.Namespace (include "tracebloc.fullname" .)) ' + '| default (lookup "v1" "Secret" .Release.Namespace (include "probe.routed" .)) -}}' +) +#: A genuine fallback: the second lookup keys on a literal, which no override can +#: move, so it still resolves after a rename. +_SELFTEST_REAL_FALLBACK = ( + '{{- $s := (lookup "v1" "Secret" .Release.Namespace (include "tracebloc.fullname" .)) ' + '| default (lookup "v1" "Secret" .Release.Namespace "tracebloc-legacy-secret") -}}' +) +#: And the single-lookup routed case, which has no fallback at all -- the negative +#: control for a detector that simply answered True. +_SELFTEST_NO_FALLBACK = ( + '{{- $s := (lookup "v1" "Secret" .Release.Namespace (include "tracebloc.fullname" .)) -}}' +) + + +def selftest_the_fallback_detector(): + """`(ok, messages)` — `_fallback_on_line` asks the property, not the arity. + + Three halves, and the FIRST is the one the old form fails: without it, a + detector counting `lookup ` occurrences passes everything here. + """ + following = {"tracebloc.fullname", "probe.routed"} + msgs, ok = [], True + if _fallback_on_line(_SELFTEST_BOTH_ROUTED, following, {}): + ok = False + msgs.append( + " [ERROR] a line whose BOTH Secret lookups key on names the override " + "moves reads as mitigated. That is the arity of the fallback, not its " + "point — on a rename both probes miss together, and assertion 5 then " + "passes a chart where a routed credential Secret is unreachable.") + if not _fallback_on_line(_SELFTEST_REAL_FALLBACK, following, {}): + ok = False + msgs.append( + " [ERROR] a lookup keyed on a literal name is not recognised as a " + "fallback, so every genuinely mitigated site reports as unmitigated " + "and the assertion fails on a safe chart — noise that gets it muted.") + if _fallback_on_line(_SELFTEST_NO_FALLBACK, following, {}): + ok = False + msgs.append( + " [ERROR] a single routed lookup with no second probe reads as " + "mitigated, so the detector answers True regardless of its input.") + # THE `$var` INDIRECTION IS LIVE CODE and gets the same treatment: the same + # routed name reached through `$name` must read exactly as it does inline. + if _fallback_on_line( + '{{- $s := (lookup "v1" "Secret" .Release.Namespace (include "tracebloc.fullname" .)) ' + '| default (lookup "v1" "Secret" .Release.Namespace $probe) -}}', + following, {"probe": "probe.routed"}, + ): + ok = False + msgs.append( + " [ERROR] a routed name reached through a `$var` reads as an " + "unmovable one, so the indirection launders a routed lookup into a " + "mitigation.") + if ok: + msgs.append(" [OK] the fallback detector keys on the property, not the lookup count") + return ok, msgs + + def selftest_the_parser(): """`(ok, messages)` — the define parser reads a body past an inner `end`. @@ -684,6 +756,12 @@ def main() -> int: if not ok: fail = True + ok, msgs = selftest_the_fallback_detector() + for m in msgs: + print(m) + if not ok: + fail = True + ok, msgs = assert_lookup_keys() for m in msgs: print(m) From 2cd99cd73a21cdd94c5b35acd4fa3ee6b27cbd97 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 16:53:25 +0200 Subject: [PATCH 16/22] fix(chart): a path is not an object, and the PV caveat outlived the refusal (backend#2626) Two Bugbot findings on the pvCaveat added in 91e95f4. High: the caveat rendered ONLY inside the credential refusal, which is skipped once a Secret exists under the new name -- so an operator on their second attempt read the copy-the-Secret remedy with the caveat missing. values.yaml made it worse by saying "the hostPath PVs keep the RELEASE name": true of the on-disk PATH, false of the PersistentVolume OBJECTS, which are named through tracebloc.fullname and carry no helm.sh/resource-policy: keep. Following that on bare metal deletes the PVs and leaves the retained PVCs Bound to them. Separated path from object in values.yaml, and NOTES.txt now carries the caveat on every hostPath render -- the path the refusal cannot reach. Gated on hostPath alone, not on fullnameOverride being set, because the A->unset rename is the same hazard and would have gone quiet. Medium: .Values.hostPath.enabled read without the (default dict ...) guard the rest of the chart uses. Guarded. Note the stated consequence does not hold as written: with hostPath nil the render already dies in shared-images-pvc.yaml:3, before secrets.yaml, both before and after this commit. Nine other unguarded sites remain and are filed separately -- one of ten does not fix the nil case. make drift: all 39 guards green. NOTES verified rendered under --set hostPath.enabled=true and absent when false. Co-Authored-By: Claude Opus 4.8 --- client/templates/NOTES.txt | 9 +++++++++ client/templates/secrets.yaml | 5 ++++- client/values.yaml | 23 ++++++++++++++++++++--- 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/client/templates/NOTES.txt b/client/templates/NOTES.txt index d941b10e..154269bf 100644 --- a/client/templates/NOTES.txt +++ b/client/templates/NOTES.txt @@ -34,6 +34,15 @@ - {{ "\033[0;36m" }}{{ include "tracebloc.clientLogsPvc" . }}{{ "\033[0m" }} {{ "\033[0;90m" }}({{ include "tracebloc.clientLogsStorage" . }}){{ "\033[0m" }} - {{ "\033[0;36m" }}{{ include "tracebloc.mysqlPvc" . }}{{ "\033[0m" }} {{ "\033[0;90m" }}({{ include "tracebloc.mysqlStorage" . }}){{ "\033[0m" }} +{{- if (default dict .Values.hostPath).enabled }} + +{{ "\033[1;33m" }}hostPath + fullnameOverride:{{ "\033[0m" }} {{ "\033[0;90m" }}the PersistentVolume OBJECTS below are named through the override and carry NO helm.sh/resource-policy: keep, unlike the PVCs. If you just changed fullnameOverride, confirm the PVCs are still Bound before trusting this release -- a rename recreates the PVs under the new name and leaves the retained PVCs Bound to the deleted ones.{{ "\033[0m" }} + - {{ "\033[0;36m" }}{{ include "tracebloc.mysqlPvName" . }}{{ "\033[0m" }} + - {{ "\033[0;36m" }}{{ include "tracebloc.clientDataPvName" . }}{{ "\033[0m" }} + - {{ "\033[0;36m" }}{{ include "tracebloc.clientLogsPvName" . }}{{ "\033[0m" }} + {{ "\033[1;32m" }}${{ "\033[0m" }} kubectl get pvc -n {{ "\033[0;33m" }}{{ .Release.Namespace }}{{ "\033[0m" }} +{{- end }} + {{ "\033[1;35m" }}Verify the deployment:{{ "\033[0m" }} {{ "\033[1;32m" }}${{ "\033[0m" }} kubectl get pods -n {{ "\033[0;33m" }}{{ .Release.Namespace }}{{ "\033[0m" }} -l app.kubernetes.io/instance={{ "\033[0;36m" }}{{ .Release.Name }}{{ "\033[0m" }} {{ "\033[1;32m" }}${{ "\033[0m" }} kubectl get pods -n {{ "\033[0;33m" }}{{ .Release.Namespace }}{{ "\033[0m" }} -l app={{ "\033[0;36m" }}manager{{ "\033[0m" }} diff --git a/client/templates/secrets.yaml b/client/templates/secrets.yaml index c74f81c5..d3c313e3 100644 --- a/client/templates/secrets.yaml +++ b/client/templates/secrets.yaml @@ -85,7 +85,10 @@ them into exactly that. */ -}} {{- $pvCaveat := "" -}} -{{- if .Values.hostPath.enabled -}} +{{- /* NIL-GUARDED like every other hostPath read in this chart: `--reuse-values` + from a release that predates the key leaves `.Values.hostPath` nil, and an + unguarded `.enabled` then dies mid-render (Bugbot, Medium). */ -}} +{{- if (default dict .Values.hostPath).enabled -}} {{- $pvCaveat = printf " NOTE, and this install is one: with hostPath enabled the PersistentVolumes %q, %q and %q DO follow fullnameOverride, and unlike the PVCs they carry no helm.sh/resource-policy: keep -- so copying the Secret is NOT sufficient on its own. The upgrade would delete those PVs and create them under the new name while the retained PVCs stay Bound to the deleted ones. Either keep the name (the ALTERNATIVELY path below), or re-point the PVCs at the new PVs by hand before re-running." (include "tracebloc.mysqlPvName" .) (include "tracebloc.clientDataPvName" .) (include "tracebloc.clientLogsPvName" .) -}} {{- end -}} {{- if and $mysqlDataPresent (not $existingSecret) -}} diff --git a/client/values.yaml b/client/values.yaml index 90440ed7..831ccbc1 100644 --- a/client/values.yaml +++ b/client/values.yaml @@ -515,9 +515,18 @@ egressProxy: # instead of no-opping. Set this via `helm upgrade` directly until #2888 lands, # or accept that the installer's idempotence check is wrong for this release. # -# IT DOES NOT MOVE DATA. Release-scoped on-disk paths -- the dataset directory, -# the hostPath PVs, the Collector's queue -- keep the RELEASE name, because a -# path is a location: renaming it orphans a tenant's data instead of moving it. +# IT DOES NOT MOVE DATA. Release-scoped on-disk PATHS -- the dataset directory, +# the hostPath PV's `path`, the Collector's queue -- keep the RELEASE name, +# because a path is a location: renaming it orphans a tenant's data instead of +# moving it. +# +# A PATH IS NOT AN OBJECT, and this comment used to conflate the two (Bugbot, +# High). The hostPath PVs' on-disk paths stay put, but the PersistentVolume +# OBJECTS are named through `tracebloc.fullname` -- `-mysql-pv`, +# `-data-pv`, `-logs-pv` -- so their names DO follow this key. Unlike the PVCs +# they carry no `helm.sh/resource-policy: keep`, so on a bare-metal (hostPath) +# release a rename deletes those PV objects and creates them under the new name +# while the retained PVCs stay Bound to the ones that just went away. # # CHANGING IT ON A LIVE RELEASE IS REFUSED, not merely discouraged. Earlier # revisions of this comment called it "a migration, not a config tweak", which @@ -530,6 +539,14 @@ egressProxy: # `mysql-pvc` datadir still holds the old ones, the upgrade would report # `deployed`, and MySQL would refuse every login. # +# ON hostPath, COPYING THE SECRET IS NOT THE WHOLE REMEDY -- see the PV-object +# paragraph above, and re-point the PVCs at the new PVs (or keep the old name). +# The refusal says so too, but only while it FIRES: once a Secret exists under +# the new name it is skipped, and an operator on their second attempt would +# otherwise read this remedy with the caveat missing. NOTES.txt repeats it on +# every hostPath render of an overridden release, which is the path the refusal +# cannot reach. +# # So: choose it at INSTALL time. On a live release, read the refusal message. # # The commented example below is the EMPTY STRING, which the template treats as From 976094495bdf727a5a9d33d8d4df5a0e30b3bb57 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 17:07:43 +0200 Subject: [PATCH 17/22] test(chart): the PV rename hazard gets a tripwire, and a refusal to go with the notice (backend#2626) Builds on 2cd99cd rather than replacing it. That commit fixed both Bugbot findings by correcting the values.yaml conflation and repeating the caveat in NOTES.txt; this adds the two things it left implicit. A REFUSAL, alongside the notice. NOTES.txt and this guard cover different halves and neither substitutes for the other. NOTES.txt renders on every hostPath install including dry-runs, so it is the half that is always visible -- but it prints after a successful render and cannot stop the upgrade that deletes the PVs. The new guard in secrets.yaml blocks that upgrade: it compares the retained claim's spec.volumeName against the volume this render names, and refuses when they disagree. It reads the claim rather than doing a PV lookup because spec.volumeName is written by the binding controller, needs no cluster-scoped RBAC, and cannot come back empty-and-look-renamed the way a forbidden lookup would. Scoped to hostPath because off hostPath the provisioner names volumes pvc-, which would make it refuse every storage-class install. It names the observed pair and both causes -- a rename, or hostPath switched on over a dynamically provisioned claim -- rather than asserting which. A TRIPWIRE UNDER THE PROSE. Nothing tested any of this. hostpath_pv_rename_test pins the four facts both comments now assert, from render output rather than from a restatement: PV names follow the override, claim names do not, claims carry helm.sh/resource-policy keep, and the PVs do NOT. That last pair is the tripwire -- annotating the PVs keep is the real fix for this hazard, and when someone does it those cases go red and force the values.yaml paragraph and this guard to be revisited in the same PR. Each fact is asserted with the override both set and unset, since the set-only form would pass on a chart that named everything renamed-* unconditionally. Two things measured along the way. The claim's spec.volumeName is set by no template, so the chart test asserts the binding from the PV's claimRef end and says so; and on helm-unittest 0.5.2 a documentIndex is silently ignored when a suite scopes several templates and a test narrows with template:, evaluating against document 0 while reporting DocumentIndex 0 -- all six claim tests here ran against the PersistentVolume before switching to documentSelector. Also: a scoped regression test for the nil-guard, which had none. It is provable only template-scoped, because a whole-chart render under hostPath: null still dies at shared-images-pvc.yaml:3 either way. And 2cd99cd's new comment said the guard was "like every other hostPath read in this chart" -- measured 8 guarded, 9 bare on this tree, so the sentence is corrected and points at backend#2910. Filed a duplicate of that ticket (#2911) and closed it, moving its three additional measurements onto #2910. helm unittest 654/654 (was 641). make drift 39/39. Every claim mutation-proved: PV gains keep -> 1 red; PV name stops following -> 1 red; claim name starts following -> 3 red; host path starts following -> 1 red; nil-guard reverted to the bare read -> the secrets case reddens with the nil pointer. Co-Authored-By: Claude Opus 4.8 --- client/templates/secrets.yaml | 53 ++++- client/tests/hostpath_pv_rename_test.yaml | 265 ++++++++++++++++++++++ client/tests/secrets_test.yaml | 20 ++ 3 files changed, 335 insertions(+), 3 deletions(-) create mode 100644 client/tests/hostpath_pv_rename_test.yaml diff --git a/client/templates/secrets.yaml b/client/templates/secrets.yaml index d3c313e3..b08eb9dd 100644 --- a/client/templates/secrets.yaml +++ b/client/templates/secrets.yaml @@ -85,15 +85,62 @@ them into exactly that. */ -}} {{- $pvCaveat := "" -}} -{{- /* NIL-GUARDED like every other hostPath read in this chart: `--reuse-values` - from a release that predates the key leaves `.Values.hostPath` nil, and an - unguarded `.enabled` then dies mid-render (Bugbot, Medium). */ -}} +{{- /* NIL-GUARDED: `--reuse-values` from a release that predates the key leaves + `.Values.hostPath` nil, and an unguarded `.enabled` then dies mid-render + (Bugbot, Medium). + + NOT "like every other read in this chart" -- that was this comment's first + wording and it is not true. Measured on this tree: 8 sites go through + `default dict`, 9 read `.enabled` bare, so the guarded form is barely half. + The nine are backend#2910. The practical consequence for anyone reading this + line as reassurance: a whole-chart render under `hostPath: null` still dies at + `shared-images-pvc.yaml:3`, before this file, so this guard is provable only + under a TEMPLATE-SCOPED render -- which is what + `client/tests/secrets_test.yaml` does. */ -}} {{- if (default dict .Values.hostPath).enabled -}} {{- $pvCaveat = printf " NOTE, and this install is one: with hostPath enabled the PersistentVolumes %q, %q and %q DO follow fullnameOverride, and unlike the PVCs they carry no helm.sh/resource-policy: keep -- so copying the Secret is NOT sufficient on its own. The upgrade would delete those PVs and create them under the new name while the retained PVCs stay Bound to the deleted ones. Either keep the name (the ALTERNATIVELY path below), or re-point the PVCs at the new PVs by hand before re-running." (include "tracebloc.mysqlPvName" .) (include "tracebloc.clientDataPvName" .) (include "tracebloc.clientLogsPvName" .) -}} {{- end -}} {{- if and $mysqlDataPresent (not $existingSecret) -}} {{- fail (printf "release %q in namespace %q already has MySQL data (PersistentVolumeClaim %q, retained by helm.sh/resource-policy: keep), but there is no Secret named %q -- the name this render resolves to. The credentials below would therefore be re-minted while that database still holds the old ones: the upgrade would report deployed and MySQL would refuse every login. FIX: copy the existing credentials to the name this render wants, then re-run -- kubectl -n %s get secret -o json | jq '.metadata.name=\"%s\" | del(.metadata.uid,.metadata.resourceVersion,.metadata.creationTimestamp,.metadata.ownerReferences)' | kubectl apply -f - . The MySQL PVC is named %q and never follows fullnameOverride, so the DATABASE does not have to move.%s ALTERNATIVELY: put fullnameOverride back to the value this release was last rendered with, so the existing Secret matches again. IF YOU ARE DELIBERATELY STARTING FROM AN EMPTY DATABASE and there is no Secret to copy -- the uninstall-and-recover flow in docs/MIGRATIONS.md Option C, where the uninstall deleted it -- then delete the datadir and let this render mint fresh credentials: kubectl -n %s delete pvc %s . That is the accept-data-loss path and it is the only other way past this refusal." .Release.Name .Release.Namespace (include "tracebloc.mysqlPvc" .) $secretName .Release.Namespace $secretName (include "tracebloc.mysqlPvc" .) $pvCaveat .Release.Namespace (include "tracebloc.mysqlPvc" .)) -}} {{- end -}} +{{- /* + AND THE PV RENAME IS REFUSED ON ITS OWN TERMS, not only described. + + WHY THIS EXISTS ALONGSIDE THE NOTES.txt CAVEAT. Those two cover different + halves and neither covers both. NOTES.txt renders on every hostPath install, + including `helm template` and every dry-run, so it is the half that is always + VISIBLE -- but it is advisory: it prints after a successful render and cannot + stop the upgrade that deletes the PVs. This guard is the half that BLOCKS. It + is `lookup`-based and therefore inert in every client-side renderer, so it can + only ever fire where the damage would actually happen. Advisory-everywhere plus + blocking-where-it-counts; drop either and the other does not cover for it. + + WHAT IT COMPARES, and why the claim rather than a PV lookup: the claim is + namespaced and already fetched above, and its `spec.volumeName` records the PV + it is actually Bound to -- written by the binding controller, which is why no + template sets it and no chart test can see it. Reading it needs no + cluster-scoped PV read, so a client without PV RBAC gets a correct answer + instead of an empty `lookup` that would read as "renamed" on every upgrade and + refuse all of them. + + WHY `$hostPathOn` IS LOAD-BEARING rather than decoration: off hostPath the + volume is provisioned dynamically and named `pvc-` by the provisioner, + which never matches `tracebloc.fullname`. Without that conjunct this guard + would refuse every single storage-class install. + + IT STATES WHAT IT MEASURED. A mismatch here is a rename in the overwhelming + case, but it is also what "hostPath switched on over a dynamically provisioned + claim" looks like, and that needs the same hand-repair. The message names the + observed pair and both causes rather than asserting one. +*/ -}} +{{- $hostPathOn := (default dict .Values.hostPath).enabled | default false -}} +{{- $boundPv := "" -}} +{{- if $mysqlDataPresent -}} +{{- $boundPv = dig "spec" "volumeName" "" $mysqlDataPresent -}} +{{- end -}} +{{- if and $hostPathOn $boundPv (ne $boundPv (include "tracebloc.mysqlPvName" .)) -}} +{{- fail (printf "release %q in namespace %q has hostPath enabled and its retained PersistentVolumeClaim %q is Bound to PersistentVolume %q, but this render names that volume %q. The hostPath PersistentVolumes follow fullnameOverride (%q, %q, %q) and, unlike the PVCs, carry no helm.sh/resource-policy: keep -- so this upgrade would DELETE the three PVs the claims are Bound to and create them under the new names, leaving the kept PVCs Bound to objects that no longer exist. The datadir itself is not at risk: the on-disk path keeps the RELEASE name, which this value does not change. TWO CAUSES look like this. (1) fullnameOverride moved on a live release -- put it back to the value this release was last rendered with, or delete and recreate the three PVCs against the new PVs by hand before re-running. (2) hostPath was switched ON over a release whose claims were dynamically provisioned -- a provisioner-generated name looks like pvc-, so compare that against %q rather than taking this guess -- and that is a storage migration, not a config change; keep hostPath off, or move the data deliberately. NOTE: copying the Secret, which the credential refusal asks for, does NOT cover this -- the two are independent, which is why you can see this message without having seen that one." .Release.Name .Release.Namespace (include "tracebloc.mysqlPvc" .) $boundPv (include "tracebloc.mysqlPvName" .) (include "tracebloc.mysqlPvName" .) (include "tracebloc.clientDataPvName" .) (include "tracebloc.clientLogsPvName" .) $boundPv) -}} +{{- end -}} {{- /* Platform client credentials (backend#2571). Issued by the backend, NOT generated here — so tier 3 is a hard failure rather than `randAlphaNum`, diff --git a/client/tests/hostpath_pv_rename_test.yaml b/client/tests/hostpath_pv_rename_test.yaml new file mode 100644 index 00000000..ee45176f --- /dev/null +++ b/client/tests/hostpath_pv_rename_test.yaml @@ -0,0 +1,265 @@ +suite: hostPath PV rename hazard +# Locks down the facts `values.yaml`'s fullnameOverride block asserts about +# hostPath storage, and that they are what makes `secrets.yaml`'s standalone PV +# refusal necessary (backend#2626, Bugbot High). +# +# THE HAZARD, in one sentence: the PV OBJECTS follow fullnameOverride and carry +# no `helm.sh/resource-policy: keep`, while the claims Bound to them are literal +# names that DO carry `keep` -- so changing the override deletes the volumes out +# from under retained claims. +# +# WHY THIS IS A CHART TEST AND NOT A PROSE GUARD. Every assertion below reads +# RENDER OUTPUT, so it cannot drift from the templates the way a hand-written +# claim can. In particular the `notExists` cases are TRIPWIRES, not preferences: +# the proper fix for the hazard is to annotate the PVs `keep`, and when someone +# does that those cases go red and force the `values.yaml` paragraph and the +# `secrets.yaml` guard to be revisited in the same PR. A green run here means +# "the prose is still true", not "the chart is ideal". +# +# WHY EACH FACT IS ASSERTED BOTH WAYS. The override-set cases alone would pass +# on a chart that named every PV `renamed-*` unconditionally, so each is paired +# with the override unset. That pairing is the test, not repetition. +# +# WHAT THIS SUITE CANNOT SEE, stated rather than implied. `secrets.yaml`'s guard +# reads the live claim's `spec.volumeName` -- and no template sets that field. +# The chart declares the binding from the other end (`spec.claimRef` on the PV); +# `volumeName` is written by the binding controller once the claim is Bound, so +# it exists on a live cluster and never in a render. Both refusals in +# `secrets.yaml` are `lookup`-based and inert here for the same reason (no +# cluster => empty `lookup`); `scripts/tests/` covers the reachable half. What is +# assertable without a cluster is the naming and annotation asymmetry, which is +# what this suite pins. +# +# NOTE ON SHAPE: these tests select their document BY KIND, not by index. On +# helm-unittest 0.5.2 a `documentIndex` is silently ignored when the suite scopes +# more than one template and the test narrows with `template:` -- the assertion +# is evaluated against document 0 while the report prints `DocumentIndex: 0`, so +# a claim assertion reads as a failing volume assertion (a *passing* one would +# have been worse). `documentSelector` says which object it means, so it cannot +# land on the wrong one. Measured 2026-08-31: under `documentIndex: 1` all six +# claim tests here ran against the PersistentVolume. +templates: + - templates/mysql-storage-pvc.yaml + - templates/logs-pvc.yaml + - templates/shared-images-pvc.yaml +release: + name: stg + namespace: tracebloc +set: + clientId: "test-id" + clientPassword: "test" + hostPath: + enabled: true +tests: + # ---------- MySQL state store ---------- + - it: the mysql-pv volume follows fullnameOverride, is not retained, and still claims a fixed name + template: templates/mysql-storage-pvc.yaml + documentSelector: + path: kind + value: PersistentVolume + set: + fullnameOverride: renamed + asserts: + - equal: + path: metadata.name + value: renamed-mysql-pv + - notExists: + path: metadata.annotations["helm.sh/resource-policy"] + # The asymmetry inside one object: the volume's own name moved, the claim + # it is hard-bound to did not. That is the break, in two adjacent fields. + - equal: + path: spec.claimRef.name + value: mysql-pvc + # And the DATA does not move -- the host path keeps the RELEASE name, which + # fullnameOverride does not change. This is the half of the `values.yaml` + # sentence that was always true, pinned so the correction stays honest. + - equal: + path: spec.hostPath.path + value: /tracebloc/stg/mysql + + - it: the mysql-pv volume is release-named when the override is unset + template: templates/mysql-storage-pvc.yaml + documentSelector: + path: kind + value: PersistentVolume + set: + fullnameOverride: "" + asserts: + - equal: + path: metadata.name + value: stg-mysql-pv + - equal: + path: spec.hostPath.path + value: /tracebloc/stg/mysql + + - it: the mysql-pvc claim keeps its name and its keep annotation under a rename + template: templates/mysql-storage-pvc.yaml + documentSelector: + path: kind + value: PersistentVolumeClaim + set: + fullnameOverride: renamed + asserts: + - equal: + path: metadata.name + value: mysql-pvc + - equal: + path: metadata.annotations["helm.sh/resource-policy"] + value: keep + + - it: the mysql-pvc claim renders identically with the override unset + template: templates/mysql-storage-pvc.yaml + documentSelector: + path: kind + value: PersistentVolumeClaim + set: + fullnameOverride: "" + asserts: + - equal: + path: metadata.name + value: mysql-pvc + - equal: + path: metadata.annotations["helm.sh/resource-policy"] + value: keep + + # ---------- client logs ---------- + - it: the logs-pv volume follows fullnameOverride, is not retained, and still claims a fixed name + template: templates/logs-pvc.yaml + documentSelector: + path: kind + value: PersistentVolume + set: + fullnameOverride: renamed + asserts: + - equal: + path: metadata.name + value: renamed-logs-pv + - notExists: + path: metadata.annotations["helm.sh/resource-policy"] + # The asymmetry inside one object: the volume's own name moved, the claim + # it is hard-bound to did not. That is the break, in two adjacent fields. + - equal: + path: spec.claimRef.name + value: client-logs-pvc + # And the DATA does not move -- the host path keeps the RELEASE name, which + # fullnameOverride does not change. This is the half of the `values.yaml` + # sentence that was always true, pinned so the correction stays honest. + - equal: + path: spec.hostPath.path + value: /tracebloc/stg/logs + + - it: the logs-pv volume is release-named when the override is unset + template: templates/logs-pvc.yaml + documentSelector: + path: kind + value: PersistentVolume + set: + fullnameOverride: "" + asserts: + - equal: + path: metadata.name + value: stg-logs-pv + - equal: + path: spec.hostPath.path + value: /tracebloc/stg/logs + + - it: the client-logs-pvc claim keeps its name and its keep annotation under a rename + template: templates/logs-pvc.yaml + documentSelector: + path: kind + value: PersistentVolumeClaim + set: + fullnameOverride: renamed + asserts: + - equal: + path: metadata.name + value: client-logs-pvc + - equal: + path: metadata.annotations["helm.sh/resource-policy"] + value: keep + + - it: the client-logs-pvc claim renders identically with the override unset + template: templates/logs-pvc.yaml + documentSelector: + path: kind + value: PersistentVolumeClaim + set: + fullnameOverride: "" + asserts: + - equal: + path: metadata.name + value: client-logs-pvc + - equal: + path: metadata.annotations["helm.sh/resource-policy"] + value: keep + + # ---------- shared dataset images ---------- + - it: the data-pv volume follows fullnameOverride, is not retained, and still claims a fixed name + template: templates/shared-images-pvc.yaml + documentSelector: + path: kind + value: PersistentVolume + set: + fullnameOverride: renamed + asserts: + - equal: + path: metadata.name + value: renamed-data-pv + - notExists: + path: metadata.annotations["helm.sh/resource-policy"] + # The asymmetry inside one object: the volume's own name moved, the claim + # it is hard-bound to did not. That is the break, in two adjacent fields. + - equal: + path: spec.claimRef.name + value: client-pvc + # And the DATA does not move -- the host path keeps the RELEASE name, which + # fullnameOverride does not change. This is the half of the `values.yaml` + # sentence that was always true, pinned so the correction stays honest. + - equal: + path: spec.hostPath.path + value: /tracebloc/stg/data + + - it: the data-pv volume is release-named when the override is unset + template: templates/shared-images-pvc.yaml + documentSelector: + path: kind + value: PersistentVolume + set: + fullnameOverride: "" + asserts: + - equal: + path: metadata.name + value: stg-data-pv + - equal: + path: spec.hostPath.path + value: /tracebloc/stg/data + + - it: the client-pvc claim keeps its name and its keep annotation under a rename + template: templates/shared-images-pvc.yaml + documentSelector: + path: kind + value: PersistentVolumeClaim + set: + fullnameOverride: renamed + asserts: + - equal: + path: metadata.name + value: client-pvc + - equal: + path: metadata.annotations["helm.sh/resource-policy"] + value: keep + + - it: the client-pvc claim renders identically with the override unset + template: templates/shared-images-pvc.yaml + documentSelector: + path: kind + value: PersistentVolumeClaim + set: + fullnameOverride: "" + asserts: + - equal: + path: metadata.name + value: client-pvc + - equal: + path: metadata.annotations["helm.sh/resource-policy"] + value: keep diff --git a/client/tests/secrets_test.yaml b/client/tests/secrets_test.yaml index 7e339bc9..dc965121 100644 --- a/client/tests/secrets_test.yaml +++ b/client/tests/secrets_test.yaml @@ -3,6 +3,26 @@ templates: - templates/secrets.yaml - templates/docker-registry-secret.yaml tests: + # --- a nil `hostPath` must not nil-pointer this template (backend#2626, Bugbot Medium) --- + # `hostPath` is reachable as nil: `-f` with `hostPath: null`, or `--reuse-values` + # from a release predating the key. The chart is SPLIT on how it reads it -- 8 + # sites go through `default dict`, 9 read `.Values.hostPath.enabled` bare -- so + # a full render of that input dies at `shared-images-pvc.yaml:3` no matter what + # this template does (backend#2910 tracks the other nine). This suite scopes to + # `secrets.yaml`, which is what makes the assertion observable at all: it proves + # THIS template survives the input, which a whole-chart render cannot show while + # an earlier site still panics on it. + - it: renders under a nil hostPath instead of nil-pointering + template: templates/secrets.yaml + set: + clientId: "my-client-id" + clientPassword: "my-secret-pass" + hostPath: null + asserts: + - isKind: + of: Secret + documentIndex: 0 + - it: should create tracebloc secret with encoded password template: templates/secrets.yaml set: From ddbc43109f71bc8a2a0497f9635d29e444f0eecf Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 31 Aug 2026 17:26:36 +0200 Subject: [PATCH 18/22] fix(guard): the identity-env class also has to name the right identity (backend#2626) Layered on 7dc86c2's `identity_env_sites`, which landed the same Bugbot Medium concurrently. Both of us reproduced the fail-open and fixed it differently; this keeps that commit's fix as the base and adds the two cases it cannot see, plus the chart-level backstop. WHAT 7dc86c2 ALREADY DOES, kept unchanged: derives the site set from the render with the override UNSET and requires every site to still be there with it set, so a routed site is a DISAPPEARANCE rather than an absence. That is the better half of the two approaches -- it catches a site vanishing for any reason, not only a routed value -- and replacing it with a same-shaped enumeration of my own would have been a discard, not a merge. WHAT IT CANNOT SEE, and both are now checked: * `identity_env_sites` accepts a value in (rel, ns) for ANY of the three names, so swapping RELEASE_NAME with RELEASE_NAMESPACE keys the same triple on both sides and passes -- while a consumer reading RELEASE_NAME gets the namespace. Each name is now checked against its own meaning. Assertable only because this profile deliberately makes rel and ns differ; under the installer's one-string-for-both convention (backend#2621) it would not be. * A `valueFrom` env has no literal to read, so it drops silently out of BOTH site sets and reads as agreement. It is now reported as a site the check cannot see, which is the finding rather than a pass (rule 3). AND PER-PROFILE EMPTINESS IS NOT A CHART FINDING. 7dc86c2 failed the guard when the default render named zero identity envs, which would refuse a complete chart on a profile rendering neither CronJob nor the storage-assertions Job -- the mistake the PATH class already made and had demoted after measuring. Replaced with the reviewed PATHCLASS shape: an `ENVCLASS` count out of the sidecar, asserted once ACROSS profiles by the shell (new assertion 7), where a missing marker exits 2 (could-not-run) instead of 0. Measured at head: aks 5, bm 5, eks 5, oc 5. Also took 7dc86c2's NOTES wording over mine -- same conclusion, tighter -- and added one clause it lacked: that the destructive upgrade is now refused at template time, so the notice tells the operator they are protected rather than only that a hazard exists. Mutation-proved, and the split is the point: routing one RELEASE_NAME -> exit 1 (both layers fire); swapping RELEASE_NAMESPACE -> exit 1 with ONLY the new per-name check firing, the site comparison silent; renaming the ENVCLASS marker -> exit 2 with NOT CHECKED; stripping the envs from all three templates -> exit 1 naming the chart-level cause. helm unittest 656/656. make drift 39/39. shellcheck clean. Co-Authored-By: Claude Opus 4.8 --- client/templates/NOTES.txt | 2 +- client/values.yaml | 12 ++- .../tests/fullname-override-completeness.sh | 48 ++++++++++++ scripts/tests/fullname_override_assertions.py | 77 ++++++++++++++++++- 4 files changed, 132 insertions(+), 7 deletions(-) diff --git a/client/templates/NOTES.txt b/client/templates/NOTES.txt index 6ba54b1f..08098698 100644 --- a/client/templates/NOTES.txt +++ b/client/templates/NOTES.txt @@ -42,7 +42,7 @@ installer they just renamed something. */ -}} {{- if (default dict .Values.hostPath).enabled }} -{{ "\033[1;33m" }}hostPath PersistentVolumes:{{ "\033[0m" }} {{ "\033[0;90m" }}these PV OBJECTS are named through fullnameOverride and carry NO helm.sh/resource-policy: keep, unlike the PVCs. So CHANGING OR CLEARING fullnameOverride on a live release recreates them under the new name and leaves the retained PVCs Bound to the objects that went away -- after any such change, confirm the PVCs are still Bound.{{ "\033[0m" }} +{{ "\033[1;33m" }}hostPath PersistentVolumes:{{ "\033[0m" }} {{ "\033[0;90m" }}these PV OBJECTS are named through fullnameOverride and carry NO helm.sh/resource-policy: keep, unlike the PVCs. So CHANGING OR CLEARING fullnameOverride on a live release recreates them under the new name and leaves the retained PVCs Bound to the objects that went away. An upgrade that would do it is now REFUSED at template time, so you cannot reach it silently -- but after any such change, confirm the PVCs are still Bound.{{ "\033[0m" }} - {{ "\033[0;36m" }}{{ include "tracebloc.mysqlPvName" . }}{{ "\033[0m" }} - {{ "\033[0;36m" }}{{ include "tracebloc.clientDataPvName" . }}{{ "\033[0m" }} - {{ "\033[0;36m" }}{{ include "tracebloc.clientLogsPvName" . }}{{ "\033[0m" }} diff --git a/client/values.yaml b/client/values.yaml index d3ccfb00..245ffe01 100644 --- a/client/values.yaml +++ b/client/values.yaml @@ -544,8 +544,16 @@ egressProxy: # The refusal says so too, but only while it FIRES: once a Secret exists under # the new name it is skipped, and an operator on their second attempt would # otherwise read this remedy with the caveat missing. NOTES.txt repeats it on -# every hostPath render of an overridden release, which is the path the refusal -# cannot reach. +# every hostPath render, which is the path the refusal cannot reach. +# +# ON EVERY hostPath RENDER, not only an overridden one -- this sentence said +# "of an overridden release" and the gate never matched it (Bugbot, Low). The +# gate is `hostPath.enabled` alone, deliberately: the hazard is the name MOVING, +# and A->unset moves it just as much as unset->A. Gating the notice on the key +# being SET would go quiet on exactly the direction where the operator has no +# override left to look at. The cost is a standing notice on first installs that +# never rename, so the notice says outright that nothing is wrong yet rather than +# addressing a reader who just renamed. # # So: choose it at INSTALL time. On a live release, read the refusal message. # diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh index 277e71dc..b44d1950 100755 --- a/scripts/tests/fullname-override-completeness.sh +++ b/scripts/tests/fullname-override-completeness.sh @@ -100,8 +100,11 @@ failures=0 # Accumulators for the cross-profile release-scoped-path assertion after the loop. path_profiles=0 pathclass_missing=0 +env_profiles=0 +envclass_missing=0 could_not_run=0 path_counts="" +env_counts="" # NOTES NEEDS ITS OWN RENDER, AND EVERY OBVIOUS ROUTE IS CLOSED. Measured on the # CI-pinned helm v3.15.4: @@ -214,6 +217,12 @@ for VALUES in "${profiles[@]}"; do pc=$(grep -E '^PATHCLASS [0-9]+$' "$tmp/out.txt" | awk '{print $2}' | head -1 || true) path_counts="${path_counts}${prof}=${pc:-none} " case "${pc:-0}" in ''|0) ;; *) path_profiles=$((path_profiles + 1)) ;; esac + # The Helm-identity env count, on exactly the same terms as PATHCLASS above and + # for the same reason: a profile that renders no CronJob has no such env, so + # per-profile emptiness is legitimate and per-CHART emptiness is not. + ec=$(grep -E '^ENVCLASS [0-9]+$' "$tmp/out.txt" | awk '{print $2}' | head -1 || true) + env_counts="${env_counts}${prof}=${ec:-none} " + case "${ec:-0}" in ''|0) ;; *) env_profiles=$((env_profiles + 1)) ;; esac # NO COUNT MEANS NO ANSWER, NOT AN ANSWER OF ZERO (@saadqbal on client#911). # A profile that never PRINTED `PATHCLASS` leaves `pc` empty, `path_profiles` # unincremented, and assertion 6 below then reports "NO profile rendered a @@ -230,6 +239,7 @@ for VALUES in "${profiles[@]}"; do # which is a legitimate finding code. The property that actually licenses # assertion 6 is that the count was PRODUCED, so that is what is tracked. [ -n "${pc:-}" ] || pathclass_missing=$((pathclass_missing + 1)) + [ -n "${ec:-}" ] || envclass_missing=$((envclass_missing + 1)) # EXIT CODES THE SIDECAR OWNS: 0 clean, 1 a real finding, 2 could-not-run. # ANYTHING ELSE MEANS IT DID NOT RUN, and lumping those into `failures` was the # half of @saadqbal's interpreter finding that survived the fix above: a @@ -342,6 +352,44 @@ else echo "-- release-scoped paths present in $path_profiles profile(s) [OK]" fi +# --- 7. HELM-IDENTITY ENVS EXIST SOMEWHERE ----------------------------------- +# The chart-level half of the env class, on the same terms as assertion 6. +# +# WHY THE CLASS NEEDED A COMPLETE ENUMERATION AT ALL (Bugbot, Medium; confirmed +# by reproduction). The sidecar's STAYED check only ever saw sites that still +# CONTAIN the release name, because the walk skips the rest -- so a routed +# RELEASE_NAME simply vanished from the class instead of failing it, and the two +# remaining sites carried the assertion. Measured: routing RELEASE_NAME on the +# auto-upgrade CronJob to `tracebloc.fullname` -- backend#2620's exact bug, where +# auto-upgrade then `helm rollback`s a name that is no longer the release -- left +# this guard printing a pass and exiting 0. The sidecar now enumerates every env +# named in RELEASE_ENV from the render and checks each positively, so that same +# mutation exits 1 and names the site. +# +# This assertion is the chart-level backstop for it: "every identity env carries +# its identity" is also true of a chart that stopped setting them, and that is a +# consumer reading an unset name rather than a wrong one. +echo "-- Helm-identity envs per profile: ${env_counts% }" +if [ "$envclass_missing" -ne 0 ]; then + echo "-- Helm-identity env class: NOT CHECKED — $envclass_missing profile(s)" + echo " produced no ENVCLASS count, so this assertion has no input. Not a" + echo " verdict on the chart in either direction; fix the finding(s) above and" + echo " re-run." + # `could_not_run`, not `failures` — same vocabulary and the same reason as + # assertion 6: renaming the ENVCLASS marker in the sidecar is a pure refactor + # that would otherwise print NOT CHECKED and exit 0 on a required guard. + could_not_run=1 +elif [ "$env_profiles" -eq 0 ]; then + echo "[ERROR] NO profile rendered a RELEASE_NAME / RELEASE / RELEASE_NAMESPACE" + echo " env, so 'every identity env carries its identity' proves nothing —" + echo " it is equally true of a chart that stopped setting them. Either the" + echo " envs disappeared or every profile now disables the CronJobs and the" + echo " storage-assertions Job, and both are findings." + failures=$((failures + 1)) +else + echo "-- Helm-identity envs present in $env_profiles profile(s) [OK]" +fi + if [ "$failures" -ne 0 ]; then echo "[ERROR] fullnameOverride is incomplete in $failures profile check(s)" exit 1 diff --git a/scripts/tests/fullname_override_assertions.py b/scripts/tests/fullname_override_assertions.py index 2d9d8c23..0ce0f04b 100644 --- a/scripts/tests/fullname_override_assertions.py +++ b/scripts/tests/fullname_override_assertions.py @@ -742,13 +742,20 @@ def main() -> int: # regression backend#2620 describes. want_sites = identity_env_sites(default_docs, rel, ns, rel) have_sites = identity_env_sites(docs, rel, ns, ovr) + # PER-PROFILE EMPTINESS IS NOT A CHART FINDING, on the same terms as the PATH + # class above. A profile that renders neither CronJob nor the + # storage-assertions Job has no identity env, and failing here would refuse a + # complete chart -- the mistake the PATH class already made and had demoted + # after measuring. So the count goes out machine-readably and the shell + # asserts it once ACROSS profiles, which is the only layer that sees all four. + # Measured at head: aks 5, bm 5, eks 5, oc 5. if not want_sites: - fail = True print( - " [ERROR] the DEFAULT render names zero helm-identity env sites, so " - "there is no domain to compare against and this assertion proves " - "nothing. RELEASE_ENV or the env walk has stopped matching." + " [note] the DEFAULT render names no helm-identity env in this profile " + "— legitimate when neither CronJob nor the storage-assertions Job " + "renders. Asserted across profiles, not here." ) + print("ENVCLASS 0") else: missing = sorted(want_sites - have_sites) if missing: @@ -765,6 +772,68 @@ def main() -> int: f" [OK] all {len(want_sites)} helm-identity env site(s) still carry " f"the release identity, site by site" ) + print(f"ENVCLASS {len(want_sites)}") + + # AND EACH NAME MUST CARRY ITS OWN IDENTITY, which the set comparison + # above cannot see. `identity_env_sites` accepts a value in (rel, ns) for + # any of the three names, so swapping RELEASE_NAME with + # RELEASE_NAMESPACE keys the same triple on both sides and passes -- a + # consumer reading RELEASE_NAME would get the namespace. Checked here + # against the name's own meaning, which is assertable only because this + # profile deliberately makes rel and ns differ + # (`relnamelongenoughtocatchatruncation38` vs `tracebloc`); under the + # installer's own one-string-for-both convention (backend#2621) it would + # not be. + # + # A `valueFrom` env has no literal to read. That is not a pass: it is a + # site this text-level check cannot see, and saying so is the finding + # (rule 3). It also drops silently out of both sets above, so this is the + # only place it is reported at all. + env_expected = {"RELEASE_NAME": rel, "RELEASE": rel, "RELEASE_NAMESPACE": ns} + assert set(env_expected) == RELEASE_ENV, ( + "env_expected and RELEASE_ENV disagree: a name in one and not the " + "other is either an unchecked env or a check for an env that does " + "not exist" + ) + swapped, unreadable = [], [] + for d in docs: + where = f"{d.get('kind')}/{(d.get('metadata') or {}).get('name')}" + envs = env_value_paths(d) + if not envs: + continue + values = {path: val for path, val in walk(d) if isinstance(val, str)} + for vpath, ename in envs.items(): + if ename not in RELEASE_ENV: + continue + if vpath not in values: + unreadable.append((where, vpath, ename)) + elif values[vpath] != env_expected[ename]: + swapped.append((where, ename, values[vpath])) + if unreadable: + fail = True + print( + f" [ERROR] {len(unreadable)} helm-identity env(s) carry no literal " + f"value, so neither this check nor the site comparison above can " + f"read them:" + ) + for where, vpath, ename in sorted(unreadable): + print(f" {where} {ename} at {vpath}") + if swapped: + fail = True + print( + f" [ERROR] {len(swapped)} helm-identity env(s) carry an identity " + f"that is not the one they name:" + ) + for where, ename, val in sorted(swapped): + print( + f" {where} {ename} = {val!r} " + f"(expected {env_expected[ename]!r})" + ) + elif not unreadable: + print( + f" [OK] each helm-identity env carries the identity it names " + f"(RELEASE_NAME/RELEASE={rel!r}, RELEASE_NAMESPACE={ns!r})" + ) # --- NOTES -------------------------------------------------------------- # A SEPARATE RENDER, because `helm template` does not emit NOTES.txt at all From 5ebbefa35bd3b57349a8d1571fdb0af383a0e7d5 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 1 Sep 2026 09:30:55 +0200 Subject: [PATCH 19/22] test(chart): force egress-enforcement and registry-Secret into the completeness denominator (backend#2626) Co-Authored-By: Claude Opus 4.8 --- .../tests/fullname-override-completeness.sh | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh index b44d1950..0d800c9a 100755 --- a/scripts/tests/fullname-override-completeness.sh +++ b/scripts/tests/fullname-override-completeness.sh @@ -153,9 +153,27 @@ for VALUES in "${profiles[@]}"; do prof=$(basename "$VALUES" -values.yaml) echo "-- profile: $prof" + # THE DENOMINATOR, forced (Asad + Arturo, review of backend#2626). The four + # client/ci profiles at their defaults render neither egress-enforcement-check + # (needs networkPolicy.training.allowExternalHttps=false) nor the registry-Secret + # family (needs dockerRegistry.create=true), so ~15 routed sites -- one Job name, + # one Secret name, thirteen imagePullSecrets[0].name refs -- rendered in 0/4 and + # the walk never saw them: "fullnameOverride routes every resource name" was + # vacuously true for them, and reverting one to .Release.Name still exited 0. + # Forcing these three keys renders both families into every profile so they enter + # the walk. The baseline stays 0 -- the chart routes all 15 correctly today, so + # this adds a tripwire, not a fix. Set on ALL renders (a/b/explicit/override) so + # the no-op diff below still compares like with like. render() { helm template "$RELEASE" ./client --namespace "$NS" \ - --set clientId=x --set clientPassword=p -f "$VALUES" "$@" + --set clientId=x --set clientPassword=p \ + --set networkPolicy.training.enabled=true \ + --set networkPolicy.training.allowExternalHttps=false \ + --set dockerRegistry.create=true \ + --set dockerRegistry.server=https://registry.example.com \ + --set dockerRegistry.username=u --set dockerRegistry.password=p \ + --set dockerRegistry.email=ci@example.com \ + -f "$VALUES" "$@" } render > "$tmp/a.yaml" render > "$tmp/b.yaml" From 3cb76d30f0d6d15f93dea23154fc8c17b75f97d7 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 1 Sep 2026 09:33:36 +0200 Subject: [PATCH 20/22] chore(chart): bump to 1.9.92 for the completeness denominator change (backend#2626) Co-Authored-By: Claude Opus 4.8 --- client/Chart.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/Chart.yaml b/client/Chart.yaml index d79f4213..d28d7c83 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.91 -appVersion: "1.9.91" +version: 1.9.92 +appVersion: "1.9.92" keywords: - tracebloc - kubernetes From 0b6fee5a893f89b18cf8443c8d7db471bc9a1836 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 1 Sep 2026 10:18:30 +0200 Subject: [PATCH 21/22] fix(completeness): demote CLS_ENV emptiness, and pin the refusal detector (backend#2626) Both findings from @saadqbal review, plus the base merge. 1. CLS_ENV EMPTINESS WAS HALF-DEMOTED (Bugbot Medium; he reproduced it). The identity-env block already treated per-profile emptiness as legitimate and printed `ENVCLASS 0` -- but the STAYED empty-class arm set `fail = True` first, so that half could never rescue the run. Reproduced here on aks with autoUpgrade, imageRefresh and sealCheck.storageAssertions.enabled all off: 38 documents, and the ORIGINAL prints "[ERROR] found NO RELEASE_NAME ..." and "ENVCLASS 0" in ONE output. A required drift guard refusing a complete chart. AND THE ONE-LINE FIX WOULD HAVE BROKEN THE CROSS-PROFILE ASSERTION. Extending the demotion is right, but it must NOT also print `ENVCLASS 0` the way CLS_PATH prints `PATHCLASS 0`: the shell parses `grep -E ^ENVCLASS | head -1` and the identity-env block below prints the REAL count unconditionally, so a zero here would be read first and the true value discarded -- turning a cross-profile assertion into one that always sees 0. CLS_PATH can print its count because its two branches are mutually exclusive; CLS_ENV count is owned by the block below. Stated in the code. STILL LATENT, and I am not claiming a test covers it: all four CI profiles name 5 identity envs, so disabling the demotion does NOT redden the suite. The evidence is the live reproduction above, not a guard. Pinning it needs either a fifth profile or a specimen driving assert_stayed empty arm -- happy to add the latter. 2. THE REFUSAL DETECTOR HAD NO SPECIMENS AT ALL, which is the asymmetry that made his mutation possible: `_fallback_on_line` has three pinning lines, `_mitigations` had none. `return {"refusal"}` unconditionally left assertion 5 printing "all 2 routed Secret lookup(s) of 2 carry a mitigation" and exiting 0, both existing selftests green -- the detector deciding whether a routed credential Secret is mitigated could be DELETED and nothing in the tree would say so. Four specimens now, written down rather than hunted for in the chart: a real refusal, a negation that merely branches, a `fail` reached by negating an UNRELATED variable (the routed_vars discrimination), and a `fail` below the 4-line window. Mutations, all reddening: his exact `return {"refusal"}` (3 of 4 halves fire); widening the window 4 -> 40; dropping the per-var discrimination. A fourth came back INERT and its anchor assertion said so rather than reporting a pass. 3. BASE MERGED, both conflicts as he specified. Makefile: develop block plus one line, verified by comparing guard sets programmatically -- 39 on develop, 40 here, the single delta being fullname-override-completeness.sh -- rather than by eye, because `--ours` would delete #933 empty-entry refusal and `--theirs` would drop this guard from the list entirely. manifest.sha256 REGENERATED from the merged tree. Guard at head: rc=0, 60 OK, 0 ERROR, aks=5 bm=5 eks=5 oc=5 -- the 56/0 baseline he measured, plus the four new selftest lines. drift-list-integrity 9/9, gen-manifest --check clean. black is NOT run by this repo CI and the file was already non-black-clean (14 hunks before and after, none in the added code), so it is left alone rather than reformatted into an unrelated diff. Co-Authored-By: Claude Opus 4.8 --- scripts/manifest.sha256 | 8 +- scripts/tests/fullname_override_assertions.py | 114 ++++++++++++++++++ 2 files changed, 118 insertions(+), 4 deletions(-) diff --git a/scripts/manifest.sha256 b/scripts/manifest.sha256 index 1d6f9428..db6a0540 100644 --- a/scripts/manifest.sha256 +++ b/scripts/manifest.sha256 @@ -1,5 +1,5 @@ 753502a4e76694bdc333eb2a0c898e4802cf6cce30f8968e95c95491472c3db8 scripts/install-k8s.sh -c260a2a4ce634de912a11a3a40484d825da449201a31a1a362473294ff95860e scripts/lib/common.sh +58c0b8a3409975cc97e391bc2854cb2fd340849dd4570f926821840e95015f12 scripts/lib/common.sh d773c8a11fa80d6880b5a104e2159de1dc9aaa35234ad6a764e4e1c49f601aec scripts/lib/telemetry.sh 3d15a215c460935a8ab4ef2a0991e378e2ff57c2c9d1a4550941d5cdb902e19f scripts/lib/preflight.sh a61f5bac3786a3283b5fa08fea9e522f7cea5f7fa44799a2b38b5caea42469d8 scripts/lib/detect-gpu.sh @@ -9,12 +9,12 @@ b569eec2d8ffb9673da287a2a59d249a7dbc7236c98ab6a5062136bcc69a942c scripts/lib/gp 58cfe1ff05074c58ff6698984c2b906b5f2540b951d0a95d1d959d18538ceef6 scripts/lib/setup-linux.sh 54c516327df6d7684a7546722092bc77a4c8db48b356cb1cb6e87923f3c4ccca scripts/lib/cluster.sh 84ed9d9b3ab4633bfaf07b256c066ed43f96a0025ec6b1a34db23fdef75f0f62 scripts/lib/gpu-plugins.sh -bbe5db7b00ecd05b054f9cf6b9eca8098e4b5c0a86441186e0ca5be6eba23107 scripts/lib/install-client-helm.sh +8731571cdd84e8906b88eab5f8a28b0cc164680bba9b8f2d3ac79931c21e1a8b scripts/lib/install-client-helm.sh 1b3e11d06e4be983ec5cecd8f55b16034b76bdb0476f3e141c025d383ddc043a scripts/lib/install-cli.sh ea2bbd9948ee9e31e93271e235c630ced50d746e51a5629b5622041d8a39df07 scripts/lib/provision.sh fc6426e454f5c113748c32ce025cc1dce52d465cc4ca97850cd25ad3cf045c24 scripts/lib/assess.sh 6b3164415dcd793880b972fb61897179d58097f7d288e3904f6f09f858417a15 scripts/lib/probe.sh -e36c0fc3568aa5054ee6c853ced0791ad45951724fff059cff63847f5bc870e2 scripts/lib/summary.sh +3d66cce74afd7894bba313a7468d0e6c5e98a1f1aead4d2569791274d70dea9d scripts/lib/summary.sh 1b5e0fb3c9b100672dc3f547b677524f98f0a9ed28d8dc7de0f619d2687cfc52 scripts/lib/diagnose.sh -2a5609686b0571dce8cea4dec1145e19a348ac055e07eb3dc3a45a9e474dd95d scripts/install-k8s.ps1 +432799f7ce8eaba90824fdf95265417c3bc6710e3da6f8863ebb75beb9f7b976 scripts/install-k8s.ps1 fc6eeeb4e4114ce74566992519a90320c52434d604218a30ba8ef2c3fa266ca5 scripts/lib/telemetry.ps1 diff --git a/scripts/tests/fullname_override_assertions.py b/scripts/tests/fullname_override_assertions.py index 0ce0f04b..bf9eb63b 100644 --- a/scripts/tests/fullname_override_assertions.py +++ b/scripts/tests/fullname_override_assertions.py @@ -416,6 +416,87 @@ def selftest_the_fallback_detector(): return ok, msgs +#: FOUR SPECIMENS THAT PIN `_mitigations` (@saadqbal, review of client#911). +#: `_fallback_on_line` had three; its sibling had NONE, and that asymmetry is the +#: whole finding: make `_mitigations` return `{"refusal"}` unconditionally and +#: assertion 5 still prints "all 2 routed Secret lookup(s) of 2 carry a mitigation" +#: and exits 0, with both existing selftests green. The detector that decides +#: whether a routed credential Secret is mitigated could be deleted and nothing in +#: the tree would say so. +#: +#: Written down here rather than hunted for in the chart, for the reason the +#: fallback specimens give: a check exercisable only once the bug is present +#: arrives too late. +_SELFTEST_REFUSAL = ( + '{{- $existingSecret := (lookup "v1" "Secret" .Release.Namespace $secretName) -}}\n' + '{{- if and $mysqlDataPresent (not $existingSecret) -}}\n' + '{{- fail "refusing to reinstall over a kept PVC" -}}\n' + '{{- end -}}\n' +) +#: The same negation, but NO `fail` follows it -- a condition that merely branches. +_SELFTEST_NO_REFUSAL = ( + '{{- $existingSecret := (lookup "v1" "Secret" .Release.Namespace $secretName) -}}\n' + '{{- if and $mysqlDataPresent (not $existingSecret) -}}\n' + '{{- $bootstrap = true -}}\n' + '{{- end -}}\n' +) +#: A `fail` reached by negating some OTHER variable. This is the discrimination +#: `routed_vars` exists for: a refusal elsewhere in the file must not license the +#: routed lookup. +_SELFTEST_OTHER_VAR_REFUSAL = ( + '{{- if (not $somethingElse) -}}\n' + '{{- fail "unrelated refusal" -}}\n' + '{{- end -}}\n' +) +#: And a `fail` too far below the negation to be its consequence -- the window the +#: detector deliberately keeps small so an unrelated `fail` cannot be adopted. +_SELFTEST_DISTANT_FAIL = ( + '{{- if (not $existingSecret) -}}\n' + '{{- $a = 1 -}}\n{{- $b = 2 -}}\n{{- $c = 3 -}}\n{{- $d = 4 -}}\n' + '{{- end -}}\n' + '{{- fail "something unrelated, much later" -}}\n' +) + + +def selftest_the_refusal_detector(): + """`(ok, messages)` — `_mitigations` answers about THIS lookup, not the file. + + The first half is the one an unconditional `return {"refusal"}` fails; the + rest stop it drifting to "any `fail` anywhere licenses anything". + """ + msgs, ok = [], True + if "refusal" not in _mitigations(_SELFTEST_REFUSAL, {"existingSecret"}): + ok = False + msgs.append( + " [ERROR] a MISS on the routed lookup that reaches `fail` is not " + "recognised as a refusal, so every mitigated site reports as " + "unmitigated and assertion 5 fails on a safe chart — noise that gets " + "it muted.") + if _mitigations(_SELFTEST_NO_REFUSAL, {"existingSecret"}): + ok = False + msgs.append( + " [ERROR] a negation that merely BRANCHES reads as a refusal. This is " + "the half an unconditional `return {\"refusal\"}` fails: without it the " + "detector can be deleted and assertion 5 still reports every routed " + "credential Secret as mitigated.") + if _mitigations(_SELFTEST_OTHER_VAR_REFUSAL, {"existingSecret"}): + ok = False + msgs.append( + " [ERROR] a `fail` reached by negating an UNRELATED variable licenses " + "the routed lookup, so one refusal anywhere in a file mitigates every " + "routed Secret in it — which is the file-wide mistake the fallback half " + "was already demoted for.") + if _mitigations(_SELFTEST_DISTANT_FAIL, {"existingSecret"}): + ok = False + msgs.append( + " [ERROR] a `fail` well below the negation is adopted as its " + "consequence, so the window is not bounded and any later refusal in the " + "file counts.") + if ok: + msgs.append(" [OK] the refusal detector answers about the routed lookup, not the file") + return ok, msgs + + def selftest_the_parser(): """`(ok, messages)` — the define parser reads a body past an inner `end`. @@ -682,6 +763,33 @@ def main() -> int: ) print("PATHCLASS 0") continue + if cls is CLS_ENV: + # SAME DEMOTION AS CLS_PATH, and it was already half-made + # (Bugbot Medium; reproduced by @saadqbal). The identity-env block + # below ALREADY treats per-profile emptiness as legitimate and + # prints `ENVCLASS 0` — but this arm set `fail = True` first, so + # that half could never rescue the run. On aks with autoUpgrade, + # imageRefresh and sealCheck.storageAssertions all off: 41 + # documents, 0 identity envs, sidecar exit 1, and BOTH messages in + # one output — an [ERROR] saying the class proves nothing, then a + # note saying the emptiness is fine. A required drift guard + # refusing a complete chart. + # + # NO `ENVCLASS 0` HERE, deliberately, and this is the part the + # one-line "extend the demotion" fix would get wrong. The shell + # parses `grep -E '^ENVCLASS [0-9]+$' | head -1`, and the + # identity-env block below prints the REAL count unconditionally. + # Printing one here too would make `head -1` read this zero and + # discard the true value — turning a cross-profile assertion into + # one that always sees 0. CLS_PATH can print its count because its + # two branches are mutually exclusive within this loop; CLS_ENV's + # count is owned by the block below. + print( + f" [note] no {cls} in this profile — legitimate when neither " + f"the CronJobs nor the storage-assertions Job renders. Counted " + f"and asserted across profiles below, not here." + ) + continue print( f" [ERROR] found NO {cls} carrying the release name — the class matches " f"nothing, so its half of this assertion proves nothing." @@ -903,6 +1011,12 @@ def main() -> int: if not ok: fail = True + ok, msgs = selftest_the_refusal_detector() + for m in msgs: + print(m) + if not ok: + fail = True + ok, msgs = assert_lookup_keys() for m in msgs: print(m) From 7db261863c103f254ca81ddee7b7615e6d1a5526 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 1 Sep 2026 10:53:29 +0200 Subject: [PATCH 22/22] docs(completeness): the preflight does NOT make 127 unreachable (backend#2626) Co-Authored-By: Claude Opus 5 --- scripts/tests/fullname-override-completeness.sh | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh index f63809ad..397bdc27 100755 --- a/scripts/tests/fullname-override-completeness.sh +++ b/scripts/tests/fullname-override-completeness.sh @@ -315,9 +315,17 @@ done # `assertRaises(Exception)` shape, in shell. # # 1 = the assertions ran and refused. 2 = could not run (no PyYAML). 127 = no -# interpreter. Only the first is the thing being asserted, and the preflight -# above now makes 127 unreachable anyway -- belt and braces, because this -# assertion must not be satisfiable by the absence of the thing it invokes. +# interpreter. Only the first is the thing being asserted. +# +# AND 127 IS STILL REACHABLE -- the earlier version of this comment said the +# preflight "makes 127 unreachable anyway", which contradicts the `case "$rc"` +# block 34 lines up that explains at length why it cannot (Asad, review of +# backend#2626). `command -v python3` answers "is there something on PATH by +# that name", not "does it run": a shim that execs a deleted interpreter passes +# the preflight and exits 127 here. So requiring exactly 1 is not belt-and-braces +# over an impossible code -- it is the whole point, because the one exit status +# this assertion must never accept is the one produced by the absence of the +# thing it invokes. set +e RELEASE="$RELEASE" NS="$NS" OVERRIDE="$OVERRIDE" \ python3 scripts/tests/fullname_override_assertions.py \