Revert "Add GC to client-go TLS cache"

This reverts commit fa9a1fe.

Author: jacobsee

cmd/kube-apiserver/app/testing/testserver.go

@@ -54,6 +54,7 @@ import (
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/kubernetes"
 	restclient "k8s.io/client-go/rest"
+	clientgotransport "k8s.io/client-go/transport"
 	"k8s.io/client-go/util/cert"
 	"k8s.io/client-go/util/keyutil"
 	basecompatibility "k8s.io/component-base/compatibility"
@@ -398,6 +399,12 @@ func StartTestServer(t ktesting.TB, instanceOptions *TestServerInstanceOptions,
 
 	if instanceOptions.EnableCertAuth {
 		if featureGate.Enabled(genericfeatures.UnknownVersionInteroperabilityProxy) {
+			// TODO: set up a general clean up for testserver
+			if clientgotransport.DialerStopCh == wait.NeverStop {
+				ctx, cancel := context.WithTimeout(context.Background(), time.Hour)
+				t.Cleanup(cancel)
+				clientgotransport.DialerStopCh = ctx.Done()
+			}
 			s.PeerCAFile = filepath.Join(s.SecureServing.ServerCert.CertDirectory, s.SecureServing.ServerCert.PairName+".crt")
 		}
 	}

staging/src/k8s.io/apiserver/pkg/util/proxy/streamtranslator_test.go

@@ -971,11 +971,20 @@ func v4WriteStatusFunc(stream io.Writer) func(status *apierrors.StatusError) err
 	}
 }
 
