From 80fbc817263de1c46e1493819aca35b1ef8e3d09 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Sat, 9 Oct 2021 00:17:04 +0200 Subject: [PATCH] RESTClient contructors for config and http client Add two new constructors for versioned and unversioned RESTClients. These new constructors allow to pass an http.Client to the RESTClient, taking precence over the transport Config parameters. Add a new helper function to generate an http.Client from the RESTClient Config object. Co-authored-by: Jordan Liggitt --- staging/src/k8s.io/client-go/rest/config.go | 79 +++++++++++++------ .../k8s.io/client-go/rest/connection_test.go | 31 ++++++++ .../src/k8s.io/client-go/rest/transport.go | 21 +++++ 3 files changed, 105 insertions(+), 26 deletions(-) diff --git a/staging/src/k8s.io/client-go/rest/config.go b/staging/src/k8s.io/client-go/rest/config.go index e179e012cda..4909dc53abb 100644 --- a/staging/src/k8s.io/client-go/rest/config.go +++ b/staging/src/k8s.io/client-go/rest/config.go @@ -305,6 +305,8 @@ type ContentConfig struct { // object. Note that a RESTClient may require fields that are optional when initializing a Client. // A RESTClient created by this method is generic - it expects to operate on an API that follows // the Kubernetes conventions, but may not be the Kubernetes API. +// RESTClientFor is equivalent to calling RESTClientForConfigAndClient(config, httpClient), +// where httpClient was generated with HTTPClientFor(config). func RESTClientFor(config *Config) (*RESTClient, error) { if config.GroupVersion == nil { return nil, fmt.Errorf("GroupVersion is required when initializing a RESTClient") @@ -313,24 +315,40 @@ func RESTClientFor(config *Config) (*RESTClient, error) { return nil, fmt.Errorf("NegotiatedSerializer is required when initializing a RESTClient") } + // Validate config.Host before constructing the transport/client so we can fail fast. + // ServerURL will be obtained later in RESTClientForConfigAndClient() + _, _, err := defaultServerUrlFor(config) + if err != nil { + return nil, err + } + + httpClient, err := HTTPClientFor(config) + if err != nil { + return nil, err + } + + return RESTClientForConfigAndClient(config, httpClient) +} + +// RESTClientForConfigAndClient returns a RESTClient that satisfies the requested attributes on a +// client Config object. +// Unlike RESTClientFor, RESTClientForConfigAndClient allows to pass an http.Client that is shared +// between all the API Groups and Versions. +// Note that the http client takes precedence over the transport values configured. +// The http client defaults to the `http.DefaultClient` if nil. +func RESTClientForConfigAndClient(config *Config, httpClient *http.Client) (*RESTClient, error) { + if config.GroupVersion == nil { + return nil, fmt.Errorf("GroupVersion is required when initializing a RESTClient") + } + if config.NegotiatedSerializer == nil { + return nil, fmt.Errorf("NegotiatedSerializer is required when initializing a RESTClient") + } + baseURL, versionedAPIPath, err := defaultServerUrlFor(config) if err != nil { return nil, err } - transport, err := TransportFor(config) - if err != nil { - return nil, err - } - - var httpClient *http.Client - if transport != http.DefaultTransport { - httpClient = &http.Client{Transport: transport} - if config.Timeout > 0 { - httpClient.Timeout = config.Timeout - } - } - rateLimiter := config.RateLimiter if rateLimiter == nil { qps := config.QPS @@ -371,24 +389,33 @@ func UnversionedRESTClientFor(config *Config) (*RESTClient, error) { return nil, fmt.Errorf("NegotiatedSerializer is required when initializing a RESTClient") } + // Validate config.Host before constructing the transport/client so we can fail fast. + // ServerURL will be obtained later in UnversionedRESTClientForConfigAndClient() + _, _, err := defaultServerUrlFor(config) + if err != nil { + return nil, err + } + + httpClient, err := HTTPClientFor(config) + if err != nil { + return nil, err + } + + return UnversionedRESTClientForConfigAndClient(config, httpClient) +} + +// UnversionedRESTClientForConfigAndClient is the same as RESTClientForConfigAndClient, +// except that it allows the config.Version to be empty. +func UnversionedRESTClientForConfigAndClient(config *Config, httpClient *http.Client) (*RESTClient, error) { + if config.NegotiatedSerializer == nil { + return nil, fmt.Errorf("NegotiatedSerializer is required when initializing a RESTClient") + } + baseURL, versionedAPIPath, err := defaultServerUrlFor(config) if err != nil { return nil, err } - transport, err := TransportFor(config) - if err != nil { - return nil, err - } - - var httpClient *http.Client - if transport != http.DefaultTransport { - httpClient = &http.Client{Transport: transport} - if config.Timeout > 0 { - httpClient.Timeout = config.Timeout - } - } - rateLimiter := config.RateLimiter if rateLimiter == nil { qps := config.QPS diff --git a/staging/src/k8s.io/client-go/rest/connection_test.go b/staging/src/k8s.io/client-go/rest/connection_test.go index e58aff194a4..70fd2aa1ba4 100644 --- a/staging/src/k8s.io/client-go/rest/connection_test.go +++ b/staging/src/k8s.io/client-go/rest/connection_test.go @@ -26,6 +26,7 @@ import ( "net/url" "os" "strconv" + "strings" "sync/atomic" "testing" "time" @@ -162,3 +163,33 @@ func TestReconnectBrokenTCP(t *testing.T) { t.Fatalf("expected %d dials, got %d", 2, dials) } } + +func TestRestClientTimeout(t *testing.T) { + ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(2 * time.Second) + fmt.Fprintf(w, "Hello, %s", r.Proto) + })) + ts.Start() + defer ts.Close() + + config := &Config{ + Host: ts.URL, + Timeout: 1 * time.Second, + // These fields are required to create a REST client. + ContentConfig: ContentConfig{ + GroupVersion: &schema.GroupVersion{}, + NegotiatedSerializer: &serializer.CodecFactory{}, + }, + } + client, err := RESTClientFor(config) + if err != nil { + t.Fatalf("failed to create REST client: %v", err) + } + _, err = client.Get().AbsPath("/").DoRaw(context.TODO()) + if err == nil { + t.Fatalf("timeout error expected") + } + if !strings.Contains(err.Error(), "deadline exceeded") { + t.Fatalf("timeout error expected, received %v", err) + } +} diff --git a/staging/src/k8s.io/client-go/rest/transport.go b/staging/src/k8s.io/client-go/rest/transport.go index 57d9215c063..7c38c6d92c1 100644 --- a/staging/src/k8s.io/client-go/rest/transport.go +++ b/staging/src/k8s.io/client-go/rest/transport.go @@ -26,6 +26,27 @@ import ( "k8s.io/client-go/transport" ) +// HTTPClientFor returns an http.Client that will provide the authentication +// or transport level security defined by the provided Config. Will return the +// default http.DefaultClient if no special case behavior is needed. +func HTTPClientFor(config *Config) (*http.Client, error) { + transport, err := TransportFor(config) + if err != nil { + return nil, err + } + var httpClient *http.Client + if transport != http.DefaultTransport || config.Timeout > 0 { + httpClient = &http.Client{ + Transport: transport, + Timeout: config.Timeout, + } + } else { + httpClient = http.DefaultClient + } + + return httpClient, nil +} + // TLSConfigFor returns a tls.Config that will provide the transport level security defined // by the provided Config. Will return nil if no transport level security is requested. func TLSConfigFor(config *Config) (*tls.Config, error) {