Skip to content

Allow container socket paths to be configured via config file #4943

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kantord wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Allow container socket paths to be configured via config file #4943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2478aec
Allow container socket paths to be configured via config file
kantord Apr 20, 2026
dca6c82
Merge branch 'main' into issue-4612
kantord Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"gopkg.in/yaml.v3"

"github.com/stacklok/toolhive-core/env"
"github.com/stacklok/toolhive/pkg/container/runtime"
"github.com/stacklok/toolhive/pkg/container/templates"
"github.com/stacklok/toolhive/pkg/lockfile"
"github.com/stacklok/toolhive/pkg/secrets"
Expand Down Expand Up @@ -47,6 +48,7 @@ type Config struct {
BuildAuthFiles map[string]string `yaml:"build_auth_files,omitempty"`
RuntimeConfigs map[string]*templates.RuntimeConfig `yaml:"runtime_configs,omitempty"`
RegistryAuth RegistryAuth `yaml:"registry_auth,omitempty"`
ContainerRuntime runtime.SocketConfig `yaml:"container_runtime,omitempty"`
}

// RegistryAuthTypeOAuth is the auth type for OAuth/OIDC authentication.
Expand Down
31 changes: 30 additions & 1 deletion pkg/container/docker/sdk/client_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func newPlatformClient(socketPath string) (*http.Client, []client.Opt) {
}

