fix(traffic-guards): make front50 look up first (#3729)

* fix(traffic-guards): make front50 look up first We need to check if traffic guards are enabled for this cluster in front50 first. If they are not enabled, no need to make any clouddriver calls. * fix(tests): add a few missing front50 mock calls
spinnaker · Jun 10, 2020 · 46866f0 · 46866f0
1 parent bbd0204
commit 46866f0
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 25 deletions.
diff --git a/...louddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java b/...louddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java
@@ -163,6 +163,16 @@ public void verifyTrafficRemoval(
       Location location,
       String cloudProvider,
       String operationDescriptor) {
+
+    if (!hasDisableLock(serverGroupMoniker, account, location)) {
+      log.debug(
+          "No traffic guard configured for '{}' in {}/{}",
+          serverGroupName,
+          account,
+          location.getValue());
+      return;
+    }
+
     Optional<Map> cluster =
         oortHelper.getCluster(
             serverGroupMoniker.getApp(), account, serverGroupMoniker.getCluster(), cloudProvider);
@@ -202,25 +212,42 @@ public void verifyTrafficRemoval(
                   return new TrafficGuardException(message);
                 });
 
-    verifyTrafficRemoval(serverGroupGoingAway, targetServerGroups, account, operationDescriptor);
+    verifyTrafficRemovalInternal(
+        Collections.singletonList(serverGroupGoingAway),
+        targetServerGroups,
+        account,
+        operationDescriptor);
   }
 
   public void verifyTrafficRemoval(
-      TargetServerGroup serverGroupGoingAway,
+      Collection<TargetServerGroup> serverGroupsGoingAway,
       Collection<TargetServerGroup> currentServerGroups,
       String account,
       String operationDescriptor) {
-    verifyTrafficRemoval(
-        Collections.singletonList(serverGroupGoingAway),
-        currentServerGroups,
-        account,
-        operationDescriptor);
+    if (serverGroupsGoingAway == null || serverGroupsGoingAway.isEmpty()) {
+      return;
+    }
+
+    TargetServerGroup someServerGroup = serverGroupsGoingAway.stream().findAny().get();
+    Location location = someServerGroup.getLocation();
+
+    if (!hasDisableLock(someServerGroup.getMoniker(), account, location)) {
+      log.debug(
+          "No traffic guard configured for '{}' in {}/{}",
+          someServerGroup.getName(),
+          account,
+          location.getValue());
+      return;
+    }
+
+    verifyTrafficRemovalInternal(
+        serverGroupsGoingAway, currentServerGroups, account, operationDescriptor);
   }
 
+  // internal call, assumes that the front50 call (hasDisableCheck) has already been performed
   // if you disable serverGroup, are there other enabled server groups in the same cluster and
   // location?
-  // TODO rz - Expose traffic guards endpoint in clouddriver
-  public void verifyTrafficRemoval(
+  private void verifyTrafficRemovalInternal(
       Collection<TargetServerGroup> serverGroupsGoingAway,
       Collection<TargetServerGroup> currentServerGroups,
       String account,
@@ -246,14 +273,7 @@ public void verifyTrafficRemoval(
             .allMatch(sg -> cluster.equals(sg.getMoniker().getCluster())),
         "server groups must all be in the same cluster but some not in " + cluster);
 
-    if (!hasDisableLock(someServerGroup.getMoniker(), account, location)) {
-      log.debug(
-          "No traffic guard configured for '{}' in {}/{}", cluster, account, location.getValue());
-      return;
-    }
-
     // let the work begin
-
     Map<String, Integer> capacityByServerGroupName =
         currentServerGroups.stream()
             .collect(Collectors.toMap(TargetServerGroup::getName, this::getServerGroupCapacity));

diff --git a/...iver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy b/...iver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy
@@ -75,15 +75,10 @@ class TrafficGuardSpec extends Specification {
     when:
     trafficGuard.verifyTrafficRemoval(targetName, moniker, "test", location, "aws", "x")
 
-    then:
+    then: 'we never look up anything in clouddriver if traffic guards are not enabled'
     notThrown(TrafficGuardException)
     1 * front50Service.get("app") >> application
-    1 * oortHelper.getCluster("app", "test", "app-foo", "aws") >> [
-      serverGroups: [
-        makeServerGroup(targetName, 1),
-        makeServerGroup(otherName, 0, 1, [isDisabled: true])
-      ]
-    ]
+    0 * oortHelper._
   }
 
   void "should throw exception when target server group is the only one enabled in cluster"() {
@@ -115,6 +110,7 @@ class TrafficGuardSpec extends Specification {
     then:
     def e = thrown(TrafficGuardException)
     e.message.startsWith("Could not find server group 'app-foo-v999'")
+    1 * front50Service.get("app") >> application
     1 * oortHelper.getCluster("app", "test", "app-foo", "aws") >> [
       serverGroups: [
         makeServerGroup(targetName, 1),
@@ -124,11 +120,15 @@ class TrafficGuardSpec extends Specification {
   }
 
   void "should be able to handle a server group in a namespace"() {
+    given:
+    addGuard([account: "test", location: "us-east-1", stack: "foo"])
+
     when:
     trafficGuard.verifyTrafficRemoval(targetName, moniker, "test", location, "aws", "x")
 
     then:
     notThrown(TrafficGuardException)
+    1 * front50Service.get("app") >> application
     1 * oortHelper.getCluster("app", "test", "app-foo", "aws") >> [
       serverGroups: [
         makeServerGroup(targetName, 2, 0, [namespace: 'us-east-1']),
@@ -368,8 +368,7 @@ class TrafficGuardSpec extends Specification {
     then:
     def e = thrown(TrafficGuardException)
     e.message.startsWith('Could not find cluster')
-//    (the front50 check has moved after the cluster check)
-//    1 * front50Service.get("app") >> application
+    1 * front50Service.get("app") >> application
     1 * oortHelper.getCluster("app", "test", "app-foo", "aws") >> Optional.empty()
   }