retry failed operations

Co-authored-by: kerenlahav <[email protected]>
Co-authored-by: I501080 <[email protected]>

Author: emilsap

client/sm/client.go

@@ -504,7 +504,7 @@ func (client *serviceManagerClient) getPlanInfo(planID string, serviceName strin
 	}
 
 	if len(planID) > 0 {
-		err = fmt.Errorf("the provided plan ID '%s' doesn't match the provided offering name '%s' and plan name '%s", planID, serviceName, planName)
+		err = fmt.Errorf("the provided plan ID '%s' doesn't match the provided offering name '%s' and plan name '%s'", planID, serviceName, planName)
 	} else {
 		err = fmt.Errorf("ambiguity error: found more than one resource that matches the provided offering name '%s' and plan name '%s'. Please provide servicePlanID", serviceName, planName)
 	}

config/samples/reference_instance.yaml

@@ -0,0 +1,9 @@
+apiVersion: services.cloud.sap.com/v1
+kind: ServiceInstance
+metadata:
+  name: xsuaa-reference-instance
+spec:
+  serviceOfferingName: xsuaa
+  servicePlanName: reference-instance
+  parameters:
+    referenced_instance_id: xxx

config/samples/services_v1_servicebinding_cred_rotation.yaml

@@ -2,7 +2,7 @@ apiVersion: services.cloud.sap.com/v1
 kind: ServiceBinding
 metadata:
   name: sample-binding-rotate
-  labels:
+  annotations:
     "services.cloud.sap.com/forceRotate": "true"
 spec:
   serviceInstanceName: sample-instance-1

controllers/servicebinding_controller.go

@@ -196,13 +196,13 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 
 		smClient, err := r.GetSMClient(ctx, serviceInstance)
 		if err != nil {
-			return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceBinding)
+			return utils.HandleOperationFailure(ctx, r.Client, serviceBinding, common.Unknown, err)
 		}
 
 		smBinding, err := r.getBindingForRecovery(ctx, smClient, serviceBinding)
 		if err != nil {
 			log.Error(err, "failed to check binding recovery")
-			return utils.MarkAsTransientError(ctx, r.Client, smClientTypes.CREATE, err, serviceBinding)
+			return utils.HandleServiceManagerError(ctx, r.Client, serviceBinding, smClientTypes.CREATE, err)
 		}
 		if smBinding != nil {
 			return r.recover(ctx, serviceBinding, smBinding)
@@ -230,7 +230,7 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s
 	bindingParameters, _, err := utils.BuildSMRequestParameters(serviceBinding.Namespace, serviceBinding.Spec.Parameters, serviceBinding.Spec.ParametersFrom)
 	if err != nil {
 		log.Error(err, "failed to parse smBinding parameters")
-		return utils.MarkAsNonTransientError(ctx, r.Client, smClientTypes.CREATE, err, serviceBinding)
+		return utils.HandleOperationFailure(ctx, r.Client, serviceBinding, smClientTypes.CREATE, err)
 	}
 
 	smBinding, operationURL, bindErr := smClient.Bind(&smClientTypes.ServiceBinding{
@@ -246,13 +246,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)
-		return utils.HandleError(ctx, r.Client, smClientTypes.CREATE, bindErr, serviceBinding)
+		return utils.HandleServiceManagerError(ctx, r.Client, serviceBinding, smClientTypes.CREATE, bindErr)
 	}
 
 	if operationURL != "" {
 		var bindingID string
 		if bindingID = sm.ExtractBindingID(operationURL); len(bindingID) == 0 {
-			return utils.MarkAsNonTransientError(ctx, r.Client, smClientTypes.CREATE, fmt.Errorf("failed to extract smBinding ID from operation URL %s", operationURL), serviceBinding)
+			return utils.HandleOperationFailure(ctx, r.Client, serviceBinding, smClientTypes.CREATE, fmt.Errorf("failed to extract smBinding ID from operation URL %s", operationURL))
 		}
 		serviceBinding.Status.BindingID = bindingID
 
@@ -264,7 +264,7 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s
 			log.Error(err, "unable to update ServiceBinding status")
 			return ctrl.Result{}, err
 		}
-		return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, nil
+		return ctrl.Result{RequeueAfter: r.Config.PollInterval}, nil
 	}
 
 	log.Info("Binding created successfully")
