Bug fix - create binding when instance update failed should be allowed

config/crd/bases/services.cloud.sap.com_servicebindings.yaml

@@ -273,10 +273,6 @@ spec:
                 description: Indicates when binding secret was rotated
                 format: date-time
                 type: string
-              observedGeneration:
-                description: Last generation that was acted on
-                format: int64
-                type: integer
               operationType:
                 description: The operation type (CREATE/UPDATE/DELETE) for ongoing
                   operation

config/crd/bases/services.cloud.sap.com_serviceinstances.yaml

@@ -255,10 +255,6 @@ spec:
                 description: The generated ID of the instance, will be automatically
                   filled once the instance is created
                 type: string
-              observedGeneration:
-                description: Last generation that was acted on
-                format: int64
-                type: integer
               operationType:
                 description: The operation type (CREATE/UPDATE/DELETE) for ongoing
                   operation

controllers/servicebinding_controller.go

@@ -123,67 +123,49 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return r.delete(ctx, serviceBinding, serviceInstance)
 	}
 
-	if len(serviceBinding.Status.OperationURL) > 0 {
-		// ongoing operation - poll status from SM
-		return r.poll(ctx, serviceBinding, serviceInstance)
-	}
-
-	if !controllerutil.ContainsFinalizer(serviceBinding, common.FinalizerName) {
-		controllerutil.AddFinalizer(serviceBinding, common.FinalizerName)
+	if controllerutil.AddFinalizer(serviceBinding, common.FinalizerName) {
 		log.Info(fmt.Sprintf("added finalizer '%s' to service binding", common.FinalizerName))
 		if err := r.Client.Update(ctx, serviceBinding); err != nil {
 			return ctrl.Result{}, err
 		}
 	}
-	condition := meta.FindStatusCondition(serviceBinding.Status.Conditions, common.ConditionReady)
 
-	if serviceBinding.Status.BindingID != "" && condition != nil && condition.ObservedGeneration != serviceBinding.Generation {
-		err := r.updateSecret(ctx, serviceBinding, serviceInstance, log)
-		if err != nil {
-			return r.handleSecretError(ctx, smClientTypes.UPDATE, err, serviceBinding)
-		}
-		err = utils.UpdateStatus(ctx, r.Client, serviceBinding)
-		if err != nil {
-			return r.handleSecretError(ctx, smClientTypes.UPDATE, err, serviceBinding)
-		}
+	if len(serviceBinding.Status.OperationURL) > 0 {
+		// ongoing operation - poll status from SM
+		return r.poll(ctx, serviceBinding, serviceInstance)
 	}
 
-	isBindingReady := condition != nil && condition.Status == metav1.ConditionTrue
-	if isBindingReady {
-		if isStaleServiceBinding(serviceBinding) {
-			return r.handleStaleServiceBinding(ctx, serviceBinding)
-		}
-
-		if initCredRotationIfRequired(serviceBinding) {
-			return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding)
+	if !serviceInstanceUsable(serviceInstance) {
+		instanceErr := fmt.Errorf("service instance '%s' is not usable", serviceBinding.Spec.ServiceInstanceName)
+		utils.SetBlockedCondition(ctx, instanceErr.Error(), serviceBinding)
+		if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil {
+			return ctrl.Result{}, err
 		}
+		return ctrl.Result{}, instanceErr
 	}
 
+	// should rotate creds
 	if meta.IsStatusConditionTrue(serviceBinding.Status.Conditions, common.ConditionCredRotationInProgress) {
+		log.Info("rotating credentials")
 		if err := r.rotateCredentials(ctx, serviceBinding, serviceInstance); err != nil {
 			return ctrl.Result{}, err
 		}
 	}
 
-	if isBindingReady {
-		log.Info("Binding in final state")
-		return r.maintain(ctx, serviceBinding)
-	}
-
-	if serviceNotUsable(serviceInstance) {
-		instanceErr := fmt.Errorf("service instance '%s' is not usable", serviceBinding.Spec.ServiceInstanceName)
-		utils.SetBlockedCondition(ctx, instanceErr.Error(), serviceBinding)
-		if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil {
-			return ctrl.Result{}, err
+	// is binding ready
+	if meta.IsStatusConditionTrue(serviceBinding.Status.Conditions, common.ConditionReady) {
+		if isStaleServiceBinding(serviceBinding) {
+			log.Info("binding is stale, handling")
+			return r.handleStaleServiceBinding(ctx, serviceBinding)
 		}
-		return ctrl.Result{}, instanceErr
-	}
 
-	if utils.IsInProgress(serviceInstance) {
-		log.Info(fmt.Sprintf("Service instance with k8s name %s is not ready for binding yet", serviceInstance.Name))
-		utils.SetInProgressConditions(ctx, smClientTypes.CREATE, fmt.Sprintf("creation in progress, waiting for service instance '%s' to be ready", serviceBinding.Spec.ServiceInstanceName), serviceBinding, false)
+		if initCredRotationIfRequired(serviceBinding) {
+			log.Info("cred rotation required, updating status")
+			return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding)
+		}
 
-		return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, utils.UpdateStatus(ctx, r.Client, serviceBinding)
+		log.Info("binding in final state, maintaining secret")
+		return r.maintain(ctx, serviceBinding, serviceInstance)
 	}
 
 	//set owner instance only for original bindings (not rotated)
@@ -199,6 +181,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 
 	if serviceBinding.Status.BindingID == "" {
 		if err := r.validateSecretNameIsAvailable(ctx, serviceBinding); err != nil {
+			log.Error(err, "secret validation failed")
 			utils.SetBlockedCondition(ctx, err.Error(), serviceBinding)
 			return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding)
 		}
@@ -224,29 +207,6 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	return ctrl.Result{}, nil
 }
 
-func (r *ServiceBindingReconciler) updateSecret(ctx context.Context, serviceBinding *v1.ServiceBinding, serviceInstance *v1.ServiceInstance, log logr.Logger) error {
-	log.Info("Updating secret according to the new template")
-	smClient, err := r.GetSMClient(ctx, serviceInstance)
-	if err != nil {
-		return err
-	}
-	smBinding, err := smClient.GetBindingByID(serviceBinding.Status.BindingID, nil)
-	if err != nil {
-		log.Error(err, "failed to get binding for update secret")
-		return err
-	}
-	if smBinding != nil {
-		if smBinding.Credentials != nil {
-			if err = r.storeBindingSecret(ctx, serviceBinding, smBinding); err != nil {
-				return err
-			}
-			log.Info("Updating binding", "bindingID", smBinding.ID)
-			utils.SetSuccessConditions(smClientTypes.UPDATE, serviceBinding, false)
-		}
-	}
-	return nil
-}
-
 func (r *ServiceBindingReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&v1.ServiceBinding{}).
@@ -491,43 +451,51 @@ func (r *ServiceBindingReconciler) getBindingForRecovery(ctx context.Context, sm
 	return nil, nil
 }
 
-func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *v1.ServiceBinding) (ctrl.Result, error) {
+func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *v1.ServiceBinding, instance *v1.ServiceInstance) (ctrl.Result, error) {
 	log := utils.GetLogger(ctx)
-	shouldUpdateStatus := false
-	if !utils.IsFailed(binding) {
-		secret, err := r.getSecret(ctx, binding.Namespace, binding.Spec.SecretName)
-		if err != nil {
-			// secret was deleted
-			if apierrors.IsNotFound(err) && !utils.IsMarkedForDeletion(binding.ObjectMeta) {
-				log.Info(fmt.Sprintf("secret not found recovering binding %s", binding.Name))
-				binding.Status.BindingID = ""
-				binding.Status.Ready = metav1.ConditionFalse
-				utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "recreating deleted secret", binding, false)
-				shouldUpdateStatus = true
-				r.Recorder.Event(binding, corev1.EventTypeWarning, "SecretDeleted", "SecretDeleted")
-			} else {
-				return ctrl.Result{}, err
-			}
-		} else { // secret exists, validate it has the required labels
-			if secret.Labels == nil {
-				secret.Labels = map[string]string{}
-			}
-			if secret.Labels[common.ManagedByBTPOperatorLabel] != "true" {
-				secret.Labels[common.ManagedByBTPOperatorLabel] = "true"
-				if err = r.Client.Update(ctx, secret); err != nil {
-					log.Error(err, "failed to update secret labels")
-					return ctrl.Result{}, err
-				}
-			}
+	if err := r.maintainSecret(ctx, binding, instance); err != nil {
+		log.Error(err, "failed to maintain secret")
+		return r.handleSecretError(ctx, smClientTypes.UPDATE, err, binding)
+	}
+
+	log.Info("maintain finished successfully")
+	return ctrl.Result{}, nil
+}
+
+func (r *ServiceBindingReconciler) maintainSecret(ctx context.Context, serviceBinding *v1.ServiceBinding, serviceInstance *v1.ServiceInstance) error {
+	log := utils.GetLogger(ctx)
+	if common.GetObservedGeneration(serviceBinding) == serviceBinding.Generation {
+		log.Info("observed generation is up to date, checking if secret exists")
+		if _, err := r.getSecret(ctx, serviceBinding.Namespace, serviceBinding.Spec.SecretName); err == nil {
+			log.Info("secret exists, no need to maintain secret")
+			return nil
 		}
+
+		log.Info("binding's secret was not found")
+		r.Recorder.Event(serviceBinding, corev1.EventTypeWarning, "SecretDeleted", "SecretDeleted")
 	}
 
-	if shouldUpdateStatus {
-		log.Info(fmt.Sprintf("maintanance required for binding %s", binding.Name))
-		return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, binding)
+	log.Info("maintaining binding's secret")
+	smClient, err := r.GetSMClient(ctx, serviceInstance)
+	if err != nil {
+		return err
+	}
+	smBinding, err := smClient.GetBindingByID(serviceBinding.Status.BindingID, nil)
+	if err != nil {
+		log.Error(err, "failed to get binding for update secret")
+		return err
+	}
+	if smBinding != nil {
+		if smBinding.Credentials != nil {
+			if err = r.storeBindingSecret(ctx, serviceBinding, smBinding); err != nil {
+				return err
+			}
+			log.Info("Updating binding", "bindingID", smBinding.ID)
+			utils.SetSuccessConditions(smClientTypes.UPDATE, serviceBinding, false)
+		}
 	}
 
-	return ctrl.Result{}, nil
+	return utils.UpdateStatus(ctx, r.Client, serviceBinding)
 }
 
 func (r *ServiceBindingReconciler) getServiceInstanceForBinding(ctx context.Context, binding *v1.ServiceBinding) (*v1.ServiceInstance, error) {
@@ -917,7 +885,6 @@ func (r *ServiceBindingReconciler) addInstanceInfo(ctx context.Context, binding
 }
 
 func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, binding *v1.ServiceBinding, serviceInstance *v1.ServiceInstance) error {
-	suffix := "-" + utils.RandStringRunes(6)
 	log := utils.GetLogger(ctx)
 	if binding.Annotations != nil {
 		if _, ok := binding.Annotations[common.ForceRotateAnnotation]; ok {
@@ -963,6 +930,7 @@ func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, bindin
 		}
 
 		// rename current binding
+		suffix := "-" + utils.RandStringRunes(6)
 		log.Info("Credentials rotation - renaming binding to old in SM", "current", binding.Spec.ExternalName)
 		if _, errRenaming := smClient.RenameBinding(binding.Status.BindingID, binding.Spec.ExternalName+suffix, binding.Name+suffix); errRenaming != nil {
 			log.Error(errRenaming, "Credentials rotation - failed renaming binding to old in SM", "binding", binding.Spec.ExternalName)
@@ -1089,7 +1057,7 @@ func isStaleServiceBinding(binding *v1.ServiceBinding) bool {
 }
 
 func initCredRotationIfRequired(binding *v1.ServiceBinding) bool {
-	if utils.IsFailed(binding) || !credRotationEnabled(binding) || meta.IsStatusConditionTrue(binding.Status.Conditions, common.ConditionCredRotationInProgress) {
+	if utils.IsFailed(binding) || !credRotationEnabled(binding) {
 		return false
 	}
 	_, forceRotate := binding.Annotations[common.ForceRotateAnnotation]
@@ -1154,14 +1122,11 @@ func bindingAlreadyOwnedByInstance(instance *v1.ServiceInstance, binding *v1.Ser
 	return false
 }
 
-func serviceNotUsable(instance *v1.ServiceInstance) bool {
+func serviceInstanceUsable(instance *v1.ServiceInstance) bool {
 	if utils.IsMarkedForDeletion(instance.ObjectMeta) {
-		return true
-	}
-	if len(instance.Status.Conditions) != 0 {
-		return instance.Status.Conditions[0].Reason == utils.GetConditionReason(smClientTypes.CREATE, smClientTypes.FAILED)
+		return false
 	}
-	return false
+	return instance.Status.Ready == metav1.ConditionTrue
 }
 
 func getInstanceNameForSecretCredentials(instance *v1.ServiceInstance) []byte {

controllers/servicebinding_controller_test.go

@@ -4,13 +4,13 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
-	"github.com/lithammer/dedent"
-	authv1 "k8s.io/api/authentication/v1"
 	"net/http"
 	"strings"
 
 	"github.com/SAP/sap-btp-service-operator/api/common"
 	"github.com/SAP/sap-btp-service-operator/internal/utils"
+	"github.com/lithammer/dedent"
+	authv1 "k8s.io/api/authentication/v1"
 	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 
@@ -324,20 +324,17 @@ var _ = Describe("ServiceBinding controller", func() {
 					secretLookupKey := types.NamespacedName{Name: createdBinding.Spec.SecretName, Namespace: createdBinding.Namespace}
 					bindingSecret := getSecret(ctx, secretLookupKey.Name, secretLookupKey.Namespace, true)
 					originalSecretUID := bindingSecret.UID
-					fakeClient.ListBindingsReturns(&smClientTypes.ServiceBindings{
-						ServiceBindings: []smClientTypes.ServiceBinding{
-							{
-								ID:          createdBinding.Status.BindingID,
-								Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}"),
-								LastOperation: &smClientTypes.Operation{
-									Type:        smClientTypes.CREATE,
-									State:       smClientTypes.SUCCEEDED,
-									Description: "fake-description",
-								},
-							},
+					Expect(k8sClient.Delete(ctx, bindingSecret)).To(Succeed())
+
+					fakeClient.GetBindingByIDReturns(&smClientTypes.ServiceBinding{
+						ID:          createdBinding.Status.BindingID,
+						Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}"),
+						LastOperation: &smClientTypes.Operation{
+							Type:        smClientTypes.CREATE,
+							State:       smClientTypes.SUCCEEDED,
+							Description: "fake-description",
 						},
 					}, nil)
-					Expect(k8sClient.Delete(ctx, bindingSecret)).To(Succeed())
 
 					//tickle the binding
 					createdBinding.Annotations = map[string]string{"tickle": "true"}
@@ -541,21 +538,22 @@ var _ = Describe("ServiceBinding controller", func() {
 
 		When("referenced service instance is failed", func() {
 			It("should retry and succeed once the instance is ready", func() {
-				utils.SetFailureConditions(smClientTypes.CREATE, "Failed to create instance (test)", createdInstance, false)
+				createdInstance.Status.Ready = metav1.ConditionFalse
 				updateInstanceStatus(ctx, createdInstance)
 
 				binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false)
 				Expect(err).ToNot(HaveOccurred())
 				waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, "", "is not usable")
 
-				utils.SetSuccessConditions(smClientTypes.CREATE, createdInstance, false)
+				createdInstance.Status.Ready = metav1.ConditionTrue
 				updateInstanceStatus(ctx, createdInstance)
 				waitForResourceToBeReady(ctx, binding)
 			})
 		})
 
 		When("referenced service instance is not ready", func() {
 			It("should retry and succeed once the instance is ready", func() {
+				createdInstance.Status.Ready = metav1.ConditionFalse
 				fakeClient.StatusReturns(&smClientTypes.Operation{ResourceID: fakeInstanceID, State: smClientTypes.INPROGRESS}, nil)
 				utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "", createdInstance, false)
 				createdInstance.Status.OperationURL = "/1234"
@@ -564,8 +562,9 @@ var _ = Describe("ServiceBinding controller", func() {
 
 				createdBinding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false)
 				Expect(err).ToNot(HaveOccurred())
-				Expect(utils.IsInProgress(createdBinding)).To(BeTrue())
+				waitForResourceCondition(ctx, createdBinding, common.ConditionSucceeded, metav1.ConditionFalse, common.Blocked, "")
 
+				createdInstance.Status.Ready = metav1.ConditionTrue
 				utils.SetSuccessConditions(smClientTypes.CREATE, createdInstance, false)
 				createdInstance.Status.OperationType = ""
 				createdInstance.Status.OperationURL = ""
@@ -574,6 +573,19 @@ var _ = Describe("ServiceBinding controller", func() {
 			})
 		})
 
+		When("referenced service instance is being deleted", func() {
+			It("should fail", func() {
+				createdInstance.Finalizers = append(createdInstance.Finalizers, "fake/finalizer")
+				updateInstance(ctx, createdInstance)
+				Expect(k8sClient.Delete(ctx, createdInstance)).To(Succeed())
+
+				createdBinding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false)
+				Expect(err).ToNot(HaveOccurred())
+				waitForResourceCondition(ctx, createdBinding, common.ConditionSucceeded, metav1.ConditionFalse, common.Blocked, "")
+				Expect(utils.RemoveFinalizer(ctx, k8sClient, createdInstance, "fake/finalizer")).To(Succeed())
+			})
+		})
+
 		When("secretTemplate", func() {
 			It("should succeed to create the secret", func() {
 				ctx := context.Background()
@@ -813,7 +825,7 @@ stringData:
 			})
 		})
 
-		When("secretTemplate  is changed", func() {
+		When("secretTemplate is changed", func() {
 			It("should succeed to create the secret", func() {
 				ctx := context.Background()
 				secretTemplate := dedent.Dedent(

controllers/serviceinstance_controller.go

@@ -104,8 +104,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		return r.poll(ctx, serviceInstance)
 	}
 
-	if !controllerutil.ContainsFinalizer(serviceInstance, common.FinalizerName) {
-		controllerutil.AddFinalizer(serviceInstance, common.FinalizerName)
+	if controllerutil.AddFinalizer(serviceInstance, common.FinalizerName) {
 		log.Info(fmt.Sprintf("added finalizer '%s' to service instance", common.FinalizerName))
 		if err := r.Client.Update(ctx, serviceInstance); err != nil {
 			return ctrl.Result{}, err