Skip to content

Commit

Permalink
Prevent duplicate requests to delete backing services
Browse files Browse the repository at this point in the history 
Backing services and bound services are compared and any instance with the
same service instance name and target space are considered duplicates.
  • Loading branch information
@royclarkson
royclarkson committed May 14, 2020
1 parent 70bdae5 commit 1160fc4
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 28 deletions.
8 changes: 8 additions & 0 deletions ...roker-core/src/main/java/org/springframework/cloud/appbroker/deployer/BackingService.java
View file
Expand Up @@ -120,6 +120,14 @@ public void setRebindOnUpdate(boolean rebindOnUpdate) {
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) {
Expand Down
50 changes: 35 additions & 15 deletions ...amework/cloud/appbroker/workflow/instance/AppDeploymentDeleteServiceInstanceWorkflow.java
View file
Expand Up @@ -16,7 +16,7 @@

package org.springframework.cloud.appbroker.workflow.instance;

import java.util.List;
import java.util.Collections;

import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
Expand All @@ -27,6 +27,7 @@
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;
Expand All @@ -35,6 +36,7 @@
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
Expand Down Expand Up @@ -75,34 +77,52 @@ public Mono<Void> delete(DeleteServiceInstanceRequest request, DeleteServiceInst
}

private Flux<String> 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'",
request.getServiceDefinition().getName(), request.getPlan().getName(), exception.getMessage()),
exception));
}

private Flux<List<BackingService>> collectConfiguredBackingServices(DeleteServiceInstanceRequest request) {
private Flux<BackingService> collectBackingServices(DeleteServiceInstanceRequest request) {
return collectConfiguredBackingServices(request)
.concatWith(collectBoundBackingServices(request))
.distinct(BackingService::serviceInstanceNameAndSpaceHashCode);
}

private Flux<BackingService> collectConfiguredBackingServices(DeleteServiceInstanceRequest request) {
return getBackingServicesForService(request.getServiceDefinition(), request.getPlan())
.flatMapMany(backingServices -> getTargetForService(request.getServiceDefinition(), request.getPlan())
.flatMap(backingServices -> getTargetForService(request.getServiceDefinition(), request.getPlan())
.flatMap(targetSpec -> targetService.addToBackingServices(backingServices, targetSpec,
request.getServiceInstanceId()))
.defaultIfEmpty(backingServices));
.defaultIfEmpty(backingServices))
.flatMapMany(Flux::fromIterable);
}

private Flux<List<BackingService>> collectBoundBackingServices(DeleteServiceInstanceRequest request) {
private Flux<BackingService> 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 -> {
BackingService backingService = BackingService.builder()
.serviceInstanceName(serviceInstanceName)
.build();
if (!CollectionUtils.isEmpty(backingApplication.getProperties())) {
backingService.setProperties(Collections.singletonMap(DeploymentProperties.TARGET_PROPERTY_KEY,
backingApplication.getProperties().get(DeploymentProperties.TARGET_PROPERTY_KEY)));
}
return backingService;
}));
}

private Flux<String> undeployBackingApplications(DeleteServiceInstanceRequest request) {
Expand Down
54 changes: 41 additions & 13 deletions ...ork/cloud/appbroker/workflow/instance/AppDeploymentDeleteServiceInstanceWorkflowTest.java
View file
Expand Up @@ -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;
Expand All @@ -34,6 +36,7 @@
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;
Expand Down Expand Up @@ -74,6 +77,8 @@ class AppDeploymentDeleteServiceInstanceWorkflowTest {

private BackingServices backingServices;

private BackingServices backingServices2;

private TargetSpec targetSpec;

private DeleteServiceInstanceWorkflow deleteServiceInstanceWorkflow;
Expand Down Expand Up @@ -104,7 +109,21 @@ void setUp() {
.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
Expand All @@ -121,6 +140,13 @@ void setUp() {
.services(backingServices)
.target(targetSpec)
.build())
.service(BrokeredService.builder()
.serviceName("service3")
.planName("plan3")
.apps(backingApps)
.services(backingServices2)
.target(targetSpec)
.build())
.build();

deleteServiceInstanceWorkflow =
Expand Down Expand Up @@ -165,22 +191,21 @@ void deleteServiceInstanceWithDeployedAppsAndBoundServicesSucceeds() {
.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())))
Expand All @@ -191,19 +216,22 @@ void deleteServiceInstanceSucceedsWhenBackingServicesDifferFromConfiguration() {
.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))
.expectNext()
.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();
}

Expand All @@ -213,7 +241,7 @@ void deleteServiceInstanceWithWithNoAppsDoesNothing() {
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))
Expand Down

0 comments on commit 1160fc4

Please sign in to comment.