diff --git a/client/sm/client.go b/client/sm/client.go index 9cc2c7a4..20504e91 100644 --- a/client/sm/client.go +++ b/client/sm/client.go @@ -67,14 +67,15 @@ type Client interface { // It should be used only in case there is no already implemented method for such an operation Call(method string, smpath string, body io.Reader, q *Parameters) (*http.Response, error) } + type ServiceManagerError struct { - Message string + Description string StatusCode int BrokerError *api.HTTPStatusCodeError `json:"broker_error,omitempty"` } func (e *ServiceManagerError) Error() string { - return e.Message + return e.Description } type serviceManagerClient struct { @@ -312,7 +313,7 @@ func (client *serviceManagerClient) register(resource interface{}, url string, q case http.StatusAccepted: return response.Header.Get("Location"), nil default: - return "", handleFailedResponse(response) + return "", handleResponseError(response) } } @@ -331,7 +332,7 @@ func (client *serviceManagerClient) delete(url string, q *Parameters, user strin case http.StatusAccepted: return response.Header.Get("Location"), nil default: - return "", handleFailedResponse(response) + return "", handleResponseError(response) } } @@ -342,7 +343,7 @@ func (client *serviceManagerClient) get(result interface{}, url string, q *Param } if response.StatusCode != http.StatusOK { - return handleFailedResponse(response) + return handleResponseError(response) } return httputil.UnmarshalResponse(response, &result) } @@ -364,7 +365,7 @@ func (client *serviceManagerClient) update(resource interface{}, url string, id case http.StatusAccepted: return response.Header.Get("Location"), nil default: - return "", handleFailedResponse(response) + return "", handleResponseError(response) } } @@ -390,7 +391,7 @@ func (client *serviceManagerClient) executeShareInstanceRequest(shouldShare bool response, err := client.callWithUser(http.MethodPatch, types.ServiceInstancesURL+"/"+id, buffer, nil, user) if response.StatusCode != http.StatusOK { if err == nil { - return handleFailedResponse(response) + return handleResponseError(response) } return httputil.UnmarshalResponse(response, err) } @@ -398,14 +399,6 @@ func (client *serviceManagerClient) executeShareInstanceRequest(shouldShare bool return nil } -func handleFailedResponse(response *http.Response) error { - err := handleResponseError(response) - return &ServiceManagerError{ - StatusCode: response.StatusCode, - Message: err.Error(), - } -} - func (client *serviceManagerClient) Call(method string, smpath string, body io.Reader, q *Parameters) (*http.Response, error) { return client.callWithUser(method, smpath, body, q, "") } @@ -541,11 +534,13 @@ func handleResponseError(response *http.Response) error { body = []byte(fmt.Sprintf("error reading response body: %s", err)) } - err = fmt.Errorf("StatusCode: %d Body: %s", response.StatusCode, body) - if response.Request != nil { - return fmt.Errorf("request %s %s failed: %s", response.Request.Method, response.Request.URL, err) + smError := &ServiceManagerError{ + StatusCode: response.StatusCode, + Description: "", } - return fmt.Errorf("request failed: %s", err) + _ = json.Unmarshal(body, &smError) + + return smError } func bodyToBytes(closer io.ReadCloser) ([]byte, error) { diff --git a/client/sm/client_test.go b/client/sm/client_test.go index 4263e928..ef29c870 100644 --- a/client/sm/client_test.go +++ b/client/sm/client_test.go @@ -73,8 +73,7 @@ var _ = Describe("Client test", func() { }) It("should handle status code != 200", func() { _, err := client.ListInstances(params) - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusCreated) }) }) @@ -86,8 +85,7 @@ var _ = Describe("Client test", func() { }) It("should handle status code > 299", func() { _, err := client.ListInstances(params) - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusBadRequest) }) }) }) @@ -115,8 +113,7 @@ var _ = Describe("Client test", func() { }) It("should return 404", func() { _, err := client.GetInstanceByID(instance.ID, params) - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path+instance.ID, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusNotFound) }) }) @@ -129,7 +126,7 @@ var _ = Describe("Client test", func() { It("should handle status code != 200", func() { _, err := client.GetInstanceByID(instance.ID, params) Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path+instance.ID, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusCreated) }) }) @@ -142,9 +139,7 @@ var _ = Describe("Client test", func() { It("should handle status code > 299", func() { _, err := client.GetInstanceByID(instance.ID, params) - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path+instance.ID, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) - + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusBadRequest) }) }) }) @@ -309,39 +304,33 @@ var _ = Describe("Client test", func() { }) It("should return error with status code", func() { res, err := client.Provision(instance, serviceName, planName, params, "test-user", "") - - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusOK) Expect(res).To(BeNil()) }) }) Context("And status code is unsuccessful", func() { BeforeEach(func() { - responseBody := []byte(`{ "description": "description", "error": "error"}`) + responseBody := []byte(`{ "description": "description"}`) handlerDetails[0] = HandlerDetails{Method: http.MethodPost, Path: types.ServiceInstancesURL, ResponseBody: responseBody, ResponseStatusCode: http.StatusBadRequest} }) It("should return error with url and description", func() { res, err := client.Provision(instance, serviceName, planName, params, "test-user", "") - - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "description", http.StatusBadRequest) Expect(res).To(BeNil()) }) }) Context("And invalid response body", func() { BeforeEach(func() { - responseBody := []byte(`{ "description": description", "error": "error"}`) + responseBody := []byte(`{ "description": description" }`) handlerDetails[0] = HandlerDetails{Method: http.MethodPost, Path: types.ServiceInstancesURL, ResponseBody: responseBody, ResponseStatusCode: http.StatusBadRequest} }) It("should return error without url and description if invalid response body", func() { res, err := client.Provision(instance, serviceName, planName, params, "test-user", "") - - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusBadRequest) Expect(res).To(BeNil()) }) }) @@ -390,9 +379,8 @@ var _ = Describe("Client test", func() { }) It("should handle error", func() { location, err := client.Deprovision(instance.ID, params, "test-user") - Expect(err).Should(HaveOccurred()) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusCreated) Expect(location).Should(BeEmpty()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path+instance.ID, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) }) }) @@ -496,44 +484,37 @@ var _ = Describe("Client test", func() { }) It("should return error with status code", func() { responseInstance, location, err := client.UpdateInstance(instance.ID, instance, serviceName, planName, params, "test-user", "") - - Expect(err).Should(HaveOccurred()) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusTeapot) Expect(location).Should(BeEmpty()) - verifyErrorMsg(err.Error(), handlerDetails[2].Path, handlerDetails[2].ResponseBody, handlerDetails[2].ResponseStatusCode) Expect(responseInstance).To(BeNil()) }) }) Context("And status code is unsuccessful", func() { BeforeEach(func() { - responseBody := []byte(`{ "description": "description", "error": "error"}`) + responseBody := []byte(`{ "description": "description"}`) handlerDetails = append(handlerDetails, HandlerDetails{Method: http.MethodPatch, Path: types.ServiceInstancesURL + "/" + instance.ID, ResponseBody: responseBody, ResponseStatusCode: http.StatusBadRequest}) }) It("should return error with url and description", func() { responseInstance, location, err := client.UpdateInstance(instance.ID, instance, serviceName, planName, params, "test-user", "") - - Expect(err).Should(HaveOccurred()) + expectErrorToContainSubstringAndStatusCode(err, "description", http.StatusBadRequest) Expect(location).Should(BeEmpty()) - verifyErrorMsg(err.Error(), handlerDetails[2].Path, handlerDetails[2].ResponseBody, handlerDetails[2].ResponseStatusCode) Expect(responseInstance).To(BeNil()) }) }) Context("And invalid response body", func() { BeforeEach(func() { - responseBody := []byte(`{ "description": description", "error": "error"}`) + responseBody := []byte(`{ "description": "description"}`) handlerDetails = append(handlerDetails, HandlerDetails{Method: http.MethodPatch, Path: types.ServiceInstancesURL + "/" + instance.ID, ResponseBody: responseBody, ResponseStatusCode: http.StatusBadRequest}) }) It("should return error without url and description if invalid response body", func() { responseInstance, location, err := client.UpdateInstance(instance.ID, instance, serviceName, planName, params, "test-user", "") - - Expect(err).Should(HaveOccurred()) - Expect(location).Should(BeEmpty()) - - verifyErrorMsg(err.Error(), handlerDetails[2].Path, handlerDetails[2].ResponseBody, handlerDetails[2].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "description", http.StatusBadRequest) Expect(responseInstance).To(BeNil()) + Expect(location).Should(BeEmpty()) }) }) @@ -619,8 +600,7 @@ var _ = Describe("Client test", func() { }) It("should handle status code != 200", func() { _, err := client.ListBindings(params) - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusCreated) }) }) @@ -632,8 +612,7 @@ var _ = Describe("Client test", func() { }) It("should handle status code > 299", func() { _, err := client.ListBindings(params) - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusBadRequest) }) }) }) @@ -661,8 +640,7 @@ var _ = Describe("Client test", func() { }) It("should return 404", func() { _, err := client.GetBindingByID(binding.ID, params) - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path+binding.ID, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusNotFound) }) }) @@ -674,8 +652,7 @@ var _ = Describe("Client test", func() { }) It("should handle status code != 200", func() { _, err := client.GetBindingByID(binding.ID, params) - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path+binding.ID, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusCreated) }) }) @@ -687,8 +664,7 @@ var _ = Describe("Client test", func() { }) It("should handle status code > 299", func() { _, err := client.GetBindingByID(binding.ID, params) - Expect(err).Should(HaveOccurred()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path+binding.ID, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusBadRequest) }) }) @@ -758,46 +734,39 @@ var _ = Describe("Client test", func() { }) It("should return error with status code", func() { responseBinding, location, err := client.Bind(binding, params, "test-user") - - Expect(err).Should(HaveOccurred()) - Expect(location).Should(BeEmpty()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusOK) Expect(responseBinding).To(BeNil()) + Expect(location).Should(BeEmpty()) }) }) Context("And status code is unsuccessful", func() { BeforeEach(func() { - responseBody := []byte(`{ "description": "description", "error": "error"}`) + responseBody := []byte(`{ "description": "description"}`) handlerDetails = []HandlerDetails{ {Method: http.MethodPost, Path: types.ServiceBindingsURL, ResponseBody: responseBody, ResponseStatusCode: http.StatusBadRequest}, } }) It("should return error with url and description", func() { responseBinding, location, err := client.Bind(binding, params, "test-user") - - Expect(err).Should(HaveOccurred()) - Expect(location).Should(BeEmpty()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "description", http.StatusBadRequest) Expect(responseBinding).To(BeNil()) + Expect(location).Should(BeEmpty()) }) }) Context("And invalid response body", func() { BeforeEach(func() { - responseBody := []byte(`{ "description": description", "error": "error"}`) + responseBody := []byte(`{ "description": description" }`) handlerDetails = []HandlerDetails{ {Method: http.MethodPost, Path: types.ServiceBindingsURL, ResponseBody: responseBody, ResponseStatusCode: http.StatusBadRequest}, } }) It("should return error without url and description if invalid response body", func() { responseBinding, location, err := client.Bind(binding, params, "test-user") - - Expect(err).Should(HaveOccurred()) - Expect(location).Should(BeEmpty()) - - verifyErrorMsg(err.Error(), handlerDetails[0].Path, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusBadRequest) Expect(responseBinding).To(BeNil()) + Expect(location).Should(BeEmpty()) }) }) @@ -940,9 +909,8 @@ TSTAhYWEQVZqRKYQMYGHpNlU }) It("should handle error", func() { location, err := client.Unbind(binding.ID, params, "test-user") - Expect(err).Should(HaveOccurred()) + expectErrorToContainSubstringAndStatusCode(err, "", http.StatusCreated) Expect(location).Should(BeEmpty()) - verifyErrorMsg(err.Error(), handlerDetails[0].Path+binding.ID, handlerDetails[0].ResponseBody, handlerDetails[0].ResponseStatusCode) }) }) @@ -1004,5 +972,10 @@ TSTAhYWEQVZqRKYQMYGHpNlU Expect(result).To(Equal(operation)) }) }) - }) + +func expectErrorToContainSubstringAndStatusCode(err error, substring string, statusCode int) { + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(substring)) + Expect(err.(*ServiceManagerError).StatusCode).To(Equal(statusCode)) +} diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 2fd3b46f..7b3449ef 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -3,6 +3,7 @@ package controllers import ( "context" "net/http" + "strings" "fmt" @@ -315,12 +316,17 @@ func isTransientError(ctx context.Context, err error) (bool, string) { } else { log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode)) } - return isTransientStatusCode(statusCode), errMsg + return isTransientStatusCode(statusCode, errMsg), errMsg } -func isTransientStatusCode(StatusCode int) bool { - return StatusCode == http.StatusTooManyRequests || StatusCode == http.StatusServiceUnavailable || - StatusCode == http.StatusGatewayTimeout || StatusCode == http.StatusNotFound || StatusCode == http.StatusBadGateway +func isTransientStatusCode(StatusCode int, description string) bool { + if StatusCode == http.StatusTooManyRequests || StatusCode == http.StatusServiceUnavailable || + StatusCode == http.StatusGatewayTimeout || StatusCode == http.StatusNotFound || StatusCode == http.StatusBadGateway { + return true + } + /* In case of 422 we only want to identify the error as transient if it is a concurrent operation error, + it may be also un-processable entity which is non-transient */ + return StatusCode == http.StatusUnprocessableEntity && strings.Contains(description, "Another concurrent operation in progress for this resource") } func isBrokerErrorExist(smError *sm.ServiceManagerError) bool { diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index 57460a7a..a3530b68 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -498,8 +498,8 @@ var _ = Describe("ServiceBinding controller", func() { BeforeEach(func() { errorMessage = "too many requests" fakeClient.BindReturnsOnCall(0, nil, "", &sm.ServiceManagerError{ - StatusCode: http.StatusTooManyRequests, - Message: errorMessage, + StatusCode: http.StatusTooManyRequests, + Description: errorMessage, }) fakeClient.BindReturnsOnCall(1, &smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) }) @@ -515,8 +515,8 @@ var _ = Describe("ServiceBinding controller", func() { BeforeEach(func() { errorMessage = "very bad request" fakeClient.BindReturnsOnCall(0, nil, "", &sm.ServiceManagerError{ - StatusCode: http.StatusBadRequest, - Message: errorMessage, + StatusCode: http.StatusBadRequest, + Description: errorMessage, }) }) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 9c4dffe2..005fd2ce 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -267,8 +267,8 @@ var _ = Describe("ServiceInstance controller", func() { errMessage := "failed to provision instance" JustBeforeEach(func() { fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ - StatusCode: http.StatusBadRequest, - Message: errMessage, + StatusCode: http.StatusBadRequest, + Description: errMessage, }) fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) }) @@ -283,8 +283,8 @@ var _ = Describe("ServiceInstance controller", func() { JustBeforeEach(func() { errMessage := "failed to provision instance" fakeClient.ProvisionReturnsOnCall(0, nil, &sm.ServiceManagerError{ - StatusCode: http.StatusTooManyRequests, - Message: errMessage, + StatusCode: http.StatusTooManyRequests, + Description: errMessage, }) fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) }) @@ -954,8 +954,8 @@ var _ = Describe("ServiceInstance controller", func() { It("should not attempt to share the instance", func() { errMessage := "failed to provision instance" fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ - StatusCode: http.StatusBadRequest, - Message: errMessage, + StatusCode: http.StatusBadRequest, + Description: errMessage, }) serviceInstance = createInstance(ctx, sharedInstanceSpec, false) Eventually(func() bool { @@ -1132,8 +1132,8 @@ func waitForInstanceToBeReady(instance *v1.ServiceInstance, ctx context.Context, func getNonTransientBrokerError(errMessage string) error { return &sm.ServiceManagerError{ - StatusCode: http.StatusBadRequest, - Message: "smErrMessage", + StatusCode: http.StatusBadRequest, + Description: "smErrMessage", BrokerError: &api.HTTPStatusCodeError{ StatusCode: 400, ErrorMessage: &errMessage, @@ -1142,8 +1142,8 @@ func getNonTransientBrokerError(errMessage string) error { func getTransientBrokerError(errorMessage string) error { return &sm.ServiceManagerError{ - StatusCode: http.StatusBadGateway, - Message: "smErrMessage", + StatusCode: http.StatusBadGateway, + Description: "smErrMessage", BrokerError: &api.HTTPStatusCodeError{ StatusCode: http.StatusTooManyRequests, ErrorMessage: &errorMessage, @@ -1167,36 +1167,36 @@ func instanceSharingReturnSuccess() { func instanceUnSharingReturnsNonTransientError() { fakeClient.UnShareInstanceReturns(&sm.ServiceManagerError{ - StatusCode: http.StatusBadRequest, - Message: "nonTransient", + StatusCode: http.StatusBadRequest, + Description: "nonTransient", }) } func instanceSharingReturnsNonTransientError400() { fakeClient.ShareInstanceReturns(&sm.ServiceManagerError{ - StatusCode: http.StatusBadRequest, - Message: "nonTransient", + StatusCode: http.StatusBadRequest, + Description: "nonTransient", }) } func instanceSharingReturnsNonTransientError500() { fakeClient.ShareInstanceReturns(&sm.ServiceManagerError{ - StatusCode: http.StatusInternalServerError, - Message: "nonTransient", + StatusCode: http.StatusInternalServerError, + Description: "nonTransient", }) } func instanceSharingReturnsTransientError() { fakeClient.ShareInstanceReturns(&sm.ServiceManagerError{ - StatusCode: http.StatusTooEarly, - Message: "transient", + StatusCode: http.StatusTooEarly, + Description: "transient", }) } func instanceSharingReturnsRateLimitError() { fakeClient.ShareInstanceReturns(&sm.ServiceManagerError{ - StatusCode: http.StatusTooManyRequests, - Message: "transient", + StatusCode: http.StatusTooManyRequests, + Description: "transient", }) }