From ab2e67f19509c1d48998c2ba051d9a50e730bb9d Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Fri, 11 Sep 2026 09:38:16 +0200 Subject: [PATCH 1/3] [settings-naming] rename resourceMonitor -> resourceMonitor.enabled (D2) (#1016) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [settings-naming] rename resourceMonitor -> resourceMonitor.enabled (D2) RFC-0076 S14 (client#1009). `resourceMonitor: ` becomes `resourceMonitor.enabled: ` (D2: component booleans are `.enabled`). This is a bool->object change, so every stored values.yaml and every `--set resourceMonitor=true` still arrives as a SCALAR. A template that read `.Values.resourceMonitor.enabled` blindly would `fail` ("can't evaluate field enabled in interface {}") on the scalar, and the old `ne .Values.resourceMonitor false` gate crashes ("incompatible types for comparison: map and bool") on the new object. So all reads now route through a single new helper, tracebloc.resourceMonitorEnabled, which resolves the effective flag from whichever shape is present (kindIs), preferring the new `.enabled` form and defaulting absent/`{}` to enabled to match the historical default. Effective behaviour is unchanged: resourceMonitor.enabled=true does exactly what resourceMonitor=true did. - values.yaml default is now the object form; legacy scalar still honoured through the alias window (remove_by: 2026-12-31). - values.schema.json accepts both a boolean and an object; the object CLOSES its keys (additionalProperties: false) so a mistyped `enabled` is refused at chart load instead of silently staying enabled. - migrated the eight readers (daemonset, rbac, scc, secrets, rbac.yaml, jobs-manager NODE_AGENTS_NAMESPACE, NOTES.txt, and the two _helpers predicates resourceMonitorRefreshPinned / nodeAgentsInUse). - Chart.yaml version+appVersion bumped 1.9.107 -> 1.9.108 (chart-version-guard). - helm-unittest: added legacy-scalar, new-object, unset, null, `{}`, both-disabled, and mistyped-key cases. Closes tracebloc/client#1009 Part of tracebloc/backend#3391 Co-Authored-By: Claude Opus 4.8 * chore(chart): bump to 1.9.109 (one patch above develop, chart-version-guard) — client#1009 Develop advanced to 1.9.108 after this branch bumped there too; re-bump so the version stays above develop. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- client/Chart.yaml | 4 +- client/templates/NOTES.txt | 2 +- client/templates/_helpers.tpl | 45 ++++++++- client/templates/jobs-manager-deployment.yaml | 2 +- client/templates/rbac.yaml | 2 +- .../templates/resource-monitor-daemonset.yaml | 4 +- client/templates/resource-monitor-rbac.yaml | 4 +- client/templates/resource-monitor-scc.yaml | 2 +- client/templates/secrets.yaml | 2 +- client/tests/resource_monitor_test.yaml | 96 +++++++++++++++++++ client/values.schema.json | 16 +++- client/values.yaml | 10 +- 12 files changed, 169 insertions(+), 20 deletions(-) diff --git a/client/Chart.yaml b/client/Chart.yaml index f8b0cd9b..d8b3ed31 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.115 -appVersion: "1.9.115" +version: 1.9.116 +appVersion: "1.9.116" keywords: - tracebloc - kubernetes diff --git a/client/templates/NOTES.txt b/client/templates/NOTES.txt index 338b8da3..87543597 100644 --- a/client/templates/NOTES.txt +++ b/client/templates/NOTES.txt @@ -5,7 +5,7 @@ {{ "\033[1;35m" }}Components:{{ "\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 }} +{{- if (include "tracebloc.resourceMonitorEnabled" .) }} - {{ "\033[1;34m" }}Resource Monitor (DS):{{ "\033[0m" }} {{ "\033[0;33m" }}{{ include "tracebloc.resourceMonitorName" . }}{{ "\033[0m" }} {{- end }} diff --git a/client/templates/_helpers.tpl b/client/templates/_helpers.tpl index 7928f9af..0f7360b3 100644 --- a/client/templates/_helpers.tpl +++ b/client/templates/_helpers.tpl @@ -126,6 +126,42 @@ tracebloc.io/seal-check-name: {{ .name | quote }} {{ include "tracebloc.fullname" . }}-resource-monitor {{- end }} +{{/* + tracebloc.resourceMonitorEnabled — the SINGLE reader of "is the resource-monitor + on", coalescing the two value shapes during the RFC-0076 alias window + (remove_by: 2026-12-31, client#1009): + + legacy scalar resourceMonitor: + new object resourceMonitor.enabled: (D2: .enabled) + + This is a bool→object rename, so a stored values.yaml or a bare + `--set resourceMonitor=true` still arrives as a SCALAR. Reading + `.Values.resourceMonitor.enabled` blindly would `fail` with "can't evaluate + field enabled in interface {}" on the scalar and, on a `--reuse-values` + upgrade that carries the scalar forward, silently drop the setting. So decide + the shape with kindIs and prefer the new `.enabled` form: + + map -> .enabled, defaulting to true when the key is absent + bool -> the scalar itself + absent -> enabled (the historical default: `ne false` was true) + + Effective behaviour is unchanged: resourceMonitor.enabled=true does exactly + what resourceMonitor=true did. Emits "true" or nothing, so callers use + `(include "tracebloc.resourceMonitorEnabled" .)` in an `and`/`or` and + `not (include ...)` for the disabled case — the same idiom as + tracebloc.nodeAgentsInUse. +*/}} +{{- define "tracebloc.resourceMonitorEnabled" -}} +{{- $rm := .Values.resourceMonitor -}} +{{- if kindIs "map" $rm -}} +{{- if ne (dig "enabled" true $rm) false -}}true{{- end -}} +{{- else if kindIs "invalid" $rm -}} +{{- "true" -}} +{{- else -}} +{{- if ne $rm false -}}true{{- end -}} +{{- end -}} +{{- end }} + {{- define "tracebloc.rbacName" -}} {{ include "tracebloc.fullname" . }}-jobs-manager-rbac {{- end }} @@ -384,11 +420,12 @@ nvidia-device-plugin-daemonset * `resourceMonitor: false` — there is no DaemonSet at all, so there is nothing to reconcile and a cross-namespace `set image` would just fail. - Nil-safe: `.Values.resourceMonitor` absent reads as enabled, matching the - `ne .Values.resourceMonitor false` gate on the DaemonSet itself. + Nil-safe via tracebloc.resourceMonitorEnabled, which absent reads as enabled, + matching the gate on the DaemonSet itself and honouring both the legacy scalar + and the new resourceMonitor.enabled object form. */}} {{- define "tracebloc.resourceMonitorRefreshPinned" -}} -{{- if eq .Values.resourceMonitor false -}} +{{- if not (include "tracebloc.resourceMonitorEnabled" .) -}} true {{- else if (default dict (default dict .Values.images).resourceMonitor).digest -}} true @@ -1556,7 +1593,7 @@ https://api.tracebloc.io/ became a second tenant, two of them were widened and the rest were not." The tri-state made `enabled` a second copy of the answer for a third time. */ -}} -{{- if or (ne .Values.resourceMonitor false) (eq (include "tracebloc.telemetryCollectorState" .) "enabled") }}true{{ end -}} +{{- if or (include "tracebloc.resourceMonitorEnabled" .) (eq (include "tracebloc.telemetryCollectorState" .) "enabled") }}true{{ end -}} {{- end -}} {{/* diff --git a/client/templates/jobs-manager-deployment.yaml b/client/templates/jobs-manager-deployment.yaml index cd8f1cfe..4a016941 100644 --- a/client/templates/jobs-manager-deployment.yaml +++ b/client/templates/jobs-manager-deployment.yaml @@ -750,7 +750,7 @@ spec: # populated would list a namespace that isn't rendered — a 403 every # heartbeat for a workload the operator turned off. - name: NODE_AGENTS_NAMESPACE - value: {{ if ne .Values.resourceMonitor false }}{{ dig "namespace" "name" "" (.Values.nodeAgents | default dict) | quote }}{{ else }}""{{ end }} + value: {{ if (include "tracebloc.resourceMonitorEnabled" .) }}{{ dig "namespace" "name" "" (.Values.nodeAgents | default dict) | quote }}{{ else }}""{{ end }} # backend#664 (Utilization Ladder L0): with NEITHER env.RESOURCE_REQUESTS # nor env.RESOURCE_LIMITS set, BOTH vars are omitted and jobs-manager # sizes the envelope from node allocatable — BUT ONLY IF diff --git a/client/templates/rbac.yaml b/client/templates/rbac.yaml index 07765305..32341007 100644 --- a/client/templates/rbac.yaml +++ b/client/templates/rbac.yaml @@ -232,7 +232,7 @@ roleRef: {{- end }} {{- $nodeAgentsNs := dig "namespace" "name" "" (.Values.nodeAgents | default dict) }} -{{- if and (ne .Values.resourceMonitor false) $nodeAgentsNs (ne $nodeAgentsNs .Release.Namespace) }} +{{- if and (include "tracebloc.resourceMonitorEnabled" .) $nodeAgentsNs (ne $nodeAgentsNs .Release.Namespace) }} --- {{/* jobs-manager reads the resource-monitor DaemonSet for the heartbeat version diff --git a/client/templates/resource-monitor-daemonset.yaml b/client/templates/resource-monitor-daemonset.yaml index 81bf11c4..08cec0eb 100644 --- a/client/templates/resource-monitor-daemonset.yaml +++ b/client/templates/resource-monitor-daemonset.yaml @@ -1,4 +1,4 @@ -{{- if ne .Values.resourceMonitor false }} +{{- if (include "tracebloc.resourceMonitorEnabled" .) }} {{/* Pre-flight: resource-monitor polls the metrics.k8s.io API, so metrics-server must be registered. We probe kube-system via `lookup` first — that returns @@ -66,7 +66,7 @@ {{- $preflight = "skipped-by-values" -}} {{- else -}} {{- if not (lookup "apiregistration.k8s.io/v1" "APIService" "" "v1beta1.metrics.k8s.io") -}} - {{- fail "resourceMonitor is enabled but the metrics.k8s.io/v1beta1 API is not registered. Install metrics-server (https://github.com/kubernetes-sigs/metrics-server) or set resourceMonitor: false. See SECURITY.md.\n\nIf THIS line was instead an `apiservices ... is forbidden` error, the problem is the caller's RBAC, not metrics-server: APIService is cluster-scoped and the built-in `admin` ClusterRole excludes it. Set nodeAgents.metricsServerPreflight: false to skip this check, or run the upgrade with cluster-scope read on apiservices (backend#2469)." -}} + {{- fail "resourceMonitor is enabled but the metrics.k8s.io/v1beta1 API is not registered. Install metrics-server (https://github.com/kubernetes-sigs/metrics-server) or set resourceMonitor.enabled: false. See SECURITY.md.\n\nIf THIS line was instead an `apiservices ... is forbidden` error, the problem is the caller's RBAC, not metrics-server: APIService is cluster-scoped and the built-in `admin` ClusterRole excludes it. Set nodeAgents.metricsServerPreflight: false to skip this check, or run the upgrade with cluster-scope read on apiservices (backend#2469)." -}} {{- end -}} {{- $preflight = "satisfied-by-apiservice" -}} {{- end -}} diff --git a/client/templates/resource-monitor-rbac.yaml b/client/templates/resource-monitor-rbac.yaml index da0339dc..5a68bf2c 100644 --- a/client/templates/resource-monitor-rbac.yaml +++ b/client/templates/resource-monitor-rbac.yaml @@ -1,4 +1,4 @@ -{{- if ne .Values.resourceMonitor false }} +{{- if (include "tracebloc.resourceMonitorEnabled" .) }} --- apiVersion: v1 kind: ServiceAccount @@ -27,7 +27,7 @@ metadata: the training/jobs isolation footprint elsewhere -- it must not cripple node telemetry by leaving the DaemonSet without the permissions it cannot run without. If a deployment genuinely cannot allow any cluster-scoped read, disable the - monitor entirely via .Values.resourceMonitor=false rather than deploying it broken. + monitor entirely via .Values.resourceMonitor.enabled=false rather than deploying it broken. */}} --- apiVersion: rbac.authorization.k8s.io/v1 diff --git a/client/templates/resource-monitor-scc.yaml b/client/templates/resource-monitor-scc.yaml index f9fba66c..b6b0f1f5 100644 --- a/client/templates/resource-monitor-scc.yaml +++ b/client/templates/resource-monitor-scc.yaml @@ -1,4 +1,4 @@ -{{- if and (ne .Values.resourceMonitor false) .Values.openshift.scc.enabled }} +{{- if and (include "tracebloc.resourceMonitorEnabled" .) .Values.openshift.scc.enabled }} --- apiVersion: security.openshift.io/v1 kind: SecurityContextConstraints diff --git a/client/templates/secrets.yaml b/client/templates/secrets.yaml index 474fc71c..cecfbeb3 100644 --- a/client/templates/secrets.yaml +++ b/client/templates/secrets.yaml @@ -510,7 +510,7 @@ data: {{- if (include "tracebloc.bootstrapDbReparent" .) }} DB_BOOTSTRAP_PASSWORD: {{ $bootstrapDbPassword | b64enc | quote }} {{- end }} -{{- if and (ne .Values.resourceMonitor false) (ne .Values.nodeAgents.namespace.name .Release.Namespace) }} +{{- if and (include "tracebloc.resourceMonitorEnabled" .) (ne .Values.nodeAgents.namespace.name .Release.Namespace) }} --- # Mirrored into the node-agents namespace so the resource-monitor DaemonSet # can read CLIENT_ID / CLIENT_PASSWORD via secretKeyRef. Secrets are diff --git a/client/tests/resource_monitor_test.yaml b/client/tests/resource_monitor_test.yaml index 2e9c7d24..4f163a85 100644 --- a/client/tests/resource_monitor_test.yaml +++ b/client/tests/resource_monitor_test.yaml @@ -251,3 +251,99 @@ tests: - equal: path: metadata.namespace value: tracebloc-node-agents + + # ── RFC-0076 bool→object alias window (client#1009) ─────────────────────────── + # `resourceMonitor: ` is being renamed to `resourceMonitor.enabled: ` + # (D2). This is a bool→object change, so a stored values.yaml or a bare + # `--set resourceMonitor=true` still arrives as a SCALAR; the templates route the + # gate through tracebloc.resourceMonitorEnabled, which must honour BOTH shapes + # for the whole window (remove_by: 2026-12-31). There are two crash directions + # the helper prevents: a blind `ne .Values.resourceMonitor false` gate fails + # "incompatible types for comparison: map and bool" on the new object, and a + # blind `.Values.resourceMonitor.enabled` read fails "can't evaluate field + # enabled in interface {}" on the legacy scalar. The three canonical inputs are + # legacy scalar, new object, and unset; null (a --reuse-values upgrade from a + # chart that predates the key) and `{}` (object with .enabled absent) are the + # two edges of "unset". + - it: renders the DaemonSet for the legacy scalar resourceMonitor=true + template: templates/resource-monitor-daemonset.yaml + set: + resourceMonitor: true + asserts: + - hasDocuments: + count: 1 + - isKind: + of: DaemonSet + + - it: renders the DaemonSet for the new object resourceMonitor.enabled=true + template: templates/resource-monitor-daemonset.yaml + set: + resourceMonitor: + enabled: true + asserts: + - hasDocuments: + count: 1 + - isKind: + of: DaemonSet + + - it: renders the DaemonSet on the chart default (unset — values.yaml object form) + template: templates/resource-monitor-daemonset.yaml + asserts: + - hasDocuments: + count: 1 + - isKind: + of: DaemonSet + + - it: treats an absent key (null, --reuse-values from a pre-key chart) as enabled + template: templates/resource-monitor-daemonset.yaml + set: + resourceMonitor: null + asserts: + - hasDocuments: + count: 1 + - isKind: + of: DaemonSet + + - it: treats an object with .enabled absent as enabled (default true) + template: templates/resource-monitor-daemonset.yaml + set: + resourceMonitor: {} + asserts: + - hasDocuments: + count: 1 + - isKind: + of: DaemonSet + + - it: renders nothing for the legacy scalar resourceMonitor=false + template: templates/resource-monitor-daemonset.yaml + set: + resourceMonitor: false + asserts: + - hasDocuments: + count: 0 + + - it: renders nothing for the new object resourceMonitor.enabled=false + template: templates/resource-monitor-daemonset.yaml + set: + resourceMonitor: + enabled: false + asserts: + - hasDocuments: + count: 0 + + # The object form CLOSES its keys (values.schema.json additionalProperties: + # false), so a mistyped `enabled` (enable / Enabled / disabled) is REFUSED at + # chart load rather than silently `dig`-defaulting back to true and turning the + # monitor back on — a silent-misconfiguration surface the scalar had no sub-key + # to expose. BARE failedTemplate, per the auto-upgrade schema tests: 0.5.2 + # reports a schema rejection as an errored test with no matchable render error, + # and errorPattern is silently ignored (backend#2606). To pin the specific + # "additional properties 'enable' not allowed" text, assert it from outside the + # plugin with `helm template` + grep. + - it: refuses a mistyped enabled sub-key instead of silently staying enabled + template: templates/resource-monitor-daemonset.yaml + set: + resourceMonitor: + enable: false + asserts: + - failedTemplate: {} diff --git a/client/values.schema.json b/client/values.schema.json index 4fc48e03..63dd3783 100644 --- a/client/values.schema.json +++ b/client/values.schema.json @@ -333,9 +333,19 @@ "description": "Use ClusterRole (true) or namespace-scoped Role (false). Defaults to true." }, "resourceMonitor": { - "type": "boolean", - "default": true, - "description": "Deploy resource-monitor DaemonSet. Defaults to true." + "type": [ + "boolean", + "object" + ], + "description": "Deploy the resource-monitor DaemonSet. RFC-0076 (D2): prefer the object form `resourceMonitor.enabled: `; the legacy scalar `resourceMonitor: ` (and `--set resourceMonitor=true`) is still accepted through the alias window (remove_by: 2026-12-31) — see tracebloc.resourceMonitorEnabled. Defaults to enabled.", + "properties": { + "enabled": { + "type": "boolean", + "default": true, + "description": "Deploy the resource-monitor DaemonSet. Defaults to true." + } + }, + "additionalProperties": false }, "nodeAgents": { "type": "object", diff --git a/client/values.yaml b/client/values.yaml index f7964a8d..f52fb307 100644 --- a/client/values.yaml +++ b/client/values.yaml @@ -376,8 +376,14 @@ clusterScope: true # OC: uses the built-in OpenShift metrics stack. # kubeadm / bare-metal: install metrics-server manually; add # --kubelet-insecure-tls on clusters with self-signed kubelet certs. -# Set to false on clusters where metrics-server cannot be installed. -resourceMonitor: true +# Set enabled to false on clusters where metrics-server cannot be installed. +# +# RFC-0076 (D2): this was the scalar `resourceMonitor: ` and is now +# `resourceMonitor.enabled: `. The chart still honours the legacy scalar +# (and `--set resourceMonitor=true|false`) through the alias window +# (remove_by: 2026-12-31) — see tracebloc.resourceMonitorEnabled in _helpers.tpl. +resourceMonitor: + enabled: true # -- Node-level agents (currently: tracebloc-resource-monitor DaemonSet). # The resource-monitor needs hostPath /proc and /sys to read node metrics, From d0abacd5e2ed4f91b66a4cc034252093fcdf2bf0 Mon Sep 17 00:00:00 2001 From: Syed Is Saqlain Date: Fri, 11 Sep 2026 12:33:50 +0400 Subject: [PATCH 2/3] fix(image-refresh): write Pass-0 stale-pin annotations before the restart block (#1039) The #563 flap guard does WARN + FLAP_KEY + exit 0 once refresh-attempt >= MAX_REFRESH_ATTEMPTS, before the end-of-tick digest annotate. The stale-pin clears were batched into that final annotate, so on a tick that is both off-digest (restart_needed=1) and latched they were dropped, leaving a false "pin is stale" finding to persist. Move the stale-pin writes (clears + finding-set) into their own accumulator, annotated in a bounded, non-fatal call above the restart block. They touch only stale-pin- keys, never last-refreshed--digest, so writing them before the rollout can't affect the recorded==latest skip; the digest record stays below, after a successful rollout. Tests: image-refresh-latched-annotate.bats (new, extracts the shipped tail and asserts the clear lands while the digest record does not on a latched tick, and both land on a healthy tick); image-refresh-stale-pin.bats harness updated for the accumulator; image_refresh_test.yaml render guard covering all four stale-pin writes on both axes. Split from #1008 (item 2). Bumps chart to 1.9.117. --- client/Chart.yaml | 4 +- client/templates/image-refresh-cronjob.yaml | 41 ++++- client/tests/image_refresh_test.yaml | 46 +++++ .../tests/image-refresh-latched-annotate.bats | 164 ++++++++++++++++++ scripts/tests/image-refresh-stale-pin.bats | 5 + 5 files changed, 254 insertions(+), 6 deletions(-) create mode 100644 scripts/tests/image-refresh-latched-annotate.bats diff --git a/client/Chart.yaml b/client/Chart.yaml index d8b3ed31..ecd2e38c 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.116 -appVersion: "1.9.116" +version: 1.9.117 +appVersion: "1.9.117" keywords: - tracebloc - kubernetes diff --git a/client/templates/image-refresh-cronjob.yaml b/client/templates/image-refresh-cronjob.yaml index 352c0921..c57cf07a 100644 --- a/client/templates/image-refresh-cronjob.yaml +++ b/client/templates/image-refresh-cronjob.yaml @@ -500,6 +500,13 @@ data: restart_needed=0 annotate_args="" + # Pass-0 diagnostic annotations -- the `stale-pin-` clears and + # finding-sets -- accumulate SEPARATELY from annotate_args and are written in + # their own bounded annotate BEFORE the restart block (#1008 item 1). + # They touch only the stale-pin keys, never `last-refreshed--digest`, + # so they are safe to land before the rollout; keeping them out of the final + # annotate is what lets them survive a latched-flap tick (see the write site). + stale_pin_args="" # `kubectl set image` argument lists, accumulated per WORKLOAD (a single # `set image` call can carry several container=ref pairs, so the # two-container jobs-manager Deployment is re-imaged in one patch and @@ -585,7 +592,7 @@ data: disabled_stale_key="tracebloc.io/stale-pin-${repo#*/}" if [ -n "$(get_annotation "$disabled_stale_key" || true)" ]; then log " clearing ${disabled_stale_key}: this image is no longer pinned by a digest" - annotate_args="$annotate_args ${disabled_stale_key}-" + stale_pin_args="$stale_pin_args ${disabled_stale_key}-" fi continue fi @@ -613,7 +620,7 @@ data: # (@saqlainsyed007 + Bugbot on client#824.) if [ -n "$(get_annotation "$stale_key" || true)" ]; then log " clearing a previous ${stale_key}: the pin is current again" - annotate_args="$annotate_args ${stale_key}-" + stale_pin_args="$stale_pin_args ${stale_key}-" fi else log " WARN: PIN IS STALE. values pin ${pin_digest}" @@ -624,7 +631,7 @@ data: log " in values (backend#2458)." # Queryable after the log ages out, on the same object the refresh # annotations use, so `kubectl describe` shows pin state beside refresh state. - annotate_args="$annotate_args ${stale_key}=${pin_latest}" + stale_pin_args="$stale_pin_args ${stale_key}=${pin_latest}" fi continue fi @@ -637,7 +644,7 @@ data: unpinned_stale_key="tracebloc.io/stale-pin-${repo#*/}" if [ -n "$(get_annotation "$unpinned_stale_key" || true)" ]; then log " clearing ${unpinned_stale_key}: this image is no longer pinned" - annotate_args="$annotate_args ${unpinned_stale_key}-" + stale_pin_args="$stale_pin_args ${unpinned_stale_key}-" fi latest="$(get_latest_digest "$repo" "$IMAGE_TAG" "$IMAGE_REGISTRY" || true)" @@ -881,6 +888,32 @@ data: esac done + # Pass-0 annotations land HERE, before the restart block (#1008 item 1). + # The restart block's #563 flap guard does `WARN + FLAP_KEY + exit 0` + # once refresh-attempt >= MAX_REFRESH_ATTEMPTS -- BEFORE the digest-record + # annotate at the end of the tick. So on a tick that is both off-digest + # (restart_needed=1) and latched, batching the stale-pin writes into that + # final annotate dropped them: a stale-pin CLEAR that never landed leaves a + # FALSE "pin is stale" finding to persist forever (a write-only annotation + # outliving its problem -- the class client#824's clear paths fixed), and it + # is dropped on the exact tick refresh is dead, when the finding matters most. + # These touch only the `stale-pin-` keys, never + # `last-refreshed--digest`, so writing them before the rollout cannot + # affect the `recorded == latest` skip logic -- unlike the digest record, + # which MUST stay after a successful `rollout status`, since annotating the + # digest before a failed rollout would freeze the workload on the old image + # (@shujaatTracebloc on #1008). NON-FATAL, like the SKIP_KEY clear above + # (backend#2007): a transient failure on a diagnostic annotation must not + # abort the tick before the re-image; a stale value is re-reconciled next tick. + if [ -n "$stale_pin_args" ]; then + log "updating stale-pin annotations:$stale_pin_args" + # shellcheck disable=SC2086 # word-split stale_pin_args intentional + if ! sp_err="$(kubectl annotate deployment -n "$RELEASE_NAMESPACE" "$DEPLOYMENT_NAME" \ + $stale_pin_args --overwrite --request-timeout=15s 2>&1 >/dev/null)"; then + log " WARNING: could not update stale-pin annotations on deployment/${DEPLOYMENT_NAME}: ${sp_err:-unknown error}. Continuing -- these are diagnostic bookkeeping and a stale value is re-reconciled on the next tick." + fi + fi + # Order matters: rollout FIRST, annotate AFTER `rollout status` # succeeds. Annotating first would let a failed rollout silently # freeze the deployment on the old image (next tick sees diff --git a/client/tests/image_refresh_test.yaml b/client/tests/image_refresh_test.yaml index ad457ebf..488314d3 100644 --- a/client/tests/image_refresh_test.yaml +++ b/client/tests/image_refresh_test.yaml @@ -1020,3 +1020,49 @@ tests: - notMatchRegex: path: data["image-refresh.sh"] pattern: 'recorded="\$\(get_annotation "\$key" \|\| true\)"' + + - it: stale-pin annotations are written BEFORE the restart block, so they survive a latched flap + # Guards #1008 item 1. The #563 flap guard does WARN + FLAP_KEY + + # exit 0 once refresh-attempt >= MAX, BEFORE the final digest-record annotate. + # Batching the stale-pin CLEARS into that final annotate dropped them on a + # tick that is both off-digest and latched -- leaving a FALSE stale-pin + # finding to persist. They now accumulate in their own list and are annotated + # above the restart block. image-refresh-latched-annotate.bats asserts the + # BEHAVIOUR; these lock the code shapes. + template: templates/image-refresh-cronjob.yaml + documentIndex: 0 + asserts: + # ALL FOUR stale-pin writes go to their own accumulator, not annotate_args. + # Every key gets a positive (goes to stale_pin_args) AND a negative (does NOT + # ride annotate_args) -- Bugbot Low + @saadqbal on #1039: a key covered by + # neither could be re-batched into annotate_args and dropped on a latched + # tick while both suites stayed green (this guard's own thesis, one level up). + # The two clears: + - matchRegex: + path: data["image-refresh.sh"] + pattern: 'stale_pin_args="\$stale_pin_args \$\{stale_key\}-"' + - matchRegex: + path: data["image-refresh.sh"] + pattern: 'stale_pin_args="\$stale_pin_args \$\{unpinned_stale_key\}-"' + # the disabled-monitor clear (the third clear -- was uncovered): + - matchRegex: + path: data["image-refresh.sh"] + pattern: 'stale_pin_args="\$stale_pin_args \$\{disabled_stale_key\}-"' + # the finding-SET (a lost set means a real staleness goes unreported): + - matchRegex: + path: data["image-refresh.sh"] + pattern: 'stale_pin_args="\$stale_pin_args \$\{stale_key\}=\$\{pin_latest\}"' + # and that accumulator is annotated BEFORE the restart block (ordering) + - matchRegex: + path: data["image-refresh.sh"] + pattern: '(?s)if \[ -n "\$stale_pin_args" \]; then.*if \[ "\$restart_needed" -eq 1 \]; then' + # NONE of the stale-pin keys may ride the final digest-record annotate_args + - notMatchRegex: + path: data["image-refresh.sh"] + pattern: 'annotate_args="\$annotate_args \$\{stale_key\}' + - notMatchRegex: + path: data["image-refresh.sh"] + pattern: 'annotate_args="\$annotate_args \$\{unpinned_stale_key\}' + - notMatchRegex: + path: data["image-refresh.sh"] + pattern: 'annotate_args="\$annotate_args \$\{disabled_stale_key\}' diff --git a/scripts/tests/image-refresh-latched-annotate.bats b/scripts/tests/image-refresh-latched-annotate.bats new file mode 100644 index 00000000..c7a54ba4 --- /dev/null +++ b/scripts/tests/image-refresh-latched-annotate.bats @@ -0,0 +1,164 @@ +#!/usr/bin/env bats +# image-refresh writes the Pass-0 stale-pin annotations BEFORE the restart block, +# so they survive a tick that is both off-digest (restart_needed=1) and LATCHED +# (refresh-attempt >= MAX_REFRESH_ATTEMPTS). +# +# #1008 item 1. The #563 flap guard does `WARN + FLAP_KEY + exit 0` once +# the attempt counter reaches MAX -- BEFORE the digest-record annotate at the end +# of the tick. When the stale-pin CLEARS were batched into that final annotate, +# a latched tick dropped them, leaving a FALSE "pin is stale" finding to persist +# forever -- and on the exact tick refresh is dead, when the finding matters most. +# The fix moves the stale-pin writes into their own bounded annotate above the +# restart block; the `last-refreshed` digest record deliberately stays BELOW, +# after a successful rollout (@shujaatTracebloc on #1008). +# +# This asserts BEHAVIOUR: it extracts the shipped tail (the stale-pin annotate + +# the restart block + the final digest annotate) from the RENDERED chart and +# drives it with kubectl and the attempt-counter read stubbed, so re-batching the +# stale-pin writes back into the final annotate reddens. + +setup() { + TMP="$(mktemp -d)" + CHART="${BATS_TEST_DIRNAME}/../../client" + helm template t "$CHART" --set clientId=x --set clientPassword=y \ + --set storageClass.create=false > "$TMP/rendered.yaml" + python3 - "$TMP/rendered.yaml" "$TMP/tail.sh" <<'PYX' +import sys + +try: + import yaml +except ImportError: + sys.exit("[ERROR] PyYAML required (pip install pyyaml)") + +MARKER = "already on the pinned digest; no-op" + +def walk(o): + if isinstance(o, str) and MARKER in o: + return o + if isinstance(o, dict): + for v in o.values(): + r = walk(v) + if r: + return r + if isinstance(o, list): + for v in o: + r = walk(v) + if r: + return r + +script = None +for d in yaml.safe_load_all(open(sys.argv[1])): + if not d: + continue + script = walk(d) + if script: + break +assert script, "no rendered image-refresh script found" + +lines = script.splitlines() +start = next(i for i, l in enumerate(lines) + if l.strip() == 'if [ -n "$stale_pin_args" ]; then') +# the LAST `log "tick complete"` -- the flap-guard early exits use the same line, +# so the first match would truncate the region mid-restart-block. +end = max(i for i in range(start, len(lines)) + if lines[i].strip() == 'log "tick complete"') +region = lines[start:end + 1] +indent = min(len(l) - len(l.lstrip()) for l in region if l.strip()) +open(sys.argv[2], "w").write("\n".join(l[indent:] for l in region)) +PYX +} +teardown() { rm -rf "$TMP"; } + +# Drives the shipped tail with kubectl + the ATTEMPT_KEY read stubbed. +# $1 = STUB_ATTEMPT what get_annotation returns for ATTEMPT_KEY (the flap count) +# $2 = JM_SET_ARGS `set image` args (non-empty => a rollout runs, stubbed OK) +# stale_pin_args and annotate_args are always populated so the test can assert +# which of the two landed. +# +# The kubectl stub records EVERY call to "$TMP/calls.log" rather than stdout, +# because the stale-pin annotate is wrapped in a non-fatal handler that discards +# its stdout (`2>&1 >/dev/null`) -- exactly as a real silent-success annotate +# would. The file captures the call regardless of the caller's redirections; +# assert kubectl invocations against "$TMP/calls.log" and log lines against stdout. +run_tail() { + : > "$TMP/calls.log" + cat > "$TMP/harness.sh" <> "\$CALLS"; } +get_annotation() { case "\$1" in "\$ATTEMPT_KEY") printf '%s' "\$STUB_ATTEMPT" ;; esac; } +$(cat "$TMP/tail.sh") +EOF + sh "$TMP/harness.sh" "${1:-0}" "${2:-}" +} + +@test "the harness really extracted the shipped tail (not an empty file)" { + [ -s "$TMP/tail.sh" ] || return 1 + grep -q 'stale_pin_args' "$TMP/tail.sh" || return 1 + grep -q 'restart_needed' "$TMP/tail.sh" || return 1 +} + +@test "LATCHED tick (restart_needed=1, attempt>=MAX): stale-pin clear LANDS, digest record does NOT" { + # The acceptance case (#1008 item 1). attempt=3, MAX=3 -> the flap guard + # WARNs, annotates FLAP_KEY, and exit 0s. The stale-pin clear must already have + # been written (before the restart block); the last-refreshed digest record + # must NOT be (its annotate is after the guard and never runs). + run run_tail "3" + [ "$status" -eq 0 ] || return 1 + calls="$(cat "$TMP/calls.log")" + # stale-pin clear landed, above the restart block + [[ "$calls" == *"annotate deployment"*"tracebloc.io/stale-pin-jobs-manager-"* ]] || return 1 + # the flap guard fired + [[ "$output" == *"FLAP DETECTED"* ]] || return 1 + [[ "$calls" == *"tracebloc.io/refresh-flap-detected=3"* ]] || return 1 + # the digest record did NOT land (dropped by the exit 0, as designed) + [[ "$calls" != *"last-refreshed-jobs-manager-digest=sha256:beef"* ]] || return 1 +} + +@test "NON-latched tick (attempt Date: Fri, 11 Sep 2026 10:57:54 +0200 Subject: [PATCH 3/3] sec(mirror-publish): assert GITLEAKS_SHA256 is a 64-hex digest before sha256sum -c (backend#3676) (#1047) A mangled, empty, or truncated pin piped straight into `sha256sum -c` can pass verification: coreutils reports a malformed checksum line as "no properly formatted checksum lines found", and whether that is a non-zero exit is build-dependent (GNU exits 1; some builds exit 0). So a bad pin can let an UNVERIFIED gitleaks binary get extracted and run, defeating the point of pinning the scanner. Add the same fail-closed hex-digest guard the kubeconform install already uses in helm-ci.yaml, adjusted for this variable and placed right after the curl download and before the sha256sum line, inside the existing set -euo pipefail block. Co-authored-by: Claude Opus 4.8 --- .github/workflows/mirror-publish.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/workflows/mirror-publish.yaml b/.github/workflows/mirror-publish.yaml index 4b593419..31aa23e1 100644 --- a/.github/workflows/mirror-publish.yaml +++ b/.github/workflows/mirror-publish.yaml @@ -277,6 +277,16 @@ jobs: curl -fsSL --tlsv1.2 --retry 3 --connect-timeout 10 --max-time 120 \ -o "$RUNNER_TEMP/gitleaks.tgz" \ "https://github.com/gitleaks/gitleaks/releases/download/v${GITLEAKS_VERSION}/gitleaks_${GITLEAKS_VERSION}_linux_x64.tar.gz" + # Assert the digest is a full SHA-256 before trusting it. `sha256sum -c` + # reports a malformed line as "no properly formatted checksum lines + # found", and whether that is a non-zero exit depends on the coreutils + # build (GNU exits 1; the macOS sha256sum exits 0). An empty or + # truncated env var must be a hard failure here, not a verification + # that quietly checks nothing and lets the install proceed. + if [[ ! "${GITLEAKS_SHA256}" =~ ^[0-9a-f]{64}$ ]]; then + echo "::error::GITLEAKS_SHA256 is not a 64-character hex SHA-256 digest." + exit 1 + fi echo "${GITLEAKS_SHA256} $RUNNER_TEMP/gitleaks.tgz" | sha256sum -c - tar -xzf "$RUNNER_TEMP/gitleaks.tgz" -C "$RUNNER_TEMP/bin" gitleaks chmod 0755 "$RUNNER_TEMP/bin/gitleaks"