fix(render): stop every function runtime gracefully and always remove Remove-policy containers

cmd/crossplane/render/engine_docker.go

@@ -63,6 +63,13 @@ type dockerRenderEngine struct {
 	// (exit-3 partial output, *docker.ContainerExitError vs non-exit errors)
 	// without a real Docker daemon.
 	runner containerRunner
+
+	// networks creates and removes the temporary Docker network Setup owns.
+	// Production callers leave it nil and Setup builds a real client from the
+	// environment only when it needs to create a network. Tests substitute a
+	// fake to exercise the create-network branch without a real Docker
+	// daemon.
+	networks networkClient
 }
 
 func (e *dockerRenderEngine) CheckContextSupport() error {
@@ -95,7 +102,16 @@ func (e *dockerRenderEngine) Setup(ctx context.Context, fns []pkgv1.Function) (f
 		return func() {}, nil
 	}
 
-	networkID, networkName, err := createRenderNetwork(ctx)
+	cli := e.networks
+	if cli == nil {
+		c, err := newNetworkClient()
+		if err != nil {
+			return func() {}, errors.Wrap(err, "cannot create Docker network for rendering")
+		}
+		cli = c
+	}
+
+	networkID, networkName, err := createRenderNetwork(ctx, cli)
 	if err != nil {
 		return func() {}, errors.Wrap(err, "cannot create Docker network for rendering")
 	}
@@ -104,7 +120,7 @@ func (e *dockerRenderEngine) Setup(ctx context.Context, fns []pkgv1.Function) (f
 	injectNetworkAnnotation(fns, networkName)
 
 	cleanup := func() { //nolint:contextcheck // Detached context for cleanup.
-		_ = removeRenderNetwork(context.Background(), networkID)
+		_ = removeRenderNetwork(context.Background(), cli, networkID)
 	}
 
 	return cleanup, nil

cmd/crossplane/render/engine_docker_test.go

@@ -23,10 +23,13 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/moby/moby/client"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/testing/protocmp"
 	"google.golang.org/protobuf/types/known/structpb"
 
+	"github.com/crossplane/crossplane-runtime/v2/pkg/errors"
 	"github.com/crossplane/crossplane-runtime/v2/pkg/logging"
 
 	pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1"
@@ -222,9 +225,8 @@ func TestDockerRenderEngineSetup(t *testing.T) {
 	// 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.
+	// cleanup. The create-new-network branch is covered separately by
+	// TestDockerRenderEngineSetupCreatesNetwork.
 	//
 	// The MultiBatchAnnotatesAdditionalFunctions case simulates the
 	// in-process multi-composition use case from crossplane/cli#96: a
@@ -346,6 +348,95 @@ func TestDockerRenderEngineSetup(t *testing.T) {
 	}
 }
 
+func TestDockerRenderEngineSetupCreatesNetwork(t *testing.T) {
+	// When e.network is unset, Setup must create a temporary network through
+	// the engine's network client, record its name, annotate the supplied
+	// functions to join it, and return a cleanup that removes it through the
+	// same client.
+	errBoom := errors.New("boom")
+
+	type args struct {
+		create func(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error)
+	}
+	type want struct {
+		err error
+		// created is whether Setup should create a network, annotate the
+		// functions to join it, and return a cleanup that removes it.
+		created bool
+	}
+
+	cases := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"CreatesNetwork": {
+			reason: "Setup should create a network, annotate the functions to join it, and return a cleanup that removes it.",
+			args: args{
+				create: createRenderNetworkReturns("network-id", nil),
+			},
+			want: want{
+				created: true,
+			},
+		},
+		"NetworkCreateError": {
+			reason: "Setup should return an error and a no-op cleanup, leaving the functions unannotated, when it cannot create the network.",
+			args: args{
+				create: createRenderNetworkReturns("", errBoom),
+			},
+			want: want{
+				err: errBoom,
+			},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			// Cleanup discards NetworkRemove's result, so record whether it
+			// was called. A cleanup that shouldn't remove anything leaves
+			// MockNetworkRemove nil.
+			removed := false
+			cli := &mockNetworkClient{MockNetworkCreate: tc.args.create}
+			if tc.want.created {
+				cli.MockNetworkRemove = func(_ context.Context, networkID string, _ client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) {
+					if diff := cmp.Diff("network-id", networkID); diff != "" {
+						t.Errorf("\n%s\nNetworkRemove(...): -want network ID, +got network ID:\n%s", tc.reason, diff)
+					}
+					removed = true
+					return client.NetworkRemoveResult{}, nil
+				}
+			}
+			e := &dockerRenderEngine{log: logging.NewNopLogger(), networks: cli}
+			fns := []pkgv1.Function{functionWithAnnotations(nil)}
+
+			cleanup, err := e.Setup(t.Context(), fns)
+
+			if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\nSetup(...): -want error, +got error:\n%s", tc.reason, diff)
+			}
+
+			wantFns := []pkgv1.Function{functionWithAnnotations(nil)}
+			if tc.want.created {
+				if !strings.HasPrefix(e.network, renderNetworkPrefix) {
+					t.Errorf("\n%s\nSetup(...): e.network = %q, want a network with prefix %q", tc.reason, e.network, renderNetworkPrefix)
+				}
+				wantFns = []pkgv1.Function{functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: e.network})}
+			} else if e.network != "" {
+				t.Errorf("\n%s\nSetup(...): e.network = %q, want it unset", tc.reason, e.network)
+			}
+			if diff := cmp.Diff(wantFns, fns); diff != "" {
+				t.Errorf("\n%s\nSetup(...): -want fns, +got fns:\n%s", tc.reason, diff)
+			}
+
+			cleanup()
+
+			if diff := cmp.Diff(tc.want.created, removed); diff != "" {
+				t.Errorf("\n%s\nSetup(...) cleanup: -want network removed, +got network removed:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}
+
 // nonExitError is a stand-in for non-*ContainerExitError failures (e.g. image
 // pull errors) returned by docker.RunContainer.
 type nonExitError struct{ msg string }

cmd/crossplane/render/network.go

@@ -46,15 +46,28 @@ func (f *EngineFlags) SetDefaultCrossplaneDockerNetwork(fns []pkgv1.Function) {
 	}
 }
 
-// createRenderNetwork creates a temporary Docker bridge network for render.
-// Function containers and the Crossplane render container join this network so
-// they can reach each other. Returns the network ID and name.
-func createRenderNetwork(ctx context.Context) (string, string, error) {
+// networkClient is the subset of the Docker client the render engine uses to
+// manage its temporary Docker network.
+type networkClient interface {
+	NetworkCreate(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error)
+	NetworkRemove(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error)
+}
+
+var _ networkClient = (*client.Client)(nil)
+
+// newNetworkClient returns a real Docker client built from the environment.
+func newNetworkClient() (networkClient, error) {
 	cli, err := docker.NewClient()
 	if err != nil {
-		return "", "", errors.Wrap(err, "cannot create Docker client")
+		return nil, errors.Wrap(err, "cannot create Docker client")
 	}
+	return cli, nil
+}
 
+// createRenderNetwork creates a temporary Docker bridge network for render.
+// Function containers and the Crossplane render container join this network so
+// they can reach each other. Returns the network ID and name.
+func createRenderNetwork(ctx context.Context, cli networkClient) (string, string, error) {
 	name := fmt.Sprintf("crossplane-render-%s", rand.String(8))
 
 	resp, err := cli.NetworkCreate(ctx, name, client.NetworkCreateOptions{
@@ -68,11 +81,7 @@ func createRenderNetwork(ctx context.Context) (string, string, error) {
 }
 
 // removeRenderNetwork removes a temporary Docker network.
-func removeRenderNetwork(ctx context.Context, networkID string) error {
-	cli, err := docker.NewClient()
-	if err != nil {
-		return errors.Wrap(err, "cannot create Docker client")
-	}
-	_, err = cli.NetworkRemove(ctx, networkID, client.NetworkRemoveOptions{})
+func removeRenderNetwork(ctx context.Context, cli networkClient, networkID string) error {
+	_, err := cli.NetworkRemove(ctx, networkID, client.NetworkRemoveOptions{})
 	return errors.Wrap(err, "cannot remove Docker network")
 }

cmd/crossplane/render/network_test.go

@@ -1,9 +1,15 @@
 package render
 
 import (
+	"context"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/moby/moby/client"
+
+	"github.com/crossplane/crossplane-runtime/v2/pkg/errors"
 
 	pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1"
 )
@@ -74,3 +80,152 @@ func TestSetDefaultCrossplaneDockerNetwork(t *testing.T) {
 		})
 	}
 }
+
+type mockNetworkClient struct {
+	MockNetworkCreate func(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error)
+	MockNetworkRemove func(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error)
+}
+
+func (m *mockNetworkClient) NetworkCreate(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) {
+	return m.MockNetworkCreate(ctx, name, options)
+}
+
+func (m *mockNetworkClient) NetworkRemove(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) {
+	return m.MockNetworkRemove(ctx, networkID, options)
+}
+
+var _ networkClient = &mockNetworkClient{}
+
+// renderNetworkPrefix is the prefix of the temporary network name
+// createRenderNetwork generates.
+const renderNetworkPrefix = "crossplane-render-"
+
+// createRenderNetworkReturns returns a MockNetworkCreate that returns the
+// supplied network ID and error. It returns an error instead when it is not
+// asked to create a render bridge network.
+func createRenderNetworkReturns(id string, err error) func(context.Context, string, client.NetworkCreateOptions) (client.NetworkCreateResult, error) {
+	return func(_ context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) {
+		if !strings.HasPrefix(name, renderNetworkPrefix) {
+			return client.NetworkCreateResult{}, errors.Errorf("NetworkCreate(...): name %q does not have prefix %q", name, renderNetworkPrefix)
+		}
+		if diff := cmp.Diff(client.NetworkCreateOptions{Driver: "bridge"}, options); diff != "" {
+			return client.NetworkCreateResult{}, errors.Errorf("NetworkCreate(...): -want options, +got options:\n%s", diff)
+		}
+		return client.NetworkCreateResult{ID: id}, err
+	}
+}
+
+func TestCreateRenderNetwork(t *testing.T) {
+	errBoom := errors.New("boom")
+
+	type args struct {
+		cli networkClient
+	}
+	type want struct {
+		id string
+		// namePrefix is a prefix the returned network name must have. The
+		// rest of the name is random.
+		namePrefix string
+		err        error
+	}
+
+	cases := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"CreatesBridgeNetwork": {
+			reason: "createRenderNetwork should create a uniquely named bridge network through the supplied client and return its ID and name.",
+			args: args{
+				cli: &mockNetworkClient{MockNetworkCreate: createRenderNetworkReturns("network-id", nil)},
+			},
+			want: want{
+				id:         "network-id",
+				namePrefix: renderNetworkPrefix,
+			},
+		},
+		"NetworkCreateError": {
+			reason: "createRenderNetwork should return an error when the client cannot create the network.",
+			args: args{
+				cli: &mockNetworkClient{MockNetworkCreate: createRenderNetworkReturns("", errBoom)},
+			},
+			want: want{
+				err: errBoom,
+			},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			id, networkName, err := createRenderNetwork(t.Context(), tc.args.cli)
+
+			if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\ncreateRenderNetwork(...): -want error, +got error:\n%s", tc.reason, diff)
+			}
+			if diff := cmp.Diff(tc.want.id, id); diff != "" {
+				t.Errorf("\n%s\ncreateRenderNetwork(...): -want ID, +got ID:\n%s", tc.reason, diff)
+			}
+			if !strings.HasPrefix(networkName, tc.want.namePrefix) {
+				t.Errorf("\n%s\ncreateRenderNetwork(...): name %q does not have prefix %q", tc.reason, networkName, tc.want.namePrefix)
+			}
+		})
+	}
+}
+
+func TestRemoveRenderNetwork(t *testing.T) {
+	errBoom := errors.New("boom")
+
+	// removeNetworkReturns returns a MockNetworkRemove that returns the
+	// supplied error, or an error of its own when asked to remove any network
+	// other than network-id.
+	removeNetworkReturns := func(err error) func(context.Context, string, client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) {
+		return func(_ context.Context, networkID string, _ client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) {
+			if diff := cmp.Diff("network-id", networkID); diff != "" {
+				return client.NetworkRemoveResult{}, errors.Errorf("NetworkRemove(...): -want network ID, +got network ID:\n%s", diff)
+			}
+			return client.NetworkRemoveResult{}, err
+		}
+	}
+
+	type args struct {
+		cli       networkClient
+		networkID string
+	}
+	type want struct {
+		err error
+	}
+
+	cases := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"RemovesNetwork": {
+			reason: "removeRenderNetwork should remove the network with the supplied ID through the supplied client.",
+			args: args{
+				cli:       &mockNetworkClient{MockNetworkRemove: removeNetworkReturns(nil)},
+				networkID: "network-id",
+			},
+		},
+		"NetworkRemoveError": {
+			reason: "removeRenderNetwork should return an error when the client cannot remove the network.",
+			args: args{
+				cli:       &mockNetworkClient{MockNetworkRemove: removeNetworkReturns(errBoom)},
+				networkID: "network-id",
+			},
+			want: want{
+				err: errBoom,
+			},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			err := removeRenderNetwork(t.Context(), tc.args.cli, tc.args.networkID)
+
+			if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\nremoveRenderNetwork(...): -want error, +got error:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}

cmd/crossplane/render/op/cmd.go

@@ -218,7 +218,11 @@ func (c *Cmd) Run(k *kong.Context, log logging.Logger, sp terminal.SpinnerPrinte
 	if err != nil {
 		return errors.Wrap(err, "cannot start function runtimes")
 	}
-	defer render.StopFunctionRuntimes(log, fnAddrs)
+	defer func() {
+		if err := render.StopFunctionRuntimes(ctx, fnAddrs); err != nil {
+			log.Info("Error stopping function runtimes", "error", err)
+		}
+	}()
 
 	// Build and execute the render request.
 	in := render.OperationInputs{