mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
fix(cli): isolate root HTTP transports (#25430)
The CLI root client shared `http.DefaultTransport` for normal API requests and for the version-check build-info request. In parallel tests, other clients can close idle connections on that process-global transport, which can fail the Boundary license check before the AGPL 404 handling runs. `TestBoundaryLicenseVerification/AGPLDeployment` configures a proxy that returns `404` from `/api/v2/entitlements`, which `verifyLicense()` maps to the expected AGPL deployment error. However, `clitest.SetupConfig()` only writes the URL and session token to disk. It does not pass the test's isolated `proxyClient.HTTPClient` into the CLI invocation, so `coder boundary` builds a fresh client through `RootCmd.InitClient()`. Before this change, that fresh client used `http.DefaultTransport`; if another parallel test closed idle connections on the shared transport while the entitlement request was in flight, Go returned `http: CloseIdleConnections called` instead of the proxy's `404`. The command then failed with `failed to get entitlements`, and the test never reached the expected AGPL error path. Clone the default transport for each CLI root HTTP client and for the unwrapped build-info client, preserving the configured TLS settings when present. Each CLI invocation now gets its own transport instance, so cleanup from unrelated parallel tests cannot interrupt its entitlement or build-info requests. Closes https://github.com/coder/internal/issues/1538 <details> <summary>Coder Agents notes</summary> Generated by Coder Agents for Linear ENG-2705. Local validation: - `go test ./cli -run 'TestNewHTTPTransport|Test_ensureTLSConfig|Test_wrapTransportWithVersionCheck' -count=1` - `go test ./enterprise/cli -run TestBoundaryLicenseVerification/AGPLDeployment -count=20 -parallel=16` - `go test ./cli ./enterprise/cli` - `make lint` - `go test ./enterprise/cli -run '^TestBoundaryLicenseVerification$' -count=50 -parallel=16` - pre-commit hook during `git commit` </details>
This commit is contained in:
+28
-13
@@ -806,27 +806,23 @@ func (r *RootCmd) HeaderTransport(ctx context.Context, serverURL *url.URL) (*cod
|
||||
}
|
||||
|
||||
func (r *RootCmd) createHTTPClient(ctx context.Context, serverURL *url.URL, inv *serpent.Invocation) (*http.Client, error) {
|
||||
transport := http.DefaultTransport
|
||||
|
||||
// Apply custom TLS config if specified
|
||||
if r.tlsConfig != nil {
|
||||
// Clone the default transport and apply TLS config
|
||||
defaultTransport, ok := http.DefaultTransport.(*http.Transport)
|
||||
if !ok {
|
||||
return nil, xerrors.New("cannot apply TLS config: http.DefaultTransport is not *http.Transport")
|
||||
}
|
||||
customTransport := defaultTransport.Clone()
|
||||
customTransport.TLSClientConfig = r.tlsConfig
|
||||
transport = customTransport
|
||||
baseTransport, err := newHTTPTransport(r.tlsConfig)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
transport := baseTransport
|
||||
|
||||
transport = wrapTransportWithTelemetryHeader(transport, inv)
|
||||
transport = wrapTransportWithUserAgentHeader(transport, inv)
|
||||
if !r.noVersionCheck {
|
||||
buildInfoTransport, err := newHTTPTransport(r.tlsConfig)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
transport = wrapTransportWithVersionCheck(transport, inv, buildinfo.Version(), func(ctx context.Context) (codersdk.BuildInfoResponse, error) {
|
||||
// Create a new client without any wrapped transport
|
||||
// otherwise it creates an infinite loop!
|
||||
basicClient := codersdk.New(serverURL)
|
||||
basicClient := codersdk.New(serverURL, codersdk.WithHTTPClient(&http.Client{Transport: buildInfoTransport}))
|
||||
return basicClient.BuildInfo(ctx)
|
||||
})
|
||||
}
|
||||
@@ -846,6 +842,25 @@ func (r *RootCmd) createHTTPClient(ctx context.Context, serverURL *url.URL, inv
|
||||
}, nil
|
||||
}
|
||||
|
||||
func newHTTPTransport(tlsConfig *tls.Config) (http.RoundTripper, error) {
|
||||
defaultTransport, ok := http.DefaultTransport.(*http.Transport)
|
||||
if !ok {
|
||||
if tlsConfig != nil {
|
||||
return nil, xerrors.New("cannot apply TLS config: http.DefaultTransport is not *http.Transport")
|
||||
}
|
||||
return http.DefaultTransport, nil
|
||||
}
|
||||
|
||||
// Clone http.DefaultTransport for each CLI client. Parallel tests and
|
||||
// embedded callers may close idle connections on their own clients, and
|
||||
// sharing the process-global transport can interrupt in-flight requests.
|
||||
transport := defaultTransport.Clone()
|
||||
if tlsConfig != nil {
|
||||
transport.TLSClientConfig = tlsConfig
|
||||
}
|
||||
return transport, nil
|
||||
}
|
||||
|
||||
func (r *RootCmd) createUnauthenticatedClient(ctx context.Context, serverURL *url.URL, inv *serpent.Invocation) (*codersdk.Client, error) {
|
||||
// Load TLS config for login and other unauthenticated requests
|
||||
if err := r.ensureTLSConfig(); err != nil {
|
||||
|
||||
@@ -473,3 +473,25 @@ func Test_ensureTLSConfig(t *testing.T) {
|
||||
require.Contains(t, err.Error(), "failed to parse CA certificate")
|
||||
})
|
||||
}
|
||||
|
||||
func TestNewHTTPTransportClonesDefaultTransport(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
transport, err := newHTTPTransport(nil)
|
||||
require.NoError(t, err)
|
||||
require.NotSame(t, http.DefaultTransport, transport)
|
||||
require.IsType(t, &http.Transport{}, transport)
|
||||
}
|
||||
|
||||
func TestNewHTTPTransportAppliesTLSConfigToClone(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tlsConfig := &tls.Config{MinVersion: tls.VersionTLS13}
|
||||
transport, err := newHTTPTransport(tlsConfig)
|
||||
require.NoError(t, err)
|
||||
require.NotSame(t, http.DefaultTransport, transport)
|
||||
|
||||
httpTransport, ok := transport.(*http.Transport)
|
||||
require.True(t, ok)
|
||||
require.Same(t, tlsConfig, httpTransport.TLSClientConfig)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user