diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index f080638bf..a2cb3e6bd 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -238,96 +238,71 @@ func (r *Reconciler) finalizeCFServiceInstance( return ctrl.Result{}, nil } - servicePlan, err := r.getServicePlan(ctx, serviceInstance.Spec.PlanGUID) - if err != nil { - log.Error(err, "failed to get service plan") - + if isDeprovisionRequested(serviceInstance) { if controllerutil.RemoveFinalizer(serviceInstance, korifiv1alpha1.CFManagedServiceInstanceFinalizerName) { log.V(1).Info("finalizer removed") } + return ctrl.Result{}, nil + } + + err := r.deprovisionServiceinstance(ctx, serviceInstance) + if err != nil { return ctrl.Result{}, err } + return ctrl.Result{Requeue: true}, nil +} + +func (r *Reconciler) deprovisionServiceinstance(ctx context.Context, serviceInstance *korifiv1alpha1.CFServiceInstance) error { + log := logr.FromContextOrDiscard(ctx).WithName("finalizeCFServiceInstance") + + servicePlan, err := r.getServicePlan(ctx, serviceInstance.Spec.PlanGUID) + if err != nil { + log.Error(err, "failed to get service plan") + return nil + } + serviceBroker, err := r.getServiceBroker(ctx, servicePlan.Labels[korifiv1alpha1.RelServiceBrokerGUIDLabel]) if err != nil { log.Error(err, "failed to get service broker") - - if controllerutil.RemoveFinalizer(serviceInstance, korifiv1alpha1.CFManagedServiceInstanceFinalizerName) { - log.V(1).Info("finalizer removed") - } - - return ctrl.Result{}, err + return nil } serviceOffering, err := r.getServiceOffering(ctx, servicePlan.Labels[korifiv1alpha1.RelServiceOfferingGUIDLabel]) if err != nil { log.Error(err, "failed to get service offering") - - if controllerutil.RemoveFinalizer(serviceInstance, korifiv1alpha1.CFManagedServiceInstanceFinalizerName) { - log.V(1).Info("finalizer removed") - } - return ctrl.Result{}, err + return nil } osbapiClient, err := r.osbapiClientFactory.CreateClient(ctx, serviceBroker) if err != nil { log.Error(err, "failed to create broker client", "broker", serviceBroker.Name) - - if controllerutil.RemoveFinalizer(serviceInstance, korifiv1alpha1.CFManagedServiceInstanceFinalizerName) { - log.V(1).Info("finalizer removed") - } - - return ctrl.Result{}, fmt.Errorf("failed to create client for broker %q: %w", serviceBroker.Name, err) - } - - if !isDeprovisionRequested(serviceInstance) { - - var deprovisionResponse osbapi.ServiceInstanceOperationResponse - deprovisionResponse, err = osbapiClient.Deprovision(ctx, osbapi.InstanceDeprovisionPayload{ - ID: serviceInstance.Name, - InstanceDeprovisionRequest: osbapi.InstanceDeprovisionRequest{ - ServiceId: serviceOffering.Spec.BrokerCatalog.ID, - PlanID: servicePlan.Spec.BrokerCatalog.ID, - }, - }) - if err != nil { - log.Error(err, "failed to deprovision service instance") - return ctrl.Result{}, k8s.NewNotReadyError().WithReason("DeprovisionFailed") - } - - serviceInstance.Status.DeprovisionOperation = deprovisionResponse.Operation - meta.SetStatusCondition(&serviceInstance.Status.Conditions, metav1.Condition{ - Type: korifiv1alpha1.DeprovisionRequestedCondition, - Status: metav1.ConditionTrue, - ObservedGeneration: serviceInstance.Generation, - LastTransitionTime: metav1.NewTime(time.Now()), - Reason: "DeprovisionRequested", - }) + return nil } - - lastOpResponse, err := osbapiClient.GetServiceInstanceLastOperation(ctx, osbapi.GetLastOperationPayload{ + var deprovisionResponse osbapi.ServiceInstanceOperationResponse + deprovisionResponse, err = osbapiClient.Deprovision(ctx, osbapi.InstanceDeprovisionPayload{ ID: serviceInstance.Name, - GetLastOperationRequest: osbapi.GetLastOperationRequest{ + InstanceDeprovisionRequest: osbapi.InstanceDeprovisionRequest{ ServiceId: serviceOffering.Spec.BrokerCatalog.ID, PlanID: servicePlan.Spec.BrokerCatalog.ID, - Operation: serviceInstance.Status.DeprovisionOperation, }, }) if err != nil { - log.Error(err, "getting service instance last operation failed") - return ctrl.Result{}, k8s.NewNotReadyError().WithCause(err).WithReason("GetLastOperationFailed") - } - - if lastOpResponse.State == "in progress" { - return ctrl.Result{}, k8s.NewNotReadyError().WithReason("DeprovisionInProgress").WithRequeue() + log.Error(err, "failed to deprovision service instance") + return k8s.NewNotReadyError().WithReason("DeprovisionFailed") } - if controllerutil.RemoveFinalizer(serviceInstance, korifiv1alpha1.CFManagedServiceInstanceFinalizerName) { - log.V(1).Info("finalizer removed", "deprovision-operation-result", lastOpResponse) - } + serviceInstance.Status.DeprovisionOperation = deprovisionResponse.Operation + meta.SetStatusCondition(&serviceInstance.Status.Conditions, metav1.Condition{ + Type: korifiv1alpha1.DeprovisionRequestedCondition, + Status: metav1.ConditionTrue, + ObservedGeneration: serviceInstance.Generation, + LastTransitionTime: metav1.NewTime(time.Now()), + Reason: "DeprovisionRequested", + }) - return ctrl.Result{}, nil + return k8s.NewNotReadyError().WithReason("DeprovisionRequested").WithRequeue() } func (r *Reconciler) getNamespace(ctx context.Context, namespaceName string) (*corev1.Namespace, error) { diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index 7cdd4cbd0..bfc77ebbf 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -516,12 +516,8 @@ var _ = Describe("CFServiceInstance", func() { It("deprovisions the instance with the broker", func() { Eventually(func(g Gomega) { - g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) - g.Expect(instance.Status.DeprovisionOperation).To(Equal("deprovision-op")) - g.Expect(instance.Status.Conditions).To(ContainElement(SatisfyAll( - HasType(Equal(korifiv1alpha1.DeprovisionRequestedCondition)), - HasStatus(Equal(metav1.ConditionTrue)), - ))) + err := adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance) + g.Expect(k8serrors.IsNotFound(err)).To(BeTrue()) g.Expect(brokerClient.DeprovisionCallCount()).To(Equal(1)) _, actualDeprovisionRequest := brokerClient.DeprovisionArgsForCall(0) @@ -535,44 +531,12 @@ var _ = Describe("CFServiceInstance", func() { }).Should(Succeed()) }) - It("checks deprovision operation status", func() { - Eventually(func(g Gomega) { - g.Expect(brokerClient.GetServiceInstanceLastOperationCallCount()).To(BeNumerically(">", 0)) - _, lastOp := brokerClient.GetServiceInstanceLastOperationArgsForCall(brokerClient.GetServiceInstanceLastOperationCallCount() - 1) - Expect(lastOp).To(Equal(osbapi.GetLastOperationPayload{ - ID: instance.Name, - GetLastOperationRequest: osbapi.GetLastOperationRequest{ - ServiceId: "service-offering-id", - PlanID: "service-plan-id", - Operation: "deprovision-op", - }, - })) - }).Should(Succeed()) - }) - It("does not delete the service instance", func() { Consistently(func(g Gomega) { g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) }).Should(Succeed()) }) - When("checking the deprovision operation fails", func() { - BeforeEach(func() { - brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{}, errors.New("last-op-error")) - }) - - It("sets ready condition to false", func() { - Eventually(func(g Gomega) { - g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed()) - g.Expect(instance.Status.Conditions).To(ContainElement(SatisfyAll( - HasType(Equal(korifiv1alpha1.StatusConditionReady)), - HasStatus(Equal(metav1.ConditionFalse)), - HasReason(Equal("GetLastOperationFailed")), - ))) - }).Should(Succeed()) - }) - }) - When("deprovision fails", func() { BeforeEach(func() { brokerClient.DeprovisionReturns(osbapi.ServiceInstanceOperationResponse{}, errors.New("deprovision-failed"))