Ensure broker deprovisions the instance on delete

* The OSBAPI client returns two flavours of errors - recoverable and
  unrecoverable
* When deleting the `CFServiceInstance` its finalization routine would
  keep trying to deprovision on recoverable errors, or set the instance
  deprovision status to `failed` upon an unrecoverable error
* Smoke and e2e tests are pushing the sample broker application to a
  dedicated org to ensure the broker is usable while deleting the org
  other sample apps are pushed to.

fixes #3586

Co-authored-by: Danail Branekov <[email protected]>

Author: uzabanov

controllers/api/v1alpha1/cfserviceinstance_types.go

@@ -31,7 +31,8 @@ const (
 
 	CFServiceInstanceFinalizerName = "cfServiceInstance.korifi.cloudfoundry.org"
 
-	ProvisioningFailedCondition = "ProvisioningFailed"
+	ProvisioningFailedCondition   = "ProvisioningFailed"
+	DeprovisioningFailedCondition = "DeprovisioningFailed"
 )
 
 // CFServiceInstanceSpec defines the desired state of CFServiceInstance

controllers/controllers/services/instances/managed/controller.go

@@ -126,8 +126,20 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor
 	serviceInstance.Status.ObservedGeneration = serviceInstance.Generation
 	log.V(1).Info("set observed generation", "generation", serviceInstance.Status.ObservedGeneration)
 
+	serviceInstanceAssets, err := r.assets.GetServiceInstanceAssets(ctx, serviceInstance)
+	if err != nil {
+		log.Error(err, "failed to get service instance assets")
+		return ctrl.Result{}, err
+	}
+
+	osbapiClient, err := r.osbapiClientFactory.CreateClient(ctx, serviceInstanceAssets.ServiceBroker)
+	if err != nil {
+		log.Error(err, "failed to create broker client", "broker", serviceInstanceAssets.ServiceBroker.Name)
+		return ctrl.Result{}, fmt.Errorf("failed to create client for broker %q: %w", serviceInstanceAssets.ServiceBroker.Name, err)
+	}
+
 	if !serviceInstance.GetDeletionTimestamp().IsZero() {
-		return r.finalizeCFServiceInstance(ctx, serviceInstance)
+		return r.finalizeCFServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient)
 	}
 
 	if isReady(serviceInstance) {
@@ -138,12 +150,6 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor
 		return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisioningFailed").WithNoRequeue()
 	}
 
-	serviceInstanceAssets, err := r.assets.GetServiceInstanceAssets(ctx, serviceInstance)
-	if err != nil {
-		log.Error(err, "failed to get service instance assets")
-		return ctrl.Result{}, err
-	}
-
 	planVisible, err := r.isServicePlanVisible(ctx, serviceInstance, serviceInstanceAssets.ServicePlan)
 	if err != nil {
 		log.Error(err, "failed to check service plan visibility")
@@ -159,52 +165,24 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor
 		serviceInstance.Spec.ServiceLabel = tools.PtrTo(serviceInstanceAssets.ServiceOffering.Spec.Name)
 	}
 
-	osbapiClient, err := r.osbapiClientFactory.CreateClient(ctx, serviceInstanceAssets.ServiceBroker)
-	if err != nil {
-		log.Error(err, "failed to create broker client", "broker", serviceInstanceAssets.ServiceBroker.Name)
-		return ctrl.Result{}, fmt.Errorf("failed to create client for broker %q: %w", serviceInstanceAssets.ServiceBroker.Name, err)
-	}
-
 	provisionResponse, err := r.provisionServiceInstance(ctx, serviceInstance, serviceInstanceAssets, osbapiClient)
 	if err != nil {
 		log.Error(err, "failed to provision service instance")
 		return ctrl.Result{}, fmt.Errorf("failed to provision service instance: %w", err)
 	}
 
 	if provisionResponse.IsAsync {
-		return r.pollProvisionOperation(ctx, serviceInstance, serviceInstanceAssets, osbapiClient, provisionResponse.Operation)
+		lastOpResponse, err := r.pollLastOperation(ctx, serviceInstance, serviceInstanceAssets, osbapiClient, provisionResponse.Operation)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+		return r.processProvisionOperation(serviceInstance, lastOpResponse)
 	}
 
 	serviceInstance.Status.LastOperation.State = "succeeded"
 	return ctrl.Result{}, nil
 }
 
