From 7c1a8b9a68262347e49137aab61ca1f331d3ccbc Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Wed, 10 Jun 2026 18:41:50 +0200 Subject: [PATCH 1/3] Fix | Stream branch root for local Helm charts with out-of-chart value files --- pkg/reposerverextract/extract.go | 69 +++++++++++- pkg/reposerverextract/extract_test.go | 151 ++++++++++++++++++++++++++ 2 files changed, 218 insertions(+), 2 deletions(-) diff --git a/pkg/reposerverextract/extract.go b/pkg/reposerverextract/extract.go index 9859ade3..b8c1de47 100644 --- a/pkg/reposerverextract/extract.go +++ b/pkg/reposerverextract/extract.go @@ -17,6 +17,7 @@ import ( "encoding/json" "fmt" "io" + "net/url" "os" "path/filepath" "strings" @@ -572,7 +573,10 @@ func buildManifestRequestForSource( // files to the repo server, which can otherwise fail Argo CD's symlink safety // checks before rendering even starts. If a path also contains a kustomization // file, keep the branch root because Argo CD discovers it as Kustomize even - // when Chart.yaml exists for kustomize helmCharts support. + // when Chart.yaml exists for kustomize helmCharts support. We also keep the + // branch root when the chart's helm.valueFiles reference files outside the + // chart directory, because those files would otherwise be missing from the + // narrowed stream (see buildStreamDirForLocalSource). // // Special case: if the primary source has a Chart field (external Helm // registry chart) there are no local files to stream. We signal this by @@ -726,13 +730,74 @@ func buildStreamDirForLocalSource(branchFolder string, source v1alpha1.Applicati if _, err := os.Stat(sourceDir); err != nil { return "", nil, fmt.Errorf("failed to inspect source dir %q: %w", sourceDir, err) } - if isLocalHelmChart(sourceDir) && !isKustomizeSource(sourceDir) { + if isLocalHelmChart(sourceDir) && !isKustomizeSource(sourceDir) && !hasOutOfChartValueFile(branchFolder, sourceDir, source) { return sourceDir, nil, nil } return branchFolder, nil, nil } +// hasOutOfChartValueFile reports whether any of the source's Helm valueFiles +// resolves to a path outside the chart directory (sourceDir). Argo CD resolves +// a leading-slash value file relative to the repository root and a relative +// value file relative to the chart (app) directory, so a value file can escape +// the chart directory either via an absolute "/path" or a relative "../path". +// +// When a value file lives outside the chart directory, narrowing the repo +// server stream to only the chart directory would drop that file from the +// tarball and the render would fail with "no such file or directory". In that +// case the caller must keep streaming the whole branch folder so the +// out-of-chart value files remain reachable. URL value files (e.g. https://…) +// are remote and never force the branch root. +func hasOutOfChartValueFile(branchFolder, sourceDir string, source v1alpha1.ApplicationSource) bool { + if source.Helm == nil { + return false + } + + absSourceDir, err := filepath.Abs(sourceDir) + if err != nil { + // If we cannot reason about the path, fall back to the safe behaviour + // of streaming the whole branch folder. + return true + } + chartRoot := absSourceDir + string(os.PathSeparator) + + for _, vf := range source.Helm.ValueFiles { + if vf == "" { + continue + } + // Skip $ref value files - this function is only reached on the fast + // path where there are no ref sources, but be defensive. + if strings.HasPrefix(vf, "$") { + continue + } + // Skip remote URL value files (e.g. https://...). A URL has a scheme + // and is fetched by the repo server, not read from the tarball. + if u, parseErr := url.Parse(vf); parseErr == nil && u.Scheme != "" { + continue + } + + var resolved string + if filepath.IsAbs(vf) { + // Leading-slash value files are resolved relative to the repo root. + abs, absErr := filepath.Abs(branchFolder) + if absErr != nil { + return true + } + resolved = filepath.Join(abs, vf) + } else { + // Relative value files are resolved relative to the chart directory. + resolved = filepath.Join(absSourceDir, vf) + } + + if resolved != absSourceDir && !strings.HasPrefix(resolved+string(os.PathSeparator), chartRoot) { + return true + } + } + + return false +} + func isLocalHelmChart(sourceDir string) bool { info, err := os.Stat(filepath.Join(sourceDir, "Chart.yaml")) return err == nil && !info.IsDir() diff --git a/pkg/reposerverextract/extract_test.go b/pkg/reposerverextract/extract_test.go index 6844bcea..e15759ad 100644 --- a/pkg/reposerverextract/extract_test.go +++ b/pkg/reposerverextract/extract_test.go @@ -980,6 +980,157 @@ spec: assertDefaultProjectFields(t, req) } +// REGRESSION (v0.2.9 / PR #443): a local Helm chart whose helm.valueFiles +// reference files OUTSIDE the chart directory via a leading-slash +// repo-root-absolute path (e.g. "/clusters//config.yaml"). Argo CD +// resolves leading-slash value files relative to the repo root (the stream +// root), so narrowing the stream to only the chart directory drops the value +// file from the tarball and the repo server fails with: +// +// helm template ... --values /tmp//clusters/.../config.yaml: no such file or directory +// +// Real-world example: egmontadministration/argo-management-cluster +// hotel-tenant-external-secrets and hotel-tenant-namespace-peerings +// ApplicationSets, where source.path is the chart dir but valueFiles point at +// sibling clusters//... config files. +// +// Fix: keep streaming the branch root (with Path set) whenever any value file +// escapes the chart directory, so the out-of-chart files remain reachable. +func TestBuildManifestRequest_SingleSource_LocalChart_OutOfChartAbsoluteValueFile_StreamsBranchRoot(t *testing.T) { + branchFolder := makeBranchFolder(t, "management-prod/hotel/per-tenant-installed/charts/external-secrets") + // The value file lives in a sibling directory tree, outside the chart dir. + valuesDir := filepath.Join(branchFolder, "management-prod", "hotel", "clusters", "eks-hotel-a-nonprod", "companies", "egmont-it", "daip") + require.NoError(t, os.MkdirAll(valuesDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(valuesDir, "config.yaml"), []byte("tenant:\n slug: egmont-it-daip\n"), 0o644)) + + app := makeApp(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: eks-hotel-a-nonprod-eso +spec: + destination: + namespace: external-secrets + source: + repoURL: https://github.com/org/repo.git + path: management-prod/hotel/per-tenant-installed/charts/external-secrets + targetRevision: HEAD + helm: + valueFiles: + - /management-prod/hotel/clusters/eks-hotel-a-nonprod/companies/egmont-it/daip/config.yaml +`) + + contentSources, refSources, hasMultipleSources, err := splitSources(app) + require.NoError(t, err) + require.Len(t, contentSources, 1) + require.Empty(t, refSources) + + req, streamDir, cleanup, err := buildManifestRequestForSource(app, contentSources[0], refSources, hasMultipleSources, branchFolder, nil, + manifestRequestRenderContext{repoSelector: testRepoSelector(t, "https://github.com/org/repo.git")}) + require.NoError(t, err) + if cleanup != nil { + defer cleanup() + } + + // Must stream the branch root (with Path set) so the out-of-chart value + // file resolves correctly relative to the repo root. + assert.Equal(t, branchFolder, streamDir, + "REGRESSION: local Helm chart with an out-of-chart value file must stream the branch root so the value file is reachable") + assert.Equal(t, "management-prod/hotel/per-tenant-installed/charts/external-secrets", req.ApplicationSource.Path) + + // The streamed tarball must contain the out-of-chart value file. + tarEntries := compressAndListEntries(t, streamDir) + assert.Contains(t, tarEntries, "management-prod/hotel/clusters/eks-hotel-a-nonprod/companies/egmont-it/daip/config.yaml", + "the out-of-chart value file must be present in the streamed tarball") + assertDefaultProjectFields(t, req) +} + +// REGRESSION (v0.2.9 / PR #443): same class of bug as the absolute case, but +// the value file escapes the chart directory via a relative "../" path. The +// fix must also keep the branch root in this case. +func TestBuildManifestRequest_SingleSource_LocalChart_OutOfChartRelativeValueFile_StreamsBranchRoot(t *testing.T) { + branchFolder := makeBranchFolder(t, "apps/my-chart") + // Value file is a sibling of the chart dir: apps/shared/values.yaml. + sharedDir := filepath.Join(branchFolder, "apps", "shared") + require.NoError(t, os.MkdirAll(sharedDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(sharedDir, "values.yaml"), []byte("key: value\n"), 0o644)) + + app := makeApp(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: my-chart +spec: + destination: + namespace: production + source: + repoURL: https://github.com/org/repo.git + path: apps/my-chart + targetRevision: HEAD + helm: + valueFiles: + - ../shared/values.yaml +`) + + contentSources, refSources, hasMultipleSources, err := splitSources(app) + require.NoError(t, err) + require.Len(t, contentSources, 1) + require.Empty(t, refSources) + + req, streamDir, cleanup, err := buildManifestRequestForSource(app, contentSources[0], refSources, hasMultipleSources, branchFolder, nil, + manifestRequestRenderContext{repoSelector: testRepoSelector(t, "https://github.com/org/repo.git")}) + require.NoError(t, err) + if cleanup != nil { + defer cleanup() + } + + assert.Equal(t, branchFolder, streamDir, + "REGRESSION: local Helm chart with a ../ value file must stream the branch root so the value file is reachable") + assert.Equal(t, "apps/my-chart", req.ApplicationSource.Path) + assertDefaultProjectFields(t, req) +} + +// A local Helm chart whose value files all stay INSIDE the chart directory +// must keep the optimised behaviour: stream only the chart dir and clear Path. +func TestBuildManifestRequest_SingleSource_LocalChart_InChartValueFile_StreamsChartDir(t *testing.T) { + branchFolder := makeBranchFolder(t, "apps/my-chart") + + app := makeApp(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: my-chart +spec: + destination: + namespace: production + source: + repoURL: https://github.com/org/repo.git + path: apps/my-chart + targetRevision: HEAD + helm: + valueFiles: + - values.yaml + - overrides/prod.yaml +`) + + contentSources, refSources, hasMultipleSources, err := splitSources(app) + require.NoError(t, err) + require.Len(t, contentSources, 1) + require.Empty(t, refSources) + + req, streamDir, cleanup, err := buildManifestRequestForSource(app, contentSources[0], refSources, hasMultipleSources, branchFolder, nil, + manifestRequestRenderContext{repoSelector: testRepoSelector(t, "https://github.com/org/repo.git")}) + require.NoError(t, err) + if cleanup != nil { + defer cleanup() + } + + assert.Equal(t, filepath.Join(branchFolder, "apps", "my-chart"), streamDir, + "local Helm chart with only in-chart value files should still stream just the chart dir") + assert.Empty(t, req.ApplicationSource.Path) + assertDefaultProjectFields(t, req) +} + func TestBuildManifestRequest_SingleSource_LocalChart_DoesNotStreamUnrelatedSymlinks(t *testing.T) { branchFolder := makeBranchFolder(t, "infra/charts/argocd") relatedAssetDir := filepath.Join(branchFolder, "assets", "co") From 5201b2963a280a0d9ecd06669a121ebf38f769ee Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Wed, 10 Jun 2026 18:59:30 +0200 Subject: [PATCH 2/3] Tests | Add repo-server-api integration test for out-of-chart Helm value files --- integration-test/README.md | 5 + integration-test/branch-18/target/output.html | 107 ++++++++++++++++++ integration-test/branch-18/target/output.md | 41 +++++++ integration-test/integration_test.go | 13 +++ 4 files changed, 166 insertions(+) create mode 100644 integration-test/branch-18/target/output.html create mode 100644 integration-test/branch-18/target/output.md diff --git a/integration-test/README.md b/integration-test/README.md index 12e6cdde..c1f3a4ae 100644 --- a/integration-test/README.md +++ b/integration-test/README.md @@ -183,6 +183,11 @@ Each `branch-N/target[-suffix]/` directory contains expected output files (`outp ### Branch 15: Additional Coverage - `target`: Basic diff (no special options) +### Branch 16-18: repo-server-api Render Method +- `branch-16/target`: Config Management Plugin (kustomize-build-with-helm) via `repo-server-api` +- `branch-17/target-1` / `target-2`: App-of-apps traversal via `repo-server-api` +- `branch-18/target`: Single-source local Helm chart whose `helm.valueFiles` reference a file OUTSIDE the chart directory via a repo-root-absolute path (`/examples/out-of-chart-values/env/values.yaml`). Regression test for `repo-server-api`: the tool must stream the whole branch folder so the out-of-chart value file is reachable, otherwise the render fails with `no such file or directory`. + ## Prerequisites - Docker running diff --git a/integration-test/branch-18/target/output.html b/integration-test/branch-18/target/output.html new file mode 100644 index 00000000..66ee3402 --- /dev/null +++ b/integration-test/branch-18/target/output.html @@ -0,0 +1,107 @@ + + + + + + +
+

Argo CD Diff Preview

+ +

Summary:

+
Modified (1):
+± out-of-chart-values (+2|-2)
+ +
+
+ +out-of-chart-values (examples/out-of-chart-values/application.yaml) + + +

Deployment: default/out-of-chart-values

+
+ + + + + + + + + + + + + + + + + + + + + + + + +
 apiVersion: apps/v1
 kind: Deployment
 metadata:
   name: out-of-chart-values
   namespace: default
 spec:
-  replicas: 2
+  replicas: 3
   selector:
     matchLabels:
       app: out-of-chart-values
   template:
     metadata:
       labels:
         app: out-of-chart-values
     spec:
       containers:
-      - image: nginx:1.25
+      - image: nginx:1.27
         name: app
         ports:
         - containerPort: 80
+
+ +
+
+ +
_Stats_:
+[Applications: 2], [Full Run: Xs], [Rendering: Xs], [Cluster: Xs], [Argo CD: Xs]
+
+ + diff --git a/integration-test/branch-18/target/output.md b/integration-test/branch-18/target/output.md new file mode 100644 index 00000000..cc046f4c --- /dev/null +++ b/integration-test/branch-18/target/output.md @@ -0,0 +1,41 @@ +## Argo CD Diff Preview + +Summary: +```yaml +Modified (1): +± out-of-chart-values (+2|-2) +``` + +
+out-of-chart-values (examples/out-of-chart-values/application.yaml) +
+ +#### Deployment: default/out-of-chart-values +```diff + apiVersion: apps/v1 + kind: Deployment + metadata: + name: out-of-chart-values + namespace: default + spec: +- replicas: 2 ++ replicas: 3 + selector: + matchLabels: + app: out-of-chart-values + template: + metadata: + labels: + app: out-of-chart-values + spec: + containers: +- - image: nginx:1.25 ++ - image: nginx:1.27 + name: app + ports: + - containerPort: 80 +``` +
+ +_Stats_: +[Applications: 2], [Full Run: Xs], [Rendering: Xs], [Cluster: Xs], [Argo CD: Xs] diff --git a/integration-test/integration_test.go b/integration-test/integration_test.go index 06c5e84b..e6ec55b3 100644 --- a/integration-test/integration_test.go +++ b/integration-test/integration_test.go @@ -326,6 +326,19 @@ var testCases = []TestCase{ FileRegex: "examples/app-of-apps/.*", TraverseAppOfApps: "true", }, + // Regression test for a single-source local Helm chart whose helm.valueFiles + // reference a file OUTSIDE the chart directory via a repo-root-absolute path + // (/examples/out-of-chart-values/env/values.yaml). With repo-server-api the + // tool must stream the whole branch folder so the out-of-chart value file is + // reachable; otherwise the render fails with "no such file or directory". + { + Name: "branch-18/target", + TargetBranch: "integration-test/branch-18/target", + BaseBranch: "integration-test/branch-18/base", + RenderMethod: "repo-server-api", + FileRegex: "examples/out-of-chart-values/.*", + WatchIfNoWatchPatternFound: "false", + }, // This test verifies that disabling cluster roles without using the API fails. // When createClusterRoles: false is set but --render-method=cli is used, // the tool should fail because it can't access cluster resources via CLI. From d7e13c95612b07b2cbc18fe51935fe0f2571109f Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Wed, 10 Jun 2026 19:24:48 +0200 Subject: [PATCH 3/3] Update agent file --- AGENTS.md | 82 +++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 71 insertions(+), 11 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a6ac3a8b..d5639dab 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,6 +47,77 @@ make update-integration-tests 2>&1 | tail -50 make run-integration-tests-go 2>&1 | tail -50 ``` +## How to Create an Integration Test + +This is the stuff that would have saved time when getting started. An integration +test runs the real tool against a real kind cluster + Argo CD and compares the +generated diff against a saved "golden" file. + +### How it works (the mental model) + +- Each test case clones **two real Git branches** from the upstream repo + (`dag-andersen/argocd-diff-preview`): a `base` branch and a `target` branch. + These branches are full snapshots of the whole repo. The tool diffs them. +- The test data lives in branches named `integration-test/branch-N/base` and + `integration-test/branch-N/target`. The example apps the tool renders live + under `examples/` on those branches. +- The expected output lives **in this repo** (not on the data branches), under + `integration-test/branch-N/target/{output.md,output.html}`. This is + the golden file the actual run is compared against. +- Test cases are defined as `TestCase` structs in + `integration-test/integration_test.go` (see the big `testCases` slice). + +### Steps to add a new test + +1. **Pick the next free branch number.** Run + `git ls-remote --heads origin 'refs/heads/integration-test/*'` and pick the + next `branch-N`. (Note: some numbers may exist on the remote but not in + `testCases` - check both.) + +2. **Create the two data branches from `main`, then edit the example files.** + The branches are just `main` with example files changed: + ```bash + git checkout main + git checkout -b integration-test/branch-N/base + # add/modify files under examples/ that the test should render + git commit -am "Tests | Add branch-N base: " + + git checkout -b integration-test/branch-N/target # branches off base + # change ONLY the file(s) whose diff you want to verify + git commit -am "Tests | Add branch-N target: " + + git push -u origin integration-test/branch-N/base integration-test/branch-N/target + ``` + Keep `base` and `target` nearly identical - they should differ only in the + file(s) under test, so the diff output stays small and focused. (Look at how + an existing pair like `branch-16` differs by a single file.) + +3. **Add the `TestCase`** to the `testCases` slice in `integration_test.go`. + The expected-output directory is derived from `TargetBranch` + `Suffix` + (e.g. `TargetBranch: integration-test/branch-N/target`, `Suffix: "-1"` -> + `branch-N/target-1/`). Useful fields: + - `RenderMethod`: `"cli"`, `"server-api"`, or `"repo-server-api"` (omit to use the default). + - `FileRegex` / `FilesChanged`: limit which changed files are considered. + - `WatchIfNoWatchPatternFound: "false"` + an `argocd-diff-preview/watch-pattern` + annotation on the app limits rendering to **just that app**, even though the + branch is a full repo snapshot. This is the easiest way to keep output focused. + +4. **Generate the golden output** by running the single test once with `-update`. + The first run for a new case must use `-update` (there is no golden file to + compare against yet). This needs Docker running: + ```bash + make go-build + cd integration-test + TEST_CASE="branch-N/target" go test -v -timeout 20m -run TestSingleCase -update ./... + ``` + This writes `output.md` / `output.html`. Read them and sanity-check the diff. + Timing values are auto-normalized to `Xs`, so the golden file is stable. + +5. **Verify it actually passes** in compare mode (no `-update`): + ```bash + TEST_CASE="branch-N/target" go test -v -timeout 20m -run TestSingleCase ./... + ``` + ### Commit Messages and PR Titles Use the format: ` | ` @@ -69,17 +140,6 @@ Chore | Update ArgoCD dependency to v3.0 **Do NOT add issue numbers in parentheses to commit messages or PR titles.** The `(#123)` format is what GitHub automatically adds when merging/squashing PRs (and it refers to the PR number, not the issue). -## Project Structure - -``` -argocd-diff-preview/ -├── cmd/ # CLI entry point (main.go, options.go) -├── pkg/ # Core go logic -├── integration-test/ # Integration tests and expected outputs -├── docs/ # MkDocs documentation -└── examples/ # Test fixtures -``` - ## Main Challenges when building a tool like this - A repository can contain multiple applications and applications with the same name. We ALWAYS need to make sure the names are unique.