feat(render): switch to crossplane internal render via crossplane/cli by jcogilvie · Pull Request #326 · crossplane-contrib/crossplane-diff

jcogilvie · 2026-06-05T17:38:57Z

Description of your changes

Replace the in-process render.Render() call (Crossplane v2.2-era) with crossplane internal render driven through github.com/crossplane/cli/v2's Engine interface. The binary runs the real composite reconciler, so crossplane-diff now produces output that matches what users would see if they reconciled the same XR in a live cluster.

Dependency bump

All four crossplane modules pinned at v2.3.2:

crossplane/cli/v2 v2.3.2 (includes the merged-and-released exit-code-3 + stderr-capture work from feat(render): capture stderr and handle pipeline-fatal exit code in both engines crossplane/cli#91 backported via e2es spend too long in setup/teardown #95)
crossplane/crossplane-runtime/v2 v2.3.2
crossplane/crossplane/apis/v2 v2.3.2
crossplane/crossplane/v2 v2.3.2

function-sdk-go was promoted to a direct dep (replaces crossplane/v2/proto/fn/v1); crank/* imports moved to crossplane/cli/v2/cmd/crossplane/*.

Render contract adaptations

Pass the input XR's XRD through CompositionInputs.XRD so the render binary can select the right composite.Schema (Legacy vs Modern). New DefinitionClient.GetCompositeSchema reads spec.scope instead of apiVersion, since v1 XRDs round-tripped through the apiserver come back as v2-form objects with scope=LegacyCluster preserved.
Honour the v2.4 partial-output-on-fatal contract: when a pipeline step FATALs, crossplane internal render exits with code 3 and a populated stdout. EngineRenderFn recognises the (rsp != nil, err != nil) shape upstream's wrapped engines now return on that path, and RenderToStableState keeps iterating on the resolved selectors until the pipeline stabilises.
New EngineRenderFn owns the render-engine lifecycle (Setup, function runtimes, Render, Cleanup), replacing cmd/diff/serial. Default rendering image flips from :main (frozen at xpkg.crossplane.io since the nix migration) to cli's :stable, which tracks current releases.
Multi-composition support: handle the case where one xr invocation processes XRs from different compositions with overlapping-but-not-identical function pipelines (e.g. diffing a GitOps directory). On the first render, capture the engine's auto-stamped network name from upstream's Setup; on subsequent renders, manually apply that annotation to any newly-discovered functions before starting their runtimes. Self-contained workaround tracked for unwind in render: unwind multi-composition workaround in EngineRenderFn once upstream Engine API supports it #338 once a clean upstream API lands (crossplane/cli#96).

Schema/fixture updates driven by the reconciler-faithful output

Add status.conditions[].observedGeneration to ~22 test CRDs (the reconciler now writes it on every condition).
Add spec.crossplane.* subtree to nested-XR test fixture CRDs that previously declared only user fields.
Strip spec.crossplane from dry-run apply input only when needed (composed resources whose CRD doesn't declare the subtree); keep it for root XRs so revision upgrades surface in the diff.
Synthesize a backing XR for existing claims that lack spec.resourceRef so the reconciler's GVK compatibility check accepts the input.
Fix NewXRWithGenerateName to emit a placeholder name that passes RFC 1123 validation; display layer continues to render (generated).

Skipped tests (with TODOs)

TestGetRestConfig/EmptyKubeconfigEnvVar: pre-existing skip, now annotated with how to make it run on developer machines too.
TestCompDiffIntegration/CrossNamespaceResourceCollision: blocked on Reference-typed extra resources fail for namespaced kinds when ref.namespace is omitted function-extra-resources#106 — the function emits Selector{Namespace:""} for Reference-typed extras that omit ref.namespace, and the v2.3+ binary fetcher takes it verbatim. Re-enable once a function release defaults to the XR's namespace.

Upstream work driven during this round (all merged or filed)

fix(render): allow xrd schema to be supplied for internal render crossplane/crossplane#7452 — render: honor input XR's schema based on supplied XRDs. Merged, shipped in v2.3.2.
feat(render): capture stderr and handle pipeline-fatal exit code in both engines crossplane/cli#91 — render: capture stderr and handle pipeline-fatal exit code in both engines. Merged, backported via e2es spend too long in setup/teardown #95, shipped in cli v2.3.2.
render: Engine.Setup not idempotent — no clean way to add functions to an existing render network crossplane/cli#96 — render: Engine.Setup not idempotent. Filed to track the cleaner upstream API for multi-composition; we ship a self-contained workaround in this PR.
render: unwind multi-composition workaround in EngineRenderFn once upstream Engine API supports it #338 — internal tracking issue to unwind the multi-composition workaround once cli#96 lands.
Reference-typed extra resources fail for namespaced kinds when ref.namespace is omitted function-extra-resources#106 — filed.

Working notes live under .requirements/20260504T230404Z_render_engine_migration/ and .requirements/20260609T220505Z_multi_composition_render/.

Test result: full unit + integration suite green. (E2E unchanged; suite gated on cli releases that publish the rendered image.)

Fixes #

I have:

Read and followed Crossplane's contribution process.
Run earthly -P +reviewable to ensure this PR is ready for review.
Added or updated unit tests.
~~Added or updated e2e tests~~ — existing E2E suite is unchanged; the migration is internal. The render-binary version under E2E test is gated on the cli release that pulls in the new field.
Documented this change as needed.
~~Followed the API promotion workflow~~ — no API changes.

🤖 Generated with Claude Code

Copilot

Pull request overview

This PR migrates crossplane-diff from the deprecated in-process render.Render() path to the Crossplane CLI engine that drives crossplane internal render, aiming to make diffs reflect the real composite reconciler behavior seen in a live cluster.

Changes:

Introduces an engine-backed render implementation (EngineRenderFn) with lifecycle management (setup, function runtimes, render, cleanup) and partial-output-on-fatal handling.
Updates requirements iteration to use the new flat RequiredResources []*ResourceSelector contract and adapts processors/tests accordingly.
Bumps Crossplane-related dependencies and updates E2E/integration fixtures (schemas, CRDs, expected outputs) to match reconciler-faithful render output.

Reviewed changes

Copilot reviewed 83 out of 84 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/e2e/xr_basic_test.go	Switches condition helpers to Crossplane `apis/v2` types.
test/e2e/xr_advanced_test.go	Switches condition helpers to Crossplane `apis/v2` types.
test/e2e/main_test.go	Updates pkg API import path to `crossplane/apis/v2`.
test/e2e/comp_test.go	Updates condition helpers and imports to `apis/v2`.
test/e2e/claim_test.go	Updates condition helpers and imports to `apis/v2`.
go.mod	Bumps Crossplane/CLI deps and related transitive modules.
cmd/diff/xr.go	Swaps crank loader import to `crossplane/cli` and removes global render mutex wiring.
cmd/diff/types/types.go	Updates apiextensions import path to `crossplane/apis/v2`.
cmd/diff/testutils/mocks.go	Updates imports and extends mock DefinitionClient / render output types.
cmd/diff/testutils/mock_builder.go	Updates imports and SecretReference type to `core/v2`.
cmd/diff/testdata/diff/crds/xparentresources.ns.nested.example.org.yaml	Fixture schema updates for `spec.crossplane.*` and `observedGeneration`.
cmd/diff/testdata/diff/crds/xparentnopclaims.claimnested.diff.example.org.yaml	Adds `observedGeneration` to conditions schema.
cmd/diff/testdata/diff/crds/xnopresource-v2withv1paths-crd.yaml	Adds `resourceRefs` + `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/xnopresource-ns-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/xnopresource-legacycluster-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/xnopresource-cluster-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/xenvresource-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/xdownstreamresource-ns-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/xdownstreamresource-legacycluster-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/xdownstreamresource-cluster-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/xdownstreamenvresource-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/xdefaultresource-ns-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/xconcurrenttest-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/xchildresources.ns.nested.example.org.yaml	Fixture schema updates for `spec.crossplane.*` and `observedGeneration`.
cmd/diff/testdata/diff/crds/xchildnopclaims.claimnested.diff.example.org.yaml	Adds `observedGeneration` to conditions schema.
cmd/diff/testdata/diff/crds/xapimigrateresource-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/diff/crds/parentnopclaims.claimnested.diff.example.org.yaml	Adds `observedGeneration` to conditions schema.
cmd/diff/testdata/diff/crds/nopclaim-crd.yaml	Adds `observedGeneration` to conditions schema.
cmd/diff/testdata/diff/crds/clusternopresources.nop.crossplane.io.yaml	Adds `observedGeneration` to conditions schema.
cmd/diff/testdata/comp/crds/xparentresource-ns-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/comp/crds/xnopresource-ns-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/comp/crds/xnopresource-legacycluster-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/comp/crds/xnopresource-cluster-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/comp/crds/xenvresource-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/comp/crds/xdownstreamresource-ns-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/comp/crds/xdownstreamresource-legacycluster-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/comp/crds/xdownstreamresource-cluster-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/comp/crds/xdownstreamenvresource-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/testdata/comp/crds/xdefaultresource-ns-crd.yaml	Adds `observedGeneration` to fixture CRD schema.
cmd/diff/serial/serial.go	Removes old global-mutex-based render serialization helper (deleted).
cmd/diff/serial/serial_test.go	Removes tests for deleted serial wrapper (deleted).
cmd/diff/diffprocessor/schema_validator.go	Switches validate imports to CLI and changes XR validation to strip `spec.crossplane` on a deep copy.
cmd/diff/diffprocessor/resource_manager.go	Updates crank imports, fixes ownerRef UID overwrite behavior, and dedupes duplicate ownerRefs.
cmd/diff/diffprocessor/resource_manager_test.go	Updates crank resource import path to CLI.
cmd/diff/diffprocessor/requirements_provider.go	Reworks requirements resolution to flat selectors (`ResolveSelectors`) and updates SDK proto import.
cmd/diff/diffprocessor/requirements_provider_test.go	Updates tests for `ResolveSelectors` and new selector contract.
cmd/diff/diffprocessor/render_engine.go	Adds engine-backed render implementation and stderr-capturing local engine.
cmd/diff/diffprocessor/render_engine_test.go	Adds unit tests for engine lifecycle, cleanup idempotence, and serialization.
cmd/diff/diffprocessor/processor_config.go	Retypes `RenderFunc` to new `RenderFn` and removes render mutex option.
cmd/diff/diffprocessor/function_provider.go	Updates imports to Crossplane `apis/v2`.
cmd/diff/diffprocessor/function_provider_test.go	Updates imports to Crossplane `apis/v2`.
cmd/diff/diffprocessor/diff_processor.go	Uses engine-backed default render fn, adds engine cleanup, updates claim backing XR logic, schema plumbing, and requirements iteration.
cmd/diff/diffprocessor/diff_processor_test.go	Updates render mocks and tests to new render types and requirements shape; adds schema plumbing assertion test.
cmd/diff/diffprocessor/diff_calculator.go	Updates render types, strips ownerReferences from SSA dry-run input to avoid multi-controller SSA failure modes.
cmd/diff/diffprocessor/diff_calculator_test.go	Updates tests to use `render.CompositionOutputs`.
cmd/diff/diffprocessor/comp_processor.go	Updates imports and avoids dead render fn defaulting in comp diff processor.
cmd/diff/diffprocessor/comp_processor_test.go	Updates render mocks to new render types.
cmd/diff/diff_test.go	Updates loader imports to CLI and Crossplane `apis/v2`.
cmd/diff/diff_it_utils_test.go	Adds helper to locate local crossplane binary and set render env var for integration tests.
cmd/diff/diff_integration_test.go	Uses local crossplane binary for internal render and updates expected patterns + skipped test annotations.
cmd/diff/comp.go	Swaps crank loader import to `crossplane/cli` and removes global render mutex wiring.
cmd/diff/cmd_utils.go	Removes now-unused global render mutex and updates loader import to CLI.
cmd/diff/client/kubernetes/schema_client.go	Updates apiextensions imports to Crossplane `apis/v2`.
cmd/diff/client/crossplane/resource_tree_client.go	Swaps crank resource/xrm imports to CLI equivalents.
cmd/diff/client/crossplane/function_client.go	Updates imports to Crossplane `apis/v2`.
cmd/diff/client/crossplane/function_client_test.go	Updates imports to Crossplane `apis/v2`.
cmd/diff/client/crossplane/definition_client.go	Adds `GetCompositeSchema` API and implements schema detection via XRD `spec.scope`.
cmd/diff/client/crossplane/definition_client_test.go	Adds unit coverage for `GetCompositeSchema`.
cmd/diff/client/crossplane/credential_client.go	Updates apiextensions imports to Crossplane `apis/v2`.
cmd/diff/client/crossplane/credential_client_test.go	Updates apiextensions imports to Crossplane `apis/v2`.
cmd/diff/client/crossplane/composition_revision_client.go	Updates apiextensions imports to Crossplane `apis/v2`.
cmd/diff/client/crossplane/composition_revision_client_test.go	Updates apiextensions imports to Crossplane `apis/v2`.
cmd/diff/client/crossplane/composition_client.go	Updates apiextensions imports to Crossplane `apis/v2`.
cmd/diff/client/crossplane/composition_client_test.go	Updates apiextensions imports to Crossplane `apis/v2`.
cmd/diff/client/core/core.go	Swaps crank xrm import to CLI and updates pkg import to `apis/v2`.
.serena/project.yml	Adds `additional_workspace_folders` config entry.
.requirements/20260527T173616Z_scope_aware_composite_render/REQUIREMENTS.md	Adds working requirements doc for scope-aware schema handling.
.requirements/20260504T230404Z_render_engine_migration/REQUIREMENTS.md	Adds working requirements doc for render engine migration.
.requirements/20260504T230404Z_render_engine_migration/upstream-issue-*.md	Adds upstream issue drafts / notes supporting the migration.

Comments suppressed due to low confidence (2)

cmd/diff/diffprocessor/requirements_provider.go:48

The resourceCache comment claims the cache key is apiVersion+kind+name, but getCachedResource uses dt.MakeDiffKey(apiVersion, kind, namespace, name). This comment should include namespace to reflect the actual (and important) behavior.
cmd/diff/diffprocessor/requirements_provider_test.go:159
This test file ends with a dangling comment block describing TestRequirementsProvider_NamespaceCollision, but the actual test was removed. Either reintroduce the test for the updated ResolveSelectors API or remove the stale comment to avoid confusion.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Mechanical fixes from the PR review pass: - diffprocessor/requirements_provider.go: drop the orphaned function-stub comments left over from ProvideRequirements, drop the unused `stepName` arg threaded through processSelector / processNameSelector / processLabelSelector / resolveNamespace (callers now always passed ""), and update the resourceCache doc to mention namespace (which has been part of the cache key since dt.MakeDiffKey took on namespace). - diffprocessor/resource_manager.go: dedupe owner refs via dt.MakeDiffKey instead of an ad-hoc "|"-joined string, matching the rest of the diff layer's keying convention. - diffprocessor/diff_processor.go: switch the err==nil/else block in RenderToStableState to a switch (project style), tighten the generateName comment to make clear "placeholder" is purely a render-side RFC1123-valid filler and the (generated) suffix the user sees is produced independently by diff_formatter.go, and update the schema re-stamp comment to point at crossplane/crossplane#7452 (merged on main; v2.3.1 still defaults the binary to SchemaModern). - diffprocessor/diff_processor_test.go: same #7452 update on the schema- plumbing test's documentation. - diffprocessor/render_engine_test.go: add a header comment justifying why the three lifecycle tests (HappyPath / CleanupIdempotent / Serialization) are procedural rather than table-driven, and switch to t.Context(). - diffprocessor/schema_validator.go: rewrite the spec.crossplane stripping comment so it accurately states that real cluster-derived CRDs declare the subtree (Crossplane's CRD generator emits it) — the gap is in our hand-rolled integration-test CRD fixtures, not in production. - client/crossplane/definition_client.go: bring the GetCompositeSchema doc comments (interface + impl) into sync with the actual implementation, which reads spec.scope rather than the XRD's apiVersion (apiserver conversion can rewrite the apiVersion stamp; spec.scope is preserved). - client/crossplane/definition_client_test.go: drop a duplicated comment line; clarify the "ModernClaim" reference (the claim kind in the test is just the fixture's "ClaimedResource" string — there's no upstream type called ModernClaim). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Copilot

Pull request overview

Copilot reviewed 80 out of 81 changed files in this pull request and generated 4 comments.

…oth engines Both render engines previously dropped information that downstream tools need: - localRenderEngine piped stderr straight to os.Stderr, so callers couldn't programmatically inspect FATAL pipeline messages. - dockerRenderEngine discarded stderr at the engine layer (the RunContainer return value was assigned to `_`); only on container error did stderr surface, folded into a fmt.Errorf string. - Neither engine implemented the partial-output-on-fatal contract from crossplane/crossplane#7455: when `crossplane internal render` exits with code 3 it populates stdout with a partial RenderResponse so callers can recover output.RequiredResources and iterate, but both engines wrapped the *exec.ExitError / *ContainerExitError and dropped stdout. This change: - Adds ExitCodePipelineFatal (= 3) in cmd/crossplane/render/render.go, documenting the contract. - Adds *ContainerExitError to internal/docker (carries ExitCode + captured stderr) so callers can errors.As against it. RunContainer returns it on non-zero exit instead of an opaque fmt.Errorf. - Captures stderr in localRenderEngine.Render via bytes.Buffer instead of os.Stderr, and surfaces it in the returned error. - Uses RunContainer's stderr return in dockerRenderEngine.Render and surfaces it in the returned error. - On exit-3 with non-empty stdout, both engines parse the partial *RenderResponse and return (rsp, err) — both non-nil — so callers can recover the partial output. - Adds engine_local_test.go (helper-process pattern via TestMain re-exec) and engine_docker_test.go (injectable runContainer fn) covering success, fatal-with-partial, fatal-no-partial, and hard-fail paths. Motivating downstream use case: crossplane-contrib/crossplane-diff currently wraps localRenderEngine to add this behaviour. See crossplane-contrib/crossplane-diff#326 — once this lands the wrapper can be deleted and crossplane-diff can drop its local-binary fallback entirely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

The runContainer field on dockerRenderEngine and the env-var-driven TestMain dispatch in engine_local_test.go are both standard Go test patterns, but they're not obvious at a glance — and reviewer questions on the downstream PR (crossplane-contrib/crossplane-diff#326) showed both warrant inline justification. - Expand the doc comment on dockerRenderEngine.runContainer to explain why it exists (hermetic failure-mode coverage without a real Docker daemon) and why a per-instance unexported field is preferable to a package-level var (the latter would race in parallel tests). - Expand the doc comment on engine_local_test.go's envHelperMode + TestMain to explain the helper-process / re-exec idiom: why we re-use the test binary as a stand-in for the `crossplane` binary (no shell scripts, no testdata helper binary, deterministic protobuf payloads), how dispatch works (parent t.Setenv -> child inherits -> TestMain sees the var and exits before m.Run), and what the trade-off is (extra TestMain plumbing in exchange for a self-contained, cross-platform fake binary). No functional changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Copilot

Pull request overview

Copilot reviewed 80 out of 81 changed files in this pull request and generated 5 comments.

…oth engines Both render engines previously dropped information that downstream tools need: - localRenderEngine piped stderr straight to os.Stderr, so callers couldn't programmatically inspect FATAL pipeline messages. - dockerRenderEngine discarded stderr at the engine layer (the RunContainer return value was assigned to `_`); only on container error did stderr surface, folded into a fmt.Errorf string. - Neither engine implemented the partial-output-on-fatal contract from crossplane/crossplane#7455: when `crossplane internal render` exits with code 3 it populates stdout with a partial RenderResponse so callers can recover output.RequiredResources and iterate, but both engines wrapped the *exec.ExitError / *ContainerExitError and dropped stdout. This change: - Adds ExitCodePipelineFatal (= 3) in cmd/crossplane/render/render.go, documenting the contract. - Adds *ContainerExitError to internal/docker (carries ExitCode + captured stderr) so callers can errors.As against it. RunContainer returns it on non-zero exit instead of an opaque fmt.Errorf. - Captures stderr in localRenderEngine.Render via bytes.Buffer instead of os.Stderr, and surfaces it in the returned error. - Uses RunContainer's stderr return in dockerRenderEngine.Render and surfaces it in the returned error. - On exit-3 with non-empty stdout, both engines parse the partial *RenderResponse and return (rsp, err) — both non-nil — so callers can recover the partial output. - Adds engine_local_test.go (helper-process pattern via TestMain re-exec) and engine_docker_test.go (injectable runContainer fn) covering success, fatal-with-partial, fatal-no-partial, and hard-fail paths. Motivating downstream use case: crossplane-contrib/crossplane-diff currently wraps localRenderEngine to add this behaviour. See crossplane-contrib/crossplane-diff#326 — once this lands the wrapper can be deleted and crossplane-diff can drop its local-binary fallback entirely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> (cherry picked from commit 1843d3d)

The runContainer field on dockerRenderEngine and the env-var-driven TestMain dispatch in engine_local_test.go are both standard Go test patterns, but they're not obvious at a glance — and reviewer questions on the downstream PR (crossplane-contrib/crossplane-diff#326) showed both warrant inline justification. - Expand the doc comment on dockerRenderEngine.runContainer to explain why it exists (hermetic failure-mode coverage without a real Docker daemon) and why a per-instance unexported field is preferable to a package-level var (the latter would race in parallel tests). - Expand the doc comment on engine_local_test.go's envHelperMode + TestMain to explain the helper-process / re-exec idiom: why we re-use the test binary as a stand-in for the `crossplane` binary (no shell scripts, no testdata helper binary, deterministic protobuf payloads), how dispatch works (parent t.Setenv -> child inherits -> TestMain sees the var and exits before m.Run), and what the trade-off is (extra TestMain plumbing in exchange for a self-contained, cross-platform fake binary). No functional changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> (cherry picked from commit b196c89)

Copilot

Pull request overview

Copilot reviewed 80 out of 81 changed files in this pull request and generated 3 comments.

Replace the in-process `render.Render()` call (Crossplane v2.2-era) with `crossplane internal render` driven through `github.com/crossplane/cli/v2`'s Engine interface. The binary runs the real composite reconciler, so crossplane-diff now produces output that matches what users would see if they reconciled the same XR in a live cluster. Dependency bump: - crossplane/v2, crossplane-runtime/v2, apis/v2 → v2.3.1 - crossplane/cli/v2 → main @ b88f8a1 (pinned via Go pseudo-version pending the next cli release; carries the `composite_resource_definition` proto field and `CompositionInputs.XRD` Go wire-up that the new render contract requires) - function-sdk-go promoted to a direct dep (replaces crossplane/v2/proto/fn/v1) - crank/* imports moved to crossplane/cli/v2/cmd/crossplane/* Render contract adaptations: - Pass the input XR's XRD through `CompositionInputs.XRD` so the render binary can select the right composite.Schema (Legacy vs Modern). New `DefinitionClient.GetCompositeSchema` reads `spec.scope` instead of apiVersion since v1 XRDs round-tripped through the apiserver come back as v2-form objects with scope=LegacyCluster preserved. - Honour the v2.4 partial-output-on-fatal contract: when a pipeline step FATALs, the binary now exits 3 with stdout populated, surfacing recorded RequiredResources. Our `stderrCapturingLocalEngine` unmarshals the partial output, and `RenderToStableState` keeps iterating on the resolved selectors until the pipeline stabilises. - New `EngineRenderFn` owns the render-engine lifecycle (Setup, function runtimes, Render, Cleanup), replacing `cmd/diff/serial`. Default rendering image flips from `:main` (frozen at xpkg.crossplane.io since the nix migration) to cli's `:stable`, which tracks current releases. Schema/fixture updates driven by the reconciler-faithful output: - Add `status.conditions[].observedGeneration` to ~22 test CRDs (the reconciler now writes it on every condition). - Add `spec.crossplane.*` subtree to nested-XR test fixture CRDs that previously declared only user fields. - Strip `spec.crossplane` from dry-run apply input only when needed (composed resources whose CRD doesn't declare the subtree); keep it for root XRs so revision upgrades surface in the diff. - Synthesize a backing XR for existing claims that lack `spec.resourceRef` so the reconciler's GVK compatibility check accepts the input. - Fix `NewXRWithGenerateName` to emit a placeholder name that passes RFC 1123 validation; display layer continues to render `(generated)`. Skipped tests, with TODOs: - `TestGetRestConfig/EmptyKubeconfigEnvVar`: pre-existing skip, now annotated with how to make it run on developer machines too. - `TestCompDiffIntegration/CrossNamespaceResourceCollision`: blocked on crossplane-contrib/function-extra-resources#106 — the function emits Selector{Namespace:""} for `Reference`-typed extras that omit `ref.namespace`, and the v2.3+ binary fetcher takes it verbatim. Re-enable once a function release defaults to the XR's namespace. Working notes for two upstream issues we drove this round live under .requirements/20260504T230404Z_render_engine_migration/, including the filed cli/proto change (crossplane/crossplane#7452, merged) and the filed function-extra-resources issue. Test result: 609 PASS / 0 FAIL / 2 SKIP. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

The test was dropped in da2f192 when ProvideRequirements (step-keyed map) was rewritten as ResolveSelectors (flat selector slice). The underlying behavior — resolveNamespace defaulting empty selector namespace to xrNamespace, and namespace-aware cache keys — is still in the code, but nothing was asserting it. Pairs with the (currently skipped) E2E TestCompDiffIntegration/CrossNamespaceResourceCollision, which is blocked on function-extra-resources#106. The unit test exercises the same defaulting + cache-keying path without depending on the function, so regressions are caught even while the E2E remains skipped. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

…v-var race Previously, integration tests pointed the render engine at a local crossplane binary by mutating the process-global CROSSPLANE_RENDER_BINARY env var inside requireCrossplaneBinary. Both TestDiffIntegration and TestCompDiffIntegration call t.Parallel(), so whichever finished first would restore/unset the var while the other was still running, racing on engine selection. Replace with explicit path threading: - ProcessorConfig.CrossplaneRenderBinary + WithCrossplaneRenderBinary option. - NewEngineRenderFn now takes the path as a positional arg; empty string means "use the upstream docker engine" (production default). - New hidden CLI flag --crossplane-render-binary plumbs the path through defaultProcessorOptions so each kong invocation gets its own copy. - requireCrossplaneBinary now returns the absolute path; runIntegrationTest threads it into args as --crossplane-render-binary=<path>. No env mutation. The local-binary path itself remains a temporary test affordance — documented in render_engine.go as something we can delete once crossplane/cli's localRenderEngine adopts equivalent stderr-capture and exit-code-3 (PR #7455 partial-output-on-fatal) semantics. Upstream PR to follow. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

google.golang.org/protobuf has no direct usage in our code anymore (the dummy 'var _ = structpb.NewNullValue' was removed in 90fa8b8). 'earthly +generate' moved it to the indirect block accordingly. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Per Copilot review on PR #326: ResolveSelectors fans out through processSelector to either processNameSelector (MatchName -> client.GetResource) or processLabelSelector (MatchLabels -> client.GetResourcesByLabel), but the unit test table only exercised MatchName. Adds two cases covering the label path: - MatchLabels: tier=cache selector returns both labelled ConfigMaps via GetResourcesByLabel; verifies the resources flow through ResolveSelectors unchanged. - MatchLabelsFetchError: error path parallel to the existing FetchError case for MatchName, covering propagation from the label-list call site. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Copilot

Pull request overview

Copilot reviewed 82 out of 83 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

cmd/diff/diffprocessor/requirements_provider.go:60

RequirementsProvider stores a renderFn field (and NewRequirementsProvider takes a renderFn parameter) but nothing ever uses it. With golangci-lint's unused linter enabled, this will be reported as an unused field/identifier and can break CI.

1. requirements_provider.go: drop the dead RequirementsProvider.renderFn field. RequirementsProvider stored a renderFn but never used it after the switch from ProvideRequirements (which needed it for env-config FATAL recovery render passes) to ResolveSelectors (which doesn't). The field, the constructor parameter, the ComponentFactories.RequirementsProvider signature, the WithRequirementsProviderFactory option signature, and the call sites in diff_processor.go + tests all drop the renderFn arg. No behaviour change. 2. diff_processor.go + definition_client.go: fold the duplicate XRD lookup in resolveSchemaAndXRDForRender into a single call. GetCompositeSchema internally calls GetXRDForXR / GetXRDForClaim, then resolveSchemaAndXRDForRender called the same lookups again to forward the XRD to the render binary - two cache hits per render iteration. Extracts the spec.scope -> Schema rule into an exported xp.SchemaFromXRD helper that both GetCompositeSchema (when called directly) and resolveSchemaAndXRDForRender (which already has the XRD) can call. Net: one lookup per iteration. 3. renderer/diff_formatter.go: handle composed-resource names that crossplane's reconciler synthesized from a generateName template. Pre-migration, render.Render() left composed resources with generateName + no name. Post-migration, crossplane internal render's reconciler stamps a deterministic name (e.g. test-claim-b0348ce08462) on resources whose template only had generateName. The diff formatter's existing logic only recognised three name shapes - the legacy '(generated)' suffix, our GenerateNamePlaceholder sentinel, and 'no name + generateName set' - so it kept the generated name in the diff body and the test framework's masking turned it into XXXXX. Adds isGeneratedFromGenerateName(desired) which returns true when generateName is non-empty and name starts with generateName (i.e. the suffix is generated). Display path substitutes <generateName>(generated); strip path drops metadata.name and normalises generateName. Both treat the new shape identically to the existing two. Fixes TestDiffNewClaim/CanDiffNewClaim e2e regression. Updates TestDefaultDiffProcessor_RenderToStableState_SchemaPlumbing to mock GetXRDForXR (matching the new single-lookup path) instead of GetCompositeSchema. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

… UID The binary at internal/render/composite/render.go:94 (v2.3.2) overwrites the input XR's UID with a deterministic SHA1 of gvk+namespace+name. The composite reconciler then drops any observed composed resource whose controller owner ref UID doesn't match xr.UID (composition_functions.go:824). Cluster-fetched observed resources carry the real XR UID, so they were silently filtered out and templates that look them up by composition-resource-name resolved to "<no value>" — surfacing as `cannot apply composed resource ... because it has an invalid name "<no value>"` in TestDiffCompositionWithGetComposedResource. Pre-rewrite controller owner refs in EngineRenderFn.Render to the binary's predictable fake UID. The formula is deterministic and stable; if upstream changes it, the same E2E will fail and we'll update in lockstep. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Copilot

Pull request overview

Copilot reviewed 82 out of 83 changed files in this pull request and generated no new comments.

The fixture pre-dated v2 schema-default propagation. New v2 XRs (scope: Namespaced) now render with spec.crossplane.compositionUpdatePolicy: Automatic — same surface that already shows for managed-resource defaults like deletionPolicy: Delete on NopResource. Regenerated via E2E_DUMP_EXPECTED=1. The Existing variant is unaffected because the cluster state already carries those fields populated by the apiserver. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Same root cause as fc1b43e: v2 XRs (scope: Cluster or Namespaced) now render with spec.crossplane.compositionUpdatePolicy: Automatic from the schema default, surfacing in the new-XR diff. Existing-XR variants are unaffected because cluster state already carries the populated field. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Copilot

Pull request overview

Copilot reviewed 86 out of 87 changed files in this pull request and generated 1 comment.

api/apimachinery had drifted to v0.35.3 via transitive bumps while client-go, apiextensions-apiserver, apiserver, cli-runtime, code-generator, component-base, kms, and kubectl stayed at v0.35.1. Bump all k8s.io/* to v0.35.3 so the dependency set stays in lockstep, matching how these modules are typically released and consumed together. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

jcogilvie · 2026-06-10T15:17:17Z

+		// (shipped in v2.3.2, the version we pin) makes `crossplane internal
+		// render` honour the supplied XRD when picking its internal wrapper
+		// schema, so the rendered output for Legacy XRs should already use v1
+		// field paths. Re-stamping here is now belt-and-braces — kept so any


do we need to do this if upstream is fixed? if not we should remove, as it's functionally dead, right?

jcogilvie · 2026-06-10T15:18:13Z

+// to SchemaModern (matching the composite.Unstructured zero-value default and
+// the binary's own fallback) and the XRD is left nil.
+func (p *DefaultDiffProcessor) resolveSchemaAndXRDForRender(ctx context.Context, xr *cmp.Unstructured, resourceID string) (cmp.Schema, *un.Unstructured) {
+	if p.defClient == nil {


defClient should never be nil. that's a programmer error and the consequences of that error are likely bigger than just this.

jcogilvie · 2026-06-10T15:19:40Z

+// picking its internal wrapper schema (crossplane/crossplane#7452, merged
+// to main; ships in the next release after v2.3.1). Until we pin past that
+// release the re-stamp here is what makes in-process accessors read v1
+// field paths for Legacy XRs.


this comment is out of date as we are now pinning 2.3.2. is this still necessary?

jcogilvie · 2026-06-10T15:20:28Z

+		newFns = append(newFns, in.Functions[i])
+	}
+
+	if !e.started {


else -> switch please

jcogilvie · 2026-06-10T15:23:29Z

+// calls — the case where one `xr` invocation processes XRs that resolve to
+// different compositions with overlapping but non-identical function pipelines.
+//
+// Pre-fix behaviour: Setup ran only on the first render, so Setup's network


we're committing this to main. the test should describe the current behavior at a snapshot in time. reframe this comment to this point in time. no need to reference pre/post "fix" since it's not clear what we were even fixing

jcogilvie · 2026-06-10T15:27:20Z

+// stripSyntheticName removes the synthesized name (and normalizes the
+// synthesized generateName) so the diff body matches what the user supplied.
+// Handles three shapes:
+//   - Legacy "(generated)" suffix on the metadata.name we used to attach.


do we do this anymore? if not, do we need this case?

diff isn't stateful, so what we used to do isn't particularly relevant if we don't currently do it.

jcogilvie · 2026-06-10T15:28:16Z

 	}, nil
 }

+// displayNameFromPlaceholder returns the user-facing display name for a


this whole section seems like a ton of work for display names. please explain.

jcogilvie · 2026-06-10T15:30:14Z

+//
+// Local developers wanting the fast path can build the binary with:
+//
+//	go build -o _output/bin/crossplane github.com/crossplane/crossplane/v2/cmd/crossplane


go build works on a remote source? should we set up earthly to do this for us? (maybe in another PR.)

Yes — go build <module-path> resolves through the local module cache (populated by go mod download / first build), no remote fetch at build time. Since our go.mod already pulls github.com/crossplane/crossplane/v2, the cmd/crossplane package is available to build directly without any extra git/clone step.

Agree on punting the Earthly automation to a follow-up — happy to file an issue for it. The wrinkle: baking a pre-built crossplane binary into +go-test would couple the test target's cache key to a specific upstream commit, so we'd want to think about whether to plumb the binary through as an artifact from a separate target (e.g. +build-crossplane) that's only invalidated when go.mod's crossplane pin changes.

- diff_processor.go: drop dead schema re-stamp on output.CompositeResource; the binary already writes data at the right path post-#7452 and we never call schema-aware accessors on the output (only GetUnstructured()). - diff_processor.go: drop defensive defClient nil check in resolveSchemaAndXRDForRender; nil defClient is a programmer error and silent fallback masked it. - diff_processor_test.go: stamp a Definition mock on the two test sites that previously relied on the nil guard. - diff_processor_test.go: rewrite the SchemaPlumbing test comment to describe current behavior at HEAD instead of historical version pinning. - render_engine.go: convert the if/else if branch in Render to a switch. - render_engine_test.go: drop the pre-fix/post-fix framing on the multi-composition test; describe the required behavior plainly. - diff_formatter.go: drop dead "(generated)" legacy paths in stripSyntheticName / normalizedGenerateName (we never stamp that suffix into metadata.name now). Add a top-of-block doc explaining the generateName-display machinery. - diff_renderer.go: collapse getKindName's two-branch switch into one expression — both branches returned identical values. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Copilot

Pull request overview

Copilot reviewed 87 out of 88 changed files in this pull request and generated 1 comment.

+// EngineRenderFn is the default RenderFn implementation. It lazily sets up the
+// render engine and starts function runtimes on first use, reuses them across
+// subsequent calls, and serializes concurrent renders with an internal mutex.
+//


Replace the ad-hoc "xrgenplace0000" sentinel with a 12-hex suffix derived deterministically from a fixed seed via the same hash truncation upstream's internal/names.ChildName uses. The synthesized XR name is now shape- compatible with the composed-resource names crossplane's nameGenerator emits ("<gen-with-dash><12 lowercase hex>") AND the suffix value itself is distinctive enough to detect when interpolated downstream. Collapse displayNameFromPlaceholder, isGeneratedFromGenerateName, and normalizedGenerateName into two helpers in renderer/types: - LooksLikeGeneratedName(name, generateName) — single detector covering both the shape-match path (binary-generated composed resources whose template carries a generateName) and the embedded-suffix path (resources whose template interpolated the synthesized XR name into their own). - GeneratedDisplayName(name, generateName) — single display formatter symmetric with the detector. Drop the dead "(generated)"-suffix legacy paths and the stale doc block. Net delete: ~30 LOC, two helpers fewer, one mental model. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Drop the hand-rolled rune scan in isLowerHex; hex.DecodeString does the character validation for us, and we keep the lowercase guard since EncodeToString emits lowercase only. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Copilot

Pull request overview

Copilot reviewed 87 out of 88 changed files in this pull request and generated 1 comment.

+func stripSyntheticName(metadata map[string]any, name, generateName string, nameFound, genNameFound bool) []string {
+	if !nameFound || !genNameFound {
+		return nil
+	}
+
+	if !t.LooksLikeGeneratedName(name, generateName) {
+		return nil
+	}
+
+	delete(metadata, "name")
+
+	return []string{fmt.Sprintf("removed display name %q", name)}
+}


The guard short-circuited before LooksLikeGeneratedName ran, which contradicted the function's contract: the embedded-suffix path (XR-synthesis suffix interpolated into a downstream resource's metadata.name via a composition template) typically has no generateName field on the rendered resource, and LooksLikeGeneratedName already handles that case via Contains-on-suffix. Without this fix, those synthetic names leaked into the diff body. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>

Copilot AI review requested due to automatic review settings June 5, 2026 17:38

Copilot started reviewing on behalf of jcogilvie June 5, 2026 17:39 View session

Copilot AI reviewed Jun 5, 2026

View reviewed changes

Comment thread cmd/diff/client/crossplane/definition_client.go

Comment thread cmd/diff/client/crossplane/definition_client.go Outdated

Comment thread cmd/diff/diffprocessor/requirements_provider.go Outdated

Comment thread cmd/diff/diff_it_utils_test.go Outdated

jcogilvie force-pushed the render-engine branch from d32caee to bb559a3 Compare June 5, 2026 18:37

Copilot AI review requested due to automatic review settings June 5, 2026 18:50

jcogilvie force-pushed the render-engine branch from bb559a3 to da2f192 Compare June 5, 2026 18:50

Copilot started reviewing on behalf of jcogilvie June 5, 2026 18:50 View session

jcogilvie commented Jun 5, 2026

View reviewed changes

Copilot AI reviewed Jun 5, 2026

Copilot AI review requested due to automatic review settings June 5, 2026 20:13

Copilot started reviewing on behalf of jcogilvie June 5, 2026 20:14 View session

Copilot AI reviewed Jun 5, 2026

View reviewed changes

Comment thread cmd/diff/diffprocessor/diff_processor.go Outdated

Comment thread cmd/diff/diff_it_utils_test.go Outdated

Comment thread go.mod Outdated

Comment thread cmd/diff/diffprocessor/requirements_provider_test.go

jcogilvie mentioned this pull request Jun 8, 2026

feat(render): capture stderr and handle pipeline-fatal exit code in both engines crossplane/cli#91

Merged

5 tasks

Copilot AI review requested due to automatic review settings June 9, 2026 18:14

Copilot started reviewing on behalf of jcogilvie June 9, 2026 18:14 View session

Copilot AI reviewed Jun 9, 2026

View reviewed changes

Comment thread cmd/diff/diffprocessor/render_engine.go Outdated

Comment thread go.mod Outdated

Comment thread cmd/diff/diffprocessor/render_engine_test.go

Comment thread cmd/diff/diffprocessor/render_engine_test.go Outdated

Comment thread cmd/diff/diffprocessor/schema_validator.go Outdated

Copilot AI review requested due to automatic review settings June 9, 2026 21:48

Copilot started reviewing on behalf of jcogilvie June 9, 2026 21:48 View session

Copilot AI reviewed Jun 9, 2026

View reviewed changes

Comment thread cmd/diff/diffprocessor/render_engine.go

Comment thread cmd/diff/diffprocessor/render_engine.go Outdated

Comment thread cmd/diff/diffprocessor/diff_processor.go Outdated

This was referenced Jun 9, 2026

render: Engine.Setup not idempotent — no clean way to add functions to an existing render network crossplane/cli#96

Closed

render: unwind multi-composition workaround in EngineRenderFn once upstream Engine API supports it #338

Closed

jcogilvie requested a review from tampakrap as a code owner June 9, 2026 22:29

jcogilvie and others added 3 commits June 9, 2026 18:41

jcogilvie and others added 2 commits June 9, 2026 23:01

Copilot AI review requested due to automatic review settings June 10, 2026 03:29

Copilot started reviewing on behalf of jcogilvie June 10, 2026 03:29 View session