From 88c44e80f786e1ebf47acd9e6dc62e237e1da441 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 2 Oct 2026 13:27:51 -0400 Subject: [PATCH 1/2] refactor(render): inject Docker client interfaces for container and network operations RuntimeDocker.Start and the render network helpers each construct their own Docker client from the environment, so the container lifecycle and network setup/teardown can only be exercised against a live Docker daemon. That blocks unit-testing the follow-up fixes for #397, #398 and #401. Introduce narrow unexported interfaces covering exactly the moby client methods each site uses: containerClient (image pull, container inspect/create/start, plus containerCleanupClient for stop/remove) for RuntimeDocker, and networkClient (network create/remove) for the render network helpers. *client.Client satisfies both, enforced by compile-time assertions. RuntimeDocker gains an unexported dockerClient field; when nil, Start builds the real client from the environment exactly as before. createRenderNetwork and removeRenderNetwork now take the client as a parameter, and dockerRenderEngine gains an unexported networks field; when nil, Setup builds the real client only on the create-network branch, with the same error wrapping as before. No exported signature, the Engine interface, or the cleanup policy semantics change. Add function-field mocks for both interfaces: mockContainerClient next to the existing mockPullClient in runtime_docker_test.go, and mockNetworkClient in network_test.go. Add table tests that assert on results and sentinel errors, proving each seam is wired: network create/remove, Setup's create-network branch and its cleanup, RuntimeDocker.Start's create/start/inspect and image pull path, and its stop closure for the Stop, Remove and Orphan cleanup policies. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- cmd/crossplane/render/engine_docker.go | 20 +- cmd/crossplane/render/engine_docker_test.go | 97 ++++- cmd/crossplane/render/network.go | 31 +- cmd/crossplane/render/network_test.go | 155 ++++++++ cmd/crossplane/render/runtime_docker.go | 41 ++- cmd/crossplane/render/runtime_docker_test.go | 356 +++++++++++++++++++ 6 files changed, 678 insertions(+), 22 deletions(-) diff --git a/cmd/crossplane/render/engine_docker.go b/cmd/crossplane/render/engine_docker.go index 9beb3e8c..ac60193d 100644 --- a/cmd/crossplane/render/engine_docker.go +++ b/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 diff --git a/cmd/crossplane/render/engine_docker_test.go b/cmd/crossplane/render/engine_docker_test.go index 73fcdddb..4d6d0d8a 100644 --- a/cmd/crossplane/render/engine_docker_test.go +++ b/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 } diff --git a/cmd/crossplane/render/network.go b/cmd/crossplane/render/network.go index 454b122a..8ef17d68 100644 --- a/cmd/crossplane/render/network.go +++ b/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") } diff --git a/cmd/crossplane/render/network_test.go b/cmd/crossplane/render/network_test.go index fde13f0f..843f16f4 100644 --- a/cmd/crossplane/render/network_test.go +++ b/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) + } + }) + } +} diff --git a/cmd/crossplane/render/runtime_docker.go b/cmd/crossplane/render/runtime_docker.go index 169fe3f5..8972945b 100644 --- a/cmd/crossplane/render/runtime_docker.go +++ b/cmd/crossplane/render/runtime_docker.go @@ -161,8 +161,33 @@ type RuntimeDocker struct { // and is reached via host port bindings. When set, the container joins // the specified network and is reached via its Docker hostname on port 9443. Network string + + // dockerClient manages the Function's container. Production callers leave + // it nil and Start builds a real client from the environment. Tests + // substitute a fake to exercise container lifecycle and cleanup handling + // without a real Docker daemon. + dockerClient containerClient +} + +// containerClient is the subset of the Docker client RuntimeDocker uses to +// pull images and manage a Function's container. +type containerClient interface { + pullClient + containerCleanupClient + ContainerInspect(ctx context.Context, containerID string, options client.ContainerInspectOptions) (client.ContainerInspectResult, error) + ContainerCreate(ctx context.Context, options client.ContainerCreateOptions) (client.ContainerCreateResult, error) + ContainerStart(ctx context.Context, containerID string, options client.ContainerStartOptions) (client.ContainerStartResult, error) +} + +// containerCleanupClient is the subset of the Docker client RuntimeDocker's +// stop function uses to clean up a Function's container. +type containerCleanupClient interface { + ContainerStop(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) + ContainerRemove(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) } +var _ containerClient = (*client.Client)(nil) + // GetDockerPullPolicy extracts PullPolicy configuration from the supplied // Function. func GetDockerPullPolicy(fn pkgv1.Function) (DockerPullPolicy, error) { @@ -250,7 +275,7 @@ func GetRuntimeDocker(fn pkgv1.Function, log logging.Logger) (*RuntimeDocker, er var _ Runtime = &RuntimeDocker{} -func (r *RuntimeDocker) findContainer(ctx context.Context, cli *client.Client) (string, error) { +func (r *RuntimeDocker) findContainer(ctx context.Context, cli containerClient) (string, error) { if r.Name == "" { return "", nil } @@ -266,7 +291,7 @@ func (r *RuntimeDocker) findContainer(ctx context.Context, cli *client.Client) ( return inspect.Container.ID, nil } -func (r *RuntimeDocker) createContainer(ctx context.Context, cli *client.Client) (string, error) { +func (r *RuntimeDocker) createContainer(ctx context.Context, cli containerClient) (string, error) { r.log.Debug("Starting Docker container runtime setup", "image", r.Image) // Let Docker automatically allocate an available port on the bind address. @@ -377,7 +402,7 @@ func (r *RuntimeDocker) createContainer(ctx context.Context, cli *client.Client) } // startContainer ensures the container is running and returns its address. -func (r *RuntimeDocker) startContainer(ctx context.Context, cli *client.Client, containerID string) (string, error) { +func (r *RuntimeDocker) startContainer(ctx context.Context, cli containerClient, containerID string) (string, error) { // Start the container (idempotent - safe to call on running containers) if _, err := cli.ContainerStart(ctx, containerID, client.ContainerStartOptions{}); err != nil { return "", errors.Wrap(err, "cannot start Docker container") @@ -457,9 +482,13 @@ func (r *RuntimeDocker) getPullOptions() (client.ImagePullOptions, error) { // Start a Function as a Docker container. func (r *RuntimeDocker) Start(ctx context.Context) (RuntimeContext, error) { - cli, err := client.New(client.FromEnv) - if err != nil { - return RuntimeContext{}, errors.Wrap(err, "cannot create Docker client using environment variables") + cli := r.dockerClient + if cli == nil { + c, err := client.New(client.FromEnv) + if err != nil { + return RuntimeContext{}, errors.Wrap(err, "cannot create Docker client using environment variables") + } + cli = c } // Try to find an existing container with the supplied container name. diff --git a/cmd/crossplane/render/runtime_docker_test.go b/cmd/crossplane/render/runtime_docker_test.go index a05a6079..68c2e90c 100644 --- a/cmd/crossplane/render/runtime_docker_test.go +++ b/cmd/crossplane/render/runtime_docker_test.go @@ -18,13 +18,20 @@ package render import ( "context" + "io" + "net" + "strconv" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/google/go-containerregistry/pkg/authn" + "github.com/moby/moby/api/types/container" + "github.com/moby/moby/api/types/network" "github.com/moby/moby/client" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "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" @@ -40,6 +47,95 @@ func (m *mockPullClient) ImagePull(ctx context.Context, ref string, options clie var _ pullClient = &mockPullClient{} +type mockContainerClient struct { + MockImagePull func(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) + MockContainerInspect func(ctx context.Context, containerID string, options client.ContainerInspectOptions) (client.ContainerInspectResult, error) + MockContainerCreate func(ctx context.Context, options client.ContainerCreateOptions) (client.ContainerCreateResult, error) + MockContainerStart func(ctx context.Context, containerID string, options client.ContainerStartOptions) (client.ContainerStartResult, error) + MockContainerStop func(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) + MockContainerRemove func(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) +} + +func (m *mockContainerClient) ImagePull(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) { + return m.MockImagePull(ctx, ref, options) +} + +func (m *mockContainerClient) ContainerInspect(ctx context.Context, containerID string, options client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + return m.MockContainerInspect(ctx, containerID, options) +} + +func (m *mockContainerClient) ContainerCreate(ctx context.Context, options client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + return m.MockContainerCreate(ctx, options) +} + +func (m *mockContainerClient) ContainerStart(ctx context.Context, containerID string, options client.ContainerStartOptions) (client.ContainerStartResult, error) { + return m.MockContainerStart(ctx, containerID, options) +} + +func (m *mockContainerClient) ContainerStop(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) { + return m.MockContainerStop(ctx, containerID, options) +} + +func (m *mockContainerClient) ContainerRemove(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return m.MockContainerRemove(ctx, containerID, options) +} + +var _ containerClient = &mockContainerClient{} + +// imagePullDone is a MockImagePull that reports an image pull that has +// already completed. +func imagePullDone(context.Context, string, client.ImagePullOptions) (client.ImagePullResponse, error) { + return pullDoneResponse{}, nil +} + +// pullDoneResponse is an ImagePullResponse whose body is empty. Only the +// io.ReadCloser methods PullImage uses are implemented; the embedded +// interface is nil. +type pullDoneResponse struct { + client.ImagePullResponse +} + +func (pullDoneResponse) Read([]byte) (int, error) { return 0, io.EOF } + +func (pullDoneResponse) Close() error { return nil } + +// createContainerReturns returns a MockContainerCreate that creates a +// container with the supplied ID. +func createContainerReturns(id string) func(context.Context, client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + return func(context.Context, client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + return client.ContainerCreateResult{ID: id}, nil + } +} + +// startContainer returns a MockContainerStart that starts the container with +// the supplied ID, and returns an error for any other container. +func startContainer(id string) func(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) { + return func(_ context.Context, containerID string, _ client.ContainerStartOptions) (client.ContainerStartResult, error) { + if diff := cmp.Diff(id, containerID); diff != "" { + return client.ContainerStartResult{}, errors.Errorf("ContainerStart(...): -want container ID, +got container ID:\n%s", diff) + } + return client.ContainerStartResult{}, nil + } +} + +// inspectContainerOnNetwork returns a MockContainerInspect that reports the +// container with the supplied ID as running with the supplied name, attached +// to the supplied Docker network. It returns an error for any other container. +func inspectContainerOnNetwork(id, name, networkName string) func(context.Context, string, client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + return func(_ context.Context, containerID string, _ client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + if diff := cmp.Diff(id, containerID); diff != "" { + return client.ContainerInspectResult{}, errors.Errorf("ContainerInspect(...): -want container ID, +got container ID:\n%s", diff) + } + return client.ContainerInspectResult{Container: container.InspectResponse{ + ID: id, + Name: "/" + name, + NetworkSettings: &container.NetworkSettings{ + Networks: map[string]*network.EndpointSettings{networkName: {}}, + }, + }}, nil + } +} + func TestGetRuntimeDocker(t *testing.T) { type args struct { fn pkgv1.Function @@ -246,3 +342,263 @@ func TestGetRuntimeDocker(t *testing.T) { }) } } + +func TestRuntimeDockerStart(t *testing.T) { + const ( + image = "xpkg.crossplane.io/crossplane-contrib/function-dummy:v0.1.0" + containerID = "container-id" + containerName = "fn-container" + dockerNetwork = "render-net" + ) + + errBoom := errors.New("boom") + + type args struct { + pullPolicy DockerPullPolicy + cli *mockContainerClient + } + type want struct { + rctx RuntimeContext + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "CreatesAndStartsContainer": { + reason: "Start should create, start, and inspect the Function's container through the injected client, without pulling an image that is present.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyIfNotPresent, + cli: &mockContainerClient{ + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + }, + }, + want: want{ + rctx: RuntimeContext{Target: net.JoinHostPort(containerName, strconv.Itoa(FunctionPort))}, + }, + }, + "PullsImage": { + reason: "Start should pull the Function's image through the injected client when the pull policy is Always.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyAlways, + cli: &mockContainerClient{ + MockImagePull: func(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) { + if diff := cmp.Diff(image, ref); diff != "" { + return nil, errors.Errorf("ImagePull(...): -want ref, +got ref:\n%s", diff) + } + return imagePullDone(ctx, ref, options) + }, + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + }, + }, + want: want{ + rctx: RuntimeContext{Target: net.JoinHostPort(containerName, strconv.Itoa(FunctionPort))}, + }, + }, + "PullError": { + reason: "Start should return an error, without creating a container, when the injected client cannot pull the image.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyAlways, + cli: &mockContainerClient{ + MockImagePull: func(context.Context, string, client.ImagePullOptions) (client.ImagePullResponse, error) { + return nil, errBoom + }, + }, + }, + want: want{ + err: errBoom, + }, + }, + "StartError": { + reason: "Start should return an error when the injected client cannot start the container.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyIfNotPresent, + cli: &mockContainerClient{ + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: func(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) { + return client.ContainerStartResult{}, errBoom + }, + }, + }, + want: want{ + err: errBoom, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + r := &RuntimeDocker{ + Image: image, + Cleanup: AnnotationValueRuntimeDockerCleanupRemove, + PullPolicy: tc.args.pullPolicy, + Keychain: authn.NewMultiKeychain(), + Network: dockerNetwork, + log: logging.NewNopLogger(), + dockerClient: tc.args.cli, + } + + rctx, err := r.Start(t.Context()) + + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nStart(...): -want error, +got error:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.rctx, rctx, cmpopts.IgnoreFields(RuntimeContext{}, "Stop")); diff != "" { + t.Errorf("\n%s\nStart(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestRuntimeDockerStop(t *testing.T) { + const ( + containerID = "container-id" + containerName = "fn-container" + dockerNetwork = "render-net" + ) + + errStop := errors.New("stop boom") + errRemove := errors.New("remove boom") + + type args struct { + cleanup DockerCleanup + // stop and remove are the cleanup client's ContainerStop and + // ContainerRemove. A nil mock must not be called. The test checks + // the arguments each non-nil mock is called with. + stop func(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) + remove func(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) + } + type want struct { + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "Stop": { + reason: "The Stop cleanup policy should stop the container and leave it in place.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupStop, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, nil + }, + }, + }, + "StopError": { + reason: "The Stop cleanup policy should return an error when the container cannot be stopped.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupStop, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, errStop + }, + }, + want: want{ + err: errStop, + }, + }, + "Remove": { + reason: "The Remove cleanup policy should stop the container, then remove it.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, nil + }, + remove: func(context.Context, string, client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return client.ContainerRemoveResult{}, nil + }, + }, + }, + "RemoveStopError": { + reason: "The Remove cleanup policy should stop the container before removing it, and return an error without removing it when it cannot be stopped.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, errStop + }, + }, + want: want{ + err: errStop, + }, + }, + "RemoveError": { + reason: "The Remove cleanup policy should return an error when the container cannot be removed.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, nil + }, + remove: func(context.Context, string, client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return client.ContainerRemoveResult{}, errRemove + }, + }, + want: want{ + err: errRemove, + }, + }, + "Orphan": { + reason: "The Orphan cleanup policy should leave the container running without calling Docker.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupOrphan, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + cli := &mockContainerClient{ + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + } + if stop := tc.args.stop; stop != nil { + cli.MockContainerStop = func(ctx context.Context, id string, options client.ContainerStopOptions) (client.ContainerStopResult, error) { + if diff := cmp.Diff(containerID, id); diff != "" { + t.Errorf("\n%s\nContainerStop(...): -want container ID, +got container ID:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(client.ContainerStopOptions{}, options); diff != "" { + t.Errorf("\n%s\nContainerStop(...): -want options, +got options:\n%s", tc.reason, diff) + } + return stop(ctx, id, options) + } + } + if remove := tc.args.remove; remove != nil { + cli.MockContainerRemove = func(ctx context.Context, id string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + if diff := cmp.Diff(containerID, id); diff != "" { + t.Errorf("\n%s\nContainerRemove(...): -want container ID, +got container ID:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(client.ContainerRemoveOptions{}, options); diff != "" { + t.Errorf("\n%s\nContainerRemove(...): -want options, +got options:\n%s", tc.reason, diff) + } + return remove(ctx, id, options) + } + } + r := &RuntimeDocker{ + Image: "xpkg.crossplane.io/crossplane-contrib/function-dummy:v0.1.0", + Cleanup: tc.args.cleanup, + PullPolicy: AnnotationValueRuntimeDockerPullPolicyIfNotPresent, + Keychain: authn.NewMultiKeychain(), + Network: dockerNetwork, + log: logging.NewNopLogger(), + dockerClient: cli, + } + + rctx, err := r.Start(t.Context()) + if err != nil { + t.Fatalf("\n%s\nStart(...): unexpected error: %v", tc.reason, err) + } + + err = rctx.Stop(t.Context()) + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nStop(...): -want error, +got error:\n%s", tc.reason, diff) + } + }) + } +} From 31e180651642872407d4b30812a21631aa579e3f Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 2 Oct 2026 13:40:38 -0400 Subject: [PATCH 2/2] fix(render): log docker network removal failures instead of discarding them The cleanup returned by dockerRenderEngine.Setup discarded the error from removing the temporary render network, so a failed removal (for example a container still attached) silently leaked the network. The cleanup signature can't return an error, so log it through the engine's logger with the network name and ID. The cleanup also used context.Background(). Derive its context from Setup's ctx via context.WithoutCancel, bounded by networkRemoveTimeout, so removal survives the caller's cancellation without hanging forever, and drop the now-unneeded contextcheck suppression. Fixes #398 Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- cmd/crossplane/render/engine_docker.go | 18 ++++- cmd/crossplane/render/engine_docker_test.go | 83 ++++++++++++++++++++- 2 files changed, 95 insertions(+), 6 deletions(-) diff --git a/cmd/crossplane/render/engine_docker.go b/cmd/crossplane/render/engine_docker.go index ac60193d..af5ed8fc 100644 --- a/cmd/crossplane/render/engine_docker.go +++ b/cmd/crossplane/render/engine_docker.go @@ -21,6 +21,7 @@ import ( "os" "runtime" "strings" + "time" "google.golang.org/protobuf/proto" @@ -48,6 +49,10 @@ func (realContainerRunner) Run(ctx context.Context, img string, opts ...docker.R return docker.RunContainer(ctx, img, opts...) } +// networkRemoveTimeout bounds how long Setup's cleanup waits to remove the +// temporary render network. +const networkRemoveTimeout = 30 * time.Second + // dockerRenderEngine executes crossplane internal render in a Docker container. type dockerRenderEngine struct { // image is the Crossplane Docker image reference. @@ -119,8 +124,17 @@ 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(), cli, networkID) + cleanup := func() { + // Derive from ctx without its cancellation: cleanup typically runs after + // the caller's context is done, but must still be bounded so removal + // can't hang forever. + rctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), networkRemoveTimeout) + defer cancel() + if err := removeRenderNetwork(rctx, cli, networkID); err != nil { + // The cleanup signature can't return the error, so log it rather than + // silently leaking the network (e.g. a container is still attached). + e.log.Info("Cannot remove Docker network used for rendering", "network", networkName, "id", networkID, "error", err) + } } return cleanup, nil diff --git a/cmd/crossplane/render/engine_docker_test.go b/cmd/crossplane/render/engine_docker_test.go index 4d6d0d8a..970a7a21 100644 --- a/cmd/crossplane/render/engine_docker_test.go +++ b/cmd/crossplane/render/engine_docker_test.go @@ -357,12 +357,21 @@ func TestDockerRenderEngineSetupCreatesNetwork(t *testing.T) { type args struct { create func(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) + // removeErr is what NetworkRemove returns. When nil, NetworkRemove + // returns the error of the context it is called with, so cleanup + // logs an error if it removes the network with a cancelled context. + removeErr error + // cancel cancels Setup's context before calling cleanup. + cancel bool } 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 + // logErr is the error cleanup should log when it cannot remove the + // network. When nil, cleanup must log nothing. + logErr error } cases := map[string]struct { @@ -379,6 +388,27 @@ func TestDockerRenderEngineSetupCreatesNetwork(t *testing.T) { created: true, }, }, + "CleanupLogsRemoveError": { + reason: "The cleanup can't return an error, so it should log a failure to remove the network, with the network's name and ID, rather than discard it.", + args: args{ + create: createRenderNetworkReturns("network-id", nil), + removeErr: errBoom, + }, + want: want{ + created: true, + logErr: errBoom, + }, + }, + "CleanupSurvivesCancelledContext": { + reason: "The cleanup typically runs after Setup's context is done, so it should still remove the network with a live context.", + args: args{ + create: createRenderNetworkReturns("network-id", nil), + cancel: true, + }, + 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{ @@ -398,18 +428,25 @@ func TestDockerRenderEngineSetupCreatesNetwork(t *testing.T) { removed := false cli := &mockNetworkClient{MockNetworkCreate: tc.args.create} if tc.want.created { - cli.MockNetworkRemove = func(_ context.Context, networkID string, _ client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + cli.MockNetworkRemove = func(ctx 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 + if tc.args.removeErr != nil { + return client.NetworkRemoveResult{}, tc.args.removeErr + } + return client.NetworkRemoveResult{}, ctx.Err() } } - e := &dockerRenderEngine{log: logging.NewNopLogger(), networks: cli} + log := newRecordingLogger() + e := &dockerRenderEngine{log: log, networks: cli} fns := []pkgv1.Function{functionWithAnnotations(nil)} - cleanup, err := e.Setup(t.Context(), fns) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + cleanup, err := e.Setup(ctx, 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) @@ -428,15 +465,53 @@ func TestDockerRenderEngineSetupCreatesNetwork(t *testing.T) { t.Errorf("\n%s\nSetup(...): -want fns, +got fns:\n%s", tc.reason, diff) } + if tc.args.cancel { + cancel() + } 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) } + var wantLog []logEntry + if tc.want.logErr != nil { + wantLog = []logEntry{{ + Msg: "Cannot remove Docker network used for rendering", + KV: []any{"network", e.network, "id", "network-id", "error", tc.want.logErr}, + }} + } + if diff := cmp.Diff(wantLog, *log.entries, cmpopts.EquateErrors(), cmpopts.EquateEmpty()); diff != "" { + t.Errorf("\n%s\nSetup(...) cleanup: -want log, +got log:\n%s", tc.reason, diff) + } }) } } +// recordingLogger is a logging.Logger that records every Info and Debug call. +type recordingLogger struct { + entries *[]logEntry + kv []any +} + +type logEntry struct { + Msg string + KV []any +} + +var _ logging.Logger = recordingLogger{} + +func newRecordingLogger() recordingLogger { return recordingLogger{entries: &[]logEntry{}} } + +func (l recordingLogger) Info(msg string, kv ...any) { + *l.entries = append(*l.entries, logEntry{Msg: msg, KV: append(append([]any{}, l.kv...), kv...)}) +} + +func (l recordingLogger) Debug(msg string, kv ...any) { l.Info(msg, kv...) } + +func (l recordingLogger) WithValues(kv ...any) logging.Logger { + return recordingLogger{entries: l.entries, kv: append(append([]any{}, l.kv...), kv...)} +} + // nonExitError is a stand-in for non-*ContainerExitError failures (e.g. image // pull errors) returned by docker.RunContainer. type nonExitError struct{ msg string }