From d0ea13f2e6b9877be66ab73a322d111ca07b362a Mon Sep 17 00:00:00 2001 From: Roy Clarkson Date: Thu, 14 May 2020 16:10:50 -0400 Subject: [PATCH] Prevent duplicate requests to delete backing services Backing services and bound services are compared and any instance with the same service instance name and target space are considered duplicates. --- .../appbroker/deployer/BackingService.java | 8 +++ ...ploymentDeleteServiceInstanceWorkflow.java | 59 +++++++++++++------ ...mentDeleteServiceInstanceWorkflowTest.java | 54 +++++++++++++---- 3 files changed, 90 insertions(+), 31 deletions(-) diff --git a/spring-cloud-app-broker-core/src/main/java/org/springframework/cloud/appbroker/deployer/BackingService.java b/spring-cloud-app-broker-core/src/main/java/org/springframework/cloud/appbroker/deployer/BackingService.java index a82d72e..09c6386 100644 --- a/spring-cloud-app-broker-core/src/main/java/org/springframework/cloud/appbroker/deployer/BackingService.java +++ b/spring-cloud-app-broker-core/src/main/java/org/springframework/cloud/appbroker/deployer/BackingService.java @@ -120,6 +120,14 @@ public class BackingService { this.rebindOnUpdate = rebindOnUpdate; } + public int serviceInstanceNameAndSpaceHashCode() { + String space = null; + if (!CollectionUtils.isEmpty(properties)) { + space = properties.get(DeploymentProperties.TARGET_PROPERTY_KEY); + } + return Objects.hash(serviceInstanceName, space); + } + @Override public boolean equals(Object o) { if (this == o) { diff --git a/spring-cloud-app-broker-core/src/main/java/org/springframework/cloud/appbroker/workflow/instance/AppDeploymentDeleteServiceInstanceWorkflow.java b/spring-cloud-app-broker-core/src/main/java/org/springframework/cloud/appbroker/workflow/instance/AppDeploymentDeleteServiceInstanceWorkflow.java index c95354c..96cd255 100644 --- a/spring-cloud-app-broker-core/src/main/java/org/springframework/cloud/appbroker/workflow/instance/AppDeploymentDeleteServiceInstanceWorkflow.java +++ b/spring-cloud-app-broker-core/src/main/java/org/springframework/cloud/appbroker/workflow/instance/AppDeploymentDeleteServiceInstanceWorkflow.java @@ -16,7 +16,8 @@ package org.springframework.cloud.appbroker.workflow.instance; -import java.util.List; +import java.util.Collections; +import java.util.Map; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -27,6 +28,7 @@ import org.springframework.cloud.appbroker.deployer.BackingAppDeploymentService; import org.springframework.cloud.appbroker.deployer.BackingService; import org.springframework.cloud.appbroker.deployer.BackingServicesProvisionService; import org.springframework.cloud.appbroker.deployer.BrokeredServices; +import org.springframework.cloud.appbroker.deployer.DeploymentProperties; import org.springframework.cloud.appbroker.extensions.credentials.CredentialProviderService; import org.springframework.cloud.appbroker.extensions.targets.TargetService; import org.springframework.cloud.appbroker.manager.BackingAppManagementService; @@ -35,6 +37,7 @@ import org.springframework.cloud.servicebroker.model.instance.DeleteServiceInsta import org.springframework.cloud.servicebroker.model.instance.DeleteServiceInstanceResponse; import org.springframework.cloud.servicebroker.model.instance.DeleteServiceInstanceResponse.DeleteServiceInstanceResponseBuilder; import org.springframework.core.annotation.Order; +import org.springframework.util.CollectionUtils; @Order(0) public class AppDeploymentDeleteServiceInstanceWorkflow @@ -75,11 +78,14 @@ public class AppDeploymentDeleteServiceInstanceWorkflow } private Flux deleteBackingServices(DeleteServiceInstanceRequest request) { - return collectConfiguredBackingServices(request) - .mergeWith(collectBoundBackingServices(request)) - .doOnEach(backingServices -> log.debug("Deleting backing services {} for {}/{}", - backingServices, request.getServiceDefinition().getName(), request.getPlan().getName())) - .flatMap(backingServicesProvisionService::deleteServiceInstance) + return collectBackingServices(request) + .collectList() + .flatMapMany(backingServices -> { + if (!CollectionUtils.isEmpty(backingServices)) { + return backingServicesProvisionService.deleteServiceInstance(backingServices); + } + return Flux.empty(); + }) .doOnComplete(() -> log.debug("Finished deleting backing services for {}/{}", request.getServiceDefinition().getName(), request.getPlan().getName())) .doOnError(exception -> log.error(String.format("Error deleting backing services for %s/%s with error '%s'", @@ -87,22 +93,39 @@ public class AppDeploymentDeleteServiceInstanceWorkflow exception)); } - private Flux> collectConfiguredBackingServices(DeleteServiceInstanceRequest request) { - return getBackingServicesForService(request.getServiceDefinition(), request.getPlan()) - .flatMapMany(backingServices -> getTargetForService(request.getServiceDefinition(), request.getPlan()) - .flatMap(targetSpec -> targetService.addToBackingServices(backingServices, targetSpec, - request.getServiceInstanceId())) - .defaultIfEmpty(backingServices)); + private Flux collectBackingServices(DeleteServiceInstanceRequest request) { + return collectConfiguredBackingServices(request) + .concatWith(collectBoundBackingServices(request)) + .distinct(BackingService::serviceInstanceNameAndSpaceHashCode); } - private Flux> collectBoundBackingServices(DeleteServiceInstanceRequest request) { + private Flux collectConfiguredBackingServices(DeleteServiceInstanceRequest request) { + return getBackingServicesForService(request.getServiceDefinition(), request.getPlan()) + .flatMap(backingServices -> getTargetForService(request.getServiceDefinition(), request.getPlan()) + .flatMap(targetSpec -> targetService.addToBackingServices(backingServices, targetSpec, + request.getServiceInstanceId())) + .defaultIfEmpty(backingServices)) + .flatMapMany(Flux::fromIterable); + } + + private Flux collectBoundBackingServices(DeleteServiceInstanceRequest request) { return backingAppManagementService.getDeployedBackingApplications(request.getServiceInstanceId()) .flatMapMany(Flux::fromIterable) - .flatMap(backingApplication -> Flux.fromIterable(backingApplication.getServices()) - .map(servicesSpec -> BackingService.builder() - .serviceInstanceName(servicesSpec.getServiceInstanceName()) - .build()) - .collectList()); + .flatMap(backingApplication -> Mono.justOrEmpty(backingApplication.getServices()) + .flatMapMany(Flux::fromIterable) + .flatMap(servicesSpec -> Mono.justOrEmpty(servicesSpec.getServiceInstanceName())) + .map(serviceInstanceName -> { + Map properties = null; + if (!CollectionUtils.isEmpty(backingApplication.getProperties())) { + String target = backingApplication.getProperties() + .get(DeploymentProperties.TARGET_PROPERTY_KEY); + properties = Collections.singletonMap(DeploymentProperties.TARGET_PROPERTY_KEY, target); + } + return BackingService.builder() + .serviceInstanceName(serviceInstanceName) + .properties(properties) + .build(); + })); } private Flux undeployBackingApplications(DeleteServiceInstanceRequest request) { diff --git a/spring-cloud-app-broker-core/src/test/java/org/springframework/cloud/appbroker/workflow/instance/AppDeploymentDeleteServiceInstanceWorkflowTest.java b/spring-cloud-app-broker-core/src/test/java/org/springframework/cloud/appbroker/workflow/instance/AppDeploymentDeleteServiceInstanceWorkflowTest.java index 8b3c646..d7eff25 100644 --- a/spring-cloud-app-broker-core/src/test/java/org/springframework/cloud/appbroker/workflow/instance/AppDeploymentDeleteServiceInstanceWorkflowTest.java +++ b/spring-cloud-app-broker-core/src/test/java/org/springframework/cloud/appbroker/workflow/instance/AppDeploymentDeleteServiceInstanceWorkflowTest.java @@ -16,6 +16,8 @@ package org.springframework.cloud.appbroker.workflow.instance; +import java.util.Collections; + import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -34,6 +36,7 @@ import org.springframework.cloud.appbroker.deployer.BackingServices; import org.springframework.cloud.appbroker.deployer.BackingServicesProvisionService; import org.springframework.cloud.appbroker.deployer.BrokeredService; import org.springframework.cloud.appbroker.deployer.BrokeredServices; +import org.springframework.cloud.appbroker.deployer.DeploymentProperties; import org.springframework.cloud.appbroker.deployer.ServicesSpec; import org.springframework.cloud.appbroker.deployer.TargetSpec; import org.springframework.cloud.appbroker.extensions.credentials.CredentialProviderService; @@ -74,6 +77,8 @@ class AppDeploymentDeleteServiceInstanceWorkflowTest { private BackingServices backingServices; + private BackingServices backingServices2; + private TargetSpec targetSpec; private DeleteServiceInstanceWorkflow deleteServiceInstanceWorkflow; @@ -104,7 +109,21 @@ class AppDeploymentDeleteServiceInstanceWorkflowTest { .build()) .build(); - targetSpec = TargetSpec.builder().name("TargetSpace").build(); + this.backingServices2 = BackingServices + .builder() + .backingService(BackingService + .builder() + .name("my-service2") + .plan("a-plan2") + .serviceInstanceName("my-service-instance2") + .properties(Collections.singletonMap(DeploymentProperties.TARGET_PROPERTY_KEY, "my-space2")) + .build()) + .build(); + + targetSpec = TargetSpec.builder() + .name("TargetSpace") + .build(); + BrokeredServices brokeredServices = BrokeredServices .builder() .service(BrokeredService @@ -121,6 +140,13 @@ class AppDeploymentDeleteServiceInstanceWorkflowTest { .services(backingServices) .target(targetSpec) .build()) + .service(BrokeredService.builder() + .serviceName("service3") + .planName("plan3") + .apps(backingApps) + .services(backingServices2) + .target(targetSpec) + .build()) .build(); deleteServiceInstanceWorkflow = @@ -165,22 +191,21 @@ class AppDeploymentDeleteServiceInstanceWorkflowTest { .expectNext() .verifyComplete(); - // the same service is a configured backing service, and it is bound to two different deployed apps - verify(this.backingServicesProvisionService, Mockito.times(3)).deleteServiceInstance(any()); + verify(this.backingServicesProvisionService, Mockito.times(1)).deleteServiceInstance(any()); verifyNoMoreInteractionsWithServices(); } @Test void deleteServiceInstanceSucceedsWhenBackingServicesDifferFromConfiguration() { - DeleteServiceInstanceRequest request = buildRequest("service1", "plan1"); + DeleteServiceInstanceRequest request = buildRequest("service3", "plan3"); DeleteServiceInstanceResponse response = DeleteServiceInstanceResponse.builder().build(); given(this.backingAppDeploymentService.undeploy(eq(backingApps))) .willReturn(Flux.just("undeployed1", "undeployed2")); // configured backing services - given(this.targetService.addToBackingServices(eq(backingServices), eq(targetSpec), eq("service-instance-id"))) - .willReturn(Mono.just(backingServices)); + given(this.targetService.addToBackingServices(eq(backingServices2), eq(targetSpec), eq("service-instance-id"))) + .willReturn(Mono.just(backingServices2)); // different bound services given(this.backingAppManagementService.getDeployedBackingApplications(eq(request.getServiceInstanceId()))) @@ -191,10 +216,14 @@ class AppDeploymentDeleteServiceInstanceWorkflowTest { .willReturn(Mono.just(backingApps)); given(this.backingServicesProvisionService.deleteServiceInstance(argThat(backingServices -> { - boolean nameMatch1 = "different-service-instance".equals(backingServices.get(0).getServiceInstanceName()); - boolean nameMatch2 = "my-service-instance".equals(backingServices.get(0).getServiceInstanceName()); - boolean sizeMatch = backingServices.size() == 1; - return sizeMatch && (nameMatch1 || nameMatch2); + boolean nameMatch0 = "my-service-instance2".equals(backingServices.get(0).getServiceInstanceName()); + boolean spaceMatch0 = "my-space2".equals(backingServices.get(0).getProperties() + .get(DeploymentProperties.TARGET_PROPERTY_KEY)); + boolean nameMatch1 = "different-service-instance".equals(backingServices.get(1).getServiceInstanceName()); + boolean spaceMatch1 = "TargetSpace".equals(backingServices.get(1).getProperties() + .get(DeploymentProperties.TARGET_PROPERTY_KEY)); + boolean sizeMatch = backingServices.size() == 2; + return sizeMatch && (nameMatch0 && spaceMatch0 || nameMatch1 && spaceMatch1); }))).willReturn(Flux.just("different-service-instance")); StepVerifier.create(deleteServiceInstanceWorkflow.delete(request, response)) @@ -202,8 +231,7 @@ class AppDeploymentDeleteServiceInstanceWorkflowTest { .expectNext() .verifyComplete(); - // the same service is a configured backing service, and it is bound to two different deployed apps - verify(this.backingServicesProvisionService, Mockito.times(3)).deleteServiceInstance(any()); + verify(this.backingServicesProvisionService, Mockito.times(1)).deleteServiceInstance(any()); verifyNoMoreInteractionsWithServices(); } @@ -213,7 +241,7 @@ class AppDeploymentDeleteServiceInstanceWorkflowTest { DeleteServiceInstanceResponse response = DeleteServiceInstanceResponse.builder().build(); given(this.backingAppManagementService.getDeployedBackingApplications(eq(request.getServiceInstanceId()))) - .willReturn(Mono.just(BackingApplications.builder().build())); + .willReturn(Mono.empty()); StepVerifier .create(deleteServiceInstanceWorkflow.delete(request, response))