From 5b137a64a8eb6d46d79aea5e7e7b39022b2173ea Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 20 Oct 2015 11:45:46 -0400 Subject: [PATCH] Return errors directly from negotiation Otherwise we're hiding problems in connectivity --- pkg/client/unversioned/fake/fake.go | 5 ++- pkg/client/unversioned/helper.go | 10 +++-- .../unversioned/helper_blackbox_test.go | 42 ++++++++++++------- 3 files changed, 38 insertions(+), 19 deletions(-) diff --git a/pkg/client/unversioned/fake/fake.go b/pkg/client/unversioned/fake/fake.go index 8e77910a64..0499122226 100644 --- a/pkg/client/unversioned/fake/fake.go +++ b/pkg/client/unversioned/fake/fake.go @@ -64,9 +64,12 @@ func (c *RESTClient) Delete() *unversioned.Request { } func (c *RESTClient) Do(req *http.Request) (*http.Response, error) { + if c.Err != nil { + return nil, c.Err + } c.Req = req if c.Client != unversioned.HTTPClient(nil) { return c.Client.Do(req) } - return c.Resp, c.Err + return c.Resp, nil } diff --git a/pkg/client/unversioned/helper.go b/pkg/client/unversioned/helper.go index b217ae07fd..f6bcbada84 100644 --- a/pkg/client/unversioned/helper.go +++ b/pkg/client/unversioned/helper.go @@ -269,7 +269,9 @@ func NegotiateVersion(client *Client, c *Config, version string, clientRegistere } apiVersions, err := client.ServerAPIVersions() if err != nil { - return "", fmt.Errorf("couldn't read version from server: %v", err) + // This is almost always a connection error, and higher level code should treat this as a generic error, + // not a negotiation specific error. + return "", err } serverVersions := sets.String{} for _, v := range apiVersions.Versions { @@ -283,7 +285,7 @@ func NegotiateVersion(client *Client, c *Config, version string, clientRegistere // If server does not support warn, but try to negotiate a lower version. if len(version) != 0 { if !clientVersions.Has(version) { - return "", fmt.Errorf("Client does not support API version '%s'. Client supported API versions: %v", version, clientVersions) + return "", fmt.Errorf("client does not support API version %q; client supported API versions: %v", version, clientVersions) } if serverVersions.Has(version) { @@ -291,7 +293,7 @@ func NegotiateVersion(client *Client, c *Config, version string, clientRegistere } // If we are using an explicit config version the server does not support, fail. if version == c.Version { - return "", fmt.Errorf("Server does not support API version '%s'.", version) + return "", fmt.Errorf("server does not support API version %q", version) } } @@ -307,7 +309,7 @@ func NegotiateVersion(client *Client, c *Config, version string, clientRegistere return clientVersion, nil } } - return "", fmt.Errorf("Failed to negotiate an api version. Server supports: %v. Client supports: %v.", + return "", fmt.Errorf("failed to negotiate an api version; server supports: %v, client supports: %v", serverVersions, clientRegisteredVersions) } diff --git a/pkg/client/unversioned/helper_blackbox_test.go b/pkg/client/unversioned/helper_blackbox_test.go index 33f54871f2..b7aa927d3d 100644 --- a/pkg/client/unversioned/helper_blackbox_test.go +++ b/pkg/client/unversioned/helper_blackbox_test.go @@ -19,9 +19,11 @@ package unversioned_test import ( "bytes" "encoding/json" + "fmt" "io" "io/ioutil" "net/http" + "strings" "testing" "k8s.io/kubernetes/pkg/api/testapi" @@ -39,12 +41,14 @@ func objBody(object interface{}) io.ReadCloser { } func TestNegotiateVersion(t *testing.T) { + refusedErr := fmt.Errorf("connection refused") tests := []struct { name, version, expectedVersion string serverVersions []string clientVersions []string config *unversioned.Config - expectErr bool + expectErr func(err error) bool + sendErr error }{ { name: "server supports client default", @@ -53,7 +57,6 @@ func TestNegotiateVersion(t *testing.T) { serverVersions: []string{"version1", testapi.Default.Version()}, clientVersions: []string{"version1", testapi.Default.Version()}, expectedVersion: "version1", - expectErr: false, }, { name: "server falls back to client supported", @@ -62,7 +65,6 @@ func TestNegotiateVersion(t *testing.T) { serverVersions: []string{"version1"}, clientVersions: []string{"version1", testapi.Default.Version()}, expectedVersion: "version1", - expectErr: false, }, { name: "explicit version supported", @@ -71,16 +73,22 @@ func TestNegotiateVersion(t *testing.T) { serverVersions: []string{"version1", testapi.Default.Version()}, clientVersions: []string{"version1", testapi.Default.Version()}, expectedVersion: testapi.Default.Version(), - expectErr: false, }, { - name: "explicit version not supported", - version: "", - config: &unversioned.Config{Version: testapi.Default.Version()}, - serverVersions: []string{"version1"}, - clientVersions: []string{"version1", testapi.Default.Version()}, - expectedVersion: "", - expectErr: true, + name: "explicit version not supported", + version: "", + config: &unversioned.Config{Version: testapi.Default.Version()}, + serverVersions: []string{"version1"}, + clientVersions: []string{"version1", testapi.Default.Version()}, + expectErr: func(err error) bool { return strings.Contains(err.Error(), `server does not support API version "v1"`) }, + }, + { + name: "connection refused error", + config: &unversioned.Config{Version: testapi.Default.Version()}, + serverVersions: []string{"version1"}, + clientVersions: []string{"version1", testapi.Default.Version()}, + sendErr: refusedErr, + expectErr: func(err error) bool { return err == refusedErr }, }, } codec := testapi.Default.Codec() @@ -93,17 +101,23 @@ func TestNegotiateVersion(t *testing.T) { Body: objBody(&unversionedapi.APIVersions{Versions: test.serverVersions}), }, Client: fake.HTTPClientFunc(func(req *http.Request) (*http.Response, error) { + if test.sendErr != nil { + return nil, test.sendErr + } return &http.Response{StatusCode: 200, Body: objBody(&unversionedapi.APIVersions{Versions: test.serverVersions})}, nil }), } c := unversioned.NewOrDie(test.config) c.Client = fakeClient.Client response, err := unversioned.NegotiateVersion(c, test.config, test.version, test.clientVersions) - if err == nil && test.expectErr { + if err == nil && test.expectErr != nil { t.Errorf("expected error, got nil for [%s].", test.name) } - if err != nil && !test.expectErr { - t.Errorf("unexpected error for [%s]: %v.", test.name, err) + if err != nil { + if test.expectErr == nil || !test.expectErr(err) { + t.Errorf("unexpected error for [%s]: %v.", test.name, err) + } + continue } if response != test.expectedVersion { t.Errorf("expected version %s, got %s.", test.expectedVersion, response)