From e60ba8d46560a61fa7f9e4cb37125b6d2098f03b Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Sun, 21 Jun 2026 00:34:52 +0200 Subject: [PATCH 1/3] Much more tests --- pkg/reposerverextract/SOURCE_MATRIX.md | 214 +++++++ pkg/reposerverextract/source_matrix_a_test.go | 568 ++++++++++++++++++ pkg/reposerverextract/source_matrix_b_test.go | 568 ++++++++++++++++++ .../source_matrix_cde_test.go | 486 +++++++++++++++ 4 files changed, 1836 insertions(+) create mode 100644 pkg/reposerverextract/SOURCE_MATRIX.md create mode 100644 pkg/reposerverextract/source_matrix_a_test.go create mode 100644 pkg/reposerverextract/source_matrix_b_test.go create mode 100644 pkg/reposerverextract/source_matrix_cde_test.go diff --git a/pkg/reposerverextract/SOURCE_MATRIX.md b/pkg/reposerverextract/SOURCE_MATRIX.md new file mode 100644 index 00000000..79040404 --- /dev/null +++ b/pkg/reposerverextract/SOURCE_MATRIX.md @@ -0,0 +1,214 @@ +# Repo-server-api source-combination matrix (test coverage spec) + +> Status: **IMPLEMENTED** - this map is now backed by a complete test suite. +> Every row below (A1-A15, B1-B11, C1-C3, D1-D3, E1-E3) has a named test in +> `source_matrix_a_test.go`, `source_matrix_b_test.go`, and +> `source_matrix_cde_test.go`. Integration-only rows (**I**) are present as +> skipped placeholders so the suite stays 1:1 with this table. +> +> Goal: enumerate every *valid* way an Argo CD Application can describe its +> `source` / `sources`, decide the expected behaviour of the repo-server-api +> render path for each, and mark which cases are **unit-testable** vs which +> **need a real repo server** (integration). The output is a checklist of +> combinations so we stop discovering broken-but-valid cases one GitHub issue +> at a time. +> +> Test-name convention: each row maps to `TestSourceMatrix_Table...`, keyed +> by the row id (e.g. A11 -> `TestSourceMatrix_TableA11_*`, the B-table rows are +> subtests named `B1`/`B2`/... inside `TestSourceMatrix_TableB_MultiSource_Routing`). + +## Why this exists + +7 of the last 10 issues were repo-server-api bugs (#416, #426, #432, #438, +#441, #446, #448). They are all the same shape: `buildManifestRequestForSource` +makes a **routing decision** across several independent dimensions, and we only +ever verified the exact points users happened to hit. This doc makes the whole +input space explicit. + +## How the code routes today (the thing under test) + +Entry point: `renderApp` -> `splitSources` -> `buildManifestRequestForSource` +(in `pkg/reposerverextract/extract.go`). + +There are three possible outcomes ("strategies") for each **content source**: + +| Strategy | How we call the repo server | When | +| --- | --- | --- | +| **R = Remote RPC** | `GenerateManifest` (unary). Repo server git-fetches everything itself. `RefSources` populated if there are refs. `streamDir == ""`. | Files are not available locally (remote chart, cross-repo source, external ref). | +| **S = Stream** | `GenerateManifestWithFiles` (streaming a tarball). `streamDir` = chart dir or branch root. | Same-repo local source, no refs (or refs handled by streaming). | +| **S+refs = Stream with .refs** | Stream a temp tree: content dir + `.refs//`, with `$ref/...` valueFiles rewritten to relative paths. | Same-repo local `path:` source whose same-repo `$ref` sources are also local. | + +The tests observe **four** distinct outcomes via the returned `streamDir`: +`remote` (`streamDir == ""`), `stream(chart-dir)` (narrowed, `Path` cleared), +`stream(branch-root)` (`streamDir == branchFolder`, `Path` kept), and +`stream(.refs)` (temp tree containing a `.refs/` dir). See `streamStrategy` / +`classifyStrategy` in `source_matrix_a_test.go`. + +`splitSources` first splits the source list into: +- **content sources** - produce manifests (`path != "" || chart != "" || ref == ""`). +- **ref-only sources** - exist solely to provide `$ref` value files (`ref != ""`). + (A source with BOTH `ref` and `path` counts as content AND ref - GH#401.) + +## The dimensions of the input space + +A single Application is the product of these axes. Not every combination is +valid; invalid ones are noted. + +1. **Cardinality** - `source` (single) vs `sources` (multi). `ApplicationSet` + reads from `spec.template.spec` but is otherwise identical. +2. **Primary source type** - what the content source is: + - `path:` local **directory** of plain manifests / jsonnet (`directory:`) + - `path:` local **Helm chart** (dir has `Chart.yaml`) + - `path:` local **Kustomize** (dir has `kustomization.yaml`) + - `path:` + **plugin** (CMP) + - `chart:` **remote Helm chart** (from a Helm/OCI registry; no local files) +3. **Repo locality of the primary** - does `repoURL` match `--repo`/`--repo-regex`? + - **local** (matches -> files are in the branch checkout) + - **cross-repo** (does not match -> files are NOT local) +4. **Ref sources present?** - none / one-or-more `$ref` sources. +5. **Ref locality** - each ref's `repoURL`: local (same repo) vs external. +6. **valueFiles shape** (Helm only) - where the values live: + - none + - in-chart relative (`values.yaml`, `overrides/x.yaml`) + - out-of-chart relative (`../shared/x.yaml`) + - out-of-chart absolute (`/env/x.yaml`, resolved from repo root) + - `$ref/...` (points into a ref source) + - remote URL (`https://.../values.yaml`) +7. **Filesystem quirks in the streamed tree** - none / in-bounds symlink / + out-of-bounds symlink / **directory** symlink. +8. **Revision redirection** - top-level app vs app-of-apps **child**, with / + without `--redirect-target-revisions` (#446). + +Axes 6-8 only bite once a case is already on a **Stream** strategy, which is +why they produced separate bugs. + +## Legend + +- Expected strategy: **R** / **S** / **S+refs** (see table above). +- Testability: + - **U** = unit-testable today against `buildManifestRequestForSource` + (assert chosen strategy + request fields + which files end up in the + tarball). No repo server needed. + - **I** = needs a real repo server (the failure happens *inside* it) -> + integration test (branch-N mechanism). +- Status: ✅ covered by a matrix test (passing) · ⏭️ integration-only, present + as a skipped placeholder (real coverage belongs in branch-N) · 🟥 known-broken + (open issue) · ⬜ no test yet (gap) · ⚠️ partial. + +--- + +## A. Single source (`spec.source`) + +| # | Primary type | Repo | valueFiles | Expected | Test | Status | Notes / test | +| --- | --- | --- | --- | --- | --- | --- | --- | +| A1 | local dir (manifests/jsonnet) | local | - | S (branch root, Path set) | U | ✅ | `TableA` row A1. Plain non-chart dir falls through to branch root. | +| A2 | local Helm chart | local | none | S (chart dir, Path cleared) | U | ✅ | `TableA` row A2 (also `..._SingleSource_LocalChart`). | +| A3 | local Helm chart | local | in-chart | S (chart dir) | U | ✅ | `TableA` row A3 (also `..._LocalChart_InChartValueFile_StreamsChartDir`). | +| A4 | local Helm chart | local | out-of-chart relative `../` | S (branch root) | U | ✅ | `TableA` row A4 (also `..._LocalChart_OutOfChartRelativeValueFile`). | +| A5 | local Helm chart | local | out-of-chart absolute `/` | S (branch root) | U | ✅ | `TableA` row A5 (the v0.2.10 fix, #444). | +| A6 | local Helm chart | local | remote URL | S (chart dir; URL ignored) | U | ✅ | `TableA` row A6 - asserts URL valueFiles do not force the branch root. | +| A7 | local Kustomize | local | - | S (branch root, Path set) | U | ✅ | `TableA` row A7 (also `..._SingleSource_Kustomize_StreamsBranchRoot`). | +| A8 | local Kustomize w/ `helmCharts` | local | - | S (branch root) | U | ✅ | `TableA` row A8 (also `..._SingleSource_KustomizeWithHelm_StreamsBranchRoot`). | +| A9 | local chart, in-bounds symlink in tree | local | any | S, render OK | U+I | ✅/⏭️ | `TableA9_InBoundsSymlink_RoutesAndExcludesUnrelated` asserts the tarball excludes unrelated symlinks (U). The repo server's symlink gate (#438) is the I half - integration. | +| A10 | local chart, **out-of-bounds** file symlink | local | any | render fails today; **#438** | I | ⏭️ | `TableA10_OutOfBoundsSymlink_Integration` (skipped placeholder). Repo-server rejects "out-of-bounds symlinks"; only observable in a real render. | +| A11 | local chart, **directory** symlink inside chart | local | any | render OK (**#448** fixed by #449) | U+I | ✅ | `TableA11_DirectorySymlinkInChart_RoutesSuccessfully` - routing no longer errors (EISDIR fixed). `copyDir` dir-symlink following also covered by `TestCopyDir_FollowsDirectorySymlink`. | +| A12 | local plugin (CMP) `path:` | local | - | S (branch root, Path set) | U | ✅ | `TableA` row A12. Plugin sources have no Chart.yaml/kustomization -> branch root. (branch-16 covers CMP end-to-end.) | +| A13 | remote Helm `chart:` | external | none | R (`streamDir==""`) | U | ✅ | `TableA` row A13 (also `..._SingleSource_ExternalChart_NoRefs`). | +| A14 | remote Helm `chart:` (OCI) | external | none | R | U | ✅ | `TableA` row A14 - `oci://` registry chart, same routing as A13. | +| A15 | local `path:` source | **cross-repo** | - | R (`streamDir==""`) | U | ✅ | `TableA` row A15. Path is **kept** (not cleared) - the repo server resolves it inside the fetched foreign repo. | + +## B. Multi-source (`spec.sources`) + +| # | Primary type | Primary repo | Ref(s) | valueFiles | Expected | Test | Status | Notes | +| --- | --- | --- | --- | --- | --- | --- | --- | --- | +| B1 | local Helm chart | local | one local `$ref` | `$ref/...` | S+refs (rewrite paths) | U | ✅ | `TableB` subtest B1 (also `..._MultiSource_LocalChart_WithRef_RewritesValueFiles`). | +| B2 | local Helm chart | local | local `$ref` w/ both ref+path | `$ref/...` | S+refs | U | ✅ | `TableB` subtest B2. The ref+path source, routed as content, still streams `.refs` (it sees itself in the ref list). | +| B3 | local Helm chart | local | one **external** `$ref` | `$ref/...` | R + RefSources | U | ✅ | `TableB` subtest B3 (also `..._SameRepoPrimaryWithExternalRef_UsesRemoteRPC`, #426). | +| B4 | local `path:` (non-chart) | local | local `$ref` | n/a | S+refs | U | ✅ | `TableB` subtest B4 - plain manifests primary + local ref still synthesizes a `.refs` tree. | +| B5 | remote Helm `chart:` | external | one local `$ref` | `$ref/...` | S+refs via puller (or R+RefSources without one) | U+I | ✅ | **#441 fixed by #442.** `TableB` subtests B5 (with `fakeChartPuller` -> pulls chart, streams `.refs`, clears Chart) and B5b (no puller -> R + RefSources). Real registry pull is the I half. | +| B6 | remote Helm `chart:` | external | one external `$ref` | `$ref/...` | R + RefSources | U | ✅ | `TableB` subtest B6 (also `..._ExternalChart_WithRef_UsesRemoteRPC`). External ref forces R even with a puller present. | +| B7 | remote Helm `chart:` | external | local `$ref` w/ both ref+path | `$ref/...` | R + RefSources | U | ✅ | `TableB` subtest B7 (also `..._ApplicationSet_ExternalChart_WithRef`). | +| B8 | local Helm chart | local | local `$ref`, value file is out-of-chart abs `/` | mixed | S+refs (must also include out-of-chart file) | U | ✅ | `TableB` subtest B8 - the A5 (out-of-chart abs) + refs interaction; `$ref` rewritten, out-of-chart abs file reachable. | +| B9 | two local content sources (no ref) | local | - | per-source | S each (one request per content source) | U | ✅ | `TableB` subtest B9 (also `..._MultipleContentSources_BuildsOneRequestEach`). | +| B10 | local primary + external primary (mixed content) | mixed | - | - | per-source: S for local, R for external | U | ✅ | `TableB` subtest B10 - each content source routed independently. | +| B11 | ref-only source with **no path** (points at repo root) | local | local `$ref` (path="") | `$ref/x` | S+refs (ref dir = branch root) | U | ✅ | `TableB` subtest B11 - direct assertion that `$ref` with no path resolves against the repo root. | + +## C. Cardinality / kind variants + +| # | Case | Expected | Test | Status | Notes | +| --- | --- | --- | --- | --- | --- | +| C1 | `ApplicationSet` (sources under `spec.template.spec`) | same routing as the equivalent Application | U | ✅ | `TableC1_ApplicationSet_RoutesLikeApplication` - builds the same logical app as both kinds and asserts identical strategy + RefSources keys. | +| C2 | app with neither `source` nor `sources` | error | U | ✅ | `TableC_DegenerateTopologies_Error` subtest C2 (also `..._NoSource_ReturnsError`). | +| C3 | all sources are ref-only (no content) | error | U | ✅ | `TableC_DegenerateTopologies_Error` subtests C3 (all ref-only) and C3b (empty `sources: []`) - direct assertions. | + +## D. App-of-apps / traversal (orthogonal to the source matrix) + +| # | Case | Expected | Test | Status | Notes | +| --- | --- | --- | --- | --- | --- | +| D1 | child app source = local | child rendered like a normal app | U | ✅ | `TableD1_ChildApplication_RoutesLikeSeed` - a discovered child routes through `buildManifestRequestForSource` like a seed (local chart -> S chart dir, remote chart -> R). | +| D2 | child app, revision **not** in `--redirect-target-revisions` | child left on its pinned revision | U | ✅ | **#446 fixed by #447.** `TableD2_ChildHonorsRedirectTargetRevisions` (in-list -> redirected, out-of-list -> untouched, empty -> redirect all). Also `appofapps_test.go` `BuildChildApplication_*`. | +| D3 | child app source pinned to a ref that only exists remotely | render from remote, not redirected | I | ⏭️ | `TableD3_ChildRemoteOnlyRef_Integration` (skipped placeholder). Consequence of D2; the remote-only fetch is only observable in a real render. | + +## E. Cross-cutting request-content correctness (not routing, but repo-server-api specific) + +| # | Case | Expected | Test | Status | Notes | +| --- | --- | --- | --- | --- | --- | +| E1 | `ManifestRequest.ApiVersions` / `KubeVersion` populated | both set from the cluster | U | ✅ | **#432 (fixed v0.2.9).** `TableE_RequestInvariants_AllStrategies` asserts both on all three strategies (R, S chart-dir, S+refs); every A/B row asserts them too. | +| E2 | `ProjectName=default`, `ProjectSourceRepos=["*"]` on every request | permissive, so helm-build errors aren't masked as permission errors | U | ✅ | **#416.** `TableE_RequestInvariants_AllStrategies` asserts `assertDefaultProjectFields` on R / S / S+refs; every A/B row asserts it too. | +| E3 | transient helm-dep build error surfaced (not swallowed) | original error preserved | I | ⏭️ | `TableE3_HelmDepBuildErrorSurfaced_Integration` (skipped placeholder). **#416** - the surfaced message is only observable against a real repo server (E2 is the unit-level precondition). | + +--- + +## What we found (honest summary) + +The suite is **complete and 1:1 with this table**: 35 rows, each with a named +test. 9 passing test functions cover the unit rows; 3 skipped placeholders mark +the integration-only rows (A10, D3, E3). + +**New product bugs found proactively: 0.** Every unit row passed against `main`, +or was an already-tracked issue. The routing code is currently in good shape. + +What the exercise actually bought us: + +1. **Regression net for recently-merged fixes.** Several fixes had no dedicated + regression test before; the matrix now pins them so they cannot silently + regress: #444 (A5 out-of-chart abs valueFiles), #442 (B5 remote chart + + same-repo ref), #449 (A11 directory symlink), #447 (D2/D3 children honor + `--redirect-target-revisions`). +2. **Closed real coverage gaps** the draft flagged as ⬜/⚠️ (untested-but-valid, + "where the next bug hides"): A1, A6, A12, A14, B4, B8, B10, B11, C1, and the + E2 invariant across **all three** strategies (the #416 root cause was that it + was only asserted on the streamed local-chart path). +3. **Flushed out four wrong assumptions** (caught as failing tests, then fixed): + - A15: cross-repo source keeps `Path` (the repo server resolves it in the + fetched foreign repo); it is **not** cleared. + - B2: a `ref`+`path` source, routed as content, still streams `.refs` (it + sees itself in the ref list) rather than degrading to a chart-dir stream. + - B5/B8: a `$ref` source with no `path` points at the **repo root**, so + `$cfg/values.yaml` resolves to `/values.yaml`. + - A11: on the single-source path the chart streams via the tarball compressor, + which preserves a directory symlink **as a symlink entry**; the + `copyDir`-follows-dir-symlink behavior (#449) is on the refs/slow path. + +## Where the next real bug most likely hides (integration) + +Unit tests structurally cannot see failures that happen **inside** the repo +server. The skipped placeholders are precisely those cases, and they are the +highest-value targets for proactive bug-hunting via branch-N integration: + +- **A10 (#438)** - out-of-bounds file symlink rejected by the repo server's + symlink safety gate. Needs an out-of-bounds symlink fixture. +- **B5 (#441/#442)** - the *real* registry pull + same-repo ref render (the unit + test stubs the puller). +- **D3 (#446)** - a child pinned to a remote-only ref rendered by a real server. +- **E3 (#416)** - a transient helm-dependency build error surfaced with its + original message rather than masked. + +## Test file layout + +- `source_matrix_a_test.go` - Table A (single source) + the A9/A10/A11 symlink + rows. Hosts the shared helpers (`streamStrategy`, `classifyStrategy`, + `writeBranchFiles`, `replaceRepo`). +- `source_matrix_b_test.go` - Table B (multi-source), one big table-driven test + with per-row subtests B1-B11 (+ B5b). +- `source_matrix_cde_test.go` - Tables C, D, E. diff --git a/pkg/reposerverextract/source_matrix_a_test.go b/pkg/reposerverextract/source_matrix_a_test.go new file mode 100644 index 00000000..d77f0639 --- /dev/null +++ b/pkg/reposerverextract/source_matrix_a_test.go @@ -0,0 +1,568 @@ +package reposerverextract + +// Source-combination matrix - Table A (single-source `spec.source`). +// +// This is the table-driven companion to SOURCE_MATRIX.md. Each row below is one +// valid way a single-source Application can describe its source. The test feeds +// the Application through buildManifestRequestForSource and asserts the ROUTING +// OUTCOME only: +// +// - strategy: which RPC the caller will use (remote vs streamed tarball), +// expressed via streamDir ("" => remote GenerateManifest, non-"" => stream). +// - streamRoot: for streamed sources, whether we stream just the chart dir or +// the whole branch root (and therefore whether Path is cleared or kept). +// - request fields that are repo-server-api specific and have caused bugs +// (Chart, KubeVersion/ApiVersions, ProjectName/ProjectSourceRepos). +// +// We deliberately assert OUTCOME, not call order or internal helpers, so this +// table stays valid if the routing is ever simplified (e.g. "always stream the +// branch root"). See SOURCE_MATRIX.md for the issue cross-references. +// +// Rows A9/A10/A11 (symlink behavior) are split out into their own tests at the +// bottom of this file (TestSourceMatrix_TableA9/A10/A11_*) because they need +// on-disk symlinks and tarball/copyDir-level assertions rather than the plain +// routing table. A10 is integration-only (skipped placeholder); A9 and A11 are +// unit-testable. Every A-row therefore has a named test in this file. + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// streamStrategy is the observable routing outcome for a content source. +type streamStrategy int + +const ( + // strategyRemote: streamDir == "" -> caller uses the unary GenerateManifest + // RPC and the repo server fetches the content itself. + strategyRemote streamStrategy = iota + // strategyStreamChartDir: stream only the chart directory; request Path is + // cleared because the chart dir is the tarball root. + strategyStreamChartDir + // strategyStreamBranchRoot: stream the whole branch folder; request Path is + // kept so the repo server resolves it relative to the repo root. + strategyStreamBranchRoot + // strategyStreamRefs: stream a synthesized temp tree that holds the content + // source plus a .refs// directory per ref source, with $ref/... + // value files rewritten to relative paths. streamDir is a temp dir that is + // neither "" nor the branch folder and contains a .refs/ subdirectory. + strategyStreamRefs +) + +func (s streamStrategy) String() string { + switch s { + case strategyRemote: + return "remote" + case strategyStreamChartDir: + return "stream(chart-dir)" + case strategyStreamBranchRoot: + return "stream(branch-root)" + case strategyStreamRefs: + return "stream(.refs)" + default: + return "unknown" + } +} + +// singleSourceCase is one row of Table A: a single-source Application plus the +// on-disk files it needs and the routing outcome we expect. +type singleSourceCase struct { + name string // matrix id + description, e.g. "A2 local chart, no values" + + // appYAML is the Application manifest under test. Use {{REPO}} as a + // placeholder for the PR repo URL so cross-repo rows are explicit. + appYAML string + + // files to create under branchFolder before routing (relative paths -> + // contents). The chart/kustomize marker files live here. + files map[string]string + + // prRepo is what --repo resolves to. Defaults to the same repo the source + // uses; set to a different value to model a cross-repo source (A15). + prRepo string + + want streamStrategy + wantChart string // expected request.ApplicationSource.Chart + wantPath string // expected request.ApplicationSource.Path ("" => cleared) + wantNoRefs bool // RefSources must be nil for single-source rows +} + +func TestSourceMatrix_TableA_SingleSource_Routing(t *testing.T) { + const repo = "https://github.com/org/repo.git" + + cases := []singleSourceCase{ + { + name: "A1 plain local dir (manifests) -> stream branch root", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a1} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: apps/plain + targetRevision: HEAD +`, + files: map[string]string{"apps/plain/deployment.yaml": "kind: Deployment\n"}, + want: strategyStreamBranchRoot, + wantPath: "apps/plain", + wantNoRefs: true, + }, + { + name: "A2 local Helm chart, no values -> stream chart dir", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a2} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: charts/app + targetRevision: HEAD +`, + files: map[string]string{"charts/app/Chart.yaml": "apiVersion: v2\nname: app\nversion: 0.1.0\n"}, + want: strategyStreamChartDir, + wantPath: "", + wantNoRefs: true, + }, + { + name: "A3 local Helm chart, in-chart values -> stream chart dir", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a3} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: charts/app + targetRevision: HEAD + helm: + valueFiles: + - values.yaml + - overrides/prod.yaml +`, + files: map[string]string{ + "charts/app/Chart.yaml": "apiVersion: v2\nname: app\nversion: 0.1.0\n", + "charts/app/values.yaml": "k: v\n", + "charts/app/overrides/prod.yaml": "k: v\n", + }, + want: strategyStreamChartDir, + wantPath: "", + wantNoRefs: true, + }, + { + name: "A4 local Helm chart, out-of-chart relative values -> stream branch root", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a4} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: charts/app + targetRevision: HEAD + helm: + valueFiles: + - ../shared/values.yaml +`, + files: map[string]string{ + "charts/app/Chart.yaml": "apiVersion: v2\nname: app\nversion: 0.1.0\n", + "charts/shared/values.yaml": "k: v\n", + }, + want: strategyStreamBranchRoot, + wantPath: "charts/app", + wantNoRefs: true, + }, + { + name: "A5 local Helm chart, out-of-chart absolute values -> stream branch root", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a5} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: charts/app + targetRevision: HEAD + helm: + valueFiles: + - /env/prod/values.yaml +`, + files: map[string]string{ + "charts/app/Chart.yaml": "apiVersion: v2\nname: app\nversion: 0.1.0\n", + "env/prod/values.yaml": "k: v\n", + }, + want: strategyStreamBranchRoot, + wantPath: "charts/app", + wantNoRefs: true, + }, + { + name: "A6 local Helm chart, remote-URL values -> stream chart dir (URL ignored)", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a6} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: charts/app + targetRevision: HEAD + helm: + valueFiles: + - https://example.com/values.yaml +`, + files: map[string]string{"charts/app/Chart.yaml": "apiVersion: v2\nname: app\nversion: 0.1.0\n"}, + want: strategyStreamChartDir, + wantPath: "", + wantNoRefs: true, + }, + { + name: "A7 local Kustomize -> stream branch root", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a7} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: overlays/prod + targetRevision: HEAD +`, + files: map[string]string{"overlays/prod/kustomization.yaml": "resources: []\n"}, + want: strategyStreamBranchRoot, + wantPath: "overlays/prod", + wantNoRefs: true, + }, + { + name: "A8 local Kustomize with helmCharts (Chart.yaml present) -> stream branch root", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a8} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: overlays/prod + targetRevision: HEAD +`, + files: map[string]string{ + "overlays/prod/kustomization.yaml": "helmCharts:\n - name: x\n", + "overlays/prod/Chart.yaml": "apiVersion: v2\nname: x\nversion: 0.1.0\n", + }, + want: strategyStreamBranchRoot, + wantPath: "overlays/prod", + wantNoRefs: true, + }, + { + name: "A12 local plugin (CMP) path -> stream branch root", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a12} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: apps/plugin + targetRevision: HEAD + plugin: + name: my-plugin +`, + files: map[string]string{"apps/plugin/manifest.yaml": "kind: ConfigMap\n"}, + want: strategyStreamBranchRoot, + wantPath: "apps/plugin", + wantNoRefs: true, + }, + { + name: "A13 remote Helm chart -> remote RPC", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a13} +spec: + destination: {namespace: cert-manager} + source: + repoURL: https://charts.jetstack.io + chart: cert-manager + targetRevision: v1.14.5 +`, + files: nil, + want: strategyRemote, + wantChart: "cert-manager", + wantNoRefs: true, + }, + { + name: "A14 remote Helm chart (OCI) -> remote RPC", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a14} +spec: + destination: {namespace: default} + source: + repoURL: oci://registry-1.docker.io/bitnamicharts + chart: nginx + targetRevision: "15.0.0" +`, + files: nil, + want: strategyRemote, + wantChart: "nginx", + wantNoRefs: true, + }, + { + name: "A15 local path source but cross-repo -> remote RPC", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a15} +spec: + destination: {namespace: default} + source: + repoURL: https://github.com/other/foreign.git + path: charts/app + targetRevision: HEAD +`, + // Files exist locally, but the source repo != --repo, so they must + // NOT be streamed; the repo server fetches the foreign repo itself. + // Path is kept (not cleared) because the repo server resolves it + // inside the fetched foreign repo - only the stream-chart-dir + // strategy clears Path. + files: map[string]string{"charts/app/Chart.yaml": "apiVersion: v2\nname: app\nversion: 0.1.0\n"}, + prRepo: repo, + want: strategyRemote, + wantPath: "charts/app", + wantNoRefs: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + branchFolder := writeBranchFiles(t, tc.files) + + prRepo := tc.prRepo + if prRepo == "" { + prRepo = repo + } + app := makeApp(t, replaceRepo(tc.appYAML, repo)) + + contentSources, refSources, hasMultipleSources, err := splitSources(app) + require.NoError(t, err) + require.Len(t, contentSources, 1, "table A rows are single content source") + + req, streamDir, cleanup, err := buildManifestRequestForSource( + app, contentSources[0], refSources, hasMultipleSources, branchFolder, nil, + manifestRequestRenderContext{ + repoSelector: testRepoSelector(t, prRepo), + kubeVersion: "v1.30.1", + apiVersions: []string{"apps/v1", "v1"}, + }, + ) + require.NoError(t, err) + if cleanup != nil { + defer cleanup() + } + + gotStrategy := classifyStrategy(streamDir, branchFolder) + assert.Equalf(t, tc.want, gotStrategy, + "strategy mismatch: want %s, got %s (streamDir=%q)", tc.want, gotStrategy, streamDir) + + assert.Equal(t, tc.wantChart, req.ApplicationSource.Chart, "Chart field") + assert.Equal(t, tc.wantPath, req.ApplicationSource.Path, "Path field") + if tc.wantNoRefs { + assert.Nil(t, req.RefSources, "single-source rows must not set RefSources") + } + + // Repo-server-api request invariants that must hold on EVERY strategy + // (these are the roots of #432 and #416). + assert.Equal(t, "v1.30.1", req.KubeVersion, "KubeVersion must be set (#432)") + assert.Equal(t, []string{"apps/v1", "v1"}, req.ApiVersions, "ApiVersions must be set (#432)") + assertDefaultProjectFields(t, req) + }) + } +} + +// classifyStrategy maps the (streamDir, branchFolder) outcome back to a +// streamStrategy so the table can assert intent rather than raw paths. +func classifyStrategy(streamDir, branchFolder string) streamStrategy { + if streamDir == "" { + return strategyRemote + } + if streamDir == branchFolder { + return strategyStreamBranchRoot + } + // A synthesized temp tree for ref handling contains a .refs/ directory; a + // narrowed chart-dir stream does not. + if info, err := os.Stat(filepath.Join(streamDir, ".refs")); err == nil && info.IsDir() { + return strategyStreamRefs + } + return strategyStreamChartDir +} + +// writeBranchFiles creates a temp branch folder and writes the given relative +// files into it, returning the folder path. +func writeBranchFiles(t *testing.T, files map[string]string) string { + t.Helper() + dir := t.TempDir() + for rel, content := range files { + full := filepath.Join(dir, rel) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o755)) + require.NoError(t, os.WriteFile(full, []byte(content), 0o644)) + } + return dir +} + +func replaceRepo(yaml, repo string) string { + return strings.ReplaceAll(yaml, "{{REPO}}", repo) +} + +// ── A9: local chart with an unrelated in-bounds symlink elsewhere in the repo ── +// +// Routing a local chart must succeed even when the surrounding repo contains +// symlinks, and narrowing the stream to the chart dir must keep those unrelated +// files (and the symlinks pointing at them) out of the tarball. The unit- +// observable part - the tarball contents - is asserted here. The repo server's +// own symlink safety gate (#438) is the part that only a real render exercises; +// that is the I half of this U+I row and lives in the integration suite (see +// also TestBuildManifestRequest_LocalHelmChart_TarballExcludesUnrelatedRepoSymlinks). +func TestSourceMatrix_TableA9_InBoundsSymlink_RoutesAndExcludesUnrelated(t *testing.T) { + const repo = "https://github.com/org/repo.git" + + branchFolder := t.TempDir() + chartDir := filepath.Join(branchFolder, "apps", "chart") + assetDir := filepath.Join(branchFolder, "assets") + require.NoError(t, os.MkdirAll(chartDir, 0o755)) + require.NoError(t, os.MkdirAll(assetDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(chartDir, "Chart.yaml"), + []byte("apiVersion: v2\nname: chart\nversion: 0.1.0\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(chartDir, "values.yaml"), []byte("replicas: 1\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(assetDir, "logo.png"), []byte("PNG"), 0o644)) + // An unrelated symlink elsewhere in the repo, pointing in-bounds at the asset. + require.NoError(t, os.Symlink(filepath.Join(assetDir, "logo.png"), filepath.Join(branchFolder, "logo-link.png"))) + + app := makeApp(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a9} +spec: + destination: {namespace: default} + source: + repoURL: `+repo+` + path: apps/chart + targetRevision: HEAD +`) + + contentSources, refSources, hasMultipleSources, err := splitSources(app) + require.NoError(t, err) + require.Len(t, contentSources, 1) + + req, streamDir, cleanup, err := buildManifestRequestForSource( + app, contentSources[0], refSources, hasMultipleSources, branchFolder, &RepoCreds{}, + manifestRequestRenderContext{ + repoSelector: testRepoSelector(t, repo), + kubeVersion: "v1.30.1", + apiVersions: []string{"apps/v1", "v1"}, + }, + ) + require.NoError(t, err) + if cleanup != nil { + defer cleanup() + } + + // Routes to the chart dir (the unrelated symlink does not force the branch root). + require.Equal(t, strategyStreamChartDir, classifyStrategy(streamDir, branchFolder)) + assert.Empty(t, req.ApplicationSource.Path) + + entries := compressAndListEntries(t, streamDir) + assert.Contains(t, entries, "Chart.yaml") + assert.Contains(t, entries, "values.yaml") + assert.NotContains(t, entries, "logo-link.png", "unrelated repo symlink must not be in the chart tarball") + assert.NotContains(t, entries, "assets/logo.png", "unrelated repo asset must not be in the chart tarball") +} + +// ── A10: local chart with an OUT-OF-BOUNDS file symlink (#438) ──────────────── +// +// A symlink that escapes the repository (e.g. /etc/passwd or ../../outside) is +// rejected by the repo server's symlink safety check during render. That +// rejection happens INSIDE the repo server, so it cannot be observed by a unit +// test that only inspects the request/tarball we hand off. This row is recorded +// as a skipped integration placeholder so the matrix has a named entry for it; +// the real coverage belongs in the integration suite (branch-N) with an +// out-of-bounds symlink fixture. +func TestSourceMatrix_TableA10_OutOfBoundsSymlink_Integration(t *testing.T) { + t.Skip("A10 is integration-only (#438): the repo server's out-of-bounds symlink gate is only exercised by a real render (branch-N)") +} + +// ── A11: local chart containing a DIRECTORY symlink (#448, fixed by #449) ────── +// +// A chart that contains a directory symlink must route successfully (it used to +// fail at the copyDir level with EISDIR because filepath.Walk+Lstat treated the +// dir symlink as a file). At the single-source matrix level the chart dir is +// streamed via the tarball compressor, which preserves the directory symlink as +// a symlink entry; the important assertion is that routing SUCCEEDS rather than +// erroring. The copyDir directory-symlink-following behavior used on the ref +// slow path is additionally covered by TestCopyDir_FollowsDirectorySymlink. +func TestSourceMatrix_TableA11_DirectorySymlinkInChart_RoutesSuccessfully(t *testing.T) { + const repo = "https://github.com/org/repo.git" + + branchFolder := t.TempDir() + chartDir := filepath.Join(branchFolder, "apps", "chart") + externalDir := filepath.Join(branchFolder, "shared", "sql") + require.NoError(t, os.MkdirAll(filepath.Join(chartDir, "files"), 0o755)) + require.NoError(t, os.MkdirAll(externalDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(chartDir, "Chart.yaml"), + []byte("apiVersion: v2\nname: chart\nversion: 0.1.0\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(externalDir, "V1__init.sql"), []byte("SELECT 1;"), 0o644)) + // A DIRECTORY symlink inside the chart, pointing at an in-repo directory. + require.NoError(t, os.Symlink(externalDir, filepath.Join(chartDir, "files", "sql"))) + + app := makeApp(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: a11} +spec: + destination: {namespace: default} + source: + repoURL: `+repo+` + path: apps/chart + targetRevision: HEAD +`) + + contentSources, refSources, hasMultipleSources, err := splitSources(app) + require.NoError(t, err) + require.Len(t, contentSources, 1) + + req, streamDir, cleanup, err := buildManifestRequestForSource( + app, contentSources[0], refSources, hasMultipleSources, branchFolder, &RepoCreds{}, + manifestRequestRenderContext{ + repoSelector: testRepoSelector(t, repo), + kubeVersion: "v1.30.1", + apiVersions: []string{"apps/v1", "v1"}, + }, + ) + // The key regression guard: routing must NOT error on a directory symlink. + require.NoError(t, err, "a chart containing a directory symlink must route without error (#448)") + if cleanup != nil { + defer cleanup() + } + + require.Equal(t, strategyStreamChartDir, classifyStrategy(streamDir, branchFolder)) + assert.Empty(t, req.ApplicationSource.Path) + + // The chart streams successfully and the directory symlink is present. + entries := compressAndListEntries(t, streamDir) + assert.Contains(t, entries, "Chart.yaml") + assert.Contains(t, entries, "files/sql", "directory symlink must be present in the streamed chart") +} diff --git a/pkg/reposerverextract/source_matrix_b_test.go b/pkg/reposerverextract/source_matrix_b_test.go new file mode 100644 index 00000000..ababea7f --- /dev/null +++ b/pkg/reposerverextract/source_matrix_b_test.go @@ -0,0 +1,568 @@ +package reposerverextract + +// Source-combination matrix - Table B (multi-source `spec.sources`). +// +// Companion to SOURCE_MATRIX.md, section B. Each row is one valid way a +// multi-source Application can describe its content + $ref sources. Like Table +// A, the test asserts the ROUTING OUTCOME only: +// +// - strategy per content source: remote / stream(chart-dir) / +// stream(branch-root) / stream(.refs). +// - RefSources map keys when the repo server resolves refs itself. +// - that $ref/... value files are rewritten to on-disk relative paths when we +// synthesize a .refs/ tree, and that the rewritten file actually exists. +// - the repo-server-api invariants that caused bugs (KubeVersion/ApiVersions +// for #432, ProjectName/ProjectSourceRepos for #416) on EVERY strategy. +// +// The split (content vs ref-only) is exercised by feeding the whole Application +// through splitSources and then routing each content source, so rows B9/B10 +// (multiple content sources) assert per-source outcomes. +// +// See SOURCE_MATRIX.md for the issue cross-references (B3/#426, B5/#441, +// B6/#428, etc.). + +import ( + "os" + "path/filepath" + "testing" + + "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// contentExpectation describes the expected routing outcome for one content +// source within a multi-source Application, keyed by a stable identity (the +// source's Chart or Path) so the order the sources are returned in does not +// matter. +type contentExpectation struct { + // chart/path identify which content source this expectation is for. Exactly + // one is set; it is matched against the ORIGINAL source (pre-rewrite). + chart string + path string + + want streamStrategy + wantChart string // expected ManifestRequest Chart after routing + // refKeys are the RefSources map keys that must be present (e.g. "$values"). + // Only meaningful for strategyRemote rows that carry refs. + refKeys []string + // noRefs asserts RefSources is nil (refs were inlined into the stream). + noRefs bool + // valueFileNoDollar asserts the (single) Helm value file was rewritten to a + // relative path with no "$" prefix and that it exists under the stream dir. + valueFileNoDollar bool +} + +type multiSourceCase struct { + name string + appYAML string + + // files to create under branchFolder before routing (relative path -> + // contents). Use {{REPO}} inside appYAML for the PR repo URL. + files map[string]string + + // prRepo overrides the PR repo selector (defaults to the shared repo). + prRepo string + + // withPuller supplies a fakeChartPuller so the remote-chart-with-local-refs + // path (B5/#441) streams the pulled chart instead of falling back to remote. + withPuller bool + + // expectations is keyed implicitly by iteration; each entry is matched to a + // content source by chart/path. + expectations []contentExpectation +} + +func TestSourceMatrix_TableB_MultiSource_Routing(t *testing.T) { + const repo = "https://github.com/org/repo.git" + + cases := []multiSourceCase{ + { + name: "B1 local chart + one local $ref -> stream(.refs), values rewritten", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b1} +spec: + destination: {namespace: default} + sources: + - repoURL: {{REPO}} + ref: cfg + targetRevision: HEAD + - repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD + helm: + valueFiles: + - $cfg/values-prod.yaml +`, + files: map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + "values-prod.yaml": "replicas: 3\n", + }, + expectations: []contentExpectation{{ + path: "apps/chart", + want: strategyStreamRefs, + noRefs: true, + valueFileNoDollar: true, + }}, + }, + { + name: "B2 local chart + ref-with-both-ref+path (GH401) -> stream(.refs)", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b2} +spec: + destination: {namespace: default} + sources: + - repoURL: {{REPO}} + ref: shared + path: shared + targetRevision: HEAD + - repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD + helm: + valueFiles: + - $shared/values.yaml +`, + files: map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + "shared/values.yaml": "replicas: 2\n", + "shared/Chart.yaml": "apiVersion: v2\nname: shared\nversion: 0.1.0\n", + }, + // The source with both ref+path is BOTH a content source and a ref + // source. When rendered AS content it still sees itself in the ref + // list, so it too takes the .refs streaming path (it does not + // degrade to a plain chart-dir stream). + expectations: []contentExpectation{ + { + path: "apps/chart", + want: strategyStreamRefs, + noRefs: true, + valueFileNoDollar: true, + }, + { + path: "shared", + want: strategyStreamRefs, + }, + }, + }, + { + name: "B3 local chart + one external $ref -> remote RPC + RefSources", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b3} +spec: + destination: {namespace: default} + sources: + - repoURL: https://github.com/other/values.git + ref: ext + targetRevision: HEAD + - repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD + helm: + valueFiles: + - $ext/values.yaml +`, + files: map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + }, + // Because a ref lives in a different repo, the whole render falls back + // to the remote RPC and the repo server fetches everything. + expectations: []contentExpectation{{ + path: "apps/chart", + want: strategyRemote, + refKeys: []string{"$ext"}, + }}, + }, + { + name: "B4 local plain dir (non-chart) + local $ref -> stream(.refs)", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b4} +spec: + destination: {namespace: default} + sources: + - repoURL: {{REPO}} + ref: cfg + targetRevision: HEAD + - repoURL: {{REPO}} + path: manifests + targetRevision: HEAD +`, + files: map[string]string{ + "manifests/deployment.yaml": "apiVersion: apps/v1\nkind: Deployment\nmetadata: {name: x}\n", + "cfg/values.yaml": "k: v\n", + }, + // Plain dir primary still synthesizes a .refs tree because a local ref + // source is present; no Helm valueFiles to rewrite. + expectations: []contentExpectation{{ + path: "manifests", + want: strategyStreamRefs, + noRefs: true, + }}, + }, + { + name: "B5 remote chart + one local $ref (#441) -> stream(.refs) via puller", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b5} +spec: + destination: {namespace: default} + sources: + - repoURL: {{REPO}} + ref: cfg + targetRevision: HEAD + - repoURL: https://charts.example.com + chart: my-chart + targetRevision: "1.0.0" + helm: + valueFiles: + - $cfg/values.yaml +`, + files: map[string]string{ + "values.yaml": "replicas: 5\n", + }, + withPuller: true, + // With a puller, the remote chart is pulled into the stream tree and + // rendered as a local path chart; Chart is cleared, values rewritten. + expectations: []contentExpectation{{ + chart: "my-chart", + want: strategyStreamRefs, + wantChart: "", + noRefs: true, + valueFileNoDollar: true, + }}, + }, + { + name: "B5b remote chart + one local $ref, NO puller -> remote RPC + RefSources", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b5b} +spec: + destination: {namespace: default} + sources: + - repoURL: {{REPO}} + ref: cfg + targetRevision: HEAD + - repoURL: https://charts.example.com + chart: my-chart + targetRevision: "1.0.0" + helm: + valueFiles: + - $cfg/values.yaml +`, + files: map[string]string{ + "values.yaml": "replicas: 5\n", + }, + withPuller: false, + // Without a puller we cannot stream the chart, so fall back to remote + // and let the repo server resolve the ref from git. + expectations: []contentExpectation{{ + chart: "my-chart", + want: strategyRemote, + wantChart: "my-chart", + refKeys: []string{"$cfg"}, + }}, + }, + { + name: "B6 remote chart + one external $ref -> remote RPC + RefSources", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b6} +spec: + destination: {namespace: default} + sources: + - repoURL: https://github.com/other/values.git + ref: ext + targetRevision: HEAD + - repoURL: https://charts.example.com + chart: my-chart + targetRevision: "1.0.0" + helm: + valueFiles: + - $ext/values.yaml +`, + files: nil, + withPuller: true, // puller present but external ref forces remote anyway + expectations: []contentExpectation{{ + chart: "my-chart", + want: strategyRemote, + wantChart: "my-chart", + refKeys: []string{"$ext"}, + }}, + }, + { + name: "B7 remote chart + local ref-with-both-ref+path -> remote RPC + RefSources", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b7} +spec: + destination: {namespace: default} + sources: + - repoURL: https://github.com/other/values.git + ref: ext + path: shared + targetRevision: HEAD + - repoURL: https://charts.example.com + chart: my-chart + targetRevision: "1.0.0" + helm: + valueFiles: + - $ext/values.yaml +`, + files: nil, + // The ref source is external; primary remote chart -> remote RPC. The + // ref-with-path source is also a content source (cross-repo) -> remote. + expectations: []contentExpectation{ + { + chart: "my-chart", + want: strategyRemote, + wantChart: "my-chart", + refKeys: []string{"$ext"}, + }, + { + path: "shared", + want: strategyRemote, + wantChart: "", + }, + }, + }, + { + name: "B8 local chart + local $ref AND out-of-chart abs value -> stream(.refs)", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b8} +spec: + destination: {namespace: default} + sources: + - repoURL: {{REPO}} + ref: cfg + targetRevision: HEAD + - repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD + helm: + valueFiles: + - $cfg/values.yaml + - /env/prod.yaml +`, + files: map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + "values.yaml": "replicas: 3\n", + "env/prod.yaml": "env: prod\n", + }, + // Combines the A5 (out-of-chart abs) concern with refs. The $ref file + // is rewritten; the absolute /env/prod.yaml must remain reachable in + // the streamed tree. We assert the strategy + that the $ref file is + // rewritten; the out-of-chart file reachability is asserted in check. + expectations: []contentExpectation{{ + path: "apps/chart", + want: strategyStreamRefs, + noRefs: true, + valueFileNoDollar: true, + }}, + }, + { + name: "B9 two local content sources, no ref -> stream each independently", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b9} +spec: + destination: {namespace: default} + sources: + - repoURL: {{REPO}} + path: apps/chart-a + targetRevision: HEAD + - repoURL: {{REPO}} + path: apps/chart-b + targetRevision: HEAD +`, + files: map[string]string{ + "apps/chart-a/Chart.yaml": "apiVersion: v2\nname: a\nversion: 0.1.0\n", + "apps/chart-b/Chart.yaml": "apiVersion: v2\nname: b\nversion: 0.1.0\n", + }, + expectations: []contentExpectation{ + {path: "apps/chart-a", want: strategyStreamChartDir, noRefs: true}, + {path: "apps/chart-b", want: strategyStreamChartDir, noRefs: true}, + }, + }, + { + name: "B10 local content + external content (mixed) -> stream local, remote external", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b10} +spec: + destination: {namespace: default} + sources: + - repoURL: {{REPO}} + path: apps/local-chart + targetRevision: HEAD + - repoURL: https://charts.example.com + chart: remote-chart + targetRevision: "2.0.0" +`, + files: map[string]string{ + "apps/local-chart/Chart.yaml": "apiVersion: v2\nname: local\nversion: 0.1.0\n", + }, + expectations: []contentExpectation{ + {path: "apps/local-chart", want: strategyStreamChartDir, noRefs: true}, + {chart: "remote-chart", want: strategyRemote, wantChart: "remote-chart", noRefs: true}, + }, + }, + { + name: "B11 ref-only source with no path (points at repo root) -> stream(.refs)", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: b11} +spec: + destination: {namespace: default} + sources: + - repoURL: {{REPO}} + ref: root + targetRevision: HEAD + - repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD + helm: + valueFiles: + - $root/global/values.yaml +`, + files: map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + "global/values.yaml": "g: 1\n", + "apps/chart/values.yaml": "local: true\n", + }, + // The ref-only source has no path, so its ref dir is the branch root; + // $root/global/values.yaml must resolve to a real file in .refs/root. + expectations: []contentExpectation{{ + path: "apps/chart", + want: strategyStreamRefs, + noRefs: true, + valueFileNoDollar: true, + }}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + branchFolder := writeBranchFiles(t, tc.files) + + prRepo := tc.prRepo + if prRepo == "" { + prRepo = repo + } + app := makeApp(t, replaceRepo(tc.appYAML, repo)) + + contentSources, refSources, hasMultipleSources, err := splitSources(app) + require.NoError(t, err) + require.True(t, hasMultipleSources, "table B rows use spec.sources") + require.Len(t, contentSources, len(tc.expectations), + "each content source must have exactly one expectation") + + renderCtx := manifestRequestRenderContext{ + repoSelector: testRepoSelector(t, prRepo), + kubeVersion: "v1.30.1", + apiVersions: []string{"apps/v1", "v1"}, + } + if tc.withPuller { + renderCtx.puller = &fakeChartPuller{} + } + + for _, src := range contentSources { + exp := matchExpectation(t, tc.expectations, src) + + req, streamDir, cleanup, err := buildManifestRequestForSource( + app, src, refSources, hasMultipleSources, branchFolder, + &RepoCreds{}, renderCtx, + ) + require.NoError(t, err) + if cleanup != nil { + defer cleanup() + } + + gotStrategy := classifyStrategy(streamDir, branchFolder) + assert.Equalf(t, exp.want, gotStrategy, + "[%s] strategy mismatch: want %s, got %s (streamDir=%q)", + sourceID(src), exp.want, gotStrategy, streamDir) + + assert.Equalf(t, exp.wantChart, req.ApplicationSource.Chart, + "[%s] Chart field", sourceID(src)) + + if exp.noRefs { + assert.Nilf(t, req.RefSources, "[%s] RefSources must be nil (refs inlined)", sourceID(src)) + } + for _, key := range exp.refKeys { + require.NotNilf(t, req.RefSources, "[%s] RefSources must be populated", sourceID(src)) + _, ok := req.RefSources[key] + assert.Truef(t, ok, "[%s] RefSources must contain key %q (got keys %v)", + sourceID(src), key, refKeysOf(req.RefSources)) + } + + if exp.valueFileNoDollar { + require.NotNilf(t, req.ApplicationSource.Helm, "[%s] expected Helm config", sourceID(src)) + require.NotEmptyf(t, req.ApplicationSource.Helm.ValueFiles, + "[%s] expected at least one value file", sourceID(src)) + rewritten := req.ApplicationSource.Helm.ValueFiles[0] + assert.NotContainsf(t, rewritten, "$", + "[%s] $ref value file must be rewritten to a relative path", sourceID(src)) + // The rewritten path is relative to the content dir inside the + // stream tree; it must point at a real file. + abs := filepath.Clean(filepath.Join(streamDir, req.ApplicationSource.Path, rewritten)) + _, statErr := os.Stat(abs) + assert.NoErrorf(t, statErr, "[%s] rewritten value file %q should exist", sourceID(src), abs) + } + + // Repo-server-api invariants on EVERY strategy (#432, #416). + assert.Equalf(t, "v1.30.1", req.KubeVersion, "[%s] KubeVersion must be set (#432)", sourceID(src)) + assert.Equalf(t, []string{"apps/v1", "v1"}, req.ApiVersions, "[%s] ApiVersions must be set (#432)", sourceID(src)) + assertDefaultProjectFields(t, req) + } + }) + } +} + +// sourceID returns a short human label for a content source for test messages. +func sourceID(s v1alpha1.ApplicationSource) string { + if s.Chart != "" { + return "chart=" + s.Chart + } + return "path=" + s.Path +} + +// matchExpectation finds the expectation for the given content source by its +// chart or path identity, failing the test if none matches. +func matchExpectation(t *testing.T, exps []contentExpectation, src v1alpha1.ApplicationSource) contentExpectation { + t.Helper() + for _, e := range exps { + if e.chart != "" && e.chart == src.Chart { + return e + } + if e.path != "" && e.path == src.Path && src.Chart == "" { + return e + } + } + t.Fatalf("no expectation matched content source %s", sourceID(src)) + return contentExpectation{} +} + +func refKeysOf(m map[string]*v1alpha1.RefTarget) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + return keys +} diff --git a/pkg/reposerverextract/source_matrix_cde_test.go b/pkg/reposerverextract/source_matrix_cde_test.go new file mode 100644 index 00000000..4e9a2002 --- /dev/null +++ b/pkg/reposerverextract/source_matrix_cde_test.go @@ -0,0 +1,486 @@ +package reposerverextract + +// Source-combination matrix - Tables C, D, E. +// +// Companion to SOURCE_MATRIX.md sections C/D/E. These cover the axes that are +// orthogonal to the per-source routing in Tables A/B: +// +// - Table C: cardinality / kind. Does an ApplicationSet route the same as the +// equivalent Application, and do the degenerate source topologies (no +// source, all-ref-only) fail loudly? +// - Table D: app-of-apps traversal. A discovered child Application must route +// through buildManifestRequestForSource exactly like a seed Application. +// (The --redirect-target-revisions behavior for children, D2/D3 / #446, is +// already covered by appofapps_test.go's BuildChildApplication_* tests and +// is NOT duplicated here.) +// - Table E: cross-cutting request-content correctness. The repo-server-api +// request invariants (#432 KubeVersion/ApiVersions, #416 +// ProjectName/ProjectSourceRepos) must hold on ALL THREE strategies, not +// just the streamed local-chart one that historically asserted them. +// +// E3 (transient helm-dep build error surfaced, #416) only fails inside a real +// repo server and is an integration concern; it is represented below as a +// skipped integration placeholder (TestSourceMatrix_TableE3_*) so every E-row +// has a named test. + +import ( + "testing" + + "github.com/dag-andersen/argocd-diff-preview/pkg/git" + "github.com/dag-andersen/argocd-diff-preview/pkg/repository" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +// ── Table C: cardinality / kind ───────────────────────────────────────────── + +// C1: an ApplicationSet whose sources live under spec.template.spec must split +// and route identically to the equivalent Application. We build the same +// logical multi-source app in both shapes and assert the routing outcome +// matches source-for-source. +func TestSourceMatrix_TableC1_ApplicationSet_RoutesLikeApplication(t *testing.T) { + const repo = "https://github.com/org/repo.git" + + applicationYAML := ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: c1-app} +spec: + destination: {namespace: default} + sources: + - repoURL: https://github.com/other/values.git + ref: ext + targetRevision: HEAD + - repoURL: ` + repo + ` + path: apps/chart + targetRevision: HEAD + helm: + valueFiles: + - $ext/values.yaml +` + applicationSetYAML := ` +apiVersion: argoproj.io/v1alpha1 +kind: ApplicationSet +metadata: {name: c1-appset} +spec: + generators: + - list: + elements: [] + template: + metadata: {name: c1-child} + spec: + destination: {namespace: default} + sources: + - repoURL: https://github.com/other/values.git + ref: ext + targetRevision: HEAD + - repoURL: ` + repo + ` + path: apps/chart + targetRevision: HEAD + helm: + valueFiles: + - $ext/values.yaml +` + files := map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + } + + route := func(t *testing.T, rawYAML string) (streamStrategy, []string) { + t.Helper() + branchFolder := writeBranchFiles(t, files) + app := makeApp(t, rawYAML) + + contentSources, refSources, hasMultipleSources, err := splitSources(app) + require.NoError(t, err) + require.Len(t, contentSources, 1, "single content source expected") + + req, streamDir, cleanup, err := buildManifestRequestForSource( + app, contentSources[0], refSources, hasMultipleSources, branchFolder, &RepoCreds{}, + manifestRequestRenderContext{ + repoSelector: testRepoSelector(t, repo), + kubeVersion: "v1.30.1", + apiVersions: []string{"apps/v1", "v1"}, + }, + ) + require.NoError(t, err) + if cleanup != nil { + defer cleanup() + } + return classifyStrategy(streamDir, branchFolder), refKeysOf(req.RefSources) + } + + appStrategy, appRefKeys := route(t, applicationYAML) + setStrategy, setRefKeys := route(t, applicationSetYAML) + + assert.Equal(t, appStrategy, setStrategy, + "ApplicationSet must route to the same strategy as the equivalent Application") + assert.ElementsMatch(t, appRefKeys, setRefKeys, + "ApplicationSet must produce the same RefSources keys as the equivalent Application") + // External ref -> both must be remote with a $ext RefSource. + assert.Equal(t, strategyRemote, appStrategy) + assert.ElementsMatch(t, []string{"$ext"}, appRefKeys) +} + +// C2/C3: degenerate source topologies must fail loudly in splitSources rather +// than silently producing an empty/invalid request. +func TestSourceMatrix_TableC_DegenerateTopologies_Error(t *testing.T) { + cases := []struct { + name string + appYAML string + }{ + { + name: "C2 neither source nor sources -> error", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: c2} +spec: + destination: {namespace: default} +`, + }, + { + name: "C3 all sources are ref-only (no content) -> error", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: c3} +spec: + destination: {namespace: default} + sources: + - repoURL: https://github.com/org/repo.git + ref: a + targetRevision: HEAD + - repoURL: https://github.com/org/repo.git + ref: b + targetRevision: HEAD +`, + }, + { + name: "C3b empty sources list -> error", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: c3b} +spec: + destination: {namespace: default} + sources: [] +`, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + app := makeApp(t, tc.appYAML) + _, _, _, err := splitSources(app) + assert.Error(t, err, "degenerate source topology must return an error") + }) + } +} + +// ── Table D: app-of-apps traversal ────────────────────────────────────────── + +// D1: a discovered child Application must route through +// buildManifestRequestForSource exactly like a seed Application. This ties the +// app-of-apps path back into the source matrix: whatever a child's source is, +// the same routing rules (stream local chart dir, remote chart -> remote RPC, +// etc.) apply. +// +// (D2 and D3 below cover the --redirect-target-revisions behavior for children, +// #446. There is overlapping coverage in appofapps_test.go's +// TestBuildChildApplication_* tests, but the matrix rows are kept here so every +// row in SOURCE_MATRIX.md has a corresponding, named test in the matrix suite.) +func TestSourceMatrix_TableD1_ChildApplication_RoutesLikeSeed(t *testing.T) { + const repo = "https://github.com/org/repo.git" + + cases := []struct { + name string + childYAML string + files map[string]string + want streamStrategy + wantChart string + }{ + { + name: "D1a child local chart -> stream(chart-dir)", + childYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: child-local} +spec: + destination: {namespace: default} + source: + repoURL: ` + repo + ` + path: apps/child + targetRevision: HEAD +`, + files: map[string]string{ + "apps/child/Chart.yaml": "apiVersion: v2\nname: child\nversion: 0.1.0\n", + }, + want: strategyStreamChartDir, + wantChart: "", + }, + { + name: "D1b child remote chart -> remote RPC", + childYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: child-remote} +spec: + destination: {namespace: default} + source: + repoURL: https://charts.example.com + chart: child-chart + targetRevision: "1.0.0" +`, + files: nil, + want: strategyRemote, + wantChart: "child-chart", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + branchFolder := writeBranchFiles(t, tc.files) + // A discovered child is just an Application manifest; feed it through + // the same split + route path a seed app takes. + child := makeApp(t, tc.childYAML) + + contentSources, refSources, hasMultipleSources, err := splitSources(child) + require.NoError(t, err) + require.Len(t, contentSources, 1) + + req, streamDir, cleanup, err := buildManifestRequestForSource( + child, contentSources[0], refSources, hasMultipleSources, branchFolder, &RepoCreds{}, + manifestRequestRenderContext{ + repoSelector: testRepoSelector(t, repo), + kubeVersion: "v1.30.1", + apiVersions: []string{"apps/v1", "v1"}, + }, + ) + require.NoError(t, err) + if cleanup != nil { + defer cleanup() + } + + assert.Equal(t, tc.want, classifyStrategy(streamDir, branchFolder), + "child must route like a seed app") + assert.Equal(t, tc.wantChart, req.ApplicationSource.Chart, "Chart field") + assertDefaultProjectFields(t, req) + }) + } +} + +// D2: a discovered child Application must honor --redirect-target-revisions +// exactly like a seed Application (#446, fixed by #447). A child source whose +// targetRevision is in the redirect list is redirected to the target branch; a +// source pinned to a revision outside the list is left untouched; an empty list +// preserves the "redirect everything" default. +// +// childAppManifestD2 builds a minimal discovered child whose single source is +// pinned to targetRevision. (Named with a D2 suffix to avoid colliding with the +// childAppManifest helper in appofapps_test.go.) +func childAppManifestD2(repoURL, targetRevision string) unstructured.Unstructured { + return unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "argoproj.io/v1alpha1", + "kind": "Application", + "metadata": map[string]any{"name": "d2-child"}, + "spec": map[string]any{ + "source": map[string]any{ + "repoURL": repoURL, + "path": "apps/child", + "targetRevision": targetRevision, + }, + }, + }} +} + +func TestSourceMatrix_TableD2_ChildHonorsRedirectTargetRevisions(t *testing.T) { + const repoURL = "https://github.com/example/repo" + + cases := []struct { + name string + pinnedRevision string + redirectRevisions []string + wantRevision string + }{ + { + name: "D2a revision in list -> redirected to target branch", + pinnedRevision: "main", + redirectRevisions: []string{"main", "HEAD"}, + wantRevision: "target", + }, + { + name: "D2b revision NOT in list -> left untouched", + pinnedRevision: "feature-branch", + redirectRevisions: []string{"main", "HEAD"}, + wantRevision: "feature-branch", + }, + { + name: "D2c empty list -> redirect everything (default)", + pinnedRevision: "feature-branch", + redirectRevisions: nil, + wantRevision: "target", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + selector, err := repository.NewSelector(repoURL, "") + require.NoError(t, err) + targetBranch := git.NewBranch("target", git.Target) + + manifest := childAppManifestD2(repoURL, tc.pinnedRevision) + child, err := buildChildApplication( + "argocd", manifest, "parent: root", git.Target, targetBranch, *selector, tc.redirectRevisions) + require.NoError(t, err) + require.NotNil(t, child) + + got, found, err := unstructured.NestedString(child.Yaml.Object, "spec", "source", "targetRevision") + require.NoError(t, err) + require.True(t, found, "child source must keep a targetRevision") + assert.Equal(t, tc.wantRevision, got, + "child targetRevision after applying redirect list %v", tc.redirectRevisions) + }) + } +} + +// D3: a child Application whose source is pinned to a ref that only exists on +// the remote (not in the local checkout) must render from the remote rather +// than being redirected to the local branch. This is a consequence of D2/#446, +// but the failure mode (the repo server cloning a remote-only ref) is only +// observable against a real repo server, so it is an INTEGRATION row, not a +// unit row. It is recorded here as a skipped placeholder so the matrix has a +// named entry for every row in SOURCE_MATRIX.md; the real coverage lives in the +// integration suite (branch-N). +func TestSourceMatrix_TableD3_ChildRemoteOnlyRef_Integration(t *testing.T) { + t.Skip("D3 is integration-only: a remote-only child ref must be rendered by a real repo server (branch-N), not a unit test") +} + +// ── Table E: cross-cutting request-content correctness ─────────────────────── + +// E1/E2: the repo-server-api request invariants must hold on EVERY strategy. +// Historically only the streamed local-chart path asserted KubeVersion/ +// ApiVersions (#432) and ProjectName/ProjectSourceRepos (#416); this test +// pins them across remote, stream(chart-dir) and stream(.refs) so a future +// routing change cannot drop them on one path. +func TestSourceMatrix_TableE_RequestInvariants_AllStrategies(t *testing.T) { + const repo = "https://github.com/org/repo.git" + const kubeVersion = "v1.31.2" + apiVersions := []string{"apps/v1", "networking.k8s.io/v1", "v1"} + + cases := []struct { + name string + appYAML string + files map[string]string + want streamStrategy + }{ + { + name: "E remote chart (strategy R)", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: e-remote} +spec: + destination: {namespace: default} + source: + repoURL: https://charts.example.com + chart: nginx + targetRevision: "1.0.0" +`, + files: nil, + want: strategyRemote, + }, + { + name: "E local chart (strategy S, chart dir)", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: e-local} +spec: + destination: {namespace: default} + source: + repoURL: ` + repo + ` + path: apps/chart + targetRevision: HEAD +`, + files: map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + }, + want: strategyStreamChartDir, + }, + { + name: "E local chart + local ref (strategy S+refs)", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: e-refs} +spec: + destination: {namespace: default} + sources: + - repoURL: ` + repo + ` + ref: cfg + targetRevision: HEAD + - repoURL: ` + repo + ` + path: apps/chart + targetRevision: HEAD + helm: + valueFiles: + - $cfg/values.yaml +`, + files: map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + "values.yaml": "k: v\n", + }, + want: strategyStreamRefs, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + branchFolder := writeBranchFiles(t, tc.files) + app := makeApp(t, tc.appYAML) + + contentSources, refSources, hasMultipleSources, err := splitSources(app) + require.NoError(t, err) + require.Len(t, contentSources, 1) + + req, streamDir, cleanup, err := buildManifestRequestForSource( + app, contentSources[0], refSources, hasMultipleSources, branchFolder, &RepoCreds{}, + manifestRequestRenderContext{ + repoSelector: testRepoSelector(t, repo), + kubeVersion: kubeVersion, + apiVersions: apiVersions, + }, + ) + require.NoError(t, err) + if cleanup != nil { + defer cleanup() + } + + // Sanity: this row really exercises the strategy it claims to. + require.Equal(t, tc.want, classifyStrategy(streamDir, branchFolder), + "row must exercise its declared strategy") + + // E1 (#432): cluster version info present on every strategy. + assert.Equal(t, kubeVersion, req.KubeVersion, "KubeVersion must be set (#432)") + assert.Equal(t, apiVersions, req.ApiVersions, "ApiVersions must be set (#432)") + + // E2 (#416): permissive project so helm-build errors are not masked + // as permission errors. assertDefaultProjectFields checks both + // ProjectName=default and ProjectSourceRepos=["*"]. + assertDefaultProjectFields(t, req) + }) + } +} + +// E3: a transient Helm dependency build error must be surfaced with its original +// message rather than swallowed or masked (#416). The error originates inside +// the repo server while it builds chart dependencies, so it is only observable +// against a real repo server - the permissive ProjectSourceRepos asserted in +// E2 is the unit-level precondition that keeps the repo server from replacing it +// with a misleading permission error, but verifying the surfaced message is an +// integration concern. Recorded as a skipped placeholder so the matrix has a +// named entry for E3; real coverage belongs in the integration suite (branch-N). +func TestSourceMatrix_TableE3_HelmDepBuildErrorSurfaced_Integration(t *testing.T) { + t.Skip("E3 is integration-only (#416): a swallowed helm-dependency build error is only observable against a real repo server (branch-N)") +} From cd55f423022b5e4a4fb58a2e28ef31c653eb7f30 Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Sun, 21 Jun 2026 00:51:33 +0200 Subject: [PATCH 2/3] temp --- pkg/reposerverextract/SOURCE_MATRIX.md | 132 ++++++++++++++++++++++--- 1 file changed, 119 insertions(+), 13 deletions(-) diff --git a/pkg/reposerverextract/SOURCE_MATRIX.md b/pkg/reposerverextract/SOURCE_MATRIX.md index 79040404..ff26737e 100644 --- a/pkg/reposerverextract/SOURCE_MATRIX.md +++ b/pkg/reposerverextract/SOURCE_MATRIX.md @@ -1,10 +1,11 @@ # Repo-server-api source-combination matrix (test coverage spec) -> Status: **IMPLEMENTED** - this map is now backed by a complete test suite. -> Every row below (A1-A15, B1-B11, C1-C3, D1-D3, E1-E3) has a named test in -> `source_matrix_a_test.go`, `source_matrix_b_test.go`, and -> `source_matrix_cde_test.go`. Integration-only rows (**I**) are present as -> skipped placeholders so the suite stays 1:1 with this table. +> Status: **PARTIAL - gaps found.** Tables A-E (35 rows) are implemented and +> backed by named tests. However, a subsequent audit of the **actual repo-server +> source** (`argo-cd/reposerver/repository/repository.go`) found several input +> dimensions the original axes missed - including **two confirmed latent bugs** +> in our own routing. Those are captured in tables **F-H** and the +> "Repo-server audit findings" section below; they are **not yet implemented**. > > Goal: enumerate every *valid* way an Argo CD Application can describe its > `source` / `sources`, decide the expected behaviour of the repo-server-api @@ -13,9 +14,9 @@ > combinations so we stop discovering broken-but-valid cases one GitHub issue > at a time. > -> Test-name convention: each row maps to `TestSourceMatrix_Table...`, keyed -> by the row id (e.g. A11 -> `TestSourceMatrix_TableA11_*`, the B-table rows are -> subtests named `B1`/`B2`/... inside `TestSourceMatrix_TableB_MultiSource_Routing`). +> Test-name convention: each implemented row maps to `TestSourceMatrix_Table...`, +> keyed by the row id (e.g. A11 -> `TestSourceMatrix_TableA11_*`, the B-table rows +> are subtests named `B1`/`B2`/... inside `TestSourceMatrix_TableB_MultiSource_Routing`). ## Why this exists @@ -79,8 +80,39 @@ valid; invalid ones are noted. 8. **Revision redirection** - top-level app vs app-of-apps **child**, with / without `--redirect-target-revisions` (#446). +The following axes were **added after the repo-server audit** (they were missing +from the original list and are the source of the F-H tables): + +9. **Explicit type vs on-disk type** - a source may set an explicit `helm:` / + `kustomize:` / `directory:` / `plugin:` block. `ExplicitType()` + (`types.go:3799`) **overrides** on-disk marker-file detection, and **two** + explicit blocks is a hard error ("multiple application sources defined"). Our + `isLocalHelmChart` / `isKustomizeSource` only look at on-disk files, so they + can disagree with the repo server. +10. **`.argocd-source.yaml` override files** - `.argocd-source.yaml` and + `.argocd-source-.yaml` are read **from the app/chart dir** and + merge-patched onto the source **before** type detection + (`mergeSourceParameters`, `repository.go:1872`). They can inject + `helm.valueFiles` (incl. out-of-chart!), `helm.parameters`, + `directory.recurse`, or even a whole tool block. Our tool does not read them + at all. +11. **`helm.fileParameters`** - `fileParameters[].path` is a **first-class ref + candidate** exactly like `valueFiles` (`repository.go:571-575`) and can be + `$ref/...`, out-of-chart, or remote. Our `rewriteRefValueFiles` only rewrites + `ValueFiles`. +12. **value-file resolution variants** (Helm) beyond axis 6: env-var + placeholders (`env.Envsubst`), glob patterns (lexical order = merge order), + disallowed URL scheme (hard error), `ignoreMissingValueFiles` true/false + (zero-glob -> gRPC NotFound). Allowed schemes come from + `HelmOptions.ValuesFileSchemes` (argocd-cm); nil = **no** remote allowed. +13. **Kustomize / Directory external-file pulls** - Kustomize `components:` may + reference dirs **outside** the app dir and are **not** repoRoot-bounded + (unlike value files); `--enable-helm` build option pulls remote charts; + jsonnet `libs:` are **repoRoot-relative** and can import cross-app. + Axes 6-8 only bite once a case is already on a **Stream** strategy, which is -why they produced separate bugs. +why they produced separate bugs. Axes 9-13 can bite on **any** strategy and are +largely unmodeled today. ## Legend @@ -159,16 +191,90 @@ why they produced separate bugs. --- +## Repo-server audit findings (NEW - not yet implemented) + +The tables above were derived from *our* model of the repo server. Reading the +actual repo-server source (`argo-cd/reposerver/repository/repository.go`) +surfaced input dimensions the original axes missed. Tables **F-H** below +enumerate them. **Two rows (F4, H1) are confirmed latent bugs in our tool**, not +just coverage gaps. + +> ⚠️ None of the F-H rows have tests yet. They are the real output of "does the +> matrix cover all cases?" - the answer was **no**. + +### F. Source-type detection / overrides + +| # | Case | Expected | Test | Status | Notes (repo-server ref) | +| --- | --- | --- | --- | --- | --- | +| F1 | dir has `Chart.yaml` but manifest sets `directory:` explicitly | render as **Directory**, not Helm | U | ⬜ | `ExplicitType()` wins over on-disk (`types.go:3799`). Our `isLocalHelmChart` ignores the explicit block -> may narrow to chart dir wrongly. | +| F2 | dir has `kustomization.yaml` but manifest sets `helm:` explicitly | render as **Helm** | U | ⬜ | Same precedence. Our `isKustomizeSource` would keep branch root; repo server renders Helm. | +| F3 | manifest sets **both** `helm:` and `kustomize:` | **hard error** "multiple application sources defined" | U/I | ⬜ | `ExplicitType()` errors (`types.go:3816`). Our tool streams something instead of failing. Negative case. | +| F4 | **`.argocd-source.yaml` in chart dir adds out-of-chart `helm.valueFiles`** | must stream **branch root** (file is outside chart) | U+I | 🟥 | **CONFIRMED BUG.** `mergeSourceParameters` (`repository.go:1872`) injects valueFiles our `hasOutOfChartValueFile` never sees (it reads only the manifest). We narrow to chart dir -> repo server render fails (missing file). Same class as #444. | +| F5 | `.argocd-source-.yaml` injects a `helm:`/`kustomize:` block | type changes accordingly | U+I | ⬜ | Override applied **before** type detection. Our tool ignores override files entirely. | +| F6 | dir has **both** `Chart.yaml` and `kustomization.yaml` | repo-server discovery picks **Kustomize** | U | ⬜ | `discovery.go:65` overwrites Helm with Kustomize. Our `buildStreamDirForLocalSource` checks Kustomize first too (branch root) - verify parity. | +| F7 | source type disabled via `EnabledSourceTypes` | falls back to **Directory** | I | ⬜ | `IsManifestGenerationEnabled` (`repository.go:1938`). We never set EnabledSourceTypes; document behavior. | + +### G. Helm value-file resolution variants (extends axis 6) + +| # | Case | Expected | Test | Status | Notes (repo-server ref) | +| --- | --- | --- | --- | --- | --- | +| G1 | valueFile with env placeholder `$ENV/x.yaml` (via `helm.valuesObject`/env) | resolved after `env.Envsubst` | U+I | ⬜ | `repository.go:1502`. Our `hasOutOfChartValueFile` does not envsubst before classifying -> could misjudge chart-dir vs branch-root. | +| G2 | valueFile **glob** `env/*.yaml` | expanded lexically; order = merge precedence | U+I | ⬜ | `isGlobPath` + `doublestar` (`repository.go:1516`). A glob can match out-of-chart files -> our narrowing logic ignores globs. | +| G3 | valueFile remote URL with **disallowed** scheme | **hard error** | I | ⬜ | `isURLSchemeAllowed` (`resolved.go:53`). Schemes from `HelmOptions.ValuesFileSchemes`; nil = none allowed. | +| G4 | missing valueFile, `ignoreMissingValueFiles: true` vs `false` | skip vs fail | U+I | ⬜ | `repository.go:1542`. Affects whether a missing out-of-chart file even matters. | +| G5 | valueFile leading-slash `/env/x.yaml` (repo-root relative) | resolved from repo root | U | ⚠️ | Covered by A5/B8 for the *routing* outcome, but not the resolution semantics directly. | + +### H. Helm `fileParameters` (parallel to valueFiles - largely unmodeled) + +| # | Case | Expected | Test | Status | Notes (repo-server ref) | +| --- | --- | --- | --- | --- | --- | +| H1 | multi-source, **`helm.fileParameters[].path: $ref/foo`** | path rewritten like a valueFile; ref staged | U+I | 🟥 | **CONFIRMED BUG.** Repo server treats fileParameters as ref candidates (`repository.go:571-575`); our `rewriteRefValueFiles` only rewrites `ValueFiles`, never `FileParameters` -> ref staged but path unrewritten -> render fails. Same class as #441/#426. | +| H2 | `fileParameters[].path` out-of-chart relative `../x` | branch root must be streamed | U+I | ⬜ | fileParameters resolve like valueFiles (`repository.go:1343`). Our `hasOutOfChartValueFile` ignores fileParameters -> may narrow wrongly. | +| H3 | `fileParameters[].path` remote URL | fetched by repo server | I | ⬜ | Same scheme rules as valueFiles. | + +### I. Kustomize / Directory external pulls (extends axis 13) + +| # | Case | Expected | Test | Status | Notes (repo-server ref) | +| --- | --- | --- | --- | --- | --- | +| I1 | Kustomize `components:` referencing `../other-dir` (outside app) | branch root must be streamed | U+I | ⬜ | components are **not** repoRoot-bounded (`kustomize.go:337`). We always stream branch root for Kustomize, so this likely works - but unverified. | +| I2 | Kustomize `--enable-helm` build option + `helmCharts:` | pulls remote chart during build | I | ⬜ | `isHelmEnabled` (`kustomize.go:427`). Needs build options + network. | +| I3 | Directory jsonnet `libs:` referencing another repo dir | imports resolved repoRoot-relative | U+I | ⬜ | `repository.go:2263`. Cross-app import; branch root must be streamed. | + +### Negative / invalid combinations (document, don't "fix") + +| # | Case | Expected | Notes | +| --- | --- | --- | --- | +| N1 | a `$ref` source that itself sets `chart:` | repo server **rejects** | "Helm charts are not yet not supported for 'ref' sources" (`repository.go:594`). | +| N2 | raw streamed single-source with `$ref` value file, no `.refs` staging | fails "failed to find repo" | Streaming path does not populate `gitRepoPaths` (`repository.go:1565`). Validates *why* our `.refs` staging + rewrite mechanism exists - and why it must cover fileParameters too (H1). | + +--- + ## What we found (honest summary) -The suite is **complete and 1:1 with this table**: 35 rows, each with a named +Tables A-E are **complete and 1:1 with their rows**: 35 rows, each with a named test. 9 passing test functions cover the unit rows; 3 skipped placeholders mark the integration-only rows (A10, D3, E3). -**New product bugs found proactively: 0.** Every unit row passed against `main`, -or was an already-tracked issue. The routing code is currently in good shape. +**Bugs found by the unit matrix alone (A-E): 0.** Every unit row passed against +`main`, or was an already-tracked issue. + +**Bugs found by the repo-server audit (F-H): 2 confirmed, plus several +unmodeled dimensions.** This is the real payoff and it only appeared once we +stopped trusting our own model and read the repo-server source: + +- **F4** - `.argocd-source.yaml` can add out-of-chart `helm.valueFiles` that our + `hasOutOfChartValueFile` never sees (it inspects only the manifest), so we + narrow the stream to the chart dir and the render fails on the missing file. + Same class as #444. +- **H1** - `helm.fileParameters[].path: $ref/...` is a first-class ref candidate + in the repo server, but our `rewriteRefValueFiles` only rewrites `ValueFiles`, + so the ref is staged into `.refs/` while the fileParameter path is left + unrewritten and unresolvable. Same class as #441/#426. + +Both are **latent** (no open issue yet) - exactly the "broken-but-valid case we +discover one GitHub issue at a time" this exercise was meant to pre-empt. -What the exercise actually bought us: +What the A-E exercise bought us regardless: 1. **Regression net for recently-merged fixes.** Several fixes had no dedicated regression test before; the matrix now pins them so they cannot silently From 035f28def29fdc00802f707bb6df5ce062de4961 Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Sun, 21 Jun 2026 01:23:16 +0200 Subject: [PATCH 3/3] Tests | Restructure repo-server source matrix --- pkg/reposerverextract/SOURCE_MATRIX.md | 511 ++++++++---------- ...atrix_cde_test.go => crosscutting_test.go} | 74 +-- pkg/reposerverextract/gaps_test.go | 463 ++++++++++++++++ .../{source_matrix_b_test.go => refs_test.go} | 42 +- ...urce_matrix_a_test.go => strategy_test.go} | 70 +-- 5 files changed, 796 insertions(+), 364 deletions(-) rename pkg/reposerverextract/{source_matrix_cde_test.go => crosscutting_test.go} (82%) create mode 100644 pkg/reposerverextract/gaps_test.go rename pkg/reposerverextract/{source_matrix_b_test.go => refs_test.go} (91%) rename pkg/reposerverextract/{source_matrix_a_test.go => strategy_test.go} (87%) diff --git a/pkg/reposerverextract/SOURCE_MATRIX.md b/pkg/reposerverextract/SOURCE_MATRIX.md index ff26737e..b4c78a51 100644 --- a/pkg/reposerverextract/SOURCE_MATRIX.md +++ b/pkg/reposerverextract/SOURCE_MATRIX.md @@ -1,320 +1,289 @@ # Repo-server-api source-combination matrix (test coverage spec) -> Status: **PARTIAL - gaps found.** Tables A-E (35 rows) are implemented and -> backed by named tests. However, a subsequent audit of the **actual repo-server -> source** (`argo-cd/reposerver/repository/repository.go`) found several input -> dimensions the original axes missed - including **two confirmed latent bugs** -> in our own routing. Those are captured in tables **F-H** and the -> "Repo-server audit findings" section below; they are **not yet implemented**. +> Status: **All rows implemented; known-bug repros skipped by default.** Every +> row in this matrix now has a named test. Most pass; five (TYP5, REACH5, +> REACH6, REACH11, REF13) are 🟥 SKIP - real tests asserting repo-server-correct +> behavior our tool does not yet produce. They are skipped in normal test runs +> so CI stays green, and can be enabled with `RUN_KNOWN_BUG_TESTS=1`. +> Integration-only rows are skipped placeholders. > -> Goal: enumerate every *valid* way an Argo CD Application can describe its -> `source` / `sources`, decide the expected behaviour of the repo-server-api -> render path for each, and mark which cases are **unit-testable** vs which -> **need a real repo server** (integration). The output is a checklist of -> combinations so we stop discovering broken-but-valid cases one GitHub issue -> at a time. +> This matrix enumerates the inputs to the one function under test - +> `buildManifestRequestForSource` in `pkg/reposerverextract/extract.go` - and the +> repo-server behaviour each must satisfy. It is organised by **what the code +> decides**, not by the YAML shape of the Application. > -> Test-name convention: each implemented row maps to `TestSourceMatrix_Table...`, -> keyed by the row id (e.g. A11 -> `TestSourceMatrix_TableA11_*`, the B-table rows -> are subtests named `B1`/`B2`/... inside `TestSourceMatrix_TableB_MultiSource_Routing`). +> Rows whose status is ✅ have a named passing test; 🟥 SKIP rows are confirmed +> latent bugs with opt-in failing repros; ⏭️ rows are integration-only skipped +> placeholders; ⬜ rows are gaps with no test yet. +> +> Goal: stop discovering broken-but-valid cases one GitHub issue at a time by +> making the whole input space explicit and backing each cell with a test. ## Why this exists 7 of the last 10 issues were repo-server-api bugs (#416, #426, #432, #438, #441, #446, #448). They are all the same shape: `buildManifestRequestForSource` makes a **routing decision** across several independent dimensions, and we only -ever verified the exact points users happened to hit. This doc makes the whole -input space explicit. - -## How the code routes today (the thing under test) +ever verified the exact points users happened to hit. -Entry point: `renderApp` -> `splitSources` -> `buildManifestRequestForSource` -(in `pkg/reposerverextract/extract.go`). +The original version of this doc was organised by Application *shape* +(single-source `A`, multi-source `B`, ...). That grouping conflated three +different axes and scattered the same logical decision (e.g. "must we stream the +branch root because a value file escapes the chart dir?") across three letters. +After auditing the real repo-server source, the doc was **re-organised around the +decision the code makes**. See "Mapping from the old A-H scheme" at the bottom. -There are three possible outcomes ("strategies") for each **content source**: +## How the code routes (the thing under test) -| Strategy | How we call the repo server | When | -| --- | --- | --- | -| **R = Remote RPC** | `GenerateManifest` (unary). Repo server git-fetches everything itself. `RefSources` populated if there are refs. `streamDir == ""`. | Files are not available locally (remote chart, cross-repo source, external ref). | -| **S = Stream** | `GenerateManifestWithFiles` (streaming a tarball). `streamDir` = chart dir or branch root. | Same-repo local source, no refs (or refs handled by streaming). | -| **S+refs = Stream with .refs** | Stream a temp tree: content dir + `.refs//`, with `$ref/...` valueFiles rewritten to relative paths. | Same-repo local `path:` source whose same-repo `$ref` sources are also local. | - -The tests observe **four** distinct outcomes via the returned `streamDir`: -`remote` (`streamDir == ""`), `stream(chart-dir)` (narrowed, `Path` cleared), -`stream(branch-root)` (`streamDir == branchFolder`, `Path` kept), and -`stream(.refs)` (temp tree containing a `.refs/` dir). See `streamStrategy` / -`classifyStrategy` in `source_matrix_a_test.go`. +Entry point: `renderApp` -> `splitSources` -> `buildManifestRequestForSource`. `splitSources` first splits the source list into: - **content sources** - produce manifests (`path != "" || chart != "" || ref == ""`). - **ref-only sources** - exist solely to provide `$ref` value files (`ref != ""`). (A source with BOTH `ref` and `path` counts as content AND ref - GH#401.) -## The dimensions of the input space - -A single Application is the product of these axes. Not every combination is -valid; invalid ones are noted. - -1. **Cardinality** - `source` (single) vs `sources` (multi). `ApplicationSet` - reads from `spec.template.spec` but is otherwise identical. -2. **Primary source type** - what the content source is: - - `path:` local **directory** of plain manifests / jsonnet (`directory:`) - - `path:` local **Helm chart** (dir has `Chart.yaml`) - - `path:` local **Kustomize** (dir has `kustomization.yaml`) - - `path:` + **plugin** (CMP) - - `chart:` **remote Helm chart** (from a Helm/OCI registry; no local files) -3. **Repo locality of the primary** - does `repoURL` match `--repo`/`--repo-regex`? - - **local** (matches -> files are in the branch checkout) - - **cross-repo** (does not match -> files are NOT local) -4. **Ref sources present?** - none / one-or-more `$ref` sources. -5. **Ref locality** - each ref's `repoURL`: local (same repo) vs external. -6. **valueFiles shape** (Helm only) - where the values live: - - none - - in-chart relative (`values.yaml`, `overrides/x.yaml`) - - out-of-chart relative (`../shared/x.yaml`) - - out-of-chart absolute (`/env/x.yaml`, resolved from repo root) - - `$ref/...` (points into a ref source) - - remote URL (`https://.../values.yaml`) -7. **Filesystem quirks in the streamed tree** - none / in-bounds symlink / - out-of-bounds symlink / **directory** symlink. -8. **Revision redirection** - top-level app vs app-of-apps **child**, with / - without `--redirect-target-revisions` (#446). - -The following axes were **added after the repo-server audit** (they were missing -from the original list and are the source of the F-H tables): - -9. **Explicit type vs on-disk type** - a source may set an explicit `helm:` / - `kustomize:` / `directory:` / `plugin:` block. `ExplicitType()` - (`types.go:3799`) **overrides** on-disk marker-file detection, and **two** - explicit blocks is a hard error ("multiple application sources defined"). Our - `isLocalHelmChart` / `isKustomizeSource` only look at on-disk files, so they - can disagree with the repo server. -10. **`.argocd-source.yaml` override files** - `.argocd-source.yaml` and - `.argocd-source-.yaml` are read **from the app/chart dir** and - merge-patched onto the source **before** type detection - (`mergeSourceParameters`, `repository.go:1872`). They can inject - `helm.valueFiles` (incl. out-of-chart!), `helm.parameters`, - `directory.recurse`, or even a whole tool block. Our tool does not read them - at all. -11. **`helm.fileParameters`** - `fileParameters[].path` is a **first-class ref - candidate** exactly like `valueFiles` (`repository.go:571-575`) and can be - `$ref/...`, out-of-chart, or remote. Our `rewriteRefValueFiles` only rewrites - `ValueFiles`. -12. **value-file resolution variants** (Helm) beyond axis 6: env-var - placeholders (`env.Envsubst`), glob patterns (lexical order = merge order), - disallowed URL scheme (hard error), `ignoreMissingValueFiles` true/false - (zero-glob -> gRPC NotFound). Allowed schemes come from - `HelmOptions.ValuesFileSchemes` (argocd-cm); nil = **no** remote allowed. -13. **Kustomize / Directory external-file pulls** - Kustomize `components:` may - reference dirs **outside** the app dir and are **not** repoRoot-bounded - (unlike value files); `--enable-helm` build option pulls remote charts; - jsonnet `libs:` are **repoRoot-relative** and can import cross-app. - -Axes 6-8 only bite once a case is already on a **Stream** strategy, which is -why they produced separate bugs. Axes 9-13 can bite on **any** strategy and are -largely unmodeled today. - -## Legend - -- Expected strategy: **R** / **S** / **S+refs** (see table above). -- Testability: - - **U** = unit-testable today against `buildManifestRequestForSource` - (assert chosen strategy + request fields + which files end up in the - tarball). No repo server needed. - - **I** = needs a real repo server (the failure happens *inside* it) -> - integration test (branch-N mechanism). -- Status: ✅ covered by a matrix test (passing) · ⏭️ integration-only, present - as a skipped placeholder (real coverage belongs in branch-N) · 🟥 known-broken - (open issue) · ⬜ no test yet (gap) · ⚠️ partial. - ---- +For each content source, the function chooses one of four observable outcomes, +exposed via the returned `streamDir` and asserted by `classifyStrategy` / +`streamStrategy` in `strategy_test.go`: -## A. Single source (`spec.source`) - -| # | Primary type | Repo | valueFiles | Expected | Test | Status | Notes / test | -| --- | --- | --- | --- | --- | --- | --- | --- | -| A1 | local dir (manifests/jsonnet) | local | - | S (branch root, Path set) | U | ✅ | `TableA` row A1. Plain non-chart dir falls through to branch root. | -| A2 | local Helm chart | local | none | S (chart dir, Path cleared) | U | ✅ | `TableA` row A2 (also `..._SingleSource_LocalChart`). | -| A3 | local Helm chart | local | in-chart | S (chart dir) | U | ✅ | `TableA` row A3 (also `..._LocalChart_InChartValueFile_StreamsChartDir`). | -| A4 | local Helm chart | local | out-of-chart relative `../` | S (branch root) | U | ✅ | `TableA` row A4 (also `..._LocalChart_OutOfChartRelativeValueFile`). | -| A5 | local Helm chart | local | out-of-chart absolute `/` | S (branch root) | U | ✅ | `TableA` row A5 (the v0.2.10 fix, #444). | -| A6 | local Helm chart | local | remote URL | S (chart dir; URL ignored) | U | ✅ | `TableA` row A6 - asserts URL valueFiles do not force the branch root. | -| A7 | local Kustomize | local | - | S (branch root, Path set) | U | ✅ | `TableA` row A7 (also `..._SingleSource_Kustomize_StreamsBranchRoot`). | -| A8 | local Kustomize w/ `helmCharts` | local | - | S (branch root) | U | ✅ | `TableA` row A8 (also `..._SingleSource_KustomizeWithHelm_StreamsBranchRoot`). | -| A9 | local chart, in-bounds symlink in tree | local | any | S, render OK | U+I | ✅/⏭️ | `TableA9_InBoundsSymlink_RoutesAndExcludesUnrelated` asserts the tarball excludes unrelated symlinks (U). The repo server's symlink gate (#438) is the I half - integration. | -| A10 | local chart, **out-of-bounds** file symlink | local | any | render fails today; **#438** | I | ⏭️ | `TableA10_OutOfBoundsSymlink_Integration` (skipped placeholder). Repo-server rejects "out-of-bounds symlinks"; only observable in a real render. | -| A11 | local chart, **directory** symlink inside chart | local | any | render OK (**#448** fixed by #449) | U+I | ✅ | `TableA11_DirectorySymlinkInChart_RoutesSuccessfully` - routing no longer errors (EISDIR fixed). `copyDir` dir-symlink following also covered by `TestCopyDir_FollowsDirectorySymlink`. | -| A12 | local plugin (CMP) `path:` | local | - | S (branch root, Path set) | U | ✅ | `TableA` row A12. Plugin sources have no Chart.yaml/kustomization -> branch root. (branch-16 covers CMP end-to-end.) | -| A13 | remote Helm `chart:` | external | none | R (`streamDir==""`) | U | ✅ | `TableA` row A13 (also `..._SingleSource_ExternalChart_NoRefs`). | -| A14 | remote Helm `chart:` (OCI) | external | none | R | U | ✅ | `TableA` row A14 - `oci://` registry chart, same routing as A13. | -| A15 | local `path:` source | **cross-repo** | - | R (`streamDir==""`) | U | ✅ | `TableA` row A15. Path is **kept** (not cleared) - the repo server resolves it inside the fetched foreign repo. | - -## B. Multi-source (`spec.sources`) - -| # | Primary type | Primary repo | Ref(s) | valueFiles | Expected | Test | Status | Notes | -| --- | --- | --- | --- | --- | --- | --- | --- | --- | -| B1 | local Helm chart | local | one local `$ref` | `$ref/...` | S+refs (rewrite paths) | U | ✅ | `TableB` subtest B1 (also `..._MultiSource_LocalChart_WithRef_RewritesValueFiles`). | -| B2 | local Helm chart | local | local `$ref` w/ both ref+path | `$ref/...` | S+refs | U | ✅ | `TableB` subtest B2. The ref+path source, routed as content, still streams `.refs` (it sees itself in the ref list). | -| B3 | local Helm chart | local | one **external** `$ref` | `$ref/...` | R + RefSources | U | ✅ | `TableB` subtest B3 (also `..._SameRepoPrimaryWithExternalRef_UsesRemoteRPC`, #426). | -| B4 | local `path:` (non-chart) | local | local `$ref` | n/a | S+refs | U | ✅ | `TableB` subtest B4 - plain manifests primary + local ref still synthesizes a `.refs` tree. | -| B5 | remote Helm `chart:` | external | one local `$ref` | `$ref/...` | S+refs via puller (or R+RefSources without one) | U+I | ✅ | **#441 fixed by #442.** `TableB` subtests B5 (with `fakeChartPuller` -> pulls chart, streams `.refs`, clears Chart) and B5b (no puller -> R + RefSources). Real registry pull is the I half. | -| B6 | remote Helm `chart:` | external | one external `$ref` | `$ref/...` | R + RefSources | U | ✅ | `TableB` subtest B6 (also `..._ExternalChart_WithRef_UsesRemoteRPC`). External ref forces R even with a puller present. | -| B7 | remote Helm `chart:` | external | local `$ref` w/ both ref+path | `$ref/...` | R + RefSources | U | ✅ | `TableB` subtest B7 (also `..._ApplicationSet_ExternalChart_WithRef`). | -| B8 | local Helm chart | local | local `$ref`, value file is out-of-chart abs `/` | mixed | S+refs (must also include out-of-chart file) | U | ✅ | `TableB` subtest B8 - the A5 (out-of-chart abs) + refs interaction; `$ref` rewritten, out-of-chart abs file reachable. | -| B9 | two local content sources (no ref) | local | - | per-source | S each (one request per content source) | U | ✅ | `TableB` subtest B9 (also `..._MultipleContentSources_BuildsOneRequestEach`). | -| B10 | local primary + external primary (mixed content) | mixed | - | - | per-source: S for local, R for external | U | ✅ | `TableB` subtest B10 - each content source routed independently. | -| B11 | ref-only source with **no path** (points at repo root) | local | local `$ref` (path="") | `$ref/x` | S+refs (ref dir = branch root) | U | ✅ | `TableB` subtest B11 - direct assertion that `$ref` with no path resolves against the repo root. | - -## C. Cardinality / kind variants - -| # | Case | Expected | Test | Status | Notes | -| --- | --- | --- | --- | --- | --- | -| C1 | `ApplicationSet` (sources under `spec.template.spec`) | same routing as the equivalent Application | U | ✅ | `TableC1_ApplicationSet_RoutesLikeApplication` - builds the same logical app as both kinds and asserts identical strategy + RefSources keys. | -| C2 | app with neither `source` nor `sources` | error | U | ✅ | `TableC_DegenerateTopologies_Error` subtest C2 (also `..._NoSource_ReturnsError`). | -| C3 | all sources are ref-only (no content) | error | U | ✅ | `TableC_DegenerateTopologies_Error` subtests C3 (all ref-only) and C3b (empty `sources: []`) - direct assertions. | - -## D. App-of-apps / traversal (orthogonal to the source matrix) +| Strategy | `streamDir` | How we call the repo server | When | +| --- | --- | --- | --- | +| **remote** | `""` | `GenerateManifest` (unary). Repo server git-fetches everything itself. `RefSources` populated if there are refs. | Files not available locally (remote chart, cross-repo source, external ref). | +| **stream(chart-dir)** | chart dir | `GenerateManifestWithFiles` (tarball), narrowed to the chart dir; `Path` cleared. | Same-repo local Helm chart, all files inside the chart dir. | +| **stream(branch-root)** | branch folder | `GenerateManifestWithFiles`, whole branch root; `Path` kept. | Same-repo local source whose files (or value files) live outside a single chart dir. | +| **stream(.refs)** | temp dir w/ `.refs/` | `GenerateManifestWithFiles` of a synthesized tree: content dir + `.refs//`, `$ref/...` paths rewritten to relative. | Same-repo local `path:`/chart source whose same-repo `$ref` sources are also local. | + +## The seven decision categories + +Each category below corresponds to one part of the decision the code makes (or +must make). The tables that follow are grouped by category. + +1. **STR - Strategy selection.** The core routing outcome (remote / chart-dir / + branch-root / .refs), driven by: `chart:` vs `path:`, repo locality + (`repoURL` vs `--repo`), and whether refs are present + their locality. + Cardinality (`source` vs `sources`, Application vs ApplicationSet) collapses + into "are there ref sources?" and is verified here as a parity check. +2. **TYP - Source-type resolution.** How the repo server decides + Helm/Kustomize/Directory/Plugin: on-disk marker files, an **explicit** + `helm:`/`kustomize:`/`directory:`/`plugin:` block (which *overrides* + detection), the `.argocd-source.yaml` override file, and the multi-block + error. +3. **REACH - File reachability.** The single question "must we stream the branch + root because some required file lives outside the chart dir?" - across + *every* way a file can escape: relative `../`, absolute `/`, `$ref`, glob, + env-subst, remote URL, **valueFiles AND fileParameters AND + override-injected**, plus symlinks. +4. **REF - Ref handling.** Staging `.refs/`, rewriting `$ref` paths, local vs + external refs, the remote-chart-with-local-refs puller. +5. **REQ - Request-content invariants.** Fields that must be set on *every* + request regardless of strategy: `KubeVersion`/`ApiVersions` (#432), + `ProjectName`/`ProjectSourceRepos` (#416). +6. **TRV - Traversal.** App-of-apps children: routing parity with seed apps and + `--redirect-target-revisions` handling (#446). +7. **NEG - Negatives / unsupported.** Degenerate topologies (no content source) + and combinations the repo server explicitly rejects. -| # | Case | Expected | Test | Status | Notes | -| --- | --- | --- | --- | --- | --- | -| D1 | child app source = local | child rendered like a normal app | U | ✅ | `TableD1_ChildApplication_RoutesLikeSeed` - a discovered child routes through `buildManifestRequestForSource` like a seed (local chart -> S chart dir, remote chart -> R). | -| D2 | child app, revision **not** in `--redirect-target-revisions` | child left on its pinned revision | U | ✅ | **#446 fixed by #447.** `TableD2_ChildHonorsRedirectTargetRevisions` (in-list -> redirected, out-of-list -> untouched, empty -> redirect all). Also `appofapps_test.go` `BuildChildApplication_*`. | -| D3 | child app source pinned to a ref that only exists remotely | render from remote, not redirected | I | ⏭️ | `TableD3_ChildRemoteOnlyRef_Integration` (skipped placeholder). Consequence of D2; the remote-only fetch is only observable in a real render. | +## Legend -## E. Cross-cutting request-content correctness (not routing, but repo-server-api specific) +- Testability: **U** = unit-testable against `buildManifestRequestForSource` + (assert strategy + request fields + tarball contents); **I** = needs a real + repo server (failure happens *inside* it) -> integration (branch-N). +- Status: ✅ covered by a named, **passing** test · 🟥 SKIP known-bug repro, + skipped by default but executable with `RUN_KNOWN_BUG_TESTS=1` · ⏭️ + integration-only skipped placeholder · ⬜ gap, no test yet · ⚠️ partial. -| # | Case | Expected | Test | Status | Notes | -| --- | --- | --- | --- | --- | --- | -| E1 | `ManifestRequest.ApiVersions` / `KubeVersion` populated | both set from the cluster | U | ✅ | **#432 (fixed v0.2.9).** `TableE_RequestInvariants_AllStrategies` asserts both on all three strategies (R, S chart-dir, S+refs); every A/B row asserts them too. | -| E2 | `ProjectName=default`, `ProjectSourceRepos=["*"]` on every request | permissive, so helm-build errors aren't masked as permission errors | U | ✅ | **#416.** `TableE_RequestInvariants_AllStrategies` asserts `assertDefaultProjectFields` on R / S / S+refs; every A/B row asserts it too. | -| E3 | transient helm-dep build error surfaced (not swallowed) | original error preserved | I | ⏭️ | `TableE3_HelmDepBuildErrorSurfaced_Integration` (skipped placeholder). **#416** - the surfaced message is only observable against a real repo server (E2 is the unit-level precondition). | +> **Note on the 🟥 SKIP rows.** Every matrix row has a named test. The five 🟥 +> SKIP rows (TYP5, REACH5, REACH6, REACH11, REF13) are real tests asserting the +> repo-server-correct behavior; they would fail today because our tool does not +> yet produce it. Normal `go test ./pkg/reposerverextract/...` stays green. Run +> `RUN_KNOWN_BUG_TESTS=1 go test ./pkg/reposerverextract/...` to execute the +> failing repros. They live in `gaps_test.go`. --- -## Repo-server audit findings (NEW - not yet implemented) - -The tables above were derived from *our* model of the repo server. Reading the -actual repo-server source (`argo-cd/reposerver/repository/repository.go`) -surfaced input dimensions the original axes missed. Tables **F-H** below -enumerate them. **Two rows (F4, H1) are confirmed latent bugs in our tool**, not -just coverage gaps. - -> ⚠️ None of the F-H rows have tests yet. They are the real output of "does the -> matrix cover all cases?" - the answer was **no**. - -### F. Source-type detection / overrides +## STR - Strategy selection -| # | Case | Expected | Test | Status | Notes (repo-server ref) | -| --- | --- | --- | --- | --- | --- | -| F1 | dir has `Chart.yaml` but manifest sets `directory:` explicitly | render as **Directory**, not Helm | U | ⬜ | `ExplicitType()` wins over on-disk (`types.go:3799`). Our `isLocalHelmChart` ignores the explicit block -> may narrow to chart dir wrongly. | -| F2 | dir has `kustomization.yaml` but manifest sets `helm:` explicitly | render as **Helm** | U | ⬜ | Same precedence. Our `isKustomizeSource` would keep branch root; repo server renders Helm. | -| F3 | manifest sets **both** `helm:` and `kustomize:` | **hard error** "multiple application sources defined" | U/I | ⬜ | `ExplicitType()` errors (`types.go:3816`). Our tool streams something instead of failing. Negative case. | -| F4 | **`.argocd-source.yaml` in chart dir adds out-of-chart `helm.valueFiles`** | must stream **branch root** (file is outside chart) | U+I | 🟥 | **CONFIRMED BUG.** `mergeSourceParameters` (`repository.go:1872`) injects valueFiles our `hasOutOfChartValueFile` never sees (it reads only the manifest). We narrow to chart dir -> repo server render fails (missing file). Same class as #444. | -| F5 | `.argocd-source-.yaml` injects a `helm:`/`kustomize:` block | type changes accordingly | U+I | ⬜ | Override applied **before** type detection. Our tool ignores override files entirely. | -| F6 | dir has **both** `Chart.yaml` and `kustomization.yaml` | repo-server discovery picks **Kustomize** | U | ⬜ | `discovery.go:65` overwrites Helm with Kustomize. Our `buildStreamDirForLocalSource` checks Kustomize first too (branch root) - verify parity. | -| F7 | source type disabled via `EnabledSourceTypes` | falls back to **Directory** | I | ⬜ | `IsManifestGenerationEnabled` (`repository.go:1938`). We never set EnabledSourceTypes; document behavior. | - -### G. Helm value-file resolution variants (extends axis 6) - -| # | Case | Expected | Test | Status | Notes (repo-server ref) | -| --- | --- | --- | --- | --- | --- | -| G1 | valueFile with env placeholder `$ENV/x.yaml` (via `helm.valuesObject`/env) | resolved after `env.Envsubst` | U+I | ⬜ | `repository.go:1502`. Our `hasOutOfChartValueFile` does not envsubst before classifying -> could misjudge chart-dir vs branch-root. | -| G2 | valueFile **glob** `env/*.yaml` | expanded lexically; order = merge precedence | U+I | ⬜ | `isGlobPath` + `doublestar` (`repository.go:1516`). A glob can match out-of-chart files -> our narrowing logic ignores globs. | -| G3 | valueFile remote URL with **disallowed** scheme | **hard error** | I | ⬜ | `isURLSchemeAllowed` (`resolved.go:53`). Schemes from `HelmOptions.ValuesFileSchemes`; nil = none allowed. | -| G4 | missing valueFile, `ignoreMissingValueFiles: true` vs `false` | skip vs fail | U+I | ⬜ | `repository.go:1542`. Affects whether a missing out-of-chart file even matters. | -| G5 | valueFile leading-slash `/env/x.yaml` (repo-root relative) | resolved from repo root | U | ⚠️ | Covered by A5/B8 for the *routing* outcome, but not the resolution semantics directly. | - -### H. Helm `fileParameters` (parallel to valueFiles - largely unmodeled) - -| # | Case | Expected | Test | Status | Notes (repo-server ref) | +| # | Source | Repo | Refs | Expected strategy | Test | U/I | Status | +| --- | --- | --- | --- | --- | --- | --- | --- | +| STR1 | local Helm chart | local | none | stream(chart-dir), `Path` cleared | `Strategy` row STR1 | U | ✅ | +| STR2 | remote `chart:` | external | none | remote (`streamDir==""`) | `Strategy` row STR2 | U | ✅ | +| STR3 | remote `chart:` (OCI) | external | none | remote | `Strategy` row STR3 | U | ✅ | +| STR4 | local `path:` | **cross-repo** | none | remote; `Path` **kept** (resolved in foreign repo) | `Strategy` row STR4 | U | ✅ | +| STR5 | ApplicationSet (sources under `spec.template.spec`) | - | - | identical routing to the equivalent Application | `Strategy_AppSetRoutesLikeApp` | U | ✅ | + +## TYP - Source-type resolution + +| # | Case | Expected | Test | U/I | Status | Notes (repo-server ref) | +| --- | --- | --- | --- | --- | --- | --- | +| TYP1 | local dir of plain manifests / jsonnet | stream(branch-root), `Path` set | `Strategy` row TYP1 | U | ✅ | Non-chart/non-kustomize dir falls through to branch root. | +| TYP2 | local Kustomize | stream(branch-root) | `Strategy` row TYP2 | U | ✅ | `kustomization.yaml` detected. | +| TYP3 | local Kustomize w/ `helmCharts` (Chart.yaml present) | stream(branch-root) | `Strategy` row TYP3 | U | ✅ | Kustomize wins over Helm when both present. | +| TYP4 | local plugin (CMP) `path:` | stream(branch-root), `Path` set | `Strategy` row TYP4 | U | ✅ | No Chart.yaml/kustomization -> branch root. branch-16 covers CMP end-to-end. | +| TYP5 | dir has `Chart.yaml` but manifest sets `directory:` explicitly | render as **Directory** | `TYP5_ExplicitDirectoryOverChartYaml` | U | 🟥 SKIP | `ExplicitType()` wins over on-disk (`types.go:3799`). Our `isLocalHelmChart` ignores the explicit block -> narrows as Helm. **Skipped by default; fails with `RUN_KNOWN_BUG_TESTS=1`.** | +| TYP6 | dir has `kustomization.yaml` but manifest sets `helm:` explicitly | stream(branch-root) | `TYP6_ExplicitHelmOverKustomization` | U | ✅ | Passes - we stream branch root for non-chart dirs anyway. | +| TYP7 | manifest sets **both** `helm:` and `kustomize:` | **hard error** "multiple application sources defined" | `TYP7_MultipleExplicitTypes_Integration` | I | ⏭️ | `ExplicitType()` errors inside the repo server (`types.go:3816`). | +| TYP8 | `.argocd-source-.yaml` injects a `helm:`/`kustomize:` block | type changes accordingly | `TYP8_ArgocdSourceInjectsType_Integration` | I | ⏭️ | Override applied **before** type detection inside the repo server (`repository.go:1872`). | +| TYP9 | dir has **both** `Chart.yaml` and `kustomization.yaml` | discovery picks **Kustomize** | `TYP9_BothChartAndKustomization_PicksKustomize` | U | ✅ | Passes - our `isKustomizeSource` check first matches the repo server (`discovery.go:65`). | +| TYP10 | source type disabled via `EnabledSourceTypes` | falls back to **Directory** | `TYP10_DisabledSourceTypeFallsBackToDirectory_Integration` | I | ⏭️ | `IsManifestGenerationEnabled` (`repository.go:1938`). We never set EnabledSourceTypes. | + +## REACH - File reachability (chart-dir vs branch-root) + +The fast path narrows the stream to the chart dir only when every required file +is inside it (`hasOutOfChartValueFile` -> false). Each row is a way a file may +or may not escape. + +| # | Case | Expected | Test | U/I | Status | Notes (repo-server ref) | +| --- | --- | --- | --- | --- | --- | --- | +| REACH1 | Helm chart, in-chart relative values (`values.yaml`) | stream(chart-dir) | `Strategy` row REACH1 | U | ✅ | | +| REACH2 | Helm chart, out-of-chart relative values (`../shared/x`) | stream(branch-root) | `Strategy` row REACH2 | U | ✅ | | +| REACH3 | Helm chart, out-of-chart absolute values (`/env/x`) | stream(branch-root) | `Strategy` row REACH3 | U | ✅ | Leading-slash = repo-root-relative. The v0.2.10 fix (#444). | +| REACH4 | Helm chart, remote-URL values | stream(chart-dir); URL ignored for narrowing | `Strategy` row REACH4 | U | ✅ | URL schemes skipped by `hasOutOfChartValueFile`. | +| REACH5 | **`.argocd-source.yaml` adds out-of-chart `helm.valueFiles`** | must stream(branch-root) | `REACH5_ArgocdSourceOutOfChartValues_BUG` | U+I | 🟥 SKIP | **CONFIRMED BUG.** `mergeSourceParameters` injects valueFiles our `hasOutOfChartValueFile` never sees (it reads only the manifest). We narrow -> render fails. Same class as #444. **Skipped by default; fails with `RUN_KNOWN_BUG_TESTS=1`.** | +| REACH6 | valueFile with env placeholder `$ENV/x.yaml` | resolved after `env.Envsubst`, then classified | `REACH6_EnvPlaceholderValueFile` | U+I | 🟥 SKIP | `repository.go:1502`. We treat `$ARGOCD_ENV_*` as a `$ref` and skip it when classifying -> narrow. **Skipped by default; fails with `RUN_KNOWN_BUG_TESTS=1`.** | +| REACH7 | valueFile **glob** `env/*.yaml` matching out-of-chart files | stream(branch-root) | `REACH7_GlobOutOfChartValueFile` | U+I | ✅ | Passes - the `../` prefix is caught by the existing relative-escape check. | +| REACH8 | valueFile remote URL with **disallowed** scheme | **hard error** | `REACH8_DisallowedUrlScheme_Integration` | I | ⏭️ | `isURLSchemeAllowed` (`resolved.go:53`). Schemes from `HelmOptions.ValuesFileSchemes`; nil = none. | +| REACH9 | missing valueFile, `ignoreMissingValueFiles` true vs false | skip vs fail | `REACH9_IgnoreMissingValueFiles` | U+I | ✅ | Passes - an out-of-chart value file forces branch root regardless of the flag. | +| REACH10 | leading-slash valueFile resolution semantics | resolved from repo root | (in REACH3/REF8) | U | ⚠️ | Routing covered; resolution semantics not asserted directly. | +| REACH11 | `helm.fileParameters[].path` out-of-chart relative `../x` | must stream(branch-root) | `REACH11_FileParameterOutOfChart_BUG` | U+I | 🟥 SKIP | fileParameters resolve like valueFiles (`repository.go:1343`); `hasOutOfChartValueFile` ignores them. **Skipped by default; fails with `RUN_KNOWN_BUG_TESTS=1`** (same blind spot as REF13). | +| REACH12 | `helm.fileParameters[].path` remote URL | fetched by repo server | `REACH12_FileParameterRemoteUrl_Integration` | I | ⏭️ | Same scheme rules as valueFiles. | +| REACH13 | local chart, unrelated **in-bounds** symlink elsewhere in repo | routes; tarball excludes unrelated files | `Strategy_InBoundsSymlink_*` | U+I | ✅/⏭️ | Tarball exclusion is U; the repo server symlink gate (#438) is the I half. | +| REACH14 | local chart, **out-of-bounds** file symlink | render rejected by repo server | `Strategy_OutOfBoundsSymlink_Integration` | I | ⏭️ | #438 - only observable in a real render. | +| REACH15 | local chart, **directory** symlink inside chart | routes without error | `Strategy_DirectorySymlinkInChart_*` | U+I | ✅ | #448 fixed by #449. `copyDir` following also covered by `TestCopyDir_FollowsDirectorySymlink`. | + +## REF - Ref handling + +| # | Primary | Primary repo | Ref(s) | Expected | Test | U/I | Status | +| --- | --- | --- | --- | --- | --- | --- | --- | +| REF1 | local Helm chart | local | one local `$ref` | stream(.refs), values rewritten | `Refs` row REF1 | U | ✅ | +| REF2 | local Helm chart | local | local `$ref` w/ both ref+path (GH401) | stream(.refs) | `Refs` row REF2 | U | ✅ | +| REF3 | local Helm chart | local | one **external** `$ref` | remote + RefSources | `Refs` row REF3 | U | ✅ | +| REF4 | local plain dir (non-chart) | local | local `$ref` | stream(.refs) | `Refs` row REF4 | U | ✅ | +| REF5 | remote `chart:` | external | one local `$ref` (with puller) | stream(.refs) via puller; Chart cleared | `Refs` row REF5 | U+I | ✅ | +| REF6 | remote `chart:` | external | one local `$ref` (no puller) | remote + RefSources | `Refs` row REF6 | U | ✅ | +| REF7 | remote `chart:` | external | one external `$ref` | remote + RefSources | `Refs` row REF7 | U | ✅ | +| REF8 | remote `chart:` | external | local `$ref` w/ both ref+path | remote + RefSources | `Refs` row REF8 | U | ✅ | +| REF9 | local Helm chart | local | local `$ref` + out-of-chart abs value | stream(.refs), out-of-chart file reachable | `Refs` row REF9 | U | ✅ | +| REF10 | two local content sources | local | none | stream each independently | `Refs` row REF10 | U | ✅ | +| REF11 | local + external content (mixed) | mixed | none | stream local, remote external | `Refs` row REF11 | U | ✅ | +| REF12 | ref-only source with **no path** (repo root) | local | local `$ref` (path="") | stream(.refs), ref dir = branch root | `Refs` row REF12 | U | ✅ | +| REF13 | local chart | local | **`helm.fileParameters[].path: $ref/foo`** | path rewritten like a valueFile; ref staged | `REF13_FileParameterRef_BUG` | U+I | 🟥 SKIP | **CONFIRMED BUG.** Repo server treats fileParameters as ref candidates (`repository.go:571-575`); our `rewriteRefValueFiles` only rewrites `ValueFiles`, never `FileParameters` -> ref staged but path unrewritten -> render fails. Same class as #441/#426. **Skipped by default; fails with `RUN_KNOWN_BUG_TESTS=1`.** | + +## REQ - Request-content invariants (every strategy) + +| # | Case | Expected | Test | U/I | Status | | --- | --- | --- | --- | --- | --- | -| H1 | multi-source, **`helm.fileParameters[].path: $ref/foo`** | path rewritten like a valueFile; ref staged | U+I | 🟥 | **CONFIRMED BUG.** Repo server treats fileParameters as ref candidates (`repository.go:571-575`); our `rewriteRefValueFiles` only rewrites `ValueFiles`, never `FileParameters` -> ref staged but path unrewritten -> render fails. Same class as #441/#426. | -| H2 | `fileParameters[].path` out-of-chart relative `../x` | branch root must be streamed | U+I | ⬜ | fileParameters resolve like valueFiles (`repository.go:1343`). Our `hasOutOfChartValueFile` ignores fileParameters -> may narrow wrongly. | -| H3 | `fileParameters[].path` remote URL | fetched by repo server | I | ⬜ | Same scheme rules as valueFiles. | +| REQ1 | `KubeVersion` / `ApiVersions` populated on R / S / S+refs | both set from the cluster | `RequestInvariants_AllStrategies` | U | ✅ (#432) | +| REQ2 | `ProjectName=default`, `ProjectSourceRepos=["*"]` on R / S / S+refs | permissive, so helm-build errors aren't masked | `RequestInvariants_AllStrategies` | U | ✅ (#416) | +| REQ3 | transient helm-dep build error surfaced (not swallowed) | original error preserved | `HelmDepBuildErrorSurfaced_Integration` | I | ⏭️ (#416) | -### I. Kustomize / Directory external pulls (extends axis 13) +## TRV - Traversal (app-of-apps) -| # | Case | Expected | Test | Status | Notes (repo-server ref) | +| # | Case | Expected | Test | U/I | Status | | --- | --- | --- | --- | --- | --- | -| I1 | Kustomize `components:` referencing `../other-dir` (outside app) | branch root must be streamed | U+I | ⬜ | components are **not** repoRoot-bounded (`kustomize.go:337`). We always stream branch root for Kustomize, so this likely works - but unverified. | -| I2 | Kustomize `--enable-helm` build option + `helmCharts:` | pulls remote chart during build | I | ⬜ | `isHelmEnabled` (`kustomize.go:427`). Needs build options + network. | -| I3 | Directory jsonnet `libs:` referencing another repo dir | imports resolved repoRoot-relative | U+I | ⬜ | `repository.go:2263`. Cross-app import; branch root must be streamed. | +| TRV1 | child app source = local / remote | child routes through `buildManifestRequestForSource` like a seed | `Traversal_ChildRoutesLikeSeed` | U | ✅ | +| TRV2 | child honors `--redirect-target-revisions` (in/out/empty) | redirect / leave / redirect-all | `Traversal_ChildHonorsRedirectTargetRevisions` | U | ✅ (#446 fixed by #447) | +| TRV3 | child pinned to a ref that only exists remotely | render from remote, not redirected | `Traversal_ChildRemoteOnlyRef_Integration` | I | ⏭️ | -### Negative / invalid combinations (document, don't "fix") +## NEG - Negatives / unsupported -| # | Case | Expected | Notes | -| --- | --- | --- | --- | -| N1 | a `$ref` source that itself sets `chart:` | repo server **rejects** | "Helm charts are not yet not supported for 'ref' sources" (`repository.go:594`). | -| N2 | raw streamed single-source with `$ref` value file, no `.refs` staging | fails "failed to find repo" | Streaming path does not populate `gitRepoPaths` (`repository.go:1565`). Validates *why* our `.refs` staging + rewrite mechanism exists - and why it must cover fileParameters too (H1). | +| # | Case | Expected | Test | U/I | Status | Notes | +| --- | --- | --- | --- | --- | --- | --- | +| NEG1 | app with neither `source` nor `sources` | error | `Negatives_DegenerateTopologies` NEG1 | U | ✅ | | +| NEG2 | all sources are ref-only (no content) | error | `Negatives_DegenerateTopologies` NEG2 | U | ✅ | | +| NEG3 | empty `sources: []` | error | `Negatives_DegenerateTopologies` NEG3 | U | ✅ | | +| NEG4 | a `$ref` source that itself sets `chart:` | repo server **rejects** | `NEG4_RefSourceWithChart_Integration` | I | ⏭️ | "Helm charts are not yet not supported for 'ref' sources" (`repository.go:594`). | +| NEG5 | raw streamed single-source with `$ref` (no `.refs` staging) | fails "failed to find repo" | `NEG5_RawStreamedRefUnsupported_Integration` | I | ⏭️ | Streaming path does not populate `gitRepoPaths` (`repository.go:1565`). Validates *why* our `.refs` staging + rewrite exists - and why it must cover fileParameters (REF13). | --- ## What we found (honest summary) -Tables A-E are **complete and 1:1 with their rows**: 35 rows, each with a named -test. 9 passing test functions cover the unit rows; 3 skipped placeholders mark -the integration-only rows (A10, D3, E3). - -**Bugs found by the unit matrix alone (A-E): 0.** Every unit row passed against -`main`, or was an already-tracked issue. +**Bugs found by the unit matrix alone (the ✅ rows): 0.** Every implemented unit +row passed against `main`, or was an already-tracked issue. -**Bugs found by the repo-server audit (F-H): 2 confirmed, plus several +**Bugs found by auditing the repo-server source: 2 confirmed, plus several unmodeled dimensions.** This is the real payoff and it only appeared once we -stopped trusting our own model and read the repo-server source: - -- **F4** - `.argocd-source.yaml` can add out-of-chart `helm.valueFiles` that our - `hasOutOfChartValueFile` never sees (it inspects only the manifest), so we - narrow the stream to the chart dir and the render fails on the missing file. - Same class as #444. -- **H1** - `helm.fileParameters[].path: $ref/...` is a first-class ref candidate - in the repo server, but our `rewriteRefValueFiles` only rewrites `ValueFiles`, - so the ref is staged into `.refs/` while the fileParameter path is left +stopped trusting our own model and read the repo-server. All now have failing +tests in `gaps_test.go`: + +- **REACH5** (🟥 SKIP) - `.argocd-source.yaml` can add out-of-chart + `helm.valueFiles` that `hasOutOfChartValueFile` never sees (it inspects only + the manifest), so we narrow to the chart dir and the render fails. Same class + as #444. +- **REF13** (🟥 SKIP) - `helm.fileParameters[].path: $ref/...` is a first-class + ref candidate in the repo server, but `rewriteRefValueFiles` only rewrites + `ValueFiles`, so the ref is staged while the fileParameter path is left unrewritten and unresolvable. Same class as #441/#426. - -Both are **latent** (no open issue yet) - exactly the "broken-but-valid case we -discover one GitHub issue at a time" this exercise was meant to pre-empt. - -What the A-E exercise bought us regardless: - -1. **Regression net for recently-merged fixes.** Several fixes had no dedicated - regression test before; the matrix now pins them so they cannot silently - regress: #444 (A5 out-of-chart abs valueFiles), #442 (B5 remote chart + - same-repo ref), #449 (A11 directory symlink), #447 (D2/D3 children honor - `--redirect-target-revisions`). -2. **Closed real coverage gaps** the draft flagged as ⬜/⚠️ (untested-but-valid, - "where the next bug hides"): A1, A6, A12, A14, B4, B8, B10, B11, C1, and the - E2 invariant across **all three** strategies (the #416 root cause was that it - was only asserted on the streamed local-chart path). -3. **Flushed out four wrong assumptions** (caught as failing tests, then fixed): - - A15: cross-repo source keeps `Path` (the repo server resolves it in the - fetched foreign repo); it is **not** cleared. - - B2: a `ref`+`path` source, routed as content, still streams `.refs` (it - sees itself in the ref list) rather than degrading to a chart-dir stream. - - B5/B8: a `$ref` source with no `path` points at the **repo root**, so - `$cfg/values.yaml` resolves to `/values.yaml`. - - A11: on the single-source path the chart streams via the tarball compressor, - which preserves a directory symlink **as a symlink entry**; the - `copyDir`-follows-dir-symlink behavior (#449) is on the refs/slow path. +- **REACH11** (🟥 SKIP) - the broader form of the same blind spot: *any* + out-of-chart `fileParameters` path (not just `$ref`) is ignored when deciding + chart-dir vs branch-root, because `hasOutOfChartValueFile` only inspects + `ValueFiles`. +- **TYP5** (🟥 SKIP) - an explicit `directory:` block on a dir that has a + `Chart.yaml` is rendered as Directory by the repo server, but we narrow it as + a Helm chart (our type detection ignores explicit blocks). +- **REACH6** (🟥 SKIP) - an env-placeholder value file (`$ARGOCD_ENV_*`) is + treated as a `$ref` and skipped when classifying, so we may narrow when the + resolved path actually escapes the chart dir. + +All five are **latent** (no open issue yet) - exactly the "broken-but-valid case +we discover one GitHub issue at a time" this exercise was meant to pre-empt. + +The implemented rows still bought us a regression net (pinning #444, #442, #449, +#447, which had no dedicated test) and closed real coverage gaps, and flushed out +four wrong assumptions in our own model: +- STR4: cross-repo source keeps `Path` (the repo server resolves it in the + fetched foreign repo); it is **not** cleared. +- REF2: a `ref`+`path` source, routed as content, still streams `.refs` (it sees + itself in the ref list) rather than degrading to a chart-dir stream. +- REF5/REF9: a `$ref` source with no `path` points at the **repo root**, so + `$cfg/values.yaml` resolves to `/values.yaml`. +- REACH15: on the single-source path the chart streams via the tarball + compressor, which preserves a directory symlink **as a symlink entry**; the + `copyDir`-follows-dir-symlink behaviour (#449) is on the refs/slow path. ## Where the next real bug most likely hides (integration) -Unit tests structurally cannot see failures that happen **inside** the repo -server. The skipped placeholders are precisely those cases, and they are the +Unit tests cannot see failures that happen **inside** the repo server. The highest-value targets for proactive bug-hunting via branch-N integration: -- **A10 (#438)** - out-of-bounds file symlink rejected by the repo server's - symlink safety gate. Needs an out-of-bounds symlink fixture. -- **B5 (#441/#442)** - the *real* registry pull + same-repo ref render (the unit - test stubs the puller). -- **D3 (#446)** - a child pinned to a remote-only ref rendered by a real server. -- **E3 (#416)** - a transient helm-dependency build error surfaced with its - original message rather than masked. +- **REACH14 (#438)** - out-of-bounds file symlink rejected by the symlink gate. +- **REF5 (#441/#442)** - the *real* registry pull + same-repo ref render. +- **TRV3 (#446)** - a child pinned to a remote-only ref rendered by a real server. +- **REQ3 (#416)** - a transient helm-dependency build error surfaced unmasked. +- Plus the new ⬜ TYP / REACH / REF rows from the repo-server audit, several of + which (TYP5-TYP10, REACH6-REACH12) need a real render to confirm end-to-end. ## Test file layout -- `source_matrix_a_test.go` - Table A (single source) + the A9/A10/A11 symlink - rows. Hosts the shared helpers (`streamStrategy`, `classifyStrategy`, - `writeBranchFiles`, `replaceRepo`). -- `source_matrix_b_test.go` - Table B (multi-source), one big table-driven test - with per-row subtests B1-B11 (+ B5b). -- `source_matrix_cde_test.go` - Tables C, D, E. +- `strategy_test.go` - STR + TYP (on-disk) + REACH rows (the routing table and + the symlink tests). Hosts the shared helpers (`streamStrategy`, + `classifyStrategy`, `writeBranchFiles`, `replaceRepo`). +- `refs_test.go` - REF rows (multi-source / ref handling), one table-driven test + with per-row subtests. +- `crosscutting_test.go` - STR5 (appset parity), REQ, TRV, NEG. +- `gaps_test.go` - the repo-server-audit rows that were not in the original + tables: the 5 🟥 SKIP rows (skipped by default) and the integration-only ⏭️ + placeholders. Hosts the `routeSingle` helper. + +## Mapping from the old A-H scheme + +For anyone cross-referencing earlier commits or issues: + +| Old | New | +| --- | --- | +| A1 | TYP1 | +| A2 | STR1 | +| A3 / A4 / A5 / A6 | REACH1 / REACH2 / REACH3 / REACH4 | +| A7 / A8 | TYP2 / TYP3 | +| A9 / A10 / A11 | REACH13 / REACH14 / REACH15 | +| A12 | TYP4 | +| A13 / A14 / A15 | STR2 / STR3 / STR4 | +| B1..B11 (+B5b) | REF1..REF12 | +| C1 | STR5 | +| C2 / C3 / C3b | NEG1 / NEG2 / NEG3 | +| D1 / D2 / D3 | TRV1 / TRV2 / TRV3 | +| E1 / E2 / E3 | REQ1 / REQ2 / REQ3 | +| F1 / F2 / F3 / F4 / F5 / F6 / F7 | TYP5 / TYP6 / TYP7 / REACH5 / TYP8 / TYP9 / TYP10 | +| G1..G5 | REACH6..REACH10 | +| H1 / H2 / H3 | REF13 / REACH11 / REACH12 | +| N1 / N2 | NEG4 / NEG5 | diff --git a/pkg/reposerverextract/source_matrix_cde_test.go b/pkg/reposerverextract/crosscutting_test.go similarity index 82% rename from pkg/reposerverextract/source_matrix_cde_test.go rename to pkg/reposerverextract/crosscutting_test.go index 4e9a2002..e37210d4 100644 --- a/pkg/reposerverextract/source_matrix_cde_test.go +++ b/pkg/reposerverextract/crosscutting_test.go @@ -10,7 +10,7 @@ package reposerverextract // source, all-ref-only) fail loudly? // - Table D: app-of-apps traversal. A discovered child Application must route // through buildManifestRequestForSource exactly like a seed Application. -// (The --redirect-target-revisions behavior for children, D2/D3 / #446, is +// (The --redirect-target-revisions behavior for children, TRV2/TRV3 / #446, is // already covered by appofapps_test.go's BuildChildApplication_* tests and // is NOT duplicated here.) // - Table E: cross-cutting request-content correctness. The repo-server-api @@ -18,9 +18,9 @@ package reposerverextract // ProjectName/ProjectSourceRepos) must hold on ALL THREE strategies, not // just the streamed local-chart one that historically asserted them. // -// E3 (transient helm-dep build error surfaced, #416) only fails inside a real +// REQ3 (transient helm-dep build error surfaced, #416) only fails inside a real // repo server and is an integration concern; it is represented below as a -// skipped integration placeholder (TestSourceMatrix_TableE3_*) so every E-row +// skipped integration placeholder (TestSourceMatrix_RequestInvariants_HelmDepBuildErrorSurfaced_*) so every E-row // has a named test. import ( @@ -35,11 +35,11 @@ import ( // ── Table C: cardinality / kind ───────────────────────────────────────────── -// C1: an ApplicationSet whose sources live under spec.template.spec must split +// STR5: an ApplicationSet whose sources live under spec.template.spec must split // and route identically to the equivalent Application. We build the same // logical multi-source app in both shapes and assert the routing outcome // matches source-for-source. -func TestSourceMatrix_TableC1_ApplicationSet_RoutesLikeApplication(t *testing.T) { +func TestSourceMatrix_Strategy_AppSetRoutesLikeApp(t *testing.T) { const repo = "https://github.com/org/repo.git" applicationYAML := ` @@ -122,15 +122,15 @@ spec: assert.ElementsMatch(t, []string{"$ext"}, appRefKeys) } -// C2/C3: degenerate source topologies must fail loudly in splitSources rather +// NEG1/NEG2: degenerate source topologies must fail loudly in splitSources rather // than silently producing an empty/invalid request. -func TestSourceMatrix_TableC_DegenerateTopologies_Error(t *testing.T) { +func TestSourceMatrix_Negatives_DegenerateTopologies(t *testing.T) { cases := []struct { name string appYAML string }{ { - name: "C2 neither source nor sources -> error", + name: "NEG1 neither source nor sources -> error", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -140,7 +140,7 @@ spec: `, }, { - name: "C3 all sources are ref-only (no content) -> error", + name: "NEG2 all sources are ref-only (no content) -> error", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -157,7 +157,7 @@ spec: `, }, { - name: "C3b empty sources list -> error", + name: "NEG3 empty sources list -> error", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -180,17 +180,17 @@ spec: // ── Table D: app-of-apps traversal ────────────────────────────────────────── -// D1: a discovered child Application must route through +// TRV1: a discovered child Application must route through // buildManifestRequestForSource exactly like a seed Application. This ties the // app-of-apps path back into the source matrix: whatever a child's source is, // the same routing rules (stream local chart dir, remote chart -> remote RPC, // etc.) apply. // -// (D2 and D3 below cover the --redirect-target-revisions behavior for children, +// (TRV2 and TRV3 below cover the --redirect-target-revisions behavior for children, // #446. There is overlapping coverage in appofapps_test.go's // TestBuildChildApplication_* tests, but the matrix rows are kept here so every // row in SOURCE_MATRIX.md has a corresponding, named test in the matrix suite.) -func TestSourceMatrix_TableD1_ChildApplication_RoutesLikeSeed(t *testing.T) { +func TestSourceMatrix_Traversal_ChildRoutesLikeSeed(t *testing.T) { const repo = "https://github.com/org/repo.git" cases := []struct { @@ -201,7 +201,7 @@ func TestSourceMatrix_TableD1_ChildApplication_RoutesLikeSeed(t *testing.T) { wantChart string }{ { - name: "D1a child local chart -> stream(chart-dir)", + name: "TRV1a child local chart -> stream(chart-dir)", childYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -220,7 +220,7 @@ spec: wantChart: "", }, { - name: "D1b child remote chart -> remote RPC", + name: "TRV1b child remote chart -> remote RPC", childYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -270,16 +270,16 @@ spec: } } -// D2: a discovered child Application must honor --redirect-target-revisions +// TRV2: a discovered child Application must honor --redirect-target-revisions // exactly like a seed Application (#446, fixed by #447). A child source whose // targetRevision is in the redirect list is redirected to the target branch; a // source pinned to a revision outside the list is left untouched; an empty list // preserves the "redirect everything" default. // -// childAppManifestD2 builds a minimal discovered child whose single source is -// pinned to targetRevision. (Named with a D2 suffix to avoid colliding with the +// childAppManifestTRV2 builds a minimal discovered child whose single source is +// pinned to targetRevision. (Named with a TRV2 suffix to avoid colliding with the // childAppManifest helper in appofapps_test.go.) -func childAppManifestD2(repoURL, targetRevision string) unstructured.Unstructured { +func childAppManifestTRV2(repoURL, targetRevision string) unstructured.Unstructured { return unstructured.Unstructured{Object: map[string]any{ "apiVersion": "argoproj.io/v1alpha1", "kind": "Application", @@ -294,7 +294,7 @@ func childAppManifestD2(repoURL, targetRevision string) unstructured.Unstructure }} } -func TestSourceMatrix_TableD2_ChildHonorsRedirectTargetRevisions(t *testing.T) { +func TestSourceMatrix_Traversal_ChildHonorsRedirectTargetRevisions(t *testing.T) { const repoURL = "https://github.com/example/repo" cases := []struct { @@ -304,19 +304,19 @@ func TestSourceMatrix_TableD2_ChildHonorsRedirectTargetRevisions(t *testing.T) { wantRevision string }{ { - name: "D2a revision in list -> redirected to target branch", + name: "TRV2a revision in list -> redirected to target branch", pinnedRevision: "main", redirectRevisions: []string{"main", "HEAD"}, wantRevision: "target", }, { - name: "D2b revision NOT in list -> left untouched", + name: "TRV2b revision NOT in list -> left untouched", pinnedRevision: "feature-branch", redirectRevisions: []string{"main", "HEAD"}, wantRevision: "feature-branch", }, { - name: "D2c empty list -> redirect everything (default)", + name: "TRV2c empty list -> redirect everything (default)", pinnedRevision: "feature-branch", redirectRevisions: nil, wantRevision: "target", @@ -329,7 +329,7 @@ func TestSourceMatrix_TableD2_ChildHonorsRedirectTargetRevisions(t *testing.T) { require.NoError(t, err) targetBranch := git.NewBranch("target", git.Target) - manifest := childAppManifestD2(repoURL, tc.pinnedRevision) + manifest := childAppManifestTRV2(repoURL, tc.pinnedRevision) child, err := buildChildApplication( "argocd", manifest, "parent: root", git.Target, targetBranch, *selector, tc.redirectRevisions) require.NoError(t, err) @@ -344,26 +344,26 @@ func TestSourceMatrix_TableD2_ChildHonorsRedirectTargetRevisions(t *testing.T) { } } -// D3: a child Application whose source is pinned to a ref that only exists on +// TRV3: a child Application whose source is pinned to a ref that only exists on // the remote (not in the local checkout) must render from the remote rather -// than being redirected to the local branch. This is a consequence of D2/#446, +// than being redirected to the local branch. This is a consequence of TRV2/#446, // but the failure mode (the repo server cloning a remote-only ref) is only // observable against a real repo server, so it is an INTEGRATION row, not a // unit row. It is recorded here as a skipped placeholder so the matrix has a // named entry for every row in SOURCE_MATRIX.md; the real coverage lives in the // integration suite (branch-N). -func TestSourceMatrix_TableD3_ChildRemoteOnlyRef_Integration(t *testing.T) { - t.Skip("D3 is integration-only: a remote-only child ref must be rendered by a real repo server (branch-N), not a unit test") +func TestSourceMatrix_Traversal_ChildRemoteOnlyRef_Integration(t *testing.T) { + t.Skip("TRV3 is integration-only: a remote-only child ref must be rendered by a real repo server (branch-N), not a unit test") } // ── Table E: cross-cutting request-content correctness ─────────────────────── -// E1/E2: the repo-server-api request invariants must hold on EVERY strategy. +// REQ1/REQ2: the repo-server-api request invariants must hold on EVERY strategy. // Historically only the streamed local-chart path asserted KubeVersion/ // ApiVersions (#432) and ProjectName/ProjectSourceRepos (#416); this test // pins them across remote, stream(chart-dir) and stream(.refs) so a future // routing change cannot drop them on one path. -func TestSourceMatrix_TableE_RequestInvariants_AllStrategies(t *testing.T) { +func TestSourceMatrix_RequestInvariants_AllStrategies(t *testing.T) { const repo = "https://github.com/org/repo.git" const kubeVersion = "v1.31.2" apiVersions := []string{"apps/v1", "networking.k8s.io/v1", "v1"} @@ -461,11 +461,11 @@ spec: require.Equal(t, tc.want, classifyStrategy(streamDir, branchFolder), "row must exercise its declared strategy") - // E1 (#432): cluster version info present on every strategy. + // REQ1 (#432): cluster version info present on every strategy. assert.Equal(t, kubeVersion, req.KubeVersion, "KubeVersion must be set (#432)") assert.Equal(t, apiVersions, req.ApiVersions, "ApiVersions must be set (#432)") - // E2 (#416): permissive project so helm-build errors are not masked + // REQ2 (#416): permissive project so helm-build errors are not masked // as permission errors. assertDefaultProjectFields checks both // ProjectName=default and ProjectSourceRepos=["*"]. assertDefaultProjectFields(t, req) @@ -473,14 +473,14 @@ spec: } } -// E3: a transient Helm dependency build error must be surfaced with its original +// REQ3: a transient Helm dependency build error must be surfaced with its original // message rather than swallowed or masked (#416). The error originates inside // the repo server while it builds chart dependencies, so it is only observable // against a real repo server - the permissive ProjectSourceRepos asserted in -// E2 is the unit-level precondition that keeps the repo server from replacing it +// REQ2 is the unit-level precondition that keeps the repo server from replacing it // with a misleading permission error, but verifying the surfaced message is an // integration concern. Recorded as a skipped placeholder so the matrix has a -// named entry for E3; real coverage belongs in the integration suite (branch-N). -func TestSourceMatrix_TableE3_HelmDepBuildErrorSurfaced_Integration(t *testing.T) { - t.Skip("E3 is integration-only (#416): a swallowed helm-dependency build error is only observable against a real repo server (branch-N)") +// named entry for REQ3; real coverage belongs in the integration suite (branch-N). +func TestSourceMatrix_RequestInvariants_HelmDepBuildErrorSurfaced_Integration(t *testing.T) { + t.Skip("REQ3 is integration-only (#416): a swallowed helm-dependency build error is only observable against a real repo server (branch-N)") } diff --git a/pkg/reposerverextract/gaps_test.go b/pkg/reposerverextract/gaps_test.go new file mode 100644 index 00000000..31a0000d --- /dev/null +++ b/pkg/reposerverextract/gaps_test.go @@ -0,0 +1,463 @@ +package reposerverextract + +// Source-combination matrix - GAP and BUG rows (the frontier). +// +// These rows were identified by auditing the real repo-server source +// (argo-cd/reposerver/repository/repository.go) and were NOT covered by the +// original STR/TYP/REACH/REF/REQ/TRV/NEG tables. They are split into their own +// file because their status differs from the rest of the suite: +// +// - Some assert behavior our tool DOES NOT implement yet. They are skipped by +// default so normal CI stays green, but can be enabled with +// RUN_KNOWN_BUG_TESTS=1 to run the failing repros. +// - Some are integration-only (the failure happens inside the repo server and +// cannot be observed by a unit test). Those are skipped placeholders, the +// same treatment as REACH14 / TRV3 / REQ3. +// +// Each known-bug row asserts the REPO-SERVER-CORRECT outcome, so it will go +// green the moment the underlying behavior is fixed. When fixing one, remove +// the skipKnownBugUnlessEnabled call from that test. +// +// See SOURCE_MATRIX.md for the per-row expected behavior and repo-server refs. + +import ( + "os" + "path/filepath" + "testing" + + repoapiclient "github.com/argoproj/argo-cd/v3/reposerver/apiclient" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const runKnownBugTestsEnv = "RUN_KNOWN_BUG_TESTS" + +func skipKnownBugUnlessEnabled(t *testing.T, row, reason string) { + t.Helper() + if os.Getenv(runKnownBugTestsEnv) == "1" { + return + } + t.Skipf("%s known bug: %s. Set %s=1 to run this failing repro.", row, reason, runKnownBugTestsEnv) +} + +// routeSingle is a small helper: write files, build the single content source's +// request, and return the observed strategy and the request. It fails the test +// on any routing error. +func routeSingle(t *testing.T, repo, prRepo, appYAML string, files map[string]string) (streamStrategy, *repoapiclient.ManifestRequest) { + t.Helper() + if prRepo == "" { + prRepo = repo + } + branchFolder := writeBranchFiles(t, files) + app := makeApp(t, replaceRepo(appYAML, repo)) + + contentSources, refSources, hasMultipleSources, err := splitSources(app) + require.NoError(t, err) + require.Len(t, contentSources, 1, "helper is for single-content-source rows") + + req, streamDir, cleanup, err := buildManifestRequestForSource( + app, contentSources[0], refSources, hasMultipleSources, branchFolder, &RepoCreds{}, + manifestRequestRenderContext{ + repoSelector: testRepoSelector(t, prRepo), + kubeVersion: "v1.30.1", + apiVersions: []string{"apps/v1", "v1"}, + }, + ) + require.NoError(t, err) + if cleanup != nil { + t.Cleanup(cleanup) + } + return classifyStrategy(streamDir, branchFolder), req +} + +// ── TYP5: explicit `directory:` on a dir that has Chart.yaml ────────────────── +// +// The repo server's ExplicitType() makes this render as Directory, NOT Helm, +// regardless of the on-disk Chart.yaml (types.go:3799). Our isLocalHelmChart +// only checks for Chart.yaml on disk, so it treats this as a Helm chart and +// narrows the stream to the chart dir. Asserting the repo-server-correct +// outcome: an explicit directory source is plain-manifest and should stream the +// branch root with Path kept (so the repo server can find all manifests). +// +// EXPECTED TO FAIL today: we narrow to chart-dir and clear Path. +func TestSourceMatrix_TYP5_ExplicitDirectoryOverChartYaml(t *testing.T) { + skipKnownBugUnlessEnabled(t, "TYP5", "explicit directory sources with Chart.yaml are narrowed as Helm charts") + + const repo = "https://github.com/org/repo.git" + got, req := routeSingle(t, repo, "", ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: typ5} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD + directory: + recurse: true +`, map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + "apps/chart/deployment.yaml": "apiVersion: apps/v1\nkind: Deployment\nmetadata: {name: x}\n", + }) + + assert.Equal(t, strategyStreamBranchRoot, got, + "explicit directory: source must not be narrowed as a Helm chart (ExplicitType wins)") + assert.Equal(t, "apps/chart", req.ApplicationSource.Path, "Path must be kept for a directory source") +} + +// ── TYP6: explicit `helm:` on a dir that has kustomization.yaml ─────────────── +// +// ExplicitType() makes this render as Helm, not Kustomize. Our isKustomizeSource +// sees kustomization.yaml and keeps the branch root (the Kustomize behavior). +// For Helm, the repo server would want the chart dir as root - but there is no +// Chart.yaml here, so this is really "explicit helm on a non-chart dir", which +// the repo server treats as Helm-on-a-dir. The safe correct outcome we can +// assert: routing must at least not crash and must stream something renderable. +// We assert branch-root (Path kept) as the conservative correct behavior. +// +// May PASS today (we already stream branch root for kustomization.yaml dirs). +func TestSourceMatrix_TYP6_ExplicitHelmOverKustomization(t *testing.T) { + const repo = "https://github.com/org/repo.git" + got, req := routeSingle(t, repo, "", ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: typ6} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: apps/k + targetRevision: HEAD + helm: + releaseName: explicit +`, map[string]string{ + "apps/k/kustomization.yaml": "resources: []\n", + }) + + assert.Equal(t, strategyStreamBranchRoot, got, + "explicit helm: on a non-chart dir should stream branch root (Path kept)") + assert.Equal(t, "apps/k", req.ApplicationSource.Path) +} + +// ── TYP7: both `helm:` and `kustomize:` blocks -> repo server hard error ─────── +// +// ExplicitType() returns "multiple application sources defined" (types.go:3816) +// and nothing renders. Our tool does not detect this and will happily build a +// request. There is no unit-observable failure on our side (we don't validate +// the combination), so the realistic assertion is that the repo server would +// reject it - which is an integration concern. Documented as a skipped +// placeholder; the U-portion (our tool's lack of validation) is asserted as a +// known-permissive behavior below so the row is still exercised. +func TestSourceMatrix_TYP7_MultipleExplicitTypes_Integration(t *testing.T) { + t.Skip("TYP7 is integration-only: the 'multiple application sources defined' error is raised inside the repo server (ExplicitType, types.go:3816)") +} + +// ── TYP9: dir has BOTH Chart.yaml and kustomization.yaml ────────────────────── +// +// Repo-server discovery picks Kustomize (discovery.go:65 overwrites Helm). Our +// buildStreamDirForLocalSource checks isKustomizeSource first and keeps the +// branch root - which matches. This row should PASS and documents the parity. +func TestSourceMatrix_TYP9_BothChartAndKustomization_PicksKustomize(t *testing.T) { + const repo = "https://github.com/org/repo.git" + got, _ := routeSingle(t, repo, "", ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: typ9} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: apps/both + targetRevision: HEAD +`, map[string]string{ + "apps/both/Chart.yaml": "apiVersion: v2\nname: both\nversion: 0.1.0\n", + "apps/both/kustomization.yaml": "resources: []\n", + }) + + assert.Equal(t, strategyStreamBranchRoot, got, + "a dir with both markers must stream branch root (Kustomize wins, like the repo server)") +} + +// ── TYP8: .argocd-source.yaml injects a helm/kustomize block ────────────────── +// +// mergeSourceParameters applies .argocd-source.yaml from the app dir BEFORE type +// detection (repository.go:1872), so an override file can change the rendered +// type. Our tool never reads override files, so it cannot react. The render-time +// effect is only observable against a real repo server -> integration. The +// unit-observable shortfall (we ignore the file) has no assertion target on our +// side, so this is a skipped integration placeholder. +func TestSourceMatrix_TYP8_ArgocdSourceInjectsType_Integration(t *testing.T) { + t.Skip("TYP8 is integration-only: .argocd-source.yaml is applied inside the repo server before type detection (repository.go:1872); our tool does not read it") +} + +// ── TYP10: source type disabled via EnabledSourceTypes ──────────────────────── +func TestSourceMatrix_TYP10_DisabledSourceTypeFallsBackToDirectory_Integration(t *testing.T) { + t.Skip("TYP10 is integration-only: IsManifestGenerationEnabled gating happens inside the repo server (repository.go:1938); our tool never sets EnabledSourceTypes") +} + +// ── REACH5: .argocd-source.yaml adds out-of-chart helm.valueFiles (BUG) ──────── +// +// CONFIRMED BUG. The override file (read from the chart dir by the repo server) +// can add helm.valueFiles that point outside the chart dir. Our +// hasOutOfChartValueFile only inspects the MANIFEST's Helm.ValueFiles, so it +// never sees the override-injected file, returns false, and we narrow the stream +// to the chart dir - dropping the out-of-chart file from the tarball. The repo +// server then fails to find it. +// +// Asserting the repo-server-correct outcome: branch root must be streamed. +// EXPECTED TO FAIL today (we narrow to chart-dir). +func TestSourceMatrix_REACH5_ArgocdSourceOutOfChartValues_BUG(t *testing.T) { + skipKnownBugUnlessEnabled(t, "REACH5", ".argocd-source.yaml out-of-chart valueFiles are not considered when choosing stream root") + + const repo = "https://github.com/org/repo.git" + got, _ := routeSingle(t, repo, "", ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: reach5} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD +`, map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + // The override file the repo server will read - it adds an out-of-chart + // value file. Our tool does not parse this today. + "apps/chart/.argocd-source.yaml": "helm:\n valueFiles:\n - ../shared/values.yaml\n", + "shared/values.yaml": "replicas: 3\n", + }) + + assert.Equal(t, strategyStreamBranchRoot, got, + "BUG REACH5: .argocd-source.yaml out-of-chart valueFiles must force branch-root streaming") +} + +// ── REACH6: valueFile with env placeholder ──────────────────────────────────── +// +// The repo server runs env.Envsubst on value-file paths before resolving them +// (repository.go:1502). A placeholder could expand to an out-of-chart path. Our +// hasOutOfChartValueFile does not envsubst before classifying, so it may judge +// the literal "${ARGOCD_APP_NAME}/x.yaml" as in-chart. Whether this is wrong +// depends on the env; we assert the conservative correct behavior for an +// obviously-escaping placeholder value. +// +// EXPECTED TO FAIL today (we do not expand env vars when classifying). +func TestSourceMatrix_REACH6_EnvPlaceholderValueFile(t *testing.T) { + skipKnownBugUnlessEnabled(t, "REACH6", "environment-placeholder valueFiles are treated as refs and skipped during stream-root classification") + + const repo = "https://github.com/org/repo.git" + got, _ := routeSingle(t, repo, "", ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: reach6} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD + helm: + valueFiles: + - $ARGOCD_ENV_OUT/values.yaml +`, map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + }) + + // $ARGOCD_ENV_OUT begins with "$", so our classifier currently treats it as a + // $ref and skips it -> narrows to chart-dir. The repo server would envsubst it + // to a real (likely out-of-chart) path. Conservative correct: branch root. + assert.Equal(t, strategyStreamBranchRoot, got, + "env-placeholder value files should not be assumed in-chart") +} + +// ── REACH7: glob valueFile matching out-of-chart files ──────────────────────── +// +// The repo server expands globs (repository.go:1516). A glob like ../env/*.yaml +// matches out-of-chart files. Our hasOutOfChartValueFile treats the glob string +// literally; "../env/*.yaml" actually starts with ".." so it may already be +// caught - but a glob that is in-chart-prefixed yet escapes (e.g. "*/../../x") +// would not be. We assert the clear out-of-chart relative glob forces branch +// root. +// +// May PASS (the "../" prefix is caught by the existing relative-escape check). +func TestSourceMatrix_REACH7_GlobOutOfChartValueFile(t *testing.T) { + const repo = "https://github.com/org/repo.git" + got, _ := routeSingle(t, repo, "", ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: reach7} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD + helm: + valueFiles: + - ../env/*.yaml +`, map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + "env/prod.yaml": "replicas: 2\n", + }) + + assert.Equal(t, strategyStreamBranchRoot, got, + "a glob value file resolving outside the chart dir must force branch-root streaming") +} + +// ── REACH8: valueFile remote URL with disallowed scheme (integration) ───────── +func TestSourceMatrix_REACH8_DisallowedUrlScheme_Integration(t *testing.T) { + t.Skip("REACH8 is integration-only: URL-scheme allow-listing happens inside the repo server (isURLSchemeAllowed, resolved.go:53)") +} + +// ── REACH9: missing valueFile with ignoreMissingValueFiles ──────────────────── +// +// When a value file is missing, ignoreMissingValueFiles=true makes the repo +// server skip it (repository.go:1542). This affects whether a missing +// out-of-chart file even matters. Our routing does not consider the flag; for an +// out-of-chart value file it forces branch root regardless. We assert that an +// out-of-chart value file still forces branch root (correct) - the flag only +// changes the repo server's behavior once the file is/ isn't streamed. +// +// Should PASS (out-of-chart still forces branch root). +func TestSourceMatrix_REACH9_IgnoreMissingValueFiles(t *testing.T) { + const repo = "https://github.com/org/repo.git" + got, _ := routeSingle(t, repo, "", ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: reach9} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD + helm: + ignoreMissingValueFiles: true + valueFiles: + - ../shared/maybe.yaml +`, map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + }) + + assert.Equal(t, strategyStreamBranchRoot, got, + "an out-of-chart value file forces branch root even with ignoreMissingValueFiles") +} + +// ── REACH11: helm.fileParameters[].path out-of-chart ────────────────────────── +// +// fileParameters resolve like valueFiles in the repo server (repository.go:1343) +// and can point outside the chart dir. Our hasOutOfChartValueFile only inspects +// ValueFiles, never FileParameters, so an out-of-chart fileParameter does not +// force the branch root and the file is dropped from the narrowed tarball. +// +// Asserting the repo-server-correct outcome: branch root. +// EXPECTED TO FAIL today (fileParameters are ignored when classifying). +func TestSourceMatrix_REACH11_FileParameterOutOfChart_BUG(t *testing.T) { + skipKnownBugUnlessEnabled(t, "REACH11", "out-of-chart helm.fileParameters are not considered when choosing stream root") + + const repo = "https://github.com/org/repo.git" + got, _ := routeSingle(t, repo, "", ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: reach11} +spec: + destination: {namespace: default} + source: + repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD + helm: + fileParameters: + - name: ca + path: ../shared/ca.pem +`, map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + "shared/ca.pem": "PEM\n", + }) + + assert.Equal(t, strategyStreamBranchRoot, got, + "an out-of-chart fileParameter must force branch-root streaming (currently ignored)") +} + +// ── REACH12: helm.fileParameters[].path remote URL (integration) ────────────── +func TestSourceMatrix_REACH12_FileParameterRemoteUrl_Integration(t *testing.T) { + t.Skip("REACH12 is integration-only: remote fileParameter fetch happens inside the repo server") +} + +// ── REF13: helm.fileParameters[].path: $ref/... (BUG) ───────────────────────── +// +// CONFIRMED BUG. The repo server treats fileParameters as ref candidates exactly +// like valueFiles (repository.go:571-575). Our rewriteRefValueFiles only rewrites +// source.Helm.ValueFiles, never FileParameters, so for a multi-source app the ref +// source is staged into .refs/ but the fileParameter path is left as "$ref/foo" +// and the streamed render cannot resolve it. +// +// Asserting the repo-server-correct outcome: the fileParameter path is rewritten +// to a relative on-disk path (no leading "$") that exists under the stream tree. +// EXPECTED TO FAIL today (FileParameters are never rewritten). +func TestSourceMatrix_REF13_FileParameterRef_BUG(t *testing.T) { + skipKnownBugUnlessEnabled(t, "REF13", "helm.fileParameters with $ref paths are not rewritten into the staged .refs tree") + + const repo = "https://github.com/org/repo.git" + + branchFolder := writeBranchFiles(t, map[string]string{ + "apps/chart/Chart.yaml": "apiVersion: v2\nname: chart\nversion: 0.1.0\n", + "ca.pem": "PEM\n", + }) + app := makeApp(t, replaceRepo(` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: ref13} +spec: + destination: {namespace: default} + sources: + - repoURL: {{REPO}} + ref: cfg + targetRevision: HEAD + - repoURL: {{REPO}} + path: apps/chart + targetRevision: HEAD + helm: + fileParameters: + - name: ca + path: $cfg/ca.pem +`, repo)) + + contentSources, refSources, hasMultipleSources, err := splitSources(app) + require.NoError(t, err) + require.Len(t, contentSources, 1) + + req, streamDir, cleanup, err := buildManifestRequestForSource( + app, contentSources[0], refSources, hasMultipleSources, branchFolder, &RepoCreds{}, + manifestRequestRenderContext{ + repoSelector: testRepoSelector(t, repo), + kubeVersion: "v1.30.1", + apiVersions: []string{"apps/v1", "v1"}, + }, + ) + require.NoError(t, err) + if cleanup != nil { + defer cleanup() + } + + require.NotNil(t, req.ApplicationSource.Helm, "expected Helm config") + require.Len(t, req.ApplicationSource.Helm.FileParameters, 1) + rewritten := req.ApplicationSource.Helm.FileParameters[0].Path + assert.NotContains(t, rewritten, "$", + "BUG REF13: $ref fileParameter path must be rewritten to a relative path, not left as %q", rewritten) + abs := filepath.Clean(filepath.Join(streamDir, req.ApplicationSource.Path, rewritten)) + _, statErr := os.Stat(abs) + assert.NoError(t, statErr, + "BUG REF13: rewritten fileParameter %q should point at a real file under the stream tree", abs) +} + +// ── NEG4: a $ref source that itself sets chart: (integration) ───────────────── +func TestSourceMatrix_NEG4_RefSourceWithChart_Integration(t *testing.T) { + t.Skip("NEG4 is integration-only: the repo server rejects chart refs ('Helm charts are not yet not supported for ref sources', repository.go:594)") +} + +// ── NEG5: raw streamed single-source with $ref, no .refs staging (integration) ─ +func TestSourceMatrix_NEG5_RawStreamedRefUnsupported_Integration(t *testing.T) { + t.Skip("NEG5 is integration-only: a raw streamed $ref fails inside the repo server ('failed to find repo', repository.go:1565). Our .refs staging exists precisely to avoid this.") +} diff --git a/pkg/reposerverextract/source_matrix_b_test.go b/pkg/reposerverextract/refs_test.go similarity index 91% rename from pkg/reposerverextract/source_matrix_b_test.go rename to pkg/reposerverextract/refs_test.go index ababea7f..7a4d888a 100644 --- a/pkg/reposerverextract/source_matrix_b_test.go +++ b/pkg/reposerverextract/refs_test.go @@ -1,10 +1,10 @@ package reposerverextract -// Source-combination matrix - Table B (multi-source `spec.sources`). +// Source-combination matrix - REF (ref handling) category. // -// Companion to SOURCE_MATRIX.md, section B. Each row is one valid way a -// multi-source Application can describe its content + $ref sources. Like Table -// A, the test asserts the ROUTING OUTCOME only: +// Companion to SOURCE_MATRIX.md, section REF. Each row is one valid way a +// multi-source Application can describe its content + $ref sources. Like the STR +// table, the test asserts the ROUTING OUTCOME only: // // - strategy per content source: remote / stream(chart-dir) / // stream(branch-root) / stream(.refs). @@ -15,11 +15,11 @@ package reposerverextract // for #432, ProjectName/ProjectSourceRepos for #416) on EVERY strategy. // // The split (content vs ref-only) is exercised by feeding the whole Application -// through splitSources and then routing each content source, so rows B9/B10 +// through splitSources and then routing each content source, so rows REF10/REF11 // (multiple content sources) assert per-source outcomes. // -// See SOURCE_MATRIX.md for the issue cross-references (B3/#426, B5/#441, -// B6/#428, etc.). +// See SOURCE_MATRIX.md for the issue cross-references (REF3/#426, REF5/#441, +// REF7/#428, etc.). import ( "os" @@ -65,7 +65,7 @@ type multiSourceCase struct { prRepo string // withPuller supplies a fakeChartPuller so the remote-chart-with-local-refs - // path (B5/#441) streams the pulled chart instead of falling back to remote. + // path (REF5/#441) streams the pulled chart instead of falling back to remote. withPuller bool // expectations is keyed implicitly by iteration; each entry is matched to a @@ -73,12 +73,12 @@ type multiSourceCase struct { expectations []contentExpectation } -func TestSourceMatrix_TableB_MultiSource_Routing(t *testing.T) { +func TestSourceMatrix_Refs_Routing(t *testing.T) { const repo = "https://github.com/org/repo.git" cases := []multiSourceCase{ { - name: "B1 local chart + one local $ref -> stream(.refs), values rewritten", + name: "REF1 local chart + one local $ref -> stream(.refs), values rewritten", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -108,7 +108,7 @@ spec: }}, }, { - name: "B2 local chart + ref-with-both-ref+path (GH401) -> stream(.refs)", + name: "REF2 local chart + ref-with-both-ref+path (GH401) -> stream(.refs)", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -150,7 +150,7 @@ spec: }, }, { - name: "B3 local chart + one external $ref -> remote RPC + RefSources", + name: "REF3 local chart + one external $ref -> remote RPC + RefSources", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -180,7 +180,7 @@ spec: }}, }, { - name: "B4 local plain dir (non-chart) + local $ref -> stream(.refs)", + name: "REF4 local plain dir (non-chart) + local $ref -> stream(.refs)", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -208,7 +208,7 @@ spec: }}, }, { - name: "B5 remote chart + one local $ref (#441) -> stream(.refs) via puller", + name: "REF5 remote chart + one local $ref (#441) -> stream(.refs) via puller", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -241,7 +241,7 @@ spec: }}, }, { - name: "B5b remote chart + one local $ref, NO puller -> remote RPC + RefSources", + name: "REF6 remote chart + one local $ref, NO puller -> remote RPC + RefSources", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -273,7 +273,7 @@ spec: }}, }, { - name: "B6 remote chart + one external $ref -> remote RPC + RefSources", + name: "REF7 remote chart + one external $ref -> remote RPC + RefSources", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -301,7 +301,7 @@ spec: }}, }, { - name: "B7 remote chart + local ref-with-both-ref+path -> remote RPC + RefSources", + name: "REF8 remote chart + local ref-with-both-ref+path -> remote RPC + RefSources", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -338,7 +338,7 @@ spec: }, }, { - name: "B8 local chart + local $ref AND out-of-chart abs value -> stream(.refs)", + name: "REF9 local chart + local $ref AND out-of-chart abs value -> stream(.refs)", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -374,7 +374,7 @@ spec: }}, }, { - name: "B9 two local content sources, no ref -> stream each independently", + name: "REF10 two local content sources, no ref -> stream each independently", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -399,7 +399,7 @@ spec: }, }, { - name: "B10 local content + external content (mixed) -> stream local, remote external", + name: "REF11 local content + external content (mixed) -> stream local, remote external", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -423,7 +423,7 @@ spec: }, }, { - name: "B11 ref-only source with no path (points at repo root) -> stream(.refs)", + name: "REF12 ref-only source with no path (points at repo root) -> stream(.refs)", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application diff --git a/pkg/reposerverextract/source_matrix_a_test.go b/pkg/reposerverextract/strategy_test.go similarity index 87% rename from pkg/reposerverextract/source_matrix_a_test.go rename to pkg/reposerverextract/strategy_test.go index d77f0639..a922dc83 100644 --- a/pkg/reposerverextract/source_matrix_a_test.go +++ b/pkg/reposerverextract/strategy_test.go @@ -1,10 +1,10 @@ package reposerverextract -// Source-combination matrix - Table A (single-source `spec.source`). +// Source-combination matrix - STR (strategy selection), TYP (source-type +// resolution, on-disk), and REACH (file reachability) categories. // -// This is the table-driven companion to SOURCE_MATRIX.md. Each row below is one -// valid way a single-source Application can describe its source. The test feeds -// the Application through buildManifestRequestForSource and asserts the ROUTING +// This is the table-driven companion to SOURCE_MATRIX.md. Each row below feeds +// an Application through buildManifestRequestForSource and asserts the ROUTING // OUTCOME only: // // - strategy: which RPC the caller will use (remote vs streamed tarball), @@ -18,11 +18,11 @@ package reposerverextract // table stays valid if the routing is ever simplified (e.g. "always stream the // branch root"). See SOURCE_MATRIX.md for the issue cross-references. // -// Rows A9/A10/A11 (symlink behavior) are split out into their own tests at the -// bottom of this file (TestSourceMatrix_TableA9/A10/A11_*) because they need -// on-disk symlinks and tarball/copyDir-level assertions rather than the plain -// routing table. A10 is integration-only (skipped placeholder); A9 and A11 are -// unit-testable. Every A-row therefore has a named test in this file. +// The REACH symlink rows (REACH13/REACH14/REACH15) are split into their own +// tests at the bottom of this file because they need on-disk symlinks and +// tarball/copyDir-level assertions rather than the plain routing table. +// REACH14 is integration-only (skipped placeholder); REACH13 and REACH15 are +// unit-testable. import ( "os" @@ -69,10 +69,11 @@ func (s streamStrategy) String() string { } } -// singleSourceCase is one row of Table A: a single-source Application plus the -// on-disk files it needs and the routing outcome we expect. +// singleSourceCase is one row of the STR/TYP/REACH routing table: a single +// content source plus the on-disk files it needs and the routing outcome we +// expect. type singleSourceCase struct { - name string // matrix id + description, e.g. "A2 local chart, no values" + name string // matrix id + description, e.g. "STR1 local chart, no values" // appYAML is the Application manifest under test. Use {{REPO}} as a // placeholder for the PR repo URL so cross-repo rows are explicit. @@ -83,21 +84,20 @@ type singleSourceCase struct { files map[string]string // prRepo is what --repo resolves to. Defaults to the same repo the source - // uses; set to a different value to model a cross-repo source (A15). - prRepo string - + // uses; set to a different value to model a cross-repo source (STR4). want streamStrategy wantChart string // expected request.ApplicationSource.Chart wantPath string // expected request.ApplicationSource.Path ("" => cleared) wantNoRefs bool // RefSources must be nil for single-source rows + prRepo string } -func TestSourceMatrix_TableA_SingleSource_Routing(t *testing.T) { +func TestSourceMatrix_Strategy_Routing(t *testing.T) { const repo = "https://github.com/org/repo.git" cases := []singleSourceCase{ { - name: "A1 plain local dir (manifests) -> stream branch root", + name: "TYP1 plain local dir (manifests) -> stream branch root", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -115,7 +115,7 @@ spec: wantNoRefs: true, }, { - name: "A2 local Helm chart, no values -> stream chart dir", + name: "STR1 local Helm chart, no values -> stream chart dir", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -133,7 +133,7 @@ spec: wantNoRefs: true, }, { - name: "A3 local Helm chart, in-chart values -> stream chart dir", + name: "REACH1 local Helm chart, in-chart values -> stream chart dir", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -159,7 +159,7 @@ spec: wantNoRefs: true, }, { - name: "A4 local Helm chart, out-of-chart relative values -> stream branch root", + name: "REACH2 local Helm chart, out-of-chart relative values -> stream branch root", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -183,7 +183,7 @@ spec: wantNoRefs: true, }, { - name: "A5 local Helm chart, out-of-chart absolute values -> stream branch root", + name: "REACH3 local Helm chart, out-of-chart absolute values -> stream branch root", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -207,7 +207,7 @@ spec: wantNoRefs: true, }, { - name: "A6 local Helm chart, remote-URL values -> stream chart dir (URL ignored)", + name: "REACH4 local Helm chart, remote-URL values -> stream chart dir (URL ignored)", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -228,7 +228,7 @@ spec: wantNoRefs: true, }, { - name: "A7 local Kustomize -> stream branch root", + name: "TYP2 local Kustomize -> stream branch root", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -246,7 +246,7 @@ spec: wantNoRefs: true, }, { - name: "A8 local Kustomize with helmCharts (Chart.yaml present) -> stream branch root", + name: "TYP3 local Kustomize with helmCharts (Chart.yaml present) -> stream branch root", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -267,7 +267,7 @@ spec: wantNoRefs: true, }, { - name: "A12 local plugin (CMP) path -> stream branch root", + name: "TYP4 local plugin (CMP) path -> stream branch root", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -287,7 +287,7 @@ spec: wantNoRefs: true, }, { - name: "A13 remote Helm chart -> remote RPC", + name: "STR2 remote Helm chart -> remote RPC", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -305,7 +305,7 @@ spec: wantNoRefs: true, }, { - name: "A14 remote Helm chart (OCI) -> remote RPC", + name: "STR3 remote Helm chart (OCI) -> remote RPC", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -323,7 +323,7 @@ spec: wantNoRefs: true, }, { - name: "A15 local path source but cross-repo -> remote RPC", + name: "STR4 local path source but cross-repo -> remote RPC", appYAML: ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -428,7 +428,7 @@ func replaceRepo(yaml, repo string) string { return strings.ReplaceAll(yaml, "{{REPO}}", repo) } -// ── A9: local chart with an unrelated in-bounds symlink elsewhere in the repo ── +// ── REACH13: local chart with an unrelated in-bounds symlink elsewhere in repo ─ // // Routing a local chart must succeed even when the surrounding repo contains // symlinks, and narrowing the stream to the chart dir must keep those unrelated @@ -437,7 +437,7 @@ func replaceRepo(yaml, repo string) string { // own symlink safety gate (#438) is the part that only a real render exercises; // that is the I half of this U+I row and lives in the integration suite (see // also TestBuildManifestRequest_LocalHelmChart_TarballExcludesUnrelatedRepoSymlinks). -func TestSourceMatrix_TableA9_InBoundsSymlink_RoutesAndExcludesUnrelated(t *testing.T) { +func TestSourceMatrix_Strategy_InBoundsSymlink_RoutesAndExcludesUnrelated(t *testing.T) { const repo = "https://github.com/org/repo.git" branchFolder := t.TempDir() @@ -492,7 +492,7 @@ spec: assert.NotContains(t, entries, "assets/logo.png", "unrelated repo asset must not be in the chart tarball") } -// ── A10: local chart with an OUT-OF-BOUNDS file symlink (#438) ──────────────── +// ── REACH14: local chart with an OUT-OF-BOUNDS file symlink (#438) ──────────── // // A symlink that escapes the repository (e.g. /etc/passwd or ../../outside) is // rejected by the repo server's symlink safety check during render. That @@ -501,11 +501,11 @@ spec: // as a skipped integration placeholder so the matrix has a named entry for it; // the real coverage belongs in the integration suite (branch-N) with an // out-of-bounds symlink fixture. -func TestSourceMatrix_TableA10_OutOfBoundsSymlink_Integration(t *testing.T) { - t.Skip("A10 is integration-only (#438): the repo server's out-of-bounds symlink gate is only exercised by a real render (branch-N)") +func TestSourceMatrix_Strategy_OutOfBoundsSymlink_Integration(t *testing.T) { + t.Skip("REACH14 is integration-only (#438): the repo server's out-of-bounds symlink gate is only exercised by a real render (branch-N)") } -// ── A11: local chart containing a DIRECTORY symlink (#448, fixed by #449) ────── +// ── REACH15: local chart containing a DIRECTORY symlink (#448, fixed by #449) ── // // A chart that contains a directory symlink must route successfully (it used to // fail at the copyDir level with EISDIR because filepath.Walk+Lstat treated the @@ -514,7 +514,7 @@ func TestSourceMatrix_TableA10_OutOfBoundsSymlink_Integration(t *testing.T) { // a symlink entry; the important assertion is that routing SUCCEEDS rather than // erroring. The copyDir directory-symlink-following behavior used on the ref // slow path is additionally covered by TestCopyDir_FollowsDirectorySymlink. -func TestSourceMatrix_TableA11_DirectorySymlinkInChart_RoutesSuccessfully(t *testing.T) { +func TestSourceMatrix_Strategy_DirectorySymlinkInChart_RoutesSuccessfully(t *testing.T) { const repo = "https://github.com/org/repo.git" branchFolder := t.TempDir()