Skip to content

Commit 09ff14b

Browse files
gberche-orangegberche-orange
committed
Handle race conditions (including for K8S dups): handle dupl sync update + handle invalid plan/service-definion id
 * [x] Test update https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#response-5 * [x] Implement update error recovery * Check si, if an update operation is in progress, then return 202 accepted, * otherwise 500 bad request if update request was previously accepted * otherwise 400 bad request otherwise * OSB api v2.16 will be supporting error details such as "usable", see cloudfoundry/servicebroker#661 * But not yet implemented in CF API v3 http://v3-apidocs.cloudfoundry.org/version/3.83.0/index.html#create-a-service-instance * Nor in SC-OSB * [x] Refine USI sync success test: sync update will trigger a new backing update and not enter error recovery branch, just perform the update twise * [x] OSB provision dupl same SI: check same dupl receives right status * [x] 200 Ok as backing service was completed * [x] Check UpdateAsyncInstanceWithBackingServiceAcceptanceTest does * [x] USI * [x] OSB provision dupl same SI: check same dupl receives right status * [x] 202 accepted, and then 200 * [x] New concurrent stalled Update async test (ConcurrentAsyncUpdateInstanceWithBackingServiceAcceptanceTest) that does * [x] New interceptor StalledAsyncUpdate * [x] USI * [x] OSB provision dupl same SI: check same dupl receives right status * [x] 202 Accepted as backing service is still in progress * [x] OSB update with invalid plan * [x] 400 Bad request
1 parent 0aaaac8 commit 09ff14b

File tree

11 files changed

+261
-16
lines changed

11 files changed

+261
-16
lines changed

‎osb-cmdb/docs/impl-notes/TODO-cmdb-native.md‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
* [ ] Handle race conditions (including for K8S dups)
2626
* [ ] Refactor race condition support
2727
* [ ] extract concurrent exception handler in its collaborator object to unit test it
28-
* [ ] Test update https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#response-5
28+
* [x] Test update https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#response-5
2929
* [x] Implement update error recovery
3030
* Check si, if an update operation is in progress, then return 202 accepted,
3131
* otherwise 500 bad request if update request was previously accepted
@@ -36,17 +36,17 @@
3636
* [x] Refine USI sync success test: sync update will trigger a new backing update and not enter error recovery branch, just perform the update twise
3737
* [x] OSB provision dupl same SI: check same dupl receives right status
3838
* [x] 200 Ok as backing service was completed
39-
* [ ] Check UpdateAsyncInstanceWithBackingServiceAcceptanceTest does
40-
* [ ] USI
41-
* [ ] OSB provision dupl same SI: check same dupl receives right status
42-
* [ ] 410 gone
43-
* [ ] New concurrent stalled Update async test that does
44-
* [ ] New interceptor StalledAsyncUpdate
39+
* [x] Check UpdateAsyncInstanceWithBackingServiceAcceptanceTest does
40+
* [x] USI
41+
* [x] OSB provision dupl same SI: check same dupl receives right status
42+
* [x] 202 accepted, and then 200
43+
* [x] New concurrent stalled Update async test (ConcurrentAsyncUpdateInstanceWithBackingServiceAcceptanceTest) that does
44+
* [x] New interceptor StalledAsyncUpdate
4545
* [ ] USI
46-
* [ ] OSB provision dupl same SI: check same dupl receives right status
47-
* [ ] 202 Accepted as backing service is still in progress
48-
* [ ] OSB provision dupl different SI: check different dupl receives right status
49-
* [ ] 409 Conflict
46+
* [x] OSB provision dupl same SI: check same dupl receives right status
47+
* [x] 202 Accepted as backing service is still in progress
48+
* [x] OSB update with invalid plan
49+
* [x] 400 Bad request
5050
* [ ] Test bind https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#response-3
5151
* [ ] New interceptor StalledAsyncBind
5252
* [ ] Refine CSI sync success test

‎osb-cmdb/src/main/java/com/orange/oss/osbcmdb/OsbCmdbBrokerConfiguration.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import com.orange.oss.osbcmdb.testfixtures.ASyncFailedCreateBackingSpaceInstanceInterceptor;
1010
import com.orange.oss.osbcmdb.testfixtures.ASyncStalledCreateBackingSpaceInstanceInterceptor;
1111
import com.orange.oss.osbcmdb.testfixtures.ASyncStalledDeleteBackingSpaceInstanceInterceptor;
12+
import com.orange.oss.osbcmdb.testfixtures.ASyncStalledUpdateBackingSpaceInstanceInterceptor;
1213
import com.orange.oss.osbcmdb.testfixtures.AsyncFailedDeleteBackingSpaceInstanceInterceptor;
1314
import com.orange.oss.osbcmdb.testfixtures.AsyncFailedUpdateBackingSpaceInstanceInterceptor;
1415
import com.orange.oss.osbcmdb.testfixtures.AsyncSuccessfulCreateUpdateDeleteBackingSpaceInstanceInterceptor;
@@ -111,6 +112,13 @@ public ServiceInstanceInterceptor acceptanceTestASyncStalledDeleteBackingSpaceIn
111112
return new ASyncStalledDeleteBackingSpaceInstanceInterceptor(targetProperties.getDefaultSpace());
112113
}
113114

