Merge pull request #6338 from smarterclayton/improve_config_error

Return a typed error for config validation, and make errors simple

pkg/api/errors/errors.go

@@ -17,8 +17,10 @@ limitations under the License.
 package errors
 
 import (
+	"encoding/json"
 	"fmt"
 	"net/http"
+	"strings"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
@@ -57,6 +59,14 @@ func (e *StatusError) Status() api.Status {
 	return e.ErrStatus
 }
 
+// DebugError reports extended info about the error to debug output.
+func (e *StatusError) DebugError() (string, []interface{}) {
+	if out, err := json.MarshalIndent(e.ErrStatus, "", "  "); err == nil {
+		return "server response object: %s", []interface{}{string(out)}
+	}
+	return "server response object: %#v", []interface{}{e.ErrStatus}
+}
+
 // UnexpectedObjectError can be returned by FromObject if it's passed a non-status object.
 type UnexpectedObjectError struct {
 	Object runtime.Object
@@ -239,6 +249,72 @@ func NewTimeoutError(message string, retryAfterSeconds int) error {
 	}}
 }
 
+// NewGenericServerResponse returns a new error for server responses that are not in a recognizable form.
+func NewGenericServerResponse(code int, verb, kind, name, serverMessage string, retryAfterSeconds int) error {
+	reason := api.StatusReasonUnknown
+	message := fmt.Sprintf("the server responded with the status code %d but did not return more information", code)
+	switch code {
+	case http.StatusConflict:
+		if verb == "POST" {
+			reason = api.StatusReasonAlreadyExists
+		} else {
+			reason = api.StatusReasonConflict
+		}
+		message = "the server reported a conflict"
+	case http.StatusNotFound:
+		reason = api.StatusReasonNotFound
+		message = "the server could not find the requested resource"
+	case http.StatusBadRequest:
+		reason = api.StatusReasonBadRequest
+		message = "the server rejected our request for an unknown reason"
+	case http.StatusUnauthorized:
+		reason = api.StatusReasonUnauthorized
+		message = "the server has asked for the client to provide credentials"
+	case http.StatusForbidden:
+		reason = api.StatusReasonForbidden
+		message = "the server does not allow access to the requested resource"
+	case StatusUnprocessableEntity:
+		reason = api.StatusReasonInvalid
+		message = "the server rejected our request due to an error in our request"
+	case StatusServerTimeout:
+		reason = api.StatusReasonServerTimeout
+		message = "the server cannot complete the requested operation at this time, try again later"
+	case StatusTooManyRequests:
+		reason = api.StatusReasonTimeout
+		message = "the server has received too many requests and has asked us to try again later"
+	default:
+		if code >= 500 {
+			reason = api.StatusReasonInternalError
+			message = "an error on the server has prevented the request from succeeding"
+		}
+	}
+	switch {
+	case len(kind) > 0 && len(name) > 0:
+		message = fmt.Sprintf("%s (%s %s %s)", message, strings.ToLower(verb), kind, name)
+	case len(kind) > 0:
+		message = fmt.Sprintf("%s (%s %s)", message, strings.ToLower(verb), kind)
+	}
+	return &StatusError{api.Status{
+		Status: api.StatusFailure,
+		Code:   code,
+		Reason: reason,
+		Details: &api.StatusDetails{
+			Kind: kind,
+			ID:   name,
+
+			Causes: []api.StatusCause{
+				{
+					Type:    api.CauseTypeUnexpectedServerResponse,
+					Message: serverMessage,
+				},
+			},
+
+			RetryAfterSeconds: retryAfterSeconds,
+		},
+		Message: message,
+	}}
+}
+
 // IsNotFound returns true if the specified error was created by NewNotFoundErr.
 func IsNotFound(err error) bool {
 	return reasonForError(err) == api.StatusReasonNotFound
@@ -288,16 +364,26 @@ func IsServerTimeout(err error) bool {
 	return reasonForError(err) == api.StatusReasonServerTimeout
 }
 
-// IsStatusError determines if err is an API Status error received from the master.
-func IsStatusError(err error) bool {
-	_, ok := err.(*StatusError)
-	return ok
+// IsUnexpectedServerError returns true if the server response was not in the expected API format,
+// and may be the result of another HTTP actor.
+func IsUnexpectedServerError(err error) bool {
+	switch t := err.(type) {
+	case *StatusError:
+		if d := t.Status().Details; d != nil {
+			for _, cause := range d.Causes {
+				if cause.Type == api.CauseTypeUnexpectedServerResponse {
+					return true
+				}
+			}
+		}
+	}
+	return false
 }
 
 // IsUnexpectedObjectError determines if err is due to an unexpected object from the master.
 func IsUnexpectedObjectError(err error) bool {
 	_, ok := err.(*UnexpectedObjectError)
-	return ok
+	return err != nil && ok
 }
 
 // SuggestsClientDelay returns true if this error suggests a client delay as well as the

pkg/api/types.go

@@ -1428,6 +1428,10 @@ const (
 	// CauseTypeFieldValueNotSupported is used to report valid (as per formatting rules)
 	// values that can not be handled (e.g. an enumerated string).
 	CauseTypeFieldValueNotSupported CauseType = "FieldValueNotSupported"
+	// CauseTypeUnexpectedServerResponse is used to report when the server responded to the client
+	// without the expected return type. The presence of this cause indicates the error may be
+	// due to an intervening proxy or the server software malfunctioning.
+	CauseTypeUnexpectedServerResponse CauseType = "UnexpectedServerResponse"
 )
 
 // ObjectReference contains enough information to let you inspect or modify the referred object.

pkg/client/clientcmd/client_config.go

@@ -26,7 +26,6 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
 	clientcmdapi "github.com/GoogleCloudPlatform/kubernetes/pkg/client/clientcmd/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/clientauth"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/util/errors"
 )
 
 var (
@@ -261,7 +260,7 @@ func (config DirectClientConfig) ConfirmUsable() error {
 	validationErrors = append(validationErrors, validateAuthInfo(config.getAuthInfoName(), config.getAuthInfo())...)
 	validationErrors = append(validationErrors, validateClusterInfo(config.getClusterName(), config.getCluster())...)
 
-	return errors.NewAggregate(validationErrors)
+	return newErrConfigurationInvalid(validationErrors)
 }
 
 func (config DirectClientConfig) getContextName() string {

pkg/client/clientcmd/validation.go

@@ -43,10 +43,47 @@ func IsContextNotFound(err error) bool {
 	if err == nil {
 		return false
 	}
-
+	if _, ok := err.(*errContextNotFound); ok || err == ErrNoContext {
+		return true
+	}
 	return strings.Contains(err.Error(), "context was not found for specified context")
 }
 
+// errConfigurationInvalid is a set of errors indicating the configuration is invalid.
+type errConfigurationInvalid []error
+
+// errConfigurationInvalid implements error and Aggregate
+var _ error = errConfigurationInvalid{}
+var _ utilerrors.Aggregate = errConfigurationInvalid{}
+
+func newErrConfigurationInvalid(errs []error) error {
+	switch len(errs) {
+	case 0:
+		return nil
+	default:
+		return errConfigurationInvalid(errs)
+	}
+}
+
+// Error implements the error interface
+func (e errConfigurationInvalid) Error() string {
+	return fmt.Sprintf("invalid configuration: %v", utilerrors.NewAggregate(e).Error())
+}
+
+// Errors implements the AggregateError interface
+func (e errConfigurationInvalid) Errors() []error {
+	return e
+}
+
+// IsConfigurationInvalid returns true if the provided error indicates the configuration is invalid.
+func IsConfigurationInvalid(err error) bool {
+	switch err.(type) {
+	case *errContextNotFound, errConfigurationInvalid:
+		return true
+	}
+	return IsContextNotFound(err)
+}
+
 // Validate checks for errors in the Config.  It does not return early so that it can find as many errors as possible.
 func Validate(config clientcmdapi.Config) error {
 	validationErrors := make([]error, 0)
@@ -69,7 +106,7 @@ func Validate(config clientcmdapi.Config) error {
 		validationErrors = append(validationErrors, validateClusterInfo(clusterName, clusterInfo)...)
 	}
 
-	return utilerrors.NewAggregate(validationErrors)
+	return newErrConfigurationInvalid(validationErrors)
 }
 
 // ConfirmUsable looks a particular context and determines if that particular part of the config is useable.  There might still be errors in the config,
@@ -99,7 +136,7 @@ func ConfirmUsable(config clientcmdapi.Config, passedContextName string) error {
 		validationErrors = append(validationErrors, validateClusterInfo(context.Cluster, config.Clusters[context.Cluster])...)
 	}
 
-	return utilerrors.NewAggregate(validationErrors)
+	return newErrConfigurationInvalid(validationErrors)
 }
 
 // validateClusterInfo looks for conflicts and errors in the cluster info
@@ -107,7 +144,11 @@ func validateClusterInfo(clusterName string, clusterInfo clientcmdapi.Cluster) [
 	validationErrors := make([]error, 0)
 
 	if len(clusterInfo.Server) == 0 {
-		validationErrors = append(validationErrors, fmt.Errorf("no server found for %v", clusterName))
+		if len(clusterName) == 0 {
+			validationErrors = append(validationErrors, fmt.Errorf("default cluster has no server defined"))
+		} else {
+			validationErrors = append(validationErrors, fmt.Errorf("no server found for cluster %q", clusterName))
+		}
 	}
 	// Make sure CA data and CA file aren't both specified
 	if len(clusterInfo.CertificateAuthority) != 0 && len(clusterInfo.CertificateAuthorityData) != 0 {
@@ -155,7 +196,7 @@ func validateAuthInfo(authInfoName string, authInfo clientcmdapi.AuthInfo) []err
 		}
 		// Make sure key data and file aren't both specified
 		if len(authInfo.ClientKey) != 0 && len(authInfo.ClientKeyData) != 0 {
-			validationErrors = append(validationErrors, fmt.Errorf("client-key-data and client-key are both specified for %v. client-key-data will override.", authInfoName))
+			validationErrors = append(validationErrors, fmt.Errorf("client-key-data and client-key are both specified for %v; client-key-data will override", authInfoName))
 		}
 		// Make sure a key is specified
 		if len(authInfo.ClientKey) == 0 && len(authInfo.ClientKeyData) == 0 {
@@ -180,7 +221,7 @@ func validateAuthInfo(authInfoName string, authInfo clientcmdapi.AuthInfo) []err
 
 	// authPath also provides information for the client to identify the server, so allow multiple auth methods in that case
 	if (len(methods) > 1) && (!usingAuthPath) {
-		validationErrors = append(validationErrors, fmt.Errorf("more than one authentication method found for  %v.  Found %v, only one is allowed", authInfoName, methods))
+		validationErrors = append(validationErrors, fmt.Errorf("more than one authentication method found for %v; found %v, only one is allowed", authInfoName, methods))
 	}
 
 	return validationErrors
@@ -191,19 +232,19 @@ func validateContext(contextName string, context clientcmdapi.Context, config cl
 	validationErrors := make([]error, 0)
 
 	if len(context.AuthInfo) == 0 {
-		validationErrors = append(validationErrors, fmt.Errorf("user was not specified for Context %v", contextName))
+		validationErrors = append(validationErrors, fmt.Errorf("user was not specified for context %q", contextName))
 	} else if _, exists := config.AuthInfos[context.AuthInfo]; !exists {
-		validationErrors = append(validationErrors, fmt.Errorf("user, %v, was not found for Context %v", context.AuthInfo, contextName))
+		validationErrors = append(validationErrors, fmt.Errorf("user %q was not found for context %q", context.AuthInfo, contextName))
 	}
 
 	if len(context.Cluster) == 0 {
-		validationErrors = append(validationErrors, fmt.Errorf("cluster was not specified for Context %v", contextName))
+		validationErrors = append(validationErrors, fmt.Errorf("cluster was not specified for context %q", contextName))
 	} else if _, exists := config.Clusters[context.Cluster]; !exists {
-		validationErrors = append(validationErrors, fmt.Errorf("cluster, %v, was not found for Context %v", context.Cluster, contextName))
+		validationErrors = append(validationErrors, fmt.Errorf("cluster %q was not found for context %q", context.Cluster, contextName))
 	}
 
 	if (len(context.Namespace) != 0) && !util.IsDNS952Label(context.Namespace) {
-		validationErrors = append(validationErrors, fmt.Errorf("namespace, %v, for context %v, does not conform to the kubernetes DNS952 rules", context.Namespace, contextName))
+		validationErrors = append(validationErrors, fmt.Errorf("namespace %q for context %q does not conform to the kubernetes DNS952 rules", context.Namespace, contextName))
 	}
 
 	return validationErrors

pkg/client/clientcmd/validation_test.go

@@ -127,14 +127,33 @@ func TestIsContextNotFound(t *testing.T) {
 	if !IsContextNotFound(err) {
 		t.Errorf("Expected context not found, but got %v", err)
 	}
+	if !IsConfigurationInvalid(err) {
+		t.Errorf("Expected configuration invalid, but got %v", err)
+	}
 }
+
+func TestIsConfigurationInvalid(t *testing.T) {
+	if newErrConfigurationInvalid([]error{}) != nil {
+		t.Errorf("unexpected error")
+	}
+	if newErrConfigurationInvalid([]error{ErrNoContext}) == ErrNoContext {
+		t.Errorf("unexpected error")
+	}
+	if newErrConfigurationInvalid([]error{ErrNoContext, ErrNoContext}) == nil {
+		t.Errorf("unexpected error")
+	}
+	if !IsConfigurationInvalid(newErrConfigurationInvalid([]error{ErrNoContext, ErrNoContext})) {
+		t.Errorf("unexpected error")
+	}
+}
+
 func TestValidateMissingReferencesConfig(t *testing.T) {
 	config := clientcmdapi.NewConfig()
 	config.CurrentContext = "anything"
 	config.Contexts["anything"] = clientcmdapi.Context{Cluster: "missing", AuthInfo: "missing"}
 	test := configValidationTest{
 		config:                 config,
-		expectedErrorSubstring: []string{"user, missing, was not found for Context anything", "cluster, missing, was not found for Context anything"},
+		expectedErrorSubstring: []string{"user \"missing\" was not found for context \"anything\"", "cluster \"missing\" was not found for context \"anything\""},
 	}
 
 	test.testContext("anything", t)
@@ -146,7 +165,7 @@ func TestValidateEmptyContext(t *testing.T) {
 	config.Contexts["anything"] = clientcmdapi.Context{}
 	test := configValidationTest{
 		config:                 config,
-		expectedErrorSubstring: []string{"user was not specified for Context anything", "cluster was not specified for Context anything"},
+		expectedErrorSubstring: []string{"user was not specified for context \"anything\"", "cluster was not specified for context \"anything\""},
 	}
 
 	test.testContext("anything", t)
@@ -377,6 +396,9 @@ func (c configValidationTest) testConfig(t *testing.T) {
 					t.Errorf("Expected error containing: %v, but got %v", c.expectedErrorSubstring, err)
 				}
 			}
+			if !IsConfigurationInvalid(err) {
+				t.Errorf("all errors should be configuration invalid: %v", err)
+			}
 		}
 	} else {
 		if err != nil {

pkg/client/request.go

@@ -52,25 +52,6 @@ type HTTPClient interface {
 	Do(req *http.Request) (*http.Response, error)
 }
 
-// UnexpectedStatusError is returned as an error if a response's body and HTTP code don't
-// make sense together.
-type UnexpectedStatusError struct {
-	Request  *http.Request
-	Response *http.Response
-	Body     string
-}
-
-// Error returns a textual description of 'u'.
-func (u *UnexpectedStatusError) Error() string {
-	return fmt.Sprintf("request [%+v] failed (%d) %s: %s", u.Request, u.Response.StatusCode, u.Response.Status, u.Body)
-}
-
-// IsUnexpectedStatusError determines if err is due to an unexpected status from the server.
-func IsUnexpectedStatusError(err error) bool {
-	_, ok := err.(*UnexpectedStatusError)
-	return ok
-}
-
 // RequestConstructionError is returned when there's an error assembling a request.
 type RequestConstructionError struct {
 	Err error
@@ -689,7 +670,6 @@ func (r *Request) transformResponse(resp *http.Response, req *http.Request, body
 //    initial contact, the presence of mismatched body contents from posted content types
 //    - Give these a separate distinct error type and capture as much as possible of the original message
 //
-// TODO: introduce further levels of refinement that allow a client to distinguish between 1 and 2-3.
 // TODO: introduce transformation of generic http.Client.Do() errors that separates 4.
 func (r *Request) transformUnstructuredResponseError(resp *http.Response, req *http.Request, body []byte) error {
 	if body == nil && resp.Body != nil {
@@ -697,43 +677,12 @@ func (r *Request) transformUnstructuredResponseError(resp *http.Response, req *h
 			body = data
 		}
 	}
-	var err error = &UnexpectedStatusError{
-		Request:  req,
-		Response: resp,
-		Body:     string(body),
-	}
 	message := "unknown"
 	if isTextResponse(resp) {
 		message = strings.TrimSpace(string(body))
 	}
-	// TODO: handle other error classes we know about
-	switch resp.StatusCode {
-	case http.StatusConflict:
-		if req.Method == "POST" {
-			err = errors.NewAlreadyExists(r.resource, r.resourceName)
-		} else {
-			err = errors.NewConflict(r.resource, r.resourceName, err)
-		}
-	case http.StatusNotFound:
-		err = errors.NewNotFound(r.resource, r.resourceName)
-	case http.StatusBadRequest:
-		err = errors.NewBadRequest(message)
-	case http.StatusUnauthorized:
-		err = errors.NewUnauthorized(message)
-	case http.StatusForbidden:
-		err = errors.NewForbidden(r.resource, r.resourceName, err)
-	case errors.StatusUnprocessableEntity:
-		err = errors.NewInvalid(r.resource, r.resourceName, nil)
-	case errors.StatusServerTimeout:
-		retryAfterSeconds, _ := retryAfterSeconds(resp)
-		err = errors.NewServerTimeout(r.resource, r.verb, retryAfterSeconds)
-	case errors.StatusTooManyRequests:
-		retryAfterSeconds, _ := retryAfterSeconds(resp)
-		err = errors.NewServerTimeout(r.resource, r.verb, retryAfterSeconds)
-	case http.StatusInternalServerError:
-		err = errors.NewInternalError(fmt.Errorf(message))
-	}
-	return err
+	retryAfter, _ := retryAfterSeconds(resp)
+	return errors.NewGenericServerResponse(resp.StatusCode, req.Method, r.resource, r.resourceName, message, retryAfter)
 }
 
 // isTextResponse returns true if the response appears to be a textual media type.

pkg/client/request_test.go

@@ -251,7 +251,7 @@ func TestTransformResponse(t *testing.T) {
 			},
 			Error: true,
 			ErrFn: func(err error) bool {
-				return err.Error() == "aaaaa" && apierrors.IsUnauthorized(err)
+				return strings.Contains(err.Error(), "server has asked for the client to provide") && apierrors.IsUnauthorized(err)
 			},
 		},
 		{Response: &http.Response{StatusCode: 403}, Error: true},

pkg/kubectl/cmd/util/helpers.go

@@ -17,11 +17,13 @@ limitations under the License.
 package util
 
 import (
+	"bytes"
 	"encoding/json"
 	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
+	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -30,7 +32,9 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/client/clientcmd"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
+	utilerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/util/errors"
 	"github.com/evanphx/json-patch"
 
 	"github.com/golang/glog"
@@ -38,21 +42,69 @@ import (
 	"github.com/spf13/pflag"
 )
 
+type debugError interface {
+	DebugError() (msg string, args []interface{})
+}
+
 func CheckErr(err error) {
 	if err != nil {
-		if errors.IsStatusError(err) {
-			glog.FatalDepth(1, fmt.Sprintf("Error received from API: %s", err.Error()))
+		if debugErr, ok := err.(debugError); ok {
+			glog.V(4).Infof(debugErr.DebugError())
 		}
-		if errors.IsUnexpectedObjectError(err) {
-			glog.FatalDepth(1, fmt.Sprintf("Unexpected object received from server: %s", err.Error()))
+		_, isStatus := err.(client.APIStatus)
+		switch {
+		case clientcmd.IsConfigurationInvalid(err):
+			fatal(MultilineError("Error in configuration: ", err))
+		case isStatus:
+			fatal(fmt.Sprintf("Error from server: %s", err.Error()))
+		case errors.IsUnexpectedObjectError(err):
+			fatal(fmt.Sprintf("Server returned an unexpected response: %s", err.Error()))
 		}
-		if client.IsUnexpectedStatusError(err) {
-			glog.FatalDepth(1, fmt.Sprintf("Unexpected status received from server: %s", err.Error()))
+		switch t := err.(type) {
+		case *url.Error:
+			glog.V(4).Infof("Connection error: %s %s: %v", t.Op, t.URL, t.Err)
+			switch {
+			case strings.Contains(t.Err.Error(), "connection refused"):
+				host := t.URL
+				if server, err := url.Parse(t.URL); err == nil {
+					host = server.Host
+				}
+				fatal(fmt.Sprintf("The connection to the server %s was refused - did you specify the right host or port?", host))
+			}
+			fatal(fmt.Sprintf("Unable to connect to the server: %v", t.Err))
 		}
-		glog.FatalDepth(1, fmt.Sprintf("Client error: %s", err.Error()))
+		fatal(fmt.Sprintf("Error: %s", err.Error()))
 	}
 }
 
+func MultilineError(prefix string, err error) string {
+	if agg, ok := err.(utilerrors.Aggregate); ok {
+		errs := agg.Errors()
+		buf := &bytes.Buffer{}
+		switch len(errs) {
+		case 0:
+			return fmt.Sprintf("%s%v", prefix, err)
+		case 1:
+			return fmt.Sprintf("%s%v", prefix, errs[0])
+		default:
+			fmt.Fprintln(buf, prefix)
+			for _, err := range errs {
+				fmt.Fprintf(buf, "* %v\n", err)
+			}
+			return buf.String()
+		}
+	}
+	return fmt.Sprintf("%s%s", prefix, err)
+}
+
+func fatal(msg string) {
+	if glog.V(2) {
+		glog.FatalDepth(2, msg)
+	}
+	fmt.Fprintln(os.Stderr, msg)
+	os.Exit(1)
+}
+
 func UsageError(cmd *cobra.Command, format string, args ...interface{}) error {
 	msg := fmt.Sprintf(format, args...)
 	return fmt.Errorf("%s\nsee '%s -h' for help.", msg, cmd.CommandPath())

pkg/util/util.go

@@ -407,12 +407,12 @@ func chooseHostInterfaceNativeGo() (net.IP, error) {
 	if i == len(intfs) {
 		return nil, err
 	}
-	glog.V(2).Infof("Choosing interface %s for from-host portals", intfs[i].Name)
+	glog.V(4).Infof("Choosing interface %s for from-host portals", intfs[i].Name)
 	addrs, err := intfs[i].Addrs()
 	if err != nil {
 		return nil, err
 	}
-	glog.V(2).Infof("Interface %s = %s", intfs[i].Name, addrs[0].String())
+	glog.V(4).Infof("Interface %s = %s", intfs[i].Name, addrs[0].String())
 	ip, _, err := net.ParseCIDR(addrs[0].String())
 	if err != nil {
 		return nil, err