diff --git a/cmd/crossplane/render/engine.go b/cmd/crossplane/render/engine.go index d8ebed0..63125f0 100644 --- a/cmd/crossplane/render/engine.go +++ b/cmd/crossplane/render/engine.go @@ -39,8 +39,18 @@ 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. + // 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.go b/cmd/crossplane/render/engine_docker.go index 7815f13..c73bdf2 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 d27bedc..696dc1a 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,137 @@ 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, 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" + + // 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 + 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()}, + batches: []batch{{ + fns: []pkgv1.Function{ + functionWithAnnotations(nil), + functionWithAnnotations(nil), + }, + wantFns: []pkgv1.Function{ + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: presetNetwork}), + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: presetNetwork}), + }, + }}, + }, + "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()}, + batches: []batch{{ + fns: []pkgv1.Function{ + functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: "user-pinned-net"}), + }, + 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 (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()}, + 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) { + 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) + } + } + + // 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 { + 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) + } + }) + } +} + // nonExitError is a stand-in for non-*ContainerExitError failures (e.g. image // pull errors) returned by docker.RunContainer. type nonExitError struct{ msg string }