TargetServerGroupResolver cleanup

orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/support/TargetServerGroupResolver.groovy

@@ -17,21 +17,24 @@
 package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support
 
 import com.fasterxml.jackson.databind.ObjectMapper
+import com.netflix.spinnaker.kork.annotations.VisibleForTesting
 import com.netflix.spinnaker.kork.core.RetrySupport
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 import com.netflix.spinnaker.orca.clouddriver.OortService
+import com.netflix.spinnaker.orca.clouddriver.model.ServerGroup
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
 import retrofit.RetrofitError
-import retrofit.client.Response
-import retrofit.converter.ConversionException
-import retrofit.converter.JacksonConverter
+
+import java.time.Duration
 
 @Component
 @Slf4j
 class TargetServerGroupResolver {
 
+  public static final int NUM_RETRIES = 15;
+
   @Autowired
   OortService oortService
 
@@ -63,37 +66,37 @@ class TargetServerGroupResolver {
 
   private TargetServerGroup resolveByTarget(TargetServerGroup.Params params, Location location) {
     try {
-      Map tsgMap = fetchWithRetries(Map) {
+      ServerGroup tsg = fetchWithRetries {
         oortService.getTargetServerGroup(params.app,
           params.credentials,
           params.cluster,
           params.cloudProvider,
           location.value,
           params.target.name())
       }
-      if (!tsgMap) {
+      if (!tsg) {
         throw new TargetServerGroup.NotFoundException("Unable to locate ${params.target.name()} in $params.credentials/$location.value/$params.cluster")
       }
-      return new TargetServerGroup(tsgMap)
+      return new TargetServerGroup(tsg)
     } catch (Exception e) {
       log.error("Unable to locate ${params.target.name()} in $params.credentials/$location.value/$params.cluster", e)
       throw e
     }
   }
 
   private TargetServerGroup resolveByServerGroupName(TargetServerGroup.Params params, Location location) {
-    List<Map> tsgList = fetchWithRetries(List) {
-      oortService.getServerGroupFromCluster(
+    // TODO(ttomsu): Add zonal support to this op. (e.g. the region param).  Note that adding a region changes the response type from a List to a singleServerGroup
+    List<ServerGroup> tsgList = fetchWithRetries {
+      oortService.getServerGroupsFromClusterTyped(
         params.app,
         params.credentials,
         params.cluster,
         params.serverGroupName,
-        null /* region */, // TODO(ttomsu): Add zonal support to this op.
         params.cloudProvider
       )
     }
     // Without zonal support in the getServerGroup call above, we have to do the filtering here.
-    def tsg = tsgList?.find { Map tsg -> tsg.region == location.value || tsg.zones?.contains(location.value) || tsg.namespace == location.value }
+    def tsg = tsgList?.find { ServerGroup tsg -> tsg.region == location.value || tsg.zones?.contains(location.value) || tsg.namespace == location.value }
     if (!tsg) {
       throw new TargetServerGroup.NotFoundException("Unable to locate $params.serverGroupName in $params.credentials/$location.value/$params.cluster")
     }
@@ -145,28 +148,17 @@ class TargetServerGroupResolver {
     return stage.type == DetermineTargetServerGroupStage.PIPELINE_CONFIG_TYPE
   }
 
-  private <T> T fetchWithRetries(Class<T> responseType, Closure<Response> fetchClosure) {
-    return fetchWithRetries(responseType, 15, 1000, fetchClosure)
-  }
-
-  private <T> T fetchWithRetries(Class<T> responseType, int maxRetries, long retryBackoff, Closure<Response> fetchClosure) {
+  @VisibleForTesting
+  <T> T fetchWithRetries(Closure<T> fetchClosure) {
     return retrySupport.retry({
-      def converter = new JacksonConverter(mapper)
-
-      Response response
       try {
-        response = fetchClosure.call()
+        return fetchClosure.call()
       } catch (RetrofitError re) {
         if (re.kind == RetrofitError.Kind.HTTP && re.response.status == 404) {
           return null
         }
         throw re
       }
-      try {
-        return (T) converter.fromBody(response.body, responseType)
-      } catch (ConversionException ce) {
-        throw RetrofitError.conversionError(response.url, response, converter, responseType, ce)
-      }
-    }, maxRetries, retryBackoff, false)
+    }, NUM_RETRIES, Duration.ofMillis(1000), false)
   }
 }

orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/DelegatingOortService.java

@@ -21,6 +21,7 @@
 import com.netflix.spinnaker.orca.clouddriver.model.Ami;
 import com.netflix.spinnaker.orca.clouddriver.model.Manifest;
 import com.netflix.spinnaker.orca.clouddriver.model.ManifestCoordinates;
+import com.netflix.spinnaker.orca.clouddriver.model.ServerGroup;
 import java.util.List;
 import java.util.Map;
 import org.apache.commons.lang3.StringUtils;
@@ -74,6 +75,13 @@ public Response getServerGroupFromCluster(
         .getServerGroupFromCluster(app, account, cluster, serverGroup, region, cloudProvider);
   }
 
+  @Override
+  public List<ServerGroup> getServerGroupsFromClusterTyped(
+      String app, String account, String cluster, String serverGroup, String cloudProvider) {
+    return getService()
+        .getServerGroupsFromClusterTyped(app, account, cluster, serverGroup, cloudProvider);
+  }
+
   @Override
   public Response getServerGroups(String app) {
     return getService().getServerGroups(app);
@@ -91,7 +99,7 @@ public Response getServerGroup(String account, String serverGroup, String region
   }
 
   @Override
-  public Response getTargetServerGroup(
+  public ServerGroup getTargetServerGroup(
       String app,
       String account,
       String cluster,

orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/OortService.java

@@ -20,6 +20,7 @@
 import com.netflix.spinnaker.orca.clouddriver.model.Ami;
 import com.netflix.spinnaker.orca.clouddriver.model.Manifest;
 import com.netflix.spinnaker.orca.clouddriver.model.ManifestCoordinates;
+import com.netflix.spinnaker.orca.clouddriver.model.ServerGroup;
 import java.util.List;
 import java.util.Map;
 import retrofit.client.Response;
@@ -51,6 +52,15 @@ Response getServerGroupFromCluster(
       @Query("region") String region,
       @Path("cloudProvider") String cloudProvider);
 
+  @GET(
+      "/applications/{app}/clusters/{account}/{cluster}/{cloudProvider}/serverGroups/{serverGroup}")
+  List<ServerGroup> getServerGroupsFromClusterTyped(
+      @Path("app") String app,
+      @Path("account") String account,
+      @Path("cluster") String cluster,
+      @Path("serverGroup") String serverGroup,
+      @Path("cloudProvider") String cloudProvider);
+
   @GET("/manifests/{account}/_/{manifest}")
   Manifest getManifest(
       @Path("account") String account,
@@ -97,7 +107,7 @@ Response getServerGroup(
 
   @GET(
       "/applications/{app}/clusters/{account}/{cluster}/{cloudProvider}/{scope}/serverGroups/target/{target}")
-  Response getTargetServerGroup(
+  ServerGroup getTargetServerGroup(
       @Path("app") String app,
       @Path("account") String account,
       @Path("cluster") String cluster,

orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/support/TargetServerGroupResolverSpec.groovy

@@ -19,8 +19,11 @@ package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.kork.core.RetrySupport
 import com.netflix.spinnaker.orca.clouddriver.OortService
+import com.netflix.spinnaker.orca.clouddriver.model.ServerGroup
 import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl
+import retrofit.RestAdapter
 import retrofit.RetrofitError
+import retrofit.client.Client
 import retrofit.client.Response
 import retrofit.mime.TypedString
 import spock.lang.Specification
@@ -44,6 +47,92 @@ class TargetServerGroupResolverSpec extends Specification {
     retrySupport: retrySupport
   )
 
+  @Unroll
+  def "resolveByParams(target) throws a ConversionError with an invalid response (#description)"() {
+    given:
+    Client client = Mock(Client)
+
+    OortService oortService =
+        new RestAdapter.Builder()
+            .setEndpoint("clouddriver")
+            .setClient(client)
+            .build()
+            .create(OortService.class);
+
+    TargetServerGroupResolver targetServerGroupResolver = new TargetServerGroupResolver(
+      oortService: oortService,
+      mapper: mapper,
+      retrySupport: retrySupport
+    )
+
+    when:
+    def tsgs = targetServerGroupResolver.resolveByParams(new TargetServerGroup.Params(
+      cloudProvider: "abc",
+      cluster: "test-app",
+      credentials: "testCreds",
+      locations: [new Location(type: Location.Type.REGION, value: "north-pole")],
+      target: TargetServerGroup.Params.Target.current_asg,
+    ))
+
+    then:
+    // Expect multiple invocations due to retries.
+    15 * client.execute(_) >> new Response("clouddriver", 200, 'ok', [], new TypedString(responseBody))
+
+    RetrofitError retrofitError = thrown(RetrofitError)
+    retrofitError.kind == RetrofitError.Kind.CONVERSION
+
+    where:
+    // Another kind of invalid is something that deserializes into a map, but
+    // from which it's not possible to construct a TargetServerGroup.  That's a
+    // different test though, as it doesn't generate a conversion error.
+    description         | responseBody
+    "non-json response" | "non-json response"
+    "not a map"         | "[ \"list-element\": 5 ]"
+  }
+
+  @Unroll
+  def "resolveByParams(serverGroupName) throws a ConversionError with an invalid response (#description)"() {
+    given:
+    Client client = Mock(Client)
+
+    OortService oortService =
+        new RestAdapter.Builder()
+            .setEndpoint("clouddriver")
+            .setClient(client)
+            .build()
+            .create(OortService.class);
+
+    TargetServerGroupResolver targetServerGroupResolver = new TargetServerGroupResolver(
+      oortService: oortService,
+      mapper: mapper,
+      retrySupport: retrySupport
+    )
+
+    when:
+    def tsgs = targetServerGroupResolver.resolveByParams(new TargetServerGroup.Params(
+      cloudProvider: "gce",
+      serverGroupName: "test-app-v010",
+      credentials: "testCreds",
+      locations: [new Location(type: Location.Type.REGION, value: "north-pole")]
+    ))
+
+    then:
+    // Expect multiple invocations due to retries.
+    15 * client.execute(_) >> new Response("clouddriver", 200, 'ok', [], new TypedString(responseBody))
+
+    RetrofitError retrofitError = thrown(RetrofitError)
+    retrofitError.kind == RetrofitError.Kind.CONVERSION
+
+    where:
+    // Another kind of invalid is something that deserializes into a list, but
+    // from which it's not possible to construct a TargetServerGroup from the
+    // appropriate element.  That's a different test though, as it doesn't
+    // generate a conversion error.
+    description         | responseBody
+    "non-json response" | "non-json response"
+    "not a list"        | "{ \"some-property\": 5 }"
+  }
+
   def "should resolve to target server groups"() {
     when:
     def tsgs = subject.resolveByParams(new TargetServerGroup.Params(
@@ -56,13 +145,11 @@ class TargetServerGroupResolverSpec extends Specification {
 
     then:
     1 * oort.getTargetServerGroup("test", "testCreds", "test-app", "abc", "north-pole", "current_asg") >>
-      new Response("clouddriver", 200, 'ok', [], new TypedString(mapper.writeValueAsString([
+      new ServerGroup([
         name  : "test-app-v010",
-        region: "north-pole",
-        data  : 123,
-      ])))
+        region: "north-pole"
+      ])
     tsgs.size() == 1
-    // TODO: is data a real property? tsgs[0].data == 123
     tsgs[0].getLocation()
     tsgs[0].getLocation().type == Location.Type.REGION
     tsgs[0].getLocation().value == "north-pole"
@@ -76,15 +163,12 @@ class TargetServerGroupResolverSpec extends Specification {
     ))
 
     then:
-    1 * oort.getServerGroupFromCluster("test", "testCreds", "test-app", "test-app-v010", null, "gce") >>
-      new Response("clouddriver", 200, 'ok', [], new TypedString(mapper.writeValueAsString([[
-                                                                                              name  : "test-app-v010",
-                                                                                              region: "north-pole",
-                                                                                              data  : 123,
-                                                                                              type  : "gce",
-                                                                                            ]])))
+    1 * oort.getServerGroupsFromClusterTyped("test", "testCreds", "test-app", "test-app-v010", "gce") >>
+      [new ServerGroup([name  : "test-app-v010",
+                       region: "north-pole",
+                       type  : "gce",
+                       ])]
     tsgs.size() == 1
-    // TODO: is data a real property? tsgs[0].data == 123
     tsgs[0].getLocation()
     tsgs[0].getLocation().type == Location.Type.REGION
     tsgs[0].getLocation().value == "north-pole"
@@ -192,7 +276,7 @@ class TargetServerGroupResolverSpec extends Specification {
 
     when:
     try {
-      capturedResult = subject.fetchWithRetries(Map, 10, 1) {
+      capturedResult = subject.fetchWithRetries {
         invocationCount++
         throw exception
       }
@@ -206,12 +290,12 @@ class TargetServerGroupResolverSpec extends Specification {
 
     where:
     exception                                         || expectNull || expectedInvocationCount
-    new IllegalStateException("should retry")         || false      || 10
-    retrofitError(RetrofitError.Kind.UNEXPECTED, 400) || false      || 10
-    retrofitError(RetrofitError.Kind.HTTP, 500)       || false      || 10
+    new IllegalStateException("should retry")         || false      || TargetServerGroupResolver.NUM_RETRIES
+    retrofitError(RetrofitError.Kind.UNEXPECTED, 400) || false      || TargetServerGroupResolver.NUM_RETRIES
+    retrofitError(RetrofitError.Kind.HTTP, 500)       || false      || TargetServerGroupResolver.NUM_RETRIES
     retrofitError(RetrofitError.Kind.HTTP, 404)       || true       || 1      // a 404 should short-circuit and return null
-    retrofitError(RetrofitError.Kind.NETWORK, 0)      || false      || 10
-    retrofitError(RetrofitError.Kind.HTTP, 429)       || false      || 10
+    retrofitError(RetrofitError.Kind.NETWORK, 0)      || false      || TargetServerGroupResolver.NUM_RETRIES
+    retrofitError(RetrofitError.Kind.HTTP, 429)       || false      || TargetServerGroupResolver.NUM_RETRIES
   }
 
   RetrofitError retrofitError(RetrofitError.Kind kind, int status) {

orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/kato/pipeline/support/SourceResolverSpec.groovy

@@ -21,11 +21,10 @@ import com.netflix.spinnaker.kork.core.RetrySupport
 import com.netflix.spinnaker.orca.clouddriver.CloudDriverService
 import com.netflix.spinnaker.orca.clouddriver.ModelUtils
 import com.netflix.spinnaker.orca.clouddriver.OortService
+import com.netflix.spinnaker.orca.clouddriver.model.ServerGroup
 import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroupResolver
 import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl
 import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl
-import retrofit.client.Response
-import retrofit.mime.TypedString
 import spock.lang.Specification
 import spock.lang.Unroll
 
@@ -132,12 +131,12 @@ class SourceResolverSpec extends Specification {
       'app-test',
       'aws',
       'us-west-1',
-      'current_asg_dynamic') >> new Response('http://oort.com', 200, 'Okay', [], new TypedString('''\
-    {
+      'current_asg_dynamic') >> new ServerGroup(
+    [
       "name": "app-test-v009",
       "region": "us-west-1",
       "createdTime": 1
-    }'''.stripIndent()))
+    ])
 
     source?.account == 'test'
     source?.region == 'us-west-1'
@@ -181,12 +180,12 @@ class SourceResolverSpec extends Specification {
       'app-test',
       'cloudfoundry',
       'org2 > space2',
-      'current_asg_dynamic') >> new Response('http://oort.com', 200, 'Okay', [], new TypedString('''\
-    {
+      'current_asg_dynamic') >> new ServerGroup(
+    [
       "name": "app-test-v009",
       "region": "org2 > space2",
       "createdTime": 1
-    }'''.stripIndent()))
+    ])
 
     source?.account == 'test2'
     source?.region == 'org2 > space2'