Skip to content

refactor(render): unwind multi-composition workaround using cli#159 #359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jcogilvie merged 6 commits into from
Jun 26, 2026
+127 −329
Merged

refactor(render): unwind multi-composition workaround using cli#159 #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d841601
refactor(render): unwind multi-composition workaround using cli#159
jcogilvie Jun 26, 2026
dbc653f
build(deps): repin crossplane/cli/v2 to upstream main post-merge of c…
jcogilvie Jun 26, 2026
88e3122
chore(lint): apply earthly +reviewable autofixes
jcogilvie Jun 26, 2026
7cdd6e5
refactor(render): route docker network env var through upstream Engin…
jcogilvie Jun 26, 2026
67b60a1
refactor(render): only call engine.Setup when there are new fns to in…
jcogilvie Jun 26, 2026
544f451
docs(render): align Render/Cleanup doc comments with current behaviour
jcogilvie Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 6 additions & 30 deletions cmd/diff/diffprocessor/function_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"crypto/sha256"
"encoding/hex"
"fmt"
"os"
"strings"
"time"

Expand Down Expand Up @@ -50,26 +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. The
// underlying applyNetworkAnnotation helper preserves any non-empty value the
// caller has already set, so this function 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)
applyNetworkAnnotation(fns, 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 {
Expand All @@ -96,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
}

Expand Down Expand Up @@ -145,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
}

Expand Down Expand Up @@ -195,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

Expand Down
143 changes: 0 additions & 143 deletions cmd/diff/diffprocessor/function_provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import (
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"

apiextensionsv1 "github.com/crossplane/crossplane/apis/v2/apiextensions/v1"
pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1"
)
Expand Down Expand Up @@ -244,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").
Expand Down Expand Up @@ -723,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")
}
}
}
})
}
}
Loading
Loading