From 9428bae87fc7e437c521a32b5ff1b9ec02f8f252 Mon Sep 17 00:00:00 2001 From: Danail Branekov Date: Mon, 9 Sep 2024 15:16:50 +0000 Subject: [PATCH] Add deletion of managed service instances --- api/handlers/job.go | 1 + api/handlers/service_instance.go | 5 + api/handlers/service_instance_test.go | 24 ++++ api/main.go | 15 ++- api/presenter/job.go | 1 + .../service_instance_repository.go | 10 ++ .../service_instance_repository_test.go | 57 ++++++++ .../api/v1alpha1/cfserviceinstance_types.go | 8 +- .../services/brokers/fake/broker_client.go | 101 ++++++++++++-- .../services/instances/managed/controller.go | 84 +++++++++--- .../instances/managed/controller_test.go | 107 ++++++++++++++- .../instances/managed/fake/broker_client.go | 101 ++++++++++++-- .../controllers/services/osbapi/client.go | 37 +++++- .../services/osbapi/client_test.go | 79 ++++++++++- .../services/osbapi/clientfactory.go | 3 +- .../controllers/services/osbapi/types.go | 12 +- ...i.cloudfoundry.org_cfserviceinstances.yaml | 2 + tests/assets/sample-broker-golang/main.go | 11 ++ tests/e2e/service_instances_test.go | 125 +++++++++++++++--- tests/smoke/services_test.go | 15 +++ 20 files changed, 717 insertions(+), 81 deletions(-) diff --git a/api/handlers/job.go b/api/handlers/job.go index 97a23cfc7..24ccff58d 100644 --- a/api/handlers/job.go +++ b/api/handlers/job.go @@ -28,6 +28,7 @@ const ( ServiceBrokerCreateJobType = "service_broker.create" ServiceBrokerUpdateJobType = "service_broker.update" ServiceBrokerDeleteJobType = "service_broker.delete" + ManagedServiceInstanceDeleteJobType = "managed_service_instance.delete" ManagedServiceInstanceCreateJobType = "managed_service_instance.create" JobTimeoutDuration = 120.0 ) diff --git a/api/handlers/service_instance.go b/api/handlers/service_instance.go index b19841d23..a34c3f90a 100644 --- a/api/handlers/service_instance.go +++ b/api/handlers/service_instance.go @@ -12,6 +12,7 @@ import ( "code.cloudfoundry.org/korifi/api/routing" "code.cloudfoundry.org/korifi/api/presenter" + korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/api/repositories" @@ -207,6 +208,10 @@ func (h *ServiceInstance) delete(r *http.Request) (*routing.Response, error) { return nil, apierrors.LogAndReturn(logger, err, "error when deleting service instance", "guid", serviceInstanceGUID) } + if serviceInstance.Type == korifiv1alpha1.ManagedType { + return routing.NewResponse(http.StatusAccepted).WithHeader("Location", presenter.JobURLForRedirects(serviceInstance.GUID, presenter.ManagedServiceInstanceDeleteOperation, h.serverURL)), nil + } + return routing.NewResponse(http.StatusNoContent), nil } diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index c4118daf8..41abad7df 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -42,6 +42,7 @@ var _ = Describe("ServiceInstance", func() { serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ GUID: "service-instance-guid", SpaceGUID: "space-guid", + Type: korifiv1alpha1.UserProvidedType, }, nil) spaceRepo = new(fake.CFSpaceRepository) @@ -566,6 +567,29 @@ var _ = Describe("ServiceInstance", func() { Expect(rr).To(HaveHTTPStatus(http.StatusNoContent)) }) + When("the service instance is managed", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ + GUID: "service-instance-guid", + SpaceGUID: "space-guid", + Type: korifiv1alpha1.ManagedType, + }, nil) + }) + + It("deletes the service instance", func() { + Expect(serviceInstanceRepo.DeleteServiceInstanceCallCount()).To(Equal(1)) + _, actualAuthInfo, message := serviceInstanceRepo.DeleteServiceInstanceArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(message.GUID).To(Equal("service-instance-guid")) + Expect(message.SpaceGUID).To(Equal("space-guid")) + + Expect(rr).To(SatisfyAll( + HaveHTTPStatus(http.StatusAccepted), + HaveHTTPHeaderWithValue("Location", ContainSubstring("/v3/jobs/managed_service_instance.delete~service-instance-guid")), + )) + }) + }) + When("getting the service instance fails with not found", func() { BeforeEach(func() { serviceInstanceRepo.GetServiceInstanceReturns( diff --git a/api/main.go b/api/main.go index 4bfe95775..a694de56d 100644 --- a/api/main.go +++ b/api/main.go @@ -355,13 +355,14 @@ func main() { handlers.NewJob( *serverURL, map[string]handlers.DeletionRepository{ - handlers.OrgDeleteJobType: orgRepo, - handlers.SpaceDeleteJobType: spaceRepo, - handlers.AppDeleteJobType: appRepo, - handlers.RouteDeleteJobType: routeRepo, - handlers.DomainDeleteJobType: domainRepo, - handlers.RoleDeleteJobType: roleRepo, - handlers.ServiceBrokerDeleteJobType: serviceBrokerRepo, + handlers.OrgDeleteJobType: orgRepo, + handlers.SpaceDeleteJobType: spaceRepo, + handlers.AppDeleteJobType: appRepo, + handlers.RouteDeleteJobType: routeRepo, + handlers.DomainDeleteJobType: domainRepo, + handlers.RoleDeleteJobType: roleRepo, + handlers.ServiceBrokerDeleteJobType: serviceBrokerRepo, + handlers.ManagedServiceInstanceDeleteJobType: serviceInstanceRepo, }, map[string]handlers.StateRepository{ handlers.ServiceBrokerCreateJobType: serviceBrokerRepo, diff --git a/api/presenter/job.go b/api/presenter/job.go index b387333a6..fca06b514 100644 --- a/api/presenter/job.go +++ b/api/presenter/job.go @@ -27,6 +27,7 @@ const ( ServiceBrokerDeleteOperation = "service_broker.delete" ServiceBrokerUpdateOperation = "service_broker.update" ManagedServiceInstanceCreateOperation = "managed_service_instance.create" + ManagedServiceInstanceDeleteOperation = "managed_service_instance.delete" ) var ( diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index 92e9f62e8..3fb7e679f 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -130,6 +130,7 @@ type ServiceInstanceRecord struct { Annotations map[string]string CreatedAt time.Time UpdatedAt *time.Time + DeletedAt *time.Time Ready bool } @@ -405,6 +406,14 @@ func (r *ServiceInstanceRepo) GetState(ctx context.Context, authInfo authorizati return model.CFResourceStateUnknown, nil } +func (r *ServiceInstanceRepo) GetDeletedAt(ctx context.Context, authInfo authorization.Info, instanceGUID string) (*time.Time, error) { + serviceInstance, err := r.GetServiceInstance(ctx, authInfo, instanceGUID) + if err != nil { + return nil, err + } + return serviceInstance.DeletedAt, nil +} + func cfServiceInstanceToRecord(cfServiceInstance korifiv1alpha1.CFServiceInstance) ServiceInstanceRecord { return ServiceInstanceRecord{ Name: cfServiceInstance.Spec.DisplayName, @@ -418,6 +427,7 @@ func cfServiceInstanceToRecord(cfServiceInstance korifiv1alpha1.CFServiceInstanc Annotations: cfServiceInstance.Annotations, CreatedAt: cfServiceInstance.CreationTimestamp.Time, UpdatedAt: getLastUpdatedTime(&cfServiceInstance), + DeletedAt: golangTime(cfServiceInstance.DeletionTimestamp), Ready: isReady(cfServiceInstance), } } diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 1bb89230a..88a18a82e 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -224,6 +224,63 @@ var _ = Describe("ServiceInstanceRepository", func() { }) }) + Describe("GetDeletedAt", func() { + var ( + cfServiceInstance *korifiv1alpha1.CFServiceInstance + deletionTime *time.Time + getErr error + ) + + BeforeEach(func() { + cfServiceInstance = &korifiv1alpha1.CFServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: uuid.NewString(), + Namespace: space.Name, + }, + Spec: korifiv1alpha1.CFServiceInstanceSpec{ + Type: "managed", + }, + } + + Expect(k8sClient.Create(ctx, cfServiceInstance)).To(Succeed()) + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) + }) + + JustBeforeEach(func() { + deletionTime, getErr = serviceInstanceRepo.GetDeletedAt(ctx, authInfo, cfServiceInstance.Name) + }) + + It("returns nil", func() { + Expect(getErr).NotTo(HaveOccurred()) + Expect(deletionTime).To(BeNil()) + }) + + When("the instance is being deleted", func() { + BeforeEach(func() { + Expect(k8s.PatchResource(ctx, k8sClient, cfServiceInstance, func() { + cfServiceInstance.Finalizers = append(cfServiceInstance.Finalizers, "foo") + })).To(Succeed()) + + Expect(k8sClient.Delete(ctx, cfServiceInstance)).To(Succeed()) + }) + + It("returns the deletion time", func() { + Expect(getErr).NotTo(HaveOccurred()) + Expect(deletionTime).To(PointTo(BeTemporally("~", time.Now(), time.Minute))) + }) + }) + + When("the instance isn't found", func() { + BeforeEach(func() { + Expect(k8sClient.Delete(ctx, cfServiceInstance)).To(Succeed()) + }) + + It("errors", func() { + Expect(getErr).To(matchers.WrapErrorAssignableToTypeOf(apierrors.NotFoundError{})) + }) + }) + }) + Describe("GetState", func() { var ( cfServiceInstance *korifiv1alpha1.CFServiceInstance diff --git a/controllers/api/v1alpha1/cfserviceinstance_types.go b/controllers/api/v1alpha1/cfserviceinstance_types.go index beb4ecc49..28c29e519 100644 --- a/controllers/api/v1alpha1/cfserviceinstance_types.go +++ b/controllers/api/v1alpha1/cfserviceinstance_types.go @@ -30,8 +30,9 @@ const ( CFManagedServiceInstanceFinalizerName = "managed.cfServiceInstance.korifi.cloudfoundry.org" - ProvisionRequestedCondition = "ProvisionRequested" - ProvisioningFailedCondition = "ProvisioningFailed" + ProvisionRequestedCondition = "ProvisionRequested" + ProvisioningFailedCondition = "ProvisioningFailed" + DeprovisionRequestedCondition = "DeprovisionRequested" ) // CFServiceInstanceSpec defines the desired state of CFServiceInstance @@ -80,7 +81,8 @@ type CFServiceInstanceStatus struct { //+kubebuilder:validation:Optional CredentialsObservedVersion string `json:"credentialsObservedVersion,omitempty"` - ProvisionOperation string `json:"provisionOperation,omitempty"` + ProvisionOperation string `json:"provisionOperation,omitempty"` + DeprovisionOperation string `json:"deprovisionOperation,omitempty"` } //+kubebuilder:object:root=true diff --git a/controllers/controllers/services/brokers/fake/broker_client.go b/controllers/controllers/services/brokers/fake/broker_client.go index a90be2e68..9ec08dcf4 100644 --- a/controllers/controllers/services/brokers/fake/broker_client.go +++ b/controllers/controllers/services/brokers/fake/broker_client.go @@ -9,6 +9,20 @@ import ( ) type BrokerClient struct { + DeprovisionStub func(context.Context, osbapi.InstanceDeprovisionPayload) (osbapi.ServiceInstanceOperationResponse, error) + deprovisionMutex sync.RWMutex + deprovisionArgsForCall []struct { + arg1 context.Context + arg2 osbapi.InstanceDeprovisionPayload + } + deprovisionReturns struct { + result1 osbapi.ServiceInstanceOperationResponse + result2 error + } + deprovisionReturnsOnCall map[int]struct { + result1 osbapi.ServiceInstanceOperationResponse + result2 error + } GetCatalogStub func(context.Context) (osbapi.Catalog, error) getCatalogMutex sync.RWMutex getCatalogArgsForCall []struct { @@ -36,24 +50,89 @@ type BrokerClient struct { result1 osbapi.LastOperationResponse result2 error } - ProvisionStub func(context.Context, osbapi.InstanceProvisionPayload) (osbapi.ProvisionServiceInstanceResponse, error) + ProvisionStub func(context.Context, osbapi.InstanceProvisionPayload) (osbapi.ServiceInstanceOperationResponse, error) provisionMutex sync.RWMutex provisionArgsForCall []struct { arg1 context.Context arg2 osbapi.InstanceProvisionPayload } provisionReturns struct { - result1 osbapi.ProvisionServiceInstanceResponse + result1 osbapi.ServiceInstanceOperationResponse result2 error } provisionReturnsOnCall map[int]struct { - result1 osbapi.ProvisionServiceInstanceResponse + result1 osbapi.ServiceInstanceOperationResponse result2 error } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } +func (fake *BrokerClient) Deprovision(arg1 context.Context, arg2 osbapi.InstanceDeprovisionPayload) (osbapi.ServiceInstanceOperationResponse, error) { + fake.deprovisionMutex.Lock() + ret, specificReturn := fake.deprovisionReturnsOnCall[len(fake.deprovisionArgsForCall)] + fake.deprovisionArgsForCall = append(fake.deprovisionArgsForCall, struct { + arg1 context.Context + arg2 osbapi.InstanceDeprovisionPayload + }{arg1, arg2}) + stub := fake.DeprovisionStub + fakeReturns := fake.deprovisionReturns + fake.recordInvocation("Deprovision", []interface{}{arg1, arg2}) + fake.deprovisionMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *BrokerClient) DeprovisionCallCount() int { + fake.deprovisionMutex.RLock() + defer fake.deprovisionMutex.RUnlock() + return len(fake.deprovisionArgsForCall) +} + +func (fake *BrokerClient) DeprovisionCalls(stub func(context.Context, osbapi.InstanceDeprovisionPayload) (osbapi.ServiceInstanceOperationResponse, error)) { + fake.deprovisionMutex.Lock() + defer fake.deprovisionMutex.Unlock() + fake.DeprovisionStub = stub +} + +func (fake *BrokerClient) DeprovisionArgsForCall(i int) (context.Context, osbapi.InstanceDeprovisionPayload) { + fake.deprovisionMutex.RLock() + defer fake.deprovisionMutex.RUnlock() + argsForCall := fake.deprovisionArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *BrokerClient) DeprovisionReturns(result1 osbapi.ServiceInstanceOperationResponse, result2 error) { + fake.deprovisionMutex.Lock() + defer fake.deprovisionMutex.Unlock() + fake.DeprovisionStub = nil + fake.deprovisionReturns = struct { + result1 osbapi.ServiceInstanceOperationResponse + result2 error + }{result1, result2} +} + +func (fake *BrokerClient) DeprovisionReturnsOnCall(i int, result1 osbapi.ServiceInstanceOperationResponse, result2 error) { + fake.deprovisionMutex.Lock() + defer fake.deprovisionMutex.Unlock() + fake.DeprovisionStub = nil + if fake.deprovisionReturnsOnCall == nil { + fake.deprovisionReturnsOnCall = make(map[int]struct { + result1 osbapi.ServiceInstanceOperationResponse + result2 error + }) + } + fake.deprovisionReturnsOnCall[i] = struct { + result1 osbapi.ServiceInstanceOperationResponse + result2 error + }{result1, result2} +} + func (fake *BrokerClient) GetCatalog(arg1 context.Context) (osbapi.Catalog, error) { fake.getCatalogMutex.Lock() ret, specificReturn := fake.getCatalogReturnsOnCall[len(fake.getCatalogArgsForCall)] @@ -183,7 +262,7 @@ func (fake *BrokerClient) GetServiceInstanceLastOperationReturnsOnCall(i int, re }{result1, result2} } -func (fake *BrokerClient) Provision(arg1 context.Context, arg2 osbapi.InstanceProvisionPayload) (osbapi.ProvisionServiceInstanceResponse, error) { +func (fake *BrokerClient) Provision(arg1 context.Context, arg2 osbapi.InstanceProvisionPayload) (osbapi.ServiceInstanceOperationResponse, error) { fake.provisionMutex.Lock() ret, specificReturn := fake.provisionReturnsOnCall[len(fake.provisionArgsForCall)] fake.provisionArgsForCall = append(fake.provisionArgsForCall, struct { @@ -209,7 +288,7 @@ func (fake *BrokerClient) ProvisionCallCount() int { return len(fake.provisionArgsForCall) } -func (fake *BrokerClient) ProvisionCalls(stub func(context.Context, osbapi.InstanceProvisionPayload) (osbapi.ProvisionServiceInstanceResponse, error)) { +func (fake *BrokerClient) ProvisionCalls(stub func(context.Context, osbapi.InstanceProvisionPayload) (osbapi.ServiceInstanceOperationResponse, error)) { fake.provisionMutex.Lock() defer fake.provisionMutex.Unlock() fake.ProvisionStub = stub @@ -222,28 +301,28 @@ func (fake *BrokerClient) ProvisionArgsForCall(i int) (context.Context, osbapi.I return argsForCall.arg1, argsForCall.arg2 } -func (fake *BrokerClient) ProvisionReturns(result1 osbapi.ProvisionServiceInstanceResponse, result2 error) { +func (fake *BrokerClient) ProvisionReturns(result1 osbapi.ServiceInstanceOperationResponse, result2 error) { fake.provisionMutex.Lock() defer fake.provisionMutex.Unlock() fake.ProvisionStub = nil fake.provisionReturns = struct { - result1 osbapi.ProvisionServiceInstanceResponse + result1 osbapi.ServiceInstanceOperationResponse result2 error }{result1, result2} } -func (fake *BrokerClient) ProvisionReturnsOnCall(i int, result1 osbapi.ProvisionServiceInstanceResponse, result2 error) { +func (fake *BrokerClient) ProvisionReturnsOnCall(i int, result1 osbapi.ServiceInstanceOperationResponse, result2 error) { fake.provisionMutex.Lock() defer fake.provisionMutex.Unlock() fake.ProvisionStub = nil if fake.provisionReturnsOnCall == nil { fake.provisionReturnsOnCall = make(map[int]struct { - result1 osbapi.ProvisionServiceInstanceResponse + result1 osbapi.ServiceInstanceOperationResponse result2 error }) } fake.provisionReturnsOnCall[i] = struct { - result1 osbapi.ProvisionServiceInstanceResponse + result1 osbapi.ServiceInstanceOperationResponse result2 error }{result1, result2} } @@ -251,6 +330,8 @@ func (fake *BrokerClient) ProvisionReturnsOnCall(i int, result1 osbapi.Provision func (fake *BrokerClient) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() + fake.deprovisionMutex.RLock() + defer fake.deprovisionMutex.RUnlock() fake.getCatalogMutex.RLock() defer fake.getCatalogMutex.RUnlock() fake.getServiceInstanceLastOperationMutex.RLock() diff --git a/controllers/controllers/services/instances/managed/controller.go b/controllers/controllers/services/instances/managed/controller.go index aef680e6e..249e59a70 100644 --- a/controllers/controllers/services/instances/managed/controller.go +++ b/controllers/controllers/services/instances/managed/controller.go @@ -93,18 +93,6 @@ 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) - if !serviceInstance.GetDeletionTimestamp().IsZero() { - return r.finalizeCFServiceInstance(ctx, serviceInstance) - } - - if isReady(serviceInstance) { - return ctrl.Result{}, nil - } - - if isFailed(serviceInstance) { - return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisioningFailed").WithNoRequeue() - } - servicePlan, err := r.getServicePlan(ctx, serviceInstance.Spec.PlanGUID) if err != nil { log.Error(err, "failed to get service plan") @@ -129,6 +117,18 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor return ctrl.Result{}, fmt.Errorf("failed to create client for broker %q: %w", serviceBroker.Name, err) } + if !serviceInstance.GetDeletionTimestamp().IsZero() { + return r.finalizeCFServiceInstance(ctx, osbapiClient, serviceInstance, serviceOffering, servicePlan) + } + + if isReady(serviceInstance) { + return ctrl.Result{}, nil + } + + if isFailed(serviceInstance) { + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("ProvisioningFailed").WithNoRequeue() + } + if !isProvisionRequested(serviceInstance) { return r.provisionServiceInstance(ctx, osbapiClient, serviceInstance, servicePlan, serviceOffering) } @@ -186,7 +186,7 @@ func (r *Reconciler) provisionServiceInstance( return ctrl.Result{}, err } - var provisionResponse osbapi.ProvisionServiceInstanceResponse + var provisionResponse osbapi.ServiceInstanceOperationResponse provisionResponse, err = osbapiClient.Provision(ctx, osbapi.InstanceProvisionPayload{ InstanceID: serviceInstance.Name, InstanceProvisionRequest: osbapi.InstanceProvisionRequest{ @@ -228,15 +228,61 @@ func getServiceInstanceParameters(serviceInstance *korifiv1alpha1.CFServiceInsta return parametersMap, nil } -func (r *Reconciler) finalizeCFServiceInstance(ctx context.Context, cfServiceInstance *korifiv1alpha1.CFServiceInstance) (ctrl.Result, error) { +func (r *Reconciler) finalizeCFServiceInstance( + ctx context.Context, + osbapiClient osbapi.BrokerClient, + serviceInstance *korifiv1alpha1.CFServiceInstance, + serviceOffering *korifiv1alpha1.CFServiceOffering, + servicePlan *korifiv1alpha1.CFServicePlan, +) (ctrl.Result, error) { log := logr.FromContextOrDiscard(ctx).WithName("finalizeCFServiceInstance") - if !controllerutil.ContainsFinalizer(cfServiceInstance, korifiv1alpha1.CFManagedServiceInstanceFinalizerName) { + if !controllerutil.ContainsFinalizer(serviceInstance, korifiv1alpha1.CFManagedServiceInstanceFinalizerName) { return ctrl.Result{}, nil } - if controllerutil.RemoveFinalizer(cfServiceInstance, korifiv1alpha1.CFManagedServiceInstanceFinalizerName) { - log.V(1).Info("finalizer removed") + if !isDeprovisionRequested(serviceInstance) { + 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", + }) + } + + lastOpResponse, err := osbapiClient.GetServiceInstanceLastOperation(ctx, osbapi.GetLastOperationPayload{ + ID: serviceInstance.Name, + GetLastOperationRequest: osbapi.GetLastOperationRequest{ + 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() + } + + if controllerutil.RemoveFinalizer(serviceInstance, korifiv1alpha1.CFManagedServiceInstanceFinalizerName) { + log.V(1).Info("finalizer removed", "deprovision-operation-result", lastOpResponse) } return ctrl.Result{}, nil @@ -304,6 +350,10 @@ func isProvisionRequested(instance *korifiv1alpha1.CFServiceInstance) bool { return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.ProvisionRequestedCondition) } +func isDeprovisionRequested(instance *korifiv1alpha1.CFServiceInstance) bool { + return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.DeprovisionRequestedCondition) +} + func isFailed(instance *korifiv1alpha1.CFServiceInstance) bool { return meta.IsStatusConditionTrue(instance.Status.Conditions, korifiv1alpha1.ProvisioningFailedCondition) } diff --git a/controllers/controllers/services/instances/managed/controller_test.go b/controllers/controllers/services/instances/managed/controller_test.go index ab0dc0f30..7cdd4cbd0 100644 --- a/controllers/controllers/services/instances/managed/controller_test.go +++ b/controllers/controllers/services/instances/managed/controller_test.go @@ -34,7 +34,7 @@ var _ = Describe("CFServiceInstance", func() { brokerClient = new(fake.BrokerClient) brokerClientFactory.CreateClientReturns(brokerClient, nil) - brokerClient.ProvisionReturns(osbapi.ProvisionServiceInstanceResponse{ + brokerClient.ProvisionReturns(osbapi.ServiceInstanceOperationResponse{ Operation: "operation-1", }, nil) @@ -263,7 +263,7 @@ var _ = Describe("CFServiceInstance", func() { When("service provisioning fails", func() { BeforeEach(func() { - brokerClient.ProvisionReturns(osbapi.ProvisionServiceInstanceResponse{}, errors.New("provision-failed")) + brokerClient.ProvisionReturns(osbapi.ServiceInstanceOperationResponse{}, errors.New("provision-failed")) }) It("sets the ready condition to false", func() { @@ -499,16 +499,111 @@ var _ = Describe("CFServiceInstance", func() { }) When("the instance is deleted", func() { + BeforeEach(func() { + brokerClient.DeprovisionReturns(osbapi.ServiceInstanceOperationResponse{ + Operation: "deprovision-op", + }, nil) + brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ + State: "in progress", + }, nil) + }) + JustBeforeEach(func() { - Expect(adminClient.Delete(ctx, instance)).To(Succeed()) + // For deletion test we want to request deletion and verify the behaviour when finalization fails. + // Therefore we use the standard k8s client instncea of `adminClient` as it ensures that the object is deleted + Expect(k8sManager.GetClient().Delete(ctx, instance)).To(Succeed()) + }) + + 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)), + ))) + + g.Expect(brokerClient.DeprovisionCallCount()).To(Equal(1)) + _, actualDeprovisionRequest := brokerClient.DeprovisionArgsForCall(0) + Expect(actualDeprovisionRequest).To(Equal(osbapi.InstanceDeprovisionPayload{ + ID: instance.Name, + InstanceDeprovisionRequest: osbapi.InstanceDeprovisionRequest{ + ServiceId: "service-offering-id", + PlanID: "service-plan-id", + }, + })) + }).Should(Succeed()) }) - It("deletes the instance", func() { + It("checks deprovision operation status", func() { Eventually(func(g Gomega) { - err := adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance) - g.Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + 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")) + }) + + 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("DeprovisionFailed")), + ))) + }).Should(Succeed()) + }) + }) + + When("deprovision operation completes", func() { + BeforeEach(func() { + brokerClient.GetServiceInstanceLastOperationReturns(osbapi.LastOperationResponse{ + State: "succeeded", + }, nil) + }) + + It("deletes the instance", func() { + Eventually(func(g Gomega) { + err := adminClient.Get(ctx, client.ObjectKeyFromObject(instance), instance) + g.Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + }).Should(Succeed()) + }) + }) }) When("the service instance is user-provided", func() { diff --git a/controllers/controllers/services/instances/managed/fake/broker_client.go b/controllers/controllers/services/instances/managed/fake/broker_client.go index a90be2e68..9ec08dcf4 100644 --- a/controllers/controllers/services/instances/managed/fake/broker_client.go +++ b/controllers/controllers/services/instances/managed/fake/broker_client.go @@ -9,6 +9,20 @@ import ( ) type BrokerClient struct { + DeprovisionStub func(context.Context, osbapi.InstanceDeprovisionPayload) (osbapi.ServiceInstanceOperationResponse, error) + deprovisionMutex sync.RWMutex + deprovisionArgsForCall []struct { + arg1 context.Context + arg2 osbapi.InstanceDeprovisionPayload + } + deprovisionReturns struct { + result1 osbapi.ServiceInstanceOperationResponse + result2 error + } + deprovisionReturnsOnCall map[int]struct { + result1 osbapi.ServiceInstanceOperationResponse + result2 error + } GetCatalogStub func(context.Context) (osbapi.Catalog, error) getCatalogMutex sync.RWMutex getCatalogArgsForCall []struct { @@ -36,24 +50,89 @@ type BrokerClient struct { result1 osbapi.LastOperationResponse result2 error } - ProvisionStub func(context.Context, osbapi.InstanceProvisionPayload) (osbapi.ProvisionServiceInstanceResponse, error) + ProvisionStub func(context.Context, osbapi.InstanceProvisionPayload) (osbapi.ServiceInstanceOperationResponse, error) provisionMutex sync.RWMutex provisionArgsForCall []struct { arg1 context.Context arg2 osbapi.InstanceProvisionPayload } provisionReturns struct { - result1 osbapi.ProvisionServiceInstanceResponse + result1 osbapi.ServiceInstanceOperationResponse result2 error } provisionReturnsOnCall map[int]struct { - result1 osbapi.ProvisionServiceInstanceResponse + result1 osbapi.ServiceInstanceOperationResponse result2 error } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } +func (fake *BrokerClient) Deprovision(arg1 context.Context, arg2 osbapi.InstanceDeprovisionPayload) (osbapi.ServiceInstanceOperationResponse, error) { + fake.deprovisionMutex.Lock() + ret, specificReturn := fake.deprovisionReturnsOnCall[len(fake.deprovisionArgsForCall)] + fake.deprovisionArgsForCall = append(fake.deprovisionArgsForCall, struct { + arg1 context.Context + arg2 osbapi.InstanceDeprovisionPayload + }{arg1, arg2}) + stub := fake.DeprovisionStub + fakeReturns := fake.deprovisionReturns + fake.recordInvocation("Deprovision", []interface{}{arg1, arg2}) + fake.deprovisionMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *BrokerClient) DeprovisionCallCount() int { + fake.deprovisionMutex.RLock() + defer fake.deprovisionMutex.RUnlock() + return len(fake.deprovisionArgsForCall) +} + +func (fake *BrokerClient) DeprovisionCalls(stub func(context.Context, osbapi.InstanceDeprovisionPayload) (osbapi.ServiceInstanceOperationResponse, error)) { + fake.deprovisionMutex.Lock() + defer fake.deprovisionMutex.Unlock() + fake.DeprovisionStub = stub +} + +func (fake *BrokerClient) DeprovisionArgsForCall(i int) (context.Context, osbapi.InstanceDeprovisionPayload) { + fake.deprovisionMutex.RLock() + defer fake.deprovisionMutex.RUnlock() + argsForCall := fake.deprovisionArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *BrokerClient) DeprovisionReturns(result1 osbapi.ServiceInstanceOperationResponse, result2 error) { + fake.deprovisionMutex.Lock() + defer fake.deprovisionMutex.Unlock() + fake.DeprovisionStub = nil + fake.deprovisionReturns = struct { + result1 osbapi.ServiceInstanceOperationResponse + result2 error + }{result1, result2} +} + +func (fake *BrokerClient) DeprovisionReturnsOnCall(i int, result1 osbapi.ServiceInstanceOperationResponse, result2 error) { + fake.deprovisionMutex.Lock() + defer fake.deprovisionMutex.Unlock() + fake.DeprovisionStub = nil + if fake.deprovisionReturnsOnCall == nil { + fake.deprovisionReturnsOnCall = make(map[int]struct { + result1 osbapi.ServiceInstanceOperationResponse + result2 error + }) + } + fake.deprovisionReturnsOnCall[i] = struct { + result1 osbapi.ServiceInstanceOperationResponse + result2 error + }{result1, result2} +} + func (fake *BrokerClient) GetCatalog(arg1 context.Context) (osbapi.Catalog, error) { fake.getCatalogMutex.Lock() ret, specificReturn := fake.getCatalogReturnsOnCall[len(fake.getCatalogArgsForCall)] @@ -183,7 +262,7 @@ func (fake *BrokerClient) GetServiceInstanceLastOperationReturnsOnCall(i int, re }{result1, result2} } -func (fake *BrokerClient) Provision(arg1 context.Context, arg2 osbapi.InstanceProvisionPayload) (osbapi.ProvisionServiceInstanceResponse, error) { +func (fake *BrokerClient) Provision(arg1 context.Context, arg2 osbapi.InstanceProvisionPayload) (osbapi.ServiceInstanceOperationResponse, error) { fake.provisionMutex.Lock() ret, specificReturn := fake.provisionReturnsOnCall[len(fake.provisionArgsForCall)] fake.provisionArgsForCall = append(fake.provisionArgsForCall, struct { @@ -209,7 +288,7 @@ func (fake *BrokerClient) ProvisionCallCount() int { return len(fake.provisionArgsForCall) } -func (fake *BrokerClient) ProvisionCalls(stub func(context.Context, osbapi.InstanceProvisionPayload) (osbapi.ProvisionServiceInstanceResponse, error)) { +func (fake *BrokerClient) ProvisionCalls(stub func(context.Context, osbapi.InstanceProvisionPayload) (osbapi.ServiceInstanceOperationResponse, error)) { fake.provisionMutex.Lock() defer fake.provisionMutex.Unlock() fake.ProvisionStub = stub @@ -222,28 +301,28 @@ func (fake *BrokerClient) ProvisionArgsForCall(i int) (context.Context, osbapi.I return argsForCall.arg1, argsForCall.arg2 } -func (fake *BrokerClient) ProvisionReturns(result1 osbapi.ProvisionServiceInstanceResponse, result2 error) { +func (fake *BrokerClient) ProvisionReturns(result1 osbapi.ServiceInstanceOperationResponse, result2 error) { fake.provisionMutex.Lock() defer fake.provisionMutex.Unlock() fake.ProvisionStub = nil fake.provisionReturns = struct { - result1 osbapi.ProvisionServiceInstanceResponse + result1 osbapi.ServiceInstanceOperationResponse result2 error }{result1, result2} } -func (fake *BrokerClient) ProvisionReturnsOnCall(i int, result1 osbapi.ProvisionServiceInstanceResponse, result2 error) { +func (fake *BrokerClient) ProvisionReturnsOnCall(i int, result1 osbapi.ServiceInstanceOperationResponse, result2 error) { fake.provisionMutex.Lock() defer fake.provisionMutex.Unlock() fake.ProvisionStub = nil if fake.provisionReturnsOnCall == nil { fake.provisionReturnsOnCall = make(map[int]struct { - result1 osbapi.ProvisionServiceInstanceResponse + result1 osbapi.ServiceInstanceOperationResponse result2 error }) } fake.provisionReturnsOnCall[i] = struct { - result1 osbapi.ProvisionServiceInstanceResponse + result1 osbapi.ServiceInstanceOperationResponse result2 error }{result1, result2} } @@ -251,6 +330,8 @@ func (fake *BrokerClient) ProvisionReturnsOnCall(i int, result1 osbapi.Provision func (fake *BrokerClient) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() + fake.deprovisionMutex.RLock() + defer fake.deprovisionMutex.RUnlock() fake.getCatalogMutex.RLock() defer fake.getCatalogMutex.RUnlock() fake.getServiceInstanceLastOperationMutex.RLock() diff --git a/controllers/controllers/services/osbapi/client.go b/controllers/controllers/services/osbapi/client.go index dc949be42..9cdb493b1 100644 --- a/controllers/controllers/services/osbapi/client.go +++ b/controllers/controllers/services/osbapi/client.go @@ -52,7 +52,7 @@ func (c *Client) GetCatalog(ctx context.Context) (Catalog, error) { return catalog, nil } -func (c *Client) Provision(ctx context.Context, payload InstanceProvisionPayload) (ProvisionServiceInstanceResponse, error) { +func (c *Client) Provision(ctx context.Context, payload InstanceProvisionPayload) (ServiceInstanceOperationResponse, error) { statusCode, respBytes, err := c.newBrokerRequester(). forBroker(c.broker). async(). @@ -63,17 +63,44 @@ func (c *Client) Provision(ctx context.Context, payload InstanceProvisionPayload payload.InstanceProvisionRequest, ) if err != nil { - return ProvisionServiceInstanceResponse{}, fmt.Errorf("provision request failed: %w", err) + return ServiceInstanceOperationResponse{}, fmt.Errorf("provision request failed: %w", err) } if statusCode >= 300 { - return ProvisionServiceInstanceResponse{}, fmt.Errorf("provision request failed with status code: %d", statusCode) + return ServiceInstanceOperationResponse{}, fmt.Errorf("provision request failed with status code: %d", statusCode) } - var response ProvisionServiceInstanceResponse + var response ServiceInstanceOperationResponse err = json.Unmarshal(respBytes, &response) if err != nil { - return ProvisionServiceInstanceResponse{}, fmt.Errorf("failed to unmarshal response: %w", err) + return ServiceInstanceOperationResponse{}, fmt.Errorf("failed to unmarshal response: %w", err) + } + + return response, nil +} + +func (c *Client) Deprovision(ctx context.Context, payload InstanceDeprovisionPayload) (ServiceInstanceOperationResponse, error) { + statusCode, respBytes, err := c.newBrokerRequester(). + forBroker(c.broker). + async(). + sendRequest( + ctx, + "/v2/service_instances/"+payload.ID, + http.MethodDelete, + payload.InstanceDeprovisionRequest, + ) + if err != nil { + return ServiceInstanceOperationResponse{}, fmt.Errorf("deprovision request failed: %w", err) + } + + if statusCode >= 300 { + return ServiceInstanceOperationResponse{}, fmt.Errorf("deprovision request failed with status code: %d", statusCode) + } + + var response ServiceInstanceOperationResponse + err = json.Unmarshal(respBytes, &response) + if err != nil { + return ServiceInstanceOperationResponse{}, fmt.Errorf("failed to unmarshal response: %w", err) } return response, nil diff --git a/controllers/controllers/services/osbapi/client_test.go b/controllers/controllers/services/osbapi/client_test.go index 4f408f2b8..a13d47688 100644 --- a/controllers/controllers/services/osbapi/client_test.go +++ b/controllers/controllers/services/osbapi/client_test.go @@ -133,7 +133,7 @@ var _ = Describe("OSBAPI Client", func() { Describe("Instances", func() { Describe("Provision", func() { var ( - provisionResp osbapi.ProvisionServiceInstanceResponse + provisionResp osbapi.ServiceInstanceOperationResponse provisionErr error ) @@ -164,7 +164,7 @@ var _ = Describe("OSBAPI Client", func() { It("provisions the service", func() { Expect(provisionErr).NotTo(HaveOccurred()) - Expect(provisionResp).To(Equal(osbapi.ProvisionServiceInstanceResponse{ + Expect(provisionResp).To(Equal(osbapi.ServiceInstanceOperationResponse{ Operation: "provision_op1", })) }) @@ -216,6 +216,81 @@ var _ = Describe("OSBAPI Client", func() { }) }) + Describe("Deprovision", func() { + var ( + deprovisionResp osbapi.ServiceInstanceOperationResponse + deprovisionErr error + ) + + BeforeEach(func() { + brokerServer.WithResponse( + "/v2/service_instances/{id}", + map[string]any{ + "operation": "provision_op1", + }, + http.StatusOK, + ) + }) + + JustBeforeEach(func() { + deprovisionResp, deprovisionErr = brokerClient.Deprovision(ctx, osbapi.InstanceDeprovisionPayload{ + ID: "my-service-instance", + InstanceDeprovisionRequest: osbapi.InstanceDeprovisionRequest{ + ServiceId: "service-guid", + PlanID: "plan-guid", + }, + }) + }) + + It("deprovisions the service", func() { + Expect(deprovisionErr).NotTo(HaveOccurred()) + Expect(deprovisionResp).To(Equal(osbapi.ServiceInstanceOperationResponse{ + Operation: "provision_op1", + })) + }) + + It("sends async deprovision request to broker", func() { + Expect(deprovisionErr).NotTo(HaveOccurred()) + requests := brokerServer.ServedRequests() + + Expect(requests).To(HaveLen(1)) + + Expect(requests[0].Method).To(Equal(http.MethodDelete)) + Expect(requests[0].URL.Path).To(Equal("/v2/service_instances/my-service-instance")) + + Expect(requests[0].URL.Query().Get("accepts_incomplete")).To(Equal("true")) + }) + + It("sends correct request body", func() { + Expect(deprovisionErr).NotTo(HaveOccurred()) + requests := brokerServer.ServedRequests() + + Expect(requests).To(HaveLen(1)) + + requestBytes, err := io.ReadAll(requests[0].Body) + Expect(err).NotTo(HaveOccurred()) + requestBody := map[string]any{} + Expect(json.Unmarshal(requestBytes, &requestBody)).To(Succeed()) + + Expect(requestBody).To(MatchAllKeys(Keys{ + "service_id": Equal("service-guid"), + "plan_id": Equal("plan-guid"), + })) + }) + + When("the deprovision request fails", func() { + BeforeEach(func() { + brokerServer = broker.NewServer().WithHandler("/v2/service_instances/{id}", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTeapot) + })) + }) + + It("returns an error", func() { + Expect(deprovisionErr).To(MatchError(ContainSubstring("deprovision request failed"))) + }) + }) + }) + Describe("GetLastOperation", func() { var ( lastOpResp osbapi.LastOperationResponse diff --git a/controllers/controllers/services/osbapi/clientfactory.go b/controllers/controllers/services/osbapi/clientfactory.go index 31bf28b94..6dcaf82f2 100644 --- a/controllers/controllers/services/osbapi/clientfactory.go +++ b/controllers/controllers/services/osbapi/clientfactory.go @@ -16,7 +16,8 @@ import ( ) type BrokerClient interface { - Provision(context.Context, InstanceProvisionPayload) (ProvisionServiceInstanceResponse, error) + Provision(context.Context, InstanceProvisionPayload) (ServiceInstanceOperationResponse, error) + Deprovision(context.Context, InstanceDeprovisionPayload) (ServiceInstanceOperationResponse, error) GetServiceInstanceLastOperation(context.Context, GetLastOperationPayload) (LastOperationResponse, error) GetCatalog(context.Context) (Catalog, error) } diff --git a/controllers/controllers/services/osbapi/types.go b/controllers/controllers/services/osbapi/types.go index 10244d98f..93d31310f 100644 --- a/controllers/controllers/services/osbapi/types.go +++ b/controllers/controllers/services/osbapi/types.go @@ -52,6 +52,16 @@ type GetLastOperationRequest struct { Operation string `json:"operation"` } +type InstanceDeprovisionPayload struct { + ID string + InstanceDeprovisionRequest +} + +type InstanceDeprovisionRequest struct { + ServiceId string `json:"service_id"` + PlanID string `json:"plan_id"` +} + type Plan struct { ID string `json:"id"` Name string `json:"name"` @@ -64,7 +74,7 @@ type Plan struct { Schemas services.ServicePlanSchemas `json:"schemas"` } -type ProvisionServiceInstanceResponse struct { +type ServiceInstanceOperationResponse struct { Operation string `json:"operation"` } diff --git a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml index 2a0c14393..b809a3bb1 100644 --- a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml +++ b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfserviceinstances.yaml @@ -162,6 +162,8 @@ spec: ObservedGeneration captures the latest version of the spec.secretName that has been reconciled This will ensure that interested contollers are notified on instance credentials change type: string + deprovisionOperation: + type: string observedGeneration: description: ObservedGeneration captures the latest generation of the CFServiceInstance that has been reconciled diff --git a/tests/assets/sample-broker-golang/main.go b/tests/assets/sample-broker-golang/main.go index 9ba360665..9702f71d8 100644 --- a/tests/assets/sample-broker-golang/main.go +++ b/tests/assets/sample-broker-golang/main.go @@ -21,6 +21,7 @@ func main() { http.HandleFunc("GET /", helloWorldHandler) http.HandleFunc("GET /v2/catalog", getCatalogHandler) http.HandleFunc("PUT /v2/service_instances/{id}", provisionServiceInstanceHandler) + http.HandleFunc("DELETE /v2/service_instances/{id}", deprovisionServiceInstanceHandler) http.HandleFunc("GET /v2/service_instances/{id}/last_operation", serviceInstanceLastOperationHandler) port := os.Getenv("PORT") @@ -77,6 +78,16 @@ func provisionServiceInstanceHandler(w http.ResponseWriter, r *http.Request) { fmt.Fprintf(w, `{"operation":"provision-%s"}`, r.PathValue("id")) } +func deprovisionServiceInstanceHandler(w http.ResponseWriter, r *http.Request) { + if status, err := checkCredentials(w, r); err != nil { + w.WriteHeader(status) + fmt.Fprintf(w, "Credentials check failed: %v", err) + return + } + + fmt.Fprintf(w, `{"operation":"deprovision-%s"}`, r.PathValue("id")) +} + func serviceInstanceLastOperationHandler(w http.ResponseWriter, r *http.Request) { if status, err := checkCredentials(w, r); err != nil { w.WriteHeader(status) diff --git a/tests/e2e/service_instances_test.go b/tests/e2e/service_instances_test.go index d5b6562b5..5de9c2805 100644 --- a/tests/e2e/service_instances_test.go +++ b/tests/e2e/service_instances_test.go @@ -2,8 +2,10 @@ package e2e_test import ( "net/http" + "strings" "github.com/go-resty/resty/v2" + "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" . "github.com/onsi/gomega/gstruct" @@ -11,17 +13,17 @@ import ( var _ = Describe("Service Instances", func() { var ( - spaceGUID string - existingInstanceGUID string - existingInstanceName string - httpResp *resty.Response - httpError error + spaceGUID string + upsiGUID string + upsiName string + httpResp *resty.Response + httpError error ) BeforeEach(func() { spaceGUID = createSpace(generateGUID("space1"), commonTestOrgGUID) - existingInstanceName = generateGUID("service-instance") - existingInstanceGUID = createServiceInstance(spaceGUID, existingInstanceName, nil) + upsiName = generateGUID("service-instance") + upsiGUID = createServiceInstance(spaceGUID, upsiName, nil) }) AfterEach(func() { @@ -142,7 +144,7 @@ var _ = Describe("Service Instances", func() { "object-new": map[string]any{"new-a": "new-b"}, }, Tags: []string{"some", "tags"}, - }).Patch("/v3/service_instances/" + existingInstanceGUID) + }).Patch("/v3/service_instances/" + upsiGUID) }) It("succeeds", func() { @@ -161,19 +163,55 @@ var _ = Describe("Service Instances", func() { }) Describe("Delete", func() { + var serviceInstanceGUID string + JustBeforeEach(func() { - httpResp, httpError = adminClient.R().Delete("/v3/service_instances/" + existingInstanceGUID) + httpResp, httpError = adminClient.R().Delete("/v3/service_instances/" + serviceInstanceGUID) }) - It("deletes the service instance", func() { - Expect(httpError).NotTo(HaveOccurred()) - Expect(httpResp).To(HaveRestyStatusCode(http.StatusNoContent)) - Expect(listServiceInstances().Resources).NotTo(ContainElement( - MatchFields(IgnoreExtras, Fields{ - "Name": Equal(existingInstanceName), - "GUID": Equal(existingInstanceGUID), - }), - )) + When("deleting a user-provided service instance", func() { + BeforeEach(func() { + serviceInstanceGUID = upsiGUID + }) + + It("responds with deletion job location", func() { + Expect(httpError).NotTo(HaveOccurred()) + Expect(httpResp).To(HaveRestyStatusCode(http.StatusNoContent)) + Expect(listServiceInstances().Resources).NotTo(ContainElement( + MatchFields(IgnoreExtras, Fields{ + "Name": Equal(upsiName), + "GUID": Equal(upsiGUID), + }), + )) + }) + }) + + When("deleting a managed service instance", func() { + BeforeEach(func() { + brokerGUID := createBroker(serviceBrokerURL) + DeferCleanup(func() { + cleanupBroker(brokerGUID) + }) + + serviceInstanceGUID = createManagedServiceInstance(brokerGUID, spaceGUID) + }) + + It("succeeds with a job redirect", func() { + Expect(httpError).NotTo(HaveOccurred()) + Expect(httpResp).To(HaveRestyStatusCode(http.StatusAccepted)) + + Expect(httpResp).To(SatisfyAll( + HaveRestyStatusCode(http.StatusAccepted), + HaveRestyHeaderWithValue("Location", ContainSubstring("/v3/jobs/managed_service_instance.delete~")), + )) + + jobURL := httpResp.Header().Get("Location") + Eventually(func(g Gomega) { + jobResp, err := adminClient.R().Get(jobURL) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(string(jobResp.Body())).To(ContainSubstring("COMPLETE")) + }).Should(Succeed()) + }) }) }) @@ -199,7 +237,7 @@ var _ = Describe("Service Instances", func() { Expect(httpResp).To(HaveRestyStatusCode(http.StatusOK)) Expect(serviceInstancesList.Resources).To(ContainElements( MatchFields(IgnoreExtras, Fields{ - "GUID": Equal(existingInstanceGUID), + "GUID": Equal(upsiGUID), }), MatchFields(IgnoreExtras, Fields{ "GUID": Equal(anotherInstanceGUID), @@ -208,3 +246,52 @@ var _ = Describe("Service Instances", func() { }) }) }) + +func createManagedServiceInstance(brokerGUID, spaceGUID string) string { + GinkgoHelper() + + var plansResp resourceList[resource] + catalogResp, err := adminClient.R().SetResult(&plansResp).Get("/v3/service_plans?service_broker_guids=" + brokerGUID) + Expect(err).NotTo(HaveOccurred()) + Expect(catalogResp).To(HaveRestyStatusCode(http.StatusOK)) + Expect(plansResp.Resources).NotTo(BeEmpty()) + + createPayload := serviceInstanceResource{ + resource: resource{ + Name: uuid.NewString(), + Relationships: relationships{ + "space": { + Data: resource{ + GUID: spaceGUID, + }, + }, + "service_plan": { + Data: resource{ + GUID: plansResp.Resources[0].GUID, + }, + }, + }, + }, + InstanceType: "managed", + } + + var result serviceInstanceResource + httpResp, httpError := adminClient.R(). + SetBody(createPayload). + SetResult(&result). + Post("/v3/service_instances") + Expect(httpError).NotTo(HaveOccurred()) + Expect(httpResp).To(SatisfyAll( + HaveRestyStatusCode(http.StatusAccepted), + HaveRestyHeaderWithValue("Location", ContainSubstring("/v3/jobs/managed_service_instance.create~")), + )) + + jobURL := httpResp.Header().Get("Location") + Eventually(func(g Gomega) { + jobResp, err := adminClient.R().Get(jobURL) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(string(jobResp.Body())).To(ContainSubstring("COMPLETE")) + }).Should(Succeed()) + + return strings.Split(jobURL, "~")[1] +} diff --git a/tests/smoke/services_test.go b/tests/smoke/services_test.go index 67c6de525..366375635 100644 --- a/tests/smoke/services_test.go +++ b/tests/smoke/services_test.go @@ -35,6 +35,21 @@ var _ = Describe("Services", func() { }) }) + Describe("cf delete-service", func() { + var serviceName string + + BeforeEach(func() { + serviceName = uuid.NewString() + session := helpers.Cf("create-service", "sample-service", "sample", serviceName, "-b", brokerName) + Expect(session).To(Exit(0)) + }) + + It("deletes the managed service", func() { + session := helpers.Cf("delete-service", "-f", serviceName) + Expect(session).To(Exit(0)) + }) + }) + Describe("cf services", func() { var serviceName string