fix(ecs): new task ID format is valid for instance details and termin…

…ation (#3983) Fixes spinnaker/spinnaker#4799
spinnaker · Aug 30, 2019 · 706333b · 706333b
1 parent 1a35154
commit 706333b
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 10 deletions.
diff --git a/...x/spinnaker/clouddriver/ecs/deploy/validators/TerminateInstancesDescriptionValidator.java b/...x/spinnaker/clouddriver/ecs/deploy/validators/TerminateInstancesDescriptionValidator.java
@@ -28,8 +28,9 @@
 @EcsOperation(AtomicOperations.TERMINATE_INSTANCES)
 @Component("ecsTerminateInstancesDescriptionValidator")
 public class TerminateInstancesDescriptionValidator extends CommonValidator {
-  public static final Pattern TASK_ID_PATTERN =
+  public static final Pattern OLD_TASK_ID_PATTERN =
       Pattern.compile("[\\da-f]{8}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{12}");
+  public static final Pattern NEW_TASK_ID_PATTERN = Pattern.compile("[\\da-f]{32}");
 
   public TerminateInstancesDescriptionValidator() {
     super("terminateInstancesDescription");
@@ -50,7 +51,8 @@ public void validate(List priorDescriptions, Object description, Errors errors)
           .getEcsTaskIds()
           .forEach(
               taskId -> {
-                if (!TASK_ID_PATTERN.matcher(taskId).find()) {
+                if (!OLD_TASK_ID_PATTERN.matcher(taskId).find()
+                    && !NEW_TASK_ID_PATTERN.matcher(taskId).find()) {
                   rejectValue(errors, "ecsTaskIds." + taskId, "invalid");
                 }
               });

diff --git a/...ver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/view/EcsInstanceProvider.java b/...ver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/view/EcsInstanceProvider.java
@@ -100,9 +100,19 @@ public String getConsoleOutput(String account, String region, String id) {
   }
 
   private boolean isValidId(String id, String region) {
-    String idRegex = "[\\da-f]{8}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{12}";
-    String idOnly = String.format("^%s$", idRegex);
-    String arn = String.format("arn:aws:ecs:%s:\\d*:task/%s", region, idRegex);
-    return id.matches(idOnly) || id.matches(arn);
+    String oldTaskIdRegex = "[\\da-f]{8}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{12}";
+    String newTaskIdRegex = "[\\da-f]{32}";
+    String clusterNameRegex = "[a-zA-Z0-9\\-_]{1,255}";
+    String oldTaskIdOnly = String.format("^%s$", oldTaskIdRegex);
+    String newTaskIdOnly = String.format("^%s$", newTaskIdRegex);
+    // arn:aws:ecs:region:account-id:task/task-id
+    String oldTaskArn = String.format("arn:aws:ecs:%s:\\d*:task/%s", region, oldTaskIdRegex);
+    // arn:aws:ecs:region:account-id:task/cluster-name/task-id
+    String newTaskArn =
+        String.format("arn:aws:ecs:%s:\\d*:task/%s/%s", region, clusterNameRegex, newTaskIdRegex);
+    return id.matches(oldTaskIdOnly)
+        || id.matches(newTaskIdOnly)
+        || id.matches(oldTaskArn)
+        || id.matches(newTaskArn);
   }
 }
diff --git a/...nnaker/clouddriver/ecs/deploy/validators/TerminateInstanceDescriptionValidatorSpec.groovy b/...nnaker/clouddriver/ecs/deploy/validators/TerminateInstanceDescriptionValidatorSpec.groovy
@@ -65,7 +65,7 @@ class TerminateInstanceDescriptionValidatorSpec extends AbstractValidatorSpec {
     def description = new TerminateInstancesDescription()
     description.credentials = TestCredential.named('test')
     description.region = 'us-west-1'
-    description.ecsTaskIds = ['deadbeef-33f7-4637-ab84-606f0c77af42']
+    description.ecsTaskIds = ['deadbeef-33f7-4637-ab84-606f0c77af42', 'deadbeef33f74637ab84606f0c77af42']
     description
   }
 }
diff --git a/...src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/view/EcsInstanceProviderSpec.groovy b/...src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/view/EcsInstanceProviderSpec.groovy
@@ -29,6 +29,7 @@ import com.netflix.spinnaker.clouddriver.ecs.services.ContainerInformationServic
 import com.netflix.spinnaker.clouddriver.security.AccountCredentialsProvider
 import spock.lang.Specification
 import spock.lang.Subject
+import spock.lang.Unroll
 
 class EcsInstanceProviderSpec extends Specification {
   def accountCredentialsProvider = Mock(AccountCredentialsProvider)
@@ -41,12 +42,10 @@ class EcsInstanceProviderSpec extends Specification {
   def provider = new EcsInstanceProvider(containerInformationService, taskCacheClient,
                                          containerInstanceCacheClient)
 
+  @Unroll
   def 'should return an EcsTask'() {
     given:
-    def region = 'us-west-1'
     def account = 'test-account'
-    def taskId = 'deadbeef-94f3-4994-8e81-339c4d1be1ba'
-    def taskArn = 'arn:aws:ecs:' + region + ':123456789012:task/' + taskId
     def address = '127.0.0.1:1337'
     def startTime = System.currentTimeMillis()
 
@@ -79,5 +78,26 @@ class EcsInstanceProviderSpec extends Specification {
 
     then:
     taskInstance == ecsTask
+
+    where:
+    region      | taskId                                 | taskArn
+    'us-west-1' | 'deadbeef-94f3-4994-8e81-339c4d1be1ba' | 'arn:aws:ecs:us-west-1:123456789012:task/deadbeef-94f3-4994-8e81-339c4d1be1ba'
+    'us-west-1' | 'deadbeef94f349948e81339c4d1be1ba'     | 'arn:aws:ecs:us-west-1:123456789012:task/my-cluster-123/deadbeef94f349948e81339c4d1be1ba'
+    'us-west-1' | 'arn:aws:ecs:us-west-1:123456789012:task/deadbeef-94f3-4994-8e81-339c4d1be1ba' | 'foo'
+    'us-west-1' | 'arn:aws:ecs:us-west-1:123456789012:task/my-cluster-123/deadbeef94f349948e81339c4d1be1ba' | 'foo'
+  }
+
+  @Unroll
+  def 'should return null for invalid ECS task ID'() {
+    when:
+    def taskInstance = provider.getInstance(account, region, taskId)
+
+    then:
+    taskInstance == null
+
+    where:
+    account        | region      | taskId
+    'test-account' | 'us-west-1' | 'i-deadbeef'
+    'test-account' | 'us-west-1' | 'arn:aws:ecs:us-west-1:123456789012:cluster/my-cluster-name'
   }
 }