diff --git a/Makefile b/Makefile index 09eff025..d61a8cb7 100644 --- a/Makefile +++ b/Makefile @@ -390,7 +390,8 @@ DRIFT_GUARDS := scripts/gen-manifest.sh --check|\ bash scripts/tests/auto-upgrade-inflight-vs-wedge.sh|\ bash scripts/tests/hostpath-reads-guarded.sh|\ bash scripts/tests/jobs-manager-waits-for-mysql.sh|\ - bash scripts/tests/control-plane-footprint.sh + bash scripts/tests/control-plane-footprint.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 d28d7c83..ce7e8ef4 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.92 -appVersion: "1.9.92" +version: 1.9.93 +appVersion: "1.9.93" keywords: - tracebloc - kubernetes diff --git a/client/templates/NOTES.txt b/client/templates/NOTES.txt index e252b1e7..4b2472f8 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 }} @@ -34,6 +34,21 @@ - {{ "\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" }} +{{- /* GATED ON hostPath ALONE, and the WORDING carries the condition rather + than the gate (Bugbot, Low). Gating on `fullnameOverride` being SET would + go quiet on the A->unset rename, which is the same hazard -- so the block + states a STANDING PROPERTY of every hostPath release ("these PVs are named + through the override and are not kept") instead of telling a first-time + 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. 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" }} + {{ "\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/_helpers.tpl b/client/templates/_helpers.tpl index 116bc6a7..8fb734ec 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 -}} @@ -1073,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 @@ -1147,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). 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 -}} 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 fe19697f..613cb036 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 }} @@ -908,5 +908,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/secrets.yaml b/client/templates/secrets.yaml index 726e24e9..474fc71c 100644 --- a/client/templates/secrets.yaml +++ b/client/templates/secrets.yaml @@ -6,6 +6,141 @@ */ -}} {{- $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. + + 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: + + 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 + + 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. +*/ -}} +{{- $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 := "" -}} +{{- /* 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/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/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 1097fc45..97e4d000 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: diff --git a/client/values.schema.json b/client/values.schema.json index b6f6a62e..64174625 100644 --- a/client/values.schema.json +++ b/client/values.schema.json @@ -1462,6 +1462,12 @@ "type": "boolean" } }, + "fullnameOverride": { + "type": "string", + "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). 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": [ "object", diff --git a/client/values.yaml b/client/values.yaml index 97606a20..245ffe01 100644 --- a/client/values.yaml +++ b/client/values.yaml @@ -493,6 +493,77 @@ 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). +# +# 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 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 +# 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. +# +# 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, 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. +# +# 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). # An OpenTelemetry Collector DaemonSet that reads Class A container stdout with a # `filelog` receiver and forwards it to the tracebloc backend's ingest endpoint. @@ -1095,7 +1166,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 a8a90a57..b7bc44be 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/install-k8s.ps1 b/scripts/install-k8s.ps1 index 563c0dfc..a3455341 100644 --- a/scripts/install-k8s.ps1 +++ b/scripts/install-k8s.ps1 @@ -1466,8 +1466,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") @@ -5698,9 +5706,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/common.sh b/scripts/lib/common.sh index 5017cc18..cd6e43bc 100644 --- a/scripts/lib/common.sh +++ b/scripts/lib/common.sh @@ -571,11 +571,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/install-client-helm.sh b/scripts/lib/install-client-helm.sh index ce243680..5208183d 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/lib/summary.sh b/scripts/lib/summary.sh index 000a470c..f708a660 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/manifest.sha256 b/scripts/manifest.sha256 index 10ea2c7d..ee59c9fd 100644 --- a/scripts/manifest.sha256 +++ b/scripts/manifest.sha256 @@ -1,5 +1,5 @@ 753502a4e76694bdc333eb2a0c898e4802cf6cce30f8968e95c95491472c3db8 scripts/install-k8s.sh -dc61f9ce45af0f03432bfa3bc5470a3384cc8988f2ddc0537da710171795e71e scripts/lib/common.sh +98becf64c1c7b0ad34c200daeab78870ff7c92cad4b9513fce06f8ec40c70b7f 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 -f0ff63af6ae2fffe95cc4eee2535c4c2ab8a2cf414d8eb1060e804621e49cd19 scripts/lib/install-client-helm.sh +e415aa06fcdae8ca4e53dda525d0dc996f60c6291a7f2e58ad333436d2cc79e2 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 -360c01c7e6a13df6da13fdb5f217c6c2bb6d66c5ce677e02b7a2ef6bf543efe9 scripts/lib/summary.sh +354da97b583c6bc34156bb66d742e5ce0d84bc63a54daea4edcbe026c298ddbd 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/client-credentials-have-a-secret-tier.sh b/scripts/tests/client-credentials-have-a-secret-tier.sh index 67eed09f..bda277ec 100755 --- a/scripts/tests/client-credentials-have-a-secret-tier.sh +++ b/scripts/tests/client-credentials-have-a-secret-tier.sh @@ -157,12 +157,75 @@ 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. +# +# 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 '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 + +# 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 + +# 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 + 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 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 diff --git a/scripts/tests/fullname-override-completeness.sh b/scripts/tests/fullname-override-completeness.sh new file mode 100755 index 00000000..397bdc27 --- /dev/null +++ b/scripts/tests/fullname-override-completeness.sh @@ -0,0 +1,471 @@ +#!/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. +# 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 +# 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 +# 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")/../.." + +# 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 +# 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 +# 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 +pathsite_profiles=0 +pathsites_missing=0 +path_counts="" +env_counts="" +pathsite_counts="" + +# 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" + + # 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 \ + --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" + 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 + # 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 -------------------------------------------------------------- + # 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" | 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 + # 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 + # The per-SITE path enumeration, on exactly the same terms as the two counts + # above. Distinct from PATHCLASS: that one counts what still carries the release + # name, this one counts the sites the DEFAULT render named -- which is the domain + # a routed path disappears from. + ps=$(grep -E '^PATHSITES [0-9]+$' "$tmp/out.txt" | awk '{print $2}' | head -1 || true) + pathsite_counts="${pathsite_counts}${prof}=${ps:-none} " + case "${ps:-0}" in ''|0) ;; *) pathsite_profiles=$((pathsite_profiles + 1)) ;; esac + [ -n "${ps:-}" ] || pathsites_missing=$((pathsites_missing + 1)) + # 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)) + [ -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 + # `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 ----------------------------- +# 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 +# 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 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 \ + "$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 "[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 + +# --- 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 [ "$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.)" + # 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" + 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 + +# --- 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 + +# --- 8. THE PATH SITE ENUMERATION RAN SOMEWHERE ------------------------------- +# The chart-level backstop for the per-site path check, and the exact analogue of +# assertion 7 for the class assertion 6 covers. `want_paths` is derived from the +# DEFAULT render, so a chart that stopped scoping paths by release at all names +# zero sites on both sides and the set difference is empty -- "no path was routed +# away" reported over a domain that is itself gone. +echo "-- release-scoped path SITES per profile: ${pathsite_counts% }" +if [ "$pathsites_missing" -ne 0 ]; then + echo "-- release-scoped path sites: NOT CHECKED — $pathsites_missing profile(s)" + echo " produced no PATHSITES 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 + # assertions 6 and 7: renaming the PATHSITES marker in the sidecar is a pure + # refactor that would otherwise print NOT CHECKED and exit 0. + could_not_run=1 +elif [ "$pathsite_profiles" -eq 0 ]; then + echo "[ERROR] NO profile's DEFAULT render named a release-scoped path site, so" + echo " 'no path site was routed away' proves nothing — the set it compares" + echo " is empty on both sides. Either the paths stopped being scoped by" + echo " release or every profile disables both hostPath and the Collector," + echo " and both are findings." + failures=$((failures + 1)) +else + echo "-- release-scoped path sites present in $pathsite_profiles profile(s) [OK]" +fi + +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 new file mode 100644 index 00000000..b5f8c6b7 --- /dev/null +++ b/scripts/tests/fullname_override_assertions.py @@ -0,0 +1,1130 @@ +"""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 the rendered manifests — produced with `fullnameOverride` set to something +distinctive — and answers two opposite questions about the SAME set of scalars: + + 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 + +import os +import re +import sys + +# 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 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" + " 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=""): + """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 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 + + +# -------------------------------------------------------------------------- +# 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" +_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): + """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: + bodies.update(define_bodies(text)) + 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. + 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 + 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") + 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 +#: 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 -}} +""" + + +#: 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 + + +#: 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`. + + 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( + 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 + # 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 " + 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 identity_env_sites(docs, rel, ns, name_token): + """`{(kind, normalised-name, ENV_NAME)}` for every helm-identity env value. + + THE CLASS BEING NON-EMPTY IS NOT THE PROPERTY (@saadqbal / Bugbot, Medium on + client#911). STAYED asked only that the `RELEASE_NAME`/`RELEASE`/ + `RELEASE_NAMESPACE` class still hold values equal to the release identity — + and a value routed through `fullnameOverride` no longer CARRIES the release + name, so the token scan never classifies it and it simply LEAVES the set. + `RELEASE_NAME` is set on both the auto-upgrade and image-refresh CronJobs and + `RELEASE` on the storage-assertions Job, so routing any one of them left the + class populated by its siblings and the guard green — while auto-upgrade + would `helm rollback` a name that is no longer the release (backend#2620), + which is the exact failure this guard exists to prevent. + + So the domain is DERIVED from the render that cannot be wrong — the one with + the override UNSET — and every site it names must still be there with the + override set. Disappearing is the finding; the old check could not see it, + because it only ever looked at what remained. + + The workload NAME moves under the override, so it cannot key the site as-is: + the release/override token is normalised out, leaving `-jobs-manager` + on both sides. + """ + out = set() + token = re.compile( + rf"(^|[^A-Za-z0-9]){re.escape(name_token)}($|[^A-Za-z0-9])" + ) + for d in docs: + kind = d.get("kind") + raw = (d.get("metadata") or {}).get("name") or "" + norm = token.sub(r"\1\2", raw) + envs = env_value_paths(d) + for path, val in walk(d): + if not isinstance(val, str) or path not in envs: + continue + # RELEASE_ENV is the declaration; this reads it rather than repeating + # the three names, so adding a fourth is covered without an edit here. + if envs[path] in RELEASE_ENV and val in (rel, ns): + out.add((kind, norm, envs[path])) + return out + + +def release_scoped_path_sites(docs, rel, ns, name_token): + """`{(kind, normalised-name, doc-path)}` for every path scoped by the RELEASE. + + THE SAME BLIND SPOT AS `identity_env_sites`, ONE CLASS OVER (Bugbot, High on + client#911). The PATH class asks that every path still CONTAINING the release + name did not follow the override — but a path routed to `tracebloc.fullname` + stops containing the release name, so the token scan never classifies it and + it LEAVES the class rather than failing it. The class-level check then reports + `[OK] N release-scoped path(s) kept the release name` over the sites that + remain, and the cross-profile `PATHCLASS` assertion only ever sees a count, so + a single routed path keeps every profile's count plausible. + + That single routed path is the one that orphans tenant data: the datadir is + keyed by release name today, and a path that follows the override points a + fresh install at a directory the old one does not own. + + So the domain is DERIVED from the render that cannot be wrong -- the one with + the override UNSET -- and every path site it names must still be scoped by the + release name with the override set. Disappearing is the finding. + + Keyed on the doc path rather than the value, because the value legitimately + differs between the two renders (the workload name is normalised out of it the + same way `identity_env_sites` normalises `metadata.name`). + + THE PREDICATE IS "STILL SCOPED BY `rel`", AND IT HAS TO BE MEASURED ON THE + VALUE. `classify` alone is not enough and the first version of this function + used it alone: `CLS_PATH` keys on the doc-path SHAPE (`.hostPath.path`, or a + `.path` whose value starts with `/`) and never looks at whether the release + name is in there -- main()'s loop applies that filter upstream, before it + classifies. So classifying without it named every hostPath in both renders, + the two sets came out identical, and the set difference was empty by + construction. Measured: routing `logs-pvc.yaml`'s path through + `tracebloc.fullname` left this guard printing `[OK] all 5` and exiting 0 -- + the very mutation it was written to catch. + + `rel` is used for BOTH renders on purpose; `name_token` is not the predicate. + Under the override the value should still carry the release name, so testing + for `ovr` would pass exactly when the path had been routed. + """ + out = set() + token = re.compile( + rf"(^|[^A-Za-z0-9]){re.escape(name_token)}($|[^A-Za-z0-9])" + ) + scoped = re.compile(rf"(^|[^A-Za-z0-9]){re.escape(rel)}($|[^A-Za-z0-9])") + for d in docs: + kind = d.get("kind") + raw = (d.get("metadata") or {}).get("name") or "" + norm = token.sub(r"\1\2", raw) + envs = env_value_paths(d) + for path, val in walk(d): + if not isinstance(val, str) or not val.startswith("/"): + continue + # The same classifier the class arm uses, so the two cannot disagree + # about what counts as an on-disk path -- plus the release-scoping test + # that arm gets from main()'s token filter. + if not scoped.search(val): + continue + if classify(d, path, val, rel, ns, envs) is CLS_PATH: + out.add((kind, norm, path)) + return out + + +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 + + 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. + 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") + + # --- 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 + scanned += 1 + if not tok.search(val): + continue + 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 + + # --- 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 — 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 + 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." + ) + fail = True + continue + 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") + # 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: + 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)} {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)} {cls} value(s) still carry the release identity") + + # --- EVERY RELEASE-SCOPED PATH SITE SURVIVED, not just the class ---------- + # See release_scoped_path_sites: a routed path vanishes from the class instead + # of failing it, so the PATHCLASS arm above is blind to the one regression that + # orphans tenant data. Same derivation as the env sites below, same reason. + want_paths = release_scoped_path_sites(default_docs, rel, ns, rel) + have_paths = release_scoped_path_sites(docs, rel, ns, ovr) + if not want_paths: + # PER-PROFILE EMPTINESS IS NOT A CHART FINDING -- the identical demotion + # the PATHCLASS arm already carries, and for the identical reason: a + # profile rendering no hostPath-backed workload has no release-scoped + # path, and failing here would refuse a complete chart. The count goes + # out machine-readably and the shell asserts it across profiles. + print( + " [note] the DEFAULT render names no release-scoped path in this " + "profile — legitimate when no hostPath-backed workload renders. " + "Asserted across profiles, not here." + ) + print("PATHSITES 0") + else: + gone = sorted(want_paths - have_paths) + if gone: + fail = True + print( + f" [ERROR] {len(gone)} release-scoped path site(s) present with the " + f"override UNSET are no longer scoped by the release name under " + f"fullnameOverride={ovr!r} — routed away, not merely changed. A path " + f"that follows the override points a fresh install at a datadir the " + f"old one does not own:" + ) + for kind, name, path in gone: + print(f" {kind}/{name} {path}") + else: + print( + f" [OK] all {len(want_paths)} release-scoped path site(s) are still " + f"scoped by the release name, site by site" + ) + print(f"PATHSITES {len(want_paths)}") + + # --- EVERY IDENTITY ENV SITE SURVIVED, not just the class ---------------- + # See identity_env_sites: a routed site vanishes from the class instead of + # failing it, so the class-level check above is blind to exactly the + # 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: + print( + " [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: + fail = True + print( + f" [ERROR] {len(missing)} helm-identity env site(s) present with the " + f"override UNSET no longer carry the release identity under " + f"fullnameOverride={ovr!r} — routed away, not merely changed:" + ) + for kind, name, env in missing: + print(f" {kind}/{name} {env}") + else: + print( + 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 + # (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] the default NOTES never mentions the release name, so the " + "override check below proves nothing. Did NOTES stop naming resources?" + ) + fail = True + # 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" [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 + + # --- 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 = selftest_the_fallback_detector() + for m in msgs: + print(m) + 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) + if not ok: + fail = True + + return 1 if fail else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/tests/install-client-helm.bats b/scripts/tests/install-client-helm.bats index 2ec93f35..ab3d3cad 100644 --- a/scripts/tests/install-client-helm.bats +++ b/scripts/tests/install-client-helm.bats @@ -1052,6 +1052,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 d1c7c0f0..f840b2de 100644 --- a/scripts/tests/install-k8s.Tests.ps1 +++ b/scripts/tests/install-k8s.Tests.ps1 @@ -4937,6 +4937,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)" { 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 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)