Merge pull request #11820 from liggitt/reuse_round_tripper

pkg/client/helper.go

@@ -27,6 +27,7 @@ import (
 	"reflect"
 	gruntime "runtime"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/golang/glog"
@@ -329,29 +330,45 @@ func RESTClientFor(config *Config) (*RESTClient, error) {
 	return client, nil
 }
 
-// TransportFor returns an http.RoundTripper that will provide the authentication
-// or transport level security defined by the provided Config. Will return the
-// default http.DefaultTransport if no special case behavior is needed.
-func TransportFor(config *Config) (http.RoundTripper, error) {
-	hasCA := len(config.CAFile) > 0 || len(config.CAData) > 0
-	hasCert := len(config.CertFile) > 0 || len(config.CertData) > 0
+var (
+	// tlsTransports stores reusable round trippers with custom TLSClientConfig options
+	tlsTransports = map[string]*http.Transport{}
 
-	// Set transport level security
-	if config.Transport != nil && (hasCA || hasCert || config.Insecure) {
-		return nil, fmt.Errorf("using a custom transport with TLS certificate options or the insecure flag is not allowed")
+	// tlsTransportLock protects retrieval and storage of round trippers into the tlsTransports map
+	tlsTransportLock sync.Mutex
+)
+
+// tlsTransportFor returns a http.RoundTripper for the given config, or an error
+// The same RoundTripper will be returned for configs with identical TLS options
+// If the config has no custom TLS options, http.DefaultTransport is returned
+func tlsTransportFor(config *Config) (http.RoundTripper, error) {
+	// Get a unique key for the TLS options in the config
+	key, err := tlsConfigKey(config)
+	if err != nil {
+		return nil, err
+	}
+
+	// Ensure we only create a single transport for the given TLS options
+	tlsTransportLock.Lock()
+	defer tlsTransportLock.Unlock()
+
+	// See if we already have a custom transport for this config
+	if cachedTransport, ok := tlsTransports[key]; ok {
+		return cachedTransport, nil
 	}
 
+	// Get the TLS options for this client config
 	tlsConfig, err := TLSConfigFor(config)
 	if err != nil {
 		return nil, err
 	}
+	// The options didn't require a custom TLS config
+	if tlsConfig == nil {
+		return http.DefaultTransport, nil
+	}
 
-	var transport http.RoundTripper
-	if config.Transport != nil {
-		transport = config.Transport
-	} else {
-		if tlsConfig != nil {
-			transport = &http.Transport{
+	// Cache a single transport for these options
+	tlsTransports[key] = &http.Transport{
 		TLSClientConfig: tlsConfig,
 		Proxy:           http.ProxyFromEnvironment,
 		Dial: (&net.Dialer{
@@ -360,11 +377,40 @@ func TransportFor(config *Config) (http.RoundTripper, error) {
 		}).Dial,
 		TLSHandshakeTimeout: 10 * time.Second,
 	}
+	return tlsTransports[key], nil
+}
+
+// TransportFor returns an http.RoundTripper that will provide the authentication
+// or transport level security defined by the provided Config. Will return the
+// default http.DefaultTransport if no special case behavior is needed.
+func TransportFor(config *Config) (http.RoundTripper, error) {
+	hasCA := len(config.CAFile) > 0 || len(config.CAData) > 0
+	hasCert := len(config.CertFile) > 0 || len(config.CertData) > 0
+
+	// Set transport level security
+	if config.Transport != nil && (hasCA || hasCert || config.Insecure) {
+		return nil, fmt.Errorf("using a custom transport with TLS certificate options or the insecure flag is not allowed")
+	}
+
+	var (
+		transport http.RoundTripper
+		err       error
+	)
+
+	if config.Transport != nil {
+		transport = config.Transport
 	} else {
-			transport = http.DefaultTransport
+		transport, err = tlsTransportFor(config)
+		if err != nil {
+			return nil, err
 		}
 	}
 
+	// Call wrap prior to adding debugging wrappers
+	if config.WrapTransport != nil {
+		transport = config.WrapTransport(transport)
+	}
+
 	switch {
 	case bool(glog.V(9)):
 		transport = NewDebuggingRoundTripper(transport, CurlCommand, URLTiming, ResponseHeaders)
@@ -376,10 +422,6 @@ func TransportFor(config *Config) (http.RoundTripper, error) {
 		transport = NewDebuggingRoundTripper(transport, URLTiming)
 	}
 
-	if config.WrapTransport != nil {
-		transport = config.WrapTransport(transport)
-	}
-
 	transport, err = HTTPWrappersForConfig(config, transport)
 	if err != nil {
 		return nil, err

pkg/client/helper_test.go

@@ -213,6 +213,66 @@ func TestTransportFor(t *testing.T) {
 	}
 }
 
+func TestTLSTransportCache(t *testing.T) {
+	// Empty the cache
+	tlsTransports = map[string]*http.Transport{}
+	// Construct several transports (Insecure=true to force a transport with custom tls settings)
+	identicalConfigurations := map[string]*Config{
+		"empty":          {Insecure: true},
+		"host":           {Insecure: true, Host: "foo"},
+		"prefix":         {Insecure: true, Prefix: "foo"},
+		"version":        {Insecure: true, Version: "foo"},
+		"codec":          {Insecure: true, Codec: latest.Codec},
+		"basic":          {Insecure: true, Username: "bob", Password: "password"},
+		"bearer":         {Insecure: true, BearerToken: "token"},
+		"user agent":     {Insecure: true, UserAgent: "useragent"},
+		"wrap transport": {Insecure: true, WrapTransport: func(http.RoundTripper) http.RoundTripper { return nil }},
+		"qps/burst":      {Insecure: true, QPS: 1.0, Burst: 10},
+	}
+	for k, v := range identicalConfigurations {
+		if _, err := TransportFor(v); err != nil {
+			t.Errorf("Unexpected error for %q: %v", k, err)
+		}
+	}
+	if len(tlsTransports) != 1 {
+		t.Errorf("Expected 1 cached transport, got %d", len(tlsTransports))
+	}
+
+	// Empty the cache
+	tlsTransports = map[string]*http.Transport{}
+	// Construct several transports with custom TLS settings
+	// (no normalization is performed on ca/cert/key data, so appending a newline lets us test "different" content)
+	uniqueConfigurations := map[string]*Config{
+		"insecure":                {Insecure: true},
+		"cadata 1":                {TLSClientConfig: TLSClientConfig{CAData: []byte(rootCACert)}},
+		"cadata 2":                {TLSClientConfig: TLSClientConfig{CAData: []byte(rootCACert + "\n")}},
+		"cert 1, key 1":           {TLSClientConfig: TLSClientConfig{CertData: []byte(certData), KeyData: []byte(keyData)}},
+		"cert 1, key 2":           {TLSClientConfig: TLSClientConfig{CertData: []byte(certData), KeyData: []byte(keyData + "\n")}},
+		"cert 2, key 1":           {TLSClientConfig: TLSClientConfig{CertData: []byte(certData + "\n"), KeyData: []byte(keyData)}},
+		"cert 2, key 2":           {TLSClientConfig: TLSClientConfig{CertData: []byte(certData + "\n"), KeyData: []byte(keyData + "\n")}},
+		"cadata 1, cert 1, key 1": {TLSClientConfig: TLSClientConfig{CAData: []byte(rootCACert), CertData: []byte(certData), KeyData: []byte(keyData)}},
+	}
+	for k, v := range uniqueConfigurations {
+		if _, err := TransportFor(v); err != nil {
+			t.Errorf("Unexpected error for %q: %v", k, err)
+		}
+	}
+	// All custom configs should result in a cache entry
+	if len(tlsTransports) != len(uniqueConfigurations) {
+		t.Errorf("Expected %d cached transports, got %d", len(uniqueConfigurations), len(tlsTransports))
+	}
+
+	// Empty the cache
+	tlsTransports = map[string]*http.Transport{}
+	if _, err := TransportFor(&Config{}); err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	// A client config with no TLS options should use http.DefaultTransport, not a cached custom transport
+	if len(tlsTransports) != 0 {
+		t.Errorf("Expected no cached transports, got %d", len(tlsTransports))
+	}
+}
+
 func TestIsConfigTransportTLS(t *testing.T) {
 	testCases := []struct {
 		Config       *Config

pkg/client/transport.go

@@ -117,6 +117,16 @@ func TLSConfigFor(config *Config) (*tls.Config, error) {
 	return tlsConfig, nil
 }
 
+// tlsConfigKey returns a unique key for tls.Config objects returned from TLSConfigFor
+func tlsConfigKey(config *Config) (string, error) {
+	// Make sure ca/key/cert content is loaded
+	if err := LoadTLSFiles(config); err != nil {
+		return "", err
+	}
+	// Only include the things that actually affect the tls.Config
+	return fmt.Sprintf("%v/%x/%x/%x", config.Insecure, config.CAData, config.CertData, config.KeyData), nil
+}
+
 // LoadTLSFiles copies the data from the CertFile, KeyFile, and CAFile fields into the CertData,
 // KeyData, and CAFile fields, or returns an error. If no error is returned, all three fields are
 // either populated or were empty to start.

pkg/client/transport_test.go

@@ -20,6 +20,8 @@ import (
 	"encoding/base64"
 	"net/http"
 	"testing"
+
+	"k8s.io/kubernetes/pkg/api/latest"
 )
 
 func TestUnsecuredTLSTransport(t *testing.T) {
@@ -99,3 +101,74 @@ func TestUserAgentRoundTripper(t *testing.T) {
 		t.Errorf("unexpected user agent header: %#v", rt.Request)
 	}
 }
+
+func TestTLSConfigKey(t *testing.T) {
+	// Make sure config fields that don't affect the tls config don't affect the cache key
+	identicalConfigurations := map[string]*Config{
+		"empty":          {},
+		"host":           {Host: "foo"},
+		"prefix":         {Prefix: "foo"},
+		"version":        {Version: "foo"},
+		"codec":          {Codec: latest.Codec},
+		"basic":          {Username: "bob", Password: "password"},
+		"bearer":         {BearerToken: "token"},
+		"user agent":     {UserAgent: "useragent"},
+		"transport":      {Transport: http.DefaultTransport},
+		"wrap transport": {WrapTransport: func(http.RoundTripper) http.RoundTripper { return nil }},
+		"qps/burst":      {QPS: 1.0, Burst: 10},
+	}
+	for nameA, valueA := range identicalConfigurations {
+		for nameB, valueB := range identicalConfigurations {
+			keyA, err := tlsConfigKey(valueA)
+			if err != nil {
+				t.Errorf("Unexpected error for %q: %v", nameA, err)
+				continue
+			}
+			keyB, err := tlsConfigKey(valueB)
+			if err != nil {
+				t.Errorf("Unexpected error for %q: %v", nameB, err)
+				continue
+			}
+			if keyA != keyB {
+				t.Errorf("Expected identical cache keys for %q and %q, got:\n\t%s\n\t%s", nameA, nameB, keyA, keyB)
+				continue
+			}
+		}
+	}
+
+	// Make sure config fields that affect the tls config affect the cache key
+	uniqueConfigurations := map[string]*Config{
+		"no tls":                  {},
+		"insecure":                {Insecure: true},
+		"cadata 1":                {TLSClientConfig: TLSClientConfig{CAData: []byte{1}}},
+		"cadata 2":                {TLSClientConfig: TLSClientConfig{CAData: []byte{2}}},
+		"cert 1, key 1":           {TLSClientConfig: TLSClientConfig{CertData: []byte{1}, KeyData: []byte{1}}},
+		"cert 1, key 2":           {TLSClientConfig: TLSClientConfig{CertData: []byte{1}, KeyData: []byte{2}}},
+		"cert 2, key 1":           {TLSClientConfig: TLSClientConfig{CertData: []byte{2}, KeyData: []byte{1}}},
+		"cert 2, key 2":           {TLSClientConfig: TLSClientConfig{CertData: []byte{2}, KeyData: []byte{2}}},
+		"cadata 1, cert 1, key 1": {TLSClientConfig: TLSClientConfig{CAData: []byte{1}, CertData: []byte{1}, KeyData: []byte{1}}},
+	}
+	for nameA, valueA := range uniqueConfigurations {
+		for nameB, valueB := range uniqueConfigurations {
+			// Don't compare to ourselves
+			if nameA == nameB {
+				continue
+			}
+
+			keyA, err := tlsConfigKey(valueA)
+			if err != nil {
+				t.Errorf("Unexpected error for %q: %v", nameA, err)
+				continue
+			}
+			keyB, err := tlsConfigKey(valueB)
+			if err != nil {
+				t.Errorf("Unexpected error for %q: %v", nameB, err)
+				continue
+			}
+			if keyA == keyB {
+				t.Errorf("Expected unique cache keys for %q and %q, got:\n\t%s\n\t%s", nameA, nameB, keyA, keyB)
+				continue
+			}
+		}
+	}
+}