-func (r *Reconciler) isServicePlanVisible(
-	ctx context.Context,
-	serviceInstance *korifiv1alpha1.CFServiceInstance,
-	servicePlan *korifiv1alpha1.CFServicePlan,
-) (bool, error) {
-	if servicePlan.Spec.Visibility.Type == korifiv1alpha1.AdminServicePlanVisibilityType {
-		return false, nil
-	}
-
-	if servicePlan.Spec.Visibility.Type == korifiv1alpha1.PublicServicePlanVisibilityType {
-		return true, nil
-	}
-
-	namespace := &corev1.Namespace{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: serviceInstance.Namespace,
-		},
-	}
-	err := r.k8sClient.Get(ctx, client.ObjectKeyFromObject(namespace), namespace)
-	if err != nil {
-		return false, err
-	}
-
-	return slices.Contains(servicePlan.Spec.Visibility.Organizations, namespace.Labels[korifiv1alpha1.OrgGUIDKey]), nil
-}
-
 func (r *Reconciler) provisionServiceInstance(
 	ctx context.Context,
 	serviceInstance *korifiv1alpha1.CFServiceInstance,
@@ -265,31 +243,10 @@ func (r *Reconciler) provisionServiceInstance(
 	return provisionResponse, nil
 }
 
-func (r *Reconciler) pollProvisionOperation(
-	ctx context.Context,
+func (r *Reconciler) processProvisionOperation(
 	serviceInstance *korifiv1alpha1.CFServiceInstance,
-	assets osbapi.ServiceInstanceAssets,
-	osbapiClient osbapi.BrokerClient,
-	operationID string,
+	lastOpResponse osbapi.LastOperationResponse,
 ) (ctrl.Result, error) {
-	log := logr.FromContextOrDiscard(ctx).WithName("poll-provision-operation")
-
-	lastOpResponse, err := osbapiClient.GetServiceInstanceLastOperation(ctx, osbapi.GetServiceInstanceLastOperationRequest{
-		InstanceID: serviceInstance.Name,
-		GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{
-			ServiceId: assets.ServiceOffering.Spec.BrokerCatalog.ID,
-			PlanID:    assets.ServicePlan.Spec.BrokerCatalog.ID,
-			Operation: operationID,
-		},
-	})
-	if err != nil {
-		log.Error(err, "getting service instance last operation failed")
-		return ctrl.Result{}, k8s.NewNotReadyError().WithCause(err).WithReason("GetLastOperationFailed")
-	}
-
-	serviceInstance.Status.LastOperation.State = lastOpResponse.State
-	serviceInstance.Status.LastOperation.Description = lastOpResponse.Description
-
 	if lastOpResponse.State == "in progress" {
 		return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisionInProgress").WithRequeue()
 	}
@@ -309,33 +266,31 @@ func (r *Reconciler) pollProvisionOperation(
 	return ctrl.Result{}, nil
 }
 
-func getServiceInstanceParameters(serviceInstance *korifiv1alpha1.CFServiceInstance) (map[string]any, error) {
-	if serviceInstance.Spec.Parameters == nil {
-		return nil, nil
-	}
-
-	parametersMap := map[string]any{}
-	err := json.Unmarshal(serviceInstance.Spec.Parameters.Raw, &parametersMap)
-	if err != nil {
-		return nil, fmt.Errorf("failed to unmarshal parameters: %w", err)
-	}
-
-	return parametersMap, nil
-}
-
 func (r *Reconciler) finalizeCFServiceInstance(
 	ctx context.Context,
 	serviceInstance *korifiv1alpha1.CFServiceInstance,
+	assets osbapi.ServiceInstanceAssets,
+	osbapiClient osbapi.BrokerClient,
 ) (ctrl.Result, error) {
 	log := logr.FromContextOrDiscard(ctx).WithName("finalizeCFServiceInstance")
 
 	if !controllerutil.ContainsFinalizer(serviceInstance, korifiv1alpha1.CFServiceInstanceFinalizerName) {
 		return ctrl.Result{}, nil
 	}
 
-	_, err := r.deprovisionServiceInstance(ctx, serviceInstance)
+	deprovisionResponse, err := r.deprovisionServiceInstance(ctx, serviceInstance, assets, osbapiClient)
 	if err != nil {
 		log.Error(err, "failed to deprovision service instance with broker")
+		return ctrl.Result{}, err
+	}
+
+	if deprovisionResponse.IsAsync {
+
+		lastOpResponse, err := r.pollLastOperation(ctx, serviceInstance, assets, osbapiClient, deprovisionResponse.Operation)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+		return r.processDeprovisionOperation(serviceInstance, lastOpResponse)
 	}
 
 	controllerutil.RemoveFinalizer(serviceInstance, korifiv1alpha1.CFServiceInstanceFinalizerName)
@@ -347,32 +302,130 @@ func (r *Reconciler) finalizeCFServiceInstance(
 func (r *Reconciler) deprovisionServiceInstance(
 	ctx context.Context,
 	serviceInstance *korifiv1alpha1.CFServiceInstance,
+	assets osbapi.ServiceInstanceAssets,
+	osbapiClient osbapi.BrokerClient,
 ) (osbapi.ServiceInstanceOperationResponse, error) {
-	assets, err := r.assets.GetServiceInstanceAssets(ctx, serviceInstance)
-	if err != nil {
-		return osbapi.ServiceInstanceOperationResponse{}, fmt.Errorf("failed to get service instance assets: %w", err)
-	}
-
-	osbapiClient, err := r.osbapiClientFactory.CreateClient(ctx, assets.ServiceBroker)
-	if err != nil {
-		return osbapi.ServiceInstanceOperationResponse{}, fmt.Errorf("failed to create broker client: %w", err)
+	serviceInstance.Status.LastOperation = services.LastOperation{
+		Type:  "delete",
+		State: "initial",
 	}
-
-	var deprovisionResponse osbapi.ServiceInstanceOperationResponse
-	deprovisionResponse, err = osbapiClient.Deprovision(ctx, osbapi.InstanceDeprovisionPayload{
+	deprovisionResponse, err := osbapiClient.Deprovision(ctx, osbapi.InstanceDeprovisionPayload{
 		ID: serviceInstance.Name,
 		InstanceDeprovisionRequest: osbapi.InstanceDeprovisionRequest{
 			ServiceId: assets.ServiceOffering.Spec.BrokerCatalog.ID,
 			PlanID:    assets.ServicePlan.Spec.BrokerCatalog.ID,
 		},
 	})
 	if err != nil {
+		if osbapi.IsUnrecoveralbeError(err) {
+			serviceInstance.Status.LastOperation.State = "failed"
+			meta.SetStatusCondition(&serviceInstance.Status.Conditions, metav1.Condition{
+				Type:               korifiv1alpha1.DeprovisioningFailedCondition,
+				Status:             metav1.ConditionTrue,
+				ObservedGeneration: serviceInstance.Generation,
+				LastTransitionTime: metav1.NewTime(time.Now()),
+				Reason:             "DeprovisionFailed",
+				Message:            err.Error(),
+			})
+			return osbapi.ServiceInstanceOperationResponse{},
+				k8s.NewNotReadyError().WithReason("DeprovisionFailed")
+		}
+
 		return osbapi.ServiceInstanceOperationResponse{}, fmt.Errorf("failed to deprovision service instance: %w", err)
 	}
 
 	return deprovisionResponse, nil
 }
 
+func (r *Reconciler) processDeprovisionOperation(
+	serviceInstance *korifiv1alpha1.CFServiceInstance,
+	lastOpResponse osbapi.LastOperationResponse,
+) (ctrl.Result, error) {
+	if lastOpResponse.State == "in progress" {
+		return ctrl.Result{}, k8s.NewNotReadyError().WithReason("DeprovisionInProgress").WithRequeue()
+	}
+
+	if lastOpResponse.State == "failed" {
+		meta.SetStatusCondition(&serviceInstance.Status.Conditions, metav1.Condition{
+			Type:               korifiv1alpha1.DeprovisioningFailedCondition,
+			Status:             metav1.ConditionTrue,
+			ObservedGeneration: serviceInstance.Generation,
+			LastTransitionTime: metav1.NewTime(time.Now()),
+			Reason:             "DeprovisionFailed",
+			Message:            lastOpResponse.Description,
+		})
+		return ctrl.Result{}, k8s.NewNotReadyError().WithReason("DeprovisionFailed")
+	}
+
+	return ctrl.Result{}, nil
+}
+
+func (r *Reconciler) pollLastOperation(
+	ctx context.Context,
+	serviceInstance *korifiv1alpha1.CFServiceInstance,
+	assets osbapi.ServiceInstanceAssets,
+	osbapiClient osbapi.BrokerClient,
+	operationID string,
+) (osbapi.LastOperationResponse, error) {
+	log := logr.FromContextOrDiscard(ctx).WithName("poll-operation")
+	lastOpResponse, err := osbapiClient.GetServiceInstanceLastOperation(ctx, osbapi.GetServiceInstanceLastOperationRequest{
+		InstanceID: serviceInstance.Name,
+		GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{
+			ServiceId: assets.ServiceOffering.Spec.BrokerCatalog.ID,
+			PlanID:    assets.ServicePlan.Spec.BrokerCatalog.ID,
+			Operation: operationID,
+		},
+	})
+	if err != nil {
+		log.Error(err, "getting service instance last operation failed")
+		return osbapi.LastOperationResponse{}, k8s.NewNotReadyError().WithCause(err).WithReason("GetLastOperationFailed")
+	}
+
+	serviceInstance.Status.LastOperation.State = lastOpResponse.State
+	serviceInstance.Status.LastOperation.Description = lastOpResponse.Description
+	return lastOpResponse, nil
+}
+
+func getServiceInstanceParameters(serviceInstance *korifiv1alpha1.CFServiceInstance) (map[string]any, error) {
+	if serviceInstance.Spec.Parameters == nil {
+		return nil, nil
+	}
+
+	parametersMap := map[string]any{}
+	err := json.Unmarshal(serviceInstance.Spec.Parameters.Raw, &parametersMap)
+	if err != nil {
+		return nil, fmt.Errorf("failed to unmarshal parameters: %w", err)
+	}
+
+	return parametersMap, nil
+}
+
+func (r *Reconciler) isServicePlanVisible(
+	ctx context.Context,
+	serviceInstance *korifiv1alpha1.CFServiceInstance,
+	servicePlan *korifiv1alpha1.CFServicePlan,
+) (bool, error) {
+	if servicePlan.Spec.Visibility.Type == korifiv1alpha1.AdminServicePlanVisibilityType {
+		return false, nil
+	}
+
+	if servicePlan.Spec.Visibility.Type == korifiv1alpha1.PublicServicePlanVisibilityType {
+		return true, nil
+	}
+
+	namespace := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: serviceInstance.Namespace,
+		},
+	}
+	err := r.k8sClient.Get(ctx, client.ObjectKeyFromObject(namespace), namespace)
+	if err != nil {
+		return false, err
+	}
+
+	return slices.Contains(servicePlan.Spec.Visibility.Organizations, namespace.Labels[korifiv1alpha1.OrgGUIDKey]), nil
+}
+
 func (r *Reconciler) getNamespace(ctx context.Context, namespaceName string) (*corev1.Namespace, error) {
 	namespace := &corev1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{