-func fakeTransport() (http.RoundTripper, error) {
+func fakeTransport() (*http.Transport, error) {
 	cfg := &transport.Config{
 		TLS: transport.TLSConfig{
 			Insecure: true,
+			CAFile:   "",
 		},
 	}
-	return transport.New(cfg)
+	rt, err := transport.New(cfg)
+	if err != nil {
+		return nil, err
+	}
+	t, ok := rt.(*http.Transport)
+	if !ok {
+		return nil, fmt.Errorf("unknown transport type: %T", rt)
+	}
+	return t, nil
 }

staging/src/k8s.io/client-go/features/known_features.go

@@ -55,13 +55,6 @@ const (
 	// "application/json" or "application/apply-patch+yaml", respectively.
 	ClientsAllowCBOR Feature = "ClientsAllowCBOR"
 
-	// owner: @enj
-	// beta: v1.36
-	//
-	// If enabled, the client-go TLS transport cache uses weak pointers to allow
-	// garbage collection of unused transports, preventing unbounded cache growth.
-	ClientsAllowTLSCacheGC Feature = "ClientsAllowTLSCacheGC"
-
 	// owner: @benluddy
 	// kep: https://kep.k8s.io/4222
 	// alpha: 1.32
@@ -118,9 +111,6 @@ var defaultVersionedKubernetesFeatureGates = map[Feature]VersionedSpecs{
 	ClientsAllowCBOR: {
 		{Version: version.MustParse("1.32"), Default: false, PreRelease: Alpha},
 	},
-	ClientsAllowTLSCacheGC: {
-		{Version: version.MustParse("1.36"), Default: true, PreRelease: Beta},
-	},
 	ClientsPreferCBOR: {
 		{Version: version.MustParse("1.32"), Default: false, PreRelease: Alpha},
 	},

staging/src/k8s.io/client-go/tools/metrics/metrics.go

@@ -80,7 +80,7 @@ type TransportCacheMetric interface {
 }
 
 // TransportCreateCallsMetric counts the number of times a transport is created
-// partitioned by the result of the cache: hit, miss, miss-gc, uncacheable
+// partitioned by the result of the cache: hit, miss, uncacheable
 type TransportCreateCallsMetric interface {
 	Increment(result string)
 }
@@ -91,18 +91,6 @@ type TransportCAReloadsMetric interface {
 	Increment(result, reason string)
 }
 
-// TransportCertRotationGCCallsMetric counts the number of times a cert rotation
-// goroutine cancel func is called via GC cleanup.
-type TransportCertRotationGCCallsMetric interface {
-	Increment()
-}
-
-// TransportCacheGCCallsMetric counts the number of times a GC cleanup
-// attempts to delete a cache entry, partitioned by the result: deleted, skipped.
-type TransportCacheGCCallsMetric interface {
-	Increment(result string)
-}
-
 var (
 	// ClientCertExpiry is the expiry time of a client certificate
 	ClientCertExpiry ExpiryMetric = noopExpiry{}
@@ -135,34 +123,27 @@ var (
 	// TransportCreateCalls is the metric that counts the number of times a new transport
 	// is created
 	TransportCreateCalls TransportCreateCallsMetric = noopTransportCreateCalls{}
+
 	// TransportCAReloads is the metric that counts the number of times a CA reload is attempted
 	TransportCAReloads TransportCAReloadsMetric = noopTransportCAReloads{}
-	// TransportCertRotationGCCalls counts the number of times a cert rotation goroutine
-	// cancel func is called via GC cleanup
-	TransportCertRotationGCCalls TransportCertRotationGCCallsMetric = noopTransportCertRotationGCCalls{}
-	// TransportCacheGCCalls counts the number of times a GC cleanup attempts
-	// to delete a transport cache entry, partitioned by result: deleted, skipped.
-	TransportCacheGCCalls TransportCacheGCCallsMetric = noopTransportCacheGCCalls{}
 )
 
 // RegisterOpts contains all the metrics to register. Metrics may be nil.
 type RegisterOpts struct {
-	ClientCertExpiry             ExpiryMetric
-	ClientCertRotationAge        DurationMetric
-	RequestLatency               LatencyMetric
-	ResolverLatency              ResolverLatencyMetric
-	RequestSize                  SizeMetric
-	ResponseSize                 SizeMetric
-	RateLimiterLatency           LatencyMetric
-	RequestResult                ResultMetric
-	ExecPluginCalls              CallsMetric
-	ExecPluginPolicyCalls        PolicyCallsMetric
-	RequestRetry                 RetryMetric
-	TransportCacheEntries        TransportCacheMetric
-	TransportCreateCalls         TransportCreateCallsMetric
-	TransportCAReloads           TransportCAReloadsMetric
-	TransportCertRotationGCCalls TransportCertRotationGCCallsMetric
-	TransportCacheGCCalls        TransportCacheGCCallsMetric
+	ClientCertExpiry      ExpiryMetric
+	ClientCertRotationAge DurationMetric
+	RequestLatency        LatencyMetric
+	ResolverLatency       ResolverLatencyMetric
+	RequestSize           SizeMetric
+	ResponseSize          SizeMetric
+	RateLimiterLatency    LatencyMetric
+	RequestResult         ResultMetric
+	ExecPluginCalls       CallsMetric
+	ExecPluginPolicyCalls PolicyCallsMetric
+	RequestRetry          RetryMetric
+	TransportCacheEntries TransportCacheMetric
+	TransportCreateCalls  TransportCreateCallsMetric
+	TransportCAReloads    TransportCAReloadsMetric
 }
 
 // Register registers metrics for the rest client to use. This can
@@ -211,12 +192,6 @@ func Register(opts RegisterOpts) {
 		if opts.TransportCAReloads != nil {
 			TransportCAReloads = opts.TransportCAReloads
 		}
-		if opts.TransportCertRotationGCCalls != nil {
-			TransportCertRotationGCCalls = opts.TransportCertRotationGCCalls
-		}
-		if opts.TransportCacheGCCalls != nil {
-			TransportCacheGCCalls = opts.TransportCacheGCCalls
-		}
 	})
 }
 
@@ -268,11 +243,3 @@ func (noopTransportCreateCalls) Increment(string) {}
 type noopTransportCAReloads struct{}
 
 func (noopTransportCAReloads) Increment(result, reason string) {}
-
-type noopTransportCertRotationGCCalls struct{}
-
-func (noopTransportCertRotationGCCalls) Increment() {}
-
-type noopTransportCacheGCCalls struct{}
-
-func (noopTransportCacheGCCalls) Increment(string) {}

staging/src/k8s.io/client-go/transport/ca_rotation_test.go

@@ -252,7 +252,7 @@ func TestCARotationConnectionBehavior(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Failed to create transport: %v", err)
 	}
-	transport.(*trackedTransport).rt.(*atomicTransportHolder).caRefreshDuration = 500 * time.Millisecond
+	transport.(*atomicTransportHolder).caRefreshDuration = 500 * time.Millisecond
 
 	client := &http.Client{
 		Transport: transport,

staging/src/k8s.io/client-go/transport/cache.go

@@ -21,14 +21,12 @@ import (
 	"fmt"
 	"net"
 	"net/http"
-	"runtime"
 	"strings"
 	"sync"
 	"time"
-	"weak"
 
 	utilnet "k8s.io/apimachinery/pkg/util/net"
-	clientgofeaturegate "k8s.io/client-go/features"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/tools/metrics"
 	"k8s.io/klog/v2"
 )
@@ -37,20 +35,19 @@ import (
 // same RoundTripper will be returned for configs with identical TLS options If
 // the config has no custom TLS options, http.DefaultTransport is returned.
 type tlsTransportCache struct {
-	mu               sync.Mutex
-	transports       map[tlsCacheKey]weak.Pointer[trackedTransport] // GC-enabled
-	strongTransports map[tlsCacheKey]http.RoundTripper              // GC-disabled
+	mu         sync.Mutex
+	transports map[tlsCacheKey]http.RoundTripper
 }
 
-const idleConnsPerHost = 25
+// DialerStopCh is stop channel that is passed down to dynamic cert dialer.
+// It's exposed as variable for testing purposes to avoid testing for goroutine
+// leakages.
+var DialerStopCh = wait.NeverStop
 
-var tlsCache = newTLSCache()
+const idleConnsPerHost = 25
 
-func newTLSCache() *tlsTransportCache {
-	return &tlsTransportCache{
-		transports:       make(map[tlsCacheKey]weak.Pointer[trackedTransport]),
-		strongTransports: make(map[tlsCacheKey]http.RoundTripper),
-	}
+var tlsCache = &tlsTransportCache{
+	transports: make(map[tlsCacheKey]http.RoundTripper),
 }
 
 type tlsCacheKey struct {
@@ -88,18 +85,14 @@ func (c *tlsTransportCache) get(config *Config) (http.RoundTripper, error) {
 		// Ensure we only create a single transport for the given TLS options
 		c.mu.Lock()
 		defer c.mu.Unlock()
-		defer func() { metrics.TransportCacheEntries.Observe(c.lenLocked()) }()
+		defer metrics.TransportCacheEntries.Observe(len(c.transports))
 
 		// See if we already have a custom transport for this config
-		if t, ok := c.getLocked(key); ok {
-			if t != nil {
-				metrics.TransportCreateCalls.Increment("hit")
-				return t, nil
-			}
-			metrics.TransportCreateCalls.Increment("miss-gc")
-		} else {
-			metrics.TransportCreateCalls.Increment("miss")
+		if t, ok := c.transports[key]; ok {
+			metrics.TransportCreateCalls.Increment("hit")
+			return t, nil
 		}
+		metrics.TransportCreateCalls.Increment("miss")
 	} else {
 		metrics.TransportCreateCalls.Increment("uncacheable")
 	}
@@ -126,17 +119,14 @@ func (c *tlsTransportCache) get(config *Config) (http.RoundTripper, error) {
 
 	// If we use are reloading files, we need to handle certificate rotation properly
 	// TODO(jackkleeman): We can also add rotation here when config.HasCertCallback() is true
-	var cancel context.CancelFunc
 	if config.TLS.ReloadTLSFiles && tlsConfig != nil && tlsConfig.GetClientCertificate != nil {
 		// The TLS cache is a singleton, so sharing the same name for all of its
 		// background activity seems okay.
 		logger := klog.Background().WithName("tls-transport-cache")
 		dynamicCertDialer := certRotatingDialer(logger, tlsConfig.GetClientCertificate, dial)
 		tlsConfig.GetClientCertificate = dynamicCertDialer.GetClientCertificate
 		dial = dynamicCertDialer.connDialer.DialContext
-		var ctx context.Context
-		ctx, cancel = context.WithCancel(context.Background())
-		go dynamicCertDialer.run(ctx.Done())
+		go dynamicCertDialer.run(DialerStopCh)
 	}
 
 	proxy := http.ProxyFromEnvironment
@@ -158,95 +148,12 @@ func (c *tlsTransportCache) get(config *Config) (http.RoundTripper, error) {
 		transport = newAtomicTransportHolder(config.TLS.CAFile, config.TLS.CAData, httpTransport)
 	}
 
-	if !canCache && cancel == nil {
-		return transport, nil // uncacheable config with no cert rotation - nothing to GC
-	}
-
-	if !clientgofeaturegate.FeatureGates().Enabled(clientgofeaturegate.ClientsAllowTLSCacheGC) {
-		if canCache {
-			c.strongTransports[key] = transport
-		}
-		return transport, nil // cancel is intentionally discarded and the cert rotation go routine leaks
-	}
-
-	transportWithGC := &trackedTransport{rt: transport}
-
-	if cancel != nil {
-		// capture metric as local var so that cleanups do not influence other tests via globals
-		transportCertRotationGCCalls := metrics.TransportCertRotationGCCalls
-		runtime.AddCleanup(transportWithGC, func(_ struct{}) {
-			cancel()
-			transportCertRotationGCCalls.Increment()
-		}, struct{}{})
-	}
-
 	if canCache {
-		wp := weak.Make(transportWithGC)
-		c.transports[key] = wp
-		// capture metrics as local vars so that cleanups do not influence other tests via globals
-		transportCacheGCCalls := metrics.TransportCacheGCCalls
-		transportCacheEntries := metrics.TransportCacheEntries
-		runtime.AddCleanup(transportWithGC, func(key tlsCacheKey) {
-			c.mu.Lock()
-			defer c.mu.Unlock()
-
-			// make sure we only delete the weak pointer created by this specific setLocked call
-			if c.transports[key] != wp {
-				transportCacheGCCalls.Increment("skipped")
-				return
-			}
-			delete(c.transports, key)
-			transportCacheGCCalls.Increment("deleted")
-			transportCacheEntries.Observe(c.lenLocked())
-		}, key)
-	}
-
-	return transportWithGC, nil
-}
-
-func (c *tlsTransportCache) getLocked(key tlsCacheKey) (http.RoundTripper, bool) {
-	if !clientgofeaturegate.FeatureGates().Enabled(clientgofeaturegate.ClientsAllowTLSCacheGC) {
-		v, ok := c.strongTransports[key]
-		return v, ok
+		// Cache a single transport for these options
+		c.transports[key] = transport
 	}
 
-	wp, ok := c.transports[key]
-	if !ok {
-		return nil, false
-	}
-
-	v := wp.Value()
-
-	if v == nil { // avoid typed nil
-		return nil, true // key exists but value has been garbage collected
-	}
-
-	return v, true
-}
-
-func (c *tlsTransportCache) lenLocked() int {
-	if !clientgofeaturegate.FeatureGates().Enabled(clientgofeaturegate.ClientsAllowTLSCacheGC) {
-		return len(c.strongTransports)
-	}
-	return len(c.transports)
-}
-
-// trackedTransport wraps an http.RoundTripper to serve as the weak.Pointer
-// target in the TLS transport cache. Dropping all references to this object
-// triggers GC cleanup of the cache entry and any cert rotation goroutine.
-type trackedTransport struct {
-	rt http.RoundTripper
-}
-
-var _ http.RoundTripper = &trackedTransport{}
-var _ utilnet.RoundTripperWrapper = &trackedTransport{}
-
-func (v *trackedTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	return v.rt.RoundTrip(req)
-}
-
-func (v *trackedTransport) WrappedRoundTripper() http.RoundTripper {
-	return v.rt
+	return transport, nil
 }
 
 // tlsConfigKey returns a unique key for tls.Config objects returned from TLSConfigFor