Merge pull request #15953 from smarterclayton/return_unmodified_error_on_neg

Auto commit by PR queue bot

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
 }

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)
 }
 

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",
+			name:           "explicit version not supported",
-			version:         "",
+			version:        "",
-			config:          &unversioned.Config{Version: testapi.Default.Version()},
+			config:         &unversioned.Config{Version: testapi.Default.Version()},
-			serverVersions:  []string{"version1"},
+			serverVersions: []string{"version1"},
-			clientVersions:  []string{"version1", testapi.Default.Version()},
+			clientVersions: []string{"version1", testapi.Default.Version()},
-			expectedVersion: "",
+			expectErr:      func(err error) bool { return strings.Contains(err.Error(), `server does not support API version "v1"`) },
-			expectErr:       true,
+		},
+		{
+			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 {
+		if err != nil {
-			t.Errorf("unexpected error for [%s]: %v.", test.name, err)
+			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)