// findPlatformContainerSocket finds a container socket path on Unix systems
func findPlatformContainerSocket(rt runtime.Type) (string, runtime.Type, error) {
func findPlatformContainerSocket(rt runtime.Type, overrides runtime.SocketConfig) (string, runtime.Type, error) {
// First check for custom socket paths via environment variables
if customSocketPath := os.Getenv(PodmanSocketEnv); customSocketPath != "" {
//nolint:gosec // G706: socket path from trusted environment variable
Expand Down Expand Up @@ -76,6 +76,11 @@ func findPlatformContainerSocket(rt runtime.Type) (string, runtime.Type, error)
return customSocketPath, runtime.TypeDocker, nil
}

// Check config file overrides (after env vars, before auto-detection)
Copy link
Copy Markdown
Contributor

@jhrozek jhrozek Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a correctness issue here. checkSocketConfigOverrides doesn't consider the rt argument, so whichever override field is non-empty wins for every iteration of the outer loop in NewDockerClient.

Concrete scenario: a user has a stale podman_socket: in config and Docker is reachable via auto-detect. Iter 1 (Podman) fails because the Podman path doesn't exist. Iter 2 should auto-detect Docker — but checkSocketConfigOverrides returns the same Podman error again, Docker auto-detect never runs, and NewDockerClient returns "no supported container runtime available: invalid Podman socket path from config" on a machine where Docker works fine.

Windows has the same shape at client_windows.go:92-99.

Fix I'd suggest: make the override check match the current rt — only look at overrides.PodmanSocket when rt == TypePodman, etc. Then a broken podman override only disables podman, and the outer loop's independent-per-runtime semantics work as intended. Side benefit: the "priority ordering" question (which field wins when multiple are set) disappears, since each runtime consults only its own field.

if p, runtimeType, err := checkSocketConfigOverrides(overrides); p != "" || err != nil {
return p, runtimeType, err
}

if rt == runtime.TypePodman {
socketPath, err := findPodmanSocket()
if err == nil {
Expand Down Expand Up @@ -254,3 +259,27 @@ func findColimaSocket() (string, error) {

return "", fmt.Errorf("colima socket not found in standard locations")
}

// checkSocketConfigOverrides checks config file socket overrides in priority order.
// Returns ("", "", nil) if no override applies.
func checkSocketConfigOverrides(overrides runtime.SocketConfig) (string, runtime.Type, error) {
if overrides.PodmanSocket != "" {
return resolveConfigSocket(overrides.PodmanSocket, runtime.TypePodman, "Podman")
}
if overrides.DockerSocket != "" {
return resolveConfigSocket(overrides.DockerSocket, runtime.TypeDocker, "Docker")
}
if overrides.ColimaSocket != "" {
return resolveConfigSocket(overrides.ColimaSocket, runtime.TypeDocker, "Colima")
Copy link
Copy Markdown
Contributor

@jhrozek jhrozek Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small inconsistency: the config override for Colima returns runtime.TypeDocker here, but the auto-detect branch at client_unix.go:98-103 returns runtime.TypeColima. The env-var branch at client_unix.go:74-76 also returns TypeDocker, so this PR is copying the existing pattern — but the disagreement with auto-detect means any caller that branches on runtime.TypeColima behaves differently depending on how the user configured Colima.

Either align all three branches (env + config to return TypeColima, or auto-detect to TypeDocker), or leave a comment explaining why the three disagree.

}
return "", "", nil
}

// resolveConfigSocket validates that a config-supplied socket path exists and returns it.
func resolveConfigSocket(socketPath string, rt runtime.Type, runtimeName string) (string, runtime.Type, error) {
slog.Debug("using socket from config", "runtime", runtimeName, "path", socketPath)
if _, err := os.Stat(socketPath); err != nil {
return "", rt, fmt.Errorf("invalid %s socket path from config: %w", runtimeName, err)
}
return socketPath, rt, nil
}
74 changes: 74 additions & 0 deletions pkg/container/docker/sdk/client_unix_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
// SPDX-License-Identifier: Apache-2.0

//go:build !windows

package sdk

import (
"os"
"path/filepath"
"testing"

"github.com/stretchr/testify/require"

"github.com/stacklok/toolhive/pkg/container/runtime"
)

func makeTempSocket(t *testing.T) string {
t.Helper()
p := filepath.Join(t.TempDir(), "test.sock")
require.NoError(t, os.WriteFile(p, nil, 0600))
return p
}

func TestFindContainerSocket_ConfigPath(t *testing.T) {
t.Parallel()

tests := []struct {
name string
makePath func(*testing.T) string
wantErr bool
}{
{
name: "valid path is used",
makePath: makeTempSocket,
wantErr: false,
},
{
name: "nonexistent path returns error",
makePath: func(t *testing.T) string {
t.Helper()
return filepath.Join(t.TempDir(), "nonexistent.sock")
},
wantErr: true,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
socketPath := tc.makePath(t)

_, _, err := findContainerSocket(runtime.TypeDocker, runtime.SocketConfig{DockerSocket: socketPath})

if tc.wantErr {
require.Error(t, err)
} else {
require.NoError(t, err)
}
})
}
}

func TestFindContainerSocket_EnvVarPrecedence(t *testing.T) {
// not parallel — t.Setenv cannot be called in parallel tests
envPath := makeTempSocket(t)
t.Setenv(DockerSocketEnv, envPath)

gotPath, gotRuntime, err := findContainerSocket(runtime.TypeDocker, runtime.SocketConfig{DockerSocket: makeTempSocket(t)})

require.NoError(t, err)
require.Equal(t, envPath, gotPath)
require.Equal(t, runtime.TypeDocker, gotRuntime)
}
24 changes: 23 additions & 1 deletion pkg/container/docker/sdk/client_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func newPlatformClient(pipePath string) (*http.Client, []client.Opt) {
}

// findPlatformContainerSocket finds a container socket path on Windows
func findPlatformContainerSocket(rt runtime.Type) (string, runtime.Type, error) {
func findPlatformContainerSocket(rt runtime.Type, overrides runtime.SocketConfig) (string, runtime.Type, error) {
// First check for custom socket paths via environment variables
if customPipePath := os.Getenv(PodmanSocketEnv); customPipePath != "" {
//nolint:gosec // G706: pipe path from trusted environment variable
Expand Down Expand Up @@ -89,6 +89,15 @@ func findPlatformContainerSocket(rt runtime.Type) (string, runtime.Type, error)
return customPipePath, runtime.TypeDocker, nil
}

// Check config file overrides (after env vars, before auto-detection).
// Colima is not supported on Windows.
if overrides.PodmanSocket != "" {
return resolveConfigPipe(overrides.PodmanSocket, runtime.TypePodman, "Podman")
}
if overrides.DockerSocket != "" {
return resolveConfigPipe(overrides.DockerSocket, runtime.TypeDocker, "Docker")
}

if rt == runtime.TypePodman {
// Try Podman named pipe with timeout
ctx, cancel := context.WithTimeout(context.Background(), pipeConnectionTimeout)
Expand Down Expand Up @@ -117,3 +126,16 @@ func findPlatformContainerSocket(rt runtime.Type) (string, runtime.Type, error)

return "", "", ErrRuntimeNotFound
}

// resolveConfigPipe validates that a config-supplied named pipe path is connectable and returns it.
func resolveConfigPipe(pipePath string, rt runtime.Type, runtimeName string) (string, runtime.Type, error) {
slog.Debug("using pipe from config", "runtime", runtimeName, "path", pipePath)
ctx, cancel := context.WithTimeout(context.Background(), pipeConnectionTimeout)
defer cancel()
conn, err := winio.DialPipeContext(ctx, pipePath)
if err != nil {
return "", rt, fmt.Errorf("invalid %s pipe path from config: %w", runtimeName, err)
}
conn.Close()
return pipePath, rt, nil
}
39 changes: 36 additions & 3 deletions pkg/container/docker/sdk/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@ import (
"context"
"fmt"
"log/slog"
"os"
"path/filepath"

"github.com/adrg/xdg"
"github.com/docker/docker/client"
"gopkg.in/yaml.v3"

"github.com/stacklok/toolhive/pkg/container/runtime"
)
Expand Down Expand Up @@ -49,15 +53,44 @@ const (

var supportedSocketPaths = []runtime.Type{runtime.TypePodman, runtime.TypeDocker, runtime.TypeColima}

// loadSocketOverrides reads socket path overrides from the ToolHive config file.
// Best-effort: returns empty overrides on any error so auto-detection takes over.
func loadSocketOverrides() runtime.SocketConfig {
Copy link
Copy Markdown
Contributor

@jhrozek jhrozek Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth routing this through the existing config Provider abstraction? pkg/config.NewProvider() dispatches to DefaultProvider / PathProvider / KubernetesProvider and honours RegisterProviderFactory for injected providers (see pkg/config/interface.go:225, :396, :566). Reading XDG directly means PathProvider consumers (custom config paths, tests) and any code using RegisterProviderFactory won't see this feature.

The reason to not just use NewProvider().GetConfig() is the "first-run creates the file + runs migrations" side effect — but in practice the CLI has almost always touched config by the time NewDockerClient runs, so the singleton is populated and no extra file creation happens. A read-only LoadIfExists() on the Provider interface would make the concern go away entirely.

configPath, err := xdg.ConfigFile("toolhive/config.yaml")
if err != nil {
slog.Debug("failed to resolve config path for socket overrides", "error", err)
return runtime.SocketConfig{}
}

// #nosec G304: path is derived from XDG config dir, not user input.
data, err := os.ReadFile(filepath.Clean(configPath))
if err != nil {
slog.Debug("failed to read config file for socket overrides", "error", err)
return runtime.SocketConfig{}
}

var cfg struct {
ContainerRuntime runtime.SocketConfig `yaml:"container_runtime,omitempty"`
}
if err := yaml.Unmarshal(data, &cfg); err != nil {
Copy link
Copy Markdown
Contributor

@jhrozek jhrozek Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadSocketOverrides has no tests, and it's the integration point that turns YAML into SocketConfig. It also has a deliberate "best-effort, return zero value on any error" contract across three distinct failure modes (xdg lookup, file read, yaml parse) — that contract should be pinned.

Worth a table-driven test that points XDG_CONFIG_HOME at t.TempDir() and covers: missing file, empty file, malformed YAML, valid file with each subset of fields populated, valid file with no container_runtime key, valid file with only unrelated keys. adrg/xdg provides xdg.Reload() if you hit caching issues on macOS. Non-parallel because of t.Setenv.

slog.Debug("failed to parse config file for socket overrides", "error", err)
return runtime.SocketConfig{}
}

return cfg.ContainerRuntime
}

// NewDockerClient creates a new container client
func NewDockerClient(ctx context.Context) (*client.Client, string, runtime.Type, error) {
var lastErr error

overrides := loadSocketOverrides()

// We try to find a container socket for the given runtime
// We try Podman first, then Docker as fallback
for _, sp := range supportedSocketPaths {
// Try to find a container socket for the given runtime
socketPath, runtimeType, err := findContainerSocket(sp)
socketPath, runtimeType, err := findContainerSocket(sp, overrides)
if err != nil {
//nolint:gosec // G706: runtime type from internal config
slog.Debug("failed to find socket", "runtime", sp, "error", err)
Expand Down Expand Up @@ -105,7 +138,7 @@ func newClientWithSocketPath(ctx context.Context, socketPath string) (*client.Cl
}

// findContainerSocket finds a container socket path, preferring Podman over Docker
func findContainerSocket(rt runtime.Type) (string, runtime.Type, error) {
func findContainerSocket(rt runtime.Type, overrides runtime.SocketConfig) (string, runtime.Type, error) {
// Use platform-specific implementation
return findPlatformContainerSocket(rt)
return findPlatformContainerSocket(rt, overrides)
}
9 changes: 9 additions & 0 deletions pkg/container/runtime/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,15 @@ type Monitor interface {
StopMonitoring()
}

// SocketConfig holds optional socket path overrides for container runtimes.
// When set, these paths take precedence over auto-detection but not over
// environment variables (TOOLHIVE_PODMAN_SOCKET, TOOLHIVE_DOCKER_SOCKET, etc.).
type SocketConfig struct {
PodmanSocket string `yaml:"podman_socket,omitempty"`
DockerSocket string `yaml:"docker_socket,omitempty"`
ColimaSocket string `yaml:"colima_socket,omitempty"`
}

// Type represents the type of container runtime
type Type string

Expand Down
Loading