From ce9a7f1cd41a33316ecabf2c09a2d02e97e564b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20M=C3=BCller?= <7556827+max06@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:14:40 +0000 Subject: [PATCH 1/2] Anchor instance-level patch/transformer file entries deployment-relative MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit applyListOverride appended apps[].strategicMergePatches/jsonPatches/ transformers entries verbatim — a file path there resolved against helmfile's cache dir in remote consumption, effectively undefined, so file entries silently never applied (only inline maps worked). String entries now anchor to the deployment dir via convertPaths, mirroring how instance values: files resolve — including the fail-on-missing contract. Fixes #63 Co-Authored-By: Claude Fable 5 --- templates/_functions.tpl | 18 +++++++++-- templates/helmfile.instance.yaml.gotmpl | 9 +++--- ...ter1-deployment48-instance-file-patch.bats | 30 +++++++++++++++++++ .../apps/deployment48/deployment.yaml | 10 +++++++ .../apps/deployment48/instance-patch.yaml | 7 +++++ .../missing-instance-patch/deployment.yaml | 7 +++++ 6 files changed, 74 insertions(+), 7 deletions(-) create mode 100644 tests/bats/values/cluster1-deployment48-instance-file-patch.bats create mode 100644 tests/deployments/cluster1/apps/deployment48/deployment.yaml create mode 100644 tests/deployments/cluster1/apps/deployment48/instance-patch.yaml create mode 100644 tests/fixtures-negative/cluster1/apps/missing-instance-patch/deployment.yaml diff --git a/templates/_functions.tpl b/templates/_functions.tpl index 1b3a0ff..e758faf 100644 --- a/templates/_functions.tpl +++ b/templates/_functions.tpl @@ -16,7 +16,10 @@ wholesale by helmfile.instance with the values-loader path, and the loader runs the progressive merge (SOPS decryption, .yaml.gotmpl rendering, and merging in declaration order) at release-evaluation time. -Context: dict with "release", "instance", "templateDir", and "field". +Context: dict with "release", "instance", "templateDir", "deploymentDir", +and "field". Template-level entries anchor template-relative, +instance-level entries anchor deployment-relative (mirroring how the +respective values: files resolve). */ -}} {{- define "atlas.applyListOverride" -}} {{- $field := .field }} @@ -35,12 +38,21 @@ Context: dict with "release", "instance", "templateDir", and "field". {{- end }} {{- end }} - {{- /* 2. Append any instance-level overrides */ -}} + {{- /* 2. Append any instance-level overrides. String entries are file + references authored in deployment.yaml — anchor them relative to + the deployment dir (unanchored they would resolve against + helmfile's cache dir, effectively undefined in remote + consumption). Inline maps pass through untouched. */ -}} {{- if hasKey .instance $field }} {{- $toAdd := .instance | get $field list }} {{- if $toAdd }} + {{- $convertedAdd := include "convertPaths" (dict + "targetPath" .deploymentDir + "values" (toJson $toAdd) + "field" (printf "%s (instance-level)" $field) + ) | fromJson }} {{- $current := .release | get $field list }} - {{- $_ := set .release $field (concat $current $toAdd) }} + {{- $_ := set .release $field (concat $current $convertedAdd) }} {{- end }} {{- end }} {{- end }} diff --git a/templates/helmfile.instance.yaml.gotmpl b/templates/helmfile.instance.yaml.gotmpl index 6645c1b..60e8690 100644 --- a/templates/helmfile.instance.yaml.gotmpl +++ b/templates/helmfile.instance.yaml.gotmpl @@ -262,10 +262,11 @@ Purpose: resolves entries itself at release-evaluation time. */ -}} {{- range $listField := list "strategicMergePatches" "jsonPatches" "transformers" }} {{- include "atlas.applyListOverride" (dict - "release" $release - "instance" $thisInstance - "templateDir" $templateDir - "field" $listField + "release" $release + "instance" $thisInstance + "templateDir" $templateDir + "deploymentDir" (dir $.Values.atlas.deployment.deploymentPath) + "field" $listField ) }} {{- end }} diff --git a/tests/bats/values/cluster1-deployment48-instance-file-patch.bats b/tests/bats/values/cluster1-deployment48-instance-file-patch.bats new file mode 100644 index 0000000..30f8676 --- /dev/null +++ b/tests/bats/values/cluster1-deployment48-instance-file-patch.bats @@ -0,0 +1,30 @@ +#!/usr/bin/env bats +# +# Scenario: instance-level strategicMergePatches FILE entry (issue #63). +# +# deployment48 references ./instance-patch.yaml next to its deployment.yaml. +# applyListOverride must anchor the path deployment-relative (unanchored it +# resolves against helmfile's cache dir and silently never applies) and the +# patch annotation must land in the rendered ConfigMap. + +load '../helpers/render' + +CLUSTER=cluster1 +DEPLOYMENT=deployment48 +RELEASE=stage3-patches-release + +setup_file() { ensure_rendered; } + +@test "d48: release rendered" { + instance_rendered_any "$CLUSTER" "$DEPLOYMENT" "$RELEASE" +} + +@test "d48: template-level strategicMergePatches still applied" { + run render_contains "$CLUSTER" "$DEPLOYMENT" "$RELEASE" "atlas-test/patched" + [ "$status" -eq 0 ] +} + +@test "d48: instance-level file patch applied (deployment-relative anchor)" { + run render_contains "$CLUSTER" "$DEPLOYMENT" "$RELEASE" "atlas-test/from-instance-file" + [ "$status" -eq 0 ] +} diff --git a/tests/deployments/cluster1/apps/deployment48/deployment.yaml b/tests/deployments/cluster1/apps/deployment48/deployment.yaml new file mode 100644 index 0000000..a059447 --- /dev/null +++ b/tests/deployments/cluster1/apps/deployment48/deployment.yaml @@ -0,0 +1,10 @@ +# Fixture: instance-level strategicMergePatches FILE entry (issue #63). +# The path is deployment-relative — applyListOverride anchors it to this +# directory before appending, so the patch file lives next to this +# deployment.yaml (unanchored it would resolve against helmfile's cache +# dir and never apply in remote consumption). +apps: + - template: app-patches + namespace: test + strategicMergePatches: + - ./instance-patch.yaml diff --git a/tests/deployments/cluster1/apps/deployment48/instance-patch.yaml b/tests/deployments/cluster1/apps/deployment48/instance-patch.yaml new file mode 100644 index 0000000..ecb4598 --- /dev/null +++ b/tests/deployments/cluster1/apps/deployment48/instance-patch.yaml @@ -0,0 +1,7 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: stage3-patches-release-chart1 + namespace: test + annotations: + atlas-test/from-instance-file: "yes" diff --git a/tests/fixtures-negative/cluster1/apps/missing-instance-patch/deployment.yaml b/tests/fixtures-negative/cluster1/apps/missing-instance-patch/deployment.yaml new file mode 100644 index 0000000..140b655 --- /dev/null +++ b/tests/fixtures-negative/cluster1/apps/missing-instance-patch/deployment.yaml @@ -0,0 +1,7 @@ +# NEGATIVE fixture (issue #63): instance-level strategicMergePatches file +# that does not exist — must fail loudly, not deploy unpatched. +apps: + - template: app-novals + namespace: test + strategicMergePatches: + - ./does-not-exist.yaml From c58888459a718792ecac3c3b24d7e259f5ed35c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20M=C3=BCller?= <7556827+max06@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:14:45 +0000 Subject: [PATCH 2/2] Discovery guard rails: leaf-name uniqueness, "." pseudo-cluster, debug gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Leaf cluster names are validated across the WHOLE repo before the stage-1 filter applies — collisions break downstream resource naming (clusterName), so a filtered per-app render must not hide them (#64). - The global apps/ dir no longer registers as pseudo-cluster "." in a repo with global deployments but no clusters; the correct outcome is zero render targets (#65). - The discovery dump (full clusters dict as comment lines in every rendered state) is now opt-in via ATLAS_DEBUG_DISCOVERY (#66). Fixes #64 Fixes #65 Fixes #66 Co-Authored-By: Claude Fable 5 --- templates/helmfile.all.yaml.gotmpl | 31 +++++++- tests/bats/integration/discovery-guards.bats | 75 +++++++++++++++++++ .../group-a/dupe/apps/app1/deployment.yaml | 5 ++ .../group-b/dupe/apps/app1/deployment.yaml | 5 ++ .../apps/global-app/deployment.yaml | 7 ++ 5 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 tests/bats/integration/discovery-guards.bats create mode 100644 tests/fixtures-dup-leaf/group-a/dupe/apps/app1/deployment.yaml create mode 100644 tests/fixtures-dup-leaf/group-b/dupe/apps/app1/deployment.yaml create mode 100644 tests/fixtures-global-only/apps/global-app/deployment.yaml diff --git a/templates/helmfile.all.yaml.gotmpl b/templates/helmfile.all.yaml.gotmpl index 28687ad..431db0a 100644 --- a/templates/helmfile.all.yaml.gotmpl +++ b/templates/helmfile.all.yaml.gotmpl @@ -86,14 +86,28 @@ {{- $clustersRaw := include "glob" $clustersPattern | fromJson }} {{- $clusters := dict }} +{{- $leafNamesSeen := dict }} {{- range $clustersRaw }} {{- $clusterPath := . | trimPrefix (printf "%s/" $deploymentsDir) | dir }} - {{- if ne $clusterPath "" }} + {{- /* "" would be an empty match; "." is the global apps/ dir itself + (trimPrefix+dir collapse it to the current dir) — in a repo with + global deployments but no clusters yet it would otherwise register + as a pseudo-cluster and produce a nonsense render target. */ -}} + {{- if and (ne $clusterPath "") (ne $clusterPath ".") }} {{- $childPattern := printf "%s/%s/*/apps" $deploymentsDir $clusterPath }} {{- $hasChildren := include "glob" $childPattern | fromJson }} {{- $isLeaf := eq (len $hasChildren) 0 }} + {{- if $isLeaf }} + {{- /* Leaf-name bookkeeping runs BEFORE the stage-1 filter so a + filtered (per-app ArgoCD) render still validates the whole + repo — name collisions are a global problem and must not + hide behind atlas.filter.cluster. */ -}} + {{- $leafName := $clusterPath | base }} + {{- $seenAt := $leafNamesSeen | get $leafName list }} + {{- $_ := set $leafNamesSeen $leafName (append $seenAt $clusterPath) }} + {{- end }} {{- $clusterMatches := or (eq $filterCluster "") (eq $clusterPath $filterCluster) }} {{- if and $isLeaf $clusterMatches }} {{- $_ := set $clusters $clusterPath (list) }} @@ -101,6 +115,16 @@ {{- end }} {{- end }} +{{- /* Leaf cluster names must be unique across the repo: clusterName (the + leaf) is used to name downstream resources (ArgoCD Applications etc.), + so two groups containing same-named clusters silently produce + colliding names. Fail with the offending paths instead. */ -}} +{{- range $leafName, $paths := $leafNamesSeen }} + {{- if gt (len $paths) 1 }} + {{- fail (printf "cluster discovery: duplicate leaf cluster name %q used by: %s — leaf names must be unique across the repo (clusterName names downstream resources)" $leafName (join ", " $paths)) }} + {{- end }} +{{- end }} + # STEP 2: COLLECT DEPLOYMENTS FOR EACH CLUSTER # For each leaf cluster, gather deployments from: # 1. Global level (applied to all). @@ -142,10 +166,15 @@ {{- $_ := set $clusters $cluster $collectedDeployments -}} {{- end }} +{{- /* Discovery dump: the full clusters/deployments dict as comment lines. + Real bytes in every rendered state file on large repos, so it is + opt-in for debugging only. */ -}} +{{- if env "ATLAS_DEBUG_DISCOVERY" }} # All recognized deployments {{- range (toYaml $clusters | splitList "\n") }} # {{ . }} {{- end }} +{{- end }} # OUTPUT # Generate YAML helmfiles list, each pointing to the single deployment renderer, diff --git a/tests/bats/integration/discovery-guards.bats b/tests/bats/integration/discovery-guards.bats new file mode 100644 index 0000000..4f39ee5 --- /dev/null +++ b/tests/bats/integration/discovery-guards.bats @@ -0,0 +1,75 @@ +#!/usr/bin/env bats +# +# Scenario: cluster-discovery guard rails (issues #63, #64, #65, #66). +# +# Uses dedicated fixture roots via atlas.deploymentDefinitions overrides — +# same isolation approach as error-paths.bats. + +_root() { cd "${BATS_TEST_DIRNAME}/../../.." && pwd; } + +_render_fixtures() { + local fixtures="$1" root; shift + root="$(_root)" + helmfile -f "$root/tests/helmfile.yaml.gotmpl" \ + --state-values-set "atlas.deploymentDefinitions=${fixtures}" \ + template --skip-schema-validation "$@" 2>&1 +} + +# --- duplicate leaf cluster names (issue #64) --------------------------------- + +@test "dup leaf names: render fails" { + run _render_fixtures fixtures-dup-leaf + [ "$status" -ne 0 ] +} + +@test "dup leaf names: error lists the name and both paths" { + run _render_fixtures fixtures-dup-leaf + echo "$output" | grep -q 'duplicate leaf cluster name "dupe"' + echo "$output" | grep -q "group-a/dupe" + echo "$output" | grep -q "group-b/dupe" +} + +@test "dup leaf names: stage-1 filter does not hide the collision" { + ATLAS_FILTER_CLUSTER=group-a/dupe run _render_fixtures fixtures-dup-leaf + [ "$status" -ne 0 ] + echo "$output" | grep -q 'duplicate leaf cluster name "dupe"' +} + +# --- zero-cluster repo: no pseudo-cluster "." (issue #65) --------------------- + +@test "global-only repo: no pseudo-cluster render target" { + run _render_fixtures fixtures-global-only + # Correct outcome is zero render targets (helmfile: no releases found), + # NOT a render for pseudo-cluster "." (the pre-fix behavior). + echo "$output" | grep -q "no releases found" + ! echo "$output" | grep -q "cluster: \.$" +} + +# --- discovery debug dump is opt-in (issue #66) ------------------------------- + +@test "discovery dump: absent by default" { + root="$(_root)" + run env ATLAS_FILTER_CLUSTER=cluster1 ATLAS_FILTER_DEPLOYMENT_NAME=deployment1 \ + helmfile build -f "$root/tests/helmfile.yaml.gotmpl" --debug + ! echo "$output" | grep -q "All recognized deployments" +} + +@test "discovery dump: present with ATLAS_DEBUG_DISCOVERY" { + root="$(_root)" + run env ATLAS_DEBUG_DISCOVERY=1 ATLAS_FILTER_CLUSTER=cluster1 ATLAS_FILTER_DEPLOYMENT_NAME=deployment1 \ + helmfile build -f "$root/tests/helmfile.yaml.gotmpl" --debug + echo "$output" | grep -q "All recognized deployments" +} + +# --- instance-level patch file missing (issue #63, negative side) ------------- + +@test "missing instance patch file: render fails with anchored path" { + root="$(_root)" + ATLAS_FILTER_CLUSTER=cluster1 ATLAS_FILTER_DEPLOYMENT_NAME=missing-instance-patch \ + run helmfile -f "$root/tests/helmfile.yaml.gotmpl" \ + --state-values-set "atlas.deploymentDefinitions=fixtures-negative" \ + template --skip-schema-validation + [ "$status" -ne 0 ] + echo "$output" | grep -q "strategicMergePatches (instance-level): file not found" + echo "$output" | grep -q "missing-instance-patch/./does-not-exist.yaml" +} diff --git a/tests/fixtures-dup-leaf/group-a/dupe/apps/app1/deployment.yaml b/tests/fixtures-dup-leaf/group-a/dupe/apps/app1/deployment.yaml new file mode 100644 index 0000000..fc7b1c6 --- /dev/null +++ b/tests/fixtures-dup-leaf/group-a/dupe/apps/app1/deployment.yaml @@ -0,0 +1,5 @@ +# Fixture (issue #64): leaf cluster name "dupe" also exists in group-b — +# discovery must fail listing both paths. +apps: + - template: app-novals + namespace: test diff --git a/tests/fixtures-dup-leaf/group-b/dupe/apps/app1/deployment.yaml b/tests/fixtures-dup-leaf/group-b/dupe/apps/app1/deployment.yaml new file mode 100644 index 0000000..b638acc --- /dev/null +++ b/tests/fixtures-dup-leaf/group-b/dupe/apps/app1/deployment.yaml @@ -0,0 +1,5 @@ +# Fixture (issue #64): leaf cluster name "dupe" also exists in group-a — +# discovery must fail listing both paths. +apps: + - template: app-novals + namespace: test diff --git a/tests/fixtures-global-only/apps/global-app/deployment.yaml b/tests/fixtures-global-only/apps/global-app/deployment.yaml new file mode 100644 index 0000000..aa1a54c --- /dev/null +++ b/tests/fixtures-global-only/apps/global-app/deployment.yaml @@ -0,0 +1,7 @@ +# Fixture for the zero-cluster repo shape (issue #65): global deployments +# exist but no cluster directories do. Discovery must not register the +# global apps/ dir itself as pseudo-cluster "." — the correct result is +# zero render targets, not a nonsense one. +apps: + - template: app-novals + namespace: test