fix(rollback): pass authentication context and consider errors fatal

This avoids a potentially dangerous scenario where we roll back to a server group of size 0 and fail to look it up for some reason, e.g. if it is in a restricted account and we fail to pass the credentials along to clouddriver.
spinnaker · Oct 2, 2019 · cb11ede · cb11ede
1 parent 695d6a2
commit cb11ede
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 7 deletions.
diff --git a/.../netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollback.groovy b/.../netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollback.groovy
@@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.rollback
 
 import com.fasterxml.jackson.annotation.JsonIgnore
 import com.netflix.spinnaker.kork.core.RetrySupport
+import com.netflix.spinnaker.kork.exceptions.SpinnakerException
 import com.netflix.spinnaker.orca.clouddriver.pipeline.providers.aws.ApplySourceServerGroupCapacityStage
 import com.netflix.spinnaker.orca.clouddriver.pipeline.providers.aws.CaptureSourceServerGroupCapacityStage
 import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.DisableServerGroupStage
@@ -29,14 +30,13 @@ import com.netflix.spinnaker.orca.kato.pipeline.support.ResizeStrategy
 import com.netflix.spinnaker.orca.pipeline.WaitStage
 import com.netflix.spinnaker.orca.pipeline.model.Stage
 import com.netflix.spinnaker.orca.pipeline.model.SyntheticStageOwner
-import groovy.transform.TimedInterrupt
+import com.netflix.spinnaker.security.AuthenticatedRequest
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 
 import javax.annotation.Nullable
 import java.util.concurrent.Callable
 import java.util.concurrent.ExecutorService
-import java.util.concurrent.Executors
 import java.util.concurrent.TimeUnit
 
 import static com.netflix.spinnaker.orca.pipeline.StageDefinitionBuilder.newStage
@@ -155,17 +155,20 @@ class ExplicitRollback implements Rollback {
       // we use an executor+future to timebox how long we can spend in this remote call
       // because we are in a StartStage message and need to response quickly
       // this is a bit of a hack, the proper long term fix would be to encapsulate this remote call in a task
-      executor.submit((Callable) {
+      Callable authenticatedRequest = AuthenticatedRequest.propagate({
         return oortHelper.getTargetServerGroup(
           fromContext.credentials,
           serverGroupName,
           fromContext.location
         )
-      }).get(5, TimeUnit.SECONDS)
+      })
+
+      executor.submit(authenticatedRequest)
+        .get(5, TimeUnit.SECONDS)
         .get()  // not sure what would cause the Optional to not be present but we would catch and log it
     } catch(Exception e) {
-      log.error('Skipping resize stage because there was an error looking up {}', serverGroupName, e)
-      return null
+      log.error('Could not generate resize stage because there was an error looking up {}', serverGroupName, e)
+      throw new SpinnakerException("failed to look up ${serverGroupName}", e)
     }
   }
 

diff --git a/...flix/spinnaker/orca/clouddriver/tasks/servergroup/ServerGroupCacheForceRefreshTask.groovy b/...flix/spinnaker/orca/clouddriver/tasks/servergroup/ServerGroupCacheForceRefreshTask.groovy
@@ -16,7 +16,6 @@
 
 package com.netflix.spinnaker.orca.clouddriver.tasks.servergroup
 
-import com.netflix.discovery.converters.Auto
 import com.netflix.spectator.api.Id
 import com.netflix.spectator.api.Registry
 

diff --git a/...flix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollbackSpec.groovy b/...flix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollbackSpec.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.rollback
 
+import com.netflix.spinnaker.kork.exceptions.SpinnakerException
 import com.netflix.spinnaker.orca.clouddriver.pipeline.providers.aws.ApplySourceServerGroupCapacityStage
 import com.netflix.spinnaker.orca.clouddriver.pipeline.providers.aws.CaptureSourceServerGroupCapacityStage
 import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.DisableServerGroupStage
@@ -191,6 +192,9 @@ class ExplicitRollbackSpec extends Specification {
     def afterStages = allStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER }
 
     then:
+    1 * oortHelper.getTargetServerGroup(_, rollbackServerGroupName, _) >> Optional.of(serverGroup(rollbackServerGroupName, 2))
+    1 * oortHelper.getTargetServerGroup(_, restoreServerGroupName, _) >> Optional.of(serverGroup(restoreServerGroupName, 1))
+
     afterStages*.type.contains("wait") == waitStageExpected
     afterStages*.type.indexOf("wait") < afterStages*.type.indexOf("disableServerGroup")
 
@@ -200,4 +204,13 @@ class ExplicitRollbackSpec extends Specification {
     0                         || false
     1                         || true
   }
+
+  def "server group lookups failures are fatal"() {
+    when:
+    rollback.buildStages(stage)
+
+    then:
+    1 * oortHelper.getTargetServerGroup(_, rollbackServerGroupName, _) >> { throw new Exception(":(") }
+    thrown(SpinnakerException)
+  }
 }