Surfaced during the refactor review of #612. Same antipattern family as the bugs fixed by #606, #607, #609 — multiple readers of the same Docker-host configuration that can drift.
Today's smells in core/adapters/docker/client.go
- Dual
DOCKER_HOST readers. NewClientWithConfig reads os.Getenv("DOCKER_HOST") to compute the normalized host for client.WithHost(...) and createHTTPClient(...). Meanwhile client.FromEnv is still in the SDK options list and reads DOCKER_HOST independently inside the SDK. Same pattern as the original #605 bug. If env changes between the two reads (t.Setenv cross-test contamination, parallel goroutines, dynamic reconfig), SDK and transport disagree.
createHTTPClient re-derives the host via resolveDockerHost(config.Host) (or resolveHostForTransport in #613) instead of being passed the already-resolved values. Caller has the work; callee redoes it.
- Two sources of truth for the scheme allow-list:
var supportedDockerHostSchemes = []string{...} and the switch in createHTTPClient. Adding a new scheme (e.g. wss://) requires touching both. A map[string]schemeKind (where schemeKind is kindUnix | kindTLS | kindPlain | kindNamedPipe) collapses them.
- Magic strings for scheme names appear in: the allow-list, the switch,
strings.TrimPrefix(host, "unix://"), formatSupportedSchemes, and tests. const ( schemeUnix = "unix"; ... ) once.
- Test boilerplate —
TestNewClientWithConfig_UnsupportedSchemes and TestValidateAndNormalizeHost duplicate the unsupported-schemes list.
formatSupportedSchemes recomputes per error. Cache once into var supportedSchemesMsg.
- The
WithHTTPClient / FromEnv TLS material clobber from #607 is the same root pattern (multiple option-ordering writers on the SDK client). The unified seam is the natural place to subsume that path too.
Suggested refactor (single PR)
Introduce one source of truth:
type dockerHostConfig struct {
URL string // resolved + normalized, e.g. "tcp://host:2375"
Scheme string // lowercase, e.g. "tcp"
SocketPath string // for unix:// only
TLSConfig *tls.Config // populated by resolveDockerHost when env / config asks for it
}
func resolveDockerHost(cfg *ClientConfig) (dockerHostConfig, error) { ... }
NewClientWithConfig calls resolveDockerHost once and threads the result through client.WithHost, createHTTPClient, and the TLS branch. Drop client.FromEnv from opts (we already mirror its host + TLS resolution) so there is no second reader.
The dialer-selection switch becomes a map[string]func(transport, dockerHostConfig) so the allow-list is implicit.
Acceptance
Severity
Medium — pure refactor. No new behavior, no security implications. Sequence: land #606, #611, #612, #613 first, then refactor on top so the bisect story for each fix stays clean.
Surfaced during the refactor review of #612. Same antipattern family as the bugs fixed by #606, #607, #609 — multiple readers of the same Docker-host configuration that can drift.
Today's smells in
core/adapters/docker/client.goDOCKER_HOSTreaders.NewClientWithConfigreadsos.Getenv("DOCKER_HOST")to compute the normalized host forclient.WithHost(...)andcreateHTTPClient(...). Meanwhileclient.FromEnvis still in the SDK options list and readsDOCKER_HOSTindependently inside the SDK. Same pattern as the original #605 bug. If env changes between the two reads (t.Setenvcross-test contamination, parallel goroutines, dynamic reconfig), SDK and transport disagree.createHTTPClientre-derives the host viaresolveDockerHost(config.Host)(orresolveHostForTransportin #613) instead of being passed the already-resolved values. Caller has the work; callee redoes it.var supportedDockerHostSchemes = []string{...}and theswitchincreateHTTPClient. Adding a new scheme (e.g.wss://) requires touching both. Amap[string]schemeKind(whereschemeKindiskindUnix | kindTLS | kindPlain | kindNamedPipe) collapses them.strings.TrimPrefix(host, "unix://"),formatSupportedSchemes, and tests.const ( schemeUnix = "unix"; ... )once.TestNewClientWithConfig_UnsupportedSchemesandTestValidateAndNormalizeHostduplicate the unsupported-schemes list.formatSupportedSchemesrecomputes per error. Cache once intovar supportedSchemesMsg.WithHTTPClient/FromEnvTLS material clobber from #607 is the same root pattern (multiple option-ordering writers on the SDK client). The unified seam is the natural place to subsume that path too.Suggested refactor (single PR)
Introduce one source of truth:
NewClientWithConfigcallsresolveDockerHostonce and threads the result throughclient.WithHost,createHTTPClient, and the TLS branch. Dropclient.FromEnvfrom opts (we already mirror its host + TLS resolution) so there is no second reader.The dialer-selection switch becomes a
map[string]func(transport, dockerHostConfig)so the allow-list is implicit.Acceptance
DOCKER_HOSTper construction. Verified by a unit test that assertsos.Getenvis called at most once duringNewClientWithConfig.unsupportedSchemestable.WithHTTPClientclobbers TLS config fromDOCKER_TLS_VERIFY/DOCKER_CERT_PATH#607 / Docker SDK adapter: unsupportedDOCKER_HOSTschemes silently fall through to plain-TCP transport #609.Severity
Medium — pure refactor. No new behavior, no security implications. Sequence: land #606, #611, #612, #613 first, then refactor on top so the bisect story for each fix stays clean.