diff --git a/api/common.go b/api/common.go index 79bb2e9a..ef48488e 100644 --- a/api/common.go +++ b/api/common.go @@ -1,6 +1,8 @@ package api import ( + "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -17,6 +19,30 @@ const ( ForceRotateAnnotation string = "services.cloud.sap.com/forceRotate" ) +type HTTPStatusCodeError struct { + // StatusCode is the HTTP status code returned by the broker. + StatusCode int + // ErrorMessage is a machine-readable error string that may be returned by the broker. + ErrorMessage *string + // Description is a human-readable description of the error that may be returned by the broker. + Description *string + // ResponseError is set to the error that occurred when unmarshalling a response body from the broker. + ResponseError error +} + +func (e HTTPStatusCodeError) Error() string { + errorMessage := "" + description := "" + + if e.ErrorMessage != nil { + errorMessage = *e.ErrorMessage + } + if e.Description != nil { + description = *e.Description + } + return fmt.Sprintf("Status: %v; ErrorMessage: %v; Description: %v; ResponseError: %v", e.StatusCode, errorMessage, description, e.ResponseError) +} + const ( // ConditionSucceeded represents whether the last operation CREATE/UPDATE/DELETE was successful. ConditionSucceeded = "Succeeded" diff --git a/client/sm/client.go b/client/sm/client.go index 6b888c1e..9cc2c7a4 100644 --- a/client/sm/client.go +++ b/client/sm/client.go @@ -28,6 +28,7 @@ import ( "strconv" "strings" + "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm/types" "github.com/SAP/sap-btp-service-operator/internal/auth" "github.com/SAP/sap-btp-service-operator/internal/httputil" @@ -67,8 +68,9 @@ type Client interface { Call(method string, smpath string, body io.Reader, q *Parameters) (*http.Response, error) } type ServiceManagerError struct { - Message string - StatusCode int + Message string + StatusCode int + BrokerError *api.HTTPStatusCodeError `json:"broker_error,omitempty"` } func (e *ServiceManagerError) Error() string { diff --git a/controllers/base_controller.go b/controllers/base_controller.go index b8741bb0..2fd3b46f 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -137,10 +137,7 @@ func (r *BaseReconciler) updateStatus(ctx context.Context, object api.SAPBTPReso func (r *BaseReconciler) init(ctx context.Context, obj api.SAPBTPResource) error { obj.SetReady(metav1.ConditionFalse) setInProgressConditions(smClientTypes.CREATE, "Pending", obj) - if err := r.updateStatus(ctx, obj); err != nil { - return err - } - return nil + return r.updateStatus(ctx, obj) } func getConditionReason(opType smClientTypes.OperationCategory, state smClientTypes.OperationState) string { @@ -302,21 +299,51 @@ func isDelete(object metav1.ObjectMeta) bool { return !object.DeletionTimestamp.IsZero() } -func isTransientError(ctx context.Context, err error) bool { +func isTransientError(ctx context.Context, err error) (bool, string) { log := GetLogger(ctx) - if smError, ok := err.(*sm.ServiceManagerError); ok { + smError, ok := err.(*sm.ServiceManagerError) + if !ok { + return false, err.Error() + } + + statusCode := smError.StatusCode + errMsg := smError.Error() + if isBrokerErrorExist(smError) { + log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) + statusCode = smError.BrokerError.StatusCode + errMsg = smError.BrokerError.Error() + } else { log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode)) - return smError.StatusCode == http.StatusTooManyRequests || smError.StatusCode == http.StatusServiceUnavailable || - smError.StatusCode == http.StatusGatewayTimeout || smError.StatusCode == http.StatusNotFound || smError.StatusCode == http.StatusBadGateway } - return false + return isTransientStatusCode(statusCode), errMsg +} + +func isTransientStatusCode(StatusCode int) bool { + return StatusCode == http.StatusTooManyRequests || StatusCode == http.StatusServiceUnavailable || + StatusCode == http.StatusGatewayTimeout || StatusCode == http.StatusNotFound || StatusCode == http.StatusBadGateway +} + +func isBrokerErrorExist(smError *sm.ServiceManagerError) bool { + return smError.BrokerError != nil && smError.BrokerError.StatusCode != 0 +} + +func (r *BaseReconciler) handleError(ctx context.Context, operationType smClientTypes.OperationCategory, err error, resource api.SAPBTPResource) (ctrl.Result, error) { + var ( + isTransient bool + errMsg string + ) + + if isTransient, errMsg = isTransientError(ctx, err); isTransient { + return r.markAsTransientError(ctx, operationType, errMsg, resource) + } + return r.markAsNonTransientError(ctx, operationType, errMsg, resource) } -func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, nonTransientErr error, object api.SAPBTPResource) (ctrl.Result, error) { +func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, errMsg string, object api.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) - setFailureConditions(operationType, nonTransientErr.Error(), object) + setFailureConditions(operationType, errMsg, object) if operationType != smClientTypes.DELETE { - log.Info(fmt.Sprintf("operation %s of %s encountered a non transient error %s, giving up operation :(", operationType, object.GetControllerName(), nonTransientErr.Error())) + log.Info(fmt.Sprintf("operation %s of %s encountered a non transient error %s, giving up operation :(", operationType, object.GetControllerName(), errMsg)) } object.SetObservedGeneration(object.GetGeneration()) err := r.updateStatus(ctx, object) @@ -324,21 +351,20 @@ func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationT return ctrl.Result{}, err } if operationType == smClientTypes.DELETE { - return ctrl.Result{}, nonTransientErr + return ctrl.Result{}, fmt.Errorf(errMsg) } return ctrl.Result{}, nil } -func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, transientErr error, object api.SAPBTPResource) (ctrl.Result, error) { +func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, errMsg string, object api.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) - if smError, ok := transientErr.(*sm.ServiceManagerError); ok && smError.StatusCode != http.StatusTooManyRequests { - setInProgressConditions(operationType, transientErr.Error(), object) - log.Info(fmt.Sprintf("operation %s of %s encountered a transient error %s, retrying operation :)", operationType, object.GetControllerName(), transientErr.Error())) - if err := r.updateStatus(ctx, object); err != nil { - return ctrl.Result{}, err - } + setInProgressConditions(operationType, errMsg, object) + log.Info(fmt.Sprintf("operation %s of %s encountered a transient error %s, retrying operation :)", operationType, object.GetControllerName(), errMsg)) + if err := r.updateStatus(ctx, object); err != nil { + return ctrl.Result{}, err } - return ctrl.Result{}, transientErr + + return ctrl.Result{}, fmt.Errorf(errMsg) } func isInProgress(object api.SAPBTPResource) bool { diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index 3d40126d..479896cb 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -85,7 +85,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque smClient, err := r.getSMClient(ctx, serviceBinding) if err != nil { - return r.markAsTransientError(ctx, Unknown, err, serviceBinding) + return r.markAsTransientError(ctx, Unknown, err.Error(), serviceBinding) } if len(serviceBinding.Status.OperationURL) > 0 { @@ -173,7 +173,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque binding, err := r.getBindingForRecovery(ctx, smClient, serviceBinding) if err != nil { log.Error(err, "failed to check binding recovery") - return r.markAsTransientError(ctx, smClientTypes.CREATE, err, serviceBinding) + return r.markAsTransientError(ctx, smClientTypes.CREATE, err.Error(), serviceBinding) } if binding != nil { // Recovery - restore binding from SM @@ -209,7 +209,7 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s _, bindingParameters, err := buildParameters(r.Client, serviceBinding.Namespace, serviceBinding.Spec.ParametersFrom, serviceBinding.Spec.Parameters) if err != nil { log.Error(err, "failed to parse smBinding parameters") - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, err, serviceBinding) + return r.markAsNonTransientError(ctx, smClientTypes.CREATE, err.Error(), serviceBinding) } smBinding, operationURL, bindErr := smClient.Bind(&smClientTypes.ServiceBinding{ @@ -225,16 +225,13 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s if bindErr != nil { log.Error(err, "failed to create service binding", "serviceInstanceID", serviceInstance.Status.InstanceID) - if isTransientError(ctx, bindErr) { - return r.markAsTransientError(ctx, smClientTypes.CREATE, bindErr, serviceBinding) - } - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, bindErr, serviceBinding) + return r.handleError(ctx, smClientTypes.CREATE, bindErr, serviceBinding) } if operationURL != "" { var bindingID string if bindingID = sm.ExtractBindingID(operationURL); len(bindingID) == 0 { - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, fmt.Errorf("failed to extract smBinding ID from operation URL %s", operationURL), serviceBinding) + return r.markAsNonTransientError(ctx, smClientTypes.CREATE, fmt.Sprintf("failed to extract smBinding ID from operation URL %s", operationURL), serviceBinding) } serviceBinding.Status.BindingID = bindingID @@ -295,7 +292,7 @@ func (r *ServiceBindingReconciler) delete(ctx context.Context, smClient sm.Clien operationURL, unbindErr := smClient.Unbind(serviceBinding.Status.BindingID, nil, buildUserInfo(ctx, serviceBinding.Spec.UserInfo)) if unbindErr != nil { // delete will proceed anyway - return r.markAsNonTransientError(ctx, smClientTypes.DELETE, unbindErr, serviceBinding) + return r.markAsNonTransientError(ctx, smClientTypes.DELETE, unbindErr.Error(), serviceBinding) } if operationURL != "" { @@ -336,8 +333,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, smClient sm.Client, } if status == nil { - err := fmt.Errorf("failed to get last operation status of %s", serviceBinding.Name) - return r.markAsTransientError(ctx, serviceBinding.Status.OperationType, err, serviceBinding) + return r.markAsTransientError(ctx, serviceBinding.Status.OperationType, fmt.Sprintf("failed to get last operation status of %s", serviceBinding.Name), serviceBinding) } switch status.State { case smClientTypes.INPROGRESS: @@ -689,9 +685,9 @@ func (r *ServiceBindingReconciler) handleSecretError(ctx context.Context, op smC log := GetLogger(ctx) log.Error(err, fmt.Sprintf("failed to store secret %s for binding %s", binding.Spec.SecretName, binding.Name)) if apierrors.ReasonForError(err) == metav1.StatusReasonUnknown { - return r.markAsNonTransientError(ctx, op, err, binding) + return r.markAsNonTransientError(ctx, op, err.Error(), binding) } - return r.markAsTransientError(ctx, op, err, binding) + return r.markAsTransientError(ctx, op, err.Error(), binding) } func (r *ServiceBindingReconciler) addInstanceInfo(ctx context.Context, binding *servicesv1.ServiceBinding, credentialsMap map[string][]byte) ([]SecretMetadataProperty, error) { diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index aea216f2..57460a7a 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -44,20 +44,7 @@ var _ = Describe("ServiceBinding controller", func() { var guid string createBindingWithoutAssertionsAndWait := func(ctx context.Context, name string, namespace string, instanceName string, externalName string, wait bool) (*v1.ServiceBinding, error) { - binding := newBindingObject(name, namespace) - binding.Spec.ServiceInstanceName = instanceName - binding.Spec.ExternalName = externalName - binding.Spec.Parameters = &runtime.RawExtension{ - Raw: []byte(`{"key": "value"}`), - } - binding.Spec.ParametersFrom = []v1.ParametersFromSource{ - { - SecretKeyRef: &v1.SecretKeyReference{ - Name: "param-secret", - Key: "secret-parameter", - }, - }, - } + binding := generateBasicBindingTemplate(name, namespace, instanceName, externalName) if err := k8sClient.Create(ctx, binding); err != nil { return nil, err @@ -538,6 +525,34 @@ var _ = Describe("ServiceBinding controller", func() { }) }) + When("SM returned error 502 and broker returned 429", func() { + BeforeEach(func() { + errorMessage = "too many requests from broker" + fakeClient.BindReturns(nil, "", getTransientBrokerError(errorMessage)) + }) + + It("should detect the error as transient and eventually succeed", func() { + createdBinding, _ := createBindingWithoutAssertionsAndWait(context.Background(), + bindingName, bindingTestNamespace, instanceName, "binding-external-name", false) + expectBindingToBeInFailedStateWithMsg(createdBinding, errorMessage) + + fakeClient.BindReturns(&smClientTypes.ServiceBinding{ID: fakeBindingID, + Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) + validateBindingIsReady(createdBinding, bindingName) + }) + }) + + When("SM returned 502 and broker returned 400", func() { + BeforeEach(func() { + errorMessage = "very bad request" + fakeClient.BindReturnsOnCall(0, nil, "", getNonTransientBrokerError(errorMessage)) + }) + + It("should detect the error as non-transient and fail", func() { + createBindingWithError(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", errorMessage) + }) + }) + }) When("SM returned invalid credentials json", func() { @@ -1167,6 +1182,41 @@ var _ = Describe("ServiceBinding controller", func() { }) }) +func expectBindingToBeInFailedStateWithMsg(binding *v1.ServiceBinding, message string) { + cond := meta.FindStatusCondition(binding.GetConditions(), api.ConditionSucceeded) + Expect(cond).To(Not(BeNil())) + Expect(cond.Message).To(ContainSubstring(message)) + Expect(cond.Status).To(Equal(metav1.ConditionFalse)) +} + +func validateBindingIsReady(createdBinding *v1.ServiceBinding, bindingName string) { + Eventually(func() bool { + err := k8sClient.Get(context.Background(), types.NamespacedName{Name: bindingName, Namespace: bindingTestNamespace}, createdBinding) + if err != nil { + return false + } + return isReady(createdBinding) + }, timeout, interval).Should(BeTrue()) +} + +func generateBasicBindingTemplate(name, namespace, instanceName, externalName string) *v1.ServiceBinding { + binding := newBindingObject(name, namespace) + binding.Spec.ServiceInstanceName = instanceName + binding.Spec.ExternalName = externalName + binding.Spec.Parameters = &runtime.RawExtension{ + Raw: []byte(`{"key": "value"}`), + } + binding.Spec.ParametersFrom = []v1.ParametersFromSource{ + { + SecretKeyRef: &v1.SecretKeyReference{ + Name: "param-secret", + Key: "secret-parameter", + }, + }, + } + return binding +} + func validateSecretData(secret *corev1.Secret, expectedKey string, expectedValue string) { Expect(secret.Data).ToNot(BeNil()) Expect(secret.Data).To(HaveKey(expectedKey)) diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 45797c3d..e8d5f629 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -81,7 +81,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ smClient, err := r.getSMClient(ctx, serviceInstance) if err != nil { - return r.markAsTransientError(ctx, Unknown, err, serviceInstance) + return r.markAsTransientError(ctx, Unknown, err.Error(), serviceInstance) } if len(serviceInstance.Status.OperationURL) > 0 { @@ -112,7 +112,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ instance, err := r.getInstanceForRecovery(ctx, smClient, serviceInstance) if err != nil { log.Error(err, "failed to check instance recovery") - return r.markAsTransientError(ctx, Unknown, err, serviceInstance) + return r.markAsTransientError(ctx, Unknown, err.Error(), serviceInstance) } if instance != nil { log.Info(fmt.Sprintf("found existing instance in SM with id %s, updating status", instance.ID)) @@ -257,7 +257,7 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient if err != nil { // if parameters are invalid there is nothing we can do, the user should fix it according to the error message in the condition log.Error(err, "failed to parse instance parameters") - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, err, serviceInstance) + return r.markAsNonTransientError(ctx, smClientTypes.CREATE, err.Error(), serviceInstance) } provision, provisionErr := smClient.Provision(&smClientTypes.ServiceInstance{ @@ -274,10 +274,7 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient if provisionErr != nil { log.Error(provisionErr, "failed to create service instance", "serviceOfferingName", serviceInstance.Spec.ServiceOfferingName, "servicePlanName", serviceInstance.Spec.ServicePlanName) - if isTransientError(ctx, provisionErr) { - return r.markAsTransientError(ctx, smClientTypes.CREATE, provisionErr, serviceInstance) - } - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, provisionErr, serviceInstance) + return r.handleError(ctx, smClientTypes.CREATE, provisionErr, serviceInstance) } if provision.Location != "" { @@ -320,7 +317,6 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient } func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1.ServiceInstance) (ctrl.Result, error) { - var err error log := GetLogger(ctx) log.Info(fmt.Sprintf("updating instance %s in SM", serviceInstance.Status.InstanceID)) @@ -329,7 +325,7 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient _, instanceParameters, err := buildParameters(r.Client, serviceInstance.Namespace, serviceInstance.Spec.ParametersFrom, serviceInstance.Spec.Parameters) if err != nil { log.Error(err, "failed to parse instance parameters") - return r.markAsNonTransientError(ctx, smClientTypes.UPDATE, fmt.Errorf("failed to parse parameters: %v", err.Error()), serviceInstance) + return r.markAsNonTransientError(ctx, smClientTypes.UPDATE, fmt.Sprintf("failed to parse parameters: %v", err.Error()), serviceInstance) } _, operationURL, err := smClient.UpdateInstance(serviceInstance.Status.InstanceID, &smClientTypes.ServiceInstance{ @@ -340,10 +336,7 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient if err != nil { log.Error(err, fmt.Sprintf("failed to update service instance with ID %s", serviceInstance.Status.InstanceID)) - if isTransientError(ctx, err) { - return r.markAsTransientError(ctx, smClientTypes.UPDATE, err, serviceInstance) - } - return r.markAsNonTransientError(ctx, smClientTypes.UPDATE, err, serviceInstance) + return r.handleError(ctx, smClientTypes.UPDATE, err, serviceInstance) } if operationURL != "" { @@ -387,7 +380,7 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, smClient operationURL, deprovisionErr := smClient.Deprovision(serviceInstance.Status.InstanceID, nil, buildUserInfo(ctx, serviceInstance.Spec.UserInfo)) if deprovisionErr != nil { // delete will proceed anyway - return r.markAsNonTransientError(ctx, smClientTypes.DELETE, deprovisionErr, serviceInstance) + return r.markAsNonTransientError(ctx, smClientTypes.DELETE, deprovisionErr.Error(), serviceInstance) } if operationURL != "" { @@ -518,9 +511,9 @@ func (r *ServiceInstanceReconciler) HandleInstanceSharingError(ctx context.Conte reason = InProgress } else if reason == ShareFailed && (smError.StatusCode == http.StatusBadRequest || smError.StatusCode == http.StatusInternalServerError) { - // non-transient error may occur only when sharing - // SM return 400 when plan is not sharable - // SM returns 500 when TOGGLES_ENABLE_INSTANCE_SHARE_FROM_OPERATOR feature toggle is off + /* non-transient error may occur only when sharing + SM return 400 when plan is not sharable + SM returns 500 when TOGGLES_ENABLE_INSTANCE_SHARE_FROM_OPERATOR feature toggle is off */ setSharedCondition(object, status, ShareNotSupported, err.Error()) return r.updateStatus(ctx, object) } @@ -554,6 +547,11 @@ func updateRequired(serviceInstance *servicesv1.ServiceInstance) bool { return false } + cond := meta.FindStatusCondition(serviceInstance.Status.Conditions, api.ConditionSucceeded) + if cond != nil && cond.Reason == UpdateInProgress { + return true + } + if getSpecHash(serviceInstance) == serviceInstance.Status.HashedSpec { return false } diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 43ecb0e6..9c4dffe2 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -2,14 +2,14 @@ package controllers import ( "context" - "github.com/SAP/sap-btp-service-operator/api" - "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/utils/pointer" + "fmt" "net/http" "strings" - "fmt" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/utils/pointer" + "github.com/SAP/sap-btp-service-operator/api" v1 "github.com/SAP/sap-btp-service-operator/api/v1" "github.com/SAP/sap-btp-service-operator/client/sm" "github.com/SAP/sap-btp-service-operator/client/sm/smfakes" @@ -29,12 +29,11 @@ import ( // +kubebuilder:docs-gen:collapse=Imports const ( - fakeInstanceID = "ic-fake-instance-id" - fakeInstanceExternalName = "ic-test-instance-external-name" - fakeInstanceExternalNameNonShared = "ic-test-instance-external-name-non-shared" - testNamespace = "ic-test-namespace" - fakeOfferingName = "offering-a" - fakePlanName = "plan-a" + fakeInstanceID = "ic-fake-instance-id" + fakeInstanceExternalName = "ic-test-instance-external-name" + testNamespace = "ic-test-namespace" + fakeOfferingName = "offering-a" + fakePlanName = "plan-a" ) var _ = Describe("ServiceInstance controller", func() { @@ -140,7 +139,6 @@ var _ = Describe("ServiceInstance controller", func() { isConditionRefersUpdateOp := func(instance *v1.ServiceInstance) bool { conditionReason := instance.Status.Conditions[0].Reason return strings.Contains(conditionReason, Updated) || strings.Contains(conditionReason, UpdateInProgress) || strings.Contains(conditionReason, UpdateFailed) - } _ = k8sClient.Update(ctx, serviceInstance) @@ -265,37 +263,25 @@ var _ = Describe("ServiceInstance controller", func() { }) When("provision request to SM fails", func() { - var errMessage string Context("with 400 status", func() { + errMessage := "failed to provision instance" JustBeforeEach(func() { - errMessage = "failed to provision instance" fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ StatusCode: http.StatusBadRequest, Message: errMessage, }) fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) - }) It("should have failure condition", func() { serviceInstance = createInstance(ctx, instanceSpec, false) - Eventually(func() bool { - err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance) - if err != nil { - return false - } - if len(serviceInstance.Status.Conditions) != 3 { - return false - } - cond := meta.FindStatusCondition(serviceInstance.Status.Conditions, api.ConditionSucceeded) - return cond != nil && cond.Status == metav1.ConditionFalse && strings.Contains(cond.Message, errMessage) - }, timeout, interval).Should(BeTrue()) + expectForInstanceCreationFailure(ctx, defaultLookupKey, serviceInstance, errMessage) }) }) Context("with 429 status eventually succeeds", func() { JustBeforeEach(func() { - errMessage = "failed to provision instance" + errMessage := "failed to provision instance" fakeClient.ProvisionReturnsOnCall(0, nil, &sm.ServiceManagerError{ StatusCode: http.StatusTooManyRequests, Message: errMessage, @@ -309,6 +295,33 @@ var _ = Describe("ServiceInstance controller", func() { Expect(meta.IsStatusConditionPresentAndEqual(serviceInstance.Status.Conditions, api.ConditionSucceeded, metav1.ConditionTrue)).To(Equal(true)) }) }) + + Context("with sm status code 502 and broker status code 429", func() { + errorMessage := "broker too many requests" + JustBeforeEach(func() { + fakeClient.ProvisionReturns(nil, getTransientBrokerError(errorMessage)) + }) + + It("should be transient error and eventually succeed", func() { + serviceInstance = createInstance(ctx, instanceSpec, false) + expectForInstanceCreationFailure(ctx, defaultLookupKey, serviceInstance, errorMessage) + fakeClient.ProvisionReturns(&sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + waitForInstanceToBeReady(serviceInstance, ctx, defaultLookupKey) + }) + }) + + Context("with sm status code 502 and broker status code 400", func() { + errMessage := "failed to provision instance" + JustBeforeEach(func() { + fakeClient.ProvisionReturns(nil, getNonTransientBrokerError(errMessage)) + fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + }) + + It("should have failure condition - non transient error", func() { + serviceInstance = createInstance(ctx, instanceSpec, false) + expectForInstanceCreationFailure(ctx, defaultLookupKey, serviceInstance, errMessage) + }) + }) }) }) @@ -330,10 +343,7 @@ var _ = Describe("ServiceInstance controller", func() { Type: smClientTypes.CREATE, State: smClientTypes.SUCCEEDED, }, nil) - Eventually(func() bool { - _ = k8sClient.Get(ctx, defaultLookupKey, serviceInstance) - return isReady(serviceInstance) - }, timeout, interval).Should(BeTrue()) + waitForInstanceToBeReady(serviceInstance, ctx, defaultLookupKey) }) }) @@ -534,6 +544,23 @@ var _ = Describe("ServiceInstance controller", func() { }) }) + Context("spec is changed, sm returns 502 and broker returns 429", func() { + errMessage := "broker too many requests" + JustBeforeEach(func() { + fakeClient.UpdateInstanceReturns(nil, "", getTransientBrokerError(errMessage)) + }) + + It("recognize the error as transient and eventually succeed", func() { + serviceInstance.Spec = updateSpec() + updatedInstance := updateInstance(ctx, serviceInstance) + Expect(updatedInstance.Status.Conditions[0].Reason).To(Equal(UpdateInProgress)) + Expect(updatedInstance.Status.Conditions[0].Message).To(ContainSubstring(errMessage)) + fakeClient.UpdateInstanceReturns(nil, "", nil) + updatedInstance = updateInstance(ctx, serviceInstance) + waitForInstanceToBeReady(updatedInstance, ctx, defaultLookupKey) + }) + }) + Context("Async", func() { When("spec is changed", func() { BeforeEach(func() { @@ -1096,6 +1123,44 @@ var _ = Describe("ServiceInstance controller", func() { }) }) +func waitForInstanceToBeReady(instance *v1.ServiceInstance, ctx context.Context, key types.NamespacedName) { + Eventually(func() bool { + _ = k8sClient.Get(ctx, key, instance) + return isReady(instance) + }, timeout, interval).Should(BeTrue()) +} + +func getNonTransientBrokerError(errMessage string) error { + return &sm.ServiceManagerError{ + StatusCode: http.StatusBadRequest, + Message: "smErrMessage", + BrokerError: &api.HTTPStatusCodeError{ + StatusCode: 400, + ErrorMessage: &errMessage, + }} +} + +func getTransientBrokerError(errorMessage string) error { + return &sm.ServiceManagerError{ + StatusCode: http.StatusBadGateway, + Message: "smErrMessage", + BrokerError: &api.HTTPStatusCodeError{ + StatusCode: http.StatusTooManyRequests, + ErrorMessage: &errorMessage, + }, + } +} + +func expectForInstanceCreationFailure(ctx context.Context, defaultLookupKey types.NamespacedName, serviceInstance *v1.ServiceInstance, errMessage string) { + Eventually(func() bool { + if err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance); err != nil { + return false + } + cond := meta.FindStatusCondition(serviceInstance.Status.Conditions, api.ConditionSucceeded) + return cond != nil && cond.Status == metav1.ConditionFalse && strings.Contains(cond.Message, errMessage) + }, timeout, interval).Should(BeTrue()) +} + func instanceSharingReturnSuccess() { fakeClient.ShareInstanceReturns(nil) } diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 5b674872..e838e0dc 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -21,10 +21,11 @@ import ( "crypto/tls" "net" "path/filepath" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "testing" "time" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/api/v1/webhooks" "github.com/SAP/sap-btp-service-operator/client/sm"