diff --git a/pkg/container/docker/client.go b/pkg/container/docker/client.go index 96a9aa5af..b50b641b3 100644 --- a/pkg/container/docker/client.go +++ b/pkg/container/docker/client.go @@ -256,10 +256,28 @@ func (c *Client) DeployWorkload( networkName = "" } - // only remap if is not an auxiliary tool - newPortBindings, hostPort, err := generatePortBindings(labels, options.PortBindings) - if err != nil { - return 0, fmt.Errorf("failed to generate port bindings: %v", err) + // Handle port binding generation differently for host vs bridge networking + var hostPort int + var newPortBindings map[string][]runtime.PortBinding + if permissionConfig.NetworkMode == "host" { + // For host networking, the container binds directly to the host port + // The port was set in MCP_PORT environment variable by the transport layer + mcpPort, ok := envVars["MCP_PORT"] + if !ok { + return 0, fmt.Errorf("MCP_PORT not found in environment variables for host networking") + } + hostPort, err = strconv.Atoi(mcpPort) + if err != nil { + return 0, fmt.Errorf("failed to parse MCP_PORT for host networking: %v", err) + } + newPortBindings = options.PortBindings // Use as-is (should be empty for host networking) + logger.Infof("Host networking: container listening on port %d (from MCP_PORT), will return this port", hostPort) + } else { + // For bridge/default networking, generate random port mappings + newPortBindings, hostPort, err = generatePortBindings(labels, options.PortBindings) + if err != nil { + return 0, fmt.Errorf("failed to generate port bindings: %v", err) + } } // Add a label to the MCP server indicating network isolation. @@ -1425,9 +1443,13 @@ func (c *Client) createMcpContainer(ctx context.Context, name string, networkNam return NewContainerError(err, "", err.Error()) } - // Setup port bindings - if err := setupPortBindings(hostConfig, portBindings); err != nil { - return NewContainerError(err, "", err.Error()) + // Setup port bindings only for non-host networking + // Host networking mode is incompatible with port bindings as the container + // uses the host's network stack directly + if permissionConfig.NetworkMode != "host" { + if err := setupPortBindings(hostConfig, portBindings); err != nil { + return NewContainerError(err, "", err.Error()) + } } // create mcp container diff --git a/pkg/container/docker/client_deploy_test.go b/pkg/container/docker/client_deploy_test.go index b1760faa2..b1a8f536e 100644 --- a/pkg/container/docker/client_deploy_test.go +++ b/pkg/container/docker/client_deploy_test.go @@ -308,3 +308,142 @@ func TestDeployWorkload_UnsupportedTransport_PropagatesError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "unsupported transport type") } + +func TestDeployWorkload_HostNetworking_UsesMCPPortAndSkipsExternalNetwork(t *testing.T) { + t.Parallel() + + fops := &fakeDeployOps{} + c := newClientWithOps(fops) + + opts := runtime.NewDeployWorkloadOptions() + opts.ExposedPorts = map[string]struct{}{"8080/tcp": {}} + // For host networking, port bindings should be empty (no mapping needed) + opts.PortBindings = map[string][]runtime.PortBinding{} + + labels := map[string]string{} + envVars := map[string]string{ + "MCP_PORT": "9876", // This port should be used as the host port + "EXISTING": "value", + } + + // Create permission profile with host network mode + profile := &permissions.Profile{ + Network: &permissions.NetworkPermissions{ + Mode: "host", + }, + } + + hostPort, err := c.DeployWorkload( + t.Context(), + "ghcr.io/example/mcp:latest", + "hostnet", + []string{"serve"}, + envVars, + labels, + profile, + "sse", + opts, + false, // no network isolation (typical for host mode) + ) + require.NoError(t, err) + + // Verify host port comes from MCP_PORT environment variable + assert.Equal(t, 9876, hostPort) + + // Verify external network creation was skipped + assert.False(t, fops.externalNetworksCalled, "external network creation should be skipped for host networking") + + // Verify no auxiliary containers were created + assert.False(t, fops.dnsCalled, "DNS container should not be created for host networking") + assert.False(t, fops.egressCalled, "egress container should not be created for host networking") + assert.False(t, fops.ingressCalled, "ingress container should not be created for host networking") + + // Verify MCP container was created with correct network mode + require.True(t, fops.mcpCalled) + require.NotNil(t, fops.mcpPermissionCfg) + assert.Equal(t, "host", fops.mcpPermissionCfg.NetworkMode) + + // Port bindings should be empty for host networking + assert.Empty(t, fops.mcpPortBindings) + + // Environment should include MCP_PORT + assert.Equal(t, "9876", fops.mcpEnvVars["MCP_PORT"]) +} + +func TestDeployWorkload_HostNetworking_ErrorsIfMCPPortMissing(t *testing.T) { + t.Parallel() + + fops := &fakeDeployOps{} + c := newClientWithOps(fops) + + opts := runtime.NewDeployWorkloadOptions() + opts.ExposedPorts = map[string]struct{}{"8080/tcp": {}} + opts.PortBindings = map[string][]runtime.PortBinding{} + + envVars := map[string]string{ + // MCP_PORT is missing + "EXISTING": "value", + } + + profile := &permissions.Profile{ + Network: &permissions.NetworkPermissions{ + Mode: "host", + }, + } + + _, err := c.DeployWorkload( + t.Context(), + "ghcr.io/example/mcp:latest", + "hostnet-no-port", + []string{"serve"}, + envVars, + map[string]string{}, + profile, + "sse", + opts, + false, + ) + + // Should error when MCP_PORT is missing for host networking + require.Error(t, err) + assert.Contains(t, err.Error(), "MCP_PORT not found in environment variables for host networking") +} + +func TestDeployWorkload_HostNetworking_ErrorsIfMCPPortInvalid(t *testing.T) { + t.Parallel() + + fops := &fakeDeployOps{} + c := newClientWithOps(fops) + + opts := runtime.NewDeployWorkloadOptions() + opts.ExposedPorts = map[string]struct{}{"8080/tcp": {}} + opts.PortBindings = map[string][]runtime.PortBinding{} + + envVars := map[string]string{ + "MCP_PORT": "not-a-number", // Invalid port value + "EXISTING": "value", + } + + profile := &permissions.Profile{ + Network: &permissions.NetworkPermissions{ + Mode: "host", + }, + } + + _, err := c.DeployWorkload( + t.Context(), + "ghcr.io/example/mcp:latest", + "hostnet-bad-port", + []string{"serve"}, + envVars, + map[string]string{}, + profile, + "sse", + opts, + false, + ) + + // Should error when MCP_PORT is not a valid number + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to parse MCP_PORT for host networking") +} diff --git a/pkg/transport/http.go b/pkg/transport/http.go index 2bf91ddcc..9234b647d 100644 --- a/pkg/transport/http.go +++ b/pkg/transport/http.go @@ -176,6 +176,17 @@ func (t *HTTPTransport) Setup( } envVars["MCP_TRANSPORT"] = env + // Check if we're using host networking mode + isHostNetworking := permissionProfile.Network != nil && permissionProfile.Network.Mode == "host" + + if isHostNetworking { + // For host networking, both proxy and container ports are directly accessible on the host. + // No Docker port mapping is needed - both ports can coexist on 127.0.0.1 + // The runner has already set proxyPort and targetPort to different values. + logger.Infof("Host networking detected, proxy on port %d will forward to container on port %d", + t.proxyPort, t.targetPort) + } + // Use the target port for the container's environment variables envVars["MCP_PORT"] = fmt.Sprintf("%d", t.targetPort) envVars["FASTMCP_PORT"] = fmt.Sprintf("%d", t.targetPort) @@ -186,28 +197,32 @@ func (t *HTTPTransport) Setup( containerOptions.K8sPodTemplatePatch = k8sPodTemplatePatch containerOptions.IgnoreConfig = ignoreConfig - // Expose the target port in the container - containerPortStr := fmt.Sprintf("%d/tcp", t.targetPort) - containerOptions.ExposedPorts[containerPortStr] = struct{}{} - - // Create host port bindings (configurable through the --host flag) - portBindings := []rt.PortBinding{ - { - HostIP: t.host, - HostPort: fmt.Sprintf("%d", t.targetPort), - }, - } + // Only set up port bindings and exposed ports for non-host networking + // Host networking doesn't support port bindings as containers use the host's network stack directly + if !isHostNetworking { + // Expose the target port in the container + containerPortStr := fmt.Sprintf("%d/tcp", t.targetPort) + containerOptions.ExposedPorts[containerPortStr] = struct{}{} + + // Create host port bindings (configurable through the --host flag) + portBindings := []rt.PortBinding{ + { + HostIP: t.host, + HostPort: fmt.Sprintf("%d", t.targetPort), + }, + } - // Check if IPv6 is available and add IPv6 localhost binding (commented out for now) - //if networking.IsIPv6Available() { - // portBindings = append(portBindings, rt.PortBinding{ - // HostIP: "::1", // IPv6 localhost - // HostPort: fmt.Sprintf("%d", t.targetPort), - // }) - //} + // Check if IPv6 is available and add IPv6 localhost binding (commented out for now) + //if networking.IsIPv6Available() { + // portBindings = append(portBindings, rt.PortBinding{ + // HostIP: "::1", // IPv6 localhost + // HostPort: fmt.Sprintf("%d", t.targetPort), + // }) + //} - // Set the port bindings - containerOptions.PortBindings[containerPortStr] = portBindings + // Set the port bindings + containerOptions.PortBindings[containerPortStr] = portBindings + } // For SSE transport, we don't need to attach stdio containerOptions.AttachStdio = false @@ -229,7 +244,7 @@ func (t *HTTPTransport) Setup( if err != nil { return fmt.Errorf("failed to create container: %v", err) } - logger.Infof("Container created: %s", containerName) + logger.Infof("Container created: %s, exposedPort returned: %d", containerName, exposedPort) if (t.Mode() == types.TransportTypeSSE || t.Mode() == types.TransportTypeStreamableHTTP) && rt.IsKubernetesRuntime() { // If the SSEHeadlessServiceName is set, use it as the target host diff --git a/pkg/transport/http_test.go b/pkg/transport/http_test.go new file mode 100644 index 000000000..b7d33f699 --- /dev/null +++ b/pkg/transport/http_test.go @@ -0,0 +1,404 @@ +package transport + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + rt "github.com/stacklok/toolhive/pkg/container/runtime" + "github.com/stacklok/toolhive/pkg/container/runtime/mocks" + "github.com/stacklok/toolhive/pkg/permissions" + "github.com/stacklok/toolhive/pkg/transport/types" +) + +func TestHTTPTransport_Setup_HostNetworking(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + transportType types.TransportType + permissionProfile *permissions.Profile + isolateNetwork bool + expectedEnvVars map[string]string + expectedExposedPort int + checkFunc func(t *testing.T, options *rt.DeployWorkloadOptions) + }{ + { + name: "host networking mode - skips port bindings", + transportType: types.TransportTypeSSE, + permissionProfile: &permissions.Profile{ + Network: &permissions.NetworkPermissions{ + Mode: "host", + }, + }, + isolateNetwork: false, + expectedEnvVars: map[string]string{ + "MCP_TRANSPORT": "sse", + "MCP_PORT": "8080", + "FASTMCP_PORT": "8080", + "MCP_HOST": "127.0.0.1", + }, + expectedExposedPort: 9876, // Different from target port to test proper handling + checkFunc: func(t *testing.T, options *rt.DeployWorkloadOptions) { + t.Helper() + // For host networking, port bindings should be empty + assert.Empty(t, options.PortBindings, "port bindings should be empty for host networking") + // Exposed ports should also be empty for host networking + assert.Empty(t, options.ExposedPorts, "exposed ports should be empty for host networking") + }, + }, + { + name: "bridge networking mode - sets port bindings", + transportType: types.TransportTypeSSE, + permissionProfile: &permissions.Profile{ + Network: &permissions.NetworkPermissions{ + Mode: "bridge", + }, + }, + isolateNetwork: false, + expectedEnvVars: map[string]string{ + "MCP_TRANSPORT": "sse", + "MCP_PORT": "8080", + "FASTMCP_PORT": "8080", + "MCP_HOST": "127.0.0.1", + }, + expectedExposedPort: 9876, + checkFunc: func(t *testing.T, options *rt.DeployWorkloadOptions) { + t.Helper() + // For bridge networking, port bindings should be set + assert.NotEmpty(t, options.PortBindings, "port bindings should not be empty for bridge networking") + assert.Contains(t, options.PortBindings, "8080/tcp", "should have binding for port 8080") + // Exposed ports should be set + assert.NotEmpty(t, options.ExposedPorts, "exposed ports should not be empty for bridge networking") + assert.Contains(t, options.ExposedPorts, "8080/tcp", "should expose port 8080") + }, + }, + { + name: "default networking mode - sets port bindings", + transportType: types.TransportTypeSSE, + permissionProfile: &permissions.Profile{ + Network: &permissions.NetworkPermissions{ + Mode: "default", + }, + }, + isolateNetwork: false, + expectedEnvVars: map[string]string{ + "MCP_TRANSPORT": "sse", + "MCP_PORT": "8080", + "FASTMCP_PORT": "8080", + "MCP_HOST": "127.0.0.1", + }, + expectedExposedPort: 9876, + checkFunc: func(t *testing.T, options *rt.DeployWorkloadOptions) { + t.Helper() + // For default networking, port bindings should be set + assert.NotEmpty(t, options.PortBindings, "port bindings should not be empty for default networking") + assert.Contains(t, options.PortBindings, "8080/tcp", "should have binding for port 8080") + // Exposed ports should be set + assert.NotEmpty(t, options.ExposedPorts, "exposed ports should not be empty for default networking") + assert.Contains(t, options.ExposedPorts, "8080/tcp", "should expose port 8080") + }, + }, + { + name: "empty network mode - sets port bindings", + transportType: types.TransportTypeSSE, + permissionProfile: &permissions.Profile{ + Network: nil, // No network permissions specified + }, + isolateNetwork: false, + expectedEnvVars: map[string]string{ + "MCP_TRANSPORT": "sse", + "MCP_PORT": "8080", + "FASTMCP_PORT": "8080", + "MCP_HOST": "127.0.0.1", + }, + expectedExposedPort: 9876, + checkFunc: func(t *testing.T, options *rt.DeployWorkloadOptions) { + t.Helper() + // For empty/nil networking, port bindings should be set (defaults to bridge-like behavior) + assert.NotEmpty(t, options.PortBindings, "port bindings should not be empty for empty network mode") + assert.Contains(t, options.PortBindings, "8080/tcp", "should have binding for port 8080") + // Exposed ports should be set + assert.NotEmpty(t, options.ExposedPorts, "exposed ports should not be empty for empty network mode") + assert.Contains(t, options.ExposedPorts, "8080/tcp", "should expose port 8080") + }, + }, + { + name: "host networking with streamable transport", + transportType: types.TransportTypeStreamableHTTP, + permissionProfile: &permissions.Profile{ + Network: &permissions.NetworkPermissions{ + Mode: "host", + }, + }, + isolateNetwork: false, + expectedEnvVars: map[string]string{ + "MCP_TRANSPORT": "streamable-http", + "MCP_PORT": "8080", + "FASTMCP_PORT": "8080", + "MCP_HOST": "127.0.0.1", + }, + expectedExposedPort: 9876, + checkFunc: func(t *testing.T, options *rt.DeployWorkloadOptions) { + t.Helper() + // For host networking, port bindings should be empty + assert.Empty(t, options.PortBindings, "port bindings should be empty for host networking") + assert.Empty(t, options.ExposedPorts, "exposed ports should be empty for host networking") + }, + }, + } + + for _, tt := range tests { + tt := tt // capture range variable + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + // Create a mock deployer + mockDeployer := mocks.NewMockDeployer(ctrl) + + // Create HTTP transport + transport := NewHTTPTransport( + tt.transportType, + "127.0.0.1", // host + 9090, // proxyPort + 8080, // targetPort + mockDeployer, + false, // debug + "127.0.0.1", // targetHost + nil, // authInfoHandler + nil, // prometheusHandler + ) + + // Set up expectations for DeployWorkload + var capturedOptions *rt.DeployWorkloadOptions + var capturedEnvVars map[string]string + + mockDeployer.EXPECT(). + DeployWorkload( + gomock.Any(), // ctx + "test-image", // image + "test-container", // name + []string{"serve"}, // command + gomock.Any(), // envVars - will capture + map[string]string{}, // labels + tt.permissionProfile, // permissionProfile + tt.transportType.String(), // transportType + gomock.Any(), // options - will capture + tt.isolateNetwork, // isolateNetwork + ). + DoAndReturn(func( + _ context.Context, + _, _ string, + _ []string, + envVars, _ map[string]string, + _ *permissions.Profile, + _ string, + options *rt.DeployWorkloadOptions, + _ bool, + ) (int, error) { + // Capture the options and envVars for inspection + capturedOptions = options + capturedEnvVars = envVars + return tt.expectedExposedPort, nil + }) + + // Call Setup + err := transport.Setup( + context.Background(), + mockDeployer, + "test-container", + "test-image", + []string{"serve"}, + map[string]string{}, // Initial envVars + map[string]string{}, // labels + tt.permissionProfile, + "", // k8sPodTemplatePatch + tt.isolateNetwork, + nil, // ignoreConfig + ) + + require.NoError(t, err) + + // Verify environment variables + for key, expectedValue := range tt.expectedEnvVars { + assert.Equal(t, expectedValue, capturedEnvVars[key], "env var %s should match", key) + } + + // Run custom check function + if tt.checkFunc != nil { + tt.checkFunc(t, capturedOptions) + } + + // Verify targetPort is set correctly (for non-Kubernetes runtime) + assert.Equal(t, tt.expectedExposedPort, transport.targetPort, "targetPort should be updated from exposedPort") + }) + } +} + +func TestHTTPTransport_Setup_HostNetworking_WithCustomHost(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockDeployer := mocks.NewMockDeployer(ctrl) + + // Create HTTP transport with custom host + transport := NewHTTPTransport( + types.TransportTypeSSE, + "192.168.1.100", // Custom host IP + 9090, // proxyPort + 8080, // targetPort + mockDeployer, + false, // debug + "10.0.0.1", // Custom target host + nil, // authInfoHandler + nil, // prometheusHandler + ) + + profile := &permissions.Profile{ + Network: &permissions.NetworkPermissions{ + Mode: "host", + }, + } + + // Set up expectations + var capturedOptions *rt.DeployWorkloadOptions + var capturedEnvVars map[string]string + + mockDeployer.EXPECT(). + DeployWorkload( + gomock.Any(), + "test-image", + "test-container", + []string{"serve"}, + gomock.Any(), + map[string]string{}, + profile, + "sse", + gomock.Any(), + false, + ). + DoAndReturn(func( + _ context.Context, + _, _ string, + _ []string, + envVars, _ map[string]string, + _ *permissions.Profile, + _ string, + options *rt.DeployWorkloadOptions, + _ bool, + ) (int, error) { + capturedOptions = options + capturedEnvVars = envVars + return 9876, nil + }) + + // Call Setup + err := transport.Setup( + context.Background(), + mockDeployer, + "test-container", + "test-image", + []string{"serve"}, + map[string]string{}, + map[string]string{}, + profile, + "", + false, + nil, + ) + + require.NoError(t, err) + + // Verify MCP_HOST uses the custom target host + assert.Equal(t, "10.0.0.1", capturedEnvVars["MCP_HOST"], "MCP_HOST should use custom target host") + + // For host networking, port bindings should be empty even with custom host + assert.Empty(t, capturedOptions.PortBindings, "port bindings should be empty for host networking") + assert.Empty(t, capturedOptions.ExposedPorts, "exposed ports should be empty for host networking") +} + +func TestHTTPTransport_Setup_UnsupportedTransportType(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockDeployer := mocks.NewMockDeployer(ctrl) + + // Create HTTP transport with an unsupported transport type + // We need to create a custom transport type for this test + unsupportedType := types.TransportType("unsupported") + transport := &HTTPTransport{ + transportType: unsupportedType, + host: "127.0.0.1", + proxyPort: 9090, + targetPort: 8080, + targetHost: "127.0.0.1", + deployer: mockDeployer, + } + + profile := &permissions.Profile{} + + // Call Setup - should fail due to unsupported transport type + err := transport.Setup( + context.Background(), + mockDeployer, + "test-container", + "test-image", + []string{"serve"}, + map[string]string{}, + map[string]string{}, + profile, + "", + false, + nil, + ) + + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported transport type") +} + +func TestHTTPTransport_Setup_RemoteURL(t *testing.T) { + t.Parallel() + + // Create HTTP transport + transport := NewHTTPTransport( + types.TransportTypeSSE, + "127.0.0.1", + 9090, + 8080, + nil, // No deployer needed for remote + false, + "127.0.0.1", + nil, + nil, + ) + + // Set remote URL + transport.SetRemoteURL("https://remote-mcp-server.com") + + // Call Setup - should succeed without calling deployer + err := transport.Setup( + context.Background(), + nil, // No deployer needed + "remote-container", + "", // No image for remote + nil, // No command for remote + map[string]string{}, + map[string]string{}, + &permissions.Profile{}, + "", + false, + nil, + ) + + require.NoError(t, err) + assert.Equal(t, "remote-container", transport.containerName) +}