Add Docker workload creation unit tests; extend dockerAPI seam (#1883)

Extend dockerAPI seam for create/start/remove

Route create/start/remove via dockerAPI to enable hermetic unit tests for createMcpContainer and createContainer. Update handleExistingContainer, removeContainer, and createContainer to use c.api. Adjust fakeDockerAPI to implement the extended interface.

Signed-off-by: Juan Antonio Osorio <[email protected]>

Author: JAORMX

pkg/container/docker/client.go

@@ -23,6 +23,7 @@ import (
 	"github.com/docker/docker/client"
 	"github.com/docker/docker/pkg/stdcopy"
 	"github.com/docker/go-connections/nat"
+	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 
 	"github.com/stacklok/toolhive/pkg/container/docker/sdk"
 	"github.com/stacklok/toolhive/pkg/container/images"
@@ -61,6 +62,61 @@ type dockerAPI interface {
 	ContainerList(ctx context.Context, options container.ListOptions) ([]container.Summary, error)
 	ContainerInspect(ctx context.Context, containerID string) (container.InspectResponse, error)
 	ContainerStop(ctx context.Context, containerID string, options container.StopOptions) error
+	ContainerCreate(
+		ctx context.Context,
+		config *container.Config,
+		hostConfig *container.HostConfig,
+		networkingConfig *network.NetworkingConfig,
+		platform *v1.Platform,
+		containerName string,
+	) (container.CreateResponse, error)
+	ContainerStart(ctx context.Context, containerID string, options container.StartOptions) error
+	ContainerRemove(ctx context.Context, containerID string, options container.RemoveOptions) error
+}
+
+// deployOps defines the internal operations used by DeployWorkload.
+// It allows unit tests to substitute a fake implementation to avoid hitting a real Docker daemon.
+type deployOps interface {
+	createExternalNetworks(ctx context.Context) error
+	createNetwork(ctx context.Context, name string, labels map[string]string, internal bool) error
+	createDnsContainer(
+		ctx context.Context,
+		dnsContainerName string,
+		attachStdio bool,
+		networkName string,
+		endpointsConfig map[string]*network.EndpointSettings,
+	) (string, string, error)
+	createEgressSquidContainer(
+		ctx context.Context,
+		containerName string,
+		squidContainerName string,
+		attachStdio bool,
+		exposedPorts map[string]struct{},
+		endpointsConfig map[string]*network.EndpointSettings,
+		perm *permissions.NetworkPermissions,
+	) (string, error)
+	createMcpContainer(
+		ctx context.Context,
+		name string,
+		networkName string,
+		image string,
+		command []string,
+		envVars map[string]string,
+		labels map[string]string,
+		attachStdio bool,
+		permissionConfig *runtime.PermissionConfig,
+		additionalDNS string,
+		exposedPorts map[string]struct{},
+		portBindings map[string][]runtime.PortBinding,
+		isolateNetwork bool,
+	) error
+	createIngressContainer(
+		ctx context.Context,
+		containerName string,
+		upstreamPort int,
+		attachStdio bool,
+		externalEndpointsConfig map[string]*network.EndpointSettings,
+	) (int, error)
 }
 
 // Client implements the Deployer interface for Docker (and compatible runtimes)
@@ -70,6 +126,7 @@ type Client struct {
 	client       *client.Client
 	api          dockerAPI
 	imageManager images.ImageManager
+	ops          deployOps
 }
 
 // NewClient creates a new container client
@@ -88,10 +145,25 @@ func NewClient(ctx context.Context) (*Client, error) {
 		api:          dockerClient,
 		imageManager: imageManager,
 	}
+	// Default ops implementation uses the real client methods.
+	c.ops = c
 
 	return c, nil
 }
 
+// createEgressSquidContainer wraps the package-level createEgressSquidContainer to satisfy deployOps.
+func (c *Client) createEgressSquidContainer(
+	ctx context.Context,
+	containerName string,
+	squidContainerName string,
+	attachStdio bool,
+	exposedPorts map[string]struct{},
+	endpointsConfig map[string]*network.EndpointSettings,
+	perm *permissions.NetworkPermissions,
+) (string, error) {
+	return createEgressSquidContainer(ctx, c, containerName, squidContainerName, attachStdio, exposedPorts, endpointsConfig, perm)
+}
+
 // DeployWorkload creates and starts a workload.
 // It configures the workload based on the provided permission profile and transport type.
 // If options is nil, default options will be used.
@@ -130,7 +202,7 @@ func (c *Client) DeployWorkload(
 		"toolhive-external": {},
 	}
 
-	err = c.createExternalNetworks(ctx)
+	err = c.ops.createExternalNetworks(ctx)
 	if err != nil {
 		return 0, fmt.Errorf("failed to create external networks: %v", err)
 	}
@@ -141,14 +213,14 @@ func (c *Client) DeployWorkload(
 
 		internalNetworkLabels := map[string]string{}
 		lb.AddNetworkLabels(internalNetworkLabels, networkName)
-		err := c.createNetwork(ctx, networkName, internalNetworkLabels, true)
+		err := c.ops.createNetwork(ctx, networkName, internalNetworkLabels, true)
 		if err != nil {
 			return 0, fmt.Errorf("failed to create internal network: %v", err)
 		}
 
 		// create dns container
 		dnsContainerName := fmt.Sprintf("%s-dns", name)
-		_, dnsContainerIP, err := c.createDnsContainer(ctx, dnsContainerName, attachStdio, networkName, externalEndpointsConfig)
+		_, dnsContainerIP, err := c.ops.createDnsContainer(ctx, dnsContainerName, attachStdio, networkName, externalEndpointsConfig)
 		if dnsContainerIP != "" {
 			additionalDNS = dnsContainerIP
 		}
@@ -158,9 +230,8 @@ func (c *Client) DeployWorkload(
 
 		// create egress container
 		egressContainerName := fmt.Sprintf("%s-egress", name)
-		_, err = createEgressSquidContainer(
+		_, err = c.ops.createEgressSquidContainer(
 			ctx,
-			c,
 			name,
 			egressContainerName,
 			attachStdio,
@@ -188,7 +259,7 @@ func (c *Client) DeployWorkload(
 	// about ingress/egress/dns containers.
 	lb.AddNetworkIsolationLabel(labels, networkIsolation)
 
-	err = c.createMcpContainer(
+	err = c.ops.createMcpContainer(
 		ctx,
 		name,
 		networkName,
@@ -218,7 +289,7 @@ func (c *Client) DeployWorkload(
 		if err != nil {
 			return 0, err // extractFirstPort already wraps the error with context.
 		}
-		hostPort, err = c.createIngressContainer(ctx, name, firstPortInt, attachStdio, externalEndpointsConfig)
+		hostPort, err = c.ops.createIngressContainer(ctx, name, firstPortInt, attachStdio, externalEndpointsConfig)
 		if err != nil {
 			return 0, fmt.Errorf("failed to create ingress container: %v", err)
 		}
@@ -952,7 +1023,7 @@ func (c *Client) handleExistingContainer(
 	desiredHostConfig *container.HostConfig,
 ) (bool, error) {
 	// Get container info
-	info, err := c.client.ContainerInspect(ctx, containerID)
+	info, err := c.api.ContainerInspect(ctx, containerID)
 	if err != nil {
 		return false, NewContainerError(err, containerID, fmt.Sprintf("failed to inspect container: %v", err))
 	}
@@ -964,7 +1035,7 @@ func (c *Client) handleExistingContainer(
 		// Check if the container is running
 		if !info.State.Running {
 			// Container exists but is not running, start it
-			err = c.client.ContainerStart(ctx, containerID, container.StartOptions{})
+			err = c.api.ContainerStart(ctx, containerID, container.StartOptions{})
 			if err != nil {
 				return false, NewContainerError(err, containerID, fmt.Sprintf("failed to start existing container: %v", err))
 			}
@@ -1040,7 +1111,7 @@ func (c *Client) deleteNetwork(ctx context.Context, name string) error {
 
 // removeContainer removes a container by ID, without removing any associated networks or proxy containers.
 func (c *Client) removeContainer(ctx context.Context, containerID string) error {
-	err := c.client.ContainerRemove(ctx, containerID, container.RemoveOptions{
+	err := c.api.ContainerRemove(ctx, containerID, container.RemoveOptions{
 		Force: true,
 	})
 	if err != nil {
@@ -1202,7 +1273,7 @@ func (c *Client) createContainer(
 	}
 
 	// Create the container
-	resp, err := c.client.ContainerCreate(
+	resp, err := c.api.ContainerCreate(
 		ctx,
 		config,
 		hostConfig,
@@ -1215,7 +1286,7 @@ func (c *Client) createContainer(
 	}
 
 	// Start the container
-	err = c.client.ContainerStart(ctx, resp.ID, container.StartOptions{})
+	err = c.api.ContainerStart(ctx, resp.ID, container.StartOptions{})
 	if err != nil {
 		return "", NewContainerError(err, resp.ID, fmt.Sprintf("failed to start container: %v", err))
 	}