From 5050d31ad43d14727d46da2df136b8fe0bb04b50 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie <679297+jcogilvie@users.noreply.github.com> Date: Fri, 26 Jun 2026 11:25:47 -0400 Subject: [PATCH 1/2] fix(render): annotate functions when reusing an existing network MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dockerRenderEngine.Setup short-circuits when e.network is already set — either pre-configured via --crossplane-docker-network or set by a prior Setup call on the same engine. Until now that short-circuit skipped injectNetworkAnnotation, leaving the supplied fns un-annotated and their containers unreachable from the render container. Annotate fns in that branch too. injectNetworkAnnotation already declines to overwrite a fn that carries its own runtime-docker-network annotation, so users who pin fns to a specific network are unaffected. This makes Setup idempotent for in-process multi-batch rendering: a tool like crossplane-contrib/crossplane-diff that calls Render across many compositions in a single process can call Setup once per composition, and every batch's fns end up on the same network. The first call returns the real cleanup; subsequent calls return no-ops, so defer cleanup() per batch is safe in LIFO order. Fixes #96 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Jonathan Ogilvie <679297+jcogilvie@users.noreply.github.com> --- cmd/crossplane/render/engine.go | 11 +- cmd/crossplane/render/engine_docker.go | 24 ++-- cmd/crossplane/render/engine_docker_test.go | 138 ++++++++++++++++++++ 3 files changed, 162 insertions(+), 11 deletions(-) diff --git a/cmd/crossplane/render/engine.go b/cmd/crossplane/render/engine.go index d8ebed0c..ba40992f 100644 --- a/cmd/crossplane/render/engine.go +++ b/cmd/crossplane/render/engine.go @@ -39,8 +39,15 @@ type Engine interface { // Setup performs engine-specific pre-render preparation, such as // creating Docker networks and annotating functions so their containers - // can reach the render engine. It may mutate fns. The returned cleanup - // function must be called when rendering is done. + // can reach the render engine. It may mutate fns. + // + // Setup may be called more than once on the same engine to integrate + // additional functions into an environment created by a prior call. The + // first call initializes the environment (e.g. creates the Docker + // network) and returns a real cleanup; subsequent calls integrate the + // supplied fns and return a no-op cleanup. The single real cleanup must + // be called when rendering is done; callers can safely defer every + // returned cleanup in LIFO order without coordinating which one is real. Setup(ctx context.Context, fns []pkgv1.Function) (cleanup func(), err error) // Render executes the render request and returns the response. diff --git a/cmd/crossplane/render/engine_docker.go b/cmd/crossplane/render/engine_docker.go index 7815f138..c73bdf26 100644 --- a/cmd/crossplane/render/engine_docker.go +++ b/cmd/crossplane/render/engine_docker.go @@ -77,20 +77,26 @@ func (e *dockerRenderEngine) CheckContextSupport() error { return nil } -// Setup creates a temporary Docker network, records its name so the render -// container joins it, and annotates the supplied functions so their -// containers also join it. The returned cleanup function removes the -// network. +// Setup wires the supplied functions into the Docker network the render +// container joins. On the first call with an unset e.network, it creates a +// temporary network, records its name, annotates fns to join it, and returns +// a cleanup that removes the network. On any subsequent call — or on the +// first call when e.network was already set via --crossplane-docker-network — +// it only annotates fns with the existing network and returns a no-op +// cleanup, leaving network ownership to whoever created it. fns whose +// runtime-docker-network annotation is already set are left untouched. func (e *dockerRenderEngine) Setup(ctx context.Context, fns []pkgv1.Function) (func(), error) { - var networkID, networkName string - if e.network != "" { - // e.network was pre-configured, we don't own the network, so there is nothing to clean up. + // Either the user pre-configured the network via the --crossplane-docker-network flag + // (we don't own it), or a prior Setup call on this engine already created one (the + // real cleanup belongs to that first call). In both cases we still need to annotate + // the supplied fns so their containers join the network — injectNetworkAnnotation + // won't overwrite an annotation a fn already carries. + injectNetworkAnnotation(fns, e.network) return func() {}, nil } - var err error - networkID, networkName, err = createRenderNetwork(ctx) + networkID, networkName, err := createRenderNetwork(ctx) if err != nil { return func() {}, errors.Wrap(err, "cannot create Docker network for rendering") } diff --git a/cmd/crossplane/render/engine_docker_test.go b/cmd/crossplane/render/engine_docker_test.go index d27bedc5..97795156 100644 --- a/cmd/crossplane/render/engine_docker_test.go +++ b/cmd/crossplane/render/engine_docker_test.go @@ -28,6 +28,8 @@ import ( "github.com/crossplane/crossplane-runtime/v2/pkg/logging" + pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" + "github.com/crossplane/cli/v2/internal/docker" renderv1alpha1 "github.com/crossplane/cli/v2/proto/render/v1alpha1" ) @@ -212,6 +214,142 @@ func TestDockerRenderEngineRender(t *testing.T) { } } +func TestDockerRenderEngineSetup(t *testing.T) { + // These cases all exercise the early-return branch of Setup — where + // e.network is already non-empty (set by a prior Setup call on the same + // engine, or by NewEngineFromFlags from the --crossplane-docker-network + // flag). The branch must annotate the supplied functions so their + // containers join the network, while never creating a second network and + // always returning a no-op cleanup. The create-new-network branch is not + // covered here because it depends on a live Docker daemon; the tests for + // the broader render commands exercise it integration-style. + const presetNetwork = "preset-net" + + type want struct { + fns []pkgv1.Function + cleanupNotNil bool + } + + cases := map[string]struct { + reason string + engine *dockerRenderEngine + fns []pkgv1.Function + want want + }{ + "AnnotatesFunctionsWhenNetworkPreset": { + reason: "When e.network is set, Setup must inject the network annotation on every fn that does not already carry one, so that crossplane-diff-style multi-batch callers can re-Setup to add new fns to the same network.", + engine: &dockerRenderEngine{network: presetNetwork, log: logging.NewNopLogger()}, + fns: []pkgv1.Function{ + functionWithAnnotations(nil), + functionWithAnnotations(nil), + }, + want: want{ + fns: []pkgv1.Function{ + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: presetNetwork}), + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: presetNetwork}), + }, + cleanupNotNil: true, + }, + }, + "PreservesUserSetFunctionAnnotation": { + reason: "If a fn already carries a runtime-docker-network annotation, Setup must not overwrite it. This preserves the don't-overwrite contract from PR #65 for users who pin their fns to a specific network.", + engine: &dockerRenderEngine{network: presetNetwork, log: logging.NewNopLogger()}, + fns: []pkgv1.Function{ + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: "user-pinned-net"}), + }, + want: want{ + fns: []pkgv1.Function{ + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: "user-pinned-net"}), + }, + cleanupNotNil: true, + }, + }, + "EmptyFunctionsList": { + reason: "An empty fns slice is the trivial boundary case: Setup must succeed and return a non-nil (no-op) cleanup. No panic on nil annotations map.", + engine: &dockerRenderEngine{network: presetNetwork, log: logging.NewNopLogger()}, + fns: nil, + want: want{ + fns: nil, + cleanupNotNil: true, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + cleanup, err := tc.engine.Setup(context.Background(), tc.fns) + if err != nil { + t.Fatalf("\n%s\nSetup(...): unexpected error: %v", tc.reason, err) + } + if tc.want.cleanupNotNil && cleanup == nil { + t.Fatalf("\n%s\nSetup(...): cleanup is nil, want non-nil", tc.reason) + } + // Cleanup must not panic. For the early-return branch it is a no-op, + // but exercising the call documents the contract. + cleanup() + + if diff := cmp.Diff(tc.want.fns, tc.fns); diff != "" { + t.Errorf("\n%s\nSetup(...): fns -want, +got:\n%s", tc.reason, diff) + } + + if tc.engine.network != presetNetwork { + t.Errorf("\n%s\nSetup(...): e.network mutated from %q to %q (early-return branch must not change it)", tc.reason, presetNetwork, tc.engine.network) + } + }) + } +} + +func TestDockerRenderEngineSetupMultiBatch(t *testing.T) { + // Simulates the in-process multi-composition use case from + // crossplane/cli#96: a downstream tool (crossplane-diff) calls Setup once + // per Composition it encounters. The first call creates the network; each + // subsequent call must annotate its new fns with the same network so all + // function containers can reach each other and the render container. + // + // We seed e.network to "first-batch-net" to stand in for the prior Setup + // call that would have created the network. This keeps the test + // hermetic (no Docker daemon required) while still exercising the + // multi-batch path that motivated this change. + const network = "first-batch-net" + + e := &dockerRenderEngine{network: network, log: logging.NewNopLogger()} + + batch1 := []pkgv1.Function{functionWithAnnotations(nil), functionWithAnnotations(nil)} + batch2 := []pkgv1.Function{functionWithAnnotations(nil)} + + cleanup1, err := e.Setup(context.Background(), batch1) + if err != nil { + t.Fatalf("Setup(batch1): unexpected error: %v", err) + } + cleanup2, err := e.Setup(context.Background(), batch2) + if err != nil { + t.Fatalf("Setup(batch2): unexpected error: %v", err) + } + + // Both cleanups are no-ops here (e.network was pre-set, so neither Setup + // call created a network). Calling them in defer-LIFO order must not + // panic. In a real caller, the very first Setup — the one that actually + // created the network — would return the real cleanup; we cannot exercise + // that path without Docker. + cleanup2() + cleanup1() + + wantBatch1 := []pkgv1.Function{ + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: network}), + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: network}), + } + wantBatch2 := []pkgv1.Function{ + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: network}), + } + + if diff := cmp.Diff(wantBatch1, batch1); diff != "" { + t.Errorf("Setup(batch1): fns -want, +got:\n%s", diff) + } + if diff := cmp.Diff(wantBatch2, batch2); diff != "" { + t.Errorf("Setup(batch2): fns -want, +got:\n%s", diff) + } +} + // nonExitError is a stand-in for non-*ContainerExitError failures (e.g. image // pull errors) returned by docker.RunContainer. type nonExitError struct{ msg string } From ad8307749f03813a3b2f261dd2d623c6809bb926 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie <679297+jcogilvie@users.noreply.github.com> Date: Fri, 26 Jun 2026 13:47:11 -0400 Subject: [PATCH 2/2] chore(render): address review feedback on PR #159 - Fold the multi-batch idempotency test into the existing table-driven Setup cases (per coderabbit.yaml path instructions for *_test.go), and add an explicit empty-slice boundary case alongside the nil one. - Tighten the Engine.Setup interface docstring so it doesn't overstate "first call returns a real cleanup": the cleanup is real only when a call creates a new environment, which excludes calls on engines that are pre-configured to use an externally-managed environment (e.g. the Docker engine constructed with --crossplane-docker-network) and engines that have no environment to clean up (e.g. the local engine). Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Jonathan Ogilvie <679297+jcogilvie@users.noreply.github.com> --- cmd/crossplane/render/engine.go | 15 +- cmd/crossplane/render/engine_docker_test.go | 183 ++++++++++---------- 2 files changed, 98 insertions(+), 100 deletions(-) diff --git a/cmd/crossplane/render/engine.go b/cmd/crossplane/render/engine.go index ba40992f..63125f05 100644 --- a/cmd/crossplane/render/engine.go +++ b/cmd/crossplane/render/engine.go @@ -42,12 +42,15 @@ type Engine interface { // can reach the render engine. It may mutate fns. // // Setup may be called more than once on the same engine to integrate - // additional functions into an environment created by a prior call. The - // first call initializes the environment (e.g. creates the Docker - // network) and returns a real cleanup; subsequent calls integrate the - // supplied fns and return a no-op cleanup. The single real cleanup must - // be called when rendering is done; callers can safely defer every - // returned cleanup in LIFO order without coordinating which one is real. + // additional functions into an environment created by a prior call. + // Only the call that creates a new environment returns a real cleanup; + // calls that integrate fns into an environment that already exists + // (because a prior Setup call established it, or because the engine was + // pre-configured to use an externally-managed environment) return a + // no-op cleanup, as do calls on engines with nothing to clean up. The + // real cleanup, if any, must be called when rendering is done; callers + // can safely defer every returned cleanup in LIFO order without + // coordinating which one is real. Setup(ctx context.Context, fns []pkgv1.Function) (cleanup func(), err error) // Render executes the render request and returns the response. diff --git a/cmd/crossplane/render/engine_docker_test.go b/cmd/crossplane/render/engine_docker_test.go index 97795156..696dc1a9 100644 --- a/cmd/crossplane/render/engine_docker_test.go +++ b/cmd/crossplane/render/engine_docker_test.go @@ -216,80 +216,126 @@ func TestDockerRenderEngineRender(t *testing.T) { func TestDockerRenderEngineSetup(t *testing.T) { // These cases all exercise the early-return branch of Setup — where - // e.network is already non-empty (set by a prior Setup call on the same - // engine, or by NewEngineFromFlags from the --crossplane-docker-network - // flag). The branch must annotate the supplied functions so their - // containers join the network, while never creating a second network and - // always returning a no-op cleanup. The create-new-network branch is not - // covered here because it depends on a live Docker daemon; the tests for - // the broader render commands exercise it integration-style. + // e.network is already non-empty, either because NewEngineFromFlags + // populated it from --crossplane-docker-network or because a prior Setup + // call on the same engine stored its created network there. The branch + // must annotate the supplied functions so their containers join the + // network, never create a second network, and always return a no-op + // cleanup. The create-new-network branch is not covered here because it + // depends on a live Docker daemon; the broader render command tests + // exercise it integration-style. + // + // The MultiBatchAnnotatesAdditionalFunctions case simulates the + // in-process multi-composition use case from crossplane/cli#96: a + // downstream tool (crossplane-diff) calls Setup once per Composition it + // encounters. Pre-seeding e.network stands in for the prior Setup call + // that would have created the network, keeping the test hermetic. const presetNetwork = "preset-net" - type want struct { - fns []pkgv1.Function - cleanupNotNil bool + // batch holds a single Setup invocation: the fns to pass in and the + // expected fn state after Setup returns. Multi-call cases provide more + // than one batch; the runner invokes Setup once per batch in order. + type batch struct { + fns []pkgv1.Function + wantFns []pkgv1.Function } cases := map[string]struct { - reason string - engine *dockerRenderEngine - fns []pkgv1.Function - want want + reason string + engine *dockerRenderEngine + batches []batch }{ "AnnotatesFunctionsWhenNetworkPreset": { reason: "When e.network is set, Setup must inject the network annotation on every fn that does not already carry one, so that crossplane-diff-style multi-batch callers can re-Setup to add new fns to the same network.", engine: &dockerRenderEngine{network: presetNetwork, log: logging.NewNopLogger()}, - fns: []pkgv1.Function{ - functionWithAnnotations(nil), - functionWithAnnotations(nil), - }, - want: want{ + batches: []batch{{ fns: []pkgv1.Function{ + functionWithAnnotations(nil), + functionWithAnnotations(nil), + }, + wantFns: []pkgv1.Function{ functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: presetNetwork}), functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: presetNetwork}), }, - cleanupNotNil: true, - }, + }}, }, "PreservesUserSetFunctionAnnotation": { reason: "If a fn already carries a runtime-docker-network annotation, Setup must not overwrite it. This preserves the don't-overwrite contract from PR #65 for users who pin their fns to a specific network.", engine: &dockerRenderEngine{network: presetNetwork, log: logging.NewNopLogger()}, - fns: []pkgv1.Function{ - functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: "user-pinned-net"}), - }, - want: want{ + batches: []batch{{ fns: []pkgv1.Function{ functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: "user-pinned-net"}), }, - cleanupNotNil: true, - }, + wantFns: []pkgv1.Function{ + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: "user-pinned-net"}), + }, + }}, + }, + "NilFunctionsList": { + reason: "A nil fns slice is a boundary case: Setup must succeed without panicking on the nil-map guard inside injectNetworkAnnotation.", + engine: &dockerRenderEngine{network: presetNetwork, log: logging.NewNopLogger()}, + batches: []batch{{ + fns: nil, + wantFns: nil, + }}, }, "EmptyFunctionsList": { - reason: "An empty fns slice is the trivial boundary case: Setup must succeed and return a non-nil (no-op) cleanup. No panic on nil annotations map.", + reason: "An empty (non-nil) fns slice is the other boundary: Setup must succeed, the slice stays empty, and no spurious entries appear.", engine: &dockerRenderEngine{network: presetNetwork, log: logging.NewNopLogger()}, - fns: nil, - want: want{ - fns: nil, - cleanupNotNil: true, + batches: []batch{{ + fns: []pkgv1.Function{}, + wantFns: []pkgv1.Function{}, + }}, + }, + "MultiBatchAnnotatesAdditionalFunctions": { + reason: "Two consecutive Setup calls on the same engine — the crossplane/cli#96 in-process multi-composition pattern — must annotate every batch with the same network. Each call returns a no-op cleanup that is safe to defer in LIFO order.", + engine: &dockerRenderEngine{network: presetNetwork, log: logging.NewNopLogger()}, + batches: []batch{ + { + fns: []pkgv1.Function{ + functionWithAnnotations(nil), + functionWithAnnotations(nil), + }, + wantFns: []pkgv1.Function{ + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: presetNetwork}), + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: presetNetwork}), + }, + }, + { + fns: []pkgv1.Function{ + functionWithAnnotations(nil), + }, + wantFns: []pkgv1.Function{ + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: presetNetwork}), + }, + }, }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { - cleanup, err := tc.engine.Setup(context.Background(), tc.fns) - if err != nil { - t.Fatalf("\n%s\nSetup(...): unexpected error: %v", tc.reason, err) - } - if tc.want.cleanupNotNil && cleanup == nil { - t.Fatalf("\n%s\nSetup(...): cleanup is nil, want non-nil", tc.reason) + cleanups := make([]func(), 0, len(tc.batches)) + for i, b := range tc.batches { + cleanup, err := tc.engine.Setup(context.Background(), b.fns) + if err != nil { + t.Fatalf("\n%s\nSetup(batch %d): unexpected error: %v", tc.reason, i, err) + } + if cleanup == nil { + t.Fatalf("\n%s\nSetup(batch %d): cleanup is nil, want non-nil", tc.reason, i) + } + cleanups = append(cleanups, cleanup) + + if diff := cmp.Diff(b.wantFns, b.fns); diff != "" { + t.Errorf("\n%s\nSetup(batch %d): fns -want, +got:\n%s", tc.reason, i, diff) + } } - // Cleanup must not panic. For the early-return branch it is a no-op, - // but exercising the call documents the contract. - cleanup() - if diff := cmp.Diff(tc.want.fns, tc.fns); diff != "" { - t.Errorf("\n%s\nSetup(...): fns -want, +got:\n%s", tc.reason, diff) + // Defer-LIFO: all cleanups in this test are no-ops (we pre-seed + // e.network, so no call took the create-network branch). Calling + // them must not panic. + for i := len(cleanups) - 1; i >= 0; i-- { + cleanups[i]() } if tc.engine.network != presetNetwork { @@ -299,57 +345,6 @@ func TestDockerRenderEngineSetup(t *testing.T) { } } -func TestDockerRenderEngineSetupMultiBatch(t *testing.T) { - // Simulates the in-process multi-composition use case from - // crossplane/cli#96: a downstream tool (crossplane-diff) calls Setup once - // per Composition it encounters. The first call creates the network; each - // subsequent call must annotate its new fns with the same network so all - // function containers can reach each other and the render container. - // - // We seed e.network to "first-batch-net" to stand in for the prior Setup - // call that would have created the network. This keeps the test - // hermetic (no Docker daemon required) while still exercising the - // multi-batch path that motivated this change. - const network = "first-batch-net" - - e := &dockerRenderEngine{network: network, log: logging.NewNopLogger()} - - batch1 := []pkgv1.Function{functionWithAnnotations(nil), functionWithAnnotations(nil)} - batch2 := []pkgv1.Function{functionWithAnnotations(nil)} - - cleanup1, err := e.Setup(context.Background(), batch1) - if err != nil { - t.Fatalf("Setup(batch1): unexpected error: %v", err) - } - cleanup2, err := e.Setup(context.Background(), batch2) - if err != nil { - t.Fatalf("Setup(batch2): unexpected error: %v", err) - } - - // Both cleanups are no-ops here (e.network was pre-set, so neither Setup - // call created a network). Calling them in defer-LIFO order must not - // panic. In a real caller, the very first Setup — the one that actually - // created the network — would return the real cleanup; we cannot exercise - // that path without Docker. - cleanup2() - cleanup1() - - wantBatch1 := []pkgv1.Function{ - functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: network}), - functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: network}), - } - wantBatch2 := []pkgv1.Function{ - functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: network}), - } - - if diff := cmp.Diff(wantBatch1, batch1); diff != "" { - t.Errorf("Setup(batch1): fns -want, +got:\n%s", diff) - } - if diff := cmp.Diff(wantBatch2, batch2); diff != "" { - t.Errorf("Setup(batch2): fns -want, +got:\n%s", diff) - } -} - // nonExitError is a stand-in for non-*ContainerExitError failures (e.g. image // pull errors) returned by docker.RunContainer. type nonExitError struct{ msg string }