From 56e6701b570adb64d434df52d4da29fd38a35e76 Mon Sep 17 00:00:00 2001 From: Yusmen Zabanov Date: Fri, 1 Nov 2024 16:14:19 +0000 Subject: [PATCH] Implement unbind from managed service instances fixes #3296 Co-authored-by: Danail Branekov --- api/handlers/job.go | 1 + api/handlers/service_binding.go | 52 ++--- api/handlers/service_binding_test.go | 78 ++++++- api/main.go | 1 + api/presenter/job.go | 24 +- .../service_binding_repository.go | 10 + .../service_binding_repository_test.go | 144 ++++++++---- .../api/v1alpha1/cfservicebinding_types.go | 14 +- .../services/bindings/controller.go | 11 +- .../services/bindings/controller_test.go | 215 +++++++++++++++++ .../services/bindings/managed/controller.go | 221 ++++++++++++------ .../services/bindings/upsi/controller.go | 21 +- .../controllers/services/osbapi/assets.go | 57 +++++ .../controllers/services/osbapi/client.go | 51 ++++ .../services/osbapi/client_test.go | 177 ++++++++++---- .../services/osbapi/clientfactory.go | 1 + .../services/osbapi/fake/broker_client.go | 81 +++++++ .../controllers/services/osbapi/types.go | 19 ++ controllers/webhooks/finalizer/suite_test.go | 4 + controllers/webhooks/finalizer/webhook.go | 3 +- .../webhooks/finalizer/webhook_test.go | 9 + ...fi.cloudfoundry.org_cfservicebindings.yaml | 7 + helm/korifi/controllers/manifests.yaml | 1 + helm/korifi/controllers/role.yaml | 1 + tests/assets/sample-broker-golang/main.go | 11 + tests/e2e/apps_test.go | 17 +- tests/e2e/domains_test.go | 9 +- tests/e2e/e2e_suite_test.go | 100 +++++++- tests/e2e/orgs_test.go | 16 +- tests/e2e/roles_test.go | 8 +- tests/e2e/routes_test.go | 8 +- tests/e2e/service_bindings_test.go | 53 +++-- tests/e2e/service_brokers_test.go | 25 +- tests/e2e/service_instances_test.go | 23 +- tests/e2e/spaces_test.go | 41 +--- tests/helpers/broker/broker.go | 4 +- tests/smoke/unbind_service_test.go | 59 +++++ 37 files changed, 1209 insertions(+), 368 deletions(-) create mode 100644 tests/smoke/unbind_service_test.go diff --git a/api/handlers/job.go b/api/handlers/job.go index 0c2c2c9d0..2d4a94523 100644 --- a/api/handlers/job.go +++ b/api/handlers/job.go @@ -31,6 +31,7 @@ const ( ManagedServiceInstanceDeleteJobType = "managed_service_instance.delete" ManagedServiceInstanceCreateJobType = "managed_service_instance.create" ManagedServiceBindingCreateJobType = "managed_service_binding.create" + ManagedServiceBindingDeleteJobType = "managed_service_binding.delete" JobTimeoutDuration = 120.0 ) diff --git a/api/handlers/service_binding.go b/api/handlers/service_binding.go index 7b648ca78..8f61bec28 100644 --- a/api/handlers/service_binding.go +++ b/api/handlers/service_binding.go @@ -78,40 +78,17 @@ func (h *ServiceBinding) create(r *http.Request) (*routing.Response, error) { ctx := logr.NewContext(r.Context(), logger.WithValues("app", app.GUID, "service-instance", serviceInstance.GUID)) - if serviceInstance.Type == korifiv1alpha1.ManagedType { - return h.createManagedServiceBinding(ctx, authInfo, payload, app) - } - - return h.createUserProvidedServiceBinding(ctx, authInfo, payload, app) -} - -func (h *ServiceBinding) createUserProvidedServiceBinding( - ctx context.Context, - authInfo authorization.Info, - payload payloads.ServiceBindingCreate, - app repositories.AppRecord, -) (*routing.Response, error) { serviceBinding, err := h.serviceBindingRepo.CreateServiceBinding(ctx, authInfo, payload.ToMessage(app.SpaceGUID)) if err != nil { return nil, apierrors.LogAndReturn(logr.FromContextOrDiscard(ctx), err, "failed to create ServiceBinding") } - return routing.NewResponse(http.StatusCreated).WithBody(presenter.ForServiceBinding(serviceBinding, h.serverURL)), nil -} - -func (h *ServiceBinding) createManagedServiceBinding( - ctx context.Context, - authInfo authorization.Info, - payload payloads.ServiceBindingCreate, - app repositories.AppRecord, -) (*routing.Response, error) { - serviceBinding, err := h.serviceBindingRepo.CreateServiceBinding(ctx, authInfo, payload.ToMessage(app.SpaceGUID)) - if err != nil { - return nil, apierrors.LogAndReturn(logr.FromContextOrDiscard(ctx), err, "failed to create ServiceBinding") + if serviceInstance.Type == korifiv1alpha1.ManagedType { + return routing.NewResponse(http.StatusAccepted). + WithHeader("Location", presenter.JobURLForRedirects(serviceBinding.GUID, presenter.ManagedServiceBindingCreateOperation, h.serverURL)), nil } - return routing.NewResponse(http.StatusAccepted). - WithHeader("Location", presenter.JobURLForRedirects(serviceBinding.GUID, presenter.ManagedServiceBindingCreateOperation, h.serverURL)), nil + return routing.NewResponse(http.StatusCreated).WithBody(presenter.ForServiceBinding(serviceBinding, h.serverURL)), nil } func (h *ServiceBinding) delete(r *http.Request) (*routing.Response, error) { @@ -119,12 +96,31 @@ func (h *ServiceBinding) delete(r *http.Request) (*routing.Response, error) { logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.service-binding.delete") serviceBindingGUID := routing.URLParam(r, "guid") + serviceBinding, err := h.serviceBindingRepo.GetServiceBinding(r.Context(), authInfo, serviceBindingGUID) + if err != nil { + return nil, apierrors.LogAndReturn(logger, apierrors.ForbiddenAsNotFound(err), "failed to get "+repositories.ServiceBindingResourceType) + } + + serviceInstance, err := h.serviceInstanceRepo.GetServiceInstance(r.Context(), authInfo, serviceBinding.ServiceInstanceGUID) + if err != nil { + return nil, apierrors.LogAndReturn( + logger, + apierrors.NewUnprocessableEntityError(err, "failed to get service instance"), + "failed to get "+repositories.ServiceInstanceResourceType, + "instance-guid", serviceBinding.ServiceInstanceGUID, + ) + } - err := h.serviceBindingRepo.DeleteServiceBinding(r.Context(), authInfo, serviceBindingGUID) + err = h.serviceBindingRepo.DeleteServiceBinding(r.Context(), authInfo, serviceBindingGUID) if err != nil { return nil, apierrors.LogAndReturn(logger, err, "error when deleting service binding", "guid", serviceBindingGUID) } + if serviceInstance.Type == korifiv1alpha1.ManagedType { + return routing.NewResponse(http.StatusAccepted). + WithHeader("Location", presenter.JobURLForRedirects(serviceBinding.GUID, presenter.ManagedServiceBindingDeleteOperation, h.serverURL)), nil + } + return routing.NewResponse(http.StatusNoContent), nil } diff --git a/api/handlers/service_binding_test.go b/api/handlers/service_binding_test.go index c241db8c7..72bc4df8a 100644 --- a/api/handlers/service_binding_test.go +++ b/api/handlers/service_binding_test.go @@ -33,7 +33,8 @@ var _ = Describe("ServiceBinding", func() { BeforeEach(func() { serviceBindingRepo = new(fake.CFServiceBindingRepository) serviceBindingRepo.GetServiceBindingReturns(repositories.ServiceBindingRecord{ - GUID: "service-binding-guid", + GUID: "service-binding-guid", + ServiceInstanceGUID: "service-instance-guid", }, nil) appRepo = new(fake.CFAppRepository) @@ -46,6 +47,7 @@ var _ = Describe("ServiceBinding", func() { serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{ GUID: "service-instance-guid", SpaceGUID: "space-guid", + Type: korifiv1alpha1.UserProvidedType, }, nil) requestValidator = new(fake.RequestValidator) @@ -376,6 +378,50 @@ var _ = Describe("ServiceBinding", func() { requestPath = "/v3/service_credential_bindings/service-binding-guid" }) + It("gets the service binding", func() { + Expect(serviceBindingRepo.GetServiceBindingCallCount()).To(Equal(1)) + _, actualAuthInfo, actualBindingGUID := serviceBindingRepo.GetServiceBindingArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualBindingGUID).To(Equal("service-binding-guid")) + }) + + When("getting the service binding is forbidden", func() { + BeforeEach(func() { + serviceBindingRepo.GetServiceBindingReturns(repositories.ServiceBindingRecord{}, apierrors.NewForbiddenError(nil, repositories.ServiceBindingResourceType)) + }) + + It("returns a not found error", func() { + expectNotFoundError(repositories.ServiceBindingResourceType) + }) + }) + + When("getting the service binding fails", func() { + BeforeEach(func() { + serviceBindingRepo.GetServiceBindingReturns(repositories.ServiceBindingRecord{}, errors.New("getting-binding-failed")) + }) + + It("returns unknown error", func() { + expectUnknownError() + }) + }) + + It("gets the service instance", func() { + Expect(serviceInstanceRepo.GetServiceInstanceCallCount()).To(Equal(1)) + _, actualAuthInfo, actualInstanceGUID := serviceInstanceRepo.GetServiceInstanceArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualInstanceGUID).To(Equal("service-instance-guid")) + }) + + When("getting the service instance fails", func() { + BeforeEach(func() { + serviceInstanceRepo.GetServiceInstanceReturns(repositories.ServiceInstanceRecord{}, errors.New("getting-instance-failed")) + }) + + It("returns error", func() { + expectUnprocessableEntityError("failed to get service instance") + }) + }) + It("deletes the service binding", func() { Expect(rr).To(HaveHTTPStatus(http.StatusNoContent)) Expect(rr).To(HaveHTTPBody(BeEmpty())) @@ -384,6 +430,36 @@ var _ = Describe("ServiceBinding", func() { _, _, guid := serviceBindingRepo.DeleteServiceBindingArgsForCall(0) Expect(guid).To(Equal("service-binding-guid")) }) + + 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 binding in a job", func() { + Expect(serviceBindingRepo.DeleteServiceBindingCallCount()).To(Equal(1)) + _, _, guid := serviceBindingRepo.DeleteServiceBindingArgsForCall(0) + Expect(guid).To(Equal("service-binding-guid")) + + Expect(rr).To(HaveHTTPStatus(http.StatusAccepted)) + Expect(rr).To(HaveHTTPHeaderWithValue("Location", + ContainSubstring("/v3/jobs/managed_service_binding.delete~service-binding-guid"))) + }) + }) + + When("deleting the service binding fails", func() { + BeforeEach(func() { + serviceBindingRepo.DeleteServiceBindingReturns(errors.New("delete-binding-failed")) + }) + + It("returns unknown error", func() { + expectUnknownError() + }) + }) }) Describe("PATCH /v3/service_credential_bindings/:guid", func() { diff --git a/api/main.go b/api/main.go index e1d15e272..fecc77c40 100644 --- a/api/main.go +++ b/api/main.go @@ -369,6 +369,7 @@ func main() { handlers.RoleDeleteJobType: roleRepo, handlers.ServiceBrokerDeleteJobType: serviceBrokerRepo, handlers.ManagedServiceInstanceDeleteJobType: serviceInstanceRepo, + handlers.ManagedServiceBindingDeleteJobType: serviceBindingRepo, }, map[string]handlers.StateRepository{ handlers.ServiceBrokerCreateJobType: serviceBrokerRepo, diff --git a/api/presenter/job.go b/api/presenter/job.go index dcf709f3c..bddbc77d8 100644 --- a/api/presenter/job.go +++ b/api/presenter/job.go @@ -16,19 +16,21 @@ const ( StateFailed = "FAILED" StateProcessing = "PROCESSING" - AppDeleteOperation = "app.delete" - OrgDeleteOperation = "org.delete" - RouteDeleteOperation = "route.delete" - SpaceApplyManifestOperation = "space.apply_manifest" - SpaceDeleteOperation = "space.delete" - DomainDeleteOperation = "domain.delete" - RoleDeleteOperation = "role.delete" - ServiceBrokerCreateOperation = "service_broker.create" - ServiceBrokerDeleteOperation = "service_broker.delete" - ServiceBrokerUpdateOperation = "service_broker.update" + AppDeleteOperation = "app.delete" + OrgDeleteOperation = "org.delete" + RouteDeleteOperation = "route.delete" + SpaceApplyManifestOperation = "space.apply_manifest" + SpaceDeleteOperation = "space.delete" + DomainDeleteOperation = "domain.delete" + RoleDeleteOperation = "role.delete" + ServiceBrokerCreateOperation = "service_broker.create" + ServiceBrokerDeleteOperation = "service_broker.delete" + ServiceBrokerUpdateOperation = "service_broker.update" + ManagedServiceInstanceCreateOperation = "managed_service_instance.create" - ManagedServiceBindingCreateOperation = "managed_service_binding.create" ManagedServiceInstanceDeleteOperation = "managed_service_instance.delete" + ManagedServiceBindingCreateOperation = "managed_service_binding.create" + ManagedServiceBindingDeleteOperation = "managed_service_binding.delete" ) var ( diff --git a/api/repositories/service_binding_repository.go b/api/repositories/service_binding_repository.go index 9439a345a..bdf516ab4 100644 --- a/api/repositories/service_binding_repository.go +++ b/api/repositories/service_binding_repository.go @@ -66,6 +66,7 @@ type ServiceBindingRecord struct { Annotations map[string]string CreatedAt time.Time UpdatedAt *time.Time + DeletedAt *time.Time LastOperation ServiceBindingLastOperation Ready bool } @@ -238,6 +239,7 @@ func serviceBindingToRecord(binding korifiv1alpha1.CFServiceBinding) ServiceBind Annotations: binding.Annotations, CreatedAt: binding.CreationTimestamp.Time, UpdatedAt: getLastUpdatedTime(&binding), + DeletedAt: golangTime(binding.DeletionTimestamp), LastOperation: serviceBindingRecordLastOperation(binding), Ready: isBindingReady(binding), } @@ -343,6 +345,14 @@ func (r *ServiceBindingRepo) GetState(ctx context.Context, authInfo authorizatio return model.CFResourceStateUnknown, nil } +func (r *ServiceBindingRepo) GetDeletedAt(ctx context.Context, authInfo authorization.Info, bindingGUID string) (*time.Time, error) { + serviceBinding, err := r.GetServiceBinding(ctx, authInfo, bindingGUID) + if err != nil { + return nil, err + } + return serviceBinding.DeletedAt, nil +} + // nolint:dupl func (r *ServiceBindingRepo) ListServiceBindings(ctx context.Context, authInfo authorization.Info, message ListServiceBindingsMessage) ([]ServiceBindingRecord, error) { userClient, err := r.userClientFactory.BuildClient(authInfo) diff --git a/api/repositories/service_binding_repository_test.go b/api/repositories/service_binding_repository_test.go index 5a86825a7..4fb23ffcd 100644 --- a/api/repositories/service_binding_repository_test.go +++ b/api/repositories/service_binding_repository_test.go @@ -27,10 +27,9 @@ import ( var _ = Describe("ServiceBindingRepo", func() { var ( - repo *repositories.ServiceBindingRepo - testCtx context.Context - org *korifiv1alpha1.CFOrg - space *korifiv1alpha1.CFSpace + repo *repositories.ServiceBindingRepo + org *korifiv1alpha1.CFOrg + space *korifiv1alpha1.CFSpace appGUID string bindingName *string @@ -43,7 +42,6 @@ var _ = Describe("ServiceBindingRepo", func() { ) BeforeEach(func() { - testCtx = context.Background() conditionAwaiter = &fakeawaiter.FakeAwaiter[ *korifiv1alpha1.CFServiceBinding, korifiv1alpha1.CFServiceBinding, @@ -52,8 +50,8 @@ var _ = Describe("ServiceBindingRepo", func() { ]{} repo = repositories.NewServiceBindingRepo(namespaceRetriever, userClientFactory, nsPerms, conditionAwaiter) - org = createOrgWithCleanup(testCtx, prefixedGUID("org")) - space = createSpaceWithCleanup(testCtx, org.Name, prefixedGUID("space1")) + org = createOrgWithCleanup(ctx, prefixedGUID("org")) + space = createSpaceWithCleanup(ctx, org.Name, prefixedGUID("space1")) appGUID = prefixedGUID("app") bindingName = nil @@ -75,7 +73,7 @@ var _ = Describe("ServiceBindingRepo", func() { }, } Expect( - k8sClient.Create(testCtx, app), + k8sClient.Create(ctx, app), ).To(Succeed()) }) @@ -103,7 +101,7 @@ var _ = Describe("ServiceBindingRepo", func() { }, } Expect( - k8sClient.Create(testCtx, cfServiceBinding), + k8sClient.Create(ctx, cfServiceBinding), ).To(Succeed()) }) @@ -159,6 +157,60 @@ var _ = Describe("ServiceBindingRepo", func() { }) }) + Describe("GetDeletedAt", func() { + var ( + cfServiceBinding *korifiv1alpha1.CFServiceBinding + deletionTime *time.Time + getErr error + ) + + BeforeEach(func() { + cfServiceBinding = &korifiv1alpha1.CFServiceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: uuid.NewString(), + Namespace: space.Name, + }, + } + + Expect(k8sClient.Create(ctx, cfServiceBinding)).To(Succeed()) + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) + }) + + JustBeforeEach(func() { + deletionTime, getErr = repo.GetDeletedAt(ctx, authInfo, cfServiceBinding.Name) + }) + + It("returns nil", func() { + Expect(getErr).NotTo(HaveOccurred()) + Expect(deletionTime).To(BeNil()) + }) + + When("the binding is being deleted", func() { + BeforeEach(func() { + Expect(k8s.PatchResource(ctx, k8sClient, cfServiceBinding, func() { + cfServiceBinding.Finalizers = append(cfServiceBinding.Finalizers, "foo") + })).To(Succeed()) + + Expect(k8sClient.Delete(ctx, cfServiceBinding)).To(Succeed()) + }) + + It("returns the deletion time", func() { + Expect(getErr).NotTo(HaveOccurred()) + Expect(deletionTime).To(PointTo(BeTemporally("~", time.Now(), time.Minute))) + }) + }) + + When("the binding isn't found", func() { + BeforeEach(func() { + Expect(k8sClient.Delete(ctx, cfServiceBinding)).To(Succeed()) + }) + + It("errors", func() { + Expect(getErr).To(matchers.WrapErrorAssignableToTypeOf(apierrors.NotFoundError{})) + }) + }) + }) + Describe("CreateUserProvidedServiceBinding", func() { var ( cfServiceInstance *korifiv1alpha1.CFServiceInstance @@ -177,7 +229,7 @@ var _ = Describe("ServiceBindingRepo", func() { }, } Expect( - k8sClient.Create(testCtx, cfServiceInstance), + k8sClient.Create(ctx, cfServiceInstance), ).To(Succeed()) conditionAwaiter.AwaitConditionStub = func(ctx context.Context, _ client.WithWatch, object client.Object, _ string) (*korifiv1alpha1.CFServiceBinding, error) { @@ -201,7 +253,7 @@ var _ = Describe("ServiceBindingRepo", func() { }) JustBeforeEach(func() { - serviceBindingRecord, createErr = repo.CreateServiceBinding(testCtx, authInfo, repositories.CreateServiceBindingMessage{ + serviceBindingRecord, createErr = repo.CreateServiceBinding(ctx, authInfo, repositories.CreateServiceBindingMessage{ Name: bindingName, ServiceInstanceGUID: cfServiceInstance.Name, AppGUID: appGUID, @@ -215,7 +267,7 @@ var _ = Describe("ServiceBindingRepo", func() { When("the user can create CFServiceBindings in the Space", func() { BeforeEach(func() { - createRoleBinding(testCtx, userName, spaceDeveloperRole.Name, space.Name) + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) }) It("creates a new CFServiceBinding resource and returns a record", func() { @@ -237,7 +289,7 @@ var _ = Describe("ServiceBindingRepo", func() { serviceBinding := new(korifiv1alpha1.CFServiceBinding) Expect( - k8sClient.Get(testCtx, types.NamespacedName{Name: serviceBindingRecord.GUID, Namespace: space.Name}, serviceBinding), + k8sClient.Get(ctx, types.NamespacedName{Name: serviceBindingRecord.GUID, Namespace: space.Name}, serviceBinding), ).To(Succeed()) Expect(serviceBinding.Labels).To(HaveKeyWithValue("servicebinding.io/provisioned-service", "true")) @@ -309,7 +361,7 @@ var _ = Describe("ServiceBindingRepo", func() { ) BeforeEach(func() { - createRoleBinding(testCtx, userName, spaceDeveloperRole.Name, space.Name) + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) cfServiceBinding = &korifiv1alpha1.CFServiceBinding{ ObjectMeta: metav1.ObjectMeta{ Name: uuid.NewString(), @@ -327,13 +379,13 @@ var _ = Describe("ServiceBindingRepo", func() { }, } Expect( - k8sClient.Create(testCtx, cfServiceBinding), + k8sClient.Create(ctx, cfServiceBinding), ).To(Succeed()) }) JustBeforeEach(func() { var err error - serviceBindingRecord, err = repo.GetServiceBinding(testCtx, authInfo, cfServiceBinding.Name) + serviceBindingRecord, err = repo.GetServiceBinding(ctx, authInfo, cfServiceBinding.Name) Expect(err).NotTo(HaveOccurred()) }) @@ -444,14 +496,14 @@ var _ = Describe("ServiceBindingRepo", func() { }, } Expect( - k8sClient.Create(testCtx, cfServiceInstance), + k8sClient.Create(ctx, cfServiceInstance), ).To(Succeed()) bindingName = nil }) JustBeforeEach(func() { - serviceBindingRecord, createErr = repo.CreateServiceBinding(testCtx, authInfo, repositories.CreateServiceBindingMessage{ + serviceBindingRecord, createErr = repo.CreateServiceBinding(ctx, authInfo, repositories.CreateServiceBindingMessage{ Name: bindingName, ServiceInstanceGUID: cfServiceInstance.Name, AppGUID: appGUID, @@ -465,7 +517,7 @@ var _ = Describe("ServiceBindingRepo", func() { When("the user can create CFServiceBindings in the Space", func() { BeforeEach(func() { - createRoleBinding(testCtx, userName, spaceDeveloperRole.Name, space.Name) + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) }) It("creates a new CFServiceBinding resource and returns a record", func() { @@ -487,7 +539,7 @@ var _ = Describe("ServiceBindingRepo", func() { serviceBinding := new(korifiv1alpha1.CFServiceBinding) Expect( - k8sClient.Get(testCtx, types.NamespacedName{Name: serviceBindingRecord.GUID, Namespace: space.Name}, serviceBinding), + k8sClient.Get(ctx, types.NamespacedName{Name: serviceBindingRecord.GUID, Namespace: space.Name}, serviceBinding), ).To(Succeed()) Expect(serviceBinding.Labels).To(HaveKeyWithValue("servicebinding.io/provisioned-service", "true")) @@ -531,17 +583,21 @@ var _ = Describe("ServiceBindingRepo", func() { Describe("DeleteServiceBinding", func() { var ( - ret error + deleteErr error serviceBindingGUID string ) BeforeEach(func() { - serviceBindingGUID = prefixedGUID("binding") + serviceBindingGUID = uuid.NewString() serviceBinding := &korifiv1alpha1.CFServiceBinding{ ObjectMeta: metav1.ObjectMeta{ Name: serviceBindingGUID, Namespace: space.Name, + Annotations: map[string]string{ + korifiv1alpha1.ServiceInstanceTypeAnnotationKey: korifiv1alpha1.UserProvidedType, + }, + Finalizers: []string{korifiv1alpha1.CFServiceBindingFinalizerName}, }, Spec: korifiv1alpha1.CFServiceBindingSpec{ Service: corev1.ObjectReference{ @@ -555,35 +611,35 @@ var _ = Describe("ServiceBindingRepo", func() { }, } Expect( - k8sClient.Create(testCtx, serviceBinding), + k8sClient.Create(ctx, serviceBinding), ).To(Succeed()) }) JustBeforeEach(func() { - ret = repo.DeleteServiceBinding(testCtx, authInfo, serviceBindingGUID) + deleteErr = repo.DeleteServiceBinding(ctx, authInfo, serviceBindingGUID) }) It("returns a not-found error for users with no role in the space", func() { - Expect(ret).To(matchers.WrapErrorAssignableToTypeOf(apierrors.NotFoundError{})) + Expect(deleteErr).To(matchers.WrapErrorAssignableToTypeOf(apierrors.NotFoundError{})) }) When("the user is a space manager", func() { BeforeEach(func() { - createRoleBinding(testCtx, userName, spaceManagerRole.Name, space.Name) + createRoleBinding(ctx, userName, spaceManagerRole.Name, space.Name) }) It("returns a forbidden error", func() { - Expect(ret).To(matchers.WrapErrorAssignableToTypeOf(apierrors.ForbiddenError{})) + Expect(deleteErr).To(matchers.WrapErrorAssignableToTypeOf(apierrors.ForbiddenError{})) }) }) When("the user is a space developer", func() { BeforeEach(func() { - createRoleBinding(testCtx, userName, spaceDeveloperRole.Name, space.Name) + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) }) It("deletes the binding", func() { - Expect(ret).NotTo(HaveOccurred()) + Expect(deleteErr).NotTo(HaveOccurred()) }) When("the binding doesn't exist", func() { @@ -592,7 +648,7 @@ var _ = Describe("ServiceBindingRepo", func() { }) It("returns a not-found error", func() { - Expect(ret).To(BeAssignableToTypeOf(apierrors.NotFoundError{})) + Expect(deleteErr).To(BeAssignableToTypeOf(apierrors.NotFoundError{})) }) }) }) @@ -611,9 +667,9 @@ var _ = Describe("ServiceBindingRepo", func() { ) BeforeEach(func() { - cfApp1 = createAppCR(testCtx, k8sClient, "app-1-name", prefixedGUID("app-1"), space.Name, "STOPPED") + cfApp1 = createAppCR(ctx, k8sClient, "app-1-name", prefixedGUID("app-1"), space.Name, "STOPPED") serviceInstance1GUID = prefixedGUID("instance-1") - cfServiceInstance1 := createServiceInstanceCR(testCtx, k8sClient, serviceInstance1GUID, space.Name, "service-instance-1-name", "secret-1-name") + cfServiceInstance1 := createServiceInstanceCR(ctx, k8sClient, serviceInstance1GUID, space.Name, "service-instance-1-name", "secret-1-name") serviceBinding1 = &korifiv1alpha1.CFServiceBinding{ ObjectMeta: metav1.ObjectMeta{ Name: prefixedGUID("binding-1"), @@ -635,10 +691,10 @@ var _ = Describe("ServiceBindingRepo", func() { } Expect(k8sClient.Create(ctx, serviceBinding1)).To(Succeed()) - space2 = createSpaceWithCleanup(testCtx, org.Name, prefixedGUID("space-2")) - cfApp2 = createAppCR(testCtx, k8sClient, "app-2-name", prefixedGUID("app-2"), space2.Name, "STOPPED") + space2 = createSpaceWithCleanup(ctx, org.Name, prefixedGUID("space-2")) + cfApp2 = createAppCR(ctx, k8sClient, "app-2-name", prefixedGUID("app-2"), space2.Name, "STOPPED") serviceInstance2GUID = prefixedGUID("instance-2") - cfServiceInstance2 := createServiceInstanceCR(testCtx, k8sClient, serviceInstance2GUID, space2.Name, "service-instance-2-name", "secret-2-name") + cfServiceInstance2 := createServiceInstanceCR(ctx, k8sClient, serviceInstance2GUID, space2.Name, "service-instance-2-name", "secret-2-name") serviceBinding2 = &korifiv1alpha1.CFServiceBinding{ ObjectMeta: metav1.ObjectMeta{ Name: prefixedGUID("binding-2"), @@ -660,9 +716,9 @@ var _ = Describe("ServiceBindingRepo", func() { } Expect(k8sClient.Create(ctx, serviceBinding2)).To(Succeed()) - cfApp3 = createAppCR(testCtx, k8sClient, "app-3-name", prefixedGUID("app-3"), space2.Name, "STOPPED") + cfApp3 = createAppCR(ctx, k8sClient, "app-3-name", prefixedGUID("app-3"), space2.Name, "STOPPED") serviceInstance3GUID = prefixedGUID("instance-3") - cfServiceInstance3 := createServiceInstanceCR(testCtx, k8sClient, serviceInstance3GUID, space2.Name, "service-instance-3-name", "secret-3-name") + cfServiceInstance3 := createServiceInstanceCR(ctx, k8sClient, serviceInstance3GUID, space2.Name, "service-instance-3-name", "secret-3-name") serviceBinding3 = &korifiv1alpha1.CFServiceBinding{ ObjectMeta: metav1.ObjectMeta{ Name: prefixedGUID("binding-3"), @@ -688,13 +744,13 @@ var _ = Describe("ServiceBindingRepo", func() { }) JustBeforeEach(func() { - responseServiceBindings, listErr = repo.ListServiceBindings(context.Background(), authInfo, requestMessage) + responseServiceBindings, listErr = repo.ListServiceBindings(ctx, authInfo, requestMessage) }) When("the user has access to both namespaces", func() { BeforeEach(func() { - createRoleBinding(testCtx, userName, spaceDeveloperRole.Name, space.Name) - createRoleBinding(testCtx, userName, spaceDeveloperRole.Name, space2.Name) + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space2.Name) }) It("succeeds", func() { @@ -788,7 +844,7 @@ var _ = Describe("ServiceBindingRepo", func() { DescribeTable("valid label selectors", func(selector string, serviceBindingGUIDPrefixes ...string) { - serviceBindings, err := repo.ListServiceBindings(context.Background(), authInfo, repositories.ListServiceBindingsMessage{ + serviceBindings, err := repo.ListServiceBindings(ctx, authInfo, repositories.ListServiceBindingsMessage{ LabelSelector: selector, }) Expect(err).NotTo(HaveOccurred()) @@ -890,7 +946,7 @@ var _ = Describe("ServiceBindingRepo", func() { }, } Expect( - k8sClient.Create(testCtx, serviceBinding), + k8sClient.Create(ctx, serviceBinding), ).To(Succeed()) }) @@ -904,7 +960,7 @@ var _ = Describe("ServiceBindingRepo", func() { When("the user is a space developer", func() { BeforeEach(func() { - createRoleBinding(testCtx, userName, spaceDeveloperRole.Name, space.Name) + createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) }) It("fetches the CFServiceBinding we're looking for", func() { @@ -957,7 +1013,7 @@ var _ = Describe("ServiceBindingRepo", func() { }, } Expect( - k8sClient.Create(testCtx, serviceBinding), + k8sClient.Create(ctx, serviceBinding), ).To(Succeed()) updateMessage = repositories.UpdateServiceBindingMessage{ diff --git a/controllers/api/v1alpha1/cfservicebinding_types.go b/controllers/api/v1alpha1/cfservicebinding_types.go index dd0b85d07..af2b8144d 100644 --- a/controllers/api/v1alpha1/cfservicebinding_types.go +++ b/controllers/api/v1alpha1/cfservicebinding_types.go @@ -24,11 +24,14 @@ import ( ) const ( - BindingFailedCondition = "BindingFailed" - BindingRequestedCondition = "BindingRequested" + BindingFailedCondition = "BindingFailed" + BindingRequestedCondition = "BindingRequested" + UnbindingRequestedCondition = "UnbindingRequested" ServiceInstanceTypeAnnotationKey = "korifi.cloudfoundry.org/service-instance-type" PlanGUIDLabelKey = "korifi.cloudfoundry.org/plan-guid" + + CFServiceBindingFinalizerName = "cfServiceBinding.korifi.cloudfoundry.org" ) // CFServiceBindingSpec defines the desired state of CFServiceBinding @@ -59,6 +62,13 @@ type CFServiceBindingStatus struct { // +optional BindingOperation string `json:"bindingOperation"` + // The + // [operation](https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#unbinding) + // of the unbind request to the the OSBAPI broker. Only makes sense for + // bindings to managed service instances + // +optional + UnbindingOperation string `json:"unbindingOperation"` + // A reference to the Secret containing the binding Credentials object. For // bindings to user-provided services this refers to the credentials secret // from the service instance. For managed services the secret contains the diff --git a/controllers/controllers/services/bindings/controller.go b/controllers/controllers/services/bindings/controller.go index 77dd72bd7..d74a9e683 100644 --- a/controllers/controllers/services/bindings/controller.go +++ b/controllers/controllers/services/bindings/controller.go @@ -139,11 +139,15 @@ func (r *Reconciler) appToServiceBindings(ctx context.Context, o client.Object) //+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfservicebindings,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfservicebindings/status,verbs=get;update;patch +//+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfservicebindings/finalizers,verbs=update //+kubebuilder:rbac:groups=servicebinding.io,resources=servicebindings,verbs=get;list;create;update;patch;watch func (r *Reconciler) ReconcileResource(ctx context.Context, cfServiceBinding *korifiv1alpha1.CFServiceBinding) (ctrl.Result, error) { log := logr.FromContextOrDiscard(ctx) + cfServiceBinding.Status.ObservedGeneration = cfServiceBinding.Generation + log.V(1).Info("set observed generation", "generation", cfServiceBinding.Status.ObservedGeneration) + cfServiceInstance := new(korifiv1alpha1.CFServiceInstance) err := r.k8sClient.Get(ctx, types.NamespacedName{Name: cfServiceBinding.Spec.Service.Name, Namespace: cfServiceBinding.Namespace}, cfServiceInstance) if err != nil { @@ -151,9 +155,6 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, cfServiceBinding *ko return ctrl.Result{}, err } - cfServiceBinding.Status.ObservedGeneration = cfServiceBinding.Generation - log.V(1).Info("set observed generation", "generation", cfServiceBinding.Status.ObservedGeneration) - if cfServiceBinding.Annotations == nil { cfServiceBinding.Annotations = map[string]string{} } @@ -167,7 +168,9 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, cfServiceBinding *ko res, err := r.reconcileByType(ctx, cfServiceInstance, cfServiceBinding) if needsRequeue(res, err) { - log.Error(err, "failed to reconcile binding credentials") + if err != nil { + log.Error(err, "failed to reconcile binding credentials") + } return res, err } diff --git a/controllers/controllers/services/bindings/controller_test.go b/controllers/controllers/services/bindings/controller_test.go index 790bd4944..1c63975e3 100644 --- a/controllers/controllers/services/bindings/controller_test.go +++ b/controllers/controllers/services/bindings/controller_test.go @@ -20,6 +20,7 @@ import ( . "github.com/onsi/gomega/gstruct" servicebindingv1beta1 "github.com/servicebinding/runtime/apis/v1beta1" corev1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -64,6 +65,9 @@ var _ = Describe("CFServiceBinding", func() { ObjectMeta: metav1.ObjectMeta{ Name: uuid.NewString(), Namespace: testNamespace, + Finalizers: []string{ + korifiv1alpha1.CFServiceBindingFinalizerName, + }, }, Spec: korifiv1alpha1.CFServiceBindingSpec{ Service: corev1.ObjectReference{ @@ -536,6 +540,19 @@ var _ = Describe("CFServiceBinding", func() { }) }) }) + + When("the binding is deleted", func() { + JustBeforeEach(func() { + Expect(adminClient.Delete(ctx, binding)).To(Succeed()) + }) + + It("is deleted", func() { + Eventually(func(g Gomega) { + err := adminClient.Get(ctx, client.ObjectKeyFromObject(binding), binding) + g.Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + }).Should(Succeed()) + }) + }) }) Describe("managed service instances", func() { @@ -1050,5 +1067,203 @@ var _ = Describe("CFServiceBinding", func() { }).Should(Succeed()) }) }) + + When("the binding is deleted", func() { + BeforeEach(func() { + brokerClient.UnbindReturns(osbapi.UnbindResponse{}, nil) + }) + + JustBeforeEach(func() { + // For deletion test we want to request deletion and verify the behaviour when finalization fails. + // Therefore we use the standard k8s client instead of `adminClient` as it ensures that the object is deleted + Expect(k8sManager.GetClient().Delete(ctx, binding)).To(Succeed()) + }) + + It("deletes the binding", func() { + Eventually(func(g Gomega) { + err := adminClient.Get(ctx, client.ObjectKeyFromObject(binding), binding) + g.Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + }).Should(Succeed()) + }) + + It("calls the unbind endpoint of the osbapi broker", func() { + Eventually(func(g Gomega) { + g.Expect(brokerClient.UnbindCallCount()).NotTo(BeZero()) + _, actualUnbindRequest := brokerClient.UnbindArgsForCall(0) + Expect(actualUnbindRequest).To(Equal(osbapi.UnbindPayload{ + InstanceID: instance.Name, + BindingID: binding.Name, + UnbindRequest: osbapi.UnbindRequest{ + ServiceId: "service-offering-id", + PlanID: "service-plan-id", + }, + })) + }).Should(Succeed()) + }) + + When("broker returns gone error on unbind", func() { + BeforeEach(func() { + brokerClient.UnbindReturns(osbapi.UnbindResponse{}, osbapi.GoneError{}) + }) + + It("deletes the binding", func() { + Eventually(func(g Gomega) { + err := adminClient.Get(ctx, client.ObjectKeyFromObject(binding), binding) + g.Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + }).Should(Succeed()) + }) + }) + + When("the broker responds asynchronously", func() { + BeforeEach(func() { + brokerClient.GetServiceBindingLastOperationReturns(osbapi.LastOperationResponse{ + State: "in progress", + }, nil) + brokerClient.UnbindReturns(osbapi.UnbindResponse{ + Operation: "unbind-id", + }, nil) + }) + + It("sets UnbindingRequested condition", func() { + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(binding), binding)).To(Succeed()) + g.Expect(binding.Status.Conditions).To(ContainElements( + SatisfyAll( + HasType(Equal(korifiv1alpha1.StatusConditionReady)), + HasStatus(Equal(metav1.ConditionFalse)), + ), + SatisfyAll( + HasType(Equal(korifiv1alpha1.UnbindingRequestedCondition)), + HasStatus(Equal(metav1.ConditionTrue)), + ), + )) + g.Expect(binding.Status.UnbindingOperation).To(Equal("unbind-id")) + }).Should(Succeed()) + }) + + When("unbind has been already requested", func() { + BeforeEach(func() { + Expect(k8s.Patch(ctx, adminClient, binding, func() { + meta.SetStatusCondition(&binding.Status.Conditions, metav1.Condition{ + Type: korifiv1alpha1.UnbindingRequestedCondition, + Status: metav1.ConditionTrue, + Reason: "UnbindingRequested", + }) + binding.Status.UnbindingOperation = "unbind-id" + })).To(Succeed()) + }) + + It("does not request unbinding anymore", func() { + Consistently(func(g Gomega) { + g.Expect(brokerClient.UnbindCallCount()).To(BeZero()) + }).Should(Succeed()) + }) + + When("the unbind operation succeeds", func() { + BeforeEach(func() { + brokerClient.GetServiceBindingLastOperationReturns(osbapi.LastOperationResponse{ + State: "succeeded", + }, nil) + }) + + It("deletes the binding", func() { + Eventually(func(g Gomega) { + err := adminClient.Get(ctx, client.ObjectKeyFromObject(binding), binding) + g.Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + }).Should(Succeed()) + }) + }) + + When("the unbind operation is gone", func() { + BeforeEach(func() { + brokerClient.GetServiceBindingLastOperationReturns(osbapi.LastOperationResponse{}, osbapi.GoneError{}) + }) + + It("deletes the binding", func() { + Eventually(func(g Gomega) { + err := adminClient.Get(ctx, client.ObjectKeyFromObject(binding), binding) + g.Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + }).Should(Succeed()) + }) + }) + + When("the unbind operation fails", func() { + BeforeEach(func() { + brokerClient.GetServiceBindingLastOperationReturns(osbapi.LastOperationResponse{ + State: "failed", + }, nil) + }) + + It("requests unbind again", func() { + Eventually(func(g Gomega) { + g.Expect(brokerClient.UnbindCallCount()).NotTo(BeZero()) + }).Should(Succeed()) + }) + }) + + When("the last operation is gone", func() { + BeforeEach(func() { + brokerClient.GetServiceBindingLastOperationReturns(osbapi.LastOperationResponse{}, osbapi.GoneError{}) + }) + + It("deletes the binding", func() { + Eventually(func(g Gomega) { + err := adminClient.Get(ctx, client.ObjectKeyFromObject(binding), binding) + g.Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + }).Should(Succeed()) + }) + }) + + When("getting the last operation fails", func() { + BeforeEach(func() { + brokerClient.GetServiceBindingLastOperationReturns(osbapi.LastOperationResponse{}, errors.New("get-lastop-failed")) + }) + + It("keeps trying to get it", func() { + getLastOpCallCount := 0 + Eventually(func(g Gomega) { + getLastOpCallCount = brokerClient.GetServiceBindingLastOperationCallCount() + g.Expect(getLastOpCallCount).NotTo(BeZero()) + }).Should(Succeed()) + + Eventually(func(g Gomega) { + g.Expect(brokerClient.GetServiceBindingLastOperationCallCount()).To(BeNumerically(">", getLastOpCallCount)) + }).Should(Succeed()) + }) + }) + }) + }) + + When("binding is already gone", func() { + BeforeEach(func() { + brokerClient.UnbindReturns(osbapi.UnbindResponse{}, osbapi.GoneError{}) + }) + + It("still deletes the binding", func() { + Eventually(func(g Gomega) { + err := adminClient.Get(ctx, client.ObjectKeyFromObject(binding), binding) + g.Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + }).Should(Succeed()) + }) + }) + + When("broker responds with error", func() { + BeforeEach(func() { + brokerClient.UnbindReturns(osbapi.UnbindResponse{}, errors.New("failed-to-unbind")) + }) + + It("keeps trying to unbind", func() { + unbindCallCount := 0 + Eventually(func(g Gomega) { + unbindCallCount = brokerClient.UnbindCallCount() + g.Expect(unbindCallCount).NotTo(BeZero()) + }).Should(Succeed()) + + Eventually(func(g Gomega) { + g.Expect(brokerClient.UnbindCallCount()).To(BeNumerically(">", unbindCallCount)) + }).Should(Succeed()) + }) + }) + }) }) }) diff --git a/controllers/controllers/services/bindings/managed/controller.go b/controllers/controllers/services/bindings/managed/controller.go index 532762ea3..eab483e75 100644 --- a/controllers/controllers/services/bindings/managed/controller.go +++ b/controllers/controllers/services/bindings/managed/controller.go @@ -2,6 +2,7 @@ package managed import ( "context" + "fmt" "time" "code.cloudfoundry.org/korifi/controllers/controllers/services/osbapi" @@ -21,15 +22,15 @@ import ( ctrl "sigs.k8s.io/controller-runtime" ) -type ManagedCredentialsReconciler struct { +type ManagedBindingsReconciler struct { k8sClient client.Client osbapiClientFactory osbapi.BrokerClientFactory scheme *runtime.Scheme assets *osbapi.Assets } -func NewReconciler(k8sClient client.Client, brokerClientFactory osbapi.BrokerClientFactory, rootNamespace string, scheme *runtime.Scheme) *ManagedCredentialsReconciler { - return &ManagedCredentialsReconciler{ +func NewReconciler(k8sClient client.Client, brokerClientFactory osbapi.BrokerClientFactory, rootNamespace string, scheme *runtime.Scheme) *ManagedBindingsReconciler { + return &ManagedBindingsReconciler{ k8sClient: k8sClient, osbapiClientFactory: brokerClientFactory, scheme: scheme, @@ -37,90 +38,73 @@ func NewReconciler(k8sClient client.Client, brokerClientFactory osbapi.BrokerCli } } -func (r *ManagedCredentialsReconciler) ReconcileResource(ctx context.Context, cfServiceBinding *korifiv1alpha1.CFServiceBinding) (ctrl.Result, error) { - if isReconciled(cfServiceBinding) { - return ctrl.Result{}, nil - } - - if isFailed(cfServiceBinding) { - return ctrl.Result{}, k8s.NewNotReadyError().WithReason("BindingFailed").WithNoRequeue() - } +func (r *ManagedBindingsReconciler) ReconcileResource(ctx context.Context, cfServiceBinding *korifiv1alpha1.CFServiceBinding) (ctrl.Result, error) { + log := logr.FromContextOrDiscard(ctx).WithName("reconcile-managed-service-binding") - credentials, err := r.bind(ctx, cfServiceBinding) + assets, err := r.assets.GetServiceBindingAssets(ctx, cfServiceBinding) if err != nil { + log.Error(err, "failed to get service binding assets") return ctrl.Result{}, err } - err = r.reconcileCredentials(ctx, cfServiceBinding, credentials) + osbapiClient, err := r.osbapiClientFactory.CreateClient(ctx, assets.ServiceBroker) if err != nil { + log.Error(err, "failed to create broker client", "broker", assets.ServiceBroker.Name) return ctrl.Result{}, err } - return ctrl.Result{}, nil -} - -func (r *ManagedCredentialsReconciler) bind(ctx context.Context, cfServiceBinding *korifiv1alpha1.CFServiceBinding) (map[string]any, error) { - log := logr.FromContextOrDiscard(ctx) - - cfServiceInstance := &korifiv1alpha1.CFServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: cfServiceBinding.Namespace, - Name: cfServiceBinding.Spec.Service.Name, - }, - } - err := r.k8sClient.Get(ctx, client.ObjectKeyFromObject(cfServiceInstance), cfServiceInstance) - if err != nil { - log.Error(err, "failed to get service instance", "service-instance", cfServiceInstance.Name) - return nil, err + if !cfServiceBinding.GetDeletionTimestamp().IsZero() { + return r.finalizeCFServiceBinding(ctx, cfServiceBinding, assets, osbapiClient) } - servicePlan, err := r.assets.GetServicePlan(ctx, cfServiceInstance.Spec.PlanGUID) - if err != nil { - log.Error(err, "failed to get service plan", "service-plan", cfServiceInstance.Spec.PlanGUID) - return nil, err + if isReconciled(cfServiceBinding) { + return ctrl.Result{}, nil } - serviceBroker, err := r.assets.GetServiceBroker(ctx, servicePlan.Labels[korifiv1alpha1.RelServiceBrokerGUIDLabel]) - if err != nil { - log.Error(err, "failed to get service broker", "service-broker", servicePlan.Labels[korifiv1alpha1.RelServiceBrokerGUIDLabel]) - return nil, err + if isFailed(cfServiceBinding) { + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("BindingFailed").WithNoRequeue() } - serviceOffering, err := r.assets.GetServiceOffering(ctx, servicePlan.Labels[korifiv1alpha1.RelServiceOfferingGUIDLabel]) + credentials, err := r.bind(ctx, cfServiceBinding, assets, osbapiClient) if err != nil { - log.Error(err, "failed to get service offering", "service-offering", servicePlan.Labels[korifiv1alpha1.RelServiceOfferingGUIDLabel]) - return nil, err + return ctrl.Result{}, err } - osbapiClient, err := r.osbapiClientFactory.CreateClient(ctx, serviceBroker) + err = r.reconcileCredentials(ctx, cfServiceBinding, credentials) if err != nil { - log.Error(err, "failed to create broker client", "broker", serviceBroker.Name) - return nil, err + return ctrl.Result{}, err } + return ctrl.Result{}, nil +} + +func (r *ManagedBindingsReconciler) bind( + ctx context.Context, + cfServiceBinding *korifiv1alpha1.CFServiceBinding, + assets osbapi.ServiceBindingAssets, + osbapiClient osbapi.BrokerClient, +) (map[string]any, error) { if !isBindRequested(cfServiceBinding) { - return r.requestBinding(ctx, cfServiceBinding, cfServiceInstance, serviceOffering, servicePlan, osbapiClient) + return r.requestBind(ctx, cfServiceBinding, assets, osbapiClient) } - return r.pollBinding(ctx, cfServiceBinding, serviceOffering, servicePlan, osbapiClient) + return r.pollBindOperation(ctx, cfServiceBinding, assets, osbapiClient) } -func (r *ManagedCredentialsReconciler) requestBinding( +func (r *ManagedBindingsReconciler) requestBind( ctx context.Context, cfServiceBinding *korifiv1alpha1.CFServiceBinding, - cfServiceInstance *korifiv1alpha1.CFServiceInstance, - serviceOffering *korifiv1alpha1.CFServiceOffering, - servicePlan *korifiv1alpha1.CFServicePlan, + assets osbapi.ServiceBindingAssets, osbapiClient osbapi.BrokerClient, ) (map[string]any, error) { log := logr.FromContextOrDiscard(ctx) bindResponse, err := osbapiClient.Bind(ctx, osbapi.BindPayload{ BindingID: cfServiceBinding.Name, - InstanceID: cfServiceInstance.Name, + InstanceID: assets.ServiceInstance.Name, BindRequest: osbapi.BindRequest{ - ServiceId: serviceOffering.Spec.BrokerCatalog.ID, - PlanID: servicePlan.Spec.BrokerCatalog.ID, + ServiceId: assets.ServiceOffering.Spec.BrokerCatalog.ID, + PlanID: assets.ServicePlan.Spec.BrokerCatalog.ID, AppGUID: cfServiceBinding.Spec.AppRef.Name, BindResource: osbapi.BindResource{ AppGUID: cfServiceBinding.Spec.AppRef.Name, @@ -158,11 +142,10 @@ func (r *ManagedCredentialsReconciler) requestBinding( return nil, k8s.NewNotReadyError().WithReason("BindingInProgress").WithRequeue() } -func (r *ManagedCredentialsReconciler) pollBinding( +func (r *ManagedBindingsReconciler) pollBindOperation( ctx context.Context, cfServiceBinding *korifiv1alpha1.CFServiceBinding, - serviceOffering *korifiv1alpha1.CFServiceOffering, - servicePlan *korifiv1alpha1.CFServicePlan, + assets osbapi.ServiceBindingAssets, osbapiClient osbapi.BrokerClient, ) (map[string]any, error) { log := logr.FromContextOrDiscard(ctx) @@ -171,8 +154,8 @@ func (r *ManagedCredentialsReconciler) pollBinding( InstanceID: cfServiceBinding.Spec.Service.Name, BindingID: cfServiceBinding.Name, GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{ - ServiceId: serviceOffering.Spec.BrokerCatalog.ID, - PlanID: servicePlan.Spec.BrokerCatalog.ID, + ServiceId: assets.ServiceOffering.Spec.BrokerCatalog.ID, + PlanID: assets.ServicePlan.Spec.BrokerCatalog.ID, Operation: cfServiceBinding.Status.BindingOperation, }, }) @@ -201,8 +184,8 @@ func (r *ManagedCredentialsReconciler) pollBinding( binding, err := osbapiClient.GetServiceBinding(ctx, osbapi.GetServiceBindingRequest{ InstanceID: cfServiceBinding.Spec.Service.Name, BindingID: cfServiceBinding.Name, - ServiceId: serviceOffering.Spec.BrokerCatalog.ID, - PlanID: servicePlan.Spec.BrokerCatalog.ID, + ServiceId: assets.ServiceOffering.Spec.BrokerCatalog.ID, + PlanID: assets.ServicePlan.Spec.BrokerCatalog.ID, }) if err != nil { log.Error(err, "failed to get binding") @@ -212,19 +195,7 @@ func (r *ManagedCredentialsReconciler) pollBinding( return binding.Credentials, nil } -func isBindRequested(binding *korifiv1alpha1.CFServiceBinding) bool { - return meta.IsStatusConditionTrue(binding.Status.Conditions, korifiv1alpha1.BindingRequestedCondition) -} - -func isFailed(binding *korifiv1alpha1.CFServiceBinding) bool { - return meta.IsStatusConditionTrue(binding.Status.Conditions, korifiv1alpha1.BindingFailedCondition) -} - -func isReconciled(binding *korifiv1alpha1.CFServiceBinding) bool { - return binding.Status.Credentials.Name != "" && binding.Status.Binding.Name != "" -} - -func (r *ManagedCredentialsReconciler) reconcileCredentials(ctx context.Context, cfServiceBinding *korifiv1alpha1.CFServiceBinding, creds map[string]any) error { +func (r *ManagedBindingsReconciler) reconcileCredentials(ctx context.Context, cfServiceBinding *korifiv1alpha1.CFServiceBinding, creds map[string]any) error { log := logr.FromContextOrDiscard(ctx) credentialsSecret := &corev1.Secret{ @@ -271,3 +242,109 @@ func (r *ManagedCredentialsReconciler) reconcileCredentials(ctx context.Context, return nil } + +func (r *ManagedBindingsReconciler) finalizeCFServiceBinding( + ctx context.Context, + serviceBinding *korifiv1alpha1.CFServiceBinding, + assets osbapi.ServiceBindingAssets, + osbapiClient osbapi.BrokerClient, +) (ctrl.Result, error) { + if !isUnbindRequested(serviceBinding) { + return r.requestUnbind(ctx, serviceBinding, assets, osbapiClient) + } + + return r.pollUnbindOperation(ctx, serviceBinding, assets, osbapiClient) +} + +func (r *ManagedBindingsReconciler) requestUnbind( + ctx context.Context, + serviceBinding *korifiv1alpha1.CFServiceBinding, + assets osbapi.ServiceBindingAssets, + osbapiClient osbapi.BrokerClient, +) (ctrl.Result, error) { + log := logr.FromContextOrDiscard(ctx).WithName("unbind") + + unbindResponse, err := osbapiClient.Unbind(ctx, osbapi.UnbindPayload{ + BindingID: serviceBinding.Name, + InstanceID: assets.ServiceInstance.Name, + UnbindRequest: osbapi.UnbindRequest{ + ServiceId: assets.ServiceOffering.Spec.BrokerCatalog.ID, + PlanID: assets.ServicePlan.Spec.BrokerCatalog.ID, + }, + }) + + if osbapi.IgnoreGone(err) != nil { + return ctrl.Result{}, fmt.Errorf("failed to unbind service instance: %w", err) + } + + if unbindResponse.IsComplete() || osbapi.IsGone(err) { + if controllerutil.RemoveFinalizer(serviceBinding, korifiv1alpha1.CFServiceBindingFinalizerName) { + log.V(1).Info("finalizer removed") + } + return ctrl.Result{}, nil + } + + meta.SetStatusCondition(&serviceBinding.Status.Conditions, metav1.Condition{ + Type: korifiv1alpha1.UnbindingRequestedCondition, + Status: metav1.ConditionTrue, + ObservedGeneration: serviceBinding.Generation, + LastTransitionTime: metav1.Now(), + Reason: "UnbindingRequested", + }) + serviceBinding.Status.UnbindingOperation = unbindResponse.Operation + + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("UnbindingRequested").WithRequeue() +} + +func (r *ManagedBindingsReconciler) pollUnbindOperation( + ctx context.Context, + serviceBinding *korifiv1alpha1.CFServiceBinding, + assets osbapi.ServiceBindingAssets, + osbapiClient osbapi.BrokerClient, +) (ctrl.Result, error) { + log := logr.FromContextOrDiscard(ctx).WithName("pollUnbindOperation") + + lastOpResp, err := osbapiClient.GetServiceBindingLastOperation(ctx, osbapi.GetServiceBindingLastOperationRequest{ + InstanceID: assets.ServiceInstance.Name, + BindingID: serviceBinding.Name, + GetLastOperationRequestParameters: osbapi.GetLastOperationRequestParameters{ + ServiceId: assets.ServiceOffering.Spec.BrokerCatalog.ID, + PlanID: assets.ServicePlan.Spec.BrokerCatalog.ID, + Operation: serviceBinding.Status.UnbindingOperation, + }, + }) + if osbapi.IgnoreGone(err) != nil { + return ctrl.Result{}, err + } + + if osbapi.IsGone(err) || lastOpResp.State == "succeeded" { + if controllerutil.RemoveFinalizer(serviceBinding, korifiv1alpha1.CFServiceBindingFinalizerName) { + log.V(1).Info("finalizer removed") + } + return ctrl.Result{}, nil + } + + if lastOpResp.State == "failed" { + meta.RemoveStatusCondition(&serviceBinding.Status.Conditions, korifiv1alpha1.UnbindingRequestedCondition) + serviceBinding.Status.UnbindingOperation = "" + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("UnbindingFailed").WithRequeue() + } + + return ctrl.Result{}, k8s.NewNotReadyError().WithReason("UnbindingInProgress").WithRequeue() +} + +func isBindRequested(binding *korifiv1alpha1.CFServiceBinding) bool { + return meta.IsStatusConditionTrue(binding.Status.Conditions, korifiv1alpha1.BindingRequestedCondition) +} + +func isUnbindRequested(binding *korifiv1alpha1.CFServiceBinding) bool { + return meta.IsStatusConditionTrue(binding.Status.Conditions, korifiv1alpha1.UnbindingRequestedCondition) +} + +func isFailed(binding *korifiv1alpha1.CFServiceBinding) bool { + return meta.IsStatusConditionTrue(binding.Status.Conditions, korifiv1alpha1.BindingFailedCondition) +} + +func isReconciled(binding *korifiv1alpha1.CFServiceBinding) bool { + return binding.Status.Credentials.Name != "" && binding.Status.Binding.Name != "" +} diff --git a/controllers/controllers/services/bindings/upsi/controller.go b/controllers/controllers/services/bindings/upsi/controller.go index 5b734d348..9dbb3a0c4 100644 --- a/controllers/controllers/services/bindings/upsi/controller.go +++ b/controllers/controllers/services/bindings/upsi/controller.go @@ -21,21 +21,28 @@ import ( ctrl "sigs.k8s.io/controller-runtime" ) -type CredentialsReconciler struct { +type UPSIBindingReconciler struct { k8sClient client.Client scheme *runtime.Scheme } -func NewReconciler(k8sClient client.Client, scheme *runtime.Scheme) *CredentialsReconciler { - return &CredentialsReconciler{ +func NewReconciler(k8sClient client.Client, scheme *runtime.Scheme) *UPSIBindingReconciler { + return &UPSIBindingReconciler{ k8sClient: k8sClient, scheme: scheme, } } -func (r *CredentialsReconciler) ReconcileResource(ctx context.Context, cfServiceBinding *korifiv1alpha1.CFServiceBinding) (ctrl.Result, error) { +func (r *UPSIBindingReconciler) ReconcileResource(ctx context.Context, cfServiceBinding *korifiv1alpha1.CFServiceBinding) (ctrl.Result, error) { log := logr.FromContextOrDiscard(ctx) - // start upsiCredentialsReconsiler.ReconcileResource + + if !cfServiceBinding.GetDeletionTimestamp().IsZero() { + if controllerutil.RemoveFinalizer(cfServiceBinding, korifiv1alpha1.CFServiceBindingFinalizerName) { + log.V(1).Info("finalizer removed") + } + + return ctrl.Result{}, nil + } cfServiceInstance := new(korifiv1alpha1.CFServiceInstance) err := r.k8sClient.Get(ctx, types.NamespacedName{Name: cfServiceBinding.Spec.Service.Name, Namespace: cfServiceBinding.Namespace}, cfServiceInstance) @@ -67,8 +74,6 @@ func (r *CredentialsReconciler) ReconcileResource(ctx context.Context, cfService return ctrl.Result{}, err } - // end of upsiCredentialsReconsiler.ReconcileResource - return ctrl.Result{}, nil } @@ -84,7 +89,7 @@ func isLegacyServiceBinding(cfServiceBinding *korifiv1alpha1.CFServiceBinding, c return cfServiceInstance.Name == cfServiceBinding.Status.Binding.Name && cfServiceInstance.Spec.SecretName == cfServiceBinding.Status.Binding.Name } -func (r *CredentialsReconciler) reconcileCredentials(ctx context.Context, cfServiceInstance *korifiv1alpha1.CFServiceInstance, cfServiceBinding *korifiv1alpha1.CFServiceBinding) error { +func (r *UPSIBindingReconciler) reconcileCredentials(ctx context.Context, cfServiceInstance *korifiv1alpha1.CFServiceInstance, cfServiceBinding *korifiv1alpha1.CFServiceBinding) error { cfServiceBinding.Status.Credentials.Name = cfServiceInstance.Status.Credentials.Name if isLegacyServiceBinding(cfServiceBinding, cfServiceInstance) { diff --git a/controllers/controllers/services/osbapi/assets.go b/controllers/controllers/services/osbapi/assets.go index 68264334c..e704ec02d 100644 --- a/controllers/controllers/services/osbapi/assets.go +++ b/controllers/controllers/services/osbapi/assets.go @@ -64,3 +64,60 @@ func (r *Assets) GetServiceBroker(ctx context.Context, brokerGUID string) (*kori return serviceBroker, nil } + +type ServiceInstanceAssets struct { + ServiceBroker *korifiv1alpha1.CFServiceBroker + ServiceOffering *korifiv1alpha1.CFServiceOffering + ServicePlan *korifiv1alpha1.CFServicePlan +} + +func (r *Assets) GetServiceInstanceAssets(ctx context.Context, serviceInstance *korifiv1alpha1.CFServiceInstance) (ServiceInstanceAssets, error) { + servicePlan, err := r.GetServicePlan(ctx, serviceInstance.Spec.PlanGUID) + if err != nil { + return ServiceInstanceAssets{}, err + } + + serviceBroker, err := r.GetServiceBroker(ctx, servicePlan.Labels[korifiv1alpha1.RelServiceBrokerGUIDLabel]) + if err != nil { + return ServiceInstanceAssets{}, err + } + + serviceOffering, err := r.GetServiceOffering(ctx, servicePlan.Labels[korifiv1alpha1.RelServiceOfferingGUIDLabel]) + if err != nil { + return ServiceInstanceAssets{}, err + } + + return ServiceInstanceAssets{ + ServiceBroker: serviceBroker, + ServiceOffering: serviceOffering, + ServicePlan: servicePlan, + }, nil +} + +type ServiceBindingAssets struct { + ServiceInstanceAssets + ServiceInstance *korifiv1alpha1.CFServiceInstance +} + +func (r *Assets) GetServiceBindingAssets(ctx context.Context, serviceBinding *korifiv1alpha1.CFServiceBinding) (ServiceBindingAssets, error) { + serviceInstance := &korifiv1alpha1.CFServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: serviceBinding.Namespace, + Name: serviceBinding.Spec.Service.Name, + }, + } + err := r.k8sClient.Get(ctx, client.ObjectKeyFromObject(serviceInstance), serviceInstance) + if err != nil { + return ServiceBindingAssets{}, err + } + + serviceInstanceAssets, err := r.GetServiceInstanceAssets(ctx, serviceInstance) + if err != nil { + return ServiceBindingAssets{}, err + } + + return ServiceBindingAssets{ + ServiceInstanceAssets: serviceInstanceAssets, + ServiceInstance: serviceInstance, + }, nil +} diff --git a/controllers/controllers/services/osbapi/client.go b/controllers/controllers/services/osbapi/client.go index 8b1315abe..41b2414eb 100644 --- a/controllers/controllers/services/osbapi/client.go +++ b/controllers/controllers/services/osbapi/client.go @@ -5,6 +5,7 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -25,6 +26,21 @@ func (c ConflictError) Error() string { return "The service binding already exists" } +func IgnoreGone(err error) error { + if IsGone(err) { + return nil + } + return err +} + +func IsGone(err error) bool { + if err == nil { + return false + } + + return errors.As(err, &GoneError{}) +} + type Client struct { broker Broker httpClient *http.Client @@ -254,6 +270,41 @@ func (c *Client) GetServiceBindingLastOperation(ctx context.Context, request Get return response, nil } +func (c *Client) Unbind(ctx context.Context, payload UnbindPayload) (UnbindResponse, error) { + statusCode, respBytes, err := c.newBrokerRequester(). + forBroker(c.broker). + async(). + sendRequest( + ctx, + "/v2/service_instances/"+payload.InstanceID+"/service_bindings/"+payload.BindingID, + http.MethodDelete, + map[string]string{ + "service_id": payload.ServiceId, + "plan_id": payload.PlanID, + }, + nil, + ) + if err != nil { + return UnbindResponse{}, fmt.Errorf("unbind request failed: %w", err) + } + + if statusCode == http.StatusGone { + return UnbindResponse{}, GoneError{} + } + + if statusCode >= 300 { + return UnbindResponse{}, fmt.Errorf("unbind request failed with status code: %d", statusCode) + } + + var response UnbindResponse + err = json.Unmarshal(respBytes, &response) + if err != nil { + return UnbindResponse{}, fmt.Errorf("failed to unmarshal response: %w", err) + } + + return response, nil +} + func payloadToReader(payload any) (io.Reader, error) { if payload == nil { return nil, nil diff --git a/controllers/controllers/services/osbapi/client_test.go b/controllers/controllers/services/osbapi/client_test.go index 99f770e0d..cd9a9f610 100644 --- a/controllers/controllers/services/osbapi/client_test.go +++ b/controllers/controllers/services/osbapi/client_test.go @@ -107,27 +107,13 @@ var _ = Describe("OSBAPI Client", func() { When("getting the catalog fails", func() { BeforeEach(func() { - brokerServer = brokerServer.WithHandler("/v2/catalog", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusTeapot) - })) + brokerServer = brokerServer.WithResponse("/v2/catalog", nil, http.StatusTeapot) }) It("returns an error", func() { Expect(getCatalogErr).To(MatchError(ContainSubstring(strconv.Itoa(http.StatusTeapot)))) }) }) - - When("the catalog response cannot be unmarshalled", func() { - BeforeEach(func() { - brokerServer = brokerServer.WithHandler("/v2/catalog", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte("hello")) - })) - }) - - It("returns an error", func() { - Expect(getCatalogErr).To(MatchError(ContainSubstring("failed to unmarshal catalog"))) - }) - }) }) Describe("Instances", func() { @@ -223,9 +209,7 @@ var _ = Describe("OSBAPI Client", func() { When("the provision request fails", func() { BeforeEach(func() { - brokerServer = brokerServer.WithHandler("/v2/service_instances/{id}", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusTeapot) - })) + brokerServer = brokerServer.WithResponse("/v2/service_instances/{id}", nil, http.StatusTeapot) }) It("returns an error", func() { @@ -298,9 +282,11 @@ var _ = Describe("OSBAPI Client", func() { When("the deprovision request fails", func() { BeforeEach(func() { - brokerServer = brokerServer.WithHandler("/v2/service_instances/{id}", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusTeapot) - })) + brokerServer = brokerServer.WithResponse( + "/v2/service_instances/{id}", + nil, + http.StatusTeapot, + ) }) It("returns an error", func() { @@ -385,9 +371,11 @@ var _ = Describe("OSBAPI Client", func() { When("getting the last operation request fails", func() { BeforeEach(func() { - brokerServer = brokerServer.WithHandler("/v2/service_instances/{id}/last_operation", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusTeapot) - })) + brokerServer = brokerServer.WithResponse( + "/v2/service_instances/{id}/last_operation", + nil, + http.StatusTeapot, + ) }) It("returns an error", func() { @@ -397,9 +385,11 @@ var _ = Describe("OSBAPI Client", func() { When("getting the last operation request fails with 410 Gone", func() { BeforeEach(func() { - brokerServer = brokerServer.WithHandler("/v2/service_instances/{id}/last_operation", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusGone) - })) + brokerServer = brokerServer.WithResponse( + "/v2/service_instances/{id}/last_operation", + nil, + http.StatusGone, + ) }) It("returns a gone error", func() { @@ -517,9 +507,11 @@ var _ = Describe("OSBAPI Client", func() { When("binding request fails", func() { BeforeEach(func() { - brokerServer = brokerServer.WithHandler("/v2/service_instances/{instance_id}/service_bindings/{binding_id}", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusTeapot) - })) + brokerServer = brokerServer.WithResponse( + "/v2/service_instances/{instance_id}/service_bindings/{binding_id}", + nil, + http.StatusTeapot, + ) }) It("returns an error", func() { @@ -529,9 +521,11 @@ var _ = Describe("OSBAPI Client", func() { When("binding request fails with 409 Confilct", func() { BeforeEach(func() { - brokerServer = brokerServer.WithHandler("/v2/service_instances/{instance_id}/service_bindings/{binding_id}", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusConflict) - })) + brokerServer = brokerServer.WithResponse( + "/v2/service_instances/{instance_id}/service_bindings/{binding_id}", + nil, + http.StatusConflict, + ) }) It("returns a confilct error", func() { @@ -593,11 +587,11 @@ var _ = Describe("OSBAPI Client", func() { When("getting the last operation request fails", func() { BeforeEach(func() { - brokerServer = brokerServer.WithHandler( + brokerServer = brokerServer.WithResponse( "/v2/service_instances/{instance_id}/service_bindings/{binding_id}/last_operation", - http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusTeapot) - })) + nil, + http.StatusTeapot, + ) }) It("returns an error", func() { @@ -607,11 +601,11 @@ var _ = Describe("OSBAPI Client", func() { When("getting the last operation request fails with 410 Gone", func() { BeforeEach(func() { - brokerServer = brokerServer.WithHandler( + brokerServer = brokerServer.WithResponse( "/v2/service_instances/{instance_id}/service_bindings/{binding_id}/last_operation", - http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusGone) - })) + nil, + http.StatusGone, + ) }) It("returns a gone error", func() { @@ -677,11 +671,11 @@ var _ = Describe("OSBAPI Client", func() { When("getting the binding fails", func() { BeforeEach(func() { - brokerServer = brokerServer.WithHandler( + brokerServer = brokerServer.WithResponse( "/v2/service_instances/{instance_id}/service_bindings/{binding_id}", - http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusTeapot) - })) + nil, + http.StatusTeapot, + ) }) It("returns an error", func() { @@ -690,4 +684,97 @@ var _ = Describe("OSBAPI Client", func() { }) }) }) + + Describe("Unbind", func() { + var ( + unbindResp osbapi.UnbindResponse + unbindErr error + ) + + BeforeEach(func() { + brokerServer.WithResponse( + "/v2/service_instances/{instance_id}/service_bindings/{binding_id}", + nil, + http.StatusOK, + ) + }) + + JustBeforeEach(func() { + unbindResp, unbindErr = brokerClient.Unbind(ctx, osbapi.UnbindPayload{ + InstanceID: "instance-id", + BindingID: "binding-id", + UnbindRequest: osbapi.UnbindRequest{ + ServiceId: "service-guid", + PlanID: "plan-guid", + }, + }) + }) + + It("sends an unbind request to broker", func() { + Expect(unbindErr).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/instance-id/service_bindings/binding-id")) + + Expect(requests[0].URL.Query()).To(BeEquivalentTo(map[string][]string{ + "service_id": {"service-guid"}, + "plan_id": {"plan-guid"}, + "accepts_incomplete": {"true"}, + })) + }) + + It("responds synchronously", func() { + Expect(unbindErr).NotTo(HaveOccurred()) + Expect(unbindResp.IsComplete()).To(BeTrue()) + }) + + When("broker return 202 Accepted", func() { + BeforeEach(func() { + brokerServer = brokerServer.WithResponse( + "/v2/service_instances/{instance_id}/service_bindings/{binding_id}", + map[string]any{ + "operation": "operation-id", + }, + http.StatusAccepted, + ) + }) + + It("responds asynchronously", func() { + Expect(unbindErr).NotTo(HaveOccurred()) + Expect(unbindResp.IsComplete()).To(BeFalse()) + Expect(unbindResp.Operation).To(Equal("operation-id")) + }) + }) + + When("the binding does not exist", func() { + BeforeEach(func() { + brokerServer = brokerServer.WithResponse( + "/v2/service_instances/{instance_id}/service_bindings/{binding_id}", + nil, + http.StatusGone, + ) + }) + + It("returns a gone error", func() { + Expect(unbindErr).To(BeAssignableToTypeOf(osbapi.GoneError{})) + }) + }) + + When("the unbind request fails", func() { + BeforeEach(func() { + brokerServer = brokerServer.WithResponse( + "/v2/service_instances/{instance_id}/service_bindings/{binding_id}", + nil, + http.StatusTeapot, + ) + }) + + It("returns an error", func() { + Expect(unbindErr).To(MatchError(ContainSubstring("unbind request failed"))) + }) + }) + }) }) diff --git a/controllers/controllers/services/osbapi/clientfactory.go b/controllers/controllers/services/osbapi/clientfactory.go index 6776f46d8..eb30f934f 100644 --- a/controllers/controllers/services/osbapi/clientfactory.go +++ b/controllers/controllers/services/osbapi/clientfactory.go @@ -22,6 +22,7 @@ type BrokerClient interface { GetServiceInstanceLastOperation(context.Context, GetServiceInstanceLastOperationRequest) (LastOperationResponse, error) GetCatalog(context.Context) (Catalog, error) Bind(context.Context, BindPayload) (BindResponse, error) + Unbind(context.Context, UnbindPayload) (UnbindResponse, error) GetServiceBindingLastOperation(context.Context, GetServiceBindingLastOperationRequest) (LastOperationResponse, error) GetServiceBinding(context.Context, GetServiceBindingRequest) (GetBindingResponse, error) } diff --git a/controllers/controllers/services/osbapi/fake/broker_client.go b/controllers/controllers/services/osbapi/fake/broker_client.go index 984a1a4c9..c27849378 100644 --- a/controllers/controllers/services/osbapi/fake/broker_client.go +++ b/controllers/controllers/services/osbapi/fake/broker_client.go @@ -106,6 +106,20 @@ type BrokerClient struct { result1 osbapi.ServiceInstanceOperationResponse result2 error } + UnbindStub func(context.Context, osbapi.UnbindPayload) (osbapi.UnbindResponse, error) + unbindMutex sync.RWMutex + unbindArgsForCall []struct { + arg1 context.Context + arg2 osbapi.UnbindPayload + } + unbindReturns struct { + result1 osbapi.UnbindResponse + result2 error + } + unbindReturnsOnCall map[int]struct { + result1 osbapi.UnbindResponse + result2 error + } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } @@ -564,6 +578,71 @@ func (fake *BrokerClient) ProvisionReturnsOnCall(i int, result1 osbapi.ServiceIn }{result1, result2} } +func (fake *BrokerClient) Unbind(arg1 context.Context, arg2 osbapi.UnbindPayload) (osbapi.UnbindResponse, error) { + fake.unbindMutex.Lock() + ret, specificReturn := fake.unbindReturnsOnCall[len(fake.unbindArgsForCall)] + fake.unbindArgsForCall = append(fake.unbindArgsForCall, struct { + arg1 context.Context + arg2 osbapi.UnbindPayload + }{arg1, arg2}) + stub := fake.UnbindStub + fakeReturns := fake.unbindReturns + fake.recordInvocation("Unbind", []interface{}{arg1, arg2}) + fake.unbindMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *BrokerClient) UnbindCallCount() int { + fake.unbindMutex.RLock() + defer fake.unbindMutex.RUnlock() + return len(fake.unbindArgsForCall) +} + +func (fake *BrokerClient) UnbindCalls(stub func(context.Context, osbapi.UnbindPayload) (osbapi.UnbindResponse, error)) { + fake.unbindMutex.Lock() + defer fake.unbindMutex.Unlock() + fake.UnbindStub = stub +} + +func (fake *BrokerClient) UnbindArgsForCall(i int) (context.Context, osbapi.UnbindPayload) { + fake.unbindMutex.RLock() + defer fake.unbindMutex.RUnlock() + argsForCall := fake.unbindArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *BrokerClient) UnbindReturns(result1 osbapi.UnbindResponse, result2 error) { + fake.unbindMutex.Lock() + defer fake.unbindMutex.Unlock() + fake.UnbindStub = nil + fake.unbindReturns = struct { + result1 osbapi.UnbindResponse + result2 error + }{result1, result2} +} + +func (fake *BrokerClient) UnbindReturnsOnCall(i int, result1 osbapi.UnbindResponse, result2 error) { + fake.unbindMutex.Lock() + defer fake.unbindMutex.Unlock() + fake.UnbindStub = nil + if fake.unbindReturnsOnCall == nil { + fake.unbindReturnsOnCall = make(map[int]struct { + result1 osbapi.UnbindResponse + result2 error + }) + } + fake.unbindReturnsOnCall[i] = struct { + result1 osbapi.UnbindResponse + result2 error + }{result1, result2} +} + func (fake *BrokerClient) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() @@ -581,6 +660,8 @@ func (fake *BrokerClient) Invocations() map[string][][]interface{} { defer fake.getServiceInstanceLastOperationMutex.RUnlock() fake.provisionMutex.RLock() defer fake.provisionMutex.RUnlock() + fake.unbindMutex.RLock() + defer fake.unbindMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} for key, value := range fake.invocations { copiedInvocations[key] = value diff --git a/controllers/controllers/services/osbapi/types.go b/controllers/controllers/services/osbapi/types.go index 9072e903d..c1baa3a71 100644 --- a/controllers/controllers/services/osbapi/types.go +++ b/controllers/controllers/services/osbapi/types.go @@ -103,6 +103,25 @@ type BindResource struct { AppGUID string `json:"app_guid"` } +type UnbindPayload struct { + BindingID string + InstanceID string + UnbindRequest +} + +type UnbindRequest struct { + ServiceId string `json:"service_id"` + PlanID string `json:"plan_id"` +} + +type UnbindResponse struct { + Operation string `json:"operation,omitempty"` +} + +func (r UnbindResponse) IsComplete() bool { + return r.Operation == "" +} + type Plan struct { ID string `json:"id"` Name string `json:"name"` diff --git a/controllers/webhooks/finalizer/suite_test.go b/controllers/webhooks/finalizer/suite_test.go index 62d6b2cc6..26d5348c2 100644 --- a/controllers/webhooks/finalizer/suite_test.go +++ b/controllers/webhooks/finalizer/suite_test.go @@ -12,6 +12,7 @@ import ( "code.cloudfoundry.org/korifi/controllers/webhooks/finalizer" "code.cloudfoundry.org/korifi/controllers/webhooks/networking/domains" "code.cloudfoundry.org/korifi/controllers/webhooks/networking/routes" + bindingswebhook "code.cloudfoundry.org/korifi/controllers/webhooks/services/bindings" "code.cloudfoundry.org/korifi/controllers/webhooks/services/instances" "code.cloudfoundry.org/korifi/controllers/webhooks/validation" "code.cloudfoundry.org/korifi/controllers/webhooks/version" @@ -111,6 +112,9 @@ var _ = BeforeSuite(func() { ).SetupWebhookWithManager(k8sManager)).To(Succeed()) Expect(packages.NewValidator().SetupWebhookWithManager(k8sManager)).To(Succeed()) Expect(instances.NewValidator(validation.NewDuplicateValidator(coordination.NewNameRegistry(uncachedClient, instances.ServiceInstanceEntityType))).SetupWebhookWithManager(k8sManager)).To(Succeed()) + Expect(bindingswebhook.NewCFServiceBindingValidator( + validation.NewDuplicateValidator(coordination.NewNameRegistry(uncachedClient, bindingswebhook.ServiceBindingEntityType)), + ).SetupWebhookWithManager(k8sManager)).To(Succeed()) stopManager = helpers.StartK8sManager(k8sManager) diff --git a/controllers/webhooks/finalizer/webhook.go b/controllers/webhooks/finalizer/webhook.go index 22479099b..af486a31b 100644 --- a/controllers/webhooks/finalizer/webhook.go +++ b/controllers/webhooks/finalizer/webhook.go @@ -1,6 +1,6 @@ package finalizer -//+kubebuilder:webhook:path=/mutate-korifi-cloudfoundry-org-v1alpha1-controllers-finalizer,mutating=true,failurePolicy=fail,sideEffects=None,groups=korifi.cloudfoundry.org,resources=cfapps;cfspaces;cfpackages;cforgs;cfroutes;cfdomains;cfserviceinstances,verbs=create,versions=v1alpha1,name=mcffinalizer.korifi.cloudfoundry.org,admissionReviewVersions={v1,v1beta1} +//+kubebuilder:webhook:path=/mutate-korifi-cloudfoundry-org-v1alpha1-controllers-finalizer,mutating=true,failurePolicy=fail,sideEffects=None,groups=korifi.cloudfoundry.org,resources=cfapps;cfspaces;cfpackages;cforgs;cfroutes;cfdomains;cfservicebindings;cfserviceinstances,verbs=create,versions=v1alpha1,name=mcffinalizer.korifi.cloudfoundry.org,admissionReviewVersions={v1,v1beta1} import ( "context" @@ -26,6 +26,7 @@ func NewControllersFinalizerWebhook() *ControllersFinalizerWebhook { "CFOrg": {FinalizerName: korifiv1alpha1.CFOrgFinalizerName, SetPolicy: k8s.Always}, "CFDomain": {FinalizerName: korifiv1alpha1.CFDomainFinalizerName, SetPolicy: k8s.Always}, "CFServiceInstance": {FinalizerName: korifiv1alpha1.CFManagedServiceInstanceFinalizerName, SetPolicy: isManagedServiceInstance}, + "CFServiceBinding": {FinalizerName: korifiv1alpha1.CFServiceBindingFinalizerName, SetPolicy: k8s.Always}, }), } } diff --git a/controllers/webhooks/finalizer/webhook_test.go b/controllers/webhooks/finalizer/webhook_test.go index 0ec656b4b..55a7483b6 100644 --- a/controllers/webhooks/finalizer/webhook_test.go +++ b/controllers/webhooks/finalizer/webhook_test.go @@ -132,5 +132,14 @@ var _ = Describe("Controllers Finalizers Webhook", func() { }, }, ), + Entry("cfservicebinding", + &korifiv1alpha1.CFServiceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-org-" + uuid.NewString(), + Name: uuid.NewString(), + }, + }, + korifiv1alpha1.CFServiceBindingFinalizerName, + ), ) }) diff --git a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfservicebindings.yaml b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfservicebindings.yaml index 6f4472386..3282636eb 100644 --- a/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfservicebindings.yaml +++ b/helm/korifi/controllers/crds/korifi.cloudfoundry.org_cfservicebindings.yaml @@ -221,6 +221,13 @@ spec: the CFServiceBinding that has been reconciled format: int64 type: integer + unbindingOperation: + description: |- + The + [operation](https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#unbinding) + of the unbind request to the the OSBAPI broker. Only makes sense for + bindings to managed service instances + type: string type: object type: object served: true diff --git a/helm/korifi/controllers/manifests.yaml b/helm/korifi/controllers/manifests.yaml index 063bdcd8d..f548ca7b9 100644 --- a/helm/korifi/controllers/manifests.yaml +++ b/helm/korifi/controllers/manifests.yaml @@ -135,6 +135,7 @@ webhooks: - cforgs - cfroutes - cfdomains + - cfservicebindings - cfserviceinstances sideEffects: None - admissionReviewVersions: diff --git a/helm/korifi/controllers/role.yaml b/helm/korifi/controllers/role.yaml index 22e45c152..aa003ebb7 100644 --- a/helm/korifi/controllers/role.yaml +++ b/helm/korifi/controllers/role.yaml @@ -132,6 +132,7 @@ rules: - cforgs/finalizers - cfprocesses/finalizers - cfroutes/finalizers + - cfservicebindings/finalizers - cfserviceinstances/finalizers - cfspaces/finalizers - cftasks/finalizers diff --git a/tests/assets/sample-broker-golang/main.go b/tests/assets/sample-broker-golang/main.go index 7ebcff215..10b0aad0d 100644 --- a/tests/assets/sample-broker-golang/main.go +++ b/tests/assets/sample-broker-golang/main.go @@ -26,6 +26,7 @@ func main() { http.HandleFunc("PUT /v2/service_instances/{instance_id}/service_bindings/{binding_id}", bindHandler) http.HandleFunc("GET /v2/service_instances/{instance_id}/service_bindings/{binding_id}/last_operation", serviceBindingLastOperationHandler) http.HandleFunc("GET /v2/service_instances/{instance_id}/service_bindings/{binding_id}", getServiceBindingHandler) + http.HandleFunc("DELETE /v2/service_instances/{instance_id}/service_bindings/{binding_id}", unbindHandler) port := os.Getenv("PORT") if port == "" { @@ -137,6 +138,16 @@ func getServiceBindingHandler(w http.ResponseWriter, r *http.Request) { }}`) } +func unbindHandler(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":"unbind-%s"}`, r.PathValue("binding_id")) +} + func checkCredentials(_ http.ResponseWriter, r *http.Request) (int, error) { authHeader := r.Header.Get("Authorization") if len(authHeader) == 0 { diff --git a/tests/e2e/apps_test.go b/tests/e2e/apps_test.go index 7f89b62e6..e77b63805 100644 --- a/tests/e2e/apps_test.go +++ b/tests/e2e/apps_test.go @@ -156,14 +156,7 @@ var _ = Describe("Apps", func() { HaveRestyStatusCode(http.StatusAccepted), HaveRestyHeaderWithValue("Location", HaveSuffix("/v3/jobs/app.delete~"+appGUID)), )) - - jobURL := resp.Header().Get("Location") - Eventually(func(g Gomega) { - resp, err = adminClient.R().Get(jobURL) - g.Expect(err).NotTo(HaveOccurred()) - jobRespBody := string(resp.Body()) - g.Expect(jobRespBody).To(ContainSubstring("COMPLETE")) - }).Should(Succeed()) + expectJobCompletes(resp) resp, err = adminClient.R().Get("/v3/apps/" + appGUID) Expect(err).NotTo(HaveOccurred()) @@ -632,7 +625,7 @@ var _ = Describe("Apps", func() { "baz": "qux", } serviceInstanceGUID = createServiceInstance(space1GUID, generateGUID("service-instance"), credentials) - bindingGUID = createServiceBinding(appGUID, serviceInstanceGUID, "") + bindingGUID = createUPSIServiceBinding(appGUID, serviceInstanceGUID, "") moreCredentials := map[string]string{ "hello": "there", @@ -640,7 +633,7 @@ var _ = Describe("Apps", func() { } secondServiceInstanceGUID = createServiceInstance(space1GUID, generateGUID("service-instance"), moreCredentials) bindingName = "custom-named-binding" - namedBindingGUID = createServiceBinding(appGUID, secondServiceInstanceGUID, bindingName) + namedBindingGUID = createUPSIServiceBinding(appGUID, secondServiceInstanceGUID, bindingName) var httpResp *resty.Response httpResp, httpError = adminClient.R().SetResult(&result).Post("/v3/apps/" + appGUID + "/actions/restart") @@ -712,10 +705,10 @@ var _ = Describe("Apps", func() { }) instanceName = generateGUID("service-instance") instanceGUID = createServiceInstance(space1GUID, instanceName, nil) - bindingGUID = createServiceBinding(appGUID, instanceGUID, "") + bindingGUID = createUPSIServiceBinding(appGUID, instanceGUID, "") instanceName2 = generateGUID("service-instance") instanceGUID2 = createServiceInstance(space1GUID, instanceName2, nil) - bindingGUID2 = createServiceBinding(appGUID, instanceGUID2, "") + bindingGUID2 = createUPSIServiceBinding(appGUID, instanceGUID2, "") }) JustBeforeEach(func() { diff --git a/tests/e2e/domains_test.go b/tests/e2e/domains_test.go index edfbd30e1..1cf0dad98 100644 --- a/tests/e2e/domains_test.go +++ b/tests/e2e/domains_test.go @@ -163,14 +163,7 @@ var _ = Describe("Domain", func() { HaveRestyStatusCode(http.StatusAccepted), HaveRestyHeaderWithValue("Location", HaveSuffix("/v3/jobs/domain.delete~"+domainGUID)), )) - - jobURL := resp.Header().Get("Location") - Eventually(func(g Gomega) { - var err error - resp, err = adminClient.R().Get(jobURL) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(string(resp.Body())).To(ContainSubstring("COMPLETE")) - }).Should(Succeed()) + expectJobCompletes(resp) }) It("deletes the domain", func() { diff --git a/tests/e2e/e2e_suite_test.go b/tests/e2e/e2e_suite_test.go index 4b6b3b07e..30598249a 100644 --- a/tests/e2e/e2e_suite_test.go +++ b/tests/e2e/e2e_suite_test.go @@ -10,6 +10,7 @@ import ( "net/http" "os" "regexp" + "slices" "strings" "sync" "testing" @@ -21,6 +22,7 @@ import ( "code.cloudfoundry.org/go-loggregator/v8/rpc/loggregator_v2" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "github.com/BooleanCat/go-functional/v2/it" "github.com/BooleanCat/go-functional/v2/it/itx" "github.com/go-resty/resty/v2" "github.com/google/uuid" @@ -742,7 +744,7 @@ func listServiceInstances(names ...string) resourceList[serviceInstanceResource] return serviceInstances } -func createServiceBinding(appGUID, instanceGUID, bindingName string) string { +func createUPSIServiceBinding(appGUID, instanceGUID, bindingName string) string { GinkgoHelper() var serviceCredentialBinding resource @@ -764,6 +766,32 @@ func createServiceBinding(appGUID, instanceGUID, bindingName string) string { return serviceCredentialBinding.GUID } +func createManagedServiceBinding(appGUID, instanceGUID, bindingName string) string { + GinkgoHelper() + + resp, err := adminClient.R(). + SetBody(typedResource{ + Type: "app", + resource: resource{ + Name: bindingName, + Relationships: relationships{"app": {Data: resource{GUID: appGUID}}, "service_instance": {Data: resource{GUID: instanceGUID}}}, + }, + }). + Post("/v3/service_credential_bindings") + + Expect(err).NotTo(HaveOccurred()) + Expect(resp).To(SatisfyAll( + HaveRestyStatusCode(http.StatusAccepted), + HaveRestyHeaderWithValue("Location", ContainSubstring("/v3/jobs/managed_service_binding.create~")), + )) + jobURL := resp.Header().Get("Location") + expectJobCompletes(resp) + + jobURLSplit := strings.Split(jobURL, "~") + Expect(jobURLSplit).To(HaveLen(2)) + return jobURLSplit[1] +} + func getServiceBindingsForApp(appGUID string) []resource { GinkgoHelper() @@ -1185,13 +1213,11 @@ func createBroker(brokerURL string) string { }).Should(Succeed()) plans := resourceList[resource]{} - listResp, err := adminClient.R().SetResult(&plans).Get("/v3/service_plans") + listResp, err := adminClient.R().SetResult(&plans).Get("/v3/service_plans?service_broker_guids=" + brokerGUID) Expect(err).NotTo(HaveOccurred()) Expect(listResp).To(HaveRestyStatusCode(http.StatusOK)) - itx.FromSlice(plans.Resources).Filter(func(r resource) bool { - return r.Metadata.Labels[korifiv1alpha1.RelServiceBrokerGUIDLabel] == brokerGUID - }).ForEach(func(plan resource) { + for _, plan := range plans.Resources { resp, err := adminClient.R(). SetBody(planVisibilityResource{ Type: "public", @@ -1201,7 +1227,7 @@ func createBroker(brokerURL string) string { Expect(resp).To(SatisfyAll( HaveRestyStatusCode(http.StatusOK), )) - }) + } return brokerGUID } @@ -1209,6 +1235,9 @@ func createBroker(brokerURL string) string { func cleanupBroker(brokerGUID string) { GinkgoHelper() + deleteBindingsForBroker(brokerGUID) + deleteInstancesForBroker(brokerGUID) + Expect(brokerGUID).NotTo(BeEmpty()) _, err := adminClient.R(). Delete("/v3/service_brokers/" + brokerGUID) @@ -1216,3 +1245,62 @@ func cleanupBroker(brokerGUID string) { broker.NewCatalogPurger(rootNamespace).ForBrokerGUID(brokerGUID).Purge() } + +func deleteBindingsForBroker(brokerGUID string) { + plans := resourceList[resource]{} + listPlanResp, err := adminClient.R().SetResult(&plans).Get("/v3/service_plans?service_broker_guids=" + brokerGUID) + Expect(err).NotTo(HaveOccurred()) + Expect(listPlanResp).To(HaveRestyStatusCode(http.StatusOK)) + + planGUIDs := slices.Collect(it.Map(itx.FromSlice(plans.Resources), func(r resource) string { + return r.GUID + })) + + bindings := resourceList[resource]{} + listBindingsResp, err := adminClient.R().SetResult(&bindings).Get("/v3/service_credential_bindings?service_plan_guids=" + strings.Join(planGUIDs, ",")) + Expect(err).NotTo(HaveOccurred()) + Expect(listBindingsResp).To(HaveRestyStatusCode(http.StatusOK)) + + for _, binding := range bindings.Resources { + resp, err := adminClient.R().Delete(fmt.Sprintf("/v3/service_credential_bindings/%s", binding.GUID)) + Expect(err).NotTo(HaveOccurred()) + expectJobCompletes(resp) + } +} + +func deleteInstancesForBroker(brokerGUID string) { + plans := resourceList[resource]{} + listPlanResp, err := adminClient.R().SetResult(&plans).Get("/v3/service_plans?service_broker_guids=" + brokerGUID) + Expect(err).NotTo(HaveOccurred()) + Expect(listPlanResp).To(HaveRestyStatusCode(http.StatusOK)) + + planGUIDs := slices.Collect(it.Map(itx.FromSlice(plans.Resources), func(r resource) string { + return r.GUID + })) + + instances := resourceList[resource]{} + listInstancesResp, err := adminClient.R().SetResult(&instances).Get("/v3/service_instances?service_plan_guids=" + strings.Join(planGUIDs, ",")) + Expect(err).NotTo(HaveOccurred()) + Expect(listInstancesResp).To(HaveRestyStatusCode(http.StatusOK)) + + for _, binding := range instances.Resources { + resp, err := adminClient.R().Delete(fmt.Sprintf("/v3/service_instances/%s", binding.GUID)) + Expect(err).NotTo(HaveOccurred()) + expectJobCompletes(resp) + } +} + +func expectJobCompletes(resp *resty.Response) { + GinkgoHelper() + + Expect(resp).To(SatisfyAll( + HaveRestyHeaderWithValue("Location", Not(BeEmpty())), + )) + + jobURL := resp.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()) +} diff --git a/tests/e2e/orgs_test.go b/tests/e2e/orgs_test.go index fcdf1cc1a..827b4adef 100644 --- a/tests/e2e/orgs_test.go +++ b/tests/e2e/orgs_test.go @@ -161,13 +161,7 @@ var _ = Describe("Orgs", func() { HaveRestyStatusCode(http.StatusAccepted), HaveRestyHeaderWithValue("Location", HaveSuffix("/v3/jobs/org.delete~"+orgGUID)), )) - - jobURL := resp.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()) + expectJobCompletes(resp) orgResp, err := adminClient.R().Get("/v3/organizations/" + orgGUID) Expect(err).NotTo(HaveOccurred()) @@ -184,13 +178,7 @@ var _ = Describe("Orgs", func() { HaveRestyStatusCode(http.StatusAccepted), HaveRestyHeaderWithValue("Location", HaveSuffix("/v3/jobs/org.delete~"+orgGUID)), )) - - jobURL := resp.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()) + expectJobCompletes(resp) orgResp, err := adminClient.R().Get("/v3/organizations/" + orgGUID) Expect(err).NotTo(HaveOccurred()) diff --git a/tests/e2e/roles_test.go b/tests/e2e/roles_test.go index 767fb8c7a..fc30ec75c 100644 --- a/tests/e2e/roles_test.go +++ b/tests/e2e/roles_test.go @@ -165,13 +165,7 @@ var _ = Describe("Roles", func() { HaveRestyStatusCode(http.StatusAccepted), HaveRestyHeaderWithValue("Location", HaveSuffix("/v3/jobs/role.delete~"+roleGUID)), )) - - jobURL := resp.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()) + expectJobCompletes(resp) resp, err := adminClient.R().Get("/v3/roles/" + roleGUID) Expect(err).NotTo(HaveOccurred()) diff --git a/tests/e2e/routes_test.go b/tests/e2e/routes_test.go index c1cd22d9a..464e86304 100644 --- a/tests/e2e/routes_test.go +++ b/tests/e2e/routes_test.go @@ -164,13 +164,7 @@ var _ = Describe("Routes", func() { HavePrefix(apiServerRoot), ContainSubstring("/v3/jobs/route.delete~"+routeGUID), ))) - - jobURL := resp.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()) + expectJobCompletes(resp) getRouteResp, err := adminClient.R().Get("/v3/routes/" + routeGUID) Expect(err).NotTo(HaveOccurred()) diff --git a/tests/e2e/service_bindings_test.go b/tests/e2e/service_bindings_test.go index f7bdafcfd..16beefc40 100644 --- a/tests/e2e/service_bindings_test.go +++ b/tests/e2e/service_bindings_test.go @@ -75,13 +75,7 @@ var _ = Describe("Service Bindings", func() { HaveRestyStatusCode(http.StatusAccepted), HaveRestyHeaderWithValue("Location", ContainSubstring("/v3/jobs/managed_service_binding.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()) + expectJobCompletes(httpResp) }) }) }) @@ -90,7 +84,7 @@ var _ = Describe("Service Bindings", func() { var respResource responseResource BeforeEach(func() { - bindingGUID = createServiceBinding(appGUID, upsiGUID, "") + bindingGUID = createUPSIServiceBinding(appGUID, upsiGUID, "") }) JustBeforeEach(func() { @@ -106,17 +100,40 @@ var _ = Describe("Service Bindings", func() { }) Describe("DELETE /v3/service_credential_bindings/{guid}", func() { - BeforeEach(func() { - bindingGUID = createServiceBinding(appGUID, upsiGUID, "") - }) - JustBeforeEach(func() { httpResp, httpError = adminClient.R().Delete("/v3/service_credential_bindings/" + bindingGUID) }) - It("succeeds", func() { - Expect(httpError).NotTo(HaveOccurred()) - Expect(httpResp).To(HaveRestyStatusCode(http.StatusNoContent)) + When("bound to a user provided service", func() { + BeforeEach(func() { + bindingGUID = createUPSIServiceBinding(appGUID, upsiGUID, "") + }) + + It("succeeds", func() { + Expect(httpError).NotTo(HaveOccurred()) + Expect(httpResp).To(HaveRestyStatusCode(http.StatusNoContent)) + }) + }) + + When("bound to a managed service instance", func() { + BeforeEach(func() { + brokerGUID := createBroker(serviceBrokerURL) + DeferCleanup(func() { + cleanupBroker(brokerGUID) + }) + + instanceGUID := createManagedServiceInstance(brokerGUID, spaceGUID) + bindingGUID = createManagedServiceBinding(appGUID, instanceGUID, "") + }) + + It("succeeds with a job redirect", func() { + Expect(httpError).NotTo(HaveOccurred()) + Expect(httpResp).To(SatisfyAll( + HaveRestyStatusCode(http.StatusAccepted), + HaveRestyHeaderWithValue("Location", ContainSubstring("/v3/jobs/managed_service_binding.delete~")), + )) + expectJobCompletes(httpResp) + }) }) }) @@ -128,10 +145,10 @@ var _ = Describe("Service Bindings", func() { ) BeforeEach(func() { - bindingGUID = createServiceBinding(appGUID, upsiGUID, "") + bindingGUID = createUPSIServiceBinding(appGUID, upsiGUID, "") anotherInstanceGUID = createServiceInstance(spaceGUID, generateGUID("another-service-instance"), nil) - anotherBindingGUID = createServiceBinding(appGUID, anotherInstanceGUID, "") + anotherBindingGUID = createUPSIServiceBinding(appGUID, anotherInstanceGUID, "") result = resourceListWithInclusion{} }) @@ -154,7 +171,7 @@ var _ = Describe("Service Bindings", func() { var respResource responseResource BeforeEach(func() { - bindingGUID = createServiceBinding(appGUID, upsiGUID, "") + bindingGUID = createUPSIServiceBinding(appGUID, upsiGUID, "") }) JustBeforeEach(func() { diff --git a/tests/e2e/service_brokers_test.go b/tests/e2e/service_brokers_test.go index edbb7e104..49edf1849 100644 --- a/tests/e2e/service_brokers_test.go +++ b/tests/e2e/service_brokers_test.go @@ -45,12 +45,7 @@ var _ = Describe("Service Brokers", func() { )) jobURL := resp.Header().Get("Location") - Eventually(func(g Gomega) { - resp, err = adminClient.R().Get(jobURL) - g.Expect(err).NotTo(HaveOccurred()) - jobRespBody := string(resp.Body()) - g.Expect(jobRespBody).To(ContainSubstring("COMPLETE")) - }).Should(Succeed()) + expectJobCompletes(resp) jobURLSplit := strings.Split(jobURL, "~") Expect(jobURLSplit).To(HaveLen(2)) @@ -123,14 +118,7 @@ var _ = Describe("Service Brokers", func() { HaveRestyStatusCode(http.StatusAccepted), HaveRestyHeaderWithValue("Location", ContainSubstring("/v3/jobs/service_broker.update~")), )) - - jobURL := resp.Header().Get("Location") - Eventually(func(g Gomega) { - jobResp, jobErr := adminClient.R().Get(jobURL) - g.Expect(jobErr).NotTo(HaveOccurred()) - jobRespBody := string(jobResp.Body()) - g.Expect(jobRespBody).To(ContainSubstring("COMPLETE")) - }).Should(Succeed()) + expectJobCompletes(resp) var servicePlans resourceList[resource] resp, err = adminClient.R().SetResult(&servicePlans).Get("/v3/service_plans") @@ -165,14 +153,7 @@ var _ = Describe("Service Brokers", func() { HaveRestyStatusCode(http.StatusAccepted), HaveRestyHeaderWithValue("Location", ContainSubstring("/v3/jobs/service_broker.delete~")), )) - - jobURL := resp.Header().Get("Location") - Eventually(func(g Gomega) { - resp, err = adminClient.R().Get(jobURL) - g.Expect(err).NotTo(HaveOccurred()) - jobRespBody := string(resp.Body()) - g.Expect(jobRespBody).To(ContainSubstring("COMPLETE")) - }).Should(Succeed()) + expectJobCompletes(resp) }) }) }) diff --git a/tests/e2e/service_instances_test.go b/tests/e2e/service_instances_test.go index 5de9c2805..4dd607e7b 100644 --- a/tests/e2e/service_instances_test.go +++ b/tests/e2e/service_instances_test.go @@ -118,13 +118,7 @@ var _ = Describe("Service Instances", func() { 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()) + expectJobCompletes(httpResp) }) }) }) @@ -204,13 +198,7 @@ var _ = Describe("Service Instances", func() { 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()) + expectJobCompletes(httpResp) }) }) }) @@ -285,13 +273,8 @@ func createManagedServiceInstance(brokerGUID, spaceGUID string) string { 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()) + expectJobCompletes(httpResp) return strings.Split(jobURL, "~")[1] } diff --git a/tests/e2e/spaces_test.go b/tests/e2e/spaces_test.go index 063bf30fd..ab1658373 100644 --- a/tests/e2e/spaces_test.go +++ b/tests/e2e/spaces_test.go @@ -157,13 +157,7 @@ var _ = Describe("Spaces", func() { HaveRestyStatusCode(http.StatusAccepted), HaveRestyHeaderWithValue("Location", HaveSuffix("/v3/jobs/space.delete~"+spaceGUID)), )) - - jobURL := resp.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()) + expectJobCompletes(resp) spaceResp, err := adminClient.R().Get("/v3/spaces/" + spaceGUID) Expect(err).NotTo(HaveOccurred()) @@ -239,13 +233,7 @@ var _ = Describe("Spaces", func() { HaveRestyStatusCode(http.StatusAccepted), HaveRestyHeaderWithValue("Location", HaveSuffix("/v3/jobs/space.apply_manifest~"+spaceGUID)), )) - - jobURL := resp.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()) + expectJobCompletes(resp) app1GUID := getAppGUIDFromName(app1Name) @@ -300,13 +288,7 @@ var _ = Describe("Spaces", func() { Expect(requestErr).NotTo(HaveOccurred()) Expect(r).To(HaveRestyStatusCode(http.StatusAccepted)) - - jobURL := r.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()) + expectJobCompletes(r) } BeforeEach(func() { @@ -328,13 +310,7 @@ var _ = Describe("Spaces", func() { It("applies the changes correctly", func() { Expect(resp).To(HaveRestyStatusCode(http.StatusAccepted)) - - jobURL := resp.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()) + expectJobCompletes(resp) app1GUID := getAppGUIDFromName(app1Name) @@ -379,14 +355,7 @@ var _ = Describe("Spaces", func() { HaveRestyStatusCode(http.StatusAccepted), HaveRestyHeaderWithValue("Location", HaveSuffix("/v3/jobs/space.apply_manifest~"+spaceGUID)), )) - - jobURL := resp.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")) - Expect(getAppGUIDFromName(app1Name)).NotTo(BeEmpty()) - }).Should(Succeed()) + expectJobCompletes(resp) app1GUID := getAppGUIDFromName(app1Name) app1 := getApp(app1GUID) diff --git a/tests/helpers/broker/broker.go b/tests/helpers/broker/broker.go index dfa0b180b..407a9070e 100644 --- a/tests/helpers/broker/broker.go +++ b/tests/helpers/broker/broker.go @@ -31,14 +31,14 @@ func (b *BrokerServer) WithResponse(pattern string, response map[string]any, sta respBytes, err := json.Marshal(response) Expect(err).NotTo(HaveOccurred()) - return b.WithHandler(pattern, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + return b.withHandler(pattern, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(statusCode) _, err := w.Write(respBytes) Expect(err).NotTo(HaveOccurred()) })) } -func (b *BrokerServer) WithHandler(pattern string, handler http.Handler) *BrokerServer { +func (b *BrokerServer) withHandler(pattern string, handler http.Handler) *BrokerServer { b.handlers[pattern] = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { bodyBytes, err := io.ReadAll(r.Body) Expect(err).NotTo(HaveOccurred()) diff --git a/tests/smoke/unbind_service_test.go b/tests/smoke/unbind_service_test.go new file mode 100644 index 000000000..83806be75 --- /dev/null +++ b/tests/smoke/unbind_service_test.go @@ -0,0 +1,59 @@ +package smoke_test + +import ( + "code.cloudfoundry.org/korifi/tests/helpers" + + "github.com/google/uuid" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + . "github.com/onsi/gomega/gexec" +) + +var _ = Describe("cf unbind-service", func() { + var ( + serviceName string + unbindSession *Session + ) + BeforeEach(func() { + serviceName = uuid.NewString() + }) + + JustBeforeEach(func() { + unbindSession = helpers.Cf("unbind-service", sharedData.BuildpackAppName, serviceName) + }) + + Describe("Unbinding from user-provided service instances", func() { + BeforeEach(func() { + Expect( + helpers.Cf("create-user-provided-service", serviceName, "-p", `{"key1":"value1","key2":"value2"}`), + ).To(Exit(0)) + Expect(helpers.Cf("bind-service", sharedData.BuildpackAppName, serviceName)).To(Exit(0)) + }) + + It("succeeds", func() { + Expect(unbindSession).To(Exit(0)) + }) + }) + + Describe("Uninding from managed service instances", func() { + BeforeEach(func() { + brokerName := uuid.NewString() + Expect(helpers.Cf( + "create-service-broker", + brokerName, + "broker-user", + "broker-password", + helpers.GetInClusterURL(getAppGUID(sharedData.BrokerAppName)), + )).To(Exit(0)) + + Expect(helpers.Cf("enable-service-access", "sample-service", "-b", brokerName)).To(Exit(0)) + session := helpers.Cf("create-service", "sample-service", "sample", serviceName, "-b", brokerName) + Expect(session).To(Exit(0)) + Expect(helpers.Cf("bind-service", sharedData.BuildpackAppName, serviceName)).To(Exit(0)) + }) + + It("succeeds", func() { + Expect(unbindSession).To(Exit(0)) + }) + }) +})