diff --git a/pkg/reposerverextract/SOURCE_MATRIX.md b/pkg/reposerverextract/SOURCE_MATRIX.md new file mode 100644 index 00000000..b4c78a51 --- /dev/null +++ b/pkg/reposerverextract/SOURCE_MATRIX.md @@ -0,0 +1,289 @@ +# Repo-server-api source-combination matrix (test coverage spec) + +> 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. +> +> 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. +> +> 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. + +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. + +## How the code routes (the thing under test) + +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.) + +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`: + +| 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. + +## Legend + +- 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. + +> **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`. + +--- + +## STR - Strategy selection + +| # | 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 | +| --- | --- | --- | --- | --- | --- | +| 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) | + +## TRV - Traversal (app-of-apps) + +| # | Case | Expected | Test | U/I | Status | +| --- | --- | --- | --- | --- | --- | +| 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 | โญ๏ธ | + +## NEG - Negatives / unsupported + +| # | 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) + +**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 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. 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. +- **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 cannot see failures that happen **inside** the repo server. The +highest-value targets for proactive bug-hunting via branch-N integration: + +- **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 + +- `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/crosscutting_test.go b/pkg/reposerverextract/crosscutting_test.go new file mode 100644 index 00000000..e37210d4 --- /dev/null +++ b/pkg/reposerverextract/crosscutting_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, 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 +// 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. +// +// 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_RequestInvariants_HelmDepBuildErrorSurfaced_*) 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 โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ + +// 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_Strategy_AppSetRoutesLikeApp(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) +} + +// NEG1/NEG2: degenerate source topologies must fail loudly in splitSources rather +// than silently producing an empty/invalid request. +func TestSourceMatrix_Negatives_DegenerateTopologies(t *testing.T) { + cases := []struct { + name string + appYAML string + }{ + { + name: "NEG1 neither source nor sources -> error", + appYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {name: c2} +spec: + destination: {namespace: default} +`, + }, + { + name: "NEG2 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: "NEG3 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 โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ + +// 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. +// +// (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_Traversal_ChildRoutesLikeSeed(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: "TRV1a 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: "TRV1b 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) + }) + } +} + +// 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. +// +// 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 childAppManifestTRV2(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_Traversal_ChildHonorsRedirectTargetRevisions(t *testing.T) { + const repoURL = "https://github.com/example/repo" + + cases := []struct { + name string + pinnedRevision string + redirectRevisions []string + wantRevision string + }{ + { + name: "TRV2a revision in list -> redirected to target branch", + pinnedRevision: "main", + redirectRevisions: []string{"main", "HEAD"}, + wantRevision: "target", + }, + { + name: "TRV2b revision NOT in list -> left untouched", + pinnedRevision: "feature-branch", + redirectRevisions: []string{"main", "HEAD"}, + wantRevision: "feature-branch", + }, + { + name: "TRV2c 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 := childAppManifestTRV2(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) + }) + } +} + +// 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 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_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 โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ + +// 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_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") + + // 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)") + + // REQ2 (#416): permissive project so helm-build errors are not masked + // as permission errors. assertDefaultProjectFields checks both + // ProjectName=default and ProjectSourceRepos=["*"]. + assertDefaultProjectFields(t, req) + }) + } +} + +// 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 +// 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 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/refs_test.go b/pkg/reposerverextract/refs_test.go new file mode 100644 index 00000000..7a4d888a --- /dev/null +++ b/pkg/reposerverextract/refs_test.go @@ -0,0 +1,568 @@ +package reposerverextract + +// Source-combination matrix - REF (ref handling) category. +// +// 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). +// - 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 REF10/REF11 +// (multiple content sources) assert per-source outcomes. +// +// See SOURCE_MATRIX.md for the issue cross-references (REF3/#426, REF5/#441, +// REF7/#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 (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 + // content source by chart/path. + expectations []contentExpectation +} + +func TestSourceMatrix_Refs_Routing(t *testing.T) { + const repo = "https://github.com/org/repo.git" + + cases := []multiSourceCase{ + { + name: "REF1 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: "REF2 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: "REF3 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: "REF4 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: "REF5 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: "REF6 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: "REF7 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: "REF8 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: "REF9 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: "REF10 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: "REF11 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: "REF12 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/strategy_test.go b/pkg/reposerverextract/strategy_test.go new file mode 100644 index 00000000..a922dc83 --- /dev/null +++ b/pkg/reposerverextract/strategy_test.go @@ -0,0 +1,568 @@ +package reposerverextract + +// 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 feeds +// an 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. +// +// 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" + "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 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. "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. + 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 (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_Strategy_Routing(t *testing.T) { + const repo = "https://github.com/org/repo.git" + + cases := []singleSourceCase{ + { + name: "TYP1 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: "STR1 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: "REACH1 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: "REACH2 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: "REACH3 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: "REACH4 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: "TYP2 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: "TYP3 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: "TYP4 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: "STR2 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: "STR3 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: "STR4 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) +} + +// โ”€โ”€ 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 +// 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_Strategy_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") +} + +// โ”€โ”€ 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 +// 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_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)") +} + +// โ”€โ”€ 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 +// 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_Strategy_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") +}