diff --git a/.requirements/20260518T224830Z_comp_resource_filter/REQUIREMENTS.md b/.requirements/20260518T224830Z_comp_resource_filter/REQUIREMENTS.md new file mode 100644 index 0000000..f97555a --- /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/.requirements/20260522T174102Z_refactor_resource_filter_architecture/REQUIREMENTS.md b/.requirements/20260522T174102Z_refactor_resource_filter_architecture/REQUIREMENTS.md new file mode 100644 index 0000000..d6a63d0 --- /dev/null +++ b/.requirements/20260522T174102Z_refactor_resource_filter_architecture/REQUIREMENTS.md @@ -0,0 +1,321 @@ +# Refactor: clean up the `--resource` filter architecture + +This document is the authoritative source during implementation. + +## As Is + +PR #322 (commit `766011d` + `538a161` lint fixes) merged the `--resource [namespace/]name` flag for `crossplane-diff comp`, which limits impact analysis to specific composites. The implementation works end-to-end and all tests pass. However, several internal abstractions don't sit cleanly: + +1. **Boolean mode flag with redundant signaling.** `processSingleComposition(ctx, newComp, namespace, preMatched []*Unstructured, resourceMode bool)` at `cmd/diff/diffprocessor/comp_processor.go:326` takes both a `preMatched` slice AND a `resourceMode` bool. The two parameters signal the same mode (`resourceMode==true ⇔ preMatched is the source of truth`), with the bool *additionally* gating an unrelated behavior — whether to surface filtered composites in `ImpactAnalysis` as `XRStatusFilteredByPolicy`. + +2. **Interface bloat on `CompositionClient`.** Two methods overlap: + - `FindCompositesUsingComposition(ctx, name string, namespace string) ([]*Unstructured, error)` — discovery via cluster listing + - `GetCompositesByName(ctx, comp *Composition, refs []ResourceRef) ([]*Unstructured, []ResourceRef, error)` — lookup by user-named refs + + They answer the same question ("which composites in the cluster reference this composition?") with different lookup strategies. The interface now has 6 methods, tripping the `interfacebloat` linter and forcing a `//nolint:interfacebloat` directive at `composition_client.go:23`. + +3. **Unused return value.** `GetCompositesByName` returns `(matched, unmatched, error)` but its only caller `preflightResourceRefs` at `comp_processor.go:279` ignores the `unmatched` slice (`matched, _, err := ...`) and re-derives unmatched-ness itself from `matched` (lines 286-303). + +4. **Long, multi-phase function.** `GetCompositesByName` at `composition_client.go:838-942` is ~100 lines doing two distinct phases inline: (a) GVK resolution (XR GVK from composition spec, claim GVK from XRD) and (b) per-ref iteration with claim-GVK fallback. + +5. **Home-grown name+namespace type.** `cmd/diff/types/types.go:33-45` defines `ResourceRef{Namespace, Name string}` with a `String()` method that returns bare `"foo"` for cluster-scoped (empty namespace) and `"default/foo"` for namespaced. `k8s.io/apimachinery/pkg/types.NamespacedName` already exists with the same data shape, but its `String()` always returns `Namespace + "/" + Name` — so cluster-scoped renders as `"/foo"`. To preserve the user-facing rendering, the swap requires a small `formatRef` helper. + +6. **`cmd/diff/types/` package** also contains `CompositionProvider` — a `func(ctx, *Unstructured) (*Composition, error)` used by `diff_processor.go`, `comp_processor.go`, `mocks.go` etc. The package can NOT be deleted; only `ResourceRef` (and its `String()`) can be removed. The pre-existing `revive: var-naming: avoid meaningless package names` warning at `types.go:18` therefore stays — fixing it would require a package rename touching every import site, which is out of scope for this refactor. + +## To Be + +After the refactor: + +1. **No mode flag with redundant signaling.** `processSingleComposition(ctx, newComp, affectedXRs []*Unstructured, surfaceFiltered bool)` always takes a pre-resolved slice. The remaining boolean `surfaceFiltered` honestly names what it controls (whether dropped-by-policy XRs go into `ImpactAnalysis` as `XRStatusFilteredByPolicy` entries, vs. only being counted in the summary). The discovery-vs-preMatched decision is hoisted up to `DiffComposition`. + +2. **One unified method on `CompositionClient`.** `FindComposites(ctx, comp *Composition, opts FindCompositesOptions) ([]*Unstructured, error)`. `FindCompositesOptions` has `Namespace string` (for default discovery) and `Refs []NamespacedName` (for user-named lookup). The two old methods are gone. Interface size returns to 5 methods. `//nolint:interfacebloat` directive removed. + +3. **No unused return.** `FindComposites` returns `([]*Unstructured, error)`. Globally-unmatched derivation stays in the processor where it belongs (it's a cross-composition concept, not a per-call concept). + +4. **`findByRefs` split into two clear phases:** `resolveCompositeTypes(ctx, comp)` returns a `compositeTypes{xrGVK, claimGVK}` struct, and `lookupRef(ctx, ref, types, compName)` does the per-ref XR-then-claim lookup. + +5. **`NamespacedName` everywhere `ResourceRef` was used.** All `[]dtypes.ResourceRef` slices become `[]k8stypes.NamespacedName`. `ResourceRef` is removed from `cmd/diff/types/types.go`; the package itself stays (it still hosts `CompositionProvider`). A small `formatRef(NamespacedName) string` helper renders cluster-scoped refs as bare `name` (preserving the current contract). Production `ref.String()` call sites that were rendering for human consumption switch to `formatRef(ref)`. Function signatures updated; tests updated; mocks updated. + +6. **Lint clean.** `interfacebloat` and `revive: package types` issues both resolved. `golangci-lint run ./cmd/diff/...` produces zero new issues over the post-PR-#322 baseline. + +User-visible CLI behavior is **unchanged**. All E2E tests should continue to pass without modification. + +## Requirements + +### R1 — `ResourceRef` is replaced by `k8s.io/apimachinery/pkg/types.NamespacedName` + +The `ResourceRef` struct and its `String()` method are removed from `cmd/diff/types/types.go`. The `CompositionProvider` declaration in that file stays (it's used elsewhere). Every reference to `dtypes.ResourceRef` in production and test code switches to `k8stypes.NamespacedName` (alias `k8stypes "k8s.io/apimachinery/pkg/types"`). A new `cmd/diff/ref` package owns CLI ref I/O: `ref.Parse([namespace/]name string) (NamespacedName, error)`, `ref.ParseAll([]string)`, and `ref.Format(NamespacedName) string` (the inverse of Parse — preserves bare `name` for cluster-scoped). Both the CLI parsing and the processor's user-facing error messages use this package. + +### R2 — `CompositionClient` exposes a single `FindComposites` method with options + +The interface methods `FindCompositesUsingComposition` and `GetCompositesByName` are removed. A new method `FindComposites(ctx, comp *Composition, opts FindCompositesOptions) ([]*Unstructured, error)` replaces both. `FindCompositesOptions` contains: +- `Namespace string` — scopes default-discovery +- `Refs []NamespacedName` — limits results to user-named refs + +When `len(opts.Refs) > 0`, the implementation performs ref-based lookup; otherwise it performs default discovery scoped to `opts.Namespace`. + +### R3 — Internal phase split on the ref-based lookup + +The implementation has two private helpers: `resolveCompositeTypes(ctx, comp) (compositeTypes, error)` and `lookupRef(ctx, ref, types, compName) (*Unstructured, error)`. The composite `compositeTypes` struct holds `{xrGVK, claimGVK}`. `lookupRef` returns nil for "not found or wrong composition" and propagates non-404 cluster errors. + +### R4 — `processSingleComposition` takes pre-resolved XRs and a single mode flag + +Signature: `processSingleComposition(ctx, newComp *Unstructured, affectedXRs []*Unstructured, surfaceFiltered bool) (*CompositionDiff, error)`. The caller (`DiffComposition`) decides which set of XRs to pass and whether `surfaceFiltered` should be true. `processSingleComposition` no longer calls `FindCompositesUsingComposition`/`FindComposites` itself. + +### R5 — Discovery branch lives in `DiffComposition`'s per-composition loop, structured with `switch` not `if/else` + +For each Composition (after the `comp.GetKind() != "Composition"` filter), `DiffComposition`: +- If `len(resources) > 0`: takes `affectedXRs` from `preflightMatches[comp.GetName()]`; sets `surfaceFiltered=true` +- Else: calls `p.compositionClient.FindComposites(ctx, typedComp, FindCompositesOptions{Namespace: namespace})`; on error, logs at debug level and uses `nil` (net-new composition graceful path); sets `surfaceFiltered=false` + +The branching uses `switch` blocks at both the outer (resource-mode vs default) and inner (find-error vs ok) levels. No `if/else` chains. + +### R6 — Mocks and tests are migrated to the new method + +`MockCompositionClient` has `FindCompositesFn` (replacing `FindCompositesUsingCompositionFn` and `GetCompositesByNameFn`). `MockCompositionClientBuilder` has `WithFindComposites(fn)`. The convenience wrappers `WithResourcesForComposition` and `WithFindResourcesError` keep their existing public surface but route through `FindCompositesFn`, disambiguating from ref-mode by checking `len(opts.Refs) == 0`. + +All affected tests (`comp_processor_test.go`, `composition_client_test.go`, `comp_test.go`) pass with the new types and mock API. Test data shape switches from `dtypes.ResourceRef` to `k8stypes.NamespacedName`. Expected `String()` outputs are unchanged. + +### R7 — `//nolint:interfacebloat` is removed + +The directive at `composition_client.go:23` is deleted; lint passes without it. + +## Acceptance Criteria + +### AC for R1 (NamespacedName replaces ResourceRef) + +- AC1.1: `cmd/diff/types/types.go` no longer declares a `ResourceRef` type or its `String()` method. `CompositionProvider` is still present. +- AC1.2: `grep -rn "dtypes\.ResourceRef\|\\btypes\\.ResourceRef\\b" cmd/diff/` returns no matches (production or test). +- AC1.3: `grep -rn "k8stypes\\.NamespacedName" cmd/diff/` returns matches in: `comp.go`, `comp_processor.go`, `composition_client.go`, `comp_test.go`, `comp_processor_test.go`, `composition_client_test.go`, `mocks.go`, `mock_builder.go`. +- AC1.4: A `Format` function exists in `cmd/diff/ref/ref.go` and renders cluster-scoped (empty namespace) as bare `Name` and namespaced as `Namespace + "/" + Name`. Unit tests cover both cases. (Co-located with `Parse` and `ParseAll` — the package owns CLI ref I/O.) +- AC1.5: All existing user-facing rendering of refs (error messages and structured logs) is identical to today: `default/foo` for namespaced, `foo` (NOT `/foo`) for cluster-scoped. Verified by adapted `TestParseResourceRef` (string outputs unchanged) and at least one preflight error-message test exercising the cluster-scoped case. + +### AC for R2 (unified FindComposites) + +- AC2.1: `CompositionClient` interface in `composition_client.go` has exactly 5 methods (was 6). `FindComposites` is one of them; `FindCompositesUsingComposition` and `GetCompositesByName` are absent. +- AC2.2: `FindCompositesOptions` struct exists in `composition_client.go` with exported fields `Namespace string` and `Refs []k8stypes.NamespacedName`. +- AC2.3: A unit test calls `FindComposites` with `opts.Refs == nil` and gets default-discovery behavior (matches the old `FindCompositesUsingComposition` semantics). +- AC2.4: A unit test calls `FindComposites` with `opts.Refs != nil` and gets ref-based lookup behavior (matches the old `GetCompositesByName` semantics, minus the `unmatched` return). + +### AC for R3 (internal phase split) + +- AC3.1: `composition_client.go` defines a `compositeTypes` struct with `xrGVK` and `claimGVK schema.GroupVersionKind`. +- AC3.2: `composition_client.go` defines private methods `resolveCompositeTypes(ctx, comp) (compositeTypes, error)` and `lookupRef(ctx, ref, types, compName) (*Unstructured, error)` on `*DefaultCompositionClient`. +- AC3.3: `findByRefs` body is concise (no inline GVK resolution, no inline per-ref claim-fallback logic) — those phases are delegated. +- AC3.4: A unit test exercises `lookupRef` directly with mocked resource client (XR-found-and-matches, XR-not-found-claim-found-and-matches, both-not-found-returns-nil, XR-found-but-wrong-composition-returns-nil, cluster-error-propagates). + +### AC for R4 (processSingleComposition takes pre-resolved XRs) + +- AC4.1: `processSingleComposition` signature is `(ctx, newComp *Unstructured, affectedXRs []*Unstructured, surfaceFiltered bool) (*CompositionDiff, error)`. +- AC4.2: `processSingleComposition` body contains no `FindComposites*` or discovery calls. +- AC4.3: A unit test calls `processSingleComposition` directly with `surfaceFiltered=true` and verifies that Manual-policy XRs in `affectedXRs` are surfaced as `XRStatusFilteredByPolicy` impacts. +- AC4.4: A unit test calls `processSingleComposition` directly with `surfaceFiltered=false` and verifies that Manual-policy XRs are *not* surfaced as impacts (only counted in summary). + +### AC for R5 (DiffComposition orchestration with switch) + +- AC5.1: `DiffComposition`'s per-composition loop uses `switch { case ...: ...; default: ...; }` for the resource-mode-vs-default branch (no `else`). +- AC5.2: The find-error branch within default uses `switch { case findErr != nil: ...; default: ... }` (no `else`). +- AC5.3: The graceful net-new behavior is preserved: when `FindComposites` returns an error, `affectedXRs = nil` and processing continues, producing a result with empty `ImpactAnalysis`. +- AC5.4: An integration test (existing in `diff_integration_test.go`) for net-new composition default-discovery still passes without modification. + +### AC for R6 (mocks migrated) + +- AC6.1: `MockCompositionClient` struct has `FindCompositesFn` field; `FindCompositesUsingCompositionFn` and `GetCompositesByNameFn` fields are absent. +- AC6.2: `MockCompositionClientBuilder` has `WithFindComposites(fn)` method; `WithFindCompositesUsingComposition` and `WithGetCompositesByName` are absent. +- AC6.3: Convenience wrappers `WithResourcesForComposition` and `WithFindResourcesError` are still present and behave identically to before, but their internal implementation routes through `FindCompositesFn`. +- AC6.4: All tests in `comp_processor_test.go` and `composition_client_test.go` compile and pass with the new mock API. + +### AC for R7 (lint clean) + +- AC7.1: `//nolint:interfacebloat` directive in `composition_client.go` is gone. +- AC7.2: `golangci-lint run ./cmd/diff/...` reports zero `interfacebloat` issues. +- AC7.3: `golangci-lint run ./cmd/diff/...` reports no NEW `revive: package types` issues. The two pre-existing `revive: package types` issues at `cmd/diff/types/types.go:18` and `cmd/diff/renderer/types/types.go:2` remain — they are out of scope for this refactor (fixing them requires a package rename touching every import site). + +### AC overall + +- AC-OVR-1: `earthly -P +reviewable` passes (unit tests + lint + generation). +- AC-OVR-2: `earthly -P +e2e --CROSSPLANE_IMAGE_TAG=main --FLAGS="-test.run TestCompositionDiff"` passes (composition diff E2E — smoke that user-visible behavior is unchanged). +- AC-OVR-3: User-visible behavior verified by spot check on a real cluster: default discovery, `--resource=ns/name`, `--resource=does/not-exist` (preflight error), bare-name cluster-scoped ref. + +## Testing Plan + +This refactor is heavily test-supported. Almost all behavior is already covered by the test suite from PR #322; we adapt those tests rather than write new ones from scratch. New tests are added to validate the smaller helpers introduced in R3. + +### Layer 1 — Compile-driven (shape changes) + +Tests that fail at compile time when the type/signature is wrong. These guide the structural changes: + +- **TC-R1.1**: Existing `TestParseResourceRef` in `comp_test.go` — change expected return type from `dtypes.ResourceRef` to `k8stypes.NamespacedName`. No assertion changes (string outputs identical). +- **TC-R6.1**: Existing mock setups in `comp_processor_test.go` — migrate to `WithFindComposites`. Compile failure if the new method doesn't exist or has wrong signature. + +### Layer 2 — Direct unit tests (new, for new helpers) + +- **TC-R3.1**: `TestResolveCompositeTypes` (new) — table-driven tests in `composition_client_test.go`: + - case: composition with valid `compositeTypeRef` and matching XRD → returns both XR GVK and claim GVK + - case: composition with valid `compositeTypeRef` but XRD lookup fails → returns XR GVK, empty claim GVK, no error + - case: composition with valid `compositeTypeRef` but XRD has no `claimNames` → returns XR GVK, empty claim GVK, no error + - case: composition with malformed `compositeTypeRef.apiVersion` → returns error +- **TC-R3.2**: `TestLookupRef` (new) — table-driven tests in `composition_client_test.go`: + - case: ref matches via XR GVK and uses target composition → returns object + - case: ref matches via XR GVK but uses different composition AND claim 404s → returns nil, no error (XR was wrong-composition; claim fallback found nothing) + - case: same-name XR+Claim collision — XR uses other-comp, Claim uses target-comp → falls through to claim GVK and returns the Claim (F1 fix from Copilot review on PR #322) + - case: ref XR-404, then matches via claim GVK and uses target composition → returns object + - case: ref XR-404, claim GVK 404 → returns nil, no error + - case: ref XR-404 and claim GVK is empty (XRD missing) → returns nil, no error + - case: ref XR returns non-404 cluster error → propagates error +- **TC-R4.3, TC-R4.4**: New tests in `comp_processor_test.go` for `processSingleComposition` directly with `surfaceFiltered=true/false` and Manual-policy XRs. + +### Layer 3 — Adapted unit tests (existing, signatures change) + +- **TC-R2.3, TC-R2.4**: Existing tests for `GetCompositesByName` in `composition_client_test.go` (named `TestGetCompositesByName`) → migrate to `TestFindComposites_WithRefs` (call the new method with `opts.Refs` populated). Existing tests for `FindCompositesUsingComposition` → migrate to `TestFindComposites_DefaultDiscovery` (call with `opts.Refs == nil`). +- **TC-R6.4**: Other tests in `comp_processor_test.go` that hit the affected code paths (preflight, default discovery in `DiffComposition`) → re-check after migration. + +### Layer 4 — Integration tests (existing, minimal changes) + +- **TC-R5.4**: `TestDiffCompositionWithGetComposedResource` and similar in `diff_integration_test.go` should pass without modification since they exercise CLI behavior, not internal Go signatures. + +### Layer 5 — Lint signal + +- **TC-R7.2, TC-R7.3**: `earthly -P +reviewable` passes cleanly (no new lint issues). + +### TDD ordering principle + +For each implementation step in the plan below, the test changes go first, then the implementation. The test should fail initially (red), then pass after the implementation step (green). Where an existing test is adapted, the adaptation IS the failing test — we change expectations, then refactor production code to satisfy them. + +## Implementation Plan + +The change is one logical refactor but is implemented in 5 small steps, each independently testable. The user has stated they will commit themselves; we land everything in one staged working tree at the end. + +### Step 1 — Type swap: `ResourceRef` → `NamespacedName` + add `formatRef` + +**Goal:** All `dtypes.ResourceRef` references replaced by `k8stypes.NamespacedName`. `ResourceRef` (and only `ResourceRef`) removed from `cmd/diff/types/types.go`. New `formatRef` helper preserves the bare-name rendering for cluster-scoped refs in user-facing strings. No other behavioral changes. + +**Test-first:** +- TC-R1.1: Update `comp_test.go` `TestParseResourceRef` — change `wantRef` field type and expected values from `types.ResourceRef{...}` to `k8stypes.NamespacedName{...}`. Add at least one assertion exercising `formatRef(got) == tt.wantString` to lock in the rendering contract (cluster-scoped: bare `name`; namespaced: `ns/name`). +- TC-R1.2 (new): Add `TestFormatRef` table-driven test in `comp_test.go` — covers `{Name: "foo"}` → `"foo"`, `{Namespace: "default", Name: "foo"}` → `"default/foo"`, and the empty edge case `{}` → `""`. +- Run `go test ./cmd/diff/...` — should fail at compile time on production code that still uses `dtypes.ResourceRef`. + +**Implementation:** +- Add `Format(n k8stypes.NamespacedName) string` to `cmd/diff/ref/ref.go` alongside `Parse`/`ParseAll`. The new package owns CLI ref I/O; both the CLI (parses incoming `--resource` values) and the processor (renders unmatched refs in error messages) call into it. +- Add `import k8stypes "k8s.io/apimachinery/pkg/types"` everywhere `cmd/diff/types` was imported as `dtypes` (or where `types.ResourceRef` was used). +- Mechanical rename: `dtypes.ResourceRef` → `k8stypes.NamespacedName` across all production and test files. +- Update `parseResourceRef` return type and constructors. +- In `cmd/diff/types/types.go`: delete the `ResourceRef` struct and its `String()` method. Keep `CompositionProvider`. Update the package doc comment if it mentions both. +- Remove the import alias `dtypes "github.com/crossplane-contrib/crossplane-diff/cmd/diff/types"` from files that no longer need anything from it (some files may keep importing the package as `types` for `CompositionProvider`). +- Replace user-facing `ref.String()` calls with `formatRef(ref)`. Specifically: the error-message construction in `preflightResourceRefs` at `comp_processor.go:306-311` (the loop that builds `names` for the `--resource ref(s) not relevant` error). Internal-only `ref.String()` calls used as map keys (e.g., `matchedAtLeastOnce[ref.String()]` at line 291, 300) can stay — those just need stable string keys, and `NamespacedName.String()` works fine for that purpose. + +**Verify:** `go test ./cmd/diff/...` passes. AC1.1, AC1.2, AC1.3, AC1.4, AC1.5. + +### Step 2 — Introduce `FindComposites` and `FindCompositesOptions` + +**Goal:** New unified method available; old methods still present (added, not yet swapped). Preserves behavior of both old methods. + +**Test-first:** +- Add a new test `TestFindComposites_DefaultDiscovery` in `composition_client_test.go` that calls `FindComposites(ctx, comp, FindCompositesOptions{Namespace: ns})` and asserts the same behavior as existing `TestFindCompositesUsingComposition`. +- Add a new test `TestFindComposites_WithRefs` that calls `FindComposites(ctx, comp, FindCompositesOptions{Refs: refs})` and asserts the same behavior as existing `TestGetCompositesByName` (matched-only return). + +**Implementation:** +- Add `FindCompositesOptions` struct to `composition_client.go`. +- Add `FindComposites(ctx, comp, opts)` method to the `CompositionClient` interface AND to `DefaultCompositionClient`. Body branches via `switch` on `len(opts.Refs)`: + - `case len(opts.Refs) > 0:` → call new private `findByRefs` (extract from existing `GetCompositesByName` body, drop `unmatched` return) + - `default:` → call new private `findByListing` (extract from existing `FindCompositesUsingComposition` body) +- Add `FindCompositesFn` to `MockCompositionClient`. +- Add `WithFindComposites` to `MockCompositionClientBuilder`. + +**Verify:** New tests pass. Old tests still pass. AC2.1 (partial — interface still has 7 methods at this stage), AC2.2, AC2.3, AC2.4. + +### Step 3 — Migrate callers and remove old methods + +**Goal:** Production callers (`preflightResourceRefs`, `processSingleComposition`) now call `FindComposites`. Old interface methods removed. + +**Test-first:** +- Migrate `comp_processor_test.go` mock setups: `WithFindCompositesUsingComposition(fn)` → `WithFindComposites(fn)` (with appropriate `opts.Refs == nil` check inside). `WithGetCompositesByName(fn)` → `WithFindComposites(fn)` (with `opts.Refs != nil` check). +- The convenience wrappers `WithResourcesForComposition` and `WithFindResourcesError` get re-routed internally to set `FindCompositesFn` (with `len(opts.Refs) == 0` check); their public API stays the same so test sites that use them don't change. +- Existing tests for `TestFindCompositesUsingComposition` and `TestGetCompositesByName` get renamed to `TestFindComposites_DefaultDiscovery` and `TestFindComposites_WithRefs` respectively. The new tests added in Step 2 either replace these or merge with them. +- Run `go test ./cmd/diff/...` — production code should fail compilation. + +**Implementation:** +- In `comp_processor.go`: change `preflightResourceRefs` to call `p.compositionClient.FindComposites(ctx, typedComp, FindCompositesOptions{Refs: refs})`, capturing only `(matched, err)`. +- In `comp_processor.go`: change `processSingleComposition`'s discovery branch to call `p.compositionClient.FindComposites(ctx, typedComp, FindCompositesOptions{Namespace: namespace})`. (We'll move this branch up to `DiffComposition` in Step 5; for now, just swap the call.) +- Remove `FindCompositesUsingComposition` and `GetCompositesByName` from the `CompositionClient` interface and from `DefaultCompositionClient`. +- Remove `FindCompositesUsingCompositionFn` and `GetCompositesByNameFn` fields from `MockCompositionClient`. +- Remove `WithFindCompositesUsingComposition` and `WithGetCompositesByName` from `MockCompositionClientBuilder`. +- Remove `//nolint:interfacebloat` directive from `composition_client.go:23`. + +**Verify:** `go test ./cmd/diff/...` passes. `golangci-lint run ./cmd/diff/...` shows no `interfacebloat` issue. AC2.1 (now satisfied), AC6.1, AC6.2, AC6.3, AC6.4, AC7.1, AC7.2. + +### Step 4 — Split `findByRefs` into phases + +**Goal:** `findByRefs` body is short; `resolveCompositeTypes` and `lookupRef` are private helpers with their own tests. + +**Test-first:** +- Add `TestResolveCompositeTypes` (table-driven, 4 cases per testing plan TC-R3.1). +- Add `TestLookupRef` (table-driven, 6 cases per testing plan TC-R3.2). Use `MockResourceClient` and `MockDefinitionClient` from existing testutils. + +**Implementation:** +- Extract `resolveCompositeTypes(ctx, comp) (compositeTypes, error)` from current `findByRefs`. Move XR GVK parsing + XRD lookup + claim GVK extraction here. +- Extract `lookupRef(ctx, ref, types, compName) (*Unstructured, error)` from current `findByRefs`. Move per-ref XR-then-claim lookup logic here. +- Rewrite `findByRefs` body to: call `resolveCompositeTypes`, loop over refs calling `lookupRef`, accumulate non-nil results. + +**Verify:** New tests pass. Existing `TestFindComposites_WithRefs` still passes. AC3.1, AC3.2, AC3.3, AC3.4. + +### Step 5 — Hoist discovery into `DiffComposition`, drop `resourceMode` + +**Goal:** `processSingleComposition` takes pre-resolved `affectedXRs` and a `surfaceFiltered` bool. `DiffComposition` decides the source via `switch` blocks. + +**Test-first:** +- Existing tests in `comp_processor_test.go` that call `processSingleComposition` directly (lines 908, 941 per earlier exploration) need their argument lists updated to the new signature. The two existing test cases already exercise both modes (`(nil, false)` for discovery, `([]Unstructured{manualXR}, true)` for resource). +- Adapt these to call the new signature directly with pre-resolved XRs. For the discovery-mode test, the discovery is no longer in `processSingleComposition` — so the equivalent test becomes "call `DiffComposition` with no `--resource` and observe the same end result," OR we rewrite to test `processSingleComposition` with explicitly pre-resolved XRs. +- New direct tests **TC-R4.3, TC-R4.4** exercise the `surfaceFiltered=true/false` semantics independently of any discovery. + +**Implementation:** +- Change `processSingleComposition` signature to `(ctx, newComp, affectedXRs, surfaceFiltered)`. +- Inside `processSingleComposition`: replace the discovery branch with direct use of `affectedXRs`. Remove the `if resourceMode { ... } else { discover... }` block. Rename `if resourceMode` (around line 383) → `if surfaceFiltered`. Body of that block unchanged. +- In `DiffComposition`'s per-composition loop, add the resolution `switch`: + ```go + var ( + affectedXRs []*un.Unstructured + surfaceFiltered bool + ) + switch { + case len(resources) > 0: + affectedXRs = preflightMatches[comp.GetName()] + surfaceFiltered = true + default: + typedComp := &apiextensionsv1.Composition{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(comp.Object, typedComp); err != nil { + // existing error handling + } + discovered, findErr := p.compositionClient.FindComposites(ctx, typedComp, FindCompositesOptions{Namespace: namespace}) + switch { + case findErr != nil: + p.config.Logger.Debug("Cannot find composites using composition (likely net-new)", + "composition", comp.GetName(), "error", findErr) + affectedXRs = nil + default: + affectedXRs = discovered + } + surfaceFiltered = false + } + ``` +- Update the call: `compResult, err := p.processSingleComposition(ctx, comp, affectedXRs, surfaceFiltered)`. +- Update `comp_processor_test.go`'s direct `processSingleComposition` calls to match the new signature. + +**Verify:** All unit tests pass. AC4.1, AC4.2, AC4.3, AC4.4, AC5.1, AC5.2, AC5.3, AC5.4. + +### Final verification + +After all 5 steps: +- `cd cmd/diff && go test ./...` — all unit tests pass (fast feedback). +- `earthly -P +reviewable` — full lint + test + generation passes (AC-OVR-1, AC7.2, AC7.3). +- `earthly -P +e2e --CROSSPLANE_IMAGE_TAG=main --FLAGS="-test.run TestCompositionDiff"` — comp diff E2E passes (AC-OVR-2). +- Spot-check binary against a real cluster (AC-OVR-3) — the user can do this themselves before committing. + +### Code review checkpoints + +After each step, invoke the `superpowers:code-reviewer` subagent (in lieu of Lad MCP, which is not available in this environment) to review the diff for: +- Adherence to CLAUDE.md (machine-readable error handling, accuracy-above-all) +- Adherence to project style (switch over else, table-driven tests, etc.) +- Catch any CLAUDE.md-mandated patterns I missed + +Apply review feedback before moving to the next step. diff --git a/README.md b/README.md index dc084f8..1473128 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 452fe98..1918f01 100644 --- a/cmd/diff/client/crossplane/composition_client.go +++ b/cmd/diff/client/crossplane/composition_client.go @@ -7,9 +7,13 @@ import ( "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/core" "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/kubernetes" + "github.com/crossplane-contrib/crossplane-diff/cmd/diff/ref" + 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" + k8stypes "k8s.io/apimachinery/pkg/types" "github.com/crossplane/crossplane-runtime/v2/pkg/errors" "github.com/crossplane/crossplane-runtime/v2/pkg/logging" @@ -30,8 +34,28 @@ type CompositionClient interface { // GetComposition gets a composition by name GetComposition(ctx context.Context, name string) (*apiextensionsv1.Composition, error) - // FindCompositesUsingComposition finds all composites (XRs and Claims) that use the specified composition - FindCompositesUsingComposition(ctx context.Context, compositionName string, namespace string) ([]*un.Unstructured, error) + // FindComposites locates composites (XRs and Claims) that reference a composition. + // `comp` is taken as the user-supplied unstructured Composition (typically loaded from a YAML + // file); the client converts to a typed *apiextensionsv1.Composition internally only when + // needed (i.e. in refs-mode, which reads spec.compositeTypeRef). Default-discovery only needs + // comp.GetName() and avoids the conversion entirely so it isn't tripped by version skew or + // unknown fields in the input file. + // + // When opts.Refs is non-empty, it performs ref-based lookup: each ref is resolved against the + // composition's XR GVK (then claim GVK on 404 if the XRD defines a claim type). A ref is included + // in the result only when (a) the cluster lookup succeeds AND (b) the resource references this + // composition by name. Refs that don't satisfy both are silently omitted; callers derive + // "unmatched" from the diff between input refs and returned objects. NotFound on per-ref XR/Claim + // lookups is tolerated; non-NotFound transport errors propagate. + // + // When opts.Refs is empty, it performs default discovery scoped to opts.Namespace, listing all + // XRs (and Claims, if the XRD defines them) of the appropriate GVK and filtering by composition. + // XR/Claim list errors are tolerated (each list is best-effort and produces an empty bucket on + // failure). However, the composition itself must be in the cluster cache: when GetComposition + // returns NotFound (a net-new composition not yet applied), FindComposites returns that error to + // the caller. Callers handle that case by treating it as "no affected XRs" — see + // DiffComposition's default-discovery branch. + FindComposites(ctx context.Context, comp *un.Unstructured, opts dtypes.FindCompositesOptions) ([]*un.Unstructured, error) } // DefaultCompositionClient implements CompositionClient. @@ -689,8 +713,11 @@ func (c *DefaultCompositionClient) findByTypeReference(ctx context.Context, _ *u return compatibleCompositions[0], nil } -// FindCompositesUsingComposition finds all composites (XRs and Claims) that use the specified composition. -func (c *DefaultCompositionClient) FindCompositesUsingComposition(ctx context.Context, compositionName string, namespace string) ([]*un.Unstructured, error) { +// findByListing implements default-discovery for FindComposites: list every XR (and Claim, if the +// XRD defines one) of the composition's target GVK, scoped to namespace, and filter by composition. +// If the composition itself isn't in the cluster (net-new), the GetComposition lookup fails and the +// error propagates to the caller, which is expected to handle "net-new" gracefully. +func (c *DefaultCompositionClient) findByListing(ctx context.Context, compositionName string, namespace string) ([]*un.Unstructured, error) { c.logger.Debug("Finding composites using composition", "compositionName", compositionName, "namespace", namespace) @@ -822,3 +849,157 @@ func (c *DefaultCompositionClient) resourceUsesComposition(resource *un.Unstruct return false } + +// findByRefs implements ref-based lookup for FindComposites: each ref is resolved against the +// composition's XR GVK first, then the claim GVK on 404 if the XRD defines a claim type. Refs that +// fail both lookups, or that exist but reference a different composition, are silently dropped from +// the result. NotFound responses are tolerated; other errors propagate. +// +// Converts the unstructured Composition to a typed *apiextensionsv1.Composition internally because +// resolveCompositeTypes needs spec.compositeTypeRef. Conversion errors propagate. +func (c *DefaultCompositionClient) findByRefs(ctx context.Context, comp *un.Unstructured, refs []k8stypes.NamespacedName) ([]*un.Unstructured, error) { + if len(refs) == 0 { + return nil, nil + } + + 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 refs lookup", comp.GetName()) + } + + types, err := c.resolveCompositeTypes(ctx, typedComp) + if err != nil { + return nil, err + } + + var matched []*un.Unstructured + + for _, n := range refs { + obj, err := c.lookupRef(ctx, n, types, comp.GetName()) + if err != nil { + return nil, err + } + + if obj != nil { + matched = append(matched, obj) + } + } + + return matched, nil +} + +// compositeTypes holds the GVKs needed to look up a composite by name. claimGVK is empty +// when the XRD has no claimNames or could not be retrieved (graceful degradation). +type compositeTypes struct { + xrGVK schema.GroupVersionKind + claimGVK schema.GroupVersionKind +} + +// resolveCompositeTypes derives the XR GVK from the composition's CompositeTypeRef and +// best-effort resolves the claim GVK from the XRD. XRD lookup failures and missing claimNames +// produce an empty claim GVK without erroring (graceful degradation). +func (c *DefaultCompositionClient) resolveCompositeTypes(ctx context.Context, comp *apiextensionsv1.Composition) (compositeTypes, error) { + gv, err := schema.ParseGroupVersion(comp.Spec.CompositeTypeRef.APIVersion) + if err != nil { + return compositeTypes{}, errors.Wrapf(err, "cannot parse compositeTypeRef apiVersion %q", comp.Spec.CompositeTypeRef.APIVersion) + } + + types := compositeTypes{ + xrGVK: schema.GroupVersionKind{ + Group: gv.Group, + Version: gv.Version, + Kind: comp.Spec.CompositeTypeRef.Kind, + }, + } + + xrd, xrdErr := c.definitionClient.GetXRDForXR(ctx, types.xrGVK) + + switch { + case xrdErr != nil: + c.logger.Debug("XRD lookup failed; skipping claim-GVK fallback for --resource lookups", + "xrGVK", types.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 { + types.claimGVK = gvk + } + } + + return types, nil +} + +// lookupRef resolves a single ref against (compName, types). Tries the XR GVK first; on miss +// (404 OR found-but-wrong-composition) falls through to the claim GVK if non-empty. Returns the +// matched object, or nil if neither GVK yielded a resource referencing compName. Non-NotFound +// cluster errors propagate. +// +// "Found-but-wrong-composition" falls through (rather than short-circuiting to nil) so a same-name +// XR + Claim collision in v2 namespaces resolves correctly: e.g. an XR `default/foo` (XBucket) +// using composition Y and a Claim `default/foo` (Bucket) using composition X both exist; a +// `--resource=default/foo` lookup against composition X must reach the Claim via the claim-GVK +// path even though the XR GET succeeds first. +func (c *DefaultCompositionClient) lookupRef(ctx context.Context, n k8stypes.NamespacedName, types compositeTypes, compName string) (*un.Unstructured, error) { + obj, err := c.tryLookupAtGVK(ctx, types.xrGVK, n, compName, "XR GVK") + if err != nil { + return nil, err + } + + if obj != nil { + return obj, nil + } + + if types.claimGVK.Empty() { + c.logger.Debug("ref not matched via XR GVK and no claim GVK to try", + "ref", n.String()) + + return nil, nil + } + + return c.tryLookupAtGVK(ctx, types.claimGVK, n, compName, "claim GVK") +} + +// tryLookupAtGVK fetches a single resource at (gvk, n.Namespace, n.Name) and returns it iff +// the resource exists AND references compName. Returns (nil, nil) for both 404 and +// found-but-wrong-composition — callers distinguish "missed at this GVK, try the next" from +// "matched at this GVK". Non-NotFound errors propagate. +func (c *DefaultCompositionClient) tryLookupAtGVK(ctx context.Context, gvk schema.GroupVersionKind, n k8stypes.NamespacedName, compName, kindLabel string) (*un.Unstructured, error) { + obj, err := c.resourceClient.GetResource(ctx, gvk, n.Namespace, n.Name) + + switch { + case apierrors.IsNotFound(err): + c.logger.Debug("ref not found at GVK", + "ref", n.String(), "gvk", gvk.String(), "via", kindLabel) + + return nil, nil + case err != nil: + // Render the ref the way the user typed it on the CLI ("foo" for cluster-scoped, + // "ns/foo" for namespaced) — n.String() always renders "/foo" for cluster-scoped, + // which contradicts the documented --resource format in user-facing errors. + return nil, errors.Wrapf(err, "cannot fetch composite %s as %s", ref.Format(n), gvk) + } + + if c.resourceUsesComposition(obj, compName) { + c.logger.Debug("matched ref", + "ref", n.String(), "composition", compName, "via", kindLabel) + + return obj, nil + } + + c.logger.Debug("ref exists at GVK but does not reference this composition", + "ref", n.String(), "gvk", gvk.String(), "composition", compName, "via", kindLabel) + + return nil, nil +} + +// FindComposites dispatches to ref-based or listing-based discovery based on opts.Refs. +func (c *DefaultCompositionClient) FindComposites(ctx context.Context, comp *un.Unstructured, opts dtypes.FindCompositesOptions) ([]*un.Unstructured, error) { + switch { + case len(opts.Refs) > 0: + return c.findByRefs(ctx, comp, opts.Refs) + default: + return c.findByListing(ctx, comp.GetName(), opts.Namespace) + } +} diff --git a/cmd/diff/client/crossplane/composition_client_test.go b/cmd/diff/client/crossplane/composition_client_test.go index 271b553..5d2312e 100644 --- a/cmd/diff/client/crossplane/composition_client_test.go +++ b/cmd/diff/client/crossplane/composition_client_test.go @@ -6,11 +6,13 @@ 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" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + k8stypes "k8s.io/apimachinery/pkg/types" "github.com/crossplane/crossplane-runtime/v2/pkg/errors" @@ -1804,3 +1806,398 @@ func TestDefaultCompositionClient_getClaimTypeFromXRD(t *testing.T) { }) } } + +func TestDefaultCompositionClient_FindComposites_WithRefs(t *testing.T) { + // Composition targeting (example.org/v1, XBucket). FindComposites takes the unstructured form. + comp := tu.NewComposition("test-comp"). + WithCompositeTypeRef("example.org/v1", "XBucket"). + BuildAsUnstructured() + + // 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() + + tests := map[string]struct { + reason string + mockResource *tu.MockResourceClient + mockDef *tu.MockDefinitionClient + comp *un.Unstructured + refs []k8stypes.NamespacedName + wantMatched []string // names of matched composites + wantErr bool + }{ + "XRGVKHit_ClusterScoped": { + reason: "Cluster-scoped XR with matching composition is matched via XR-GVK lookup", + mockResource: tu.NewMockResourceClient().WithResourcesExist(xrCluster).Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []k8stypes.NamespacedName{{Name: "xr-cluster"}}, + wantMatched: []string{"xr-cluster"}, + }, + "ClaimGVKHit_NamespacedClaim": { + reason: "Claim found via claim-GVK fallback when XR-GVK 404s", + mockResource: tu.NewMockResourceClient().WithResourcesExist(claim).Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []k8stypes.NamespacedName{{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: []k8stypes.NamespacedName{{Namespace: "claim-ns", Name: "ghost"}}, + }, + "FoundButUsesDifferentComposition_Unmatched": { + reason: "Resource exists but its compositionRef points to a different composition", + mockResource: tu.NewMockResourceClient().WithResourcesExist(xrWrongComp).Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []k8stypes.NamespacedName{{Name: "xr-wrong"}}, + }, + "TransportErrorPropagated": { + reason: "Non-NotFound errors from GetResource propagate up", + mockResource: tu.NewMockResourceClient().WithGetResourceError(errors.New("connection refused")).Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + comp: comp, + refs: []k8stypes.NamespacedName{{Name: "x"}}, + wantErr: true, + }, + "NoClaimType_OnlyXRLookupAttempted": { + reason: "When XRD has no claimNames, claim-GVK lookup is skipped without crashing", + mockResource: tu.NewMockResourceClient().WithResourcesExist(xrCluster).Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrdNoClaim).Build(), + comp: comp, + refs: []k8stypes.NamespacedName{{Name: "xr-cluster"}, {Namespace: "x", Name: "ghost"}}, + wantMatched: []string{"xr-cluster"}, + }, + "XRDNotFound_OnlyXRLookupAttempted": { + reason: "When XRD itself is missing, claim-GVK lookup is skipped", + mockResource: tu.NewMockResourceClient().WithResourcesExist(xrCluster).Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXRNotFound().Build(), + comp: comp, + refs: []k8stypes.NamespacedName{{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, err := c.FindComposites(t.Context(), tt.comp, dtypes.FindCompositesOptions{Refs: tt.refs}) + + if tt.wantErr { + if err == nil { + t.Fatalf("\n%s: expected error, got matched=%v", tt.reason, matched) + } + + 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) + } + }) + } +} + +func TestDefaultCompositionClient_FindComposites_DefaultDiscovery(t *testing.T) { + // FindComposites takes the unstructured composition; the typed copy is just for + // seeding the client's GetComposition cache (which findByListing reads through). + compBuilder := tu.NewComposition("test-comp").WithCompositeTypeRef("example.org/v1", "XBucket") + comp := compBuilder.BuildAsUnstructured() + compTyped := compBuilder.Build() + + xrd := tu.NewXRD("xbuckets.example.org", "example.org", "XBucket"). + WithPlural("xbuckets"). + WithClaimNames("Bucket", "buckets"). + WithVersion("v1", true, true). + BuildAsUnstructured() + + xrCluster := tu.NewResource("example.org/v1", "XBucket", "xr-cluster"). + WithSpecField("compositionRef", map[string]any{"name": "test-comp"}). + Build() + + xrGVK := schema.GroupVersionKind{Group: "example.org", Version: "v1", Kind: "XBucket"} + + c := &DefaultCompositionClient{ + resourceClient: tu.NewMockResourceClient(). + WithListResources(func(_ context.Context, gvk schema.GroupVersionKind, _ string) ([]*un.Unstructured, error) { + if gvk == xrGVK { + return []*un.Unstructured{xrCluster}, nil + } + // Claim listing returns empty (XRD has Bucket claim type but none exist). + return []*un.Unstructured{}, nil + }). + Build(), + definitionClient: tu.NewMockDefinitionClient().WithXRDForXR(xrd).Build(), + compositions: map[string]*apiextensionsv1.Composition{"test-comp": compTyped}, + logger: tu.TestLogger(t, false), + } + + got, err := c.FindComposites(t.Context(), comp, dtypes.FindCompositesOptions{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(got) != 1 || got[0].GetName() != "xr-cluster" { + t.Errorf("DefaultDiscovery: expected [xr-cluster], got %v", got) + } +} + +func TestDefaultCompositionClient_resolveCompositeTypes(t *testing.T) { + xrdWithClaim := tu.NewXRD("xbuckets.example.org", "example.org", "XBucket"). + WithPlural("xbuckets"). + WithClaimNames("Bucket", "buckets"). + WithVersion("v1", true, true). + BuildAsUnstructured() + + xrdNoClaim := tu.NewXRD("xbuckets.example.org", "example.org", "XBucket"). + WithPlural("xbuckets"). + WithVersion("v1", true, true). + BuildAsUnstructured() + + wantXRGVK := schema.GroupVersionKind{Group: "example.org", Version: "v1", Kind: "XBucket"} + wantClaimGVK := schema.GroupVersionKind{Group: "example.org", Version: "v1", Kind: "Bucket"} + + tests := map[string]struct { + reason string + comp *apiextensionsv1.Composition + mockDef *tu.MockDefinitionClient + wantXR schema.GroupVersionKind + wantClaim schema.GroupVersionKind + wantErr bool + wantErrMatch string + }{ + "ValidComp_XRDWithClaim": { + reason: "Composition with valid compositeTypeRef and XRD with claim names → both GVKs populated", + comp: tu.NewComposition("c").WithCompositeTypeRef("example.org/v1", "XBucket").Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrdWithClaim).Build(), + wantXR: wantXRGVK, + wantClaim: wantClaimGVK, + }, + "ValidComp_XRDNoClaim": { + reason: "Composition with valid compositeTypeRef and XRD without claim names → XR GVK populated, claim GVK empty", + comp: tu.NewComposition("c").WithCompositeTypeRef("example.org/v1", "XBucket").Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXR(xrdNoClaim).Build(), + wantXR: wantXRGVK, + wantClaim: schema.GroupVersionKind{}, + }, + "ValidComp_XRDLookupFails": { + reason: "Composition with valid compositeTypeRef but XRD lookup fails → XR GVK populated, claim GVK empty, no error", + comp: tu.NewComposition("c").WithCompositeTypeRef("example.org/v1", "XBucket").Build(), + mockDef: tu.NewMockDefinitionClient().WithXRDForXRNotFound().Build(), + wantXR: wantXRGVK, + wantClaim: schema.GroupVersionKind{}, + }, + "MalformedAPIVersion": { + reason: "Composition with malformed compositeTypeRef.apiVersion → returns error", + comp: tu.NewComposition("c").WithCompositeTypeRef("not/a/valid/apiversion", "XBucket").Build(), + mockDef: tu.NewMockDefinitionClient().Build(), + wantErr: true, + wantErrMatch: "cannot parse compositeTypeRef apiVersion", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + c := &DefaultCompositionClient{ + definitionClient: tt.mockDef, + logger: tu.TestLogger(t, false), + } + + got, err := c.resolveCompositeTypes(t.Context(), tt.comp) + + if tt.wantErr { + if err == nil { + t.Fatalf("\n%s: expected error, got %+v", tt.reason, got) + } + + if tt.wantErrMatch != "" && !strings.Contains(err.Error(), tt.wantErrMatch) { + t.Errorf("\n%s: error %q must contain %q", tt.reason, err.Error(), tt.wantErrMatch) + } + + return + } + + if err != nil { + t.Fatalf("\n%s: unexpected error: %v", tt.reason, err) + } + + if got.xrGVK != tt.wantXR { + t.Errorf("\n%s: xrGVK = %v, want %v", tt.reason, got.xrGVK, tt.wantXR) + } + + if got.claimGVK != tt.wantClaim { + t.Errorf("\n%s: claimGVK = %v, want %v", tt.reason, got.claimGVK, tt.wantClaim) + } + }) + } +} + +func TestDefaultCompositionClient_lookupRef(t *testing.T) { + xrGVK := schema.GroupVersionKind{Group: "example.org", Version: "v1", Kind: "XBucket"} + claimGVK := schema.GroupVersionKind{Group: "example.org", Version: "v1", Kind: "Bucket"} + + xrMatched := tu.NewResource("example.org/v1", "XBucket", "xr"). + WithSpecField("compositionRef", map[string]any{"name": "test-comp"}). + Build() + + xrWrongComp := tu.NewResource("example.org/v1", "XBucket", "xr-wrong"). + WithSpecField("compositionRef", map[string]any{"name": "other-comp"}). + Build() + + claimMatched := tu.NewResource("example.org/v1", "Bucket", "claim"). + InNamespace("ns"). + WithSpecField("compositionRef", map[string]any{"name": "test-comp"}). + Build() + + // Same-name collision: XR `ns/collision` (XBucket, other-comp) and Claim `ns/collision` + // (Bucket, test-comp) both exist. The lookup against test-comp must reach the Claim. + collisionXR := tu.NewResource("example.org/v1", "XBucket", "collision"). + InNamespace("ns"). + WithSpecField("compositionRef", map[string]any{"name": "other-comp"}). + Build() + collisionClaim := tu.NewResource("example.org/v1", "Bucket", "collision"). + InNamespace("ns"). + WithSpecField("compositionRef", map[string]any{"name": "test-comp"}). + Build() + + tests := map[string]struct { + reason string + ref k8stypes.NamespacedName + types compositeTypes + mockResource *tu.MockResourceClient + wantName string // empty = expect nil result + wantErr bool + }{ + "XRMatch": { + reason: "Ref hits XR GVK and references target composition → returns object", + ref: k8stypes.NamespacedName{Name: "xr"}, + types: compositeTypes{xrGVK: xrGVK, claimGVK: claimGVK}, + mockResource: tu.NewMockResourceClient().WithResourcesExist(xrMatched).Build(), + wantName: "xr", + }, + "XRWrongComposition_ClaimAlso404_ReturnsNil": { + reason: "Ref hits XR GVK with wrong composition AND claim 404s → returns nil (XR was wrong-composition; falling through to claim found nothing)", + ref: k8stypes.NamespacedName{Name: "xr-wrong"}, + types: compositeTypes{xrGVK: xrGVK, claimGVK: claimGVK}, + mockResource: tu.NewMockResourceClient().WithResourcesExist(xrWrongComp).Build(), + wantName: "", + }, + "XRWrongComposition_ClaimMatchesTarget_FallsThroughAndReturnsClaim": { + reason: "Same-name XR + Claim collision: XR uses other-comp but Claim uses test-comp → falls through to claim path, returns Claim", + ref: k8stypes.NamespacedName{Namespace: "ns", Name: "collision"}, + types: compositeTypes{xrGVK: xrGVK, claimGVK: claimGVK}, + mockResource: tu.NewMockResourceClient().WithResourcesExist(collisionXR, collisionClaim).Build(), + wantName: "collision", + }, + "XR404_ClaimMatch": { + reason: "Ref XR-404, matches via claim GVK and references target composition → returns object", + ref: k8stypes.NamespacedName{Namespace: "ns", Name: "claim"}, + types: compositeTypes{xrGVK: xrGVK, claimGVK: claimGVK}, + mockResource: tu.NewMockResourceClient().WithResourcesExist(claimMatched).Build(), + wantName: "claim", + }, + "XR404_ClaimGVKEmpty": { + reason: "Ref XR-404, claim GVK is empty (XRD missing) → returns nil, no error", + ref: k8stypes.NamespacedName{Name: "ghost"}, + types: compositeTypes{xrGVK: xrGVK}, + mockResource: tu.NewMockResourceClient().WithResourceNotFound().Build(), + wantName: "", + }, + "XR404_Claim404": { + reason: "Ref XR-404 and claim 404 → returns nil, no error", + ref: k8stypes.NamespacedName{Namespace: "ns", Name: "ghost"}, + types: compositeTypes{xrGVK: xrGVK, claimGVK: claimGVK}, + mockResource: tu.NewMockResourceClient().WithResourceNotFound().Build(), + wantName: "", + }, + "XRTransportError_Propagates": { + reason: "Non-NotFound errors from GetResource on XR-GVK propagate up", + ref: k8stypes.NamespacedName{Name: "x"}, + types: compositeTypes{xrGVK: xrGVK, claimGVK: claimGVK}, + mockResource: tu.NewMockResourceClient().WithGetResourceError(errors.New("connection refused")).Build(), + wantErr: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + c := &DefaultCompositionClient{ + resourceClient: tt.mockResource, + logger: tu.TestLogger(t, false), + } + + got, err := c.lookupRef(t.Context(), tt.ref, tt.types, "test-comp") + + if tt.wantErr { + if err == nil { + t.Fatalf("\n%s: expected error, got %+v", tt.reason, got) + } + + return + } + + if err != nil { + t.Fatalf("\n%s: unexpected error: %v", tt.reason, err) + } + + switch tt.wantName { + case "": + if got != nil { + t.Errorf("\n%s: expected nil, got %s", tt.reason, got.GetName()) + } + default: + if got == nil { + t.Fatalf("\n%s: expected %s, got nil", tt.reason, tt.wantName) + } + + if got.GetName() != tt.wantName { + t.Errorf("\n%s: name = %q, want %q", tt.reason, got.GetName(), tt.wantName) + } + } + }) + } +} diff --git a/cmd/diff/comp.go b/cmd/diff/comp.go index 936925e..2856a3d 100644 --- a/cmd/diff/comp.go +++ b/cmd/diff/comp.go @@ -22,10 +22,12 @@ import ( "github.com/alecthomas/kong" dp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/diffprocessor" - ld "github.com/crossplane/cli/v2/cmd/crossplane/common/load" + "github.com/crossplane-contrib/crossplane-diff/cmd/diff/ref" "github.com/crossplane/crossplane-runtime/v2/pkg/errors" "github.com/crossplane/crossplane-runtime/v2/pkg/logging" + + ld "github.com/crossplane/cli/v2/cmd/crossplane/common/load" ) // CompDiffProcessor is imported from the diffprocessor package @@ -38,8 +40,18 @@ 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. @@ -68,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. ` } @@ -75,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) @@ -147,7 +172,13 @@ 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, err := ref.ParseAll(c.Resources) + if err != nil { + exitCode.Code = dp.ExitCodeToolError + return err + } + + 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 0000000..8d60a5e --- /dev/null +++ b/cmd/diff/comp_test.go @@ -0,0 +1,68 @@ +/* +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" +) + +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 da5434a..b71223f 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. @@ -264,6 +267,21 @@ 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...) @@ -3219,6 +3237,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 01b6abb..097cdd3 100644 --- a/cmd/diff/diffprocessor/comp_processor.go +++ b/cmd/diff/diffprocessor/comp_processor.go @@ -22,12 +22,15 @@ import ( "os" xp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/crossplane" + "github.com/crossplane-contrib/crossplane-diff/cmd/diff/ref" "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" "k8s.io/apimachinery/pkg/runtime" + k8stypes "k8s.io/apimachinery/pkg/types" "github.com/crossplane/crossplane-runtime/v2/pkg/errors" "github.com/crossplane/crossplane-runtime/v2/pkg/logging" @@ -67,7 +70,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 []k8stypes.NamespacedName) (bool, error) Initialize(ctx context.Context) error // Cleanup releases any resources held by the processor (e.g., Docker containers). Cleanup(ctx context.Context) error @@ -152,13 +159,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 []k8stypes.NamespacedName) (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{}, @@ -179,8 +197,39 @@ func (p *DefaultCompDiffProcessor) DiffComposition(ctx context.Context, composit compositionID := comp.GetName() // Use actual name from unstructured p.config.Logger.Debug("Processing composition", "name", compositionID) + // Resolve the affected XR set up-front. In --resource mode the preflight already produced it; + // in default-discovery mode we query the cluster (best-effort: net-new compositions yield empty). + // surfaceFiltered controls whether Manual-policy XRs go into ImpactAnalysis as XRStatusFilteredByPolicy + // entries (true in --resource mode so users see what was matched-but-skipped) vs. only being counted + // in the summary (default-discovery mode). + var ( + affectedXRs []*un.Unstructured + surfaceFiltered bool + ) + + switch { + case len(resources) > 0: + affectedXRs = preflightMatches[compositionID] + surfaceFiltered = true + default: + // Default-discovery only needs comp.GetName() — pass the unstructured directly. + // FindComposites converts to typed internally only in refs mode (which we're not in here). + discovered, findErr := p.compositionClient.FindComposites(ctx, comp, dtypes.FindCompositesOptions{Namespace: namespace}) + + switch { + case findErr != nil: + // Net-new composition (won't exist in cluster) → graceful empty result, same as before. + p.config.Logger.Debug("Cannot find composites using composition (likely net-new composition)", + "composition", compositionID, "error", findErr) + + affectedXRs = nil + default: + affectedXRs = discovered + } + } + // Process this single composition and build the result - compResult, err := p.processSingleComposition(ctx, comp, namespace) + compResult, err := p.processSingleComposition(ctx, comp, affectedXRs, surfaceFiltered) if err != nil { p.config.Logger.Debug("Failed to process composition", "composition", compositionID, "error", err) @@ -242,9 +291,70 @@ 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 []k8stypes.NamespacedName) (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 + } + + // FindComposites takes the unstructured composition; it converts to typed internally + // in refs mode (resolveCompositeTypes needs spec.compositeTypeRef). + matched, err := p.compositionClient.FindComposites(ctx, comp, dtypes.FindCompositesOptions{Refs: 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 []k8stypes.NamespacedName + + 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, ref.Format(r)) + } + + return nil, errors.Errorf("--resource ref(s) not relevant to any supplied composition: %v (resource not found, or it doesn't reference one of the supplied compositions)", names) + } + + return perComp, nil +} + // 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) { +// `affectedXRs` is the pre-resolved set of XRs to evaluate (caller decides via DiffComposition's +// switch whether this comes from the --resource preflight or default-discovery via FindComposites). +// When `surfaceFiltered` is true, XRs dropped by update-policy filtering are surfaced in +// ImpactAnalysis with XRStatusFilteredByPolicy so users see what was matched-but-skipped. +func (p *DefaultCompDiffProcessor) processSingleComposition(ctx context.Context, newComp *un.Unstructured, affectedXRs []*un.Unstructured, surfaceFiltered bool) (*renderer.CompositionDiff, error) { result := &renderer.CompositionDiff{ Name: newComp.GetName(), ImpactAnalysis: []renderer.XRImpact{}, @@ -265,46 +375,57 @@ 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 - } - - p.config.Logger.Debug("Found affected XRs", "composition", newComp.GetName(), "count", len(affectedXRs)) + p.config.Logger.Debug("Processing affected XRs", "composition", newComp.GetName(), "count", len(affectedXRs), "surfaceFiltered", surfaceFiltered) // 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 (surfaceFiltered=true), 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 surfaceFiltered { + 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 affected XRs and collect diffs to determine which ones have changes - p.config.Logger.Debug("Processing XRs to collect diff information", "count", len(affectedXRs)) + // 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)) - xrResults := p.collectXRDiffs(ctx, affectedXRs, newComp) + xrResults := p.collectXRDiffs(ctx, keptXRs, 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 } @@ -337,10 +458,10 @@ func (p *DefaultCompDiffProcessor) collectXRDiffs(ctx context.Context, xrs []*un "kind", cliCompTargetKind) // Build a set of root-level resource keys (apiVersion/kind/namespace/name) for quick lookup. - // Root-level resources are XRs and Claims found by FindCompositesUsingComposition - // that use the CLI composition. These should always use the CLI composition. - // We include namespace to avoid collisions between resources with the same name - // in different namespaces (e.g., two claims with the same name). + // Root-level resources are XRs and Claims supplied as `affectedXRs` to processSingleComposition + // (resolved by DiffComposition via either preflight or FindComposites default-discovery). + // These should always use the CLI composition. Namespace is included in the key to avoid + // collisions between resources with the same name in different namespaces. rootResourceKeys := make(map[string]bool) for _, xr := range xrs { @@ -359,7 +480,7 @@ func (p *DefaultCompDiffProcessor) collectXRDiffs(ctx context.Context, xrs []*un resKind := resGVK.Kind resourceID := fmt.Sprintf("%s/%s", res.GetKind(), res.GetName()) - // Check 1: Is this a root-level resource (XR or Claim found by FindCompositesUsingComposition)? + // Check 1: Is this a root-level resource (XR or Claim supplied as affectedXRs to this composition)? // Root-level resources always use the CLI composition, even claims whose GVK differs from the XR type. key := dt.MakeDiffKeyFromResource(res) if rootResourceKeys[key] { @@ -486,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) @@ -506,13 +622,16 @@ 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 != compositionUpdatePolicyManual { - filtered = append(filtered, xr) + switch { + case policy == compositionUpdatePolicyManual: + dropped = append(dropped, xr) + default: + // Automatic or empty/default policy — keep. + 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 5eb85c0..9e1987c 100644 --- a/cmd/diff/diffprocessor/comp_processor_test.go +++ b/cmd/diff/diffprocessor/comp_processor_test.go @@ -24,12 +24,14 @@ import ( "testing" 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" tu "github.com/crossplane-contrib/crossplane-diff/cmd/diff/testutils" "github.com/crossplane-contrib/crossplane-diff/cmd/diff/types" "github.com/crossplane/cli/v2/cmd/crossplane/render" gcmp "github.com/google/go-cmp/cmp" un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + k8stypes "k8s.io/apimachinery/pkg/types" "github.com/crossplane/crossplane-runtime/v2/pkg/logging" @@ -119,7 +121,9 @@ func TestDefaultCompDiffProcessor_findResourcesUsingComposition(t *testing.T) { }, } - got, err := processor.compositionClient.FindCompositesUsingComposition(ctx, tt.compositionName, tt.namespace) + comp := &un.Unstructured{} + comp.SetName(tt.compositionName) + got, err := processor.compositionClient.FindComposites(ctx, comp, types.FindCompositesOptions{Namespace: tt.namespace}) if (err != nil) != tt.wantErr { t.Errorf("findResourcesUsingComposition() error = %v, wantErr %v", err, tt.wantErr) @@ -289,7 +293,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 +428,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 +709,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 +730,181 @@ 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 RenderInputs) (render.CompositionOutputs, error) { + return render.CompositionOutputs{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) { + 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() + + // DispatchesToCorrectFindMode: DiffComposition routes to ref-lookup vs default-discovery + // based on whether `resources` is non-empty. Verification is implicit via mode-specific + // helpers — WithResourcesForComposition errors on refs-mode, WithCompositesByRef errors on + // default-discovery, so wrong-mode dispatch surfaces as a non-nil DiffComposition error. + t.Run("DispatchesToCorrectFindMode", func(t *testing.T) { + dispatchTests := map[string]struct { + resources []k8stypes.NamespacedName + namespace string + compositionClient xp.CompositionClient + }{ + "EmptyResources_DefaultDiscovery": { + resources: nil, + namespace: "ns", + compositionClient: tu.NewMockCompositionClient(). + WithSuccessfulCompositionFetch(&apiextensionsv1.Composition{}). + WithResourcesForComposition("test-comp", "ns", []*un.Unstructured{xr1}). + Build(), + }, + "WithResources_RefLookup": { + resources: []k8stypes.NamespacedName{{Namespace: "ns", Name: "xr-1"}, {Namespace: "ns", Name: "xr-2"}}, + namespace: "", + compositionClient: tu.NewMockCompositionClient(). + WithSuccessfulCompositionFetch(&apiextensionsv1.Composition{}). + WithCompositesByRef(xr1, xr2). + Build(), + }, + } + + for name, tt := range dispatchTests { + t.Run(name, func(t *testing.T) { + proc, _ := newCompProcessorForTest(t, tt.compositionClient, false) + if _, err := proc.DiffComposition(t.Context(), []*un.Unstructured{comp}, tt.namespace, tt.resources); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } + }) + + t.Run("ResourceMode_GloballyUnmatched_FailsFastNoRender", func(t *testing.T) { + client := tu.NewMockCompositionClient(). + WithSuccessfulCompositionFetch(&apiextensionsv1.Composition{}). + WithNoMatchingComposites(). + Build() + + proc, stdout := newCompProcessorForTest(t, client, false) + + _, err := proc.DiffComposition(t.Context(), []*un.Unstructured{comp}, "", + []k8stypes.NamespacedName{{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()) + } + }) + + // SurfaceFilteredControlsImpactAnalysis: when surfaceFiltered=true (resource mode), + // Manual-policy XRs are surfaced as XRStatusFilteredByPolicy impacts; when false + // (default-discovery), they're counted in the summary but absent from ImpactAnalysis. + t.Run("SurfaceFilteredControlsImpactAnalysis", func(t *testing.T) { + surfaceTests := map[string]struct { + surfaceFiltered bool + wantImpactCount int + wantStatus renderer.XRStatus // empty when wantImpactCount == 0 + }{ + "ResourceMode_SurfacesFilteredXRs": { + surfaceFiltered: true, + wantImpactCount: 1, + wantStatus: renderer.XRStatusFilteredByPolicy, + }, + "DefaultDiscovery_OmitsFilteredFromImpactAnalysis": { + surfaceFiltered: false, + wantImpactCount: 0, + }, + } + + for name, tt := range surfaceTests { + t.Run(name, func(t *testing.T) { + client := tu.NewMockCompositionClient(). + WithSuccessfulCompositionFetch(&apiextensionsv1.Composition{}). + Build() + + proc, _ := newCompProcessorForTest(t, client, false /* IncludeManual */) + + got, err := proc.processSingleComposition(t.Context(), comp, []*un.Unstructured{manualXR}, tt.surfaceFiltered) + if err != nil { + t.Fatalf("processSingleComposition: %v", err) + } + + if len(got.ImpactAnalysis) != tt.wantImpactCount { + t.Fatalf("ImpactAnalysis: got %d entries, want %d (%+v)", len(got.ImpactAnalysis), tt.wantImpactCount, got.ImpactAnalysis) + } + + if tt.wantImpactCount > 0 && got.ImpactAnalysis[0].Status != tt.wantStatus { + t.Errorf("ImpactAnalysis[0].Status: got %q, want %q", got.ImpactAnalysis[0].Status, tt.wantStatus) + } + + if got.AffectedResources.FilteredByPolicy != 1 { + t.Errorf("FilteredByPolicy: got %d, want 1", got.AffectedResources.FilteredByPolicy) + } + + if got.AffectedResources.Total != 1 { + t.Errorf("Total: got %d, want 1", got.AffectedResources.Total) + } + }) + } + }) +} diff --git a/cmd/diff/ref/ref.go b/cmd/diff/ref/ref.go new file mode 100644 index 0000000..ee13389 --- /dev/null +++ b/cmd/diff/ref/ref.go @@ -0,0 +1,104 @@ +/* +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 ref handles parsing and formatting of [namespace/]name composite +// references supplied via the `--resource` CLI flag. Parse and Format are +// inverses: Parse turns a user-typed string into a NamespacedName, Format +// turns a NamespacedName back into the user's original spelling +// (bare "name" for cluster-scoped, "namespace/name" for namespaced). +package ref + +import ( + "strings" + + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + k8stypes "k8s.io/apimachinery/pkg/types" +) + +// ParseAll parses each value via Parse and returns the resulting slice. +// Returns (nil, error) on the first parse failure — no partial results. +// Returns (nil, nil) for an empty input. +func ParseAll(values []string) ([]k8stypes.NamespacedName, error) { + if len(values) == 0 { + return nil, nil + } + + out := make([]k8stypes.NamespacedName, 0, len(values)) + + for _, v := range values { + n, err := Parse(v) + if err != nil { + return nil, err + } + + out = append(out, n) + } + + return out, nil +} + +// Parse parses a "[namespace/]name" CLI arg into a NamespacedName. +// 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 Parse(value string) (k8stypes.NamespacedName, error) { + trimmed := strings.TrimSpace(value) + if trimmed == "" { + return k8stypes.NamespacedName{}, errors.Errorf("invalid --resource value %q: cannot be empty", value) + } + + parts := strings.Split(trimmed, "/") + // Trim each part so inputs like "default / my-claim" — which would otherwise + // parse as namespace "default " and fail downstream with a confusing error — + // are normalized at the point of CLI parsing. Kubernetes names/namespaces + // can't contain whitespace, so per-part trimming never loses information. + for i := range parts { + parts[i] = strings.TrimSpace(parts[i]) + } + + switch len(parts) { + case 1: + return k8stypes.NamespacedName{Name: parts[0]}, nil + case 2: + ns, name := parts[0], parts[1] + if ns == "" { + return k8stypes.NamespacedName{}, errors.Errorf("invalid --resource value %q: namespace must not be empty (use bare name for cluster-scoped composites)", value) + } + + if name == "" { + return k8stypes.NamespacedName{}, errors.Errorf("invalid --resource value %q: name must not be empty", value) + } + + return k8stypes.NamespacedName{Namespace: ns, Name: name}, nil + default: + return k8stypes.NamespacedName{}, errors.Errorf("invalid --resource value %q: expected [namespace/]name format, got %d slashes", value, len(parts)-1) + } +} + +// Format renders a NamespacedName the way the user typed it on the command line: +// bare "name" for cluster-scoped, "namespace/name" for namespaced. +// +// NamespacedName.String() always renders "namespace/name" (so "/foo" for +// cluster-scoped), which is wrong for human-facing output where users expect +// their original spelling. +func Format(n k8stypes.NamespacedName) string { + switch { + case n.Namespace == "": + return n.Name + default: + return n.Namespace + "/" + n.Name + } +} diff --git a/cmd/diff/ref/ref_test.go b/cmd/diff/ref/ref_test.go new file mode 100644 index 0000000..2607d31 --- /dev/null +++ b/cmd/diff/ref/ref_test.go @@ -0,0 +1,199 @@ +/* +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 ref + +import ( + "strings" + "testing" + + k8stypes "k8s.io/apimachinery/pkg/types" +) + +func TestParse(t *testing.T) { + tests := map[string]struct { + input string + want k8stypes.NamespacedName + wantErr bool + }{ + "BareName_ClusterScoped": { + input: "my-xr", + want: k8stypes.NamespacedName{Namespace: "", Name: "my-xr"}, + }, + "NamespaceAndName": { + input: "default/my-claim", + want: k8stypes.NamespacedName{Namespace: "default", Name: "my-claim"}, + }, + "WhitespaceTrimmed": { + input: " default/my-claim ", + want: k8stypes.NamespacedName{Namespace: "default", Name: "my-claim"}, + }, + "WhitespaceAroundSlash_TrimmedPerPart": { + input: "default / my-claim", + want: k8stypes.NamespacedName{Namespace: "default", Name: "my-claim"}, + }, + "WhitespaceAroundClusterScopedName_Trimmed": { + input: " my-xr ", + want: k8stypes.NamespacedName{Name: "my-xr"}, + }, + "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 := Parse(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("Parse(%q) = %+v, want %+v", tt.input, got, tt.want) + } + }) + } +} + +func TestParseAll(t *testing.T) { + tests := map[string]struct { + input []string + want []k8stypes.NamespacedName + wantErr bool + wantErrSubstr string + }{ + "Empty_NilInput": { + input: nil, + want: nil, + }, + "Empty_EmptySlice": { + input: []string{}, + want: nil, + }, + "AllValid": { + input: []string{"foo", "ns/bar", "default/baz"}, + want: []k8stypes.NamespacedName{ + {Name: "foo"}, + {Namespace: "ns", Name: "bar"}, + {Namespace: "default", Name: "baz"}, + }, + }, + "FirstErrorStopsParsing": { + input: []string{"good", "/bad", "would-be-good"}, + wantErr: true, + wantErrSubstr: "/bad", + }, + "InvalidEntry": { + input: []string{"a/b/c"}, + wantErr: true, + wantErrSubstr: "a/b/c", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + got, err := ParseAll(tt.input) + + if tt.wantErr { + if err == nil { + t.Fatalf("expected error, got %+v", got) + } + + if tt.wantErrSubstr != "" && !strings.Contains(err.Error(), tt.wantErrSubstr) { + t.Errorf("error %q should contain %q", err.Error(), tt.wantErrSubstr) + } + + if got != nil { + t.Errorf("expected nil result on error, got %+v", got) + } + + return + } + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(got) != len(tt.want) { + t.Fatalf("got %d refs, want %d (%+v vs %+v)", len(got), len(tt.want), got, tt.want) + } + + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("ParseAll[%d] = %+v, want %+v", i, got[i], tt.want[i]) + } + } + }) + } +} + +func TestFormat(t *testing.T) { + tests := map[string]struct { + ref k8stypes.NamespacedName + want string + }{ + "ClusterScoped_BareName": { + ref: k8stypes.NamespacedName{Name: "my-xr"}, + want: "my-xr", + }, + "Namespaced": { + ref: k8stypes.NamespacedName{Namespace: "default", Name: "my-claim"}, + want: "default/my-claim", + }, + "EmptyEverything": { + ref: k8stypes.NamespacedName{}, + want: "", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + if got := Format(tt.ref); got != tt.want { + t.Errorf("Format(%+v) = %q, want %q", tt.ref, got, tt.want) + } + }) + } +} diff --git a/cmd/diff/renderer/comp_diff_renderer.go b/cmd/diff/renderer/comp_diff_renderer.go index 94e0473..063ca47 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 f7c5d8a..3fbb856 100644 --- a/cmd/diff/renderer/comp_diff_renderer_test.go +++ b/cmd/diff/renderer/comp_diff_renderer_test.go @@ -414,16 +414,22 @@ func Test_formatXRStatusSummary(t *testing.T) { } func Test_pluralize(t *testing.T) { - if pluralize(1) != "" { - t.Error("pluralize(1) should return empty string") - } - - if pluralize(0) != "s" { - t.Error("pluralize(0) should return 's'") + tests := map[string]struct { + count int + want string + }{ + "Singular_NoSuffix": {count: 1, want: ""}, + "Zero_HasSuffix": {count: 0, want: "s"}, + "Plural_HasSuffix": {count: 2, want: "s"}, + "Negative_HasSuffix": {count: -1, want: "s"}, } - if pluralize(2) != "s" { - t.Error("pluralize(2) should return 's'") + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + if got := pluralize(tt.count); got != tt.want { + t.Errorf("pluralize(%d) = %q, want %q", tt.count, got, tt.want) + } + }) } } @@ -536,3 +542,87 @@ 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 83d8c6a..d7e81ce 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 cd1fae5..e034320 100644 --- a/cmd/diff/testutils/mock_builder.go +++ b/cmd/diff/testutils/mock_builder.go @@ -127,6 +127,15 @@ func (b *MockResourceClientBuilder) WithResourceNotFound() *MockResourceClientBu }) } +// WithGetResourceError sets GetResource to always return the given error +// (for non-NotFound transport-failure scenarios; use WithResourceNotFound +// for clean 404 cases). +func (b *MockResourceClientBuilder) WithGetResourceError(err error) *MockResourceClientBuilder { + return b.WithGetResource(func(context.Context, schema.GroupVersionKind, string, string) (*un.Unstructured, error) { + return nil, err + }) +} + // WithListResources sets the ListResources behavior. func (b *MockResourceClientBuilder) WithListResources(fn func(context.Context, schema.GroupVersionKind, string) ([]*un.Unstructured, error)) *MockResourceClientBuilder { b.mock.ListResourcesFn = fn @@ -723,30 +732,66 @@ func (b *MockCompositionClientBuilder) WithSuccessfulCompositionFetches(comps [] }) } -// WithFindCompositesUsingComposition sets the FindCompositesUsingComposition behavior. -func (b *MockCompositionClientBuilder) WithFindCompositesUsingComposition(fn func(context.Context, string, string) ([]*un.Unstructured, error)) *MockCompositionClientBuilder { - b.mock.FindCompositesUsingCompositionFn = fn +// WithFindComposites sets the FindComposites behavior. +func (b *MockCompositionClientBuilder) WithFindComposites(fn func(context.Context, *un.Unstructured, dtypes.FindCompositesOptions) ([]*un.Unstructured, error)) *MockCompositionClientBuilder { + b.mock.FindCompositesFn = fn return b } -// WithResourcesForComposition sets FindCompositesUsingComposition to return specific resources for a given composition name and namespace. +// WithResourcesForComposition sets FindComposites (default-discovery mode) to return specific resources +// for a given composition name and namespace. Refs-mode calls return an explicit error identifying this +// helper as default-discovery only — use WithFindComposites directly if you need to mock both modes. func (b *MockCompositionClientBuilder) WithResourcesForComposition(compositionName, namespace string, resources []*un.Unstructured) *MockCompositionClientBuilder { - return b.WithFindCompositesUsingComposition(func(_ context.Context, compName, ns string) ([]*un.Unstructured, error) { - if compName == compositionName && ns == namespace { + return b.WithFindComposites(func(_ context.Context, comp *un.Unstructured, opts dtypes.FindCompositesOptions) ([]*un.Unstructured, error) { + if len(opts.Refs) > 0 { + return nil, errors.New("WithResourcesForComposition only handles default-discovery (empty Refs)") + } + + if comp.GetName() == compositionName && opts.Namespace == namespace { return resources, nil } - return nil, errors.Errorf("no resources found for composition %s in namespace %s", compName, ns) + return nil, errors.Errorf("no resources found for composition %s in namespace %s", comp.GetName(), opts.Namespace) }) } -// WithFindResourcesError sets FindCompositesUsingComposition to return an error. +// WithFindResourcesError sets FindComposites (default-discovery mode) to return an error. Refs-mode calls +// return an explicit error identifying this helper as default-discovery only — use WithFindComposites +// directly if you need to mock both modes. func (b *MockCompositionClientBuilder) WithFindResourcesError(errMsg string) *MockCompositionClientBuilder { - return b.WithFindCompositesUsingComposition(func(context.Context, string, string) ([]*un.Unstructured, error) { + return b.WithFindComposites(func(_ context.Context, _ *un.Unstructured, opts dtypes.FindCompositesOptions) ([]*un.Unstructured, error) { + if len(opts.Refs) > 0 { + return nil, errors.New("WithFindResourcesError only handles default-discovery (empty Refs)") + } + return nil, errors.New(errMsg) }) } +// WithCompositesByRef sets FindComposites (refs mode) to return the given resources for any +// non-empty-Refs request. Default-discovery calls return an explicit error identifying this helper +// as refs-only — use WithFindComposites directly if you need to mock both modes. Mirror of +// WithResourcesForComposition for the refs-mode side. +func (b *MockCompositionClientBuilder) WithCompositesByRef(matched ...*un.Unstructured) *MockCompositionClientBuilder { + return b.WithFindComposites(func(_ context.Context, _ *un.Unstructured, opts dtypes.FindCompositesOptions) ([]*un.Unstructured, error) { + if len(opts.Refs) == 0 { + return nil, errors.New("WithCompositesByRef only handles refs-mode (non-empty Refs)") + } + + return matched, nil + }) +} + +// WithNoMatchingComposites sets FindComposites to return an empty matched set regardless of mode +// (default-discovery or refs). Use for "nothing matches anywhere" tests where neither default nor +// refs lookup should produce results. Mirrors WithNoMatchingComposition for the FindMatchingComposition +// method. +func (b *MockCompositionClientBuilder) WithNoMatchingComposites() *MockCompositionClientBuilder { + return b.WithFindComposites(func(context.Context, *un.Unstructured, dtypes.FindCompositesOptions) ([]*un.Unstructured, error) { + return nil, nil + }) +} + // 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 5f3eb75..998e083 100644 --- a/cmd/diff/testutils/mocks.go +++ b/cmd/diff/testutils/mocks.go @@ -555,11 +555,11 @@ func (m *MockTypeConverter) GetResourceNameForGVK(ctx context.Context, gvk schem // MockCompositionClient implements the crossplane.CompositionClient interface. type MockCompositionClient struct { - InitializeFn func(ctx context.Context) error - FindMatchingCompositionFn func(ctx context.Context, res *un.Unstructured) (*xpextv1.Composition, error) - 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) + InitializeFn func(ctx context.Context) error + FindMatchingCompositionFn func(ctx context.Context, res *un.Unstructured) (*xpextv1.Composition, error) + ListCompositionsFn func(ctx context.Context) ([]*xpextv1.Composition, error) + GetCompositionFn func(ctx context.Context, name string) (*xpextv1.Composition, error) + FindCompositesFn func(ctx context.Context, comp *un.Unstructured, opts types.FindCompositesOptions) ([]*un.Unstructured, error) } // Initialize implements crossplane.CompositionClient. @@ -598,13 +598,13 @@ func (m *MockCompositionClient) GetComposition(ctx context.Context, name string) return nil, errors.New("GetComposition not implemented") } -// FindCompositesUsingComposition implements crossplane.CompositionClient. -func (m *MockCompositionClient) FindCompositesUsingComposition(ctx context.Context, compositionName string, namespace string) ([]*un.Unstructured, error) { - if m.FindCompositesUsingCompositionFn != nil { - return m.FindCompositesUsingCompositionFn(ctx, compositionName, namespace) +// FindComposites implements crossplane.CompositionClient. +func (m *MockCompositionClient) FindComposites(ctx context.Context, comp *un.Unstructured, opts types.FindCompositesOptions) ([]*un.Unstructured, error) { + if m.FindCompositesFn != nil { + return m.FindCompositesFn(ctx, comp, opts) } - return nil, errors.New("FindCompositesUsingComposition not implemented") + return nil, errors.New("FindComposites not implemented") } // MockFunctionClient implements the crossplane.FunctionClient interface. diff --git a/cmd/diff/types/types.go b/cmd/diff/types/types.go index e697723..5ef9b51 100644 --- a/cmd/diff/types/types.go +++ b/cmd/diff/types/types.go @@ -21,9 +21,24 @@ import ( "context" un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + k8stypes "k8s.io/apimachinery/pkg/types" apiextensionsv1 "github.com/crossplane/crossplane/apis/v2/apiextensions/v1" ) // CompositionProvider is a function that provides a composition for a given resource. type CompositionProvider func(ctx context.Context, res *un.Unstructured) (*apiextensionsv1.Composition, error) + +// FindCompositesOptions narrows what CompositionClient.FindComposites returns. +// Lives here (not in the crossplane client package) so test mocks in cmd/diff/testutils +// can implement the interface without creating an import cycle with cmd/diff/client/crossplane. +type FindCompositesOptions struct { + // Namespace scopes default discovery to a single namespace. Empty = all namespaces. + // Ignored when Refs is non-empty (refs carry their own namespace). + Namespace string + // Refs limits the result to specific user-named composites. When non-empty, a ref is included + // in the result only if (a) the named resource exists at the ref's [namespace/]name and (b) it + // references the supplied composition. Refs that don't satisfy both are silently omitted; the + // caller derives "unmatched" from the diff between input refs and returned objects. + Refs []k8stypes.NamespacedName +}