Merge pull request #4390 from mboersma/cli-non-json-errors

fix(client): show non-JSON HTTP errors

Author: mboersma

client/cmd/auth.go

@@ -29,7 +29,7 @@ func Register(controller string, username string, password string, email string,
 		return err
 	}
 
-	if err = client.CheckConection(httpClient, controllerURL); err != nil {
+	if err = client.CheckConnection(httpClient, controllerURL); err != nil {
 		return err
 	}
 
@@ -115,7 +115,7 @@ func Login(controller string, username string, password string, sslVerify bool)
 		return err
 	}
 
-	if err = client.CheckConection(httpClient, controllerURL); err != nil {
+	if err = client.CheckConnection(httpClient, controllerURL); err != nil {
 		return err
 	}
 

client/controller/client/http.go

@@ -57,7 +57,11 @@ func (c Client) Request(method string, path string, body []byte) (*http.Response
 		return nil, err
 	}
 
-	checkAPICompatability(res.Header.Get("DEIS_API_VERSION"))
+	if err = checkForErrors(res, ""); err != nil {
+		return nil, err
+	}
+
+	checkAPICompatibility(res.Header.Get("DEIS_API_VERSION"))
 
 	return res, nil
 }
@@ -108,22 +112,32 @@ func checkForErrors(res *http.Response, body string) error {
 		return nil
 	}
 
-	bodyMap := make(map[string]interface{})
+	// Read the response body if none was provided.
+	if body == "" {
+		defer res.Body.Close()
+		resBody, err := ioutil.ReadAll(res.Body)
+		if err != nil {
+			return err
+		}
+		body = string(resBody)
+	}
 
+	// Unmarshal the response as JSON, or return the status and body.
+	bodyMap := make(map[string]interface{})
 	if err := json.Unmarshal([]byte(body), &bodyMap); err != nil {
-		return err
+		return fmt.Errorf("\n%s\n%s\n", res.Status, body)
 	}
 
-	errorMessage := "\n"
+	errorMessage := fmt.Sprintf("\n%s\n", res.Status)
 	for key, value := range bodyMap {
 		switch v := value.(type) {
 		case string:
-			errorMessage += key + ": " + v + "\n"
+			errorMessage += fmt.Sprintf("%s: %s\n", key, v)
 		case []interface{}:
 			for _, subValue := range v {
 				switch sv := subValue.(type) {
 				case string:
-					errorMessage += key + ": " + sv + "\n"
+					errorMessage += fmt.Sprintf("%s: %s\n", key, sv)
 				default:
 					fmt.Printf("Unexpected type in %s error message array. Contents: %v",
 						reflect.TypeOf(value), sv)
@@ -135,12 +149,11 @@ func checkForErrors(res *http.Response, body string) error {
 		}
 	}
 
-	errorMessage += res.Status + "\n"
 	return errors.New(errorMessage)
 }
 
-// CheckConection checks that the user is connected to a network and the URL points to a valid controller.
-func CheckConection(client *http.Client, controllerURL url.URL) error {
+// CheckConnection checks that the user is connected to a network and the URL points to a valid controller.
+func CheckConnection(client *http.Client, controllerURL url.URL) error {
 	errorMessage := `%s does not appear to be a valid Deis controller.
 Make sure that the Controller URI is correct and the server is running.`
 
@@ -167,7 +180,7 @@ Make sure that the Controller URI is correct and the server is running.`
 		return fmt.Errorf(errorMessage, baseURL)
 	}
 
-	checkAPICompatability(res.Header.Get("DEIS_API_VERSION"))
+	checkAPICompatibility(res.Header.Get("DEIS_API_VERSION"))
 
 	return nil
 }

client/controller/client/http_test.go

@@ -109,7 +109,7 @@ func TestCheckConnection(t *testing.T) {
 
 	httpClient := CreateHTTPClient(false)
 
-	if err = CheckConection(httpClient, *u); err != nil {
+	if err = CheckConnection(httpClient, *u); err != nil {
 		t.Error(err)
 	}
 }
@@ -147,16 +147,16 @@ func TestCheckErrors(t *testing.T) {
 	t.Parallel()
 
 	expected := `
+404 NOT FOUND
 error: This is an error.
 error_array: This is an array.
 error_array: Foo!
-404 NOT FOUND
 `
 	altExpected := `
+404 NOT FOUND
 error_array: This is an array.
 error_array: Foo!
 error: This is an error.
-404 NOT FOUND
 `
 
 	body := `
@@ -178,6 +178,36 @@ error: This is an error.
 	if actual != expected && actual != altExpected {
 		t.Errorf("Expected %s or %s, Got %s", expected, altExpected, actual)
 	}
+
+	expected = `
+503 Service Temporarily Unavailable
+<html>
+<head><title>503 Service Temporarily Unavailable</title></head>
+<body bgcolor="white">
+<center><h1>503 Service Temporarily Unavailable</h1></center>
+<hr><center>nginx/1.9.4</center>
+</body>
+</html>
+`
+
+	body = `<html>
+<head><title>503 Service Temporarily Unavailable</title></head>
+<body bgcolor="white">
+<center><h1>503 Service Temporarily Unavailable</h1></center>
+<hr><center>nginx/1.9.4</center>
+</body>
+</html>`
+
+	res = http.Response{
+		StatusCode: http.StatusServiceUnavailable,
+		Status:     "503 Service Temporarily Unavailable",
+	}
+
+	actual = checkForErrors(&res, body).Error()
+
+	if actual != expected {
+		t.Errorf("Expected %s, Got %s", expected, actual)
+	}
 }
 
 func TestCheckErrorsReturnsNil(t *testing.T) {

client/controller/client/utils.go

@@ -18,7 +18,7 @@ func locateSettingsFile() string {
 	return path.Join(FindHome(), ".deis", filename+".json")
 }
 
-func checkAPICompatability(serverAPIVersion string) {
+func checkAPICompatibility(serverAPIVersion string) {
 	if serverAPIVersion != version.APIVersion {
 		fmt.Printf(`!    WARNING: Client and server API versions do not match. Please consider upgrading.
 !    Client version: %s