@@ -292,14 +292,14 @@ func (r *ServiceBindingReconciler) delete(ctx context.Context, serviceBinding *v
 	if controllerutil.ContainsFinalizer(serviceBinding, common.FinalizerName) {
 		smClient, err := r.GetSMClient(ctx, serviceInstance)
 		if err != nil {
-			return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceBinding)
+			return utils.HandleOperationFailure(ctx, r.Client, serviceBinding, smClientTypes.DELETE, err)
 		}
 
 		if len(serviceBinding.Status.BindingID) == 0 {
 			log.Info("No binding id found validating binding does not exists in SM before removing finalizer")
 			smBinding, err := r.getBindingForRecovery(ctx, smClient, serviceBinding)
 			if err != nil {
-				return ctrl.Result{}, err
+				return utils.HandleServiceManagerError(ctx, r.Client, serviceBinding, smClientTypes.DELETE, err)
 			}
 			if smBinding != nil {
 				log.Info("binding exists in SM continue with deletion")
@@ -328,7 +328,7 @@ func (r *ServiceBindingReconciler) delete(ctx context.Context, serviceBinding *v
 		log.Info(fmt.Sprintf("Deleting binding with id %v from SM", serviceBinding.Status.BindingID))
 		operationURL, unbindErr := smClient.Unbind(serviceBinding.Status.BindingID, nil, utils.BuildUserInfo(ctx, serviceBinding.Spec.UserInfo))
 		if unbindErr != nil {
-			return utils.HandleDeleteError(ctx, r.Client, unbindErr, serviceBinding)
+			return utils.HandleServiceManagerError(ctx, r.Client, serviceBinding, smClientTypes.DELETE, unbindErr)
 		}
 
 		if operationURL != "" {
@@ -339,7 +339,7 @@ func (r *ServiceBindingReconciler) delete(ctx context.Context, serviceBinding *v
 			if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil {
 				return ctrl.Result{}, err
 			}
-			return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, nil
+			return ctrl.Result{RequeueAfter: r.Config.PollInterval}, nil
 		}
 
 		log.Info("Binding was deleted successfully")
@@ -354,7 +354,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *v1.
 
 	smClient, err := r.GetSMClient(ctx, serviceInstance)
 	if err != nil {
-		return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceBinding)
+		return utils.HandleOperationFailure(ctx, r.Client, serviceBinding, common.Unknown, err)
 	}
 
 	status, statusErr := smClient.Status(serviceBinding.Status.OperationURL, nil)
@@ -375,7 +375,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *v1.
 	}
 
 	if status == nil {
-		return utils.MarkAsTransientError(ctx, r.Client, serviceBinding.Status.OperationType, fmt.Errorf("failed to get last operation status of %s", serviceBinding.Name), serviceBinding)
+		return utils.HandleOperationFailure(ctx, r.Client, serviceBinding, serviceBinding.Status.OperationType, fmt.Errorf("failed to get last operation status of %s", serviceBinding.Name))
 	}
 	switch status.State {
 	case smClientTypes.INPROGRESS:
@@ -388,9 +388,9 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *v1.
 				return ctrl.Result{}, err
 			}
 		}
-		return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, nil
+		return ctrl.Result{RequeueAfter: r.Config.PollInterval}, nil
 	case smClientTypes.FAILED:
-		// non transient error - should not retry
+		// if async operation failed we should not retry
 		utils.SetFailureConditions(status.Type, status.Description, serviceBinding, true)
 		if serviceBinding.Status.OperationType == smClientTypes.DELETE {
 			serviceBinding.Status.OperationURL = ""
@@ -822,10 +822,7 @@ func (r *ServiceBindingReconciler) validateSecretNameIsAvailable(ctx context.Con
 func (r *ServiceBindingReconciler) handleSecretError(ctx context.Context, op smClientTypes.OperationCategory, err error, binding *v1.ServiceBinding) (ctrl.Result, error) {
 	log := utils.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 utils.MarkAsNonTransientError(ctx, r.Client, op, err, binding)
-	}
-	return utils.MarkAsTransientError(ctx, r.Client, op, err, binding)
+	return utils.HandleOperationFailure(ctx, r.Client, binding, op, err)
 }
 
 func (r *ServiceBindingReconciler) getInstanceInfo(ctx context.Context, binding *v1.ServiceBinding) (map[string]string, error) {

controllers/servicebinding_controller_test.go

@@ -361,7 +361,7 @@ var _ = Describe("ServiceBinding controller", func() {
 					It("should fail with the error returned from SM", func() {
 						binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false)
 						Expect(err).ToNot(HaveOccurred())
-						waitForResourceCondition(ctx, binding, common.ConditionFailed, metav1.ConditionTrue, "", errorMessage)
+						waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, "", errorMessage)
 					})
 				})
 
@@ -394,7 +394,7 @@ var _ = Describe("ServiceBinding controller", func() {
 					It("should fail", func() {
 						binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false)
 						Expect(err).ToNot(HaveOccurred())
-						waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateInProgress, errorMessage)
+						waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateFailed, errorMessage)
 					})
 				})
 
