From 4ba23313c52cf1a513f0973d2140c5d29a902edf Mon Sep 17 00:00:00 2001 From: Drew Walters Date: Mon, 9 Mar 2020 22:00:10 +0000 Subject: [PATCH] [#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 --- pkg/remote/redfish/errors.go | 32 +++++++++---- pkg/remote/redfish/redfish_test.go | 49 ++++++++----------- pkg/remote/redfish/utils_test.go | 77 +++++++++++++----------------- 3 files changed, 76 insertions(+), 82 deletions(-) diff --git a/pkg/remote/redfish/errors.go b/pkg/remote/redfish/errors.go index a10ff86e7..ae58249e5 100644 --- a/pkg/remote/redfish/errors.go +++ b/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. -// In this case this function further checks the -// HTTP response code. -func ScreenRedfishError(httpResp *http.Response, err error) error { - if err != nil && httpResp != nil { - if httpResp.StatusCode < 200 || httpResp.StatusCode >= 400 { +// ScreenRedfishError provides detailed error checking on a Redfish client response. +func ScreenRedfishError(httpResp *http.Response, clientErr error) error { + // NOTE(drewwalters96): clientErr may not be nil even though the request was successful. The HTTP status code + // has to be verified for success on each request. The Redfish client uses HTTP codes 200 and 204 to indicate + // success. + 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 { - return ErrRedfishClient{Message: err.Error()} + } else if httpResp == nil { + return ErrRedfishClient{Message: "HTTP request failed. Please try again."} } + return nil } diff --git a/pkg/remote/redfish/redfish_test.go b/pkg/remote/redfish/redfish_test.go index fa3a6218f..62ee24e11 100644 --- a/pkg/remote/redfish/redfish_test.go +++ b/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{ - StatusCode: 408, - } + httpResp := &http.Response{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 := fmt.Errorf(errStr) - httpResp := &http.Response{ - StatusCode: 500, - } + realErr := redfishClient.GenericOpenAPIError{} + 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) diff --git a/pkg/remote/redfish/utils_test.go b/pkg/remote/redfish/utils_test.go index a02a6fb86..e1953e08c 100644 --- a/pkg/remote/redfish/utils_test.go +++ b/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, realErr) - assert.Error(t, err) + err := ScreenRedfishError(nil, redfishClient.GenericOpenAPIError{}) + _, 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(httpResp, realErr) - assert.Equal(t, err, ErrRedfishClient{Message: realErr.Error()}) + err := ScreenRedfishError(&http.Response{StatusCode: 408}, respErr) + _, ok := err.(ErrRedfishClient) + assert.True(t, ok) - httpResp.StatusCode = 400 - err = ScreenRedfishError(httpResp, realErr) - assert.Equal(t, err, ErrRedfishClient{Message: realErr.Error()}) + err = ScreenRedfishError(&http.Response{StatusCode: 500}, respErr) + _, ok = err.(ErrRedfishClient) + assert.True(t, ok) - httpResp.StatusCode = 199 - err = ScreenRedfishError(httpResp, realErr) - assert.Equal(t, err, ErrRedfishClient{Message: realErr.Error()}) + err = ScreenRedfishError(&http.Response{StatusCode: 199}, respErr) + _, ok = err.(ErrRedfishClient) + assert.True(t, ok) } func TestRedfishErrorNonNilErrorWithHttpRespOK(t *testing.T) { - realErr := fmt.Errorf("sample error") + respErr := redfishClient.GenericOpenAPIError{} - httpResp := &http.Response{StatusCode: 204} - err := ScreenRedfishError(httpResp, realErr) + // NOTE: Redfish client only uses HTTP 200 & HTTP 204 for success. + err := ScreenRedfishError(&http.Response{StatusCode: 204}, respErr) assert.NoError(t, err) - httpResp.StatusCode = 200 - err = ScreenRedfishError(httpResp, realErr) - assert.NoError(t, err) - - httpResp.StatusCode = 399 - err = ScreenRedfishError(httpResp, realErr) + err = ScreenRedfishError(&http.Response{StatusCode: 200}, respErr) 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) }