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