fix: resolve proxy transport false-positives and windows linter issues - #3900
fix: resolve proxy transport false-positives and windows linter issues#3900Piyush0049 wants to merge 1 commit into
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
The proxy transport logic fix is well-motivated and the guard correctly preserves the main code paths (Unix socket and Windows HTTP CONNECT). One edge case remains: bare-TCP proxy connection failures (without the proxyconnect tcp prefix) are now silently classified as target-host errors rather than proxy failures, because "connect: connection refused" was removed from proxyErrorPatterns. See inline comment for details.
|
|
||
| // Check for common proxy socket failure patterns | ||
| // Target TCP connection errors are target host errors, not proxy socket failures. | ||
| if strings.Contains(errStr, "dial tcp") && !strings.Contains(errStr, "proxyconnect tcp") { |
There was a problem hiding this comment.
[medium] Removal of "connect: connection refused" pattern may leave bare-TCP proxy failures undetected
The PR removes "connect: connection refused" from proxyErrorPatterns (previously the safety net for the case where a socket exists but nothing is listening). The replacement relies on two assumptions: (1) every genuine proxy connection failure will contain "proxyconnect tcp", and (2) every "dial tcp … connect: connection refused" without "proxyconnect tcp" is a target-host error.
Assumption (1) holds for standard Go net/http HTTP-CONNECT proxy paths — those always produce a proxyconnect tcp: … prefix. However if the proxy transport is configured as a direct TCP dial (not an HTTP CONNECT tunnel), Go wraps the failure without that prefix, yielding a bare "dial tcp 127.0.0.1:<port>: connect: connection refused" error. That string hits the new early-return guard at line 152 and returns false, so disableProxy() is never called and the agent keeps retrying against the dead proxy.
In practice this codebase appears to go through HTTP CONNECT, so the risk is low — but the gap is real and there is no test covering the bare-TCP proxy case. Consider re-adding the pattern with a tighter guard (e.g. only match when the error also contains "proxyconnect tcp" or "dial unix"), or add a comment documenting why bare-TCP proxy failures cannot occur here.
| Confidence | Score |
|---|---|
| 🟡 moderate | 60/100 |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The proxy transport fix (isProxySocketError) correctly distinguishes target-host TCP dial errors from upstream proxy tunnel failures using the "dial tcp" / "proxyconnect tcp" guard. The three new test cases cover the key boundary conditions. Windows linter suppressions (nolint:gosec, nolint:noctx) are appropriate for the contexts. The %w error-wrapping changes are valid under Go 1.26.5.
Lower-confidence findings (not posted inline)
-
[medium]
pkg/desktop/transport/transport.go:152—"connect: connection refused"removed fromproxyErrorPatterns— may affect proxy fallback when Unix socket exists but is not listening (confidence: 🟠 weak 52/100)The PR removes the
"connect: connection refused"pattern. For Unix socket errors, Go's net package typically formats them as"dial unix /path/to/socket: connect: connection refused", which still matches the remaining"dial unix"pattern — so on Linux/macOS this is likely safe. However, the safety of the removal depends on Go runtime error-message formatting across all supported platforms; if an OS or wrapper ever produces a bare"connect: connection refused"without the"dial unix"prefix, proxy socket outages during startup (socket file present, process not yet listening) would now fail hard rather than falling back to direct transport. Worth a manual cross-platform spot-check before merging.
Description
This pull request addresses a critical logic bug in the Docker Desktop proxy transport and resolves multiple linter failures specific to the Windows build.
Proxy Error Classification Fix
Before:
The
isProxySocketErrorfunction used a broad string match for"connect: connection refused". When an agent attempted to reach an offline target service (e.g.dial tcp 127.0.0.1:8080: connect: connection refused), the transport misclassified this target host failure as a dead Docker Desktop proxy socket. This falsely triggered a 30-second global disablement of the proxy, routing all subsequent outbound agent traffic directly and bypassing Docker Desktop networking.After:
Added an explicit guard to exclude direct target TCP dial errors. The function now correctly distinguishes between target host connection failures (
dial tcp) and upstream proxy tunnel failures (proxyconnect tcp), ensuring the proxy is only disabled during genuine proxy socket outages.Windows Linter Fixes
Resolved 7 linting issues on the Windows build:
errorlintwarnings by changing%vto%winfmt.Errorfcalls. Addednolint:noctxannotation forexec.Commandre-exec behavior.nolint:gosecannotations forunsafe.PointerWin32 struct conversions and process PIDuint32conversions.nolint:gosecannotations for identical Windows Job Object API calls.require.NoErrorandrequire.NotNilto satisfy thestaticcheck SA5011control-flow analyzer.Testing
transport_test.goto explicitly verify target TCP refusal and proxy tunnel refusal logic.golangci-lint runpasses with 0 issues across the entire repository.go test ./...passes cleanly on all modified packages.