[#52] Provide Redfish feedback in RemoteDirect

Currently, errors thrown by RemoteDirect are simply HTTP status codes
from the Redfish client HTTP requests. This change adds feedback to
RemoteDirect errors when the Redfish client provides error feedback.

Closes #52

Change-Id: I4441baf3b98405e92613a2d4e8c44ef63a5d4001
Signed-off-by: Drew Walters <andrew.walters@att.com>

pkg/remote/redfish/errors.go

@@ -1,9 +1,12 @@
 package redfish
 
 import (
+	"encoding/json"
 	"fmt"
 	"net/http"
 
+	redfishClient "opendev.org/airship/go-redfish/client"
+
 	aerror "opendev.org/airship/airshipctl/pkg/errors"
 )
 
@@ -26,16 +29,29 @@ func (e ErrRedfishMissingConfig) Error() string {
 	return "missing configuration: " + e.What
 }
 
-// Redfish client return error if response has no body.
+// ScreenRedfishError provides detailed error checking on a Redfish client response.
-// In this case this function further checks the
+func ScreenRedfishError(httpResp *http.Response, clientErr error) error {
-// HTTP response code.
+	// NOTE(drewwalters96): clientErr may not be nil even though the request was successful. The HTTP status code
-func ScreenRedfishError(httpResp *http.Response, err error) error {
+	// has to be verified for success on each request. The Redfish client uses HTTP codes 200 and 204 to indicate
-	if err != nil && httpResp != nil {
+	// success.
-		if httpResp.StatusCode < 200 || httpResp.StatusCode >= 400 {
+	if httpResp != nil && httpResp.StatusCode != 200 && httpResp.StatusCode != 204 {
+		if clientErr == nil {
+			return ErrRedfishClient{Message: http.StatusText(httpResp.StatusCode)}
+		}
+
+		oAPIErr, ok := clientErr.(redfishClient.GenericOpenAPIError)
+		if !ok {
+			return ErrRedfishClient{Message: "Unable to decode client error."}
+		}
+
+		var resp redfishClient.RedfishError
+		if err := json.Unmarshal(oAPIErr.Body(), &resp); err != nil {
+			// No JSON response included; use generic error text.
 			return ErrRedfishClient{Message: err.Error()}
 		}
-	} else if err != nil {
+	} else if httpResp == nil {
-		return ErrRedfishClient{Message: err.Error()}
+		return ErrRedfishClient{Message: "HTTP request failed. Please try again."}
 	}
+
 	return nil
 }

pkg/remote/redfish/redfish_test.go

@@ -27,10 +27,11 @@ func TestRedfishRemoteDirectNormal(t *testing.T) {
 	defer m.AssertExpectations(t)
 
 	systemID := computerSystemID
+	httpResp := &http.Response{StatusCode: 200}
 	m.On("GetSystem", context.Background(), systemID).
-		Return(getTestSystem(), nil, nil)
+		Return(getTestSystem(), httpResp, nil)
 	m.On("InsertVirtualMedia", context.Background(), "manager-1", "Cd", mock.Anything).
-		Return(redfishClient.RedfishError{}, nil, nil)
+		Return(redfishClient.RedfishError{}, httpResp, nil)
 
 	systemReq := redfishClient.ComputerSystem{
 		Boot: redfishClient.Boot{
@@ -39,18 +40,18 @@ func TestRedfishRemoteDirectNormal(t *testing.T) {
 	}
 	m.On("SetSystem", context.Background(), systemID, systemReq).
 		Times(1).
-		Return(redfishClient.ComputerSystem{}, nil, nil)
+		Return(redfishClient.ComputerSystem{}, httpResp, nil)
 
 	resetReq := redfishClient.ResetRequestBody{}
 	resetReq.ResetType = redfishClient.RESETTYPE_FORCE_OFF
 	m.On("ResetSystem", context.Background(), systemID, resetReq).
 		Times(1).
-		Return(redfishClient.RedfishError{}, nil, nil)
+		Return(redfishClient.RedfishError{}, httpResp, nil)
 
 	resetReq.ResetType = redfishClient.RESETTYPE_ON
 	m.On("ResetSystem", context.Background(), systemID, resetReq).
 		Times(1).
-		Return(redfishClient.RedfishError{}, nil, nil)
+		Return(redfishClient.RedfishError{}, httpResp, nil)
 
 	rDCfg := getDefaultRedfishRemoteDirectObj(t, m)
 
@@ -85,9 +86,7 @@ func TestRedfishRemoteDirectGetSystemNetworkError(t *testing.T) {
 
 	systemID := computerSystemID
 	realErr := fmt.Errorf("server request timeout")
-	httpResp := &http.Response{
+	httpResp := &http.Response{StatusCode: 408}
-		StatusCode: 408,
-	}
 	m.On("GetSystem", context.Background(), systemID).
 		Times(1).
 		Return(redfishClient.ComputerSystem{}, httpResp, realErr)
@@ -109,11 +108,8 @@ func TestRedfishRemoteDirectInvalidIsoPath(t *testing.T) {
 	localRDCfg := rDCfg
 	localRDCfg.IsoPath = "bogus/path/to.iso"
 
-	errStr := "invalid remote boot path"
+	realErr := redfishClient.GenericOpenAPIError{}
-	realErr := fmt.Errorf(errStr)
+	httpResp := &http.Response{StatusCode: 500}
-	httpResp := &http.Response{
-		StatusCode: 500,
-	}
 	m.On("GetSystem", context.Background(), systemID).
 		Times(1).
 		Return(getTestSystem(), nil, nil)
@@ -157,19 +153,17 @@ func TestRedfishRemoteDirectSetSystemBootSourceFailed(t *testing.T) {
 	defer m.AssertExpectations(t)
 
 	systemID := computerSystemID
+	httpSuccResp := &http.Response{StatusCode: 200}
 	m.On("GetSystem", context.Background(), systemID).
-		Return(getTestSystem(), nil, nil)
+		Return(getTestSystem(), httpSuccResp, nil)
 
 	m.On("InsertVirtualMedia", context.Background(), "manager-1", "Cd", mock.Anything).
-		Return(redfishClient.RedfishError{}, nil, nil)
+		Return(redfishClient.RedfishError{}, httpSuccResp, nil)
 
-	realErr := fmt.Errorf("unauthorized")
-	httpResp := &http.Response{
-		StatusCode: 401,
-	}
 	m.On("SetSystem", context.Background(), systemID, mock.Anything).
 		Times(1).
-		Return(redfishClient.ComputerSystem{}, httpResp, realErr)
+		Return(redfishClient.ComputerSystem{}, &http.Response{StatusCode: 401},
+			redfishClient.GenericOpenAPIError{})
 
 	rDCfg := getDefaultRedfishRemoteDirectObj(t, m)
 
@@ -184,26 +178,23 @@ func TestRedfishRemoteDirectSystemRebootFailed(t *testing.T) {
 	defer m.AssertExpectations(t)
 
 	systemID := computerSystemID
-
+	httpSuccResp := &http.Response{StatusCode: 200}
 	m.On("GetSystem", context.Background(), systemID).
-		Return(getTestSystem(), nil, nil)
+		Return(getTestSystem(), httpSuccResp, nil)
 
 	m.On("InsertVirtualMedia", context.Background(), mock.Anything, mock.Anything, mock.Anything).
-		Return(redfishClient.RedfishError{}, nil, nil)
+		Return(redfishClient.RedfishError{}, httpSuccResp, nil)
 
 	m.On("SetSystem", context.Background(), systemID, mock.Anything).
 		Times(1).
-		Return(redfishClient.ComputerSystem{}, nil, nil)
+		Return(redfishClient.ComputerSystem{}, httpSuccResp, nil)
 
-	realErr := fmt.Errorf("unauthorized")
-	httpResp := &http.Response{
-		StatusCode: 401,
-	}
 	resetReq := redfishClient.ResetRequestBody{}
 	resetReq.ResetType = redfishClient.RESETTYPE_FORCE_OFF
 	m.On("ResetSystem", context.Background(), systemID, resetReq).
 		Times(1).
-		Return(redfishClient.RedfishError{}, httpResp, realErr)
+		Return(redfishClient.RedfishError{}, &http.Response{StatusCode: 401},
+			redfishClient.GenericOpenAPIError{})
 
 	rDCfg := getDefaultRedfishRemoteDirectObj(t, m)
 

pkg/remote/redfish/utils_test.go

@@ -2,7 +2,6 @@ package redfish
 
 import (
 	"context"
-	"fmt"
 	"net/http"
 	"testing"
 
@@ -17,47 +16,41 @@ const (
 )
 
 func TestRedfishErrorNoError(t *testing.T) {
-	err := ScreenRedfishError(nil, nil)
+	err := ScreenRedfishError(&http.Response{StatusCode: 200}, nil)
 	assert.NoError(t, err)
 }
 
 func TestRedfishErrorNonNilErrorWithoutHttpResp(t *testing.T) {
-	realErr := fmt.Errorf("sample error")
+	err := ScreenRedfishError(nil, redfishClient.GenericOpenAPIError{})
-	err := ScreenRedfishError(nil, realErr)
+
-	assert.Error(t, err)
 	_, ok := err.(ErrRedfishClient)
 	assert.True(t, ok)
 }
 
 func TestRedfishErrorNonNilErrorWithHttpRespError(t *testing.T) {
-	realErr := fmt.Errorf("sample error")
+	respErr := redfishClient.GenericOpenAPIError{}
 
-	httpResp := &http.Response{StatusCode: 408}
+	err := ScreenRedfishError(&http.Response{StatusCode: 408}, respErr)
-	err := ScreenRedfishError(httpResp, realErr)
+	_, ok := err.(ErrRedfishClient)
-	assert.Equal(t, err, ErrRedfishClient{Message: realErr.Error()})
+	assert.True(t, ok)
 
-	httpResp.StatusCode = 400
+	err = ScreenRedfishError(&http.Response{StatusCode: 500}, respErr)
-	err = ScreenRedfishError(httpResp, realErr)
+	_, ok = err.(ErrRedfishClient)
-	assert.Equal(t, err, ErrRedfishClient{Message: realErr.Error()})
+	assert.True(t, ok)
 
-	httpResp.StatusCode = 199
+	err = ScreenRedfishError(&http.Response{StatusCode: 199}, respErr)
-	err = ScreenRedfishError(httpResp, realErr)
+	_, ok = err.(ErrRedfishClient)
-	assert.Equal(t, err, ErrRedfishClient{Message: realErr.Error()})
+	assert.True(t, ok)
 }
 
 func TestRedfishErrorNonNilErrorWithHttpRespOK(t *testing.T) {
-	realErr := fmt.Errorf("sample error")
+	respErr := redfishClient.GenericOpenAPIError{}
 
-	httpResp := &http.Response{StatusCode: 204}
+	// NOTE: Redfish client only uses HTTP 200 & HTTP 204 for success.
-	err := ScreenRedfishError(httpResp, realErr)
+	err := ScreenRedfishError(&http.Response{StatusCode: 204}, respErr)
 	assert.NoError(t, err)
 
-	httpResp.StatusCode = 200
+	err = ScreenRedfishError(&http.Response{StatusCode: 200}, respErr)
-	err = ScreenRedfishError(httpResp, realErr)
-	assert.NoError(t, err)
-
-	httpResp.StatusCode = 399
-	err = ScreenRedfishError(httpResp, realErr)
 	assert.NoError(t, err)
 }
 
@@ -106,12 +99,12 @@ func TestRedfishUtilRebootSystemOK(t *testing.T) {
 	resetReq.ResetType = redfishClient.RESETTYPE_FORCE_OFF
 	m.On("ResetSystem", ctx, systemID, resetReq).
 		Times(1).
-		Return(redfishClient.RedfishError{}, nil, nil)
+		Return(redfishClient.RedfishError{}, &http.Response{StatusCode: 200}, nil)
 
 	resetReq.ResetType = redfishClient.RESETTYPE_ON
 	m.On("ResetSystem", ctx, systemID, resetReq).
 		Times(1).
-		Return(redfishClient.RedfishError{}, nil, nil)
+		Return(redfishClient.RedfishError{}, &http.Response{StatusCode: 200}, nil)
 
 	err := RebootSystem(ctx, m, systemID)
 	assert.NoError(t, err)
@@ -122,19 +115,17 @@ func TestRedfishUtilRebootSystemForceOffError2(t *testing.T) {
 	defer m.AssertExpectations(t)
 
 	ctx := context.Background()
-	realErr := fmt.Errorf("unauthorized")
-	httpResp := &http.Response{
-		StatusCode: 401,
-	}
 	resetReq := redfishClient.ResetRequestBody{}
 	resetReq.ResetType = redfishClient.RESETTYPE_FORCE_OFF
 
 	m.On("ResetSystem", ctx, systemID, resetReq).
 		Times(1).
-		Return(redfishClient.RedfishError{}, httpResp, realErr)
+		Return(redfishClient.RedfishError{}, &http.Response{StatusCode: 401},
+			redfishClient.GenericOpenAPIError{})
 
 	err := RebootSystem(ctx, m, systemID)
-	assert.Error(t, err)
+	_, ok := err.(ErrRedfishClient)
+	assert.True(t, ok)
 }
 
 func TestRedfishUtilRebootSystemForceOffError(t *testing.T) {
@@ -142,18 +133,16 @@ func TestRedfishUtilRebootSystemForceOffError(t *testing.T) {
 	defer m.AssertExpectations(t)
 
 	ctx := context.Background()
-	realErr := fmt.Errorf("unauthorized")
-	httpResp := &http.Response{
-		StatusCode: 401,
-	}
 	resetReq := redfishClient.ResetRequestBody{}
 	resetReq.ResetType = redfishClient.RESETTYPE_FORCE_OFF
 	m.On("ResetSystem", ctx, systemID, resetReq).
 		Times(1).
-		Return(redfishClient.RedfishError{}, httpResp, realErr)
+		Return(redfishClient.RedfishError{}, &http.Response{StatusCode: 401},
+			redfishClient.GenericOpenAPIError{})
 
 	err := RebootSystem(ctx, m, systemID)
-	assert.Error(t, err)
+	_, ok := err.(ErrRedfishClient)
+	assert.True(t, ok)
 }
 
 func TestRedfishUtilRebootSystemTurningOnError(t *testing.T) {
@@ -165,18 +154,16 @@ func TestRedfishUtilRebootSystemTurningOnError(t *testing.T) {
 	resetReq.ResetType = redfishClient.RESETTYPE_FORCE_OFF
 	m.On("ResetSystem", ctx, systemID, resetReq).
 		Times(1).
-		Return(redfishClient.RedfishError{}, nil, nil)
+		Return(redfishClient.RedfishError{}, &http.Response{StatusCode: 200}, nil)
 
-	realErr := fmt.Errorf("unauthorized")
-	httpResp := &http.Response{
-		StatusCode: 401,
-	}
 	resetOnReq := redfishClient.ResetRequestBody{}
 	resetOnReq.ResetType = redfishClient.RESETTYPE_ON
 	m.On("ResetSystem", ctx, systemID, resetOnReq).
 		Times(1).
-		Return(redfishClient.RedfishError{}, httpResp, realErr)
+		Return(redfishClient.RedfishError{}, &http.Response{StatusCode: 401},
+			redfishClient.GenericOpenAPIError{})
 
 	err := RebootSystem(ctx, m, systemID)
-	assert.Error(t, err)
+	_, ok := err.(ErrRedfishClient)
+	assert.True(t, ok)
 }