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.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/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/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/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-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 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