From 766011da95087c507359d4d9832d9cc05eb40f17 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 19 May 2026 11:36:47 -0400 Subject: [PATCH 1/2] feat(comp): allow filtering downstream resources Signed-off-by: Jonathan Ogilvie --- .../REQUIREMENTS.md | 273 ++++++++++++++++++ README.md | 15 + .../client/crossplane/composition_client.go | 116 ++++++++ .../crossplane/composition_client_test.go | 185 ++++++++++++ cmd/diff/comp.go | 70 ++++- cmd/diff/comp_test.go | 131 +++++++++ cmd/diff/diff_integration_test.go | 122 ++++++++ cmd/diff/diffprocessor/comp_processor.go | 214 +++++++++++--- cmd/diff/diffprocessor/comp_processor_test.go | 218 +++++++++++++- cmd/diff/renderer/comp_diff_renderer.go | 12 +- cmd/diff/renderer/comp_diff_renderer_test.go | 83 ++++++ cmd/diff/renderer/structured_renderer.go | 4 + cmd/diff/testutils/mock_builder.go | 6 + cmd/diff/testutils/mocks.go | 10 + cmd/diff/types/types.go | 15 + 15 files changed, 1423 insertions(+), 51 deletions(-) create mode 100644 .requirements/20260518T224830Z_comp_resource_filter/REQUIREMENTS.md create mode 100644 cmd/diff/comp_test.go diff --git a/.requirements/20260518T224830Z_comp_resource_filter/REQUIREMENTS.md b/.requirements/20260518T224830Z_comp_resource_filter/REQUIREMENTS.md new file mode 100644 index 00000000..f97555a2 --- /dev/null +++ b/.requirements/20260518T224830Z_comp_resource_filter/REQUIREMENTS.md @@ -0,0 +1,273 @@ +# Composition Diff: `--resource` filter (Issue #321) + +## As Is + +`crossplane-diff comp ` always discovers the full set of composites (XRs and Claims) that use the supplied composition by listing every resource of the composition's target XR GVK (and its claim GVK, if the XRD defines one) in the cluster, then filtering those that reference the composition by name. The only narrowing knob today is `--namespace` (default `default`; empty = all namespaces) which is applied at list time. + +For a user running `comp` in a CI / PR-validation context against a cluster with hundreds of XRs/Claims, every run does a full LIST + per-XR render. The runtime grows linearly with composite count, even when the reviewer only cares about a representative subset (or one specific Claim during composition development/debug). + +Key facts about the existing plumbing: + +- `CompCmd` (`cmd/diff/comp.go`) declares `Namespace string` and passes it as the third argument to `proc.DiffComposition(ctx, compositions, c.Namespace)`. +- `DefaultCompDiffProcessor.processSingleComposition` (`cmd/diff/diffprocessor/comp_processor.go:234`) calls `p.compositionClient.FindCompositesUsingComposition(ctx, name, namespace)` to discover affected composites. +- `CompositionClient.FindCompositesUsingComposition` (`cmd/diff/client/crossplane/composition_client.go:692`) returns the union of XRs (cluster-scoped or namespaced; v1 or v2) and Claims (always namespaced) that reference the composition. v1 vs v2 is transparent to callers — both are `*unstructured.Unstructured`. +- `ResourceClient.GetResource(ctx, gvk, namespace, name)` (`cmd/diff/client/kubernetes/resource_client.go:21`) already supports name-keyed lookups; we don't need a new low-level Kubernetes plumbing. +- Kong's default behavior for `[]string` flags: comma-separated **and** repeatable simultaneously (`tag.go:261` — `Sep` defaults to `,`). So one `[]string` field handles both `--resource=a --resource=b` and `--resource=a,b` with no extra tags. +- `IntegrationTestCase` (`cmd/diff/diff_integration_test.go:44`) already gates `namespace` to `CompositionDiffTest` (line 219). A parallel `resources []string` gate would mirror that pattern. + +## To Be + +`crossplane-diff comp` accepts a new `--resource` flag (singular, repeatable, also accepts comma-separated values) that constrains the impact analysis to a user-supplied set of named composites: + +- Each value is `[namespace/]name`. Bare `name` (no slash) means cluster-scoped lookup (used by v1 XRs and v2 cluster-scoped XRs). `ns/name` is namespaced (used by v2 namespaced XRs and Claims). +- `--resource` and `--namespace` are **mutually exclusive**. Supplying both is a CLI usage error reported during Kong parsing / `AfterApply`. +- When `--resource` is set, the processor **direct-fetches** each named composite via `ResourceClient.GetResource` against (a) the composition's `compositeTypeRef` GVK, then (b) the claim GVK derived from the XRD (if any). Whichever returns 200 wins. Both 404 → resource is "not relevant to this composition". +- For each `(composition, resource)` pair: relevant means the cluster lookup succeeded AND the composite's `spec.compositionRef.name` (or v2 `spec.crossplane.compositionRef.name`) equals the composition's name. +- **Fail-fast preflight.** Before rendering any composition, every `--resource` ref is resolved against every supplied composition's `(XR GVK, Claim GVK)` pair. If any ref is relevant to **zero** of the supplied compositions, the command exits with `ExitCodeToolError` and an error naming the unmatched ref(s) — *no composition diffs are rendered*. This is a CLI/user-input error, distinct from downstream XR processing failures, so render-then-error doesn't apply. +- **Update-policy filtered composites are surfaced explicitly.** When a user-supplied `--resource` matches a composite that the existing `--include-manual` filter would drop (Manual update policy without `--include-manual`), that composite is included in the composition's `ImpactAnalysis` with a new status `XRStatusFilteredByPolicy` (`filtered_by_policy`) and zero diffs. This is visible in both text and structured (JSON/YAML) output. Users see the composite was matched but skipped, and the suggested remediation (`--include-manual`) is mentioned in stderr / the renderer. +- v1 (cluster-scoped XRs + namespaced Claims) and v2 (namespaced or cluster-scoped XRs + namespaced Claims) all work — the GVK pair derived from the composition + XRD covers all cases. +- Help text, README, and CLI examples advertise the new flag. + +## Requirements + +1. **R1. Flag definition.** `CompCmd` declares `Resources []string` with Kong tag `name:"resource"` and a singular help text (e.g., `"Limit impact analysis to specific composites in [namespace/]name format. Repeatable or comma-separated."`). It uses singular `--resource` because each value is exactly one composite. +2. **R2. Mutual exclusion.** `CompCmd.AfterApply` (or `Run`) returns a clear error if both `c.Namespace != ""` and `len(c.Resources) > 0`. The error mentions both flag names. +3. **R3. Resource value parsing.** A helper parses each `--resource` value into `(namespace, name)` honoring the `[namespace/]name` rule. Empty string, more than one `/`, or empty `name` is a parse error reported with the offending value. Whitespace around values is trimmed. +4. **R4. New CompositionClient method.** `CompositionClient` gains `GetCompositesByName(ctx, comp *apiextensionsv1.Composition, refs []ResourceRef) (matched []*un.Unstructured, unmatched []ResourceRef, err error)`. Implementation: + - Resolve the composition's XR GVK from `comp.Spec.CompositeTypeRef` (passed as a typed argument so the method works for net-new compositions not yet in the cluster). + - Resolve the claim GVK via `definitionClient.GetXRDForXR` + `getClaimTypeFromXRD` (may be empty if the XRD is missing or doesn't define claims; that's OK, just skip claim lookups). + - For each input `ResourceRef`, call `resourceClient.GetResource` against the XR GVK (using the ref's namespace as-is), then if NotFound try the claim GVK. + - On 200, run the existing `resourceUsesComposition` check; relevant only if it returns true. + - 404 from both lookups, OR 200 but `resourceUsesComposition == false`, marks the ref as "not relevant for this composition" — returned in the unmatched slice, not as an error. + - Any non-404 transport error from `GetResource` IS an error (return it). + - Note: this method does NOT itself apply update-policy filtering — that remains the processor's responsibility (see R5/R5b). +5. **R5. CompDiffProcessor preflight.** Before per-composition processing, `DefaultCompDiffProcessor.DiffComposition` performs a preflight pass: for every supplied composition, it calls `GetCompositesByName` to resolve the user's refs. It builds a `map[compositionName][]*Unstructured` of resolved composites and a global "unmatched" set (refs that no composition matched). If the global unmatched set is non-empty, return an error immediately — *no rendering*. +5a. **R5a. Per-composition processing uses preflight result.** When `--resource` mode is active, `processSingleComposition` skips `FindCompositesUsingComposition` entirely and uses the preflight map's entry for that composition. Composition diff calculation and downstream rendering otherwise unchanged. +5b. **R5b. Filtered-by-policy entries surfaced.** When `--resource` mode is active, the existing `filterXRsByUpdatePolicy` step produces TWO sets: kept (passed to `collectXRDiffs`) and dropped-by-policy. The dropped set is added to the composition's `ImpactAnalysis` with `Status = XRStatusFilteredByPolicy` and zero diffs. The summary `FilteredByPolicy` count still increments so the total is consistent. + - Default-discovery mode (no `--resource`): existing behavior preserved — filtered composites are NOT added to `ImpactAnalysis`, only counted. Keeps this PR's blast radius tight; we can unify later if desired. +6. **R6. New status enum value.** `cmd/diff/renderer/structured_renderer.go`: add `XRStatusFilteredByPolicy XRStatus = "filtered_by_policy"`. Update the renderer's status handling: + - Text renderer (`comp_diff_renderer.go::buildXRStatusList`): add a `case XRStatusFilteredByPolicy` branch with a clear visual marker (e.g., `⊘ / (filtered: Manual update policy)`). The renderer also notes "use --include-manual to include these" in the section header when any filtered entries are present. + - JSON/YAML renderer: status string serializes to `"filtered_by_policy"`; downstream changes section is omitted (the field is `omitempty`). + - `CompositionDiff.HasChanges()` returns false for `XRStatusFilteredByPolicy` (filtered composites are not "changes"). +7. **R7. Help & docs.** `CompCmd.Help()` adds usage examples mirroring the issue's description, plus a note about Manual policy / `--include-manual`. `README.md` (in the `comp` flags / examples block) documents the flag and includes one example covering the filtered-by-policy case. +8. **R8. Test harness extension.** `IntegrationTestCase` gains a `resources []string` field gated to `CompositionDiffTest` (parallel to the existing `namespace` gate). `runIntegrationTest` appends `--resource=` args one per entry (so the test exercises the repeatable form by default). A second harness option (`resourcesCSV string`) handles the comma-separated test variant; alternatively, a single test case toggles the join style at arg-construction time. + +### Acceptance Criteria + +- **AC1 (R1).** `crossplane-diff comp --help` lists `--resource` with `[namespace/]name` example syntax and notes that values are repeatable / comma-separated. +- **AC2 (R2).** Running `comp foo.yaml -n bar --resource=baz/qux` exits with a non-zero exit code and stderr includes both `--namespace` and `--resource` in the error message. Unit/integration test asserts. +- **AC3 (R3).** Unit tests cover: + - `name` → `("", "name")` + - `ns/name` → `("ns", "name")` + - ` ns/name ` (whitespace) → `("ns", "name")` + - `""` → error + - `ns/` → error (empty name) + - `ns/name/extra` → error (too many slashes) + - `/name` → error (empty namespace) — preferable to silently treating as cluster-scoped because the user's intent is clearly namespaced. +- **AC4 (R4).** Unit tests for `GetCompositesByName` with mocked `resourceClient` cover: XR-only hit, Claim-only hit, both-404 (returns ref as unmatched), 200-but-uses-different-composition (unmatched), transport-error propagation. v1 cluster-scoped XR (empty namespace ref) and v2 namespaced XR/Claim are both exercised. +- **AC5 (R5).** Integration test `ResourceFilterScopesAffectedXRs`: cluster has 3 XRs using composition `c`; running `comp c.yaml --resource=default/xr-1 --resource=default/xr-2` shows impact only for those two and ignores `xr-3`. Asserts via `tu.ExpectCompDiff()` JSON output. +- **AC6 (R5).** Integration test `ResourceFilterCommaSeparated`: same scenario but invoked with `--resource=default/xr-1,default/xr-2`. Equivalent result. Confirms kong's auto-parsing. +- **AC7 (R5).** Integration test `ResourceFilterClusterScoped`: a v1 XRD with cluster-scoped XR; `--resource=xr-1` (no namespace) matches. +- **AC8 (R5).** Integration test `ResourceFilterClaim`: composition with claim-bearing XRD; `--resource=ns/my-claim` looks up via the claim GVK (XR GVK 404s in this case). +- **AC9 (R5).** Integration test `ResourceFilterUnmatched`: `--resource=default/does-not-exist` produces `ExitCodeToolError` (non-zero), the error message names the unmatched ref, and **no composition impact analysis is rendered** (preflight fails early). Stderr carries the error in human-readable form; structured-output mode still emits a valid empty/error-only JSON object so CI tooling can parse it. +- **AC10 (R5b/R6).** Integration test `ResourceFilterRespectsManualPolicy`: setup includes a Manual-policy XR (`spec.crossplane.compositionUpdatePolicy: Manual` on v2, or v1 equivalent). Run with `--resource=default/manual-xr` and WITHOUT `--include-manual`. Assert via JSON: the impact analysis contains exactly one entry for `manual-xr` with `status == "filtered_by_policy"` and no downstream changes. Re-run with `--include-manual` and assert the same XR shows up with `status == "changed"` (or `"unchanged"`) and an evaluated diff. +- **AC11 (R7).** README diff under `comp` reference block shows the new flag and includes the filtered-by-policy note; `--help` smoke test in CI passes. +- **AC12 (R8).** Existing comp tests remain green (regression). Existing test `CompositionChangeImpactsXRs` still uses `namespace: "default"` and continues to pass (mutex check only fires when BOTH `--namespace` and `--resource` are set). +- **AC13.** If a `--resource` value names a composite that exists but uses a *different* composition (not present in the supplied files), it is treated as "not relevant" — not a special-case error. Existing `resourceUsesComposition` handles this. The ref is reported as unmatched if no other supplied composition claims it. + +## Testing Plan + +TDD — red tests first, smallest steps. Tests live alongside the code they exercise. + +### T1. Unit: `parseResourceRef` (R3 / AC3) + +**Location:** new file `cmd/diff/comp_test.go` (or extend an existing one if a `comp_test.go` exists; otherwise create alongside `comp.go`). + +**Cases:** the seven enumerated in AC3 above. Table-driven. + +**Red:** test fails to compile until `parseResourceRef` exists; first commit adds the test + minimal stub returning errors for everything; second commit fills in logic to pass each case. + +### T2. Unit: `CompCmd.AfterApply` mutex (R2 / AC2) + +**Location:** `cmd/diff/comp_test.go`. + +**Shape:** construct a `CompCmd{Namespace: "x", Resources: []string{"y/z"}}`, run `AfterApply` with a stub kong context, assert error contains both `--namespace` and `--resource`. (If `AfterApply` is too dependent on kong wiring, place this validation in a helper and unit-test the helper directly.) + +### T3. Unit: `GetCompositesByName` on `DefaultCompositionClient` (R4 / AC4) + +**Location:** `cmd/diff/client/crossplane/composition_client_test.go`. + +**Mocks:** existing `MockResourceClient` (or build one if absent under `cmd/diff/testutils`). Cases: +1. XR-GVK GET 200, refs the composition → returned, no unmatched. +2. XR-GVK GET 404, claim-GVK GET 200, refs the composition → returned, no unmatched. +3. XR-GVK GET 404, claim-GVK GET 404 → unmatched. +4. XR-GVK GET 200, but `resourceUsesComposition == false` → unmatched. +5. XR-GVK GET returns transport error (not NotFound) → propagated as error. +6. v1 cluster-scoped XR (empty-namespace ref) → `GetResource(gvk, "", name)` is called. +7. Composition with no claim GVK → only XR-GVK lookup is attempted; XR-GVK 404 → unmatched (does not crash). + +### T4. Unit: composition processor wiring (R5/R5a/R5b / partial AC5, AC10) + +**Location:** `cmd/diff/diffprocessor/comp_processor_test.go`. + +**Shape:** with mocked `compositionClient`, prove that: +- When the resource list is empty, `FindCompositesUsingComposition` is called (no behavior change). +- When the resource list is non-empty, the preflight loop calls `GetCompositesByName` for every supplied composition and `FindCompositesUsingComposition` is NOT called. +- Unmatched refs are aggregated across compositions and surface as an error from `DiffComposition` BEFORE any rendering occurs. Mock the renderer; assert it was NOT called when the unmatched-error path fires. +- In `--resource` mode, when a matched composite has Manual update policy and `--include-manual` is false, the resulting `CompositionDiff.ImpactAnalysis` contains an entry with `Status == XRStatusFilteredByPolicy` and the kept set passed to `collectXRDiffs` excludes that composite. +- In default-discovery mode (no `--resource`), filtered composites are NOT added to `ImpactAnalysis` (regression: existing behavior preserved). + +### T5. Integration: `ResourceFilterScopesAffectedXRs` (AC5) + +**Location:** `cmd/diff/diff_integration_test.go`, inside `TestCompDiffIntegration`. + +Reuse `testdata/comp/resources/xrd.yaml`, `original-composition.yaml`, `existing-xr-1.yaml`, `existing-xr-2.yaml`, plus a third XR fixture (or a copy). Setup all three; diff `updated-composition.yaml`; pass `resources: []string{"default/xr-1", "default/xr-2"}`. Use `outputFormat: "json"` and assert via `tu.ExpectCompDiff()` that the impact analysis includes exactly those two and not the third. + +### T6. Integration: `ResourceFilterCommaSeparated` (AC6) + +Same as T5 but with the test harness configured to emit a single `--resource=a,b` arg (a small variant on the gate added in R8). This is mainly a kong-parsing smoke test; one such case is enough. + +### T7. Integration: `ResourceFilterClusterScoped` (AC7) + +Reuse a v1 cluster-scoped XR fixture (or add one if not present under `testdata/comp/resources/`). `resources: []string{"xr-1"}` (no namespace). Assert it matches. + +### T8. Integration: `ResourceFilterClaim` (AC8) + +Use a setup with a claim-defining XRD + a Claim resource. `resources: []string{"default/my-claim"}`. Assert it matches via the claim-GVK lookup branch. + +### T9. Integration: `ResourceFilterUnmatched` (AC9) + +`resources: []string{"default/does-not-exist"}`. Expect non-zero exit code (`ExitCodeToolError`), `expectedErrorContains: "does-not-exist"`. Confirms preflight fail-fast (R5). + +### T9b. Integration: `ResourceFilterRespectsManualPolicy` (AC10) + +Setup: composition + matching XR with `spec.crossplane.compositionUpdatePolicy: Manual` (or v1 equivalent). Run twice: +1. `resources: []string{"default/manual-xr"}` (no `--include-manual`) → JSON output asserts a single `XRImpact` with `status: "filtered_by_policy"`. Exit code reflects no diffs detected (filtered != changed). +2. `resources: []string{"default/manual-xr"}` + `--include-manual` → JSON output asserts the XR shows up with `status: "changed"` (or `"unchanged"`) and an evaluated diff. + +### T10. Regression — full `cmd/diff/...` test sweep + +`go test ./cmd/diff/...` after each step. Existing comp tests must stay green. + +## Implementation Plan + +Smallest possible steps, each paired with its verification. + +### Step 1: Failing unit test for `parseResourceRef` (RED) + +**Change:** Add `cmd/diff/comp_test.go` (if absent) with the table-driven test for `parseResourceRef` (T1). Reference the parser symbol, which doesn't exist yet — compile error confirms RED. + +**Verify:** `go test ./cmd/diff -run TestParseResourceRef` fails to compile. + +### Step 2: Add `parseResourceRef` (GREEN for Step 1) + +**Change:** In `cmd/diff/comp.go`, add a `ResourceRef` struct (`{Namespace, Name string}`) and a `parseResourceRef(value string) (ResourceRef, error)` helper covering all AC3 cases. + +**Verify:** `go test ./cmd/diff -run TestParseResourceRef` passes. + +### Step 3: Failing unit test for mutex validation (RED) + +**Change:** Add `TestCompCmd_NamespaceAndResourceMutuallyExclusive` to `cmd/diff/comp_test.go`. Construct a `CompCmd` with both fields populated; call the validation helper (which doesn't exist yet) and expect an error. + +**Verify:** Compile fail or test fail until Step 4. + +### Step 4: Add mutex validation + Resources field (GREEN for Step 3) + +**Change:** +1. Add `Resources []string` field to `CompCmd` with the kong tag (R1). +2. Add `validateFlags()` method (or inline in `AfterApply`) that returns an error if both `Namespace` and `Resources` are set. + +**Verify:** Step 3 test passes. `crossplane-diff comp --help` (manual or smoke test) shows the new flag. + +### Step 5: Failing unit test for `GetCompositesByName` (RED) + +**Change:** Add `TestGetCompositesByName` in `cmd/diff/client/crossplane/composition_client_test.go` with the seven cases from T3. Expect the symbol not to exist yet. + +**Verify:** Compile fail. + +### Step 6: Implement `GetCompositesByName` (GREEN for Step 5) + +**Change:** In `cmd/diff/client/crossplane/composition_client.go`: +1. Extend the `CompositionClient` interface with `GetCompositesByName(ctx context.Context, comp *apiextensionsv1.Composition, refs []ResourceRef) (matched []*un.Unstructured, unmatched []ResourceRef, err error)`. (Place `ResourceRef` in a stable package — likely `cmd/diff/types/` or co-located with the interface. Decide during implementation; favor `cmd/diff/types/` to avoid cyclic imports if the type is shared with `comp.go` parsing.) +2. Implement on `DefaultCompositionClient`: derive XR GVK from `comp.Spec.CompositeTypeRef` and claim GVK via XRD lookup once per call, then loop refs calling `resourceClient.GetResource` with NotFound-tolerance, then `resourceUsesComposition` filter against `comp.GetName()`. +3. Update `MockCompositionClient` + builder under `cmd/diff/testutils/` with the new method (matching existing patterns). + +**Verify:** All cases in Step 5 pass. `go test ./cmd/diff/client/crossplane/...` green. + +### Step 7: Add `XRStatusFilteredByPolicy` (RED on a focused renderer test) + +**Change:** Failing test first — extend an existing comp renderer test (`cmd/diff/renderer/comp_diff_renderer_test.go`) or add a small unit test asserting: +1. JSON output for an `XRImpact` with `Status = XRStatusFilteredByPolicy` serializes `"status": "filtered_by_policy"` and omits `downstreamChanges`. +2. Text renderer prints a recognizable filtered-by-policy line (`⊘` or similar marker + `(Manual update policy)` suffix). +3. `CompositionDiff.HasChanges()` returns false when the only impacts are `XRStatusFilteredByPolicy`. + +Then add `XRStatusFilteredByPolicy` const in `structured_renderer.go` and the renderer cases in `comp_diff_renderer.go::buildXRStatusList` and JSON conversion path. Update `HasChanges()` to short-circuit appropriately. + +**Verify:** Renderer tests pass. + +### Step 8: Failing processor preflight test (RED) + +**Change:** Add T4 cases to `cmd/diff/diffprocessor/comp_processor_test.go`. The test calls a not-yet-existing variant of `DiffComposition` (or sets a not-yet-existing config field). Cases must include: +1. Empty resources → `FindCompositesUsingComposition` called, `GetCompositesByName` NOT called. +2. Non-empty resources, all match → `GetCompositesByName` called per composition, `FindCompositesUsingComposition` NOT called. +3. Non-empty resources, one ref globally unmatched → preflight returns error and renderer is NEVER invoked (mock the renderer). +4. Non-empty resources, one match has Manual policy without `--include-manual` → `ImpactAnalysis` includes it with `XRStatusFilteredByPolicy`; `collectXRDiffs` is called only with the kept set. + +**Verify:** Compile fail or assertion fail. + +### Step 9: Wire `[]ResourceRef` + preflight (GREEN for Step 8) + +**Change:** +1. Update `CompDiffProcessor` interface: `DiffComposition(ctx, compositions, namespace, resources []ResourceRef) (bool, error)`. Update all callers (`comp.go`, tests). Single signature change beats dual-path config plumbing. +2. In `DefaultCompDiffProcessor.DiffComposition`: + - If `len(resources) > 0`: convert each composition once to typed (reusing existing pattern in `collectXRDiffs`), then for each composition call `GetCompositesByName(ctx, typedComp, refs)`. Build `perCompMatched map[string][]*Unstructured` and accumulate `globallyUnmatched []ResourceRef` (a ref globally unmatched only if every composition returned it as unmatched). + - If `len(globallyUnmatched) > 0`: return `ExitCodeToolError` error naming the unmatched refs immediately (before any rendering). +3. `processSingleComposition` accepts an optional `preMatched []*Unstructured` (or reads from the preflight map). When provided, skip `FindCompositesUsingComposition` and use the pre-matched set. +4. Update `filterXRsByUpdatePolicy` (or its caller) to also return the *dropped* set when in `--resource` mode. Add the dropped set to `ImpactAnalysis` with `XRStatusFilteredByPolicy`. Default-discovery mode keeps the existing count-only behavior. +5. In `cmd/diff/comp.go::Run`, parse `c.Resources` via `parseResourceRef` (errors → `ExitCodeToolError`), then pass into `DiffComposition`. + +**Verify:** Step 8 tests pass; existing `comp_processor_test.go` cases still pass. + +### Step 10: Failing integration test `ResourceFilterScopesAffectedXRs` (RED) + +**Change:** Add T5 case + extend `IntegrationTestCase` with `resources []string` and gate at `runIntegrationTest` (R8): when set and `testType == CompositionDiffTest`, append `--resource=` for each entry. + +**Verify:** Run `go test ./cmd/diff -run TestCompDiffIntegration/ResourceFilterScopesAffectedXRs -v` — expect failure or pass depending on Step 9 completeness. + +### Step 11: GREEN + remaining integration tests + +For each of T6, T7, T8, T9, T9b in turn: add the test (RED if it surfaces a gap), then make GREEN. These exercise comma-separated form, cluster-scoped XR, claim, unmatched-error semantics, and Manual-policy filtering respectively. + +**Verify:** Each test passes; full `TestCompDiffIntegration` green. + +### Step 12: Help text + README + +**Change:** +- Add example lines to `CompCmd.Help()`: + ``` + # Limit impact analysis to specific composites + crossplane-diff comp updated-composition.yaml --resource=default/my-claim + crossplane-diff comp updated-composition.yaml --resource=default/xr-1,default/xr-2 + + # Note: --resource cannot be combined with --namespace. + # Composites with Manual update policy are shown as "filtered_by_policy" + # unless --include-manual is also passed. + ``` +- Add `--resource` to the README's `comp` flags reference and an example under "Composition Diff" usage covering the filtered-by-policy callout. + +**Verify:** Visual review; `go test ./cmd/diff/...`. + +### Step 13: Full sweep + +**Verify:** +- `go test ./cmd/diff/...` +- `earthly +go-test` if time permits +- Spot-check: run `crossplane-diff comp --help` against the locally-built binary to confirm flag presence and help formatting. + +## Open Questions / Notes + +- **Multi-composition + per-composition unmatched semantics (R5).** A ref is "globally unmatched" only if every supplied composition rejects it. Single-composition usage (the issue's primary case) collapses to "this ref doesn't apply to this composition → error" — exactly what users expect. Preflight runs to completion across all compositions before erroring so the error message can list every unmatched ref at once (better UX than failing on the first miss). +- **Why fail-fast over render-then-error.** The render-then-error pattern at `comp_processor.go:211-222` exists so that *partial* impact analysis stays visible when *some* XRs fail rendering. That's about downstream processing failures. An unmatched `--resource` is a CLI input error — there's no useful partial information to show. Fail-fast also prevents misleading "0 affected" output on typos. +- **Why `XRStatusFilteredByPolicy` instead of a boolean.** The composite's evaluation state is genuinely "we did not evaluate this" — distinct from "evaluated and unchanged" or "evaluated and changed". A boolean alongside one of those statuses would mislead JSON consumers. A new status accurately models the state and is mutually exclusive with the others, matching the existing `Changed`/`Unchanged`/`Error` model. +- **Default-discovery mode preserves existing behavior.** Filtered-by-policy entries are NOT added to `ImpactAnalysis` when running without `--resource` — only counted in the summary. Keeps the PR scope focused on the new flag's UX. A future change can unify if users want listing-by-default. +- **Net-new compositions** (composition file doesn't exist in cluster yet): `GetCompositesByName` takes the typed composition as an argument so it doesn't need to fetch from the cluster. XR GVK comes from the file's `compositeTypeRef`. The XRD lookup is the only cluster dependency for deriving the claim GVK; if the XRD also doesn't exist, claim-lookup is skipped and only the XR-GVK branch runs. diff --git a/README.md b/README.md index e7e41400..de270ea1 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,14 @@ crossplane-diff comp updated-composition.yaml --context production # Show impact only on XRs in a specific namespace crossplane-diff comp updated-composition.yaml -n production +# Limit impact analysis to specific composites — useful for fast PR-time validation +# against a representative subset of XRs/Claims, or for debugging against a single composite. +# Format is [namespace/]name; bare name means cluster-scoped (v1 XRs and v2 cluster-scoped XRs). +crossplane-diff comp updated-composition.yaml --resource=default/my-claim +crossplane-diff comp updated-composition.yaml --resource=default/xr-1,default/xr-2 +# Note: --resource cannot be combined with --namespace. Composites with Manual update policy +# are surfaced with status "filtered_by_policy" unless --include-manual is also passed. + # Include XRs with Manual update policy (pinned revisions) crossplane-diff comp updated-composition.yaml --include-manual @@ -202,6 +210,13 @@ Flags: --eventual-state Show eventual state after all reconciliation cycles complete. Useful with function-sequencer which hides later stage resources until earlier stages become Ready. + --resource=STRING,... Limit impact analysis to specific composites in + [namespace/]name format. Repeatable or comma-separated. + Bare name means cluster-scoped. Mutually exclusive with + --namespace. Composites matched by --resource but excluded + by the update-policy filter are reported in the impact + analysis with status "filtered_by_policy" (use + --include-manual to evaluate them instead). ``` **Note**: The `diff` subcommand is deprecated. Use `xr` instead. diff --git a/cmd/diff/client/crossplane/composition_client.go b/cmd/diff/client/crossplane/composition_client.go index 091eca32..48194bbb 100644 --- a/cmd/diff/client/crossplane/composition_client.go +++ b/cmd/diff/client/crossplane/composition_client.go @@ -7,6 +7,8 @@ import ( "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/core" "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/kubernetes" + dtypes "github.com/crossplane-contrib/crossplane-diff/cmd/diff/types" + apierrors "k8s.io/apimachinery/pkg/api/errors" un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -32,6 +34,14 @@ type CompositionClient interface { // FindCompositesUsingComposition finds all composites (XRs and Claims) that use the specified composition FindCompositesUsingComposition(ctx context.Context, compositionName string, namespace string) ([]*un.Unstructured, error) + + // GetCompositesByName fetches the user-named composites (XRs or Claims) for a composition. + // For each ResourceRef, the XR GVK derived from comp.Spec.CompositeTypeRef is tried first, then the + // claim GVK derived from the XRD if defined. A ref is "matched" only when (a) the cluster lookup + // succeeds AND (b) the resource references this composition by name. Refs whose lookups all 404, or + // that exist but reference a different composition, are returned in `unmatched` (not as an error). + // NotFound responses are tolerated; non-NotFound transport errors propagate. + GetCompositesByName(ctx context.Context, comp *apiextensionsv1.Composition, refs []dtypes.ResourceRef) (matched []*un.Unstructured, unmatched []dtypes.ResourceRef, err error) } // DefaultCompositionClient implements CompositionClient. @@ -822,3 +832,109 @@ func (c *DefaultCompositionClient) resourceUsesComposition(resource *un.Unstruct return false } + +// GetCompositesByName fetches the user-named composites for a composition. +// See the CompositionClient interface for the full contract. +func (c *DefaultCompositionClient) GetCompositesByName(ctx context.Context, comp *apiextensionsv1.Composition, refs []dtypes.ResourceRef) ([]*un.Unstructured, []dtypes.ResourceRef, error) { + if len(refs) == 0 { + return nil, nil, nil + } + + // Resolve XR GVK from the (possibly net-new) composition file. No cluster lookup needed. + gv, err := schema.ParseGroupVersion(comp.Spec.CompositeTypeRef.APIVersion) + if err != nil { + return nil, nil, errors.Wrapf(err, "cannot parse compositeTypeRef apiVersion %q", comp.Spec.CompositeTypeRef.APIVersion) + } + + xrGVK := schema.GroupVersionKind{ + Group: gv.Group, + Version: gv.Version, + Kind: comp.Spec.CompositeTypeRef.Kind, + } + + // Resolve claim GVK best-effort. Missing XRD or no claimNames → claim lookups are skipped. + var claimGVK schema.GroupVersionKind + + xrd, xrdErr := c.definitionClient.GetXRDForXR(ctx, xrGVK) + + switch { + case xrdErr != nil: + c.logger.Debug("XRD lookup failed; skipping claim-GVK fallback for --resource lookups", + "xrGVK", xrGVK.String(), "error", xrdErr) + case xrd != nil: + gvk, err := c.getClaimTypeFromXRD(xrd) + if err != nil { + c.logger.Debug("could not extract claim type from XRD; skipping claim-GVK fallback", + "xrd", xrd.GetName(), "error", err) + } else { + claimGVK = gvk + } + } + + var ( + matched []*un.Unstructured + unmatched []dtypes.ResourceRef + ) + + for _, ref := range refs { + // Try XR-GVK first. + obj, err := c.resourceClient.GetResource(ctx, xrGVK, ref.Namespace, ref.Name) + + switch { + case err == nil: + if c.resourceUsesComposition(obj, comp.GetName()) { + c.logger.Debug("matched ref via XR GVK", + "ref", ref.String(), "composition", comp.GetName()) + + matched = append(matched, obj) + + continue + } + + c.logger.Debug("ref exists as XR but does not reference this composition", + "ref", ref.String(), "composition", comp.GetName()) + + unmatched = append(unmatched, ref) + + continue + case !apierrors.IsNotFound(err): + return nil, nil, errors.Wrapf(err, "cannot fetch composite %s as %s", ref.String(), xrGVK) + } + + // XR-GVK was 404. Try claim GVK if available. + if claimGVK.Empty() { + c.logger.Debug("ref not found as XR and no claim GVK to try", + "ref", ref.String()) + + unmatched = append(unmatched, ref) + + continue + } + + obj, err = c.resourceClient.GetResource(ctx, claimGVK, ref.Namespace, ref.Name) + + switch { + case err == nil: + if c.resourceUsesComposition(obj, comp.GetName()) { + c.logger.Debug("matched ref via claim GVK", + "ref", ref.String(), "composition", comp.GetName()) + + matched = append(matched, obj) + + continue + } + + c.logger.Debug("ref exists as claim but does not reference this composition", + "ref", ref.String(), "composition", comp.GetName()) + + unmatched = append(unmatched, ref) + case apierrors.IsNotFound(err): + c.logger.Debug("ref not found as XR or claim", "ref", ref.String()) + unmatched = append(unmatched, ref) + default: + return nil, nil, errors.Wrapf(err, "cannot fetch composite %s as %s", ref.String(), claimGVK) + } + } + + return matched, unmatched, nil +} diff --git a/cmd/diff/client/crossplane/composition_client_test.go b/cmd/diff/client/crossplane/composition_client_test.go index b73adfdf..8d22ad2b 100644 --- a/cmd/diff/client/crossplane/composition_client_test.go +++ b/cmd/diff/client/crossplane/composition_client_test.go @@ -6,7 +6,9 @@ import ( "testing" tu "github.com/crossplane-contrib/crossplane-diff/cmd/diff/testutils" + dtypes "github.com/crossplane-contrib/crossplane-diff/cmd/diff/types" "github.com/google/go-cmp/cmp" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -1809,3 +1811,186 @@ func TestDefaultCompositionClient_getClaimTypeFromXRD(t *testing.T) { }) } } + +func TestDefaultCompositionClient_GetCompositesByName(t *testing.T) { + ctx := t.Context() + + // Composition targeting (example.org/v1, XBucket). + comp := tu.NewComposition("test-comp"). + WithCompositeTypeRef("example.org/v1", "XBucket"). + Build() + + // XRD with claim "Bucket" (so claim GVK is example.org/v1, Bucket). + xrd := tu.NewXRD("xbuckets.example.org", "example.org", "XBucket"). + WithPlural("xbuckets"). + WithClaimNames("Bucket", "buckets"). + WithVersion("v1", true, true). + BuildAsUnstructured() + + // XRD with no claim type (just XR). + xrdNoClaim := tu.NewXRD("xbuckets.example.org", "example.org", "XBucket"). + WithPlural("xbuckets"). + WithVersion("v1", true, true). + BuildAsUnstructured() + + // Cluster-scoped XR named "xr-cluster", refs comp via v1 path. + xrCluster := tu.NewResource("example.org/v1", "XBucket", "xr-cluster"). + WithSpecField("compositionRef", map[string]any{"name": "test-comp"}). + Build() + + // Namespaced claim "claim-ns/claim-1", refs comp via v1 path. + claim := tu.NewResource("example.org/v1", "Bucket", "claim-1"). + InNamespace("claim-ns"). + WithSpecField("compositionRef", map[string]any{"name": "test-comp"}). + Build() + + // XR that uses a DIFFERENT composition. + xrWrongComp := tu.NewResource("example.org/v1", "XBucket", "xr-wrong"). + WithSpecField("compositionRef", map[string]any{"name": "some-other-comp"}). + Build() + + xrGVK := schema.GroupVersionKind{Group: "example.org", Version: "v1", Kind: "XBucket"} + claimGVK := schema.GroupVersionKind{Group: "example.org", Version: "v1", Kind: "Bucket"} + + tests := map[string]struct { + reason string + mockResource *tu.MockResourceClient + mockDef *tu.MockDefinitionClient + comp *apiextensionsv1.Composition + refs []dtypes.ResourceRef + wantMatched []string // names of matched composites + wantUnmatched []dtypes.ResourceRef + wantErr bool + }{ + "XRGVKHit_ClusterScoped": { + reason: "Cluster-scoped XR with matching composition is matched via XR-GVK lookup", + mockResource: tu.NewMockResourceClient(). + WithGetResource(func(_ context.Context, gvk schema.GroupVersionKind, _, name string) (*un.Unstructured, error) { + if gvk == xrGVK && name == "xr-cluster" { + return xrCluster, nil + } + return nil, apierrors.NewNotFound(schema.GroupResource{Group: gvk.Group, Resource: "xbuckets"}, name) + }). + Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Name: "xr-cluster"}}, + wantMatched: []string{"xr-cluster"}, + }, + "ClaimGVKHit_NamespacedClaim": { + reason: "Claim found via claim-GVK fallback when XR-GVK 404s", + mockResource: tu.NewMockResourceClient(). + WithGetResource(func(_ context.Context, gvk schema.GroupVersionKind, ns, name string) (*un.Unstructured, error) { + if gvk == claimGVK && ns == "claim-ns" && name == "claim-1" { + return claim, nil + } + return nil, apierrors.NewNotFound(schema.GroupResource{Group: gvk.Group, Resource: "x"}, name) + }). + Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Namespace: "claim-ns", Name: "claim-1"}}, + wantMatched: []string{"claim-1"}, + }, + "BothLookupsNotFound_Unmatched": { + reason: "Returned in unmatched when neither XR-GVK nor claim-GVK lookup succeeds", + mockResource: tu.NewMockResourceClient().WithResourceNotFound().Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Namespace: "claim-ns", Name: "ghost"}}, + wantUnmatched: []dtypes.ResourceRef{{Namespace: "claim-ns", Name: "ghost"}}, + }, + "FoundButUsesDifferentComposition_Unmatched": { + reason: "Resource exists but its compositionRef points to a different composition", + mockResource: tu.NewMockResourceClient(). + WithGetResource(func(_ context.Context, gvk schema.GroupVersionKind, _, name string) (*un.Unstructured, error) { + if gvk == xrGVK && name == "xr-wrong" { + return xrWrongComp, nil + } + return nil, apierrors.NewNotFound(schema.GroupResource{Group: gvk.Group, Resource: "x"}, name) + }). + Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Name: "xr-wrong"}}, + wantUnmatched: []dtypes.ResourceRef{{Name: "xr-wrong"}}, + }, + "TransportErrorPropagated": { + reason: "Non-NotFound errors from GetResource propagate up", + mockResource: tu.NewMockResourceClient(). + WithGetResource(func(context.Context, schema.GroupVersionKind, string, string) (*un.Unstructured, error) { + return nil, errors.New("connection refused") + }). + Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Name: "x"}}, + wantErr: true, + }, + "NoClaimType_OnlyXRLookupAttempted": { + reason: "When XRD has no claimNames, claim-GVK lookup is skipped without crashing", + mockResource: tu.NewMockResourceClient(). + WithGetResource(func(_ context.Context, gvk schema.GroupVersionKind, _, name string) (*un.Unstructured, error) { + if gvk == xrGVK && name == "xr-cluster" { + return xrCluster, nil + } + return nil, apierrors.NewNotFound(schema.GroupResource{Group: gvk.Group, Resource: "x"}, name) + }). + Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrdNoClaim).Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Name: "xr-cluster"}, {Namespace: "x", Name: "ghost"}}, + wantMatched: []string{"xr-cluster"}, + wantUnmatched: []dtypes.ResourceRef{{Namespace: "x", Name: "ghost"}}, + }, + "XRDNotFound_OnlyXRLookupAttempted": { + reason: "When XRD itself is missing, claim-GVK lookup is skipped", + mockResource: tu.NewMockResourceClient(). + WithGetResource(func(_ context.Context, gvk schema.GroupVersionKind, _, name string) (*un.Unstructured, error) { + if gvk == xrGVK && name == "xr-cluster" { + return xrCluster, nil + } + return nil, apierrors.NewNotFound(schema.GroupResource{Group: gvk.Group, Resource: "x"}, name) + }). + Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXRNotFound().Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Name: "xr-cluster"}}, + wantMatched: []string{"xr-cluster"}, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + c := &DefaultCompositionClient{ + resourceClient: tt.mockResource, + definitionClient: tt.mockDef, + revisionClient: NewCompositionRevisionClient(tt.mockResource, tu.TestLogger(t, false)), + logger: tu.TestLogger(t, false), + } + + matched, unmatched, err := c.GetCompositesByName(ctx, tt.comp, tt.refs) + + if tt.wantErr { + if err == nil { + t.Fatalf("\n%s: expected error, got matched=%v unmatched=%v", tt.reason, matched, unmatched) + } + return + } + if err != nil { + t.Fatalf("\n%s: unexpected error: %v", tt.reason, err) + } + + var gotMatchedNames []string + for _, m := range matched { + gotMatchedNames = append(gotMatchedNames, m.GetName()) + } + if diff := cmp.Diff(tt.wantMatched, gotMatchedNames); diff != "" { + t.Errorf("\n%s: matched mismatch:\n%s", tt.reason, diff) + } + if diff := cmp.Diff(tt.wantUnmatched, unmatched); diff != "" { + t.Errorf("\n%s: unmatched mismatch:\n%s", tt.reason, diff) + } + }) + } +} diff --git a/cmd/diff/comp.go b/cmd/diff/comp.go index bf36bfb9..fa95e67e 100644 --- a/cmd/diff/comp.go +++ b/cmd/diff/comp.go @@ -18,10 +18,12 @@ package main import ( "context" + "strings" "time" "github.com/alecthomas/kong" dp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/diffprocessor" + "github.com/crossplane-contrib/crossplane-diff/cmd/diff/types" "github.com/crossplane/crossplane-runtime/v2/pkg/errors" "github.com/crossplane/crossplane-runtime/v2/pkg/logging" @@ -39,8 +41,17 @@ type CompCmd struct { Files []string `arg:"" help:"YAML files containing updated Composition(s)." optional:""` // Configuration options - Namespace string `default:"" help:"Namespace to find XRs (empty = all namespaces)." name:"namespace" short:"n"` - IncludeManual bool `default:"false" help:"Include XRs with Manual update policy (default: only Automatic policy XRs)" name:"include-manual"` + Namespace string `default:"" help:"Namespace to find XRs (empty = all namespaces)." name:"namespace" short:"n"` + IncludeManual bool `default:"false" help:"Include XRs with Manual update policy (default: only Automatic policy XRs)" name:"include-manual"` + Resources []string `help:"Limit impact analysis to specific composites in [namespace/]name format. Repeatable or comma-separated. Mutually exclusive with --namespace." name:"resource"` +} + +// validateFlags returns an error if mutually exclusive flags are set together. +func (c *CompCmd) validateFlags() error { + if c.Namespace != "" && len(c.Resources) > 0 { + return errors.New("--namespace and --resource are mutually exclusive; use --resource=[namespace/]name to scope by name") + } + return nil } // Help returns help instructions for the composition diff command. @@ -69,6 +80,15 @@ Examples: # Show eventual state with function-sequencer (all stages, not just first). crossplane-diff comp updated-composition.yaml --eventual-state + + # Limit impact analysis to specific composites (by [namespace/]name) + crossplane-diff comp updated-composition.yaml --resource=default/my-claim + crossplane-diff comp updated-composition.yaml --resource=default/xr-1,default/xr-2 + +Notes: + --resource cannot be combined with --namespace. + Composites with Manual update policy are surfaced as "filtered_by_policy" + unless --include-manual is also passed. ` } @@ -76,6 +96,10 @@ Examples: // AppContext is received via dependency injection - Kong resolves it through the provider chain: // ContextProvider (bound in CommonCmdFields.BeforeApply) -> provideRestConfig -> provideAppContext. func (c *CompCmd) AfterApply(ctx *kong.Context, log logging.Logger, appCtx *AppContext) error { + if err := c.validateFlags(); err != nil { + return err + } + proc := makeDefaultCompProc(c, ctx, appCtx, log) loader, err := ld.NewCompositeLoader(c.Files) @@ -113,6 +137,34 @@ func makeDefaultCompProc(c *CompCmd, kongCtx *kong.Context, appCtx *AppContext, return dp.NewCompDiffProcessor(xrProc, appCtx.XpClients.Composition, opts...) } +// parseResourceRef parses a "[namespace/]name" string into a ResourceRef. +// Bare "name" (no slash) means cluster-scoped (v1 XRs, v2 cluster-scoped XRs). +// "ns/name" means namespaced (Claims, v2 namespaced XRs). +// "/name" (empty namespace before slash) is rejected because the user's intent is clearly namespaced. +func parseResourceRef(value string) (types.ResourceRef, error) { + trimmed := strings.TrimSpace(value) + if trimmed == "" { + return types.ResourceRef{}, errors.Errorf("invalid --resource value %q: cannot be empty", value) + } + + parts := strings.Split(trimmed, "/") + switch len(parts) { + case 1: + return types.ResourceRef{Name: parts[0]}, nil + case 2: + ns, name := parts[0], parts[1] + if ns == "" { + return types.ResourceRef{}, errors.Errorf("invalid --resource value %q: namespace must not be empty (use bare name for cluster-scoped composites)", value) + } + if name == "" { + return types.ResourceRef{}, errors.Errorf("invalid --resource value %q: name must not be empty", value) + } + return types.ResourceRef{Namespace: ns, Name: name}, nil + default: + return types.ResourceRef{}, errors.Errorf("invalid --resource value %q: expected [namespace/]name format, got %d slash-separated parts", value, len(parts)-1) + } +} + // Run executes the composition diff command. func (c *CompCmd) Run(_ *kong.Context, log logging.Logger, appCtx *AppContext, proc dp.CompDiffProcessor, loader ld.Loader, exitCode *ExitCode) error { ctx, cancel, err := initializeAppContext(c.Timeout, appCtx, log) @@ -149,7 +201,19 @@ func (c *CompCmd) Run(_ *kong.Context, log logging.Logger, appCtx *AppContext, p return errors.Wrap(err, "cannot load compositions") } - hasDiffs, err := proc.DiffComposition(ctx, compositions, c.Namespace) + parsedRefs := make([]types.ResourceRef, 0, len(c.Resources)) + + for _, raw := range c.Resources { + ref, err := parseResourceRef(raw) + if err != nil { + exitCode.Code = dp.ExitCodeToolError + return err + } + + parsedRefs = append(parsedRefs, ref) + } + + hasDiffs, err := proc.DiffComposition(ctx, compositions, c.Namespace, parsedRefs) // Determine exit code based on result exitCode.Code = dp.DetermineExitCode(err, hasDiffs) diff --git a/cmd/diff/comp_test.go b/cmd/diff/comp_test.go new file mode 100644 index 00000000..c8bf0c22 --- /dev/null +++ b/cmd/diff/comp_test.go @@ -0,0 +1,131 @@ +/* +Copyright 2025 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "strings" + "testing" + + "github.com/crossplane-contrib/crossplane-diff/cmd/diff/types" +) + +func TestParseResourceRef(t *testing.T) { + tests := map[string]struct { + input string + want types.ResourceRef + wantErr bool + }{ + "BareName_ClusterScoped": { + input: "my-xr", + want: types.ResourceRef{Namespace: "", Name: "my-xr"}, + }, + "NamespaceAndName": { + input: "default/my-claim", + want: types.ResourceRef{Namespace: "default", Name: "my-claim"}, + }, + "WhitespaceTrimmed": { + input: " default/my-claim ", + want: types.ResourceRef{Namespace: "default", Name: "my-claim"}, + }, + "Empty": { + input: "", + wantErr: true, + }, + "OnlyWhitespace": { + input: " ", + wantErr: true, + }, + "EmptyNameAfterSlash": { + input: "default/", + wantErr: true, + }, + "TooManySlashes": { + input: "default/foo/bar", + wantErr: true, + }, + "EmptyNamespaceLeadingSlash": { + input: "/foo", + wantErr: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + got, err := parseResourceRef(tt.input) + + if tt.wantErr { + if err == nil { + t.Fatalf("expected error for input %q, got %+v", tt.input, got) + } + if !strings.Contains(err.Error(), tt.input) && tt.input != "" { + t.Errorf("error message %q should reference offending input %q", err.Error(), tt.input) + } + return + } + + if err != nil { + t.Fatalf("unexpected error for input %q: %v", tt.input, err) + } + if got != tt.want { + t.Errorf("parseResourceRef(%q) = %+v, want %+v", tt.input, got, tt.want) + } + }) + } +} + +func TestCompCmd_ValidateFlags(t *testing.T) { + tests := map[string]struct { + cmd CompCmd + wantErr bool + errMustContain []string + }{ + "NeitherSet": { + cmd: CompCmd{}, + }, + "OnlyNamespace": { + cmd: CompCmd{Namespace: "default"}, + }, + "OnlyResources": { + cmd: CompCmd{Resources: []string{"default/foo"}}, + }, + "BothSet": { + cmd: CompCmd{Namespace: "default", Resources: []string{"default/foo"}}, + wantErr: true, + errMustContain: []string{"--namespace", "--resource"}, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + err := tt.cmd.validateFlags() + if tt.wantErr { + if err == nil { + t.Fatal("expected error, got nil") + } + for _, sub := range tt.errMustContain { + if !strings.Contains(err.Error(), sub) { + t.Errorf("error %q must contain %q", err.Error(), sub) + } + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} diff --git a/cmd/diff/diff_integration_test.go b/cmd/diff/diff_integration_test.go index 45277198..86d7d629 100644 --- a/cmd/diff/diff_integration_test.go +++ b/cmd/diff/diff_integration_test.go @@ -57,6 +57,9 @@ type IntegrationTestCase struct { functionCredentials string // Path to function credentials file (optional) eventualState bool // Enable eventual state simulation for XR or composition tests (optional) timeout time.Duration // Custom timeout for this test (0 = use default) + resources []string // For composition tests: --resource values; each entry passed as one --resource flag + resourcesCSV string // For composition tests: alternative single --resource=a,b style invocation + includeManual bool // For composition tests: pass --include-manual flag skip bool skipReason string // JSON output support: set outputFormat to "json" to use structured assertions. @@ -247,6 +250,19 @@ func runIntegrationTest(t *testing.T, testType DiffTestType, tt IntegrationTestC args = append(args, "--eventual-state") } + // Add --resource flags (composition tests only) + if testType == CompositionDiffTest { + for _, r := range tt.resources { + args = append(args, fmt.Sprintf("--resource=%s", r)) + } + if tt.resourcesCSV != "" { + args = append(args, fmt.Sprintf("--resource=%s", tt.resourcesCSV)) + } + if tt.includeManual { + args = append(args, "--include-manual") + } + } + // Add files as positional arguments args = append(args, testFiles...) @@ -3192,6 +3208,112 @@ Summary: 2 modified`, AndXR().AndComp().And(), expectedError: false, }, + // --resource flag tests (issue #321) + "ResourceFilterScopesAffectedXRs": { + reason: "--resource limits impact analysis to the named composites and ignores the rest", + setupFiles: []string{ + "testdata/comp/resources/xrd.yaml", + "testdata/comp/resources/original-composition.yaml", + "testdata/comp/resources/composition-revision-v1.yaml", + "testdata/comp/resources/functions.yaml", + "testdata/comp/resources/existing-xr-1.yaml", // test-resource (default ns) + "testdata/comp/resources/existing-downstream-1.yaml", + "testdata/comp/resources/existing-xr-2.yaml", // another-resource (default ns) + "testdata/comp/resources/existing-downstream-2.yaml", + }, + inputFiles: []string{"testdata/comp/updated-composition.yaml"}, + resources: []string{"default/test-resource"}, + outputFormat: "json", + expectedExitCode: dp.ExitCodeDiffDetected, + expectedStructuredCompOutput: tu.ExpectCompDiff(). + WithComposition("xnopresources.diff.example.org"). + WithCompositionModified(). + WithAffectedResources(1, 1, 0, 0). + WithXRImpact("XNopResource", "test-resource", "default", "changed"). + AndComp().And(), + }, + "ResourceFilterCommaSeparated": { + reason: "--resource accepts comma-separated values via kong's auto-parsing", + setupFiles: []string{ + "testdata/comp/resources/xrd.yaml", + "testdata/comp/resources/original-composition.yaml", + "testdata/comp/resources/composition-revision-v1.yaml", + "testdata/comp/resources/functions.yaml", + "testdata/comp/resources/existing-xr-1.yaml", + "testdata/comp/resources/existing-downstream-1.yaml", + "testdata/comp/resources/existing-xr-2.yaml", + "testdata/comp/resources/existing-downstream-2.yaml", + }, + inputFiles: []string{"testdata/comp/updated-composition.yaml"}, + resourcesCSV: "default/test-resource,default/another-resource", + outputFormat: "json", + expectedExitCode: dp.ExitCodeDiffDetected, + expectedStructuredCompOutput: tu.ExpectCompDiff(). + WithComposition("xnopresources.diff.example.org"). + WithCompositionModified(). + WithAffectedResources(2, 2, 0, 0). + WithXRImpact("XNopResource", "test-resource", "default", "changed"). + AndComp(). + WithXRImpact("XNopResource", "another-resource", "default", "changed"). + AndComp().And(), + }, + "ResourceFilterUnmatched_FailsBeforeRendering": { + reason: "--resource naming a non-existent composite fails fast with an error before any rendering", + setupFiles: []string{ + "testdata/comp/resources/xrd.yaml", + "testdata/comp/resources/original-composition.yaml", + "testdata/comp/resources/composition-revision-v1.yaml", + "testdata/comp/resources/functions.yaml", + "testdata/comp/resources/existing-xr-1.yaml", + "testdata/comp/resources/existing-downstream-1.yaml", + }, + inputFiles: []string{"testdata/comp/updated-composition.yaml"}, + resources: []string{"default/does-not-exist"}, + expectedError: true, + expectedErrorContains: "does-not-exist", + expectedExitCode: dp.ExitCodeToolError, + }, + "ResourceFilterRespectsManualPolicy_WithoutIncludeManual": { + reason: "--resource matching a Manual-policy composite surfaces it as filtered_by_policy when --include-manual is unset", + setupFiles: []string{ + "testdata/comp/resources/xrd.yaml", + "testdata/comp/resources/original-composition.yaml", + "testdata/comp/resources/composition-revision-v1.yaml", + "testdata/comp/resources/functions.yaml", + "testdata/comp/resources/existing-xr-manual.yaml", + "testdata/comp/resources/existing-downstream-manual.yaml", + }, + inputFiles: []string{"testdata/comp/updated-composition.yaml"}, + resources: []string{"default/manual-resource"}, + outputFormat: "json", + expectedExitCode: dp.ExitCodeDiffDetected, // composition itself changed + expectedStructuredCompOutput: tu.ExpectCompDiff(). + WithComposition("xnopresources.diff.example.org"). + WithCompositionModified(). + WithXRImpact("XNopResource", "manual-resource", "default", "filtered_by_policy"). + AndComp().And(), + }, + "ResourceFilterRespectsManualPolicy_WithIncludeManual": { + reason: "--include-manual evaluates the Manual-policy composite normally instead of marking it filtered_by_policy", + setupFiles: []string{ + "testdata/comp/resources/xrd.yaml", + "testdata/comp/resources/original-composition.yaml", + "testdata/comp/resources/composition-revision-v1.yaml", + "testdata/comp/resources/functions.yaml", + "testdata/comp/resources/existing-xr-manual.yaml", + "testdata/comp/resources/existing-downstream-manual.yaml", + }, + inputFiles: []string{"testdata/comp/updated-composition.yaml"}, + resources: []string{"default/manual-resource"}, + includeManual: true, + outputFormat: "json", + expectedExitCode: dp.ExitCodeDiffDetected, + expectedStructuredCompOutput: tu.ExpectCompDiff(). + WithComposition("xnopresources.diff.example.org"). + WithCompositionModified(). + WithXRImpact("XNopResource", "manual-resource", "default", "changed"). + AndComp().And(), + }, } for name, tt := range tests { diff --git a/cmd/diff/diffprocessor/comp_processor.go b/cmd/diff/diffprocessor/comp_processor.go index 5e1549a4..3779f7ad 100644 --- a/cmd/diff/diffprocessor/comp_processor.go +++ b/cmd/diff/diffprocessor/comp_processor.go @@ -24,6 +24,7 @@ import ( xp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/crossplane" "github.com/crossplane-contrib/crossplane-diff/cmd/diff/renderer" dt "github.com/crossplane-contrib/crossplane-diff/cmd/diff/renderer/types" + dtypes "github.com/crossplane-contrib/crossplane-diff/cmd/diff/types" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -68,7 +69,11 @@ func (r *XRDiffResult) HasError() bool { type CompDiffProcessor interface { // DiffComposition processes composition changes and shows impact on existing XRs. // Returns (hasDiffs, error) where hasDiffs indicates if any differences were detected. - DiffComposition(ctx context.Context, compositions []*un.Unstructured, namespace string) (bool, error) + // When `resources` is non-empty, impact analysis is restricted to the named composites: + // each ref is resolved against every supplied composition's (XR GVK, claim GVK) pair via a + // preflight pass. If any ref is relevant to no supplied composition, the call fails before + // rendering any diffs (CLI input error). When `resources` is empty, behavior is unchanged. + DiffComposition(ctx context.Context, compositions []*un.Unstructured, namespace string, resources []dtypes.ResourceRef) (bool, error) Initialize(ctx context.Context) error // Cleanup releases any resources held by the processor (e.g., Docker containers). Cleanup(ctx context.Context) error @@ -139,13 +144,24 @@ func (p *DefaultCompDiffProcessor) Cleanup(ctx context.Context) error { // DiffComposition processes composition changes and shows impact on existing XRs. // Returns (hasDiffs, error) where hasDiffs indicates if any differences were detected. -func (p *DefaultCompDiffProcessor) DiffComposition(ctx context.Context, compositions []*un.Unstructured, namespace string) (bool, error) { - p.config.Logger.Debug("Processing composition diff", "compositionCount", len(compositions), "namespace", namespace) +func (p *DefaultCompDiffProcessor) DiffComposition(ctx context.Context, compositions []*un.Unstructured, namespace string, resources []dtypes.ResourceRef) (bool, error) { + p.config.Logger.Debug("Processing composition diff", + "compositionCount", len(compositions), + "namespace", namespace, + "resourceCount", len(resources)) if len(compositions) == 0 { return false, errors.New("no compositions provided") } + // When --resource is set, run a preflight pass that resolves every ref against every supplied + // composition. If any ref is relevant to no supplied composition, fail loudly BEFORE rendering + // any diffs (this is a CLI input error, not a downstream processing failure). + preflightMatches, err := p.preflightResourceRefs(ctx, compositions, resources) + if err != nil { + return false, err + } + output := &renderer.CompDiffOutput{ Compositions: make([]renderer.CompositionDiff, 0, len(compositions)), Errors: []dt.OutputError{}, @@ -166,8 +182,15 @@ func (p *DefaultCompDiffProcessor) DiffComposition(ctx context.Context, composit compositionID := comp.GetName() // Use actual name from unstructured p.config.Logger.Debug("Processing composition", "name", compositionID) + // In --resource mode, hand the per-composition matched set into processSingleComposition; + // nil signals default-discovery mode. + var preMatched []*un.Unstructured + if len(resources) > 0 { + preMatched = preflightMatches[compositionID] + } + // Process this single composition and build the result - compResult, err := p.processSingleComposition(ctx, comp, namespace) + compResult, err := p.processSingleComposition(ctx, comp, namespace, preMatched, len(resources) > 0) if err != nil { p.config.Logger.Debug("Failed to process composition", "composition", compositionID, "error", err) @@ -229,9 +252,89 @@ func (p *DefaultCompDiffProcessor) DiffComposition(ctx context.Context, composit return hasDiffs, nil } +// preflightResourceRefs resolves user --resource refs against every supplied composition before +// any rendering happens. Returns the per-composition matched set keyed by composition name. +// If any ref is relevant to no supplied composition, it returns an error naming the unmatched +// refs (CLI input error). When `refs` is empty, returns (nil, nil) and the caller falls back to +// default-discovery mode. +func (p *DefaultCompDiffProcessor) preflightResourceRefs(ctx context.Context, compositions []*un.Unstructured, refs []dtypes.ResourceRef) (map[string][]*un.Unstructured, error) { + if len(refs) == 0 { + return nil, nil + } + + perComp := make(map[string][]*un.Unstructured, len(compositions)) + matchedAtLeastOnce := make(map[string]bool, len(refs)) + + for _, comp := range compositions { + if comp.GetKind() != "Composition" { + continue + } + + typedComp := &apiextensionsv1.Composition{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(comp.Object, typedComp); err != nil { + return nil, errors.Wrapf(err, "cannot convert composition %s to typed for preflight", comp.GetName()) + } + + matched, _, err := p.compositionClient.GetCompositesByName(ctx, typedComp, refs) + if err != nil { + return nil, errors.Wrapf(err, "preflight: cannot resolve --resource refs for composition %s", comp.GetName()) + } + + perComp[comp.GetName()] = matched + + // A ref is matched globally if any composition's matched-set contains a composite whose + // (namespace, name) equals the ref. + for _, m := range matched { + for _, ref := range refs { + if m.GetName() == ref.Name && m.GetNamespace() == ref.Namespace { + matchedAtLeastOnce[ref.String()] = true + } + } + } + } + + var globallyUnmatched []dtypes.ResourceRef + + for _, ref := range refs { + if !matchedAtLeastOnce[ref.String()] { + globallyUnmatched = append(globallyUnmatched, ref) + } + } + + if len(globallyUnmatched) > 0 { + names := make([]string, 0, len(globallyUnmatched)) + for _, r := range globallyUnmatched { + names = append(names, r.String()) + } + + return nil, errors.Errorf("--resource ref(s) not relevant to any supplied composition: %s (resource not found, or it doesn't reference one of the supplied compositions)", joinRefs(names)) + } + + return perComp, nil +} + +// joinRefs renders a list of human-readable refs joined by commas. +func joinRefs(refs []string) string { + switch len(refs) { + case 0: + return "" + case 1: + return refs[0] + default: + out := refs[0] + for _, r := range refs[1:] { + out += ", " + r + } + + return out + } +} + // processSingleComposition processes a single composition and builds the result. -// Returns (*CompositionDiff, error). -func (p *DefaultCompDiffProcessor) processSingleComposition(ctx context.Context, newComp *un.Unstructured, namespace string) (*renderer.CompositionDiff, error) { +// Returns (*CompositionDiff, error). When `resourceMode` is true, the function uses the +// caller-supplied `preMatched` set instead of calling FindCompositesUsingComposition, and +// surfaces update-policy-filtered composites in ImpactAnalysis with XRStatusFilteredByPolicy. +func (p *DefaultCompDiffProcessor) processSingleComposition(ctx context.Context, newComp *un.Unstructured, namespace string, preMatched []*un.Unstructured, resourceMode bool) (*renderer.CompositionDiff, error) { result := &renderer.CompositionDiff{ Name: newComp.GetName(), ImpactAnalysis: []renderer.XRImpact{}, @@ -252,46 +355,77 @@ func (p *DefaultCompDiffProcessor) processSingleComposition(ctx context.Context, result.CompositionDiff = compDiff - // Find all composites (XRs and Claims) that use this composition - affectedXRs, err := p.compositionClient.FindCompositesUsingComposition(ctx, newComp.GetName(), namespace) - if err != nil { - // For net-new compositions, the composition won't exist in the cluster - // so FindCompositesUsingComposition will fail. This is expected behavior. - p.config.Logger.Debug("Cannot find composites using composition (likely net-new composition)", - "composition", newComp.GetName(), "error", err) - // Return result with empty impact analysis for net-new compositions - return result, nil + // Resolve the affected composite set. In --resource mode, the preflight pass already produced it. + // In default-discovery mode, query the cluster. + var affectedXRs []*un.Unstructured + + if resourceMode { + affectedXRs = preMatched + } else { + discovered, err := p.compositionClient.FindCompositesUsingComposition(ctx, newComp.GetName(), namespace) + if err != nil { + // For net-new compositions, the composition won't exist in the cluster + // so FindCompositesUsingComposition will fail. This is expected behavior. + p.config.Logger.Debug("Cannot find composites using composition (likely net-new composition)", + "composition", newComp.GetName(), "error", err) + // Return result with empty impact analysis for net-new compositions + return result, nil + } + + affectedXRs = discovered } - p.config.Logger.Debug("Found affected XRs", "composition", newComp.GetName(), "count", len(affectedXRs)) + p.config.Logger.Debug("Found affected XRs", "composition", newComp.GetName(), "count", len(affectedXRs), "resourceMode", resourceMode) // Filter XRs based on IncludeManual flag - filteredXRs := p.filterXRsByUpdatePolicy(affectedXRs) - filteredByPolicy := len(affectedXRs) - len(filteredXRs) + keptXRs, droppedXRs := p.partitionXRsByUpdatePolicy(affectedXRs) + filteredByPolicy := len(droppedXRs) p.config.Logger.Debug("Filtered XRs by update policy", "composition", newComp.GetName(), "originalCount", len(affectedXRs), - "filteredCount", len(filteredXRs), + "keptCount", len(keptXRs), + "droppedCount", filteredByPolicy, "includeManual", p.config.IncludeManual) - if len(filteredXRs) == 0 { + // In --resource mode, surface filtered composites in the impact analysis as + // XRStatusFilteredByPolicy so users see what was matched-but-skipped. In default-discovery + // mode, preserve the existing summary-only behavior. + if resourceMode { + for _, xr := range droppedXRs { + result.ImpactAnalysis = append(result.ImpactAnalysis, renderer.XRImpact{ + ObjectReference: corev1.ObjectReference{ + APIVersion: xr.GetAPIVersion(), + Kind: xr.GetKind(), + Name: xr.GetName(), + Namespace: xr.GetNamespace(), + }, + Status: renderer.XRStatusFilteredByPolicy, + }) + } + } + + if len(keptXRs) == 0 { // All XRs were filtered by policy + result.AffectedResources.Total = len(affectedXRs) result.AffectedResources.FilteredByPolicy = filteredByPolicy + return result, nil } - // Use filtered XRs for the rest of the processing - affectedXRs = filteredXRs + // Process kept XRs and collect diffs to determine which ones have changes + p.config.Logger.Debug("Processing XRs to collect diff information", "count", len(keptXRs)) - // Process affected XRs and collect diffs to determine which ones have changes - p.config.Logger.Debug("Processing XRs to collect diff information", "count", len(affectedXRs)) + xrResults := p.collectXRDiffs(ctx, keptXRs, newComp) - xrResults := p.collectXRDiffs(ctx, affectedXRs, newComp) - - // Build impact analysis and counts from results - result.ImpactAnalysis, result.AffectedResources = p.buildImpactAnalysis(affectedXRs, xrResults) - result.AffectedResources.FilteredByPolicy = filteredByPolicy + // Build impact analysis and counts from results for the kept set, then merge in any + // already-appended filtered-by-policy entries. + keptImpacts, keptSummary := p.buildImpactAnalysis(keptXRs, xrResults) + result.ImpactAnalysis = append(result.ImpactAnalysis, keptImpacts...) + // keptSummary.Total counts only kept; widen to include filtered so totals stay consistent. + keptSummary.Total = len(affectedXRs) + keptSummary.FilteredByPolicy = filteredByPolicy + result.AffectedResources = keptSummary return result, nil } @@ -473,18 +607,13 @@ func (p *DefaultCompDiffProcessor) calculateCompositionDiff(ctx context.Context, return compDiff, nil } -// filterXRsByUpdatePolicy filters XRs based on the IncludeManual configuration. -// By default (IncludeManual=false), only XRs with Automatic policy are included. -// When IncludeManual=true, all XRs are included regardless of policy. -func (p *DefaultCompDiffProcessor) filterXRsByUpdatePolicy(xrs []*un.Unstructured) []*un.Unstructured { +// partitionXRsByUpdatePolicy splits XRs into a kept set (Automatic policy or default) and a +// dropped set (Manual policy). When IncludeManual is true, all XRs are kept. +func (p *DefaultCompDiffProcessor) partitionXRsByUpdatePolicy(xrs []*un.Unstructured) (kept, dropped []*un.Unstructured) { if p.config.IncludeManual { - // Include all XRs when flag is set - return xrs + return xrs, nil } - // Filter to only include Automatic policy XRs - filtered := make([]*un.Unstructured, 0, len(xrs)) - for _, xr := range xrs { policy := p.getCompositionUpdatePolicy(xr) @@ -493,13 +622,14 @@ func (p *DefaultCompDiffProcessor) filterXRsByUpdatePolicy(xrs []*un.Unstructure "kind", xr.GetKind(), "policy", policy) - // Include XRs that are not explicitly set to Manual (i.e., Automatic or empty/default) - if policy != "Manual" { - filtered = append(filtered, xr) + if policy == "Manual" { + dropped = append(dropped, xr) + } else { + kept = append(kept, xr) } } - return filtered + return kept, dropped } // getCompositionUpdatePolicy retrieves the compositionUpdatePolicy from an XR. diff --git a/cmd/diff/diffprocessor/comp_processor_test.go b/cmd/diff/diffprocessor/comp_processor_test.go index 973dcac6..f84849ee 100644 --- a/cmd/diff/diffprocessor/comp_processor_test.go +++ b/cmd/diff/diffprocessor/comp_processor_test.go @@ -289,7 +289,7 @@ func TestDefaultCompDiffProcessor_DiffComposition(t *testing.T) { compDiffRenderer: compDiffRenderer, } - _, err := processor.DiffComposition(ctx, tt.compositions, tt.namespace) + _, err := processor.DiffComposition(ctx, tt.compositions, tt.namespace, nil) if (err != nil) != tt.wantErr { t.Errorf("DiffComposition() error = %v, wantErr %v", err, tt.wantErr) @@ -424,10 +424,10 @@ func TestDefaultCompDiffProcessor_filterXRsByUpdatePolicy(t *testing.T) { }, } - got := processor.filterXRsByUpdatePolicy(tt.xrs) + got, _ := processor.partitionXRsByUpdatePolicy(tt.xrs) if len(got) != len(tt.want) { - t.Errorf("filterXRsByUpdatePolicy() returned %d XRs, want %d", len(got), len(tt.want)) + t.Errorf("partitionXRsByUpdatePolicy() returned %d kept XRs, want %d", len(got), len(tt.want)) } // Compare XR names to verify correct filtering @@ -705,7 +705,7 @@ func TestDefaultCompDiffProcessor_DiffComposition_StderrErrorOutput(t *testing.T WithCompositeTypeRef("example.org/v1", "XResource"). WithPipelineMode(). BuildAsUnstructured(), - }, "default") + }, "default", nil) // Should return error because one XR failed if err == nil { @@ -726,3 +726,213 @@ func TestDefaultCompDiffProcessor_DiffComposition_StderrErrorOutput(t *testing.T t.Errorf("Expected stderr to contain error message 'render pipeline failed', got: %q", stderrOutput) } } + + +// newCompProcessorForTest builds a DefaultCompDiffProcessor wrapping the given composition client +// for use by the --resource preflight tests below. +func newCompProcessorForTest(t *testing.T, compClient xp.CompositionClient, includeManual bool) (*DefaultCompDiffProcessor, *bytes.Buffer) { + t.Helper() + + logger := tu.TestLogger(t, false) + + stdout := &bytes.Buffer{} + stderr := &bytes.Buffer{} + + config := ProcessorConfig{ + IncludeManual: includeManual, + Logger: logger, + Stdout: stdout, + Stderr: stderr, + RenderFunc: func(_ context.Context, _ logging.Logger, in render.Inputs) (render.Outputs, error) { + return render.Outputs{CompositeResource: in.CompositeResource}, nil + }, + } + config.SetDefaultFactories() + + diffOpts := config.GetDiffOptions() + diffRenderer := config.Factories.DiffRenderer(logger, diffOpts) + compRenderer := config.Factories.CompDiffRenderer(logger, diffRenderer, diffOpts) + + mockXR := &tu.MockDiffProcessor{ + DiffSingleResourceFn: func(context.Context, *un.Unstructured, types.CompositionProvider) (map[string]*dt.ResourceDiff, error) { + return map[string]*dt.ResourceDiff{}, nil + }, + } + + return &DefaultCompDiffProcessor{ + compositionClient: compClient, + config: config, + xrProc: mockXR, + compDiffRenderer: compRenderer, + }, stdout +} + +// TestDefaultCompDiffProcessor_DiffComposition_ResourceMode covers the --resource code path: +// preflight, fail-fast on globally-unmatched refs, and surfacing of policy-filtered composites. +func TestDefaultCompDiffProcessor_DiffComposition_ResourceMode(t *testing.T) { + ctx := t.Context() + + comp := tu.NewComposition("test-comp"). + WithCompositeTypeRef("example.org/v1", "XR"). + WithPipelineMode(). + BuildAsUnstructured() + + xr1 := tu.NewResource("example.org/v1", "XR", "xr-1"). + InNamespace("ns"). + WithSpecField("compositionRef", map[string]any{"name": "test-comp"}). + Build() + + xr2 := tu.NewResource("example.org/v1", "XR", "xr-2"). + InNamespace("ns"). + WithSpecField("compositionRef", map[string]any{"name": "test-comp"}). + Build() + + // Manual-policy XR (v2 path). + manualXR := tu.NewResource("example.org/v1", "XR", "manual-xr"). + InNamespace("ns"). + WithSpecField("compositionRef", map[string]any{"name": "test-comp"}). + WithSpecField("crossplane", map[string]any{"compositionUpdatePolicy": "Manual"}). + Build() + + t.Run("EmptyResources_FallsBackToFindCompositesUsingComposition", func(t *testing.T) { + findCalls := 0 + getByNameCalls := 0 + + client := tu.NewMockCompositionClient(). + WithSuccessfulCompositionFetch(&apiextensionsv1.Composition{}). + WithFindCompositesUsingComposition(func(context.Context, string, string) ([]*un.Unstructured, error) { + findCalls++ + return []*un.Unstructured{xr1}, nil + }). + WithGetCompositesByName(func(context.Context, *apiextensionsv1.Composition, []types.ResourceRef) ([]*un.Unstructured, []types.ResourceRef, error) { + getByNameCalls++ + return nil, nil, nil + }). + Build() + + proc, _ := newCompProcessorForTest(t, client, false) + _, err := proc.DiffComposition(ctx, []*un.Unstructured{comp}, "ns", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if findCalls != 1 { + t.Errorf("FindCompositesUsingComposition: expected 1 call, got %d", findCalls) + } + if getByNameCalls != 0 { + t.Errorf("GetCompositesByName: expected 0 calls, got %d", getByNameCalls) + } + }) + + t.Run("ResourceMode_AllMatch_UsesGetCompositesByName", func(t *testing.T) { + findCalls := 0 + getByNameCalls := 0 + + client := tu.NewMockCompositionClient(). + WithSuccessfulCompositionFetch(&apiextensionsv1.Composition{}). + WithFindCompositesUsingComposition(func(context.Context, string, string) ([]*un.Unstructured, error) { + findCalls++ + return nil, nil + }). + WithGetCompositesByName(func(_ context.Context, _ *apiextensionsv1.Composition, refs []types.ResourceRef) ([]*un.Unstructured, []types.ResourceRef, error) { + getByNameCalls++ + // Both refs match. + _ = refs + return []*un.Unstructured{xr1, xr2}, nil, nil + }). + Build() + + proc, _ := newCompProcessorForTest(t, client, false) + _, err := proc.DiffComposition(ctx, []*un.Unstructured{comp}, "", + []types.ResourceRef{{Namespace: "ns", Name: "xr-1"}, {Namespace: "ns", Name: "xr-2"}}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if findCalls != 0 { + t.Errorf("FindCompositesUsingComposition: expected 0 calls, got %d", findCalls) + } + if getByNameCalls != 1 { + t.Errorf("GetCompositesByName: expected 1 call, got %d", getByNameCalls) + } + }) + + t.Run("ResourceMode_GloballyUnmatched_FailsFastNoRender", func(t *testing.T) { + client := tu.NewMockCompositionClient(). + WithSuccessfulCompositionFetch(&apiextensionsv1.Composition{}). + WithGetCompositesByName(func(_ context.Context, _ *apiextensionsv1.Composition, refs []types.ResourceRef) ([]*un.Unstructured, []types.ResourceRef, error) { + // No matches; everything unmatched. + return nil, refs, nil + }). + Build() + + proc, stdout := newCompProcessorForTest(t, client, false) + _, err := proc.DiffComposition(ctx, []*un.Unstructured{comp}, "", + []types.ResourceRef{{Namespace: "ns", Name: "ghost"}}) + if err == nil { + t.Fatal("expected error from globally-unmatched preflight, got nil") + } + if !strings.Contains(err.Error(), "ns/ghost") { + t.Errorf("error message should name the unmatched ref, got: %v", err) + } + if stdout.Len() != 0 { + t.Errorf("expected no output before fail-fast, got: %q", stdout.String()) + } + }) + + t.Run("ResourceMode_ManualPolicyMatchSurfacedAsFiltered", func(t *testing.T) { + client := tu.NewMockCompositionClient(). + WithSuccessfulCompositionFetch(&apiextensionsv1.Composition{}). + WithGetCompositesByName(func(context.Context, *apiextensionsv1.Composition, []types.ResourceRef) ([]*un.Unstructured, []types.ResourceRef, error) { + return []*un.Unstructured{manualXR}, nil, nil + }). + Build() + + proc, _ := newCompProcessorForTest(t, client, false /* IncludeManual */) + _, err := proc.DiffComposition(ctx, []*un.Unstructured{comp}, "", + []types.ResourceRef{{Namespace: "ns", Name: "manual-xr"}}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Drive the same call again, but this time inspect the result via processSingleComposition + // directly — easier than parsing renderer output. + got, err := proc.processSingleComposition(ctx, comp, "", []*un.Unstructured{manualXR}, true) + if err != nil { + t.Fatalf("processSingleComposition: %v", err) + } + if len(got.ImpactAnalysis) != 1 { + t.Fatalf("expected 1 impact entry, got %d", len(got.ImpactAnalysis)) + } + if got.ImpactAnalysis[0].Status != "filtered_by_policy" { + t.Errorf("expected filtered_by_policy status, got %q", got.ImpactAnalysis[0].Status) + } + if got.AffectedResources.FilteredByPolicy != 1 { + t.Errorf("expected FilteredByPolicy=1, got %d", got.AffectedResources.FilteredByPolicy) + } + if got.AffectedResources.Total != 1 { + t.Errorf("expected Total=1, got %d", got.AffectedResources.Total) + } + }) + + t.Run("DefaultDiscoveryMode_ManualPolicyNotInImpactAnalysis", func(t *testing.T) { + client := tu.NewMockCompositionClient(). + WithSuccessfulCompositionFetch(&apiextensionsv1.Composition{}). + WithFindCompositesUsingComposition(func(context.Context, string, string) ([]*un.Unstructured, error) { + return []*un.Unstructured{manualXR}, nil + }). + Build() + + proc, _ := newCompProcessorForTest(t, client, false /* IncludeManual */) + + // Default-discovery: pass empty resources; processSingleComposition called with resourceMode=false. + got, err := proc.processSingleComposition(ctx, comp, "", nil, false) + if err != nil { + t.Fatalf("processSingleComposition: %v", err) + } + if len(got.ImpactAnalysis) != 0 { + t.Errorf("default-discovery mode: expected NO impact entries for filtered-by-policy XRs, got %d (%+v)", len(got.ImpactAnalysis), got.ImpactAnalysis) + } + if got.AffectedResources.FilteredByPolicy != 1 { + t.Errorf("expected FilteredByPolicy count=1, got %d", got.AffectedResources.FilteredByPolicy) + } + }) +} diff --git a/cmd/diff/renderer/comp_diff_renderer.go b/cmd/diff/renderer/comp_diff_renderer.go index 94e04732..063ca478 100644 --- a/cmd/diff/renderer/comp_diff_renderer.go +++ b/cmd/diff/renderer/comp_diff_renderer.go @@ -238,7 +238,10 @@ func (r *DefaultCompDiffRenderer) buildXRStatusList(impacts []XRImpact) string { } // Determine status indicator and color based on status - var indicator, color string + var ( + indicator, color string + suffix string + ) switch impact.Status { case XRStatusError: @@ -250,12 +253,17 @@ func (r *DefaultCompDiffRenderer) buildXRStatusList(impacts []XRImpact) string { case XRStatusUnchanged: indicator = checkMark color = colorGreen + case XRStatusFilteredByPolicy: + indicator = "⊘" // ⊘ + color = colorYellow + suffix = " — filtered: Manual update policy (use --include-manual to evaluate)" } - fmt.Fprintf(&sb, "%s %s %s/%s (%s)%s\n", + fmt.Fprintf(&sb, "%s %s %s/%s (%s)%s%s\n", color, indicator, impact.Kind, impact.Name, scope, + suffix, colorReset) // Include error details for XRStatusError impacts so users can diagnose issues. diff --git a/cmd/diff/renderer/comp_diff_renderer_test.go b/cmd/diff/renderer/comp_diff_renderer_test.go index f7c5d8a8..71d3967c 100644 --- a/cmd/diff/renderer/comp_diff_renderer_test.go +++ b/cmd/diff/renderer/comp_diff_renderer_test.go @@ -536,3 +536,86 @@ func TestCompDiffOutput_JSONSchema(t *testing.T) { t.Errorf("Expected 1 modified, got %d", comp.ImpactAnalysis[0].DownstreamChanges.Summary.Modified) } } + +func TestXRStatusFilteredByPolicy_JSON(t *testing.T) { + output := &CompDiffOutput{ + Compositions: []CompositionDiff{{ + Name: "test-comp", + AffectedResources: AffectedResourcesSummary{Total: 1, FilteredByPolicy: 1}, + ImpactAnalysis: []XRImpact{ + { + ObjectReference: corev1.ObjectReference{APIVersion: "example.org/v1", Kind: "XR", Name: "manual-xr", Namespace: "ns"}, + Status: XRStatusFilteredByPolicy, + }, + }, + }}, + } + + logger := tu.TestLogger(t, false) + + var jsonBuf bytes.Buffer + + opts := DefaultDiffOptions() + opts.Format = OutputFormatJSON + opts.Stdout = &jsonBuf + opts.Stderr = &bytes.Buffer{} + + r := NewStructuredCompDiffRenderer(logger, opts) + if err := r.RenderCompDiff(output); err != nil { + t.Fatalf("RenderCompDiff: %v", err) + } + + var parsed compDiffJSONOutput + if err := json.Unmarshal(jsonBuf.Bytes(), &parsed); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if len(parsed.Compositions) != 1 || len(parsed.Compositions[0].ImpactAnalysis) != 1 { + t.Fatalf("expected 1 composition with 1 impact, got %+v", parsed) + } + + imp := parsed.Compositions[0].ImpactAnalysis[0] + if got, want := string(imp.Status), "filtered_by_policy"; got != want { + t.Errorf("status: got %q, want %q", got, want) + } + + if imp.DownstreamChanges != nil { + t.Errorf("downstreamChanges should be omitted for filtered_by_policy, got %+v", imp.DownstreamChanges) + } +} + +func TestXRStatusFilteredByPolicy_TextRenderer(t *testing.T) { + comp := CompositionDiff{ + Name: "test-comp", + AffectedResources: AffectedResourcesSummary{Total: 1, FilteredByPolicy: 1}, + ImpactAnalysis: []XRImpact{ + { + ObjectReference: corev1.ObjectReference{APIVersion: "example.org/v1", Kind: "XR", Name: "manual-xr", Namespace: "ns"}, + Status: XRStatusFilteredByPolicy, + }, + }, + } + + logger := tu.TestLogger(t, false) + r := &DefaultCompDiffRenderer{logger: logger, opts: DefaultDiffOptions()} + got := r.buildXRStatusList(comp.ImpactAnalysis) + + if !strings.Contains(got, "manual-xr") { + t.Errorf("expected XR name in output, got %q", got) + } + if !strings.Contains(strings.ToLower(got), "manual") && !strings.Contains(strings.ToLower(got), "filtered") { + t.Errorf("expected 'manual' or 'filtered' marker in output, got %q", got) + } +} + +func TestCompositionDiff_HasChanges_FilteredByPolicyOnly(t *testing.T) { + c := &CompositionDiff{ + ImpactAnalysis: []XRImpact{ + {Status: XRStatusFilteredByPolicy}, + {Status: XRStatusFilteredByPolicy}, + }, + } + if c.HasChanges() { + t.Errorf("CompositionDiff with only filtered-by-policy impacts should not be HasChanges()") + } +} diff --git a/cmd/diff/renderer/structured_renderer.go b/cmd/diff/renderer/structured_renderer.go index 83d8c6af..d7e81ce6 100644 --- a/cmd/diff/renderer/structured_renderer.go +++ b/cmd/diff/renderer/structured_renderer.go @@ -36,6 +36,10 @@ const ( XRStatusUnchanged XRStatus = "unchanged" // XRStatusError indicates an error occurred while processing the XR. XRStatusError XRStatus = "error" + // XRStatusFilteredByPolicy indicates the XR matched a user --resource selector but was excluded + // from evaluation by the update-policy filter (e.g., Manual policy without --include-manual). + // The XR is surfaced in impact analysis with no downstream changes so users see the skip explicitly. + XRStatusFilteredByPolicy XRStatus = "filtered_by_policy" ) // OutputError is an alias for dt.OutputError for convenience. diff --git a/cmd/diff/testutils/mock_builder.go b/cmd/diff/testutils/mock_builder.go index 54e6221d..0ae0caab 100644 --- a/cmd/diff/testutils/mock_builder.go +++ b/cmd/diff/testutils/mock_builder.go @@ -747,6 +747,12 @@ func (b *MockCompositionClientBuilder) WithFindResourcesError(errMsg string) *Mo }) } +// WithGetCompositesByName sets the GetCompositesByName behavior. +func (b *MockCompositionClientBuilder) WithGetCompositesByName(fn func(context.Context, *xpextv1.Composition, []dtypes.ResourceRef) ([]*un.Unstructured, []dtypes.ResourceRef, error)) *MockCompositionClientBuilder { + b.mock.GetCompositesByNameFn = fn + return b +} + // WithComposition is an alias for WithSuccessfulCompositionMatch for convenience. func (b *MockCompositionClientBuilder) WithComposition(comp *xpextv1.Composition) *MockCompositionClientBuilder { return b.WithSuccessfulCompositionMatch(comp) diff --git a/cmd/diff/testutils/mocks.go b/cmd/diff/testutils/mocks.go index aedd8386..1cecda5a 100644 --- a/cmd/diff/testutils/mocks.go +++ b/cmd/diff/testutils/mocks.go @@ -560,6 +560,7 @@ type MockCompositionClient struct { ListCompositionsFn func(ctx context.Context) ([]*xpextv1.Composition, error) GetCompositionFn func(ctx context.Context, name string) (*xpextv1.Composition, error) FindCompositesUsingCompositionFn func(ctx context.Context, compositionName string, namespace string) ([]*un.Unstructured, error) + GetCompositesByNameFn func(ctx context.Context, comp *xpextv1.Composition, refs []types.ResourceRef) ([]*un.Unstructured, []types.ResourceRef, error) } // Initialize implements crossplane.CompositionClient. @@ -607,6 +608,15 @@ func (m *MockCompositionClient) FindCompositesUsingComposition(ctx context.Conte return nil, errors.New("FindCompositesUsingComposition not implemented") } +// GetCompositesByName implements crossplane.CompositionClient. +func (m *MockCompositionClient) GetCompositesByName(ctx context.Context, comp *xpextv1.Composition, refs []types.ResourceRef) ([]*un.Unstructured, []types.ResourceRef, error) { + if m.GetCompositesByNameFn != nil { + return m.GetCompositesByNameFn(ctx, comp, refs) + } + + return nil, nil, errors.New("GetCompositesByName not implemented") +} + // MockFunctionClient implements the crossplane.FunctionClient interface. type MockFunctionClient struct { InitializeFn func(ctx context.Context) error diff --git a/cmd/diff/types/types.go b/cmd/diff/types/types.go index 0f231cf4..baed0b47 100644 --- a/cmd/diff/types/types.go +++ b/cmd/diff/types/types.go @@ -27,3 +27,18 @@ import ( // CompositionProvider is a function that provides a composition for a given resource. type CompositionProvider func(ctx context.Context, res *un.Unstructured) (*apiextensionsv1.Composition, error) + +// ResourceRef identifies a single composite (XR or Claim) by namespace and name. +// Namespace is empty for cluster-scoped composites (v1 XRs and v2 cluster-scoped XRs). +type ResourceRef struct { + Namespace string + Name string +} + +// String returns a human-readable representation: "namespace/name" or "name" for cluster-scoped. +func (r ResourceRef) String() string { + if r.Namespace == "" { + return r.Name + } + return r.Namespace + "/" + r.Name +} From 538a161f7554ce4319cb683e0b1627ed0fe7b466 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Thu, 21 May 2026 16:16:59 -0400 Subject: [PATCH 2/2] fix: lint Signed-off-by: Jonathan Ogilvie --- .../client/crossplane/composition_client.go | 2 + .../crossplane/composition_client_test.go | 57 +++++++++++-------- cmd/diff/comp.go | 7 ++- cmd/diff/comp_test.go | 6 ++ cmd/diff/diff_integration_test.go | 4 ++ cmd/diff/diffprocessor/comp_processor.go | 15 +---- cmd/diff/diffprocessor/comp_processor_test.go | 18 +++++- cmd/diff/main.go | 8 +-- cmd/diff/renderer/comp_diff_renderer_test.go | 1 + cmd/diff/types/types.go | 1 + 10 files changed, 75 insertions(+), 44 deletions(-) diff --git a/cmd/diff/client/crossplane/composition_client.go b/cmd/diff/client/crossplane/composition_client.go index 48194bbb..536b14f7 100644 --- a/cmd/diff/client/crossplane/composition_client.go +++ b/cmd/diff/client/crossplane/composition_client.go @@ -20,6 +20,8 @@ import ( ) // CompositionClient handles operations related to Compositions. +// +//nolint:interfacebloat // Composition operations are cohesive; splitting would fragment the API. type CompositionClient interface { core.Initializable diff --git a/cmd/diff/client/crossplane/composition_client_test.go b/cmd/diff/client/crossplane/composition_client_test.go index 8d22ad2b..8d05b928 100644 --- a/cmd/diff/client/crossplane/composition_client_test.go +++ b/cmd/diff/client/crossplane/composition_client_test.go @@ -1853,14 +1853,14 @@ func TestDefaultCompositionClient_GetCompositesByName(t *testing.T) { claimGVK := schema.GroupVersionKind{Group: "example.org", Version: "v1", Kind: "Bucket"} tests := map[string]struct { - reason string - mockResource *tu.MockResourceClient - mockDef *tu.MockDefinitionClient - comp *apiextensionsv1.Composition - refs []dtypes.ResourceRef - wantMatched []string // names of matched composites + reason string + mockResource *tu.MockResourceClient + mockDef *tu.MockDefinitionClient + comp *apiextensionsv1.Composition + refs []dtypes.ResourceRef + wantMatched []string // names of matched composites wantUnmatched []dtypes.ResourceRef - wantErr bool + wantErr bool }{ "XRGVKHit_ClusterScoped": { reason: "Cluster-scoped XR with matching composition is matched via XR-GVK lookup", @@ -1869,12 +1869,13 @@ func TestDefaultCompositionClient_GetCompositesByName(t *testing.T) { if gvk == xrGVK && name == "xr-cluster" { return xrCluster, nil } + return nil, apierrors.NewNotFound(schema.GroupResource{Group: gvk.Group, Resource: "xbuckets"}, name) }). Build(), - mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), - comp: comp, - refs: []dtypes.ResourceRef{{Name: "xr-cluster"}}, + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Name: "xr-cluster"}}, wantMatched: []string{"xr-cluster"}, }, "ClaimGVKHit_NamespacedClaim": { @@ -1884,20 +1885,21 @@ func TestDefaultCompositionClient_GetCompositesByName(t *testing.T) { if gvk == claimGVK && ns == "claim-ns" && name == "claim-1" { return claim, nil } + return nil, apierrors.NewNotFound(schema.GroupResource{Group: gvk.Group, Resource: "x"}, name) }). Build(), - mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), - comp: comp, - refs: []dtypes.ResourceRef{{Namespace: "claim-ns", Name: "claim-1"}}, + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Namespace: "claim-ns", Name: "claim-1"}}, wantMatched: []string{"claim-1"}, }, "BothLookupsNotFound_Unmatched": { - reason: "Returned in unmatched when neither XR-GVK nor claim-GVK lookup succeeds", - mockResource: tu.NewMockResourceClient().WithResourceNotFound().Build(), - mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), - comp: comp, - refs: []dtypes.ResourceRef{{Namespace: "claim-ns", Name: "ghost"}}, + reason: "Returned in unmatched when neither XR-GVK nor claim-GVK lookup succeeds", + mockResource: tu.NewMockResourceClient().WithResourceNotFound().Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Namespace: "claim-ns", Name: "ghost"}}, wantUnmatched: []dtypes.ResourceRef{{Namespace: "claim-ns", Name: "ghost"}}, }, "FoundButUsesDifferentComposition_Unmatched": { @@ -1907,6 +1909,7 @@ func TestDefaultCompositionClient_GetCompositesByName(t *testing.T) { if gvk == xrGVK && name == "xr-wrong" { return xrWrongComp, nil } + return nil, apierrors.NewNotFound(schema.GroupResource{Group: gvk.Group, Resource: "x"}, name) }). Build(), @@ -1934,12 +1937,13 @@ func TestDefaultCompositionClient_GetCompositesByName(t *testing.T) { if gvk == xrGVK && name == "xr-cluster" { return xrCluster, nil } + return nil, apierrors.NewNotFound(schema.GroupResource{Group: gvk.Group, Resource: "x"}, name) }). Build(), - mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrdNoClaim).Build(), - comp: comp, - refs: []dtypes.ResourceRef{{Name: "xr-cluster"}, {Namespace: "x", Name: "ghost"}}, + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrdNoClaim).Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Name: "xr-cluster"}, {Namespace: "x", Name: "ghost"}}, wantMatched: []string{"xr-cluster"}, wantUnmatched: []dtypes.ResourceRef{{Namespace: "x", Name: "ghost"}}, }, @@ -1950,12 +1954,13 @@ func TestDefaultCompositionClient_GetCompositesByName(t *testing.T) { if gvk == xrGVK && name == "xr-cluster" { return xrCluster, nil } + return nil, apierrors.NewNotFound(schema.GroupResource{Group: gvk.Group, Resource: "x"}, name) }). Build(), - mockDef: tu.NewMockDefinitionClient().WithXRDForXRNotFound().Build(), - comp: comp, - refs: []dtypes.ResourceRef{{Name: "xr-cluster"}}, + mockDef: tu.NewMockDefinitionClient().WithXRDForXRNotFound().Build(), + comp: comp, + refs: []dtypes.ResourceRef{{Name: "xr-cluster"}}, wantMatched: []string{"xr-cluster"}, }, } @@ -1975,8 +1980,10 @@ func TestDefaultCompositionClient_GetCompositesByName(t *testing.T) { if err == nil { t.Fatalf("\n%s: expected error, got matched=%v unmatched=%v", tt.reason, matched, unmatched) } + return } + if err != nil { t.Fatalf("\n%s: unexpected error: %v", tt.reason, err) } @@ -1985,9 +1992,11 @@ func TestDefaultCompositionClient_GetCompositesByName(t *testing.T) { for _, m := range matched { gotMatchedNames = append(gotMatchedNames, m.GetName()) } + if diff := cmp.Diff(tt.wantMatched, gotMatchedNames); diff != "" { t.Errorf("\n%s: matched mismatch:\n%s", tt.reason, diff) } + if diff := cmp.Diff(tt.wantUnmatched, unmatched); diff != "" { t.Errorf("\n%s: unmatched mismatch:\n%s", tt.reason, diff) } diff --git a/cmd/diff/comp.go b/cmd/diff/comp.go index fa95e67e..cee2077c 100644 --- a/cmd/diff/comp.go +++ b/cmd/diff/comp.go @@ -41,8 +41,8 @@ type CompCmd struct { Files []string `arg:"" help:"YAML files containing updated Composition(s)." optional:""` // Configuration options - Namespace string `default:"" help:"Namespace to find XRs (empty = all namespaces)." name:"namespace" short:"n"` - IncludeManual bool `default:"false" help:"Include XRs with Manual update policy (default: only Automatic policy XRs)" name:"include-manual"` + Namespace string `default:"" help:"Namespace to find XRs (empty = all namespaces)." name:"namespace" short:"n"` + IncludeManual bool `default:"false" help:"Include XRs with Manual update policy (default: only Automatic policy XRs)" name:"include-manual"` Resources []string `help:"Limit impact analysis to specific composites in [namespace/]name format. Repeatable or comma-separated. Mutually exclusive with --namespace." name:"resource"` } @@ -51,6 +51,7 @@ func (c *CompCmd) validateFlags() error { if c.Namespace != "" && len(c.Resources) > 0 { return errors.New("--namespace and --resource are mutually exclusive; use --resource=[namespace/]name to scope by name") } + return nil } @@ -156,9 +157,11 @@ func parseResourceRef(value string) (types.ResourceRef, error) { if ns == "" { return types.ResourceRef{}, errors.Errorf("invalid --resource value %q: namespace must not be empty (use bare name for cluster-scoped composites)", value) } + if name == "" { return types.ResourceRef{}, errors.Errorf("invalid --resource value %q: name must not be empty", value) } + return types.ResourceRef{Namespace: ns, Name: name}, nil default: return types.ResourceRef{}, errors.Errorf("invalid --resource value %q: expected [namespace/]name format, got %d slash-separated parts", value, len(parts)-1) diff --git a/cmd/diff/comp_test.go b/cmd/diff/comp_test.go index c8bf0c22..89ac597d 100644 --- a/cmd/diff/comp_test.go +++ b/cmd/diff/comp_test.go @@ -71,15 +71,18 @@ func TestParseResourceRef(t *testing.T) { if err == nil { t.Fatalf("expected error for input %q, got %+v", tt.input, got) } + if !strings.Contains(err.Error(), tt.input) && tt.input != "" { t.Errorf("error message %q should reference offending input %q", err.Error(), tt.input) } + return } if err != nil { t.Fatalf("unexpected error for input %q: %v", tt.input, err) } + if got != tt.want { t.Errorf("parseResourceRef(%q) = %+v, want %+v", tt.input, got, tt.want) } @@ -116,13 +119,16 @@ func TestCompCmd_ValidateFlags(t *testing.T) { if err == nil { t.Fatal("expected error, got nil") } + for _, sub := range tt.errMustContain { if !strings.Contains(err.Error(), sub) { t.Errorf("error %q must contain %q", err.Error(), sub) } } + return } + if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/cmd/diff/diff_integration_test.go b/cmd/diff/diff_integration_test.go index 86d7d629..edb56bd5 100644 --- a/cmd/diff/diff_integration_test.go +++ b/cmd/diff/diff_integration_test.go @@ -114,9 +114,11 @@ func runIntegrationTest(t *testing.T, testType DiffTestType, tt IntegrationTestC if tt.expectedStructuredOutput != nil && tt.expectedStructuredCompOutput != nil { t.Fatalf("test case sets both expectedStructuredOutput and expectedStructuredCompOutput; set only one") } + if tt.expectedStructuredOutput != nil && testType != XRDiffTest { t.Fatalf("expectedStructuredOutput is only valid for XRDiffTest (got %q)", testType) } + if tt.expectedStructuredCompOutput != nil && testType != CompositionDiffTest { t.Fatalf("expectedStructuredCompOutput is only valid for CompositionDiffTest (got %q)", testType) } @@ -255,9 +257,11 @@ func runIntegrationTest(t *testing.T, testType DiffTestType, tt IntegrationTestC for _, r := range tt.resources { args = append(args, fmt.Sprintf("--resource=%s", r)) } + if tt.resourcesCSV != "" { args = append(args, fmt.Sprintf("--resource=%s", tt.resourcesCSV)) } + if tt.includeManual { args = append(args, "--include-manual") } diff --git a/cmd/diff/diffprocessor/comp_processor.go b/cmd/diff/diffprocessor/comp_processor.go index 3779f7ad..a84b9c76 100644 --- a/cmd/diff/diffprocessor/comp_processor.go +++ b/cmd/diff/diffprocessor/comp_processor.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "os" + "strings" xp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/crossplane" "github.com/crossplane-contrib/crossplane-diff/cmd/diff/renderer" @@ -315,19 +316,7 @@ func (p *DefaultCompDiffProcessor) preflightResourceRefs(ctx context.Context, co // joinRefs renders a list of human-readable refs joined by commas. func joinRefs(refs []string) string { - switch len(refs) { - case 0: - return "" - case 1: - return refs[0] - default: - out := refs[0] - for _, r := range refs[1:] { - out += ", " + r - } - - return out - } + return strings.Join(refs, ", ") } // processSingleComposition processes a single composition and builds the result. diff --git a/cmd/diff/diffprocessor/comp_processor_test.go b/cmd/diff/diffprocessor/comp_processor_test.go index f84849ee..1e078705 100644 --- a/cmd/diff/diffprocessor/comp_processor_test.go +++ b/cmd/diff/diffprocessor/comp_processor_test.go @@ -727,7 +727,6 @@ func TestDefaultCompDiffProcessor_DiffComposition_StderrErrorOutput(t *testing.T } } - // newCompProcessorForTest builds a DefaultCompDiffProcessor wrapping the given composition client // for use by the --resource preflight tests below. func newCompProcessorForTest(t *testing.T, compClient xp.CompositionClient, includeManual bool) (*DefaultCompDiffProcessor, *bytes.Buffer) { @@ -811,13 +810,16 @@ func TestDefaultCompDiffProcessor_DiffComposition_ResourceMode(t *testing.T) { Build() proc, _ := newCompProcessorForTest(t, client, false) + _, err := proc.DiffComposition(ctx, []*un.Unstructured{comp}, "ns", nil) if err != nil { t.Fatalf("unexpected error: %v", err) } + if findCalls != 1 { t.Errorf("FindCompositesUsingComposition: expected 1 call, got %d", findCalls) } + if getByNameCalls != 0 { t.Errorf("GetCompositesByName: expected 0 calls, got %d", getByNameCalls) } @@ -837,19 +839,23 @@ func TestDefaultCompDiffProcessor_DiffComposition_ResourceMode(t *testing.T) { getByNameCalls++ // Both refs match. _ = refs + return []*un.Unstructured{xr1, xr2}, nil, nil }). Build() proc, _ := newCompProcessorForTest(t, client, false) + _, err := proc.DiffComposition(ctx, []*un.Unstructured{comp}, "", []types.ResourceRef{{Namespace: "ns", Name: "xr-1"}, {Namespace: "ns", Name: "xr-2"}}) if err != nil { t.Fatalf("unexpected error: %v", err) } + if findCalls != 0 { t.Errorf("FindCompositesUsingComposition: expected 0 calls, got %d", findCalls) } + if getByNameCalls != 1 { t.Errorf("GetCompositesByName: expected 1 call, got %d", getByNameCalls) } @@ -865,14 +871,17 @@ func TestDefaultCompDiffProcessor_DiffComposition_ResourceMode(t *testing.T) { Build() proc, stdout := newCompProcessorForTest(t, client, false) + _, err := proc.DiffComposition(ctx, []*un.Unstructured{comp}, "", []types.ResourceRef{{Namespace: "ns", Name: "ghost"}}) if err == nil { t.Fatal("expected error from globally-unmatched preflight, got nil") } + if !strings.Contains(err.Error(), "ns/ghost") { t.Errorf("error message should name the unmatched ref, got: %v", err) } + if stdout.Len() != 0 { t.Errorf("expected no output before fail-fast, got: %q", stdout.String()) } @@ -887,6 +896,7 @@ func TestDefaultCompDiffProcessor_DiffComposition_ResourceMode(t *testing.T) { Build() proc, _ := newCompProcessorForTest(t, client, false /* IncludeManual */) + _, err := proc.DiffComposition(ctx, []*un.Unstructured{comp}, "", []types.ResourceRef{{Namespace: "ns", Name: "manual-xr"}}) if err != nil { @@ -899,15 +909,19 @@ func TestDefaultCompDiffProcessor_DiffComposition_ResourceMode(t *testing.T) { if err != nil { t.Fatalf("processSingleComposition: %v", err) } + if len(got.ImpactAnalysis) != 1 { t.Fatalf("expected 1 impact entry, got %d", len(got.ImpactAnalysis)) } + if got.ImpactAnalysis[0].Status != "filtered_by_policy" { t.Errorf("expected filtered_by_policy status, got %q", got.ImpactAnalysis[0].Status) } + if got.AffectedResources.FilteredByPolicy != 1 { t.Errorf("expected FilteredByPolicy=1, got %d", got.AffectedResources.FilteredByPolicy) } + if got.AffectedResources.Total != 1 { t.Errorf("expected Total=1, got %d", got.AffectedResources.Total) } @@ -928,9 +942,11 @@ func TestDefaultCompDiffProcessor_DiffComposition_ResourceMode(t *testing.T) { if err != nil { t.Fatalf("processSingleComposition: %v", err) } + if len(got.ImpactAnalysis) != 0 { t.Errorf("default-discovery mode: expected NO impact entries for filtered-by-policy XRs, got %d (%+v)", len(got.ImpactAnalysis), got.ImpactAnalysis) } + if got.AffectedResources.FilteredByPolicy != 1 { t.Errorf("expected FilteredByPolicy count=1, got %d", got.AffectedResources.FilteredByPolicy) } diff --git a/cmd/diff/main.go b/cmd/diff/main.go index 136bd0d2..75c622de 100644 --- a/cmd/diff/main.go +++ b/cmd/diff/main.go @@ -95,16 +95,16 @@ func (f *FunctionCredentials) Decode(ctx *kong.DecodeContext) error { type CommonCmdFields struct { // Configuration options Context KubeContext `help:"Kubernetes context to use (defaults to current context)." name:"context"` - Output string `default:"diff" enum:"diff,json,yaml" help:"Output format (diff, json, or yaml)." name:"output" short:"o"` + Output string `default:"diff" enum:"diff,json,yaml" help:"Output format (diff, json, or yaml)." name:"output" short:"o"` NoColor bool `help:"Disable colorized output." name:"no-color"` Compact bool `help:"Show compact diffs with minimal context." name:"compact"` - MaxNestedDepth int `default:"10" help:"Maximum depth for nested XR recursion." name:"max-nested-depth"` + MaxNestedDepth int `default:"10" help:"Maximum depth for nested XR recursion." name:"max-nested-depth"` MaxIterations int `default:"20" help:"Maximum render iterations for requirements resolution or eventual-state simulation. Increase for complex pipelines that need more cycles to converge." name:"max-iterations"` Timeout time.Duration `default:"1m" help:"How long to run before timing out."` IgnorePaths []string `help:"Paths to ignore in diffs (e.g., 'metadata.annotations[argocd.argoproj.io/tracking-id]')." name:"ignore-paths"` - FunctionCredentials FunctionCredentials `help:"A YAML file or directory of YAML files specifying Secret credentials to pass to Functions." name:"function-credentials" placeholder:"PATH"` + FunctionCredentials FunctionCredentials `help:"A YAML file or directory of YAML files specifying Secret credentials to pass to Functions." name:"function-credentials" placeholder:"PATH"` FunctionRegistryOverride string `help:"Override the registry for all function images (e.g., 'my-company.registry.io')." name:"function-registry-override"` - EventualState bool `default:"false" help:"Show eventual state after all reconciliation cycles complete (useful with function-sequencer)." name:"eventual-state"` + EventualState bool `default:"false" help:"Show eventual state after all reconciliation cycles complete (useful with function-sequencer)." name:"eventual-state"` } // GetKubeContext implements ContextProvider. diff --git a/cmd/diff/renderer/comp_diff_renderer_test.go b/cmd/diff/renderer/comp_diff_renderer_test.go index 71d3967c..37ddd962 100644 --- a/cmd/diff/renderer/comp_diff_renderer_test.go +++ b/cmd/diff/renderer/comp_diff_renderer_test.go @@ -603,6 +603,7 @@ func TestXRStatusFilteredByPolicy_TextRenderer(t *testing.T) { if !strings.Contains(got, "manual-xr") { t.Errorf("expected XR name in output, got %q", got) } + if !strings.Contains(strings.ToLower(got), "manual") && !strings.Contains(strings.ToLower(got), "filtered") { t.Errorf("expected 'manual' or 'filtered' marker in output, got %q", got) } diff --git a/cmd/diff/types/types.go b/cmd/diff/types/types.go index baed0b47..546411e3 100644 --- a/cmd/diff/types/types.go +++ b/cmd/diff/types/types.go @@ -40,5 +40,6 @@ func (r ResourceRef) String() string { if r.Namespace == "" { return r.Name } + return r.Namespace + "/" + r.Name }