refactor(clouddriver): Make OortHelper use typed cluster from CloudDr…

…iverService (cherry picked from commit 1c61168450586bf120506240134c55962fb280bd)
ziggyw · Apr 28, 2021 · e695a40 · e695a40
1 parent 2b1d4ef
commit e695a40
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 69 deletions.
diff --git a/...louddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/OortHelper.groovy b/...louddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/OortHelper.groovy
@@ -16,73 +16,57 @@
 
 package com.netflix.spinnaker.orca.clouddriver.utils
 
-import com.fasterxml.jackson.databind.ObjectMapper
-import com.netflix.spinnaker.orca.clouddriver.OortService
-import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroup
+import com.netflix.spinnaker.orca.clouddriver.CloudDriverService
+import com.netflix.spinnaker.orca.clouddriver.model.Cluster
+import com.netflix.spinnaker.orca.clouddriver.model.ServerGroup
 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
 
 /**
  * Helper methods for filtering Cluster/ASG/Instance information from Oort
  */
 @Component
 class OortHelper {
 
-  private final OortService oortService
-
-  private final ObjectMapper objectMapper
-
-  private final JacksonConverter converter
+  private final CloudDriverService cloudDriverService
 
   @Autowired
-  OortHelper(OortService oortService, ObjectMapper objectMapper) {
-    this.oortService = oortService
-    this.objectMapper = objectMapper
-    converter = new JacksonConverter(objectMapper)
-  }
-
-  private <T> T convert(Response response, Class<T> type) {
-    try {
-      return type.cast(converter.fromBody(response.body, type))
-    } catch (ConversionException ce) {
-      throw RetrofitError.conversionError(response.url, response, converter, type, ce)
-    }
+  OortHelper(CloudDriverService cloudDriverService) {
+    this.cloudDriverService = cloudDriverService
   }
 
-  Map getInstancesForCluster(Map context, String expectedAsgName = null, boolean expectOneAsg = false, boolean failIfAnyInstancesUnhealthy = false) {
+  // TODO: failIfAnyInstancesUnhealthy seems to only be false in tasks that call this
+  Map<String, Object> getInstancesForCluster(Map<String, Object> context, String expectedAsgName, boolean expectOneAsg, boolean failIfAnyInstancesUnhealthy) {
     // infer the app from the cluster prefix since this is used by quip and we want to be able to quick patch different apps from the same pipeline
-    def app
-    def clusterName
+    String app
+    String clusterName
     if (expectedAsgName) {
       app = expectedAsgName.substring(0, expectedAsgName.indexOf("-"))
       clusterName = expectedAsgName.substring(0, expectedAsgName.lastIndexOf("-"))
-    } else if (context?.clusterName?.indexOf("-") > 0) {
+    } else if (context.clusterName?.indexOf("-") > 0) {
       app = context.clusterName.substring(0, context.clusterName.indexOf("-"))
       clusterName = context.clusterName
     } else {
       app = context.clusterName
       clusterName = context.clusterName
     }
 
-    def response = oortService.getCluster(app, context.account, clusterName, context.cloudProvider ?: context.providerType ?: "aws")
-    def oortCluster = convert(response, Map)
-    def instanceMap = [:]
+    String account = context.account
+    String cloudProvider = context.cloudProvider ?: context.providerType ?: "aws"
+
+    Cluster oortCluster = cloudDriverService.getCluster(app, account, clusterName, cloudProvider)
 
     if (!oortCluster || !oortCluster.serverGroups) {
       throw new RuntimeException("unable to find any server groups")
     }
 
-    def region = context.region ?: context.source.region
+    String region = context.region ?: context.source.region
 
     if (!region) {
       throw new RuntimeException("unable to determine region")
     }
 
-    def asgsForCluster = oortCluster.serverGroups.findAll {
+    List<ServerGroup> asgsForCluster = oortCluster.serverGroups.findAll {
       it.region == region
     }
 
@@ -103,31 +87,29 @@ class OortHelper {
       }
     }
 
+    Map<String, Object> instanceMap = [:]
     searchAsg.instances.each { instance ->
       String hostName = instance.publicDnsName
       if (!hostName || hostName.isEmpty()) { // some instances dont have a public address, fall back to the private ip
         hostName = instance.privateIpAddress
       }
 
-      String healthCheckUrl
-      instance.health.eachWithIndex { health, idx ->
+      String healthCheckUrl = null
+      instance.health.each { health ->
         if (health.healthCheckUrl != null && !health.healthCheckUrl.isEmpty()) {
           healthCheckUrl = health.healthCheckUrl
         }
       }
 
-      def status = instance.health.find { healthItem ->
-        healthItem.find {
-          key, value ->
-            key == "status"
-        }
-      }?.status
+      def status = instance.health.findResult { healthItem ->
+        healthItem.status // TODO: should this be state?
+      }
 
       if (failIfAnyInstancesUnhealthy && (!healthCheckUrl || !status || status != "UP")) {
         throw new RuntimeException("at least one instance is DOWN or in the STARTING state, exiting")
       }
 
-      Map instanceInfo = [
+      Map<String, Object> instanceInfo = [
           hostName: hostName,
           healthCheckUrl: healthCheckUrl,
           privateIpAddress: instance.privateIpAddress

diff --git a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/model/Health.java b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/model/Health.java
@@ -4,7 +4,10 @@
 
 @Data
 public class Health {
-  String type;
-  HealthState state;
-  String healthClass;
+  public String type;
+  public HealthState state;
+  public String healthClass;
+  public String healthCheckUrl; // TODO: is this real? I don't see it in CloudDriver
+  public String
+      status; // TODO: is this real or a bug in OortHelper? The path that calls it seems unused
 }
diff --git a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/model/Instance.java b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/model/Instance.java
@@ -14,6 +14,7 @@ public class Instance {
   String zone;
   String cloudProvider;
   String privateIpAddress;
+  String publicDnsName;
 
   /**
    * Method to cut down on the instance data that is serialized to the context

diff --git a/...iver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/CloudDriverServiceSpec.groovy b/...iver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/CloudDriverServiceSpec.groovy
@@ -1,6 +1,5 @@
 package com.netflix.spinnaker.orca.clouddriver
 
-
 import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper
 import retrofit.RetrofitError
 import retrofit.client.Response
@@ -12,7 +11,8 @@ import spock.lang.Unroll
 class CloudDriverServiceSpec extends Specification {
 
   OortService oortService = Mock()
-  @Subject CloudDriverService cloudDriverService = new CloudDriverService(oortService, OrcaObjectMapper.instance)
+  @Subject
+  CloudDriverService cloudDriverService = new CloudDriverService(oortService, OrcaObjectMapper.instance)
 
   @Unroll
   def "should support fetching a target server group that does not exist"() {

diff --git a/...driver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/OortHelperSpec.groovy b/...driver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/OortHelperSpec.groovy
@@ -16,22 +16,15 @@
 
 package com.netflix.spinnaker.orca.clouddriver.utils
 
-import com.fasterxml.jackson.databind.ObjectMapper
-import com.netflix.spinnaker.orca.clouddriver.OortService
-import com.netflix.spinnaker.orca.clouddriver.utils.OortHelper
-import retrofit.RetrofitError
-import retrofit.client.Response
-import retrofit.mime.TypedString
+import com.netflix.spinnaker.orca.clouddriver.CloudDriverService
+import com.netflix.spinnaker.orca.clouddriver.model.Cluster
+import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper
 import spock.lang.Specification
 import spock.lang.Subject
-import spock.lang.Unroll
-
-import static java.net.HttpURLConnection.HTTP_NOT_FOUND
 
 class OortHelperSpec extends Specification {
-  ObjectMapper objectMapper = new ObjectMapper()
-  OortService oortService = Mock()
-  @Subject oortHelper = new OortHelper(oortService, objectMapper)
+  CloudDriverService cloudDriverService = Mock()
+  @Subject oortHelper = new OortHelper(cloudDriverService)
 
   def "getInstancesForCluster fails if > 1 asg in the cluster"() {
     given:
@@ -54,9 +47,8 @@ class OortHelperSpec extends Specification {
       }]
     }
     '''.stripIndent()
-    Response response = new Response('http://oort', 200, 'OK', [], new TypedString(oortResponse))
     Map deployContext = ["region" : "us-west-2", "account" : "prod", "kato.tasks" : [[resultObjects : [[ancestorServerGroupNameByRegion: ["us-west-2" : "myapp-v000"]],[serverGroupNameByRegion : ["us-west-2" : "myapp-v002"]]]]]]
-    1 * oortService.getCluster("myapp", "prod", "myapp", "aws") >> response
+    1 * cloudDriverService.getCluster("myapp", "prod", "myapp", "aws") >> cluster(oortResponse)
 
     when:
     oortHelper.getInstancesForCluster(deployContext, "myapp-v002", true, true)
@@ -80,9 +72,8 @@ class OortHelperSpec extends Specification {
       }]
     }
     '''.stripIndent()
-    Response response = new Response('http://oort', 200, 'OK', [], new TypedString(oortResponse))
     Map deployContext = ["region" : "us-west-2", "account" : "prod", "kato.tasks" : [[resultObjects : [[ancestorServerGroupNameByRegion: ["us-west-2" : "myapp-v000"]],[serverGroupNameByRegion : ["us-west-2" : "myapp-v002"]]]]]]
-    1 * oortService.getCluster("myapp", "prod", "myapp", "aws") >> response
+    1 * cloudDriverService.getCluster("myapp", "prod", "myapp", "aws") >> cluster(oortResponse)
 
     when:
     oortHelper.getInstancesForCluster(deployContext, "myapp-v002", true, true)
@@ -106,16 +97,15 @@ class OortHelperSpec extends Specification {
       }]
     }
     '''.stripIndent()
-    Response response = new Response('http://oort', 200, 'OK', [], new TypedString(oortResponse))
     Map deployContext = ["region" : "us-west-2", "account" : "prod", "kato.tasks" : [[resultObjects : [[ancestorServerGroupNameByRegion: ["us-west-2" : "myapp-v000"]],[serverGroupNameByRegion : ["us-west-2" : "myapp-v002"]]]]]]
-    1 * oortService.getCluster("myapp", "prod", "myapp", "aws") >> response
+    1 * cloudDriverService.getCluster("myapp", "prod", "myapp", "aws") >> cluster(oortResponse)
 
     when:
     def result = oortHelper.getInstancesForCluster(deployContext, "myapp-v002", true, true)
 
     then:
-    result.get(1).healthCheckUrl == "http://foo/bar"
-    result.get(2).healthCheckUrl == "http://foo2/bar2"
+    result.get('1').healthCheckUrl == "http://foo/bar"
+    result.get('2').healthCheckUrl == "http://foo2/bar2"
   }
 
   def "getInstancesForCluster passes if any instances are down/starting and failIfAnyInstancesUnhealthy == false"() {
@@ -136,14 +126,17 @@ class OortHelperSpec extends Specification {
       }]
     }
     '''.stripIndent()
-    Response response = new Response('http://oort', 200, 'OK', [], new TypedString(oortResponse))
     Map deployContext = ["region" : "us-west-2", "account" : "prod", "kato.tasks" : [[resultObjects : [[ancestorServerGroupNameByRegion: ["us-west-2" : "myapp-v000"]],[serverGroupNameByRegion : ["us-west-2" : "myapp-v002"]]]]]]
-    1 * oortService.getCluster("myapp", "prod", "myapp", "aws") >> response
+    1 * cloudDriverService.getCluster("myapp", "prod", "myapp", "aws") >> cluster(oortResponse)
 
     when:
     def result = oortHelper.getInstancesForCluster(deployContext, "myapp-v002", true, false)
 
     then:
     result.size() == 4
   }
+
+  static Cluster cluster(String json) {
+    OrcaObjectMapper.instance.readValue(json, Cluster)
+  }
 }