Merge pull request #116 from orange-cloudfoundry/svcat-updates

support for K8S svcat service instance updates

Author: gberche-orange

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

@@ -16,9 +16,6 @@
 import reactor.util.Loggers;
 
 import org.springframework.cloud.servicebroker.exception.ServiceBrokerException;
-import org.springframework.cloud.servicebroker.exception.ServiceBrokerInvalidParametersException;
-import org.springframework.cloud.servicebroker.model.catalog.Plan;
-import org.springframework.cloud.servicebroker.model.catalog.ServiceDefinition;
 
 public class AbstractOsbCmdbService {
 
@@ -66,7 +63,7 @@ protected ServiceInstance getCfServiceInstance(CloudFoundryOperations spacedTarg
 	}
 
 	protected CloudFoundryOperations getSpaceScopedOperations(String spaceName) {
-		createSpace(spaceName).block();
+		lookupOrCreateSpace(spaceName).block();
 
 		//We expect the builder to reuse most of Cf Java client internal objects and thus be lightweigth
 		return DefaultCloudFoundryOperations.builder()
@@ -85,20 +82,6 @@ protected String getSpacedIdFromTargettedOperationsInternals(CloudFoundryOperati
 		return spaceId;
 	}
 
-	protected void validateServiceDefinitionAndPlanIds(ServiceDefinition serviceDefinition, Plan plan,
-		String serviceDefinitionId,
-		String planId) {
-		if (plan == null) {
-			LOG.info("Invalid plan received with unknown id {}", planId);
-			throw new ServiceBrokerInvalidParametersException("Invalid plan received with unknown id:" + planId);
-		}
-		if (serviceDefinition == null) {
-			LOG.info("Invalid service definition received with unknown id {}", serviceDefinitionId);
-			throw new ServiceBrokerInvalidParametersException(
-				"Invalid service definition received with unknown id:" + serviceDefinitionId);
-		}
-	}
-
 	private Mono<Void> addSpaceDeveloperRoleForCurrentUser(String orgName, String spaceName) {
 		return Mono.defer(() -> operations.userAdmin().setSpaceRole(SetSpaceRoleRequest.builder()
 			.spaceRole(SpaceRole.DEVELOPER)
@@ -111,7 +94,7 @@ private Mono<Void> addSpaceDeveloperRoleForCurrentUser(String orgName, String sp
 				.format("Error setting space developer role for space %s: %s", spaceName, e.getMessage()))));
 	}
 
-	private Mono<String> createSpace(String spaceName) {
+	private Mono<String> lookupOrCreateSpace(String spaceName) {
 		return getSpaceId(spaceName)
 			.switchIfEmpty(Mono.just(this.defaultOrg)
 				.flatMap(orgName -> getOrganizationId(orgName)

osb-cmdb/src/main/java/com/orange/oss/osbcmdb/servicebinding/OsbCmdbServiceBinding.java

@@ -46,9 +46,7 @@ public Mono<CreateServiceInstanceBindingResponse> createServiceInstanceBinding(
 			return osbInterceptor.createServiceInstanceBinding(request);
 		}
 
-		//Validate mandatory service id and plan Id (even if plan is currently unused)
-		validateServiceDefinitionAndPlanIds(request.getServiceDefinition(), request.getPlan(),
-			request.getServiceDefinitionId(), request.getPlanId());
+		//No need to validate mandatory service id and plan Id as sc-osb does it already
 
 		//Lookup corresponding service instance in the backend org to validate incoming request against security
 		// attacks passing forged service instance guid
@@ -98,9 +96,7 @@ public Mono<DeleteServiceInstanceBindingResponse> deleteServiceInstanceBinding(
 			return osbInterceptor.deleteServiceInstanceBinding(request);
 		}
 
-		//Validate mandatory service id and plan Id (even if plan is currently unused)
-		validateServiceDefinitionAndPlanIds(request.getServiceDefinition(), request.getPlan(),
-			request.getServiceDefinitionId(), request.getPlanId());
+		//No need to validate mandatory service id and plan Id as sc-osb does it already
 
 		//Lookup corresponding service instance to validate incoming request against security attacks passing
 		// forged service instance guid

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

@@ -9,6 +9,7 @@
 
 import org.springframework.cloud.servicebroker.exception.ServiceBrokerMaintenanceInfoConflictException;
 import org.springframework.cloud.servicebroker.model.catalog.MaintenanceInfo;
+import org.springframework.cloud.servicebroker.model.catalog.Plan;
 import org.springframework.cloud.servicebroker.model.instance.CreateServiceInstanceRequest;
 import org.springframework.cloud.servicebroker.model.instance.UpdateServiceInstanceRequest;
 import org.springframework.lang.Nullable;
@@ -76,7 +77,9 @@ public org.cloudfoundry.client.v2.MaintenanceInfo formatForBackendInstance(Updat
 				.description(requestMaintenanceInfo.getDescription())
 				.build();
 		}
-		MaintenanceInfo brokeredServiceMI = request.getPlan().getMaintenanceInfo();
+		Plan requestedPlan = request.getPlan(); // may be null with svcat client, but we should have returned early
+		// since no maintenance info change requested
+		MaintenanceInfo brokeredServiceMI = requestedPlan.getMaintenanceInfo();
 		MaintenanceInfo inferredBackendMI = unmergeInfos(brokeredServiceMI);
 		if (DEFAULT_MISSING_BACKEND_MI.equals(inferredBackendMI)) {
 			return null;
@@ -114,7 +117,7 @@ public MaintenanceInfo getOsbCmdbMaintenanceInfo() {
 	 * Indicates whether the request is a pure upgrade request `cf update-service --upgrade` resulting from a version
 	 * bump introduced by osb-cmdb, and no backend version bump.
 	 *
-	 * @return False if the update request should be passed to tbe backend broker, true if backend update should be
+	 * @return False if the update request should be passed to the backend broker, true if backend update should be
 	 * 	skipped (i.e. noop)
 	 */
 	public boolean isNoOpUpgradeBackingService(UpdateServiceInstanceRequest request) {
@@ -124,7 +127,16 @@ public boolean isNoOpUpgradeBackingService(UpdateServiceInstanceRequest request)
 		if (!hasMaintenanceInfoChangeRequest(request)) {
 			return false;
 		}
-		MaintenanceInfo brokeredServiceMI = request.getPlan().getMaintenanceInfo();
+		Plan requestedPlan = request.getPlan();
+		if (requestedPlan == null) {
+			//Likely case of K8S service catalog client which is ommited service plan unless a plan upgrade is requested
+			//Svcat does not support maintenance info upgrade requests, so we always propagate the request to the
+			//backing service broker in this case
+			LOG.info("Update request received without plan. Assuming req from K8S svcat client and propagating to the" +
+					" backing broker");
+			return false;
+		}
+		MaintenanceInfo brokeredServiceMI = requestedPlan.getMaintenanceInfo();
 		MaintenanceInfo inferredBackendMI = unmergeInfos(brokeredServiceMI);
 		return inferredBackendMI.equals(DEFAULT_MISSING_BACKEND_MI);
 	}
@@ -133,7 +145,14 @@ public boolean isNoOpUpgradeBackingService(UpdateServiceInstanceRequest request)
 	 * Validates incoming USI request and rejects requests with unsupported maintenance info
 	 */
 	public void validateAnyUpgradeRequest(UpdateServiceInstanceRequest request) {
-		MaintenanceInfo catalogMi = request.getPlan().getMaintenanceInfo();
+		Plan requestPlan = request.getPlan();
+		if (requestPlan == null) {
+			//Case of a K8S svcat which is omitting the plan when plan change isn't requested.
+			//Since no plan change is requested, we don't need to validate that the plan matches the current
+			//plan (i.e. maintenance info match)
+			return;
+		}
+		MaintenanceInfo catalogMi = requestPlan.getMaintenanceInfo();
 		MaintenanceInfo requestedMi = request.getMaintenanceInfo();
 		validateRequestedMaintenanceInfo(requestedMi, catalogMi);
 	}
@@ -172,6 +191,7 @@ boolean hasMaintenanceInfoChangeRequest(UpdateServiceInstanceRequest request) {
 				"client)");
 			return false;
 		}
+
 		if (request.getMaintenanceInfo() != null) {
 			hasMaintenanceInfoChangeRequest =
 				!request.getMaintenanceInfo().equals(request.getPreviousValues().getMaintenanceInfo());

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

@@ -202,8 +202,10 @@ public Mono<CreateServiceInstanceResponse> createServiceInstance(CreateServiceIn
 		if (osbInterceptor != null && osbInterceptor.accept(request)) {
 			return osbInterceptor.createServiceInstance(request);
 		}
-		validateServiceDefinitionAndPlanIds(request.getServiceDefinition(), request.getPlan(),
-			request.getServiceDefinitionId(), request.getPlanId());
+		//No need to validate mandatory service id and plan Id as sc-osb does it already
+		//See https://github.com/spring-cloud/spring-cloud-open-service-broker/blob
+		///7d374332419aad628ca76351fd53ca7e351c57de/spring-cloud-open-service-broker-core/src/main/java/org/springframework/cloud/servicebroker/controller/ServiceInstanceController.java#L111-L112
+
 		maintenanceInfoFormatterService.validateAnyCreateRequest(request);
 		String backingServiceName = request.getServiceDefinition().getName();
 		String backingServicePlanName = request.getPlan().getName();
@@ -213,7 +215,7 @@ public Mono<CreateServiceInstanceResponse> createServiceInstance(CreateServiceIn
 			spacedTargetedOperations = getSpaceScopedOperations(backingServiceName);
 			//Lookup guids necessary for low level api usage, and that CloudFoundryOperations hides in its response
 			String spaceId = getSpacedIdFromTargettedOperationsInternals(spacedTargetedOperations);
-			String backingServicePlanId = fetchBackingServicePlanId(backingServiceName, backingServicePlanName,
+			String backingServicePlanId = fetchRequestedBackingServicePlanId(backingServiceName, backingServicePlanName,
 				spaceId);
 
 
@@ -305,9 +307,7 @@ public Mono<DeleteServiceInstanceResponse> deleteServiceInstance(DeleteServiceIn
 			return osbInterceptor.deleteServiceInstance(request);
 		}
 
-		//Validate mandatory service id and plan Id
-		validateServiceDefinitionAndPlanIds(request.getServiceDefinition(), request.getPlan(),
-			request.getServiceDefinitionId(), request.getPlanId());
+		//No need to validate mandatory service id and plan Id as sc-osb does it already
 
 		String backingServiceInstanceName =
 			ServiceInstanceNameHelper.truncateNameToCfMaxSize(request.getServiceInstanceId());
@@ -475,12 +475,22 @@ public Mono<UpdateServiceInstanceResponse> updateServiceInstance(UpdateServiceIn
 			return osbInterceptor.updateServiceInstance(request);
 		}
 
-		validateServiceDefinitionAndPlanIds(request.getServiceDefinition(), request.getPlan(),
-			request.getServiceDefinitionId(), request.getPlanId());
+		//No need to validate mandatory service id as sc-osb does it already
+		//However since planId params optional sc-osb does not yet reject planId not matching any known plan
+		validateServicePlanId(request.getPlan(), request.getPlanId());
+
 		maintenanceInfoFormatterService.validateAnyUpgradeRequest(request);
 
 		String backingServiceName = request.getServiceDefinition().getName();
-		String backingServicePlanName = request.getPlan().getName();
+		String requestedBackingServicePlanName;
+		if (request.getPlan() != null) {
+			//possibly receiving a plan update, or just CF CC_NG passing the current planid unchanged
+			//in case of param update
+			requestedBackingServicePlanName = request.getPlan().getName();
+		} else {
+			//Possibly a K8S svcat param update which is omitting the optional planId (as specs allows)
+			requestedBackingServicePlanName = null;
+		}
 		String backingServiceInstanceName = ServiceInstanceNameHelper
 			.truncateNameToCfMaxSize(request.getServiceInstanceId());
 
@@ -497,7 +507,7 @@ public Mono<UpdateServiceInstanceResponse> updateServiceInstance(UpdateServiceIn
 
 		//Lookup guids necessary for low level api usage, and that CloudFoundryOperations hides in its response
 		String spaceId = getSpacedIdFromTargettedOperationsInternals(spacedTargetedOperations);
-		String backingServicePlanId = fetchBackingServicePlanId(backingServiceName, backingServicePlanName, spaceId);
+		String requestedBackingServicePlanId = fetchRequestedBackingServicePlanId(backingServiceName, requestedBackingServicePlanName, spaceId);
 
 		if (maintenanceInfoFormatterService.isNoOpUpgradeBackingService(request)) {
 			LOG.info("NoOp upgrade detected, returning early 200 OK");
@@ -531,7 +541,7 @@ public Mono<UpdateServiceInstanceResponse> updateServiceInstance(UpdateServiceIn
 			updateServiceInstanceResponse = client.serviceInstances()
 				.update(org.cloudfoundry.client.v2.serviceinstances.UpdateServiceInstanceRequest.builder()
 					.serviceInstanceId(existingBackingServiceInstance.getId())
-					.servicePlanId(backingServicePlanId)
+					.servicePlanId(requestedBackingServicePlanId) //possibly null
 					.parameters(formatParameters(metaData, request.getParameters()))
 					.maintenanceInfo(formattedForBackendInstanceMI)
 					.acceptsIncomplete(request.isAsyncAccepted())
@@ -581,6 +591,13 @@ public Mono<UpdateServiceInstanceResponse> updateServiceInstance(UpdateServiceIn
 		return Mono.just(responseBuilder.build());
 	}
 
+	protected void validateServicePlanId(Plan plan, String planId) {
+		if (planId !=null && plan == null) {
+			LOG.info("Invalid plan received with unknown id {}", planId);
+			throw new ServiceBrokerInvalidParametersException("Invalid plan received with unknown id:" + planId);
+		}
+	}
+
 	protected CmdbOperationState fromJson(String operation) {
 		try {
 			return OBJECT_MAPPER.readValue(operation, CmdbOperationState.class);
@@ -617,8 +634,11 @@ private OperationState convertCfStateToOsbState(String cfServiceInstanceState) {
 		}
 	}
 
-	private String fetchBackingServicePlanId(String backingServiceName, String backingServicePlanName, String spaceId) {
-		//Inspired from first instrcutions of cf-java-client:
+	private String fetchRequestedBackingServicePlanId(String backingServiceName, String updatedBackingServicePlanName, String spaceId) {
+		if (updatedBackingServicePlanName == null) {
+			return null;
+		}
+		//Inspired from first instructions of cf-java-client:
 		// org.cloudfoundry.operations.services.DefaultServices.createInstance()
 		String backingServiceId = PaginationUtils
 			.requestClientV2Resources(page -> client.spaces()
@@ -641,11 +661,11 @@ private String fetchBackingServicePlanId(String backingServiceName, String backi
 					.page(page)
 					.serviceId(backingServiceId)
 					.build()))
-			.filter(resource -> backingServicePlanName.equals(ResourceUtils.getEntity(resource).getName()))
+			.filter(resource -> updatedBackingServicePlanName.equals(ResourceUtils.getEntity(resource).getName()))
 			.single()
 			.map(ResourceUtils::getId)
 			.onErrorResume(NoSuchElementException.class,
-				t -> ExceptionUtils.illegalArgument("Service plan %s does not exist", backingServicePlanName))
+				t -> ExceptionUtils.illegalArgument("Service plan %s does not exist", updatedBackingServicePlanName))
 			.block();
 	}
 
@@ -849,8 +869,12 @@ private Mono<UpdateServiceInstanceResponse> handleUpdateException(Exception orig
 						.build());
 
 				case OsbApiConstants.LAST_OPERATION_STATE_SUCCEEDED:
+					Plan requestedPlan = request.getPlan();
+					String requestedPlanName = requestedPlan == null ?
+						null :
+						requestedPlan.getName();
 					if (updatedSi.getService().equals(request.getServiceDefinition().getName()) &&
-						updatedSi.getPlan().equals(request.getPlan().getName())) {
+						updatedSi.getPlan().equals(requestedPlanName)) {
 						LOG.info("Concurrent update request has completed. " +
 							"Returning 200 OK");
 						//200 OK

osb-cmdb/src/test/java/com/orange/oss/osbcmdb/serviceinstance/MaintenanceInfoFormatterServiceTest.java

@@ -71,6 +71,17 @@ void test_validate_upgrade_request_no_mi() {
 		maintenanceInfoFormatterService.validateAnyUpgradeRequest(updateServiceInstanceRequest);
 	}
 
+	@DisplayName("validates upgrade requests include valid maintenance info (when no plan specified)")
+	@Test
+	void test_validate_upgrade_request_no_plan_specified() {
+		//Given
+		MaintenanceInfoFormatterService maintenanceInfoFormatterService = new MaintenanceInfoFormatterService(
+			anOsbCmdbInfoV1());
+		UpdateServiceInstanceRequest updateServiceInstanceRequest = UpdateServiceInstanceRequest.builder()
+			.build();
+		maintenanceInfoFormatterService.validateAnyUpgradeRequest(updateServiceInstanceRequest);
+	}
+
 	@DisplayName("validates upgrade requests include valid maintenance info")
 	@Test
 	void test_validate_create_request() {
@@ -234,6 +245,22 @@ void test_isNoOpUpgradeBackingService_with_params() {
 		assertThat(isNoOpUpgradeBackingService).isFalse();
 	}
 
+	@DisplayName("an update request should NOT be a noop, when params requested (no plan update), even if backend has" +
+		" no MI")
+	@Test
+	void test_isNoOpUpgradeBackingService_with_params_without_plan() {
+		//Given
+		MaintenanceInfoFormatterService maintenanceInfoFormatterService = new MaintenanceInfoFormatterService(
+			anOsbCmdbInfoV1());
+		UpdateServiceInstanceRequest updateServiceInstanceRequest = UpdateServiceInstanceRequest.builder()
+			.parameters(Collections.singletonMap("a-key", "a-value"))
+			.build();
+		//when
+		boolean isNoOpUpgradeBackingService = maintenanceInfoFormatterService
+			.isNoOpUpgradeBackingService(updateServiceInstanceRequest);
+		assertThat(isNoOpUpgradeBackingService).isFalse();
+	}
+
 	@DisplayName("an update request should NOT be a noop, when plan update requested, even if backend has no MI")
 	@Test
 	void test_isNoOpUpgradeBackingService_with_plan_update() {
@@ -253,6 +280,21 @@ void test_isNoOpUpgradeBackingService_with_plan_update() {
 		assertThat(isNoOpUpgradeBackingService).isFalse();
 	}
 
+	@DisplayName("Formats the request to send to backend service : case of a client K8S svcat request without MI/ plan")
+	@Test
+	void test_formatForBackendInstance_case0() {
+		//given an osb-cmdb configured with a bump
+		MaintenanceInfoFormatterService maintenanceInfoFormatterService = new MaintenanceInfoFormatterService(
+			anOsbCmdbInfoV1());
+		//when receiving a request made by a client from catalog
+		org.cloudfoundry.client.v2.MaintenanceInfo requestBackendServiceMi = maintenanceInfoFormatterService
+			.formatForBackendInstance(UpdateServiceInstanceRequest.builder()
+				.build());
+		//then no MI is passed to backend
+		assertThat(requestBackendServiceMi).isNull();
+		//actually this should previously be detected as a noop and update should not be requested to backend
+	}
+
 	@DisplayName("Formats the request to send to backend service : case of backend service has no Mi and osb-cmdb " +
 		"bump " +
 		"(e.g. cf-mysql")