115+
@Bean
116+
@Profile("acceptanceTests & ASyncStalledUpdateBackingSpaceInstanceInterceptor")
117+
public ServiceInstanceInterceptor acceptanceTestASyncStalledUpdateBackingSpaceInstanceInterceptor(
118+
CloudFoundryTargetProperties targetProperties) {
119+
return new ASyncStalledUpdateBackingSpaceInstanceInterceptor(targetProperties.getDefaultSpace());
120+
}
121+
114122
@Bean
115123
@Profile("acceptanceTests")
116124
@ConditionalOnMissingBean

‎osb-cmdb/src/main/java/com/orange/oss/osbcmdb/serviceinstance/OsbCmdbServiceInstance.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,8 @@ public Mono<CreateServiceInstanceResponse> createServiceInstance(CreateServiceIn
105105
if (osbInterceptor != null && osbInterceptor.accept(request)) {
106106
return osbInterceptor.createServiceInstance(request);
107107
}
108+
validateServiceDefinitionAndPlanIds(request.getServiceDefinition(), request.getPlan(),
109+
request.getServiceDefinitionId(), request.getPlanId());
108110
String backingServiceName = request.getServiceDefinition().getName();
109111
String backingServicePlanName = request.getPlan().getName();
110112
CloudFoundryOperations spacedTargetedOperations = null;
@@ -689,6 +691,8 @@ public Mono<UpdateServiceInstanceResponse> updateServiceInstance(UpdateServiceIn
689691
return osbInterceptor.updateServiceInstance(request);
690692
}
691693

694+
validateServiceDefinitionAndPlanIds(request.getServiceDefinition(), request.getPlan(),
695+
request.getServiceDefinitionId(), request.getPlanId());
692696
String backingServiceName = request.getServiceDefinition().getName();
693697
String backingServicePlanName = request.getPlan().getName();
694698
String backingServiceInstanceName = ServiceInstanceNameHelper.truncateNameToCfMaxSize(request.getServiceInstanceId());
@@ -754,6 +758,19 @@ public Mono<UpdateServiceInstanceResponse> updateServiceInstance(UpdateServiceIn
754758
return Mono.just(responseBuilder.build());
755759
}
756760

761+
private void validateServiceDefinitionAndPlanIds(ServiceDefinition serviceDefinition, Plan plan,
762+
String serviceDefinitionId,
763+
String planId) {
764+
if (plan == null) {
765+
LOG.info("Invalid plan received with unknown id {}", planId);
766+
throw new ServiceBrokerInvalidParametersException("Invalid plan received with unknown id:" + planId);
767+
}
768+
if (serviceDefinition == null) {
769+
LOG.info("Invalid service definition received with unknown id {}", serviceDefinitionId);
770+
throw new ServiceBrokerInvalidParametersException("Invalid service definition received with unknown id:" + serviceDefinitionId);
771+
}
772+
}
773+
757774
protected CmdbOperationState fromJson(String operation) {
758775
try {
759776
return OBJECT_MAPPER.readValue(operation, CmdbOperationState.class);

‎osb-cmdb/src/main/java/com/orange/oss/osbcmdb/testfixtures/ASyncStalledUpdateBackingSpaceInstanceInterceptor.java‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
package com.orange.oss.osbcmdb.testfixtures;
2+
3+
import com.orange.oss.osbcmdb.serviceinstance.ServiceInstanceInterceptor;
4+
import reactor.core.publisher.Mono;
5+
import reactor.util.Logger;
6+
import reactor.util.Loggers;
7+
8+
import org.springframework.cloud.servicebroker.model.instance.GetLastServiceOperationRequest;
9+
import org.springframework.cloud.servicebroker.model.instance.GetLastServiceOperationResponse;
10+
import org.springframework.cloud.servicebroker.model.instance.OperationState;
11+
import org.springframework.cloud.servicebroker.model.instance.UpdateServiceInstanceRequest;
12+
import org.springframework.cloud.servicebroker.model.instance.UpdateServiceInstanceResponse;
13+
14+
/**
15+
* Simulates a failed async backing service update requested in backing space: create/delete always succeeds sync,
16+
* update always hangs (asynch)
17+
*
18+
* Only accept OSB calls when space is a backing space, i.e. not the default space
19+
*/
20+
public class ASyncStalledUpdateBackingSpaceInstanceInterceptor extends BaseServiceInstanceBackingSpaceInstanceInterceptor
21+
implements ServiceInstanceInterceptor {
22+
23+
private static final Logger LOG = Loggers.getLogger(ASyncStalledUpdateBackingSpaceInstanceInterceptor.class);
24+
25+
public ASyncStalledUpdateBackingSpaceInstanceInterceptor(String defaultSpaceName) {
26+
super(defaultSpaceName);
27+
}
28+
29+
@Override
30+
public Mono<UpdateServiceInstanceResponse> updateServiceInstance(UpdateServiceInstanceRequest request) {
31+
provisionnedServiceInstanceGuids.add(request.getServiceInstanceId());
32+
return Mono.just(UpdateServiceInstanceResponse.builder()
33+
.async(true)
34+
.build());
35+
}
36+
37+
@Override
38+
public Mono<GetLastServiceOperationResponse> getLastOperation(GetLastServiceOperationRequest request) {
39+
return Mono.just(GetLastServiceOperationResponse.builder()
40+
.operationState(OperationState.IN_PROGRESS)
41+
.deleteOperation(false)
42+
.description(this.getClass().getSimpleName())
43+
.build());
44+
}
45+
46+
}

‎osb-cmdb/src/main/java/com/orange/oss/osbcmdb/testfixtures/BaseBackingSpaceInstanceInterceptor.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@ public BaseBackingSpaceInstanceInterceptor(
3030
protected boolean isScabAcceptanceTest(Context context, String requestToString) {
3131
CloudFoundryContext cloudFoundryContext = (CloudFoundryContext) context;
3232
if (context == null) {
33-
LOG.info("No context specified in request, assuming not an acceptance test with a CF compliant client, " +
34-
"therefore I'm not accepting the request");
33+
LOG.info("No context specified in request, assuming not an acceptance test sending OSB request with a " +
34+
"compliant CF CC-NG client, therefore I'm not accepting the request");
3535
return false;
3636
}
3737
String spaceName = cloudFoundryContext.getSpaceName();

‎osb-cmdb/src/test/java/com/orange/oss/osbcmdb/OsbCmdbBrokerConfigurationTest.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.orange.oss.osbcmdb.testfixtures.ASyncFailedCreateBackingSpaceInstanceInterceptor;
2121
import com.orange.oss.osbcmdb.testfixtures.ASyncStalledCreateBackingSpaceInstanceInterceptor;
2222
import com.orange.oss.osbcmdb.testfixtures.ASyncStalledDeleteBackingSpaceInstanceInterceptor;
23+
import com.orange.oss.osbcmdb.testfixtures.ASyncStalledUpdateBackingSpaceInstanceInterceptor;
2324
import com.orange.oss.osbcmdb.testfixtures.AsyncFailedDeleteBackingSpaceInstanceInterceptor;
2425
import com.orange.oss.osbcmdb.testfixtures.AsyncFailedUpdateBackingSpaceInstanceInterceptor;
2526
import com.orange.oss.osbcmdb.testfixtures.AsyncSuccessfulCreateUpdateDeleteBackingSpaceInstanceInterceptor;
@@ -127,6 +128,21 @@ void aSyncStalledDeleteBackingSpaceInstanceInterceptorIsCreatedWithAssociatedPro
127128
});
128129
}
129130

131+
@Test
132+
void aSyncStalledUpdateBackingSpaceInstanceInterceptorIsCreatedWithAssociatedProfile() {
133+
this.contextRunner
134+
.withPropertyValues(
135+
"spring.profiles.active=acceptanceTests,ASyncStalledUpdateBackingSpaceInstanceInterceptor"
136+
)
137+
.withPropertyValues(cloudFoundryDeploymentProperties())
138+
.run((context) -> {
139+
assertThat(context).hasSingleBean(ServiceInstanceInterceptor.class);
140+
assertThat(context)
141+
.getBean(ServiceInstanceInterceptor.class)
142+
.isInstanceOf(ASyncStalledUpdateBackingSpaceInstanceInterceptor.class);
143+
});
144+
}
145+
130146
@Test
131147
void syncFailingUpdateInterceptorIsCreatedWithAssociatedProfile() {
132148
this.contextRunner

‎spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/CloudFoundryAcceptanceTest.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -403,7 +403,8 @@ public ServiceInstance updateServiceInstanceWithoutAsserts(String serviceInstanc
403403
}
404404

405405
protected void updateServiceInstance(String serviceInstanceName, String planName) {
406-
cloudFoundryService.updateServiceInstance(serviceInstanceName, planName)
406+
Duration completionTimeout = Duration.ofMinutes(5);
407+
cloudFoundryService.updateServiceInstance(serviceInstanceName, planName, completionTimeout)
407408
.then(getServiceInstanceMono(serviceInstanceName))
408409
.flatMap(serviceInstance -> {
409410
assertThat(serviceInstance.getStatus())
@@ -413,6 +414,16 @@ protected void updateServiceInstance(String serviceInstanceName, String planName
413414
})
414415
.block();
415416
}
417+
protected void updateServiceInstanceWithoutAsserts(String serviceInstanceName, String planName,
418+
Duration completionTimeout) {
419+
try {
420+
cloudFoundryService.updateServiceInstance(serviceInstanceName, planName, completionTimeout)
421+
.block();
422+
}
423+
catch (DelayTimeoutException e) {
424+
LOG.info("deleteServiceInstance() completed with {}", e.toString());
425+
}
426+
}
416427

417428
protected void deleteServiceInstance(String serviceInstanceName) {
418429
blockingSubscribe(cloudFoundryService.deleteServiceInstance(serviceInstanceName));

‎spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/ConcurrentAsyncUpdateInstanceWithBackingServiceAcceptanceTest.java‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
/*
2+
* Copyright 2002-2020 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.cloud.appbroker.acceptance;
18+
19+
import java.time.Duration;
20+
21+
import org.cloudfoundry.operations.services.ServiceInstance;
22+
import org.junit.jupiter.api.AfterEach;
23+
import org.junit.jupiter.api.Tag;
24+
import org.junit.jupiter.api.Test;
25+
import org.slf4j.Logger;
26+
import org.slf4j.LoggerFactory;
27+
28+
import org.springframework.http.HttpStatus;
29+
30+
import static io.restassured.RestAssured.given;
31+
import static org.assertj.core.api.Assertions.assertThat;
32+
33+
@Tag("cmdb")
34+
class ConcurrentAsyncUpdateInstanceWithBackingServiceAcceptanceTest extends CmdbCloudFoundryAcceptanceTest {
35+
36+
private static final Logger LOG = LoggerFactory.getLogger(
37+
ConcurrentAsyncUpdateInstanceWithBackingServiceAcceptanceTest.class);
38+
39+
private static final String SUFFIX = "concurrent-staled-update-instance";
40+
41+
@Override
42+
protected String testSuffix() {
43+
return SUFFIX;
44+
}
45+
46+
/**
47+
* Maintain state before tearDown
48+
*/
49+
private String backingServiceName = null;
50+
51+
52+
@Test
53+
@AppBrokerTestProperties({
54+
"debug=true", //Spring boot debug mode
55+
//osb-cmdb auth
56+
"spring.security.user.name=user",
57+
"spring.security.user.password=password",
58+
"osbcmdb.admin.user=admin",
59+
"osbcmdb.admin.password=password",
60+
// control backing service response
61+
"spring.profiles.active=acceptanceTests,ASyncStalledUpdateBackingSpaceInstanceInterceptor",
62+
//cf java client wire traces
63+
"logging.level.cloudfoundry-client.wire=debug",
64+
// "logging.level.cloudfoundry-client.wire=trace",
65+
"logging.level.cloudfoundry-client.operations=debug",
66+
"logging.level.cloudfoundry-client.request=debug",
67+
"logging.level.cloudfoundry-client.response=debug",
68+
"logging.level.okhttp3=debug",
69+
70+
"logging.level.com.orange.oss.osbcmdb=debug",
71+
"osbcmdb.dynamic-catalog.enabled=false",
72+
})
73+
void brokeredServiceUpdates() {
74+
// given a brokered service instance is created
75+
createServiceInstance(brokeredServiceInstanceName());
76+
ServiceInstance brokeredServiceInstance = getServiceInstance(brokeredServiceInstanceName());
77+
78+
//When requesting an invalid update request to the same broker and invalid plan id
79+
//then it returns a 400 Bad request
80+
given(brokerFixture.serviceInstanceRequest(SERVICE_ID, "invalid-plan-id"))
81+
.when()
82+
.patch(brokerFixture.createServiceInstanceUrl(), brokeredServiceInstance.getId())
83+
.then()
84+
.statusCode(HttpStatus.BAD_REQUEST.value());
85+
86+
87+
//given a backend service is configured to stall on any update
88+
//when a brokered service update plan is requested
89+
updateServiceInstanceWithoutAsserts(brokeredServiceInstanceName(), PLAN2_NAME, Duration.ofSeconds(5));
90+
91+
//then a brokered service is stalled updating
92+
brokeredServiceInstance = getServiceInstance(brokeredServiceInstanceName());
93+
assertThat(brokeredServiceInstance.getLastOperation()).isEqualTo("update");
94+
assertThat(brokeredServiceInstance.getStatus()).isEqualTo("in progress");
95+
96+
//and backing service is also stalled updating
97+
backingServiceName = brokeredServiceInstance.getId();
98+
ServiceInstance backingServiceInstance = getServiceInstance(backingServiceName, brokeredServiceName());
99+
//was indeed updated, and has still its last operation as failed
100+
assertThat(backingServiceInstance.getLastOperation()).isEqualTo("update");
101+
assertThat(backingServiceInstance.getStatus()).isEqualTo("in progress");
102+
assertThat(backingServiceInstance.getPlan()).isEqualTo(PLAN_NAME); //Updated Plan name only gets published
103+
// once update is complete
104+
105+
106+
//When requesting a concurrent update request to the same broker with the same instance id, service
107+
// definition,
108+
// plan and params
109+
//then it returns a 202 accepted status
110+
given(brokerFixture.serviceInstanceRequest(SERVICE_ID, PLAN2_ID))
111+
.when()
112+
.patch(brokerFixture.createServiceInstanceUrl(), brokeredServiceInstance.getId())
113+
.then()
114+
.statusCode(HttpStatus.ACCEPTED.value());
115+
}
116+
117+
@AfterEach
118+
void tearDown() {
119+
purgeServiceInstance(brokeredServiceInstanceName());
120+
if (backingServiceName != null) {
121+
purgeServiceInstance(backingServiceName, brokeredServiceName());
122+
}
123+
}
124+
125+
126+
127+
}

‎spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/CreateDeleteInstanceWithBackingServiceKeysAcceptanceTest.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,26 @@ void deployAppsAndCreateServiceKeysOnBindService() throws InterruptedException {
119119
//when concurrent deprovision requests as received, they are properly handled
120120
assertDuplicateDeleteServiceInstanceOsbRequestsHandling(brokeredServiceInstance);
121121

122+
//when invalid service id or service plan is passed, they are rejected
123+
assertInvalidServiceProvisionningRequestsAreRejected();
124+
}
125+
126+
private void assertInvalidServiceProvisionningRequestsAreRejected() {
127+
//When requesting an invalid create request with invalid plan id
128+
//then it returns a 400 Bad request
129+
given(brokerFixture.serviceInstanceRequest(SERVICE_ID, "invalid-plan-id"))
130+
.when()
131+
.put(brokerFixture.createServiceInstanceUrl(), "a-fake_id")
132+
.then()
133+
.statusCode(HttpStatus.BAD_REQUEST.value());
134+
135+
//When requesting an invalid create request with invalid service definition id
136+
//then it returns a 400 Bad request
137+
given(brokerFixture.serviceInstanceRequest("invalid-service-id", PLAN_ID))
138+
.when()
139+
.put(brokerFixture.createServiceInstanceUrl(), "a-fake_id")
140+
.then()
141+
.statusCode(HttpStatus.BAD_REQUEST.value());
122142
}
123143

124144
private void assertDuplicateCreateServiceInstanceOsbRequestsHandling(ServiceInstance brokeredServiceInstance) {

‎spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/fixtures/cf/CloudFoundryService.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -355,11 +355,12 @@ public Mono<Void> updateServiceInstance(String serviceInstanceName, Map<String,
355355
.doOnError(error -> LOG.error("Error updating service instance " + serviceInstanceName + ": " + error));
356356
}
357357

358-
public Mono<Void> updateServiceInstance(String serviceInstanceName, String planName) {
358+
public Mono<Void> updateServiceInstance(String serviceInstanceName, String planName, Duration completionTimeout) {
359359
return cloudFoundryOperations.services()
360360
.updateInstance(UpdateServiceInstanceRequest.builder()
361361
.serviceInstanceName(serviceInstanceName)
362362
.planName(planName)
363+
.completionTimeout(completionTimeout)
363364
.build())
364365
.doOnSuccess(item -> LOG.info("Updated service instance " + serviceInstanceName))
365366
.doOnError(error -> LOG.error("Error updating service instance " + serviceInstanceName + ": " + error));

0 commit comments

Comments
 (0)