Skip to content

Fix cluster-proxy memory leak by pooling proxied transports - #19

Open
RokibulHasan7 wants to merge 12 commits into
masterfrom
leak
Open

Fix cluster-proxy memory leak by pooling proxied transports#19
RokibulHasan7 wants to merge 12 commits into
masterfrom
leak

Conversation

@RokibulHasan7

@RokibulHasan7 RokibulHasan7 commented Jun 22, 2026 •
edited by tamalsaha
Loading

Copy link
Copy Markdown
Member

Problem

Every proxied request to a ClusterGateway built a brand-new http.Transport (via restclient.TransportFor) and, for cluster-proxy endpoints, a fresh konnectivity gRPC tunnel — none of which were pooled or reused. Under sustained proxy / exec / attach / port-forward traffic the process steadily accumulated transports, TLS state, and gRPC connections, leaking memory and connections/file descriptors.

This PR pools the cluster-proxy transports so one http.Transport is reused per credential instead of per request, then hardens the pooling, the credential rotation, and the cache lifecycle in response to two rounds of review.

Scope is limited to transport.go, clustergateway_proxy.go, and a go.mod/go.sum tidy. (The unit-test repair and CI fixes that surfaced alongside this work merged separately in #21; GitHub Actions SHA-pinning merged in #20.)

Commits

Initial pooling

  1. Fix memory leak — Create konnectivity tunnels through a process-wide stable DialHolder (ClusterProxyDialHolder) so client-go's transport cache can reuse one pooled http.Transport per credential across requests instead of allocating one per request.
  2. Read cluster-proxy TLS material once instead of per dial — Hoist GetClientTLSConfig (which reads CA/cert/key files from disk) out of the per-dial closure so the TLS material and gRPC transport credentials are loaded once per process for the pooled DialHolder.
  3. Fail closed when the cluster-proxy dial holder is unavailable — Surface the error instead of silently falling back to restclient.TransportFor(cfg) (which would reintroduce the per-request transport / the leak).
  4. Build the transport config once per proxied request — Compute the transport config once in ServeHTTP and pass it in, reusing it for the TLS and upgrader wrappers; transport.New(transportCfg) is equivalent to the prior restclient.TransportFor(cfg) for the non-cluster-proxy path.
  5. Pool the cluster-proxy SPDY upgrade transport per credential — Cache the exec/attach/port-forward http.Transport instead of rebuilding it on every request; non-cluster-proxy endpoints keep the per-request transport.

Review hardening

  1. Address review feedback on cluster-proxy transport pooling — Build the dial holder lazily under a mutex and memoize only on success (so a transient startup failure retries instead of bricking cluster-proxy until a restart); resolve the holder once in ServeHTTP and fail closed consistently for both the proxied and upgrade transports; key the upgrade-transport cache by cluster name with replace-on-rotation; apply cfg.Proxy to the non-cluster-proxy upgrade transport; build the upgrade tls.Config only on the non-pooled path.
  2. Reuse the pooled dial holder in NewConfigFromCluster — Stop calling DialerGetter (and re-reading the client TLS material) on every cluster-proxy request only to have ServeHTTP discard the result; resolve cfg.Dial from the pooled holder so no files are touched per request and every consumer of the config gets the pooled dialer.
  3. Drop the unreachable dial-holder nil check — ClusterProxyDialHolder never returns (nil, nil); remove the dead branch.
  4. Bound the cluster-proxy upgrade-transport cache — There is no cluster-deletion hook at this layer, so cap the cache and evict the least-recently-used entry (closing its idle connections) to prevent unbounded growth under cluster-name churn.
  5. Reload cluster-proxy cert and CA by content with a fallback — Replace the mtime-based client-cert-only reloader with one that reloads both the CA bundle and the client cert/key, throttled and compared by content (off the per-handshake path), keeping the last good material on a transient read error; each new tunnel gets a fresh tls.Config, so CA rotation no longer requires a restart.
  6. Close pooled konnectivity tunnels on credential rotation — Route tunnel dials through client-go's connrotation.Dialer and call CloseAll when the cert/CA content changes, so warm pooled tunnels re-dial with the new material instead of clinging to the old credential until they idle out.

Verification

  • go build ./pkg/... ./cmd/..., make vet, and go test ./pkg/apis/gateway/v1alpha1/ all pass on the rebased branch.

Notes / trade-offs

  • Client cert/key and CA rotation are now picked up live (no restart), and existing pooled tunnels are dropped on rotation so they re-handshake with the new material.

@tamalsaha tamalsaha changed the title Fix memory leak Fix cluster-proxy memory leak by pooling proxied transports Jun 26, 2026
@tamalsaha
tamalsaha force-pushed the leak branch 2 times, most recently from 65edf20 to 2be11fc Compare June 26, 2026 06:10
RokibulHasan7 and others added 11 commits June 26, 2026 12:56
@RokibulHasan7 @tamalsaha
Fix memory leak
dccb7a3 
Signed-off-by: RokibulHasan7 <mdrokibulhasan@appscode.com>
Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha
Read cluster-proxy TLS material once instead of per dial
5943fd5 
GetClientTLSConfig reads the CA/cert/key files from disk on every call. It
was being invoked inside the per-dial closure, so every new tunnel connection
re-read three files. Hoist it into the dialer-builder body so the TLS material
is loaded once when the dialer is constructed (i.e. once per process for the
pooled DialHolder). The gRPC transport credentials are likewise built once and
shared across dials.

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha
Fail closed when the cluster-proxy dial holder is unavailable
9351e6e 
Previously proxyTransportFor fell back to restclient.TransportFor(cfg) when the
DialHolder could not be obtained. That path bypasses the shared DialHolder and
reintroduces the per-request transport (the very leak this change fixes) without
any signal. Surface the error instead so the request fails loudly rather than
silently regressing to the leaking transport.

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha
Build the transport config once per proxied request
774c9f8 
ServeHTTP called cfg.TransportConfig() and proxyTransportFor() called it again
internally, so each request derived the transport config twice. Compute it once
in ServeHTTP and pass it into proxyTransportFor, which now also reuses it for the
TLS and upgrader wrappers. transport.New(transportCfg) is equivalent to the prior
restclient.TransportFor(cfg) for the non-cluster-proxy path.

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha
Pool the cluster-proxy SPDY upgrade transport per credential
c2d096b 
The upgrade (exec/attach/port-forward) path built a fresh http.Transport on
every request. Cache it in a process-wide map keyed by the TLS credential so
repeated upgrade requests to the same cluster reuse one transport. The shared
DialHolder dial routes connections by address, so the transport is safe to share
across clusters presenting the same credential. Non-cluster-proxy endpoints keep
the per-request transport.

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha
Address review feedback on cluster-proxy transport pooling
314d150 
- Don't latch a transient dial-holder build error forever: build the
  DialHolder lazily under a mutex and memoize only on success, so a
  startup race (certs not yet mounted) is retried instead of bricking
  cluster-proxy until a restart.
- Stop re-reading the client TLS material per request: NewConfigFromCluster
  now reuses the pooled DialHolder dial for the cluster-proxy endpoint
  instead of calling DialerGetter (and GetClientTLSConfig) on every request
  and discarding the result.
- Bound the upgrade (SPDY) transport cache: key it by cluster name and
  replace-on-credential-rotation (closing the stale transport's idle
  connections) instead of an unbounded, never-evicted per-credential map.
- Reload the cluster-proxy client cert/key via tls.Config.GetClientCertificate
  so rotation no longer requires a process restart, while keeping the TLS
  read off the per-dial hot path.
- Resolve the dial holder once in ServeHTTP and fail closed consistently for
  both the proxied and the upgrade transport, removing the duplicated
  endpoint-type branching and the fail-open/fail-closed mismatch.
- Replace the hand-rolled client-go tls cache-key copy with a private
  credential fingerprint used only for rotation detection.
- Apply cfg.Proxy to the non-cluster-proxy upgrade transport so SPDY
  (exec/attach/port-forward) honors a configured const-endpoint ProxyURL.
- Build the upgrade tls.Config only on the non-pooled path / cache miss
  instead of unconditionally per request.

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha
Reuse the pooled dial holder in NewConfigFromCluster
001e696 
For the cluster-proxy endpoint NewConfigFromCluster called DialerGetter(ctx)
on every request, which re-read the CA/cert/key files from disk and allocated
a fresh tunnel-dial closure. ServeHTTP then discarded that cfg.Dial: it injects
the shared singleton DialHolder via proxyTransportFor and uses dialHolder.Dial
for the upgrade transport. So the per-request TLS read was pure waste on the
hot path and added a transient-failure surface (a momentary unreadable cert
returned 500) that the memoized holder was built to avoid.

Resolve cfg.Dial from the pooled ClusterProxyDialHolder so no files are touched
per request and every consumer of the rest.Config gets the pooled dialer.

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha
Drop the unreachable dial-holder nil check
6d468f9 
ClusterProxyDialHolder returns either a non-nil holder with a nil error or a
nil holder with a non-nil error; it never returns (nil, nil). The explicit
`if dialHolder == nil` branch after a successful call was dead code that
implied a contract the function does not have.

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha
Bound the cluster-proxy upgrade-transport cache
bebfe7f 
The upgrade-transport map is keyed by cluster name and there is no
cluster-deletion hook at this layer, so a fleet whose cluster names churn over
time would accumulate one *http.Transport per name forever. Cap the cache and
evict the least-recently-used entry (closing its idle connections) when full,
so the upgrade path cannot reintroduce the unbounded growth this change set is
fixing.

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha
Reload cluster-proxy cert and CA by content with a fallback
dd3efc6 
The previous reloader only handled the client cert/key and triggered on
strict mtime ordering, so a same-second secret swap, clock skew, or an
mtime-preserving write would silently keep the stale (eventually expired)
cert; the CA bundle was pinned for the process lifetime; and it stat-ed the
files under a process-global mutex on every TLS handshake.

Replace it with reloadingTLS, which reloads both the CA bundle and the client
cert/key from disk at most once per interval, compares by content rather than
mtime, and keeps the last good material if a re-read transiently fails (e.g.
the brief window while a Kubernetes secret ..data symlink is swapped). Each new
konnectivity tunnel gets a fresh tls.Config carrying the current CA pool and
cert, so CA rotation no longer requires a restart. This drops the only
remaining use of sigs.k8s.io/apiserver-network-proxy/pkg/util, so go mod tidy
removes that dependency.

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha
Close pooled konnectivity tunnels on credential rotation
935a92b 
Updating GetClientCertificate/the CA pool only affects new TLS handshakes;
konnectivity tunnels already pooled by the http.Transport keep presenting the
old client cert (and trusting the old CA) until they idle out (~90s) or die, so
right after a rotation warm exec/attach/port-forward connections still use the
stale credential.

Route every tunnel dial through client-go's connrotation.Dialer and call
CloseAll from the reloader's onRotate hook when the cert or CA content changes,
so existing pooled tunnels are dropped and re-dial with the new material. This
reuses the same connection-rotation primitive client-go uses for its own
dynamic client certificates.

Signed-off-by: Tamal Saha <tamal@appscode.com>
@tamalsaha
Polish the cluster-proxy TLS reloader after review
937af47 
Address minor review findings on the rotation reloader:

- Reload the CA bundle and client cert/key independently: a transient failure
  reading one no longer discards a successfully re-read other (previously the
  whole pass bailed on the first error).
- Do not cache unparseable CA bytes: on an AppendCertsFromPEM failure keep the
  last good pool without storing the bad bytes, so a later revert to the
  original content does not look like a rotation.
- Invoke onRotate (which closes pooled tunnels) after releasing r.mu instead of
  while holding it, so a slow connection close cannot stall concurrent dials.
  reloadLocked now reports whether anything changed and the caller fires the hook.
- Add a test helper that resets the process-wide cluster-proxy caches so a test
  overriding DialerGetter is not affected by state memoized by an earlier test.
- Clarify the ServeHTTP comment: the dial-holder resolution returns the memoized
  holder (already resolved in NewConfigFromCluster) and its error branch is a
  defensive fail-closed guard.

Signed-off-by: Tamal Saha <tamal@appscode.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@RokibulHasan7 @tamalsaha