Skip to content

Bound the hand-rolled http.Transport pools outside networking.Build #6483

Description

@tgrunnagle

Summary

#6480 fixed the unbounded connection pool in networking.HttpClientBuilder.Build by setting IdleConnTimeout, MaxIdleConns and MaxIdleConnsPerHost. Several &http.Transport{} literals elsewhere in the tree construct their own transport with only the two timeouts set and keep-alives left on, so IdleConnTimeout is still zero — meaning an idle pooled connection never expires and a dropped client pins a socket plus its goroutine pair for the process lifetime.

Known sites:

  • pkg/auth/discovery/discovery.go (two)
  • pkg/oauthproto/discovery.go
  • pkg/oauthproto/dcr.go
  • pkg/auth/oauth/oidc.go

Already correct, for reference: pkg/webhook/client.go (bounded) and pkg/oauthproto/cimd/fetch.go (keep-alives disabled).

Related: wrappers that swallow CloseIdleConnections

http.Client.CloseIdleConnections type-asserts the outermost transport, so any wrapping RoundTripper that does not implement and forward the method silently makes the call a no-op. #6480 fixed networking.ValidatingTransport, the oauth2.Transport wrapper, and limitedBodyTransport, and added networking.IdleConnectionCloser as the named capability to assert against. These wrappers still re-hide the pool of a builder-built client:

  • bearerTokenTransportpkg/auth/dcr/resolver.go
  • oauthproto.UserAgentTransportpkg/auth/oauth/flow.go
  • auth.WrapTransportpkg/registry/api/shared.go
  • pkg/authz/authorizers/http/http_client.go

There are also three pre-existing anonymous interface{ CloseIdleConnections() } assertions in pkg/vmcp (client/client.go, headerforward/transport.go) that should assert against networking.IdleConnectionCloser instead.

Why this is separate

#6479 scoped itself to the shared builder, and these are now bounded in practice by that fix only where the client comes from Build. The hand-rolled transports are not. Fixing them is mechanical; the better long-term answer is probably routing them through the builder, which is a larger refactor.

A shared networking.ForwardCloseIdle helper would make the wrapper half hard to get wrong.

Context

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-triageIssue needs initial triage by a maintainer

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions