resources sync - validate resource exists (#593)

Author: kerenlahav

api/common/common.go

@@ -86,10 +86,15 @@ type SAPBTPResource interface {
 }
 
 func GetObservedGeneration(obj SAPBTPResource) int64 {
-	cond := meta.FindStatusCondition(obj.GetConditions(), ConditionSucceeded)
 	observedGen := int64(0)
-	if cond != nil {
-		observedGen = cond.ObservedGeneration
+	if len(obj.GetConditions()) == 0 {
+		return observedGen
+	}
+
+	cond := meta.FindStatusCondition(obj.GetConditions(), ConditionSucceeded)
+	if cond == nil {
+		cond = &obj.GetConditions()[0]
 	}
+	observedGen = cond.ObservedGeneration
 	return observedGen
 }

api/common/consts.go

@@ -31,6 +31,7 @@ const (
 	ShareNotSupported = "ShareNotSupported"
 	UnShareFailed     = "UnShareFailed"
 	UnShareSucceeded  = "UnShareSucceeded"
+	ResourceNotFound  = "NotFound"
 
 	Blocked = "Blocked"
 	Unknown = "Unknown"
@@ -42,4 +43,7 @@ const (
 	// Constance for seceret template
 	InstanceKey    = "instance"
 	CredentialsKey = "credentials"
+
+	//messages
+	ResourceNotFoundMessageFormat = "%s %s not found for this cluster or namespace; or it is not managed by this operator-access instance."
 )

client/sm/client.go

@@ -592,8 +592,12 @@ func handleResponseError(response *http.Response) error {
 		body = []byte(fmt.Sprintf("error reading response body: %s", err))
 	}
 
+	statusCode := response.StatusCode
+	if response.Header.Get("X-Cf-RouterError") == "unknown_route" {
+		statusCode = http.StatusServiceUnavailable
+	}
 	smError := &ServiceManagerError{
-		StatusCode:      response.StatusCode,
+		StatusCode:      statusCode,
 		ResponseHeaders: response.Header,
 	}
 	_ = json.Unmarshal(body, &smError)

controllers/servicebinding_controller.go

@@ -19,6 +19,7 @@ package controllers
 import (
 	"context"
 	"encoding/json"
+	"net/http"
 	"strings"
 	"time"
 
@@ -125,6 +126,30 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return r.delete(ctx, serviceBinding, serviceInstance)
 	}
 
+	smClient, err := r.GetSMClient(ctx, serviceInstance)
+	if err != nil {
+		return utils.HandleOperationFailure(ctx, r.Client, serviceBinding, common.Unknown, err)
+	}
+	if len(serviceBinding.Status.BindingID) > 0 {
+		if bindingExist, err := isBindingExistInSM(smClient, serviceInstance, serviceBinding.Status.BindingID, log); err != nil {
+			log.Error(err, "failed to check if binding exist in sm due to unknown error")
+			return ctrl.Result{}, err
+		} else if !bindingExist {
+			log.Info("binding not found in SM, updating status")
+			condition := metav1.Condition{
+				Type:               common.ConditionReady,
+				Status:             metav1.ConditionFalse,
+				ObservedGeneration: serviceBinding.Generation,
+				LastTransitionTime: metav1.NewTime(time.Now()),
+				Reason:             common.ResourceNotFound,
+				Message:            fmt.Sprintf(common.ResourceNotFoundMessageFormat, "binding", serviceBinding.Status.BindingID),
+			}
+			serviceBinding.Status.Conditions = []metav1.Condition{condition}
+			serviceBinding.Status.Ready = metav1.ConditionFalse
+			return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding)
+		}
+	}
+
 	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 {
@@ -184,11 +209,6 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 			return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding)
 		}
 
-		smClient, err := r.GetSMClient(ctx, serviceInstance)
-		if err != nil {
-			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")
@@ -1127,3 +1147,26 @@ func truncateString(str string, length int) string {
 	}
 	return str
 }
+
+func isBindingExistInSM(smClient sm.Client, instance *v1.ServiceInstance, bindingID string, log logr.Logger) (bool, error) {
+	log.Info("checking if k8s instance status is NotFound")
+	instanceReadyCond := meta.FindStatusCondition(instance.GetConditions(), common.ConditionReady)
+	if instanceReadyCond != nil && instanceReadyCond.Reason == common.ResourceNotFound {
+		log.Info("k8s instance is in NotFound state -> invalid binding")
+		return false, nil
+	}
+
+	log.Info(fmt.Sprintf("trying to get from SM binding with id %s", bindingID))
+	if _, err := smClient.GetBindingByID(bindingID, nil); err != nil {
+		var smError *sm.ServiceManagerError
+		if ok := errors.As(err, &smError); ok {
+			log.Error(smError, fmt.Sprintf("SM returned status code %d", smError.StatusCode))
+			if smError.StatusCode == http.StatusNotFound {
+				return false, nil
+			}
+		}
+		return false, err
+	}
+	log.Info("binding found in SM")
+	return true, nil
+}

controllers/servicebinding_controller_test.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"net/http"
 	"strings"
+	"time"
 
 	"github.com/SAP/sap-btp-service-operator/internal/utils/logutils"
 	"github.com/lithammer/dedent"
@@ -1547,6 +1548,30 @@ stringData:
 			})
 		})
 	})
+
+	When("binding id exist on resource but instance is in state NotFound", func() {
+		BeforeEach(func() {
+			createdBinding = createAndValidateBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", fakeBindingID)
+			createdInstance.Status.Conditions = []metav1.Condition{{
+				Type:               common.ConditionReady,
+				Status:             metav1.ConditionFalse,
+				ObservedGeneration: createdInstance.Generation,
+				LastTransitionTime: metav1.NewTime(time.Now()),
+				Reason:             common.ResourceNotFound,
+			}}
+			Expect(k8sClient.Status().Update(ctx, createdInstance)).To(Succeed())
+		})
+		When("reconcile", func() {
+			It("should update status to ready=false", func() {
+				createdBinding.Labels = map[string]string{"tickle": "1"}
+				updateBinding(ctx, getResourceNamespacedName(createdBinding), createdBinding)
+				waitForResourceCondition(ctx, createdBinding, common.ConditionReady, metav1.ConditionFalse, common.ResourceNotFound, fmt.Sprintf(common.ResourceNotFoundMessageFormat, "binding", createdBinding.Status.BindingID))
+
+				By("deleting the binding should succeed")
+				deleteAndWait(ctx, createdBinding)
+			})
+		})
+	})
 })
 
 func generateBasicBindingTemplate(name, namespace, instanceName, instanceNamespace, externalName string, secretTemplate string) *v1.ServiceBinding {

controllers/serviceinstance_controller.go

@@ -20,7 +20,9 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"net/http"
 	"strings"
+	"time"
 
 	"github.com/SAP/sap-btp-service-operator/internal/utils/logutils"
 	"github.com/pkg/errors"
@@ -88,12 +90,6 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	if utils.IsMarkedForDeletion(serviceInstance.ObjectMeta) {
 		return r.deleteInstance(ctx, serviceInstance)
 	}
-	if len(serviceInstance.GetConditions()) == 0 {
-		err := utils.InitConditions(ctx, r.Client, serviceInstance)
-		if err != nil {
-			return ctrl.Result{}, err
-		}
-	}
 
 	// If stored hash is MD5 (32 chars) and we're now using SHA256 (64 chars),
 	// perform one-time migration by updating the stored hash without triggering update
@@ -105,6 +101,42 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceInstance)
 	}
 
+	smClient, err := r.GetSMClient(ctx, serviceInstance)
+	if err != nil {
+		log.Error(err, "failed to get sm client")
+		return utils.HandleOperationFailure(ctx, r.Client, serviceInstance, common.Unknown, err)
+	}
+	if len(serviceInstance.Status.InstanceID) > 0 {
+		if _, err := smClient.GetInstanceByID(serviceInstance.Status.InstanceID, nil); err != nil {
+			var smError *sm.ServiceManagerError
+			if ok := errors.As(err, &smError); ok {
+				if smError.StatusCode == http.StatusNotFound {
+					log.Info(fmt.Sprintf("instance %s not found in SM", serviceInstance.Status.InstanceID))
+					condition := metav1.Condition{
+						Type:               common.ConditionReady,
+						Status:             metav1.ConditionFalse,
+						ObservedGeneration: serviceInstance.Generation,
+						LastTransitionTime: metav1.NewTime(time.Now()),
+						Reason:             common.ResourceNotFound,
+						Message:            fmt.Sprintf(common.ResourceNotFoundMessageFormat, "instance", serviceInstance.Status.InstanceID),
+					}
+					serviceInstance.Status.Conditions = []metav1.Condition{condition}
+					serviceInstance.Status.Ready = metav1.ConditionFalse
+					return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceInstance)
+				}
+			}
+			log.Error(err, fmt.Sprintf("failed to get instance %s from SM", serviceInstance.Status.InstanceID))
+			return ctrl.Result{}, err
+		}
+	}
+
+	if len(serviceInstance.GetConditions()) == 0 {
+		err := utils.InitConditions(ctx, r.Client, serviceInstance)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+	}
+
 	if len(serviceInstance.Status.OperationURL) > 0 {
 		// ongoing operation - poll status from SM
 		return r.poll(ctx, serviceInstance)
@@ -126,12 +158,6 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		}
 	}
 
-	smClient, err := r.GetSMClient(ctx, serviceInstance)
-	if err != nil {
-		log.Error(err, "failed to get sm client")
-		return utils.HandleOperationFailure(ctx, r.Client, serviceInstance, common.Unknown, err)
-	}
-
 	if serviceInstance.Status.InstanceID == "" {
 		log.Info("Instance ID is empty, checking if instance exist in SM")
 		smInstance, err := r.getInstanceForRecovery(ctx, smClient, serviceInstance)

controllers/serviceinstance_controller_test.go

@@ -1549,6 +1549,26 @@ var _ = Describe("ServiceInstance controller", func() {
 		})
 
 	})
+
+	When("instance id exist on resource but not found in sm", func() {
+		BeforeEach(func() {
+			serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, true)
+			Expect(len(serviceInstance.Status.InstanceID) > 0).To(Equal(true))
+			fakeClient.GetInstanceByIDReturns(nil, &sm.ServiceManagerError{
+				StatusCode: http.StatusNotFound,
+			})
+		})
+		When("updating instance", func() {
+			It("should update status to ready=false", func() {
+				serviceInstance.Spec.ExternalName = "new-name"
+				Expect(k8sClient.Update(ctx, serviceInstance)).To(Succeed())
+				waitForResourceCondition(ctx, serviceInstance, common.ConditionReady, metav1.ConditionFalse, common.ResourceNotFound, fmt.Sprintf(common.ResourceNotFoundMessageFormat, "instance", serviceInstance.Status.InstanceID))
+
+				By("deleting the instance should succeed")
+				deleteInstance(ctx, serviceInstance, true)
+			})
+		})
+	})
 })
 
 func waitForInstanceConditionAndMessage(ctx context.Context, key types.NamespacedName, conditionType, msg string) {