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,52 @@ 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 utils.IsMarkedForDeletion(serviceInstance.ObjectMeta) {
+		log.Info(fmt.Sprintf("service instance name: %s namespace: %s is marked for deletion, unable to create binding", serviceInstance.Name, serviceInstance.Namespace))
+		utils.SetBlockedCondition(ctx, "instance is in deletion process", serviceBinding)
+		return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding)
+	}
 
-		if initCredRotationIfRequired(serviceBinding) {
-			return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding)
-		}
+	if !serviceInstanceReady(serviceInstance) {
+		log.Info(fmt.Sprintf("service instance name: %s namespace: %s is not ready, unable to create binding", serviceInstance.Name, serviceInstance.Namespace))
+		utils.SetBlockedCondition(ctx, "service instance is not ready", serviceBinding)
+		return ctrl.Result{Requeue: true}, utils.UpdateStatus(ctx, r.Client, serviceBinding)
 	}
 
+	// 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 +184,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 +210,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{}).
@@ -458,6 +421,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *v1.
 		}
 	}
 
+	log.Info(fmt.Sprintf("finished polling operation %s '%s'", serviceBinding.Status.OperationType, serviceBinding.Status.OperationURL))
 	serviceBinding.Status.OperationURL = ""
 	serviceBinding.Status.OperationType = ""
 
@@ -491,43 +455,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,17 +889,10 @@ 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 {
-			log.Info("Credentials rotation - deleting force rotate annotation")
-			delete(binding.Annotations, common.ForceRotateAnnotation)
-			if err := r.Client.Update(ctx, binding); err != nil {
-				log.Info("Credentials rotation - failed to delete force rotate annotation")
-				return err
-			}
-		}
+	if err := r.removeForceRotateAnnotationIfNeeded(ctx, binding, log); err != nil {
+		log.Info("Credentials rotation - failed to delete force rotate annotation")
+		return err
 	}
 
 	credInProgressCondition := meta.FindStatusCondition(binding.GetConditions(), common.ConditionCredRotationInProgress)
@@ -957,12 +922,14 @@ func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, bindin
 	}
 
 	if len(bindings.Items) == 0 {
+		// create the backup binding
 		smClient, err := r.GetSMClient(ctx, serviceInstance)
 		if err != nil {
 			return err
 		}
 
 		// 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)
@@ -992,6 +959,17 @@ func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, bindin
 	return utils.UpdateStatus(ctx, r.Client, binding)
 }
 
+func (r *ServiceBindingReconciler) removeForceRotateAnnotationIfNeeded(ctx context.Context, binding *v1.ServiceBinding, log logr.Logger) error {
+	if binding.Annotations != nil {
+		if _, ok := binding.Annotations[common.ForceRotateAnnotation]; ok {
+			log.Info("Credentials rotation - deleting force rotate annotation")
+			delete(binding.Annotations, common.ForceRotateAnnotation)
+			return r.Client.Update(ctx, binding)
+		}
+	}
+	return nil
+}
+
 func (r *ServiceBindingReconciler) stopRotation(ctx context.Context, binding *v1.ServiceBinding) error {
 	conditions := binding.GetConditions()
 	meta.RemoveStatusCondition(&conditions, common.ConditionCredRotationInProgress)
@@ -1089,7 +1067,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 +1132,8 @@ func bindingAlreadyOwnedByInstance(instance *v1.ServiceInstance, binding *v1.Ser
 	return false
 }
 
-func serviceNotUsable(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
+func serviceInstanceReady(instance *v1.ServiceInstance) bool {
+	return instance.Status.Ready == metav1.ConditionTrue
 }
 
 func getInstanceNameForSecretCredentials(instance *v1.ServiceInstance) []byte {