From d84160177be56c4161d8579f0ed125809e488d69 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 26 Jun 2026 13:17:44 -0400 Subject: [PATCH 1/6] refactor(render): unwind multi-composition workaround using cli#159 crossplane/cli#159 makes dockerRenderEngine.Setup idempotent in the network-already-set branch by calling injectNetworkAnnotation there too. That removes the need for EngineRenderFn to capture the docker network name off the first batch's annotations and re-stamp it on later batches itself. EngineRenderFn now calls engine.Setup(ctx, newFns) on every render and accumulates the returned cleanups in a slice that Cleanup walks in LIFO order. The first cleanup is the real one; the rest are no-ops, so the order keeps network teardown last without any first-vs-subsequent bookkeeping. The applyNetworkAnnotation helper was also used by function_provider's CROSSPLANE_DIFF_DOCKER_NETWORK env-var path. That's an independent use case (pre-stamping fns so dockerRenderEngine.Setup's don't-overwrite clause picks up the user's network), so the 6-line loop is inlined into applyDockerNetworkAnnotation, its only remaining caller. The go.mod replace directive points at the PR #159 head while it's in review. Drop it once that PR merges and a crossplane/cli release ships with the fix. Fixes #338 Signed-off-by: Jonathan Ogilvie --- cmd/diff/diffprocessor/function_provider.go | 19 ++- cmd/diff/diffprocessor/render_engine.go | 142 ++++++-------------- go.mod | 2 + go.sum | 4 +- 4 files changed, 59 insertions(+), 108 deletions(-) diff --git a/cmd/diff/diffprocessor/function_provider.go b/cmd/diff/diffprocessor/function_provider.go index a43c4524..b6796b8c 100644 --- a/cmd/diff/diffprocessor/function_provider.go +++ b/cmd/diff/diffprocessor/function_provider.go @@ -27,6 +27,7 @@ import ( "time" xp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/crossplane" + "github.com/crossplane/cli/v2/cmd/crossplane/render" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/client" @@ -56,10 +57,9 @@ type FunctionProvider interface { const EnvDockerNetwork = "CROSSPLANE_DIFF_DOCKER_NETWORK" // applyDockerNetworkAnnotation stamps each function with the Docker network -// annotation drawn from EnvDockerNetwork when that variable is set. The -// underlying applyNetworkAnnotation helper preserves any non-empty value the -// caller has already set, so this function never overwrites an explicitly -// configured network. +// annotation drawn from EnvDockerNetwork when that variable is set. Functions +// that already carry a non-empty value for the annotation are left untouched, +// so this never overwrites an explicitly configured network. func applyDockerNetworkAnnotation(fns []pkgv1.Function, log logging.Logger) { network := os.Getenv(EnvDockerNetwork) if network == "" { @@ -67,7 +67,16 @@ func applyDockerNetworkAnnotation(fns []pkgv1.Function, log logging.Logger) { } log.Debug("Setting Docker network annotation on functions", "network", network) - applyNetworkAnnotation(fns, network) + + for i := range fns { + if fns[i].Annotations == nil { + fns[i].Annotations = make(map[string]string) + } + + if fns[i].Annotations[render.AnnotationKeyRuntimeDockerNetwork] == "" { + fns[i].Annotations[render.AnnotationKeyRuntimeDockerNetwork] = network + } + } } // DefaultFunctionProvider fetches functions from the cluster on each call. diff --git a/cmd/diff/diffprocessor/render_engine.go b/cmd/diff/diffprocessor/render_engine.go index a34a4fcb..c2a4470d 100644 --- a/cmd/diff/diffprocessor/render_engine.go +++ b/cmd/diff/diffprocessor/render_engine.go @@ -68,41 +68,26 @@ type RenderInputs struct { // render engine and starts function runtimes on first use, reuses them across // subsequent calls, and serializes concurrent renders with an internal mutex. // -// Multi-composition note. A single `xr` invocation can render against XRs from -// multiple compositions whose function pipelines overlap but aren't identical. -// To handle this correctly without leaking docker networks (upstream's -// dockerRenderEngine.Setup creates a fresh network on every call as of cli -// v2.3.2), we: -// - call engine.Setup exactly once on the first render — that creates the -// docker network N and stamps each first-batch function with its name via -// the runtime-docker-network annotation; -// - capture N from any first-batch function's annotation into networkName; -// - on later renders, manually apply the same annotation to any function we -// haven't already started (preserving any value the caller pre-set); -// - track started functions by name in addrs, and accumulate every -// *FunctionAddresses returned by startRuntimes in fnAddrsList so Cleanup -// can stop them all. -// -// This is a self-contained workaround. The cleaner shape — Engine.Setup either -// idempotent or paired with an Engine.AnnotateFunctions method — needs an -// upstream API change in crossplane/cli (tracked in crossplane/cli#96). -// Unwind once a cli release ships that fix; tracked downstream in -// crossplane-contrib/crossplane-diff#338. +// A single `xr` invocation can render against XRs from multiple compositions +// whose function pipelines overlap but aren't identical. Upstream's +// dockerRenderEngine.Setup is idempotent (crossplane/cli#159): the first call +// creates the docker network and annotates the supplied fns; later calls +// annotate any newly-arrived fns with the same network and return a no-op +// cleanup. We accumulate every cleanup and call them in LIFO order on +// Cleanup, alongside stopping every FunctionAddresses we ever started. type EngineRenderFn struct { - engine render.Engine - networkCleanup func() - networkName string - fnAddrsList []*render.FunctionAddresses + engine render.Engine + cleanups []func() + addrsList []*render.FunctionAddresses // startedNames is the set of function names we've already passed to // startRuntimes; used for dedup so we don't restart a function that's // already running. startedNames map[string]struct{} // addrs is the merged Addresses() map from every startRuntimes call. // Filtered to in.Functions when building each render request. - addrs map[string]string - started bool - mu sync.Mutex - log logging.Logger + addrs map[string]string + mu sync.Mutex + log logging.Logger // startRuntimes / stopRuntimes are seams for testing. They default to the // real render package functions. @@ -130,9 +115,9 @@ func NewEngineRenderFn(log logging.Logger, binaryPath string) *EngineRenderFn { } // Render performs one render. It is safe for concurrent use — calls are -// serialized internally. Setup runs on the first invocation (creating the -// docker network); subsequent invocations annotate any newly-encountered -// functions with the captured network name and start their runtimes. +// serialized internally. Setup runs on every invocation; upstream guarantees +// it is idempotent (the first call creates the docker network, later calls +// just annotate any newly-arrived functions with it). func (e *EngineRenderFn) Render(ctx context.Context, log logging.Logger, in RenderInputs) (render.CompositionOutputs, error) { e.mu.Lock() defer e.mu.Unlock() @@ -157,43 +142,27 @@ func (e *EngineRenderFn) Render(ctx context.Context, log logging.Logger, in Rend newFns = append(newFns, in.Functions[i]) } - switch { - case !e.started: - // First call: let upstream Setup create the docker network and - // stamp the first batch with the network annotation. We then read - // the network name off the annotated functions for use on later - // calls. - cleanup, err := e.engine.Setup(ctx, newFns) - if err != nil { - return render.CompositionOutputs{}, errors.Wrap(err, "cannot setup render engine") - } - - e.networkCleanup = cleanup - e.networkName = firstNetworkAnnotation(newFns) - e.started = true - case e.networkName != "": - // Subsequent call: upstream's Setup is single-shot in cli v2.3.2 - // (calling it again would create a new network and leak the first - // one), so we apply the same annotation to new functions ourselves. - // Preserve any value the caller pre-set. - applyNetworkAnnotation(newFns, e.networkName) + // Setup is idempotent (crossplane/cli#159): the first call creates the + // docker network and returns the real cleanup; subsequent calls only + // annotate newFns to join the existing network and return a no-op + // cleanup. Accumulating every returned cleanup and calling them in LIFO + // order on Cleanup is the recommended caller shape. + cleanup, err := e.engine.Setup(ctx, newFns) + if err != nil { + return render.CompositionOutputs{}, errors.Wrap(err, "cannot setup render engine") } if len(newFns) > 0 { - fa, err := e.startRuntimes(ctx, log, newFns) - if err != nil { - // Unwind the setup cleanup on the first-call failure path so - // we don't leak a network when no functions are running. - if e.networkCleanup != nil && len(e.fnAddrsList) == 0 { - e.networkCleanup() - e.networkCleanup = nil - e.started = false - } - - return render.CompositionOutputs{}, errors.Wrap(err, "cannot start function runtimes") + fa, startErr := e.startRuntimes(ctx, log, newFns) + if startErr != nil { + // Roll back this Setup. If it was the very first call, this + // releases the docker network it just created; if it was a + // subsequent call, the cleanup is a no-op and no harm is done. + cleanup() + return render.CompositionOutputs{}, errors.Wrap(startErr, "cannot start function runtimes") } - e.fnAddrsList = append(e.fnAddrsList, fa) + e.addrsList = append(e.addrsList, fa) for i := range newFns { e.startedNames[newFns[i].GetName()] = struct{}{} } @@ -201,6 +170,8 @@ func (e *EngineRenderFn) Render(ctx context.Context, log logging.Logger, in Rend maps.Copy(e.addrs, fa.Addresses()) } + e.cleanups = append(e.cleanups, cleanup) + // Build request with addresses for in.Functions only — the binary needs // addresses for this render's pipeline, not for every function we've // ever started. @@ -256,37 +227,6 @@ func (e *EngineRenderFn) Render(ctx context.Context, log logging.Logger, in Rend return out, nil } -// firstNetworkAnnotation returns the value of the runtime-docker-network -// annotation of the first function in fns that has it, or "" if none do. -// Upstream's dockerRenderEngine.Setup stamps every function in its input slice -// with the same value, so picking the first non-empty one is sufficient. The -// local engine path leaves no annotation, so this returns "" for that case. -func firstNetworkAnnotation(fns []pkgv1.Function) string { - for i := range fns { - if v := fns[i].GetAnnotations()[render.AnnotationKeyRuntimeDockerNetwork]; v != "" { - return v - } - } - - return "" -} - -// applyNetworkAnnotation sets the runtime-docker-network annotation on each -// function that doesn't already have a non-empty value for it. Mirrors the -// upstream injectNetworkAnnotation helper (which is unexported) so we can do -// the same job for functions discovered after the first Setup call. -func applyNetworkAnnotation(fns []pkgv1.Function, networkName string) { - for i := range fns { - if fns[i].Annotations == nil { - fns[i].Annotations = make(map[string]string) - } - - if fns[i].Annotations[render.AnnotationKeyRuntimeDockerNetwork] == "" { - fns[i].Annotations[render.AnnotationKeyRuntimeDockerNetwork] = networkName - } - } -} - // fakeXRUID mirrors the deterministic UID the binary assigns to the XR after // deserializing it. The binary at internal/render/composite/render.go (line 94 // in v2.3.2) overwrites xr.UID with @@ -359,21 +299,21 @@ func (e *EngineRenderFn) Cleanup(_ context.Context) error { e.mu.Lock() defer e.mu.Unlock() - for _, fa := range e.fnAddrsList { + for _, fa := range e.addrsList { e.stopRuntimes(e.log, fa) } - e.fnAddrsList = nil + e.addrsList = nil e.addrs = nil e.startedNames = nil - e.networkName = "" - if e.networkCleanup != nil { - e.networkCleanup() - e.networkCleanup = nil + // Run cleanups in LIFO order so the first Setup's real cleanup (which + // owns the docker network) runs last, after any later no-op cleanups. + for i := len(e.cleanups) - 1; i >= 0; i-- { + e.cleanups[i]() } - e.started = false + e.cleanups = nil return nil } diff --git a/go.mod b/go.mod index 30f484a2..4797d170 100644 --- a/go.mod +++ b/go.mod @@ -158,3 +158,5 @@ require ( k8s.io/kube-openapi v0.0.0-20260127142750-a19766b6e2d4 sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect ) + +replace github.com/crossplane/cli/v2 => github.com/jcogilvie/cli/v2 v2.0.0-20260626152547-5050d31ad43d diff --git a/go.sum b/go.sum index 97255889..95fac17b 100644 --- a/go.sum +++ b/go.sum @@ -47,8 +47,6 @@ github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSV github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= github.com/creack/pty v1.1.18/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= -github.com/crossplane/cli/v2 v2.4.0 h1:yzMTCGYncx6FeaoXkjF6BOMISgq90dbZDnht1cqHV38= -github.com/crossplane/cli/v2 v2.4.0/go.mod h1:myc+idRY+CHQZsLZE1a1CAFcn8fpL6EVvxwpF9RrrBM= github.com/crossplane/crossplane-runtime/v2 v2.4.0-rc.0 h1:Zgiq+hrh9lbjWtv8ECCLd1A0I9knt3c8ZUELExw6M1w= github.com/crossplane/crossplane-runtime/v2 v2.4.0-rc.0/go.mod h1:PAo3zIfmMzrS18HGyHJLXCeXIp0nFW2Md2Fn9gocMaU= github.com/crossplane/crossplane/apis/v2 v2.4.0-rc.0 h1:4PBahj+tnK9RwSZm1bYGvOkHOU+1CSHjJF2PoPzBMD0= @@ -169,6 +167,8 @@ github.com/hexops/gotextdiff v1.0.3 h1:gitA9+qJrrTCsiCl7+kh75nPqQt1cx4ZkudSTLoUq github.com/hexops/gotextdiff v1.0.3/go.mod h1:pSWU5MAI3yDq+fZBTazCSJysOMbxWL1BSow5/V2vxeg= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= +github.com/jcogilvie/cli/v2 v2.0.0-20260626152547-5050d31ad43d h1:/OavKQIx/+x2AuwAXkjY6JTI6PpUm+MWitWeK1nYmCE= +github.com/jcogilvie/cli/v2 v2.0.0-20260626152547-5050d31ad43d/go.mod h1:myc+idRY+CHQZsLZE1a1CAFcn8fpL6EVvxwpF9RrrBM= github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM= github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= github.com/klauspost/compress v1.18.6 h1:2jupLlAwFm95+YDR+NwD2MEfFO9d4z4Prjl1XXDjuao= From dbc653f795aa06329e2b7a1bd1c7d8cc51083179 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 26 Jun 2026 14:17:01 -0400 Subject: [PATCH 2/6] build(deps): repin crossplane/cli/v2 to upstream main post-merge of cli#159 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upstream PR crossplane/cli#159 — the idempotent-Setup fix this branch relies on — merged to crossplane/cli main at 86f5f7a0. Drop the fork replace directive and pin directly to that commit via the v2.5.0-rc.0.0.20260626181113-86f5f7a0c4bf pseudo-version so this PR can land without waiting for a tagged cli release. The transitive bumps in go.sum (go-openapi/swag/* 0.25.5 → 0.26.0, docker/cli 29.5.3 → 29.6.0, klog/v2 2.130.1 → 2.140.0) come from upstream cli main's newer indirect deps; nothing in crossplane-diff touches those modules directly. Once a tagged cli release containing cli#159 ships (likely v2.4.1 or v2.5.0), a follow-up should bump the pseudo-version to that tag. Signed-off-by: Jonathan Ogilvie --- go.mod | 32 ++++++++++++-------------- go.sum | 72 +++++++++++++++++++++++++++++----------------------------- 2 files changed, 51 insertions(+), 53 deletions(-) diff --git a/go.mod b/go.mod index 4797d170..180c0d2f 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( dario.cat/mergo v1.0.2 github.com/Masterminds/semver v1.5.0 github.com/alecthomas/kong v1.15.0 - github.com/crossplane/cli/v2 v2.4.0 + github.com/crossplane/cli/v2 v2.5.0-rc.0.0.20260626181113-86f5f7a0c4bf github.com/crossplane/crossplane-runtime/v2 v2.4.0-rc.0 github.com/crossplane/crossplane/apis/v2 v2.4.0-rc.0 github.com/crossplane/crossplane/v2 v2.3.3 @@ -45,17 +45,17 @@ require ( github.com/fxamacker/cbor/v2 v2.9.0 // indirect github.com/go-errors/errors v1.4.2 // indirect github.com/go-json-experiment/json v0.0.0-20240815175050-ebd3a8989ca1 // indirect - github.com/go-openapi/swag/cmdutils v0.25.5 // indirect - github.com/go-openapi/swag/conv v0.25.5 // indirect - github.com/go-openapi/swag/fileutils v0.25.5 // indirect - github.com/go-openapi/swag/jsonname v0.25.5 // indirect - github.com/go-openapi/swag/jsonutils v0.25.5 // indirect - github.com/go-openapi/swag/loading v0.25.5 // indirect - github.com/go-openapi/swag/mangling v0.25.5 // indirect - github.com/go-openapi/swag/netutils v0.25.5 // indirect - github.com/go-openapi/swag/stringutils v0.25.5 // indirect - github.com/go-openapi/swag/typeutils v0.25.5 // indirect - github.com/go-openapi/swag/yamlutils v0.25.5 // indirect + github.com/go-openapi/swag/cmdutils v0.26.0 // indirect + github.com/go-openapi/swag/conv v0.26.0 // indirect + github.com/go-openapi/swag/fileutils v0.26.0 // indirect + github.com/go-openapi/swag/jsonname v0.26.0 // indirect + github.com/go-openapi/swag/jsonutils v0.26.0 // indirect + github.com/go-openapi/swag/loading v0.26.0 // indirect + github.com/go-openapi/swag/mangling v0.26.0 // indirect + github.com/go-openapi/swag/netutils v0.26.0 // indirect + github.com/go-openapi/swag/stringutils v0.26.0 // indirect + github.com/go-openapi/swag/typeutils v0.26.0 // indirect + github.com/go-openapi/swag/yamlutils v0.26.0 // indirect github.com/google/btree v1.1.3 // indirect github.com/google/cel-go v0.28.0 // indirect github.com/google/gnostic-models v0.7.1 // indirect @@ -103,7 +103,7 @@ require ( github.com/beorn7/perks v1.0.1 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect - github.com/docker/cli v29.5.3+incompatible // indirect + github.com/docker/cli v29.6.0+incompatible // indirect github.com/docker/docker-credential-helpers v0.9.6 // indirect github.com/docker/go-units v0.5.0 // indirect github.com/emicklei/go-restful/v3 v3.13.0 // indirect @@ -114,7 +114,7 @@ require ( github.com/go-logr/zapr v1.3.0 // indirect github.com/go-openapi/jsonpointer v0.22.5 // indirect github.com/go-openapi/jsonreference v0.21.5 // indirect - github.com/go-openapi/swag v0.25.5 // indirect + github.com/go-openapi/swag v0.26.0 // indirect github.com/gobuffalo/flect v1.0.3 // indirect github.com/google/uuid v1.6.0 github.com/inconshreveable/mousetrap v1.1.0 // indirect @@ -154,9 +154,7 @@ require ( gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 k8s.io/component-base v0.35.3 // indirect - k8s.io/klog/v2 v2.130.1 + k8s.io/klog/v2 v2.140.0 k8s.io/kube-openapi v0.0.0-20260127142750-a19766b6e2d4 sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect ) - -replace github.com/crossplane/cli/v2 => github.com/jcogilvie/cli/v2 v2.0.0-20260626152547-5050d31ad43d diff --git a/go.sum b/go.sum index 95fac17b..bcbbf863 100644 --- a/go.sum +++ b/go.sum @@ -47,6 +47,8 @@ github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSV github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= github.com/creack/pty v1.1.18/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= +github.com/crossplane/cli/v2 v2.5.0-rc.0.0.20260626181113-86f5f7a0c4bf h1:iDRjmH/CbCBNOuLK0Y8rG0zvYRPGPvcnEr0F+I5DJy8= +github.com/crossplane/cli/v2 v2.5.0-rc.0.0.20260626181113-86f5f7a0c4bf/go.mod h1:GWNXbWM0hWwgyPE3LMFwwvtwIAiF8+YkXg56m+w3Ie0= github.com/crossplane/crossplane-runtime/v2 v2.4.0-rc.0 h1:Zgiq+hrh9lbjWtv8ECCLd1A0I9knt3c8ZUELExw6M1w= github.com/crossplane/crossplane-runtime/v2 v2.4.0-rc.0/go.mod h1:PAo3zIfmMzrS18HGyHJLXCeXIp0nFW2Md2Fn9gocMaU= github.com/crossplane/crossplane/apis/v2 v2.4.0-rc.0 h1:4PBahj+tnK9RwSZm1bYGvOkHOU+1CSHjJF2PoPzBMD0= @@ -61,8 +63,8 @@ github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/distribution/reference v0.6.0 h1:0IXCQ5g4/QMHHkarYzh5l+u8T3t73zM5QvfrDyIgxBk= github.com/distribution/reference v0.6.0/go.mod h1:BbU0aIcezP1/5jX/8MP0YiH4SdvB5Y4f/wlDRiLyi3E= -github.com/docker/cli v29.5.3+incompatible h1:nbEFfz774vBwQ5KRYv7c/AghjReqnGISvrRhzjV0evs= -github.com/docker/cli v29.5.3+incompatible/go.mod h1:JLrzqnKDaYBop7H2jaqPtU4hHvMKP+vjCwu2uszcLI8= +github.com/docker/cli v29.6.0+incompatible h1:nw9himxMMZ7eIeherJNlKQq+acnlzGgHd+4uf10QRSc= +github.com/docker/cli v29.6.0+incompatible/go.mod h1:JLrzqnKDaYBop7H2jaqPtU4hHvMKP+vjCwu2uszcLI8= github.com/docker/docker v28.5.2+incompatible h1:DBX0Y0zAjZbSrm1uzOkdr1onVghKaftjlSWt4AFexzM= github.com/docker/docker v28.5.2+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk= github.com/docker/docker-credential-helpers v0.9.6 h1:cT2PbRPSlnMmNTfT2TDMXRyQ1KMWHG7xoTLBcn1ZNv0= @@ -101,36 +103,36 @@ github.com/go-openapi/jsonpointer v0.22.5 h1:8on/0Yp4uTb9f4XvTrM2+1CPrV05QPZXu+r github.com/go-openapi/jsonpointer v0.22.5/go.mod h1:gyUR3sCvGSWchA2sUBJGluYMbe1zazrYWIkWPjjMUY0= github.com/go-openapi/jsonreference v0.21.5 h1:6uCGVXU/aNF13AQNggxfysJ+5ZcU4nEAe+pJyVWRdiE= github.com/go-openapi/jsonreference v0.21.5/go.mod h1:u25Bw85sX4E2jzFodh1FOKMTZLcfifd1Q+iKKOUxExw= -github.com/go-openapi/swag v0.25.5 h1:pNkwbUEeGwMtcgxDr+2GBPAk4kT+kJ+AaB+TMKAg+TU= -github.com/go-openapi/swag v0.25.5/go.mod h1:B3RT6l8q7X803JRxa2e59tHOiZlX1t8viplOcs9CwTA= -github.com/go-openapi/swag/cmdutils v0.25.5 h1:yh5hHrpgsw4NwM9KAEtaDTXILYzdXh/I8Whhx9hKj7c= -github.com/go-openapi/swag/cmdutils v0.25.5/go.mod h1:pdae/AFo6WxLl5L0rq87eRzVPm/XRHM3MoYgRMvG4A0= -github.com/go-openapi/swag/conv v0.25.5 h1:wAXBYEXJjoKwE5+vc9YHhpQOFj2JYBMF2DUi+tGu97g= -github.com/go-openapi/swag/conv v0.25.5/go.mod h1:CuJ1eWvh1c4ORKx7unQnFGyvBbNlRKbnRyAvDvzWA4k= -github.com/go-openapi/swag/fileutils v0.25.5 h1:B6JTdOcs2c0dBIs9HnkyTW+5gC+8NIhVBUwERkFhMWk= -github.com/go-openapi/swag/fileutils v0.25.5/go.mod h1:V3cT9UdMQIaH4WiTrUc9EPtVA4txS0TOmRURmhGF4kc= -github.com/go-openapi/swag/jsonname v0.25.5 h1:8p150i44rv/Drip4vWI3kGi9+4W9TdI3US3uUYSFhSo= -github.com/go-openapi/swag/jsonname v0.25.5/go.mod h1:jNqqikyiAK56uS7n8sLkdaNY/uq6+D2m2LANat09pKU= -github.com/go-openapi/swag/jsonutils v0.25.5 h1:XUZF8awQr75MXeC+/iaw5usY/iM7nXPDwdG3Jbl9vYo= -github.com/go-openapi/swag/jsonutils v0.25.5/go.mod h1:48FXUaz8YsDAA9s5AnaUvAmry1UcLcNVWUjY42XkrN4= -github.com/go-openapi/swag/jsonutils/fixtures_test v0.25.5 h1:SX6sE4FrGb4sEnnxbFL/25yZBb5Hcg1inLeErd86Y1U= -github.com/go-openapi/swag/jsonutils/fixtures_test v0.25.5/go.mod h1:/2KvOTrKWjVA5Xli3DZWdMCZDzz3uV/T7bXwrKWPquo= -github.com/go-openapi/swag/loading v0.25.5 h1:odQ/umlIZ1ZVRteI6ckSrvP6e2w9UTF5qgNdemJHjuU= -github.com/go-openapi/swag/loading v0.25.5/go.mod h1:I8A8RaaQ4DApxhPSWLNYWh9NvmX2YKMoB9nwvv6oW6g= -github.com/go-openapi/swag/mangling v0.25.5 h1:hyrnvbQRS7vKePQPHHDso+k6CGn5ZBs5232UqWZmJZw= -github.com/go-openapi/swag/mangling v0.25.5/go.mod h1:6hadXM/o312N/h98RwByLg088U61TPGiltQn71Iw0NY= -github.com/go-openapi/swag/netutils v0.25.5 h1:LZq2Xc2QI8+7838elRAaPCeqJnHODfSyOa7ZGfxDKlU= -github.com/go-openapi/swag/netutils v0.25.5/go.mod h1:lHbtmj4m57APG/8H7ZcMMSWzNqIQcu0RFiXrPUara14= -github.com/go-openapi/swag/stringutils v0.25.5 h1:NVkoDOA8YBgtAR/zvCx5rhJKtZF3IzXcDdwOsYzrB6M= -github.com/go-openapi/swag/stringutils v0.25.5/go.mod h1:PKK8EZdu4QJq8iezt17HM8RXnLAzY7gW0O1KKarrZII= -github.com/go-openapi/swag/typeutils v0.25.5 h1:EFJ+PCga2HfHGdo8s8VJXEVbeXRCYwzzr9u4rJk7L7E= -github.com/go-openapi/swag/typeutils v0.25.5/go.mod h1:itmFmScAYE1bSD8C4rS0W+0InZUBrB2xSPbWt6DLGuc= -github.com/go-openapi/swag/yamlutils v0.25.5 h1:kASCIS+oIeoc55j28T4o8KwlV2S4ZLPT6G0iq2SSbVQ= -github.com/go-openapi/swag/yamlutils v0.25.5/go.mod h1:Gek1/SjjfbYvM+Iq4QGwa/2lEXde9n2j4a3wI3pNuOQ= -github.com/go-openapi/testify/enable/yaml/v2 v2.4.0 h1:7SgOMTvJkM8yWrQlU8Jm18VeDPuAvB/xWrdxFJkoFag= -github.com/go-openapi/testify/enable/yaml/v2 v2.4.0/go.mod h1:14iV8jyyQlinc9StD7w1xVPW3CO3q1Gj04Jy//Kw4VM= -github.com/go-openapi/testify/v2 v2.4.0 h1:8nsPrHVCWkQ4p8h1EsRVymA2XABB4OT40gcvAu+voFM= -github.com/go-openapi/testify/v2 v2.4.0/go.mod h1:HCPmvFFnheKK2BuwSA0TbbdxJ3I16pjwMkYkP4Ywn54= +github.com/go-openapi/swag v0.26.0 h1:GVDXCmfvhfu1BxiHo8/FA+BbKmhecHnG3varjON5/RI= +github.com/go-openapi/swag v0.26.0/go.mod h1:82g3193sZJRbocs7bNCqGfIgq8pkuwVwCfhKIRlEQF0= +github.com/go-openapi/swag/cmdutils v0.26.0 h1:iowihOcvq7y4egO8cOq0dmfohz6wfeQ63U1EnuhO2TU= +github.com/go-openapi/swag/cmdutils v0.26.0/go.mod h1:Sm1MVFMkF6guJJ+pQqHnQA3N0j9qALV3NxzDSv6bETM= +github.com/go-openapi/swag/conv v0.26.0 h1:5yGGsPYI1ZCva93U0AoKi/iZrNhaJEjr324YVsiD89I= +github.com/go-openapi/swag/conv v0.26.0/go.mod h1:tpAmIL7X58VPnHHiSO4uE3jBeRamGsFsfdDeDtb5ECE= +github.com/go-openapi/swag/fileutils v0.26.0 h1:WJoPRvsA7QRiiWluowkLJa9jaYR7FCuxmDvnCgaRRxU= +github.com/go-openapi/swag/fileutils v0.26.0/go.mod h1:0WDJ7lp67eNjPMO50wAWYlKvhOb6CQ37rzR7wrgI8Tc= +github.com/go-openapi/swag/jsonname v0.26.0 h1:gV1NFX9M8avo0YSpmWogqfQISigCmpaiNci8cGECU5w= +github.com/go-openapi/swag/jsonname v0.26.0/go.mod h1:urBBR8bZNoDYGr653ynhIx+gTeIz0ARZxHkAPktJK2M= +github.com/go-openapi/swag/jsonutils v0.26.0 h1:FawFML2iAXsPqmERscuMPIHmFsoP1tOqWkxBaKNMsnA= +github.com/go-openapi/swag/jsonutils v0.26.0/go.mod h1:2VmA0CJlyFqgawOaPI9psnjFDqzyivIqLYN34t9p91E= +github.com/go-openapi/swag/jsonutils/fixtures_test v0.26.0 h1:apqeINu/ICHouqiRZbyFvuDge5jCmmLTqGQ9V95EaOM= +github.com/go-openapi/swag/jsonutils/fixtures_test v0.26.0/go.mod h1:AyM6QT8uz5IdKxk5akv0y6u4QvcL9GWERt0Jx/F/R8Y= +github.com/go-openapi/swag/loading v0.26.0 h1:Apg6zaKhCJurpJer0DCxq99qwmhFddBhaMX7kilDcko= +github.com/go-openapi/swag/loading v0.26.0/go.mod h1:dBxQ/6V2uBaAQdevN18VELE6xSpJWZxLX4txe12JwDg= +github.com/go-openapi/swag/mangling v0.26.0 h1:Du2YC4YLA/Y5m/YKQd7AnY5qq0wRKSFZTTt8ktFaXcQ= +github.com/go-openapi/swag/mangling v0.26.0/go.mod h1:jifS7W9vbg+pw63bT+GI53otluMQL3CeemuyCHKwVx0= +github.com/go-openapi/swag/netutils v0.26.0 h1:CmZp+ZT7HrmFwrC3GdGsXBq2+42T1bjKBapcqVpIs3c= +github.com/go-openapi/swag/netutils v0.26.0/go.mod h1:5iK+Ok3ZohWWex1C50BFTPexi03UaPwjW4Oj8kgrpwo= +github.com/go-openapi/swag/stringutils v0.26.0 h1:qZQngLxs5s7SLijc3N2ZO+fUq2o8LjuWAASSrJuh+xg= +github.com/go-openapi/swag/stringutils v0.26.0/go.mod h1:sWn5uY+QIIspwPhvgnqJsH8xqFT2ZbYcvbcFanRyhFE= +github.com/go-openapi/swag/typeutils v0.26.0 h1:2kdEwdiNWy+JJdOvu5MA2IIg2SylWAFuuyQIKYybfq4= +github.com/go-openapi/swag/typeutils v0.26.0/go.mod h1:oovDuIUvTrEHVMqWilQzKzV4YlSKgyZmFh7AlfABNVE= +github.com/go-openapi/swag/yamlutils v0.26.0 h1:H7O8l/8NJJQ/oiReEN+oMpnGMyt8G0hl460nRZxhLMQ= +github.com/go-openapi/swag/yamlutils v0.26.0/go.mod h1:1evKEGAtP37Pkwcc7EWMF0hedX0/x3Rkvei2wtG/TbU= +github.com/go-openapi/testify/enable/yaml/v2 v2.4.2 h1:5zRca5jw7lzVREKCZVNBpysDNBjj74rBh0N2BGQbSR0= +github.com/go-openapi/testify/enable/yaml/v2 v2.4.2/go.mod h1:XVevPw5hUXuV+5AkI1u1PeAm27EQVrhXTTCPAF85LmE= +github.com/go-openapi/testify/v2 v2.4.2 h1:tiByHpvE9uHrrKjOszax7ZvKB7QOgizBWGBLuq0ePx4= +github.com/go-openapi/testify/v2 v2.4.2/go.mod h1:SgsVHtfooshd0tublTtJ50FPKhujf47YRqauXXOUxfw= github.com/go-task/slim-sprig/v3 v3.0.0 h1:sUs3vkvUymDpBKi3qH1YSqBQk9+9D/8M2mN1vB6EwHI= github.com/go-task/slim-sprig/v3 v3.0.0/go.mod h1:W848ghGpv3Qj3dhTPRyJypKRiqCdHZiAzKg9hl15HA8= github.com/gobuffalo/flect v1.0.3 h1:xeWBM2nui+qnVvNM4S3foBhCAL2XgPU+a7FdpelbTq4= @@ -167,8 +169,6 @@ github.com/hexops/gotextdiff v1.0.3 h1:gitA9+qJrrTCsiCl7+kh75nPqQt1cx4ZkudSTLoUq github.com/hexops/gotextdiff v1.0.3/go.mod h1:pSWU5MAI3yDq+fZBTazCSJysOMbxWL1BSow5/V2vxeg= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= -github.com/jcogilvie/cli/v2 v2.0.0-20260626152547-5050d31ad43d h1:/OavKQIx/+x2AuwAXkjY6JTI6PpUm+MWitWeK1nYmCE= -github.com/jcogilvie/cli/v2 v2.0.0-20260626152547-5050d31ad43d/go.mod h1:myc+idRY+CHQZsLZE1a1CAFcn8fpL6EVvxwpF9RrrBM= github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM= github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= github.com/klauspost/compress v1.18.6 h1:2jupLlAwFm95+YDR+NwD2MEfFO9d4z4Prjl1XXDjuao= @@ -414,8 +414,8 @@ k8s.io/component-base v0.35.3 h1:mbKbzoIMy7JDWS/wqZobYW1JDVRn/RKRaoMQHP9c4P0= k8s.io/component-base v0.35.3/go.mod h1:IZ8LEG30kPN4Et5NeC7vjNv5aU73ku5MS15iZyvyMYk= k8s.io/gengo/v2 v2.0.0-20251215205346-5ee0d033ba5b h1:0YkdvW3rX2vaBWsqCGZAekxPRwaI5NuYNprOsMNVLns= k8s.io/gengo/v2 v2.0.0-20251215205346-5ee0d033ba5b/go.mod h1:yvyl3l9E+UxlqOMUULdKTAYB0rEhsmjr7+2Vb/1pCSo= -k8s.io/klog/v2 v2.130.1 h1:n9Xl7H1Xvksem4KFG4PYbdQCQxqc/tTUyrgXaOhHSzk= -k8s.io/klog/v2 v2.130.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE= +k8s.io/klog/v2 v2.140.0 h1:Tf+J3AH7xnUzZyVVXhTgGhEKnFqye14aadWv7bzXdzc= +k8s.io/klog/v2 v2.140.0/go.mod h1:o+/RWfJ6PwpnFn7OyAG3QnO47BFsymfEfrz6XyYSSp0= k8s.io/kube-openapi v0.0.0-20260127142750-a19766b6e2d4 h1:HhDfevmPS+OalTjQRKbTHppRIz01AWi8s45TMXStgYY= k8s.io/kube-openapi v0.0.0-20260127142750-a19766b6e2d4/go.mod h1:kdmbQkyfwUagLfXIad1y2TdrjPFWp2Q89B3qkRwf/pQ= k8s.io/kubectl v0.35.3 h1:1KqSYXk/sodU7VeDvK6atX2kAGUZd2QTeR5K7Hb9r9w= From 88e3122101d73a004604e3752395b1f5039091ba Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 26 Jun 2026 14:28:16 -0400 Subject: [PATCH 3/6] chore(lint): apply earthly +reviewable autofixes - render_engine.go: rewrite Cleanup's reverse loop as slices.Backward (Go 1.23+ idiom, suggested by intrange/copyloopvar lint). - function_provider_test.go: regroup the render import added in the workaround unwind alongside the other third-party imports. Signed-off-by: Jonathan Ogilvie --- cmd/diff/diffprocessor/function_provider_test.go | 3 +-- cmd/diff/diffprocessor/render_engine.go | 5 +++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/diff/diffprocessor/function_provider_test.go b/cmd/diff/diffprocessor/function_provider_test.go index 611675eb..0ebeb31f 100644 --- a/cmd/diff/diffprocessor/function_provider_test.go +++ b/cmd/diff/diffprocessor/function_provider_test.go @@ -21,9 +21,8 @@ import ( "testing" tu "github.com/crossplane-contrib/crossplane-diff/cmd/diff/testutils" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/crossplane/cli/v2/cmd/crossplane/render" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apiextensionsv1 "github.com/crossplane/crossplane/apis/v2/apiextensions/v1" pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" diff --git a/cmd/diff/diffprocessor/render_engine.go b/cmd/diff/diffprocessor/render_engine.go index c2a4470d..ce11c563 100644 --- a/cmd/diff/diffprocessor/render_engine.go +++ b/cmd/diff/diffprocessor/render_engine.go @@ -19,6 +19,7 @@ package diffprocessor import ( "context" "maps" + "slices" "sync" "github.com/crossplane/cli/v2/cmd/crossplane/render" @@ -309,8 +310,8 @@ func (e *EngineRenderFn) Cleanup(_ context.Context) error { // Run cleanups in LIFO order so the first Setup's real cleanup (which // owns the docker network) runs last, after any later no-op cleanups. - for i := len(e.cleanups) - 1; i >= 0; i-- { - e.cleanups[i]() + for _, v := range slices.Backward(e.cleanups) { + v() } e.cleanups = nil From 7cdd6e5a956ab8cc004f7759db4535807173c920 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 26 Jun 2026 15:18:54 -0400 Subject: [PATCH 4/6] refactor(render): route docker network env var through upstream EngineFlags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that we no longer manage the docker network ourselves, the CROSSPLANE_DIFF_DOCKER_NETWORK env var doesn't need its own stamping pass over fetched functions. NewEngineRenderFn reads it once and threads the value through render.EngineFlags.CrossplaneDockerNetwork (crossplane/cli#65). The docker engine then: - runs the crossplane-render container on that network (which fixes the other half of crossplane/cli#75 — our prior implementation only placed function containers on the network, leaving the render container unreachable from inside-a-container deployments); - annotates each fn at Setup time so its container joins it too, honouring the don't-overwrite contract for fns the caller has already pinned. What's deleted: - applyDockerNetworkAnnotation helper. - The DefaultFunctionProvider/CachedFunctionProvider calls into it. - The CachedFunctionProvider cache-hit re-stamp dance and its accompanying TestCachedFunctionProvider_DockerNetworkAnnotation_CacheHit test (cache entries no longer need to track env state). - TestApplyDockerNetworkAnnotation (the helper's gone; engine.Setup owns this behaviour now and has its own coverage upstream). Also tightens the EngineRenderFn / Render docstrings and the startRuntimes-error rollback comment to match the merged upstream Engine.Setup contract (cli#159 + follow-up commit ad8307749). The new phrasing acknowledges that no Setup call may ever return a real cleanup (--crossplane-docker-network preset, or local engine), and frames the rollback as "if this call created the environment" rather than "if this was the very first call". Signed-off-by: Jonathan Ogilvie --- cmd/diff/diffprocessor/function_provider.go | 45 +----- .../diffprocessor/function_provider_test.go | 142 ------------------ cmd/diff/diffprocessor/render_engine.go | 61 +++++--- 3 files changed, 44 insertions(+), 204 deletions(-) diff --git a/cmd/diff/diffprocessor/function_provider.go b/cmd/diff/diffprocessor/function_provider.go index b6796b8c..d521c233 100644 --- a/cmd/diff/diffprocessor/function_provider.go +++ b/cmd/diff/diffprocessor/function_provider.go @@ -22,12 +22,10 @@ import ( "crypto/sha256" "encoding/hex" "fmt" - "os" "strings" "time" xp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/crossplane" - "github.com/crossplane/cli/v2/cmd/crossplane/render" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/client" @@ -51,34 +49,14 @@ type FunctionProvider interface { } // EnvDockerNetwork is the environment variable that specifies which Docker -// network function containers should join. This is needed when crossplane-diff -// runs inside a Docker container (e.g. a GitHub Actions container job) so that -// function containers are on the same network and reachable via container IP. +// network the crossplane-render container and function containers should +// join. NewEngineRenderFn reads this once and routes the value through +// render.EngineFlags.CrossplaneDockerNetwork; the upstream docker engine +// then both runs the render container on that network and annotates fns +// to join it at Setup time. This is needed when crossplane-diff runs +// inside a Docker container (e.g. a GitHub Actions container job). const EnvDockerNetwork = "CROSSPLANE_DIFF_DOCKER_NETWORK" -// applyDockerNetworkAnnotation stamps each function with the Docker network -// annotation drawn from EnvDockerNetwork when that variable is set. Functions -// that already carry a non-empty value for the annotation are left untouched, -// so this never overwrites an explicitly configured network. -func applyDockerNetworkAnnotation(fns []pkgv1.Function, log logging.Logger) { - network := os.Getenv(EnvDockerNetwork) - if network == "" { - return - } - - log.Debug("Setting Docker network annotation on functions", "network", network) - - for i := range fns { - if fns[i].Annotations == nil { - fns[i].Annotations = make(map[string]string) - } - - if fns[i].Annotations[render.AnnotationKeyRuntimeDockerNetwork] == "" { - fns[i].Annotations[render.AnnotationKeyRuntimeDockerNetwork] = network - } - } -} - // DefaultFunctionProvider fetches functions from the cluster on each call. // This is appropriate for the xr command where each XR is processed independently. type DefaultFunctionProvider struct { @@ -105,8 +83,6 @@ func (p *DefaultFunctionProvider) GetFunctionsForComposition(comp *apiextensions p.logger.Debug("Fetched functions from pipeline", "composition", comp.GetName(), "count", len(fns)) - applyDockerNetworkAnnotation(fns, p.logger) - return fns, nil } @@ -154,15 +130,8 @@ func generateInstanceID() string { func (p *CachedFunctionProvider) GetFunctionsForComposition(comp *apiextensionsv1.Composition) ([]pkgv1.Function, error) { compName := comp.GetName() - // Check cache first. Re-apply the Docker network annotation on every cache - // hit so an entry first cached when EnvDockerNetwork was unset still gets - // stamped on a later call once the env var is set. applyNetworkAnnotation - // preserves any non-empty value already present, so this is safe to run - // unconditionally. if cached, ok := p.cache[compName]; ok { p.logger.Debug("Using cached functions", "composition", compName, "count", len(cached)) - applyDockerNetworkAnnotation(cached, p.logger) - return cached, nil } @@ -204,8 +173,6 @@ func (p *CachedFunctionProvider) GetFunctionsForComposition(comp *apiextensionsv p.containerNames = append(p.containerNames, containerName) } - applyDockerNetworkAnnotation(fns, p.logger) - // Cache for future calls p.cache[compName] = fns diff --git a/cmd/diff/diffprocessor/function_provider_test.go b/cmd/diff/diffprocessor/function_provider_test.go index 0ebeb31f..bc142960 100644 --- a/cmd/diff/diffprocessor/function_provider_test.go +++ b/cmd/diff/diffprocessor/function_provider_test.go @@ -21,7 +21,6 @@ import ( "testing" tu "github.com/crossplane-contrib/crossplane-diff/cmd/diff/testutils" - "github.com/crossplane/cli/v2/cmd/crossplane/render" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apiextensionsv1 "github.com/crossplane/crossplane/apis/v2/apiextensions/v1" @@ -243,56 +242,6 @@ func TestCachedFunctionProvider_GetFunctionsForComposition_CacheHit(t *testing.T } } -// TestCachedFunctionProvider_DockerNetworkAnnotation_CacheHit verifies that -// the Docker network annotation reflects the *current* value of -// CROSSPLANE_DIFF_DOCKER_NETWORK on every call, including when a cached entry -// is returned. Without this guarantee, a cache populated before the env var -// was set would keep returning unannotated functions for the rest of the -// process, defeating the feature. -func TestCachedFunctionProvider_DockerNetworkAnnotation_CacheHit(t *testing.T) { - functions := []pkgv1.Function{ - {ObjectMeta: metav1.ObjectMeta{Name: "function-test"}}, - } - - fnClient := tu.NewMockFunctionClient(). - WithFunctionsFetchCallback(func() ([]pkgv1.Function, error) { - return functions, nil - }). - Build() - - logger := tu.TestLogger(t, false) - provider := NewCachedFunctionProvider(fnClient, logger) - - comp := &apiextensionsv1.Composition{ - ObjectMeta: metav1.ObjectMeta{Name: "test-composition"}, - } - - // Prime the cache with the env var unset so the cached entry has no - // network annotation written during the miss path. - t.Setenv(EnvDockerNetwork, "") - - if _, err := provider.GetFunctionsForComposition(comp); err != nil { - t.Fatalf("priming GetFunctionsForComposition() error = %v", err) - } - - // Now set the env var and request the same composition again. The - // returned (cached) functions must carry the annotation. - t.Setenv(EnvDockerNetwork, "ci-network") - - fns, err := provider.GetFunctionsForComposition(comp) - if err != nil { - t.Fatalf("cache-hit GetFunctionsForComposition() error = %v", err) - } - - if len(fns) != 1 { - t.Fatalf("got %d functions, want 1", len(fns)) - } - - if got := fns[0].Annotations[render.AnnotationKeyRuntimeDockerNetwork]; got != "ci-network" { - t.Errorf("cache-hit network annotation = %q, want %q", got, "ci-network") - } -} - func TestCachedFunctionProvider_GetFunctionsForComposition_Error(t *testing.T) { fnClient := tu.NewMockFunctionClient(). WithFailedFunctionsFetch("fetch error"). @@ -722,94 +671,3 @@ func TestGenerateContainerName(t *testing.T) { }) } } - -func TestApplyDockerNetworkAnnotation(t *testing.T) { - tests := map[string]struct { - envValue string - fns []pkgv1.Function - wantNetwork string - checkExistingPreserved bool - }{ - "EnvSet": { - envValue: "github_network_abc123", - fns: []pkgv1.Function{ - {ObjectMeta: metav1.ObjectMeta{Name: "function-1"}}, - {ObjectMeta: metav1.ObjectMeta{Name: "function-2"}}, - }, - wantNetwork: "github_network_abc123", - }, - "EnvNotSet": { - envValue: "", - fns: []pkgv1.Function{ - {ObjectMeta: metav1.ObjectMeta{Name: "function-1"}}, - }, - wantNetwork: "", - }, - "PreservesPreSetNetwork": { - // When a function already carries a runtime-docker-network - // annotation (e.g. set by the render engine's Setup pass or by a - // caller), the env var must not clobber it. See applyNetworkAnnotation - // in render_engine.go. - envValue: "env-network", - fns: []pkgv1.Function{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "function-1", - Annotations: map[string]string{ - render.AnnotationKeyRuntimeDockerNetwork: "pre-set-network", - }, - }, - }, - }, - wantNetwork: "pre-set-network", - }, - "ExistingAnnotations": { - envValue: "my-network", - fns: []pkgv1.Function{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "function-1", - Annotations: map[string]string{ - "existing-key": "existing-value", - }, - }, - }, - }, - wantNetwork: "my-network", - checkExistingPreserved: true, - }, - } - - for name, tt := range tests { - t.Run(name, func(t *testing.T) { - // t.Setenv (even with an empty value) snapshots the prior value - // and restores it on subtest cleanup, avoiding leaks into other - // tests in this package or into the developer's shell. - t.Setenv(EnvDockerNetwork, tt.envValue) - - logger := tu.TestLogger(t, false) - applyDockerNetworkAnnotation(tt.fns, logger) - - for _, fn := range tt.fns { - got := fn.Annotations[render.AnnotationKeyRuntimeDockerNetwork] - if got != tt.wantNetwork { - t.Errorf("function %q: network annotation = %q, want %q", fn.Name, got, tt.wantNetwork) - } - } - - if tt.checkExistingPreserved { - for _, fn := range tt.fns { - v, ok := fn.Annotations["existing-key"] - if !ok { - t.Errorf("function %q: existing annotation %q was removed", fn.Name, "existing-key") - continue - } - - if v != "existing-value" { - t.Errorf("function %q: existing annotation = %q, want %q", fn.Name, v, "existing-value") - } - } - } - }) - } -} diff --git a/cmd/diff/diffprocessor/render_engine.go b/cmd/diff/diffprocessor/render_engine.go index ce11c563..563c1ea2 100644 --- a/cmd/diff/diffprocessor/render_engine.go +++ b/cmd/diff/diffprocessor/render_engine.go @@ -19,6 +19,7 @@ package diffprocessor import ( "context" "maps" + "os" "slices" "sync" @@ -65,17 +66,21 @@ type RenderInputs struct { XRD *kunstructured.Unstructured } -// 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. +// EngineRenderFn is the default RenderFn implementation. It lazily starts +// function runtimes on first use, reuses them across subsequent calls, and +// serializes concurrent renders with an internal mutex. // // A single `xr` invocation can render against XRs from multiple compositions -// whose function pipelines overlap but aren't identical. Upstream's -// dockerRenderEngine.Setup is idempotent (crossplane/cli#159): the first call -// creates the docker network and annotates the supplied fns; later calls -// annotate any newly-arrived fns with the same network and return a no-op -// cleanup. We accumulate every cleanup and call them in LIFO order on -// Cleanup, alongside stopping every FunctionAddresses we ever started. +// whose function pipelines overlap but aren't identical. Engine.Setup +// (crossplane/cli#159) accepts being called once per batch: the call that +// creates a new environment — if any — 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 via --crossplane-docker-network) return a +// no-op cleanup, as do calls on engines with nothing to clean up (the local +// engine). We accumulate every returned cleanup in a slice and call them in +// LIFO order on Cleanup without coordinating which one is real, per the +// caller contract in the upstream Engine.Setup docstring. type EngineRenderFn struct { engine render.Engine cleanups []func() @@ -106,9 +111,19 @@ type EngineRenderFn struct { // fatal — crossplane/crossplane#7455) per crossplane/cli#91, so we no longer // wrap them. Our EngineRenderFn.Render still expects (rsp != nil, err != nil) // on pipeline fatal and surfaces both — see the comment there. +// +// CROSSPLANE_DIFF_DOCKER_NETWORK is read once here and threaded through +// render.EngineFlags.CrossplaneDockerNetwork (crossplane/cli#65). The docker +// engine then runs the crossplane-render container on that network AND +// annotates each fn at Setup time so its container joins it too — closing +// both halves of the "crossplane-diff inside a container" case +// (crossplane/cli#75). For the local engine the flag is a no-op. func NewEngineRenderFn(log logging.Logger, binaryPath string) *EngineRenderFn { return &EngineRenderFn{ - engine: render.NewEngineFromFlags(&render.EngineFlags{CrossplaneBinary: binaryPath}, log), + engine: render.NewEngineFromFlags(&render.EngineFlags{ + CrossplaneBinary: binaryPath, + CrossplaneDockerNetwork: os.Getenv(EnvDockerNetwork), + }, log), log: log, startRuntimes: render.StartFunctionRuntimes, stopRuntimes: render.StopFunctionRuntimes, @@ -116,9 +131,8 @@ func NewEngineRenderFn(log logging.Logger, binaryPath string) *EngineRenderFn { } // Render performs one render. It is safe for concurrent use — calls are -// serialized internally. Setup runs on every invocation; upstream guarantees -// it is idempotent (the first call creates the docker network, later calls -// just annotate any newly-arrived functions with it). +// serialized internally. Setup runs on every invocation; upstream's Engine +// contract makes that safe across batches (see the EngineRenderFn docstring). func (e *EngineRenderFn) Render(ctx context.Context, log logging.Logger, in RenderInputs) (render.CompositionOutputs, error) { e.mu.Lock() defer e.mu.Unlock() @@ -143,11 +157,10 @@ func (e *EngineRenderFn) Render(ctx context.Context, log logging.Logger, in Rend newFns = append(newFns, in.Functions[i]) } - // Setup is idempotent (crossplane/cli#159): the first call creates the - // docker network and returns the real cleanup; subsequent calls only - // annotate newFns to join the existing network and return a no-op - // cleanup. Accumulating every returned cleanup and calling them in LIFO - // order on Cleanup is the recommended caller shape. + // Setup integrates newFns into the engine's environment. Whether this + // call creates the environment or only adds to one that already exists + // is the engine's concern; we just hold onto whatever cleanup it gives + // back and let Cleanup walk them LIFO. cleanup, err := e.engine.Setup(ctx, newFns) if err != nil { return render.CompositionOutputs{}, errors.Wrap(err, "cannot setup render engine") @@ -156,9 +169,9 @@ func (e *EngineRenderFn) Render(ctx context.Context, log logging.Logger, in Rend if len(newFns) > 0 { fa, startErr := e.startRuntimes(ctx, log, newFns) if startErr != nil { - // Roll back this Setup. If it was the very first call, this - // releases the docker network it just created; if it was a - // subsequent call, the cleanup is a no-op and no harm is done. + // Roll back this Setup. If this call created the environment, + // cleanup releases it; otherwise cleanup is a no-op and no harm + // is done. cleanup() return render.CompositionOutputs{}, errors.Wrap(startErr, "cannot start function runtimes") } @@ -308,8 +321,10 @@ func (e *EngineRenderFn) Cleanup(_ context.Context) error { e.addrs = nil e.startedNames = nil - // Run cleanups in LIFO order so the first Setup's real cleanup (which - // owns the docker network) runs last, after any later no-op cleanups. + // Run cleanups in LIFO order. Per the upstream Engine.Setup contract, + // at most one cleanup is real (the one that created the environment, + // if any); the rest are no-ops. LIFO matches the natural deferred- + // cleanup shape and is correct even if a future engine wants ordering. for _, v := range slices.Backward(e.cleanups) { v() } From 67b60a17c493f90d19ea1f89fa7c742105e57598 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 26 Jun 2026 15:27:56 -0400 Subject: [PATCH 5/6] refactor(render): only call engine.Setup when there are new fns to integrate Per Copilot review feedback on PR #359: we were calling engine.Setup and appending its returned cleanup to e.cleanups on every Render, even when newFns was empty (all fns already started by a prior render). The slice would grow without bound across long-running engines and the Setup call itself was wasted work. Gate the whole Setup+startRuntimes+bookkeeping block on len(newFns) > 0. Renders where every fn is already running now skip straight to building the render request, leaving e.cleanups bounded by the number of distinct fn batches the engine has actually integrated. No behavioural change for the renders that DO integrate new fns: Setup, startRuntimes, the rollback-on-startRuntimes-error path, and the LIFO cleanup ordering all work the same. Signed-off-by: Jonathan Ogilvie --- cmd/diff/diffprocessor/render_engine.go | 29 ++++++++++++++----------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/cmd/diff/diffprocessor/render_engine.go b/cmd/diff/diffprocessor/render_engine.go index 563c1ea2..e2d43332 100644 --- a/cmd/diff/diffprocessor/render_engine.go +++ b/cmd/diff/diffprocessor/render_engine.go @@ -157,21 +157,25 @@ func (e *EngineRenderFn) Render(ctx context.Context, log logging.Logger, in Rend newFns = append(newFns, in.Functions[i]) } - // Setup integrates newFns into the engine's environment. Whether this - // call creates the environment or only adds to one that already exists - // is the engine's concern; we just hold onto whatever cleanup it gives - // back and let Cleanup walk them LIFO. - cleanup, err := e.engine.Setup(ctx, newFns) - if err != nil { - return render.CompositionOutputs{}, errors.Wrap(err, "cannot setup render engine") - } - + // Only call Setup when there's actually a new batch to integrate. + // Renders where all fns are already running (newFns is empty) have no + // work for the engine and would just accumulate no-op cleanups in the + // slice for the lifetime of the engine. if len(newFns) > 0 { + // Setup integrates newFns into the engine's environment. Whether + // this call creates the environment or only adds to one that + // already exists is the engine's concern; we just hold onto + // whatever cleanup it gives back and let Cleanup walk them LIFO. + cleanup, err := e.engine.Setup(ctx, newFns) + if err != nil { + return render.CompositionOutputs{}, errors.Wrap(err, "cannot setup render engine") + } + fa, startErr := e.startRuntimes(ctx, log, newFns) if startErr != nil { // Roll back this Setup. If this call created the environment, - // cleanup releases it; otherwise cleanup is a no-op and no harm - // is done. + // cleanup releases it; otherwise cleanup is a no-op and no + // harm is done. cleanup() return render.CompositionOutputs{}, errors.Wrap(startErr, "cannot start function runtimes") } @@ -182,10 +186,9 @@ func (e *EngineRenderFn) Render(ctx context.Context, log logging.Logger, in Rend } maps.Copy(e.addrs, fa.Addresses()) + e.cleanups = append(e.cleanups, cleanup) } - e.cleanups = append(e.cleanups, cleanup) - // Build request with addresses for in.Functions only — the binary needs // addresses for this render's pipeline, not for every function we've // ever started. From 544f451cba1ccf32f42b3d02ea4f79ced9c455cb Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 26 Jun 2026 15:43:46 -0400 Subject: [PATCH 6/6] docs(render): align Render/Cleanup doc comments with current behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Copilot review comments on PR #359, both valid drift introduced by the prior commit that gated Setup on len(newFns) > 0: - Render's doc said "Setup runs on every invocation" — it doesn't anymore. Rewrite to "Setup runs only on invocations that introduce previously-unseen functions; renders whose fns are all already running skip straight to building the request." - Cleanup's doc said it "releases the docker network" — that's only one possible effect now. The cleanups slice is engine-agnostic, so rewrite to "runs the cleanups accumulated from each engine.Setup call in LIFO order. The effect of those cleanups is engine-specific — docker creates and releases a network; local and pre-configured docker accumulate only no-op cleanups." No code change. Signed-off-by: Jonathan Ogilvie --- cmd/diff/diffprocessor/render_engine.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cmd/diff/diffprocessor/render_engine.go b/cmd/diff/diffprocessor/render_engine.go index e2d43332..49666870 100644 --- a/cmd/diff/diffprocessor/render_engine.go +++ b/cmd/diff/diffprocessor/render_engine.go @@ -131,8 +131,10 @@ func NewEngineRenderFn(log logging.Logger, binaryPath string) *EngineRenderFn { } // Render performs one render. It is safe for concurrent use — calls are -// serialized internally. Setup runs on every invocation; upstream's Engine -// contract makes that safe across batches (see the EngineRenderFn docstring). +// serialized internally. Setup runs only on invocations that introduce +// previously-unseen functions; renders whose fns are all already running +// skip straight to building the request. See the EngineRenderFn docstring +// for the per-batch Setup contract this relies on. func (e *EngineRenderFn) Render(ctx context.Context, log logging.Logger, in RenderInputs) (render.CompositionOutputs, error) { e.mu.Lock() defer e.mu.Unlock() @@ -310,8 +312,12 @@ func alignObservedOwnerRefs(xr *ucomposite.Unstructured, observed []composed.Uns } // Cleanup stops every function runtime started across the engine's lifetime -// and releases the docker network. Idempotent and safe to call when Render -// was never invoked. +// and runs the cleanups accumulated from each engine.Setup call in LIFO +// order. The effect of those cleanups is engine-specific — for the docker +// engine the one real cleanup releases the docker network it created; +// other engines (local, or docker pre-configured with an externally-managed +// network) accumulate only no-op cleanups. Idempotent and safe to call when +// Render was never invoked. func (e *EngineRenderFn) Cleanup(_ context.Context) error { e.mu.Lock() defer e.mu.Unlock()