Skip to content

Commit

Permalink
Fix an issue where configured backing services are not being deleted
Browse files Browse the repository at this point in the history
When a service instance is deleted, all configured backing services will
be deleted as well as all services bound to any backing applications

Resolve #344
  • Loading branch information
royclarkson committed May 12, 2020
1 parent d370ae5 commit f09bd96
Show file tree
Hide file tree
Showing 5 changed files with 263 additions and 63 deletions.
Expand Up @@ -16,6 +16,8 @@

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

import java.util.List;

import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
import reactor.util.Logger;
Expand Down Expand Up @@ -73,14 +75,8 @@ public Mono<Void> delete(DeleteServiceInstanceRequest request, DeleteServiceInst
}

private Flux<String> deleteBackingServices(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())
return collectConfiguredBackingServices(request)
.mergeWith(collectBoundBackingServices(request))
.doOnEach(backingServices -> log.debug("Deleting backing services {} for {}/{}",
backingServices, request.getServiceDefinition().getName(), request.getPlan().getName()))
.flatMap(backingServicesProvisionService::deleteServiceInstance)
Expand All @@ -91,6 +87,23 @@ private Flux<String> deleteBackingServices(DeleteServiceInstanceRequest request)
exception));
}

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

private Flux<List<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());
}

private Flux<String> undeployBackingApplications(DeleteServiceInstanceRequest request) {
return getBackingApplicationsForService(request.getServiceDefinition(), request.getPlan())
.flatMap(backingApps ->
Expand Down
Expand Up @@ -20,6 +20,7 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.jupiter.MockitoExtension;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
Expand All @@ -44,9 +45,11 @@
import org.springframework.cloud.servicebroker.model.instance.DeleteServiceInstanceRequest;
import org.springframework.cloud.servicebroker.model.instance.DeleteServiceInstanceResponse;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;

@ExtendWith(MockitoExtension.class)
Expand All @@ -69,6 +72,8 @@ class AppDeploymentDeleteServiceInstanceWorkflowTest {

private BackingApplications backingApps;

private BackingServices backingServices;

private TargetSpec targetSpec;

private DeleteServiceInstanceWorkflow deleteServiceInstanceWorkflow;
Expand All @@ -89,7 +94,7 @@ void setUp() {
.build())
.build();

BackingServices backingServices = BackingServices
this.backingServices = BackingServices
.builder()
.backingService(BackingService
.builder()
Expand All @@ -110,6 +115,12 @@ void setUp() {
.services(backingServices)
.target(targetSpec)
.build())
.service(BrokeredService.builder()
.serviceName("service2")
.planName("plan2")
.services(backingServices)
.target(targetSpec)
.build())
.build();

deleteServiceInstanceWorkflow =
Expand All @@ -123,18 +134,25 @@ void setUp() {
}

@Test
void deleteServiceInstanceSucceeds() {
void deleteServiceInstanceWithDeployedAppsAndBoundServicesSucceeds() {
DeleteServiceInstanceRequest request = buildRequest("service1", "plan1");
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));

// services bound to deployed apps
given(this.backingAppManagementService.getDeployedBackingApplications(eq(request.getServiceInstanceId())))
.willReturn(Mono.just(getExistingBackingAppsWithService("my-service-instance")));
given(this.credentialProviderService.deleteCredentials(eq(backingApps), eq(request.getServiceInstanceId())))
.willReturn(Mono.just(backingApps));
given(this.targetService.addToBackingApplications(eq(backingApps), eq(targetSpec), eq("service-instance-id")))
.willReturn(Mono.just(backingApps));

given(this.backingServicesProvisionService.deleteServiceInstance(argThat(backingServices -> {
boolean nameMatch = "my-service-instance".equals(backingServices.get(0).getServiceInstanceName());
boolean sizeMatch = backingServices.size() == 1;
Expand All @@ -147,6 +165,8 @@ void deleteServiceInstanceSucceeds() {
.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());
verifyNoMoreInteractionsWithServices();
}

Expand All @@ -157,24 +177,33 @@ void deleteServiceInstanceSucceedsWhenBackingServicesDifferFromConfiguration() {

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));

// different bound services
given(this.backingAppManagementService.getDeployedBackingApplications(eq(request.getServiceInstanceId())))
.willReturn(Mono.just(getExistingBackingAppsWithService("different-service-instance")));
given(this.credentialProviderService.deleteCredentials(eq(backingApps), eq(request.getServiceInstanceId())))
.willReturn(Mono.just(backingApps));
given(this.targetService.addToBackingApplications(eq(backingApps), eq(targetSpec), eq("service-instance-id")))
.willReturn(Mono.just(backingApps));

given(this.backingServicesProvisionService.deleteServiceInstance(argThat(backingServices -> {
boolean nameMatch = "different-service-instance".equals(backingServices.get(0).getServiceInstanceName());
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 && nameMatch;
return sizeMatch && (nameMatch1 || nameMatch2);
}))).willReturn(Flux.just("different-service-instance"));

StepVerifier
.create(deleteServiceInstanceWorkflow.delete(request, response))
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());
verifyNoMoreInteractionsWithServices();
}

Expand All @@ -184,7 +213,7 @@ void deleteServiceInstanceWithWithNoAppsDoesNothing() {
DeleteServiceInstanceResponse response = DeleteServiceInstanceResponse.builder().build();

given(this.backingAppManagementService.getDeployedBackingApplications(eq(request.getServiceInstanceId())))
.willReturn(Mono.just(BackingApplications.builder().build()));
.willReturn(Mono.just(BackingApplications.builder().build()));

StepVerifier
.create(deleteServiceInstanceWorkflow.delete(request, response))
Expand All @@ -193,6 +222,36 @@ void deleteServiceInstanceWithWithNoAppsDoesNothing() {
verifyNoMoreInteractionsWithServices();
}

@Test
void deleteServiceInstanceWithOnlyBoundServicesSucceeds() {
DeleteServiceInstanceRequest request = buildRequest("service2", "plan2");
DeleteServiceInstanceResponse response = DeleteServiceInstanceResponse.builder().build();

// configured backing services
given(this.targetService.addToBackingServices(eq(backingServices), eq(targetSpec), eq("service-instance-id")))
.willReturn(Mono.just(backingServices));

// no backing apps
given(this.backingAppManagementService.getDeployedBackingApplications(eq(request.getServiceInstanceId())))
.willReturn(Mono.empty());
given(this.credentialProviderService.deleteCredentials(any(), eq(request.getServiceInstanceId())))
.willReturn(Mono.empty());

given(this.backingServicesProvisionService.deleteServiceInstance(argThat(backingServices -> {
boolean nameMatch = "my-service-instance".equals(backingServices.get(0).getServiceInstanceName());
boolean sizeMatch = backingServices.size() == 1;
return sizeMatch && nameMatch;
}))).willReturn(Flux.just("my-service-instance"));

StepVerifier.create(deleteServiceInstanceWorkflow.delete(request, response))
.expectNext()
.expectNext()
.verifyComplete();

verify(this.backingServicesProvisionService, Mockito.times(1)).deleteServiceInstance(any());
verifyNoMoreInteractionsWithServices();
}

private void verifyNoMoreInteractionsWithServices() {
verifyNoMoreInteractions(this.backingServicesProvisionService);
verifyNoMoreInteractions(this.backingAppDeploymentService);
Expand Down
@@ -0,0 +1,159 @@
/*
* Copyright 2002-2020 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.cloud.appbroker.integration;

import org.junit.jupiter.api.Test;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.cloud.appbroker.integration.fixtures.CloudControllerStubFixture;
import org.springframework.cloud.appbroker.integration.fixtures.OpenServiceBrokerApiFixture;
import org.springframework.cloud.servicebroker.model.instance.OperationState;
import org.springframework.http.HttpStatus;
import org.springframework.test.context.TestPropertySource;

import static io.restassured.RestAssured.given;
import static org.assertj.core.api.Assertions.assertThat;
import static org.hamcrest.Matchers.either;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.springframework.cloud.appbroker.integration.DeleteInstanceWithAppsAndServicesComponentTest.APP_NAME;
import static org.springframework.cloud.appbroker.integration.DeleteInstanceWithAppsAndServicesComponentTest.BACKING_PLAN_NAME;
import static org.springframework.cloud.appbroker.integration.DeleteInstanceWithAppsAndServicesComponentTest.BACKING_SERVICE_NAME;
import static org.springframework.cloud.appbroker.integration.DeleteInstanceWithAppsAndServicesComponentTest.BACKING_SI_NAME;
import static org.springframework.cloud.appbroker.integration.DeleteInstanceWithAppsAndServicesComponentTest.PLAN_NAME;
import static org.springframework.cloud.appbroker.integration.DeleteInstanceWithAppsAndServicesComponentTest.SERVICE_NAME;

@TestPropertySource(properties = {
"spring.cloud.appbroker.services[0].service-name=" + SERVICE_NAME,
"spring.cloud.appbroker.services[0].plan-name=" + PLAN_NAME,
"spring.cloud.appbroker.services[0].apps[0].path=classpath:demo.jar",
"spring.cloud.appbroker.services[0].apps[0].name=" + APP_NAME,
"spring.cloud.appbroker.services[0].apps[0].services[0].service-instance-name=" + BACKING_SI_NAME,
"spring.cloud.appbroker.services[0].services[0].service-instance-name=" + BACKING_SI_NAME,
"spring.cloud.appbroker.services[0].services[0].name=" + BACKING_SERVICE_NAME,
"spring.cloud.appbroker.services[0].services[0].plan=" + BACKING_PLAN_NAME
})
class DeleteInstanceWithAppsAndServicesComponentTest extends WiremockComponentTest {

protected static final String APP_NAME = "app-delete-with-services";

protected static final String SERVICE_NAME = "example";

protected static final String PLAN_NAME = "standard";

protected static final String BACKING_SI_NAME = "my-db-service";

protected static final String BACKING_SERVICE_NAME = "db-service";

protected static final String BACKING_PLAN_NAME = "backing-standard";

@Autowired
private OpenServiceBrokerApiFixture brokerFixture;

@Autowired
private CloudControllerStubFixture cloudControllerFixture;

@Test
void deleteAppsAndServicesWhenTheyExist() {
cloudControllerFixture.stubGetServiceInstanceWithNoBinding("instance-id", "instance-name",
SERVICE_NAME, PLAN_NAME);
cloudControllerFixture.stubAppExistsWithBackingService(APP_NAME, BACKING_SI_NAME,
BACKING_SERVICE_NAME, BACKING_PLAN_NAME);
cloudControllerFixture.stubServiceBindingDoesNotExist(APP_NAME);
cloudControllerFixture.stubDeleteApp(APP_NAME);

cloudControllerFixture.stubGetBackingServiceInstance(BACKING_SI_NAME, BACKING_SERVICE_NAME, BACKING_PLAN_NAME);

cloudControllerFixture.stubServiceBindingExists(APP_NAME, BACKING_SI_NAME);
cloudControllerFixture.stubDeleteServiceBinding(APP_NAME, BACKING_SI_NAME);
cloudControllerFixture.stubDeleteServiceInstance(BACKING_SI_NAME);

// when the service instance is deleted
given(brokerFixture.serviceInstanceRequest())
.when()
.delete(brokerFixture.deleteServiceInstanceUrl(), "instance-id")
.then()
.statusCode(HttpStatus.ACCEPTED.value());

// when the "last_operation" API is polled
given(brokerFixture.serviceInstanceRequest())
.when()
.get(brokerFixture.getLastInstanceOperationUrl(), "instance-id")
.then()
.statusCode(HttpStatus.OK.value())
.body("state", is(equalTo(OperationState.IN_PROGRESS.toString())));

String state = brokerFixture.waitForAsyncOperationComplete("instance-id");
assertThat(state).isEqualTo(OperationState.SUCCEEDED.toString());
}

@Test
void deleteAppsWhenTheyExistAndServicesWhenTheyDoNotExist() {
cloudControllerFixture.stubGetServiceInstanceWithNoBinding("instance-id", "instance-name",
SERVICE_NAME, PLAN_NAME);
cloudControllerFixture.stubAppExists(APP_NAME);
cloudControllerFixture.stubServiceBindingDoesNotExist(APP_NAME);
cloudControllerFixture.stubDeleteApp(APP_NAME);

// when the service instance is deleted
given(brokerFixture.serviceInstanceRequest())
.when()
.delete(brokerFixture.deleteServiceInstanceUrl(), "instance-id")
.then()
.statusCode(HttpStatus.ACCEPTED.value());

// when the "last_operation" API is polled
given(brokerFixture.serviceInstanceRequest())
.when()
.get(brokerFixture.getLastInstanceOperationUrl(), "instance-id")
.then()
.statusCode(HttpStatus.OK.value())
.body("state", is(equalTo(OperationState.IN_PROGRESS.toString())));

String state = brokerFixture.waitForAsyncOperationComplete("instance-id");
assertThat(state).isEqualTo(OperationState.SUCCEEDED.toString());
}

@Test
void deleteAppsAndServicesWhenTheyDoNotExist() {
cloudControllerFixture.stubGetServiceInstanceWithNoBinding("instance-id", "instance-name",
SERVICE_NAME, PLAN_NAME);
cloudControllerFixture.stubAppDoesNotExist(APP_NAME);

// when the service instance is deleted
given(brokerFixture.serviceInstanceRequest())
.when()
.delete(brokerFixture.deleteServiceInstanceUrl(), "instance-id")
.then()
.statusCode(HttpStatus.ACCEPTED.value());

// when the "last_operation" API is polled
given(brokerFixture.serviceInstanceRequest())
.when()
.get(brokerFixture.getLastInstanceOperationUrl(), "instance-id")
.then()
.statusCode(HttpStatus.OK.value())
.body("state",
either(equalTo(OperationState.IN_PROGRESS.toString()))
// if the error occurs immediately it will return succeeded status
.or(equalTo(OperationState.SUCCEEDED.toString())));

String state = brokerFixture.waitForAsyncOperationComplete("instance-id");
assertThat(state).isEqualTo(OperationState.SUCCEEDED.toString());
}

}

0 comments on commit f09bd96

Please sign in to comment.