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 3dc33719e81ef51ff7fc2b5f8f8373fefc6185c0 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 2 Oct 2026 13:41:31 -0400 Subject: [PATCH 2/2] feat(render): label docker containers and networks created by render Docker resources created by crossplane render carried no labels, so leftovers after a crash, kill -9, a CI timeout, or a deliberate Orphan cleanup policy could not be reliably identified or swept. Apply render.crossplane.io/managed-by=crossplane to Function containers, the render engine container and the temporary render network, and record the effective cleanup policy on Function containers as render.crossplane.io/cleanup. The label keys are exported constants, and a new docker.RunWithLabels option plumbs labels into docker.RunContainer. Fixes #401 Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- cmd/crossplane/render/engine_docker.go | 1 + cmd/crossplane/render/network.go | 1 + cmd/crossplane/render/network_test.go | 4 +- cmd/crossplane/render/runtime_docker.go | 34 +++++++ cmd/crossplane/render/runtime_docker_test.go | 100 +++++++++++++++++++ internal/docker/docker.go | 11 ++ internal/docker/docker_test.go | 54 ++++++++++ 7 files changed, 203 insertions(+), 2 deletions(-) create mode 100644 internal/docker/docker_test.go diff --git a/cmd/crossplane/render/engine_docker.go b/cmd/crossplane/render/engine_docker.go index ac60193d..0b5a1c91 100644 --- a/cmd/crossplane/render/engine_docker.go +++ b/cmd/crossplane/render/engine_docker.go @@ -156,6 +156,7 @@ func (e *dockerRenderEngine) Render(ctx context.Context, req *renderv1alpha1.Ren // Let the container access any functions running in development mode on // the host. docker.RunWithExtraHosts([]string{"host.docker.internal:host-gateway"}), + docker.RunWithLabels(managedLabels()), } if e.network != "" { opts = append(opts, docker.RunWithNetworkName(e.network)) diff --git a/cmd/crossplane/render/network.go b/cmd/crossplane/render/network.go index 8ef17d68..3dfa2346 100644 --- a/cmd/crossplane/render/network.go +++ b/cmd/crossplane/render/network.go @@ -72,6 +72,7 @@ func createRenderNetwork(ctx context.Context, cli networkClient) (string, string resp, err := cli.NetworkCreate(ctx, name, client.NetworkCreateOptions{ Driver: "bridge", + Labels: managedLabels(), }) if err != nil { return "", "", errors.Wrapf(err, "cannot create Docker network %q", name) diff --git a/cmd/crossplane/render/network_test.go b/cmd/crossplane/render/network_test.go index 843f16f4..315fa708 100644 --- a/cmd/crossplane/render/network_test.go +++ b/cmd/crossplane/render/network_test.go @@ -102,13 +102,13 @@ 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. +// asked to create a render bridge network labeled as managed by render. 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 != "" { + if diff := cmp.Diff(client.NetworkCreateOptions{Driver: "bridge", Labels: map[string]string{LabelKeyManagedBy: LabelValueManagedByCrossplane}}, options); diff != "" { return client.NetworkCreateResult{}, errors.Errorf("NetworkCreate(...): -want options, +got options:\n%s", diff) } return client.NetworkCreateResult{ID: id}, err diff --git a/cmd/crossplane/render/runtime_docker.go b/cmd/crossplane/render/runtime_docker.go index 8972945b..317c4ea0 100644 --- a/cmd/crossplane/render/runtime_docker.go +++ b/cmd/crossplane/render/runtime_docker.go @@ -81,6 +81,36 @@ const ( AnnotationKeyRuntimeDockerNetwork = "render.crossplane.io/runtime-docker-network" ) +// Labels that render applies to the Docker resources it creates, so that +// leftovers (e.g. after a crash or with an Orphan cleanup policy) can be +// identified and swept. +const ( + // LabelKeyManagedBy is applied to every Docker container and network + // created by render. Its value is always LabelValueManagedByCrossplane. + LabelKeyManagedBy = "render.crossplane.io/managed-by" + + // LabelValueManagedByCrossplane is the value of LabelKeyManagedBy. + LabelValueManagedByCrossplane = "crossplane" + + // LabelKeyCleanup is applied to Function containers and records the + // effective DockerCleanup policy the container was created with. + LabelKeyCleanup = "render.crossplane.io/cleanup" +) + +// managedLabels returns the labels applied to every Docker resource render +// creates. +func managedLabels() map[string]string { + return map[string]string{LabelKeyManagedBy: LabelValueManagedByCrossplane} +} + +// functionContainerLabels returns the labels applied to a Function container +// created with the supplied cleanup policy. +func functionContainerLabels(cleanup DockerCleanup) map[string]string { + l := managedLabels() + l[LabelKeyCleanup] = string(cleanup) + return l +} + // DockerCleanup specifies what Docker should do with a Function container after // it has been run. type DockerCleanup string @@ -303,6 +333,10 @@ func (r *RuntimeDocker) createContainer(ctx context.Context, cli containerClient Cmd: []string{"--insecure"}, ExposedPorts: network.PortSet{port: struct{}{}}, Env: r.Env, + // Labels are only set at creation time. A pre-existing named container + // that is reused is not relabeled, since Docker can't change labels on + // an existing container. + Labels: functionContainerLabels(r.Cleanup), } hcfg := &container.HostConfig{} var ncfg *network.NetworkingConfig diff --git a/cmd/crossplane/render/runtime_docker_test.go b/cmd/crossplane/render/runtime_docker_test.go index 68c2e90c..a06ceccb 100644 --- a/cmd/crossplane/render/runtime_docker_test.go +++ b/cmd/crossplane/render/runtime_docker_test.go @@ -602,3 +602,103 @@ func TestRuntimeDockerStop(t *testing.T) { }) } } + +func TestRuntimeDockerStartLabels(t *testing.T) { + const ( + containerID = "container-id" + containerName = "fn-container" + dockerNetwork = "render-net" + ) + + // createLabeledContainer returns a MockContainerCreate that creates a + // container, or returns an error when the container config doesn't carry + // the supplied labels. + createLabeledContainer := func(labels map[string]string) func(context.Context, client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + return func(_ context.Context, options client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + if options.Config == nil { + return client.ContainerCreateResult{}, errors.New("ContainerCreate(...): called without a container config") + } + if diff := cmp.Diff(labels, options.Config.Labels); diff != "" { + return client.ContainerCreateResult{}, errors.Errorf("ContainerCreate(...): -want labels, +got labels:\n%s", diff) + } + return client.ContainerCreateResult{ID: containerID}, nil + } + } + + type args struct { + cleanup DockerCleanup + cli *mockContainerClient + } + type want struct { + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "Stop": { + reason: "Function containers should be marked as managed by render and record the Stop policy.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupStop, + cli: &mockContainerClient{ + MockContainerCreate: createLabeledContainer(map[string]string{ + LabelKeyManagedBy: LabelValueManagedByCrossplane, + LabelKeyCleanup: string(AnnotationValueRuntimeDockerCleanupStop), + }), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + }, + }, + }, + "Remove": { + reason: "Function containers should be marked as managed by render and record the Remove policy.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + cli: &mockContainerClient{ + MockContainerCreate: createLabeledContainer(map[string]string{ + LabelKeyManagedBy: LabelValueManagedByCrossplane, + LabelKeyCleanup: string(AnnotationValueRuntimeDockerCleanupRemove), + }), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + }, + }, + }, + "Orphan": { + reason: "Function containers should record the Orphan policy so sweeps can skip them.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupOrphan, + cli: &mockContainerClient{ + MockContainerCreate: createLabeledContainer(map[string]string{ + LabelKeyManagedBy: LabelValueManagedByCrossplane, + LabelKeyCleanup: string(AnnotationValueRuntimeDockerCleanupOrphan), + }), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + }, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + 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: tc.args.cli, + } + + _, 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) + } + }) + } +} diff --git a/internal/docker/docker.go b/internal/docker/docker.go index 3ae056af..7db79ceb 100644 --- a/internal/docker/docker.go +++ b/internal/docker/docker.go @@ -26,6 +26,7 @@ import ( "encoding/json" "fmt" "io" + "maps" "path" "path/filepath" "slices" @@ -377,6 +378,16 @@ func RunWithExtraHosts(hosts []string) RunContainerOption { } } +// RunWithLabels adds labels to the container. +func RunWithLabels(labels map[string]string) RunContainerOption { + return func(cfg *runContainerConfig) { + if cfg.containerConfig.Labels == nil { + cfg.containerConfig.Labels = map[string]string{} + } + maps.Copy(cfg.containerConfig.Labels, labels) + } +} + // RunWithBindMount adds a bind mount to the container. func RunWithBindMount(hostPath, containerPath string) RunContainerOption { return func(cfg *runContainerConfig) { diff --git a/internal/docker/docker_test.go b/internal/docker/docker_test.go new file mode 100644 index 00000000..6b282bf1 --- /dev/null +++ b/internal/docker/docker_test.go @@ -0,0 +1,54 @@ +/* +Copyright 2026 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package docker + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/moby/moby/api/types/container" +) + +func TestRunWithLabels(t *testing.T) { + cases := map[string]struct { + reason string + existing map[string]string + labels map[string]string + want map[string]string + }{ + "NilLabels": { + reason: "Labels should be set when the container config has none.", + labels: map[string]string{"a": "1"}, + want: map[string]string{"a": "1"}, + }, + "MergeLabels": { + reason: "Labels should be merged into existing labels.", + existing: map[string]string{"a": "1"}, + labels: map[string]string{"b": "2"}, + want: map[string]string{"a": "1", "b": "2"}, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + cfg := &runContainerConfig{containerConfig: &container.Config{Labels: tc.existing}} + RunWithLabels(tc.labels)(cfg) + if diff := cmp.Diff(tc.want, cfg.containerConfig.Labels); diff != "" { + t.Errorf("\n%s\nRunWithLabels(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +}