feat(cf): Add Updatable flag for services (#3474)

- Supersedes functionality whereby versioning of services provided via tags was used to determine whether a service should be updated spinnaker/spinnaker#4084 Co-Authored-By: Jason Chu <jchu@pivotal.io>
spinnaker · Mar 15, 2019 · b4efd5d · b4efd5d
1 parent 1e0856d
commit b4efd5d
Show file tree

Hide file tree

Showing 10 changed files with 62 additions and 43 deletions.
diff --git a/...src/main/java/com/netflix/spinnaker/clouddriver/cloudfoundry/client/ServiceInstances.java b/...src/main/java/com/netflix/spinnaker/clouddriver/cloudfoundry/client/ServiceInstances.java
@@ -49,7 +49,6 @@
 
 @RequiredArgsConstructor
 public class ServiceInstances {
-  private static final String SPINNAKER_VERSION_V_D_3 = "spinnakerVersion-v\\d{3}";
   private final ServiceInstanceService api;
   private final ConfigService configApi;
   private final Organizations orgs;
@@ -355,6 +354,7 @@ public ServiceInstanceResponse createServiceInstance(
     String servicePlanName,
     Set<String> tags,
     Map<String, Object> parameters,
+    boolean updatable,
     CloudFoundrySpace space) {
     List<CloudFoundryServicePlan> cloudFoundryServicePlans = findAllServicePlansByServiceName(serviceName);
     if (cloudFoundryServicePlans.isEmpty()) {
@@ -378,16 +378,18 @@ public ServiceInstanceResponse createServiceInstance(
       command,
       api::createServiceInstance,
       api::updateServiceInstance,
+      api::all,
       c -> getOsbServiceInstance(space, c.getName()),
       (c, r) -> {
         if (!r.getPlanId().equals(c.getServicePlanGuid())) {
           throw new CloudFoundryApiException("A service with name '" + c.getName() + "' exists but has a different plan");
         }
       },
+      updatable,
       space
     );
 
-    response.setState(IN_PROGRESS);
+    response.setState(updatable ? IN_PROGRESS : SUCCEEDED);
     return response;
   }
 
@@ -397,6 +399,7 @@ public ServiceInstanceResponse createUserProvidedServiceInstance(
     Set<String> tags,
     Map<String, Object> credentials,
     String routeServiceUrl,
+    boolean updatable,
     CloudFoundrySpace space) {
     CreateUserProvidedServiceInstance command = new CreateUserProvidedServiceInstance();
     command.setName(newUserProvidedServiceInstanceName);
@@ -410,9 +413,11 @@ public ServiceInstanceResponse createUserProvidedServiceInstance(
       command,
       api::createUserProvidedServiceInstance,
       api::updateUserProvidedServiceInstance,
+      api::allUserProvided,
       c -> getUserProvidedServiceInstance(space, c.getName()),
       (c, r) -> {
       },
+      updatable,
       space
     );
 
@@ -425,46 +430,32 @@ public ServiceInstanceResponse createUserProvidedServiceInstance(
   createServiceInstance(T command,
                         Function<T, Resource<S>> create,
                         BiFunction<String, T, Resource<S>> update,
+                        BiFunction<Integer, List<String>, Page<S>> getAllServices,
                         Function<T, CloudFoundryServiceInstance> getServiceInstance,
                         BiConsumer<T, CloudFoundryServiceInstance> updateValidation,
+                        boolean updatable,
                         CloudFoundrySpace space) {
     LastOperation.Type operationType;
     List<String> serviceInstanceQuery = getServiceQueryParams(Collections.singletonList(command.getName()), space);
     List<Resource<? extends AbstractServiceInstance>> serviceInstances = new ArrayList<>();
-    serviceInstances.addAll(collectPageResources("service instances", pg -> api.all(pg, serviceInstanceQuery)));
-    serviceInstances.addAll(collectPageResources("service instances", pg -> api.allUserProvided(pg, serviceInstanceQuery)));
+    serviceInstances.addAll(collectPageResources("service instances", pg -> getAllServices.apply(pg, serviceInstanceQuery)));
 
+    operationType = CREATE;
     if (serviceInstances.size() == 0) {
-      operationType = CREATE;
       safelyCall(() -> create.apply(command)).map(res -> res.getMetadata().getGuid())
         .orElseThrow(() -> new CloudFoundryApiException("service instance '" + command.getName() + "' could not be created"));
-    } else {
+    } else if (updatable) {
       operationType = UPDATE;
       serviceInstances.stream()
         .findFirst()
         .map(r -> r.getMetadata().getGuid())
         .orElseThrow(() -> new CloudFoundryApiException("Service instance '" + command.getName() + "' not found"));
-      String existingServiceInstanceVersionTag = serviceInstances.stream()
-        .findFirst().map(s -> s.getEntity().getTags())
-        .orElse(new HashSet<>())
-        .stream()
-        .filter(t -> t.matches(SPINNAKER_VERSION_V_D_3))
-        .min(Comparator.reverseOrder())
-        .orElse("");
-      String newServiceInstanceVersionTag = Optional.ofNullable(command.getTags())
-        .orElse(Collections.emptySet())
-        .stream()
-        .filter(t -> t.matches(SPINNAKER_VERSION_V_D_3))
-        .min(Comparator.reverseOrder())
-        .orElse("");
-      if (newServiceInstanceVersionTag.isEmpty() || !existingServiceInstanceVersionTag.equals(newServiceInstanceVersionTag)) {
-        CloudFoundryServiceInstance serviceInstance = getServiceInstance.apply(command);
-        if (serviceInstance == null) {
-          throw new CloudFoundryApiException("No service instances with name '" + command.getName() + "' found in space " + space.getName());
-        }
-        updateValidation.accept(command, serviceInstance);
-        safelyCall(() -> update.apply(serviceInstance.getId(), command));
+      CloudFoundryServiceInstance serviceInstance = getServiceInstance.apply(command);
+      if (serviceInstance == null) {
+        throw new CloudFoundryApiException("No service instances with name '" + command.getName() + "' found in space " + space.getName());
       }
+      updateValidation.accept(command, serviceInstance);
+      safelyCall(() -> update.apply(serviceInstance.getId(), command));
     }
 
     return new ServiceInstanceResponse()

diff --git a/...lix/spinnaker/clouddriver/cloudfoundry/client/model/v2/AbstractCreateServiceInstance.java b/...lix/spinnaker/clouddriver/cloudfoundry/client/model/v2/AbstractCreateServiceInstance.java
@@ -24,6 +24,8 @@
 @Data
 public abstract class AbstractCreateServiceInstance {
   private String name;
+  private String spaceGuid;
+  private boolean updatable = true;
 
   @Nullable
   private Set<String> tags;

diff --git a/...com/netflix/spinnaker/clouddriver/cloudfoundry/client/model/v2/CreateServiceInstance.java b/...com/netflix/spinnaker/clouddriver/cloudfoundry/client/model/v2/CreateServiceInstance.java
@@ -27,7 +27,6 @@
 @EqualsAndHashCode(callSuper = true)
 @JsonInclude(JsonInclude.Include.NON_NULL)
 public class CreateServiceInstance extends AbstractCreateServiceInstance {
-  private String spaceGuid;
   private String servicePlanGuid;
 
   @Nullable

diff --git a/...spinnaker/clouddriver/cloudfoundry/client/model/v2/CreateUserProvidedServiceInstance.java b/...spinnaker/clouddriver/cloudfoundry/client/model/v2/CreateUserProvidedServiceInstance.java
@@ -27,8 +27,6 @@
 @EqualsAndHashCode(callSuper = true)
 @JsonInclude(JsonInclude.Include.NON_NULL)
 public class CreateUserProvidedServiceInstance extends AbstractCreateServiceInstance {
-  private String spaceGuid;
-
   @Nullable
   private String syslogDrainUrl;
 

diff --git a/...ver/cloudfoundry/deploy/converters/DeployCloudFoundryServiceAtomicOperationConverter.java b/...ver/cloudfoundry/deploy/converters/DeployCloudFoundryServiceAtomicOperationConverter.java
@@ -95,6 +95,7 @@ DeployCloudFoundryServiceDescription.ServiceAttributes convertManifest(Object ma
     attrs.setServiceInstanceName(manifest.getServiceInstanceName());
     attrs.setServicePlan(manifest.getServicePlan());
     attrs.setTags(manifest.getTags());
+    attrs.setUpdatable(manifest.isUpdatable());
     attrs.setParameterMap(parseParameters(manifest.getParameters()));
     return attrs;
   }
@@ -111,6 +112,7 @@ DeployCloudFoundryServiceDescription.UserProvidedServiceAttributes convertUserPr
     attrs.setSyslogDrainUrl(manifest.getSyslogDrainUrl());
     attrs.setRouteServiceUrl(manifest.getRouteServiceUrl());
     attrs.setTags(manifest.getTags());
+    attrs.setUpdatable(manifest.isUpdatable());
     attrs.setCredentials(parseParameters(manifest.getCredentials()));
     return attrs;
   }
@@ -131,6 +133,7 @@ private Map<String, Object> parseParameters(String parameterString) {
   @Data
   private static class ServiceManifest {
     private String service;
+    private boolean updatable = true;
 
     @JsonAlias("service_instance_name")
     private String serviceInstanceName;
@@ -147,6 +150,8 @@ private static class ServiceManifest {
 
   @Data
   private static class UserProvidedServiceManifest {
+    private boolean updatable = true;
+
     @JsonAlias("service_instance_name")
     private String serviceInstanceName;
 

diff --git a/...ker/clouddriver/cloudfoundry/deploy/description/DeployCloudFoundryServiceDescription.java b/...ker/clouddriver/cloudfoundry/deploy/description/DeployCloudFoundryServiceDescription.java
@@ -48,6 +48,7 @@ public static class ServiceAttributes {
     String service;
     String serviceInstanceName;
     String servicePlan;
+    boolean updatable = true;
 
     @Nullable
     Set<String> tags;
@@ -59,6 +60,7 @@ public static class ServiceAttributes {
   @Data
   public static class UserProvidedServiceAttributes {
     String serviceInstanceName;
+    boolean updatable = true;
 
     @Nullable
     Set<String> tags;

diff --git a/...innaker/clouddriver/cloudfoundry/deploy/ops/DeployCloudFoundryServiceAtomicOperation.java b/...innaker/clouddriver/cloudfoundry/deploy/ops/DeployCloudFoundryServiceAtomicOperation.java
@@ -53,6 +53,7 @@ public ServiceInstanceResponse operate(List priorOutputs) {
           serviceAttributes.getServicePlan(),
           serviceAttributes.getTags(),
           serviceAttributes.getParameterMap(),
+          serviceAttributes.isUpdatable(),
           description.getSpace());
       String gerund = serviceInstanceResponse.getType() == UPDATE
         ? "Updating"
@@ -71,6 +72,7 @@ public ServiceInstanceResponse operate(List priorOutputs) {
           userProvidedServiceAttributes.getTags(),
           userProvidedServiceAttributes.getCredentials(),
           userProvidedServiceAttributes.getRouteServiceUrl(),
+          userProvidedServiceAttributes.isUpdatable(),
           description.getSpace());
       String verb = serviceInstanceResponse.getType() == UPDATE
         ? "Updated"

diff --git a/...test/java/com/netflix/spinnaker/clouddriver/cloudfoundry/client/ServiceInstancesTest.java b/...test/java/com/netflix/spinnaker/clouddriver/cloudfoundry/client/ServiceInstancesTest.java
@@ -155,6 +155,7 @@ void shouldSuccessfullyCreateService() {
       "ServicePlan1",
       Collections.emptySet(),
       null,
+      true,
       cloudFoundrySpace);
 
     assertThat(response).isEqualTo(new ServiceInstanceResponse()
@@ -182,6 +183,7 @@ void shouldThrowExceptionWhenCreationReturnsHttpNotFound() {
         "ServicePlan1",
         Collections.emptySet(),
         null,
+        false,
         cloudFoundrySpace),
       CloudFoundryApiException.class, "Cloud Foundry API returned with error(s): service instance 'new-service-instance-name' could not be created");
     verify(serviceInstanceService, times(1)).createServiceInstance(any());
@@ -202,6 +204,7 @@ void throwExceptionWhenNoServicePlanExistsWithTheNameProvided() {
         "servicePlanName",
         Collections.emptySet(),
         null,
+        true,
         cloudFoundrySpace),
       ResourceNotFoundException.class, "No plans available for service name 'serviceName'");
   }
@@ -217,6 +220,7 @@ void shouldUpdateTheServiceIfAlreadyExists() {
       "ServicePlan1",
       Collections.emptySet(),
       null,
+      true,
       cloudFoundrySpace);
 
     assertThat(response).isEqualTo(new ServiceInstanceResponse()
@@ -229,26 +233,26 @@ void shouldUpdateTheServiceIfAlreadyExists() {
   }
 
   @Test
-  void shouldUpdateTheServiceIfAlreadyExistsAndVersionTagsDiffer() {
+  void shouldNotUpdateTheServiceIfAlreadyExists() {
     when(serviceInstanceService.all(any(), anyListOf(String.class))).thenReturn(createOsbServiceInstancePage());
     when(serviceInstanceService.allUserProvided(any(), anyListOf(String.class))).thenReturn(createEmptyUserProvidedServiceInstancePage());
     when(serviceInstanceService.updateServiceInstance(any(), any())).thenReturn(createServiceInstanceResource());
 
-    ServiceInstanceResponse response = serviceInstances.createServiceInstance(
-      "new-service-instance-name",
+    ServiceInstanceResponse response = serviceInstances.createServiceInstance("new-service-instance-name",
       "serviceName",
       "ServicePlan1",
-      Collections.singleton("spinnakerVersion-v002"),
+      Collections.emptySet(),
       null,
+      false,
       cloudFoundrySpace);
 
     assertThat(response).isEqualTo(new ServiceInstanceResponse()
       .setServiceInstanceName("new-service-instance-name")
-      .setType(UPDATE)
-      .setState(IN_PROGRESS)
+      .setType(CREATE)
+      .setState(SUCCEEDED)
     );
     verify(serviceInstanceService, times(0)).createServiceInstance(any());
-    verify(serviceInstanceService, times(1)).updateServiceInstance(any(), any());
+    verify(serviceInstanceService, times(0)).updateServiceInstance(any(), any());
   }
 
   @Test
@@ -267,6 +271,7 @@ void shouldThrowExceptionIfServiceExistsAndNeedsChangingButUpdateFails() {
         "ServicePlan1",
         Collections.emptySet(),
         null,
+        true,
         cloudFoundrySpace),
       CloudFoundryApiException.class, "Cloud Foundry API returned with error(s): ");
 
@@ -296,6 +301,7 @@ void shouldThrowCloudFoundryApiErrorWhenMoreThanOneServiceInstanceWithTheSameNam
         "ServicePlan1",
         Collections.emptySet(),
         null,
+        true,
         cloudFoundrySpace),
       CloudFoundryApiException.class, "Cloud Foundry API returned with error(s): 2 service instances found with name 'new-service-instance-name' in space 'space', but expected only 1");
   }
@@ -312,6 +318,7 @@ void shouldSuccessfullyCreateUserProvidedService() {
       Collections.emptySet(),
       Collections.emptyMap(),
       "routeServiceUrl",
+      true,
       cloudFoundrySpace
     );
 
@@ -336,6 +343,7 @@ void shouldUpdateUserProvidedServiceInstanceIfAlreadyExists() {
       Collections.emptySet(),
       Collections.emptyMap(),
       "routeServiceUrl",
+      true,
       cloudFoundrySpace
     );
 
@@ -349,27 +357,28 @@ void shouldUpdateUserProvidedServiceInstanceIfAlreadyExists() {
   }
 
   @Test
-  void shouldUpdateUserProvidedServiceInstanceIfVersionTagsDiffer() {
+  void shouldNotUpdateUserProvidedServiceInstanceIfAlreadyExists() {
     when(serviceInstanceService.all(any(), any())).thenReturn(createEmptyOsbServiceInstancePage());
     when(serviceInstanceService.allUserProvided(any(), anyListOf(String.class))).thenReturn(createUserProvidedServiceInstancePage());
     when(serviceInstanceService.updateUserProvidedServiceInstance(any(), any())).thenReturn(createUserProvidedServiceInstanceResource());
 
     ServiceInstanceResponse response = serviceInstances.createUserProvidedServiceInstance(
       "new-up-service-instance-name",
       "syslogDrainUrl",
-      Collections.singleton("spinnakerVersion-v001"),
+      Collections.emptySet(),
       Collections.emptyMap(),
       "routeServiceUrl",
+      false,
       cloudFoundrySpace
     );
 
     assertThat(response).isEqualTo(new ServiceInstanceResponse()
       .setServiceInstanceName("new-up-service-instance-name")
-      .setType(UPDATE)
+      .setType(CREATE)
       .setState(SUCCEEDED)
     );
     verify(serviceInstanceService, times(0)).createUserProvidedServiceInstance(any());
-    verify(serviceInstanceService, times(1)).updateUserProvidedServiceInstance(any(), any());
+    verify(serviceInstanceService, times(0)).updateUserProvidedServiceInstance(any(), any());
   }
 
   @Test