@@ -434,7 +434,7 @@ var _ = Describe("ServiceBinding controller", func() {
 					It("should detect the error as non-transient and fail", func() {
 						binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false)
 						Expect(err).ToNot(HaveOccurred())
-						waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateInProgress, errorMessage)
+						waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateFailed, errorMessage)
 					})
 				})
 
@@ -447,7 +447,7 @@ var _ = Describe("ServiceBinding controller", func() {
 
 				It("creation will fail with appropriate message", func() {
 					createdBinding, _ = createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", "", false)
-					waitForResourceCondition(ctx, createdBinding, common.ConditionFailed, metav1.ConditionTrue, "CreateFailed", "failed to create secret")
+					waitForResourceCondition(ctx, createdBinding, common.ConditionSucceeded, metav1.ConditionFalse, "", "failed to create secret")
 				})
 			})
 		})
@@ -646,7 +646,7 @@ stringData:
 				                                         name: my-secret-name`)
 				binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, false)
 				Expect(err).ToNot(HaveOccurred())
-				waitForResourceCondition(ctx, binding, common.ConditionFailed, metav1.ConditionTrue, "", "the Secret template is invalid: Secret's metadata field")
+				waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, "", "the Secret template is invalid: Secret's metadata field")
 			})
 			It("should fail to create the secret if wrong template key in the spec.secretTemplate is provided", func() {
 				ctx := context.Background()
@@ -664,7 +664,7 @@ stringData:
 						return false
 					}
 					cond := meta.FindStatusCondition(binding.GetConditions(), common.ConditionSucceeded)
-					return cond != nil && cond.Reason == "CreateFailed" && strings.Contains(cond.Message, "map has no entry for key \"non_existing_key\"")
+					return cond != nil && cond.Reason == common.CreateFailed && strings.Contains(cond.Message, "map has no entry for key \"non_existing_key\"")
 				}, timeout*2, interval).Should(BeTrue())
 			})
 			It("should fail to create the secret if secretTemplate is an unexpected type", func() {
@@ -674,7 +674,7 @@ stringData:
 				                                       kind: Pod`)
 				binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, false)
 				Expect(err).ToNot(HaveOccurred())
-				waitForResourceCondition(ctx, binding, common.ConditionFailed, metav1.ConditionTrue, "", "but needs to be of kind 'Secret'")
+				waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, "", "but needs to be of kind 'Secret'")
 			})
 			It("should succeed to create the secret- empty data", func() {
 				ctx := context.Background()
@@ -884,7 +884,7 @@ stringData:
 			err := k8sClient.Update(ctx, createdBinding)
 			Expect(err).ToNot(HaveOccurred())
 			By("Verify binding update failed")
-			waitForResourceCondition(ctx, createdBinding, common.ConditionFailed, metav1.ConditionTrue, "UpdateFailed", "failed to create secret")
+			waitForResourceCondition(ctx, createdBinding, common.ConditionSucceeded, metav1.ConditionFalse, "UpdateFailed", "failed to create secret from template")
 
 			secretTemplate = dedent.Dedent(
 				`apiVersion: v1
@@ -979,8 +979,8 @@ stringData:
 						if err != nil {
 							return false
 						}
-						failedCond := meta.FindStatusCondition(createdBinding.GetConditions(), common.ConditionFailed)
-						return failedCond != nil && strings.Contains(failedCond.Message, errorMessage)
+						cond := meta.FindStatusCondition(createdBinding.GetConditions(), common.ConditionSucceeded)
+						return cond != nil && strings.Contains(cond.Message, errorMessage)
 					}, timeout, interval).Should(BeTrue())
 					Expect(len(createdBinding.Finalizers)).To(Equal(1))
 					getSecret(ctx, createdBinding.Spec.SecretName, createdBinding.Namespace, true)
@@ -1139,7 +1139,7 @@ stringData:
 						case smClientTypes.FAILED:
 							Expect(utils.IsFailed(createdBinding))
 						case smClientTypes.INPROGRESS:
-							Expect(utils.IsInProgress(createdBinding))
+							Expect(utils.ShouldRetryOperation(createdBinding))
 						case smClientTypes.SUCCEEDED:
 							Expect(isResourceReady(createdBinding))
 						}