fix(amazon): do not tag/share public images on AllowLaunch (#1579)

clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/AmiIdResolver.groovy

@@ -41,7 +41,7 @@ class AmiIdResolver {
     }
     Image resolvedImage = amazonEC2.describeImages(req)?.images?.getAt(0)
     if (resolvedImage) {
-      return new ResolvedAmiResult(nameOrId, region, resolvedImage.imageId, resolvedImage.virtualizationType, resolvedImage.ownerId, resolvedImage.blockDeviceMappings)
+      return new ResolvedAmiResult(nameOrId, region, resolvedImage.imageId, resolvedImage.virtualizationType, resolvedImage.ownerId, resolvedImage.blockDeviceMappings, resolvedImage.public)
     }
 
     return null

clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/ResolvedAmiResult.groovy

@@ -30,4 +30,5 @@ class ResolvedAmiResult {
   String virtualizationType
   String ownerId
   List<BlockDeviceMapping> blockDeviceMappings
+  Boolean isPublic
 }

clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/ops/AllowLaunchAtomicOperation.groovy

@@ -36,8 +36,6 @@ import org.springframework.beans.factory.annotation.Autowired
 class AllowLaunchAtomicOperation implements AtomicOperation<ResolvedAmiResult> {
   private static final String BASE_PHASE = "ALLOW_LAUNCH"
 
-  private static final int MAX_TARGET_DESCRIBE_ATTEMPTS = 2
-
   private static Task getTask() {
     TaskRepository.threadLocalTask.get()
   }
@@ -68,8 +66,15 @@ class AllowLaunchAtomicOperation implements AtomicOperation<ResolvedAmiResult> {
       throw new IllegalArgumentException("unable to resolve AMI imageId from $description.amiName")
     }
 
+    // If the AMI is public, this is a no-op
+    if (resolvedAmi.isPublic) {
+      task.updateStatus BASE_PHASE, "AMI is public, no need to allow launch"
+      return resolvedAmi
+    }
+
     // If the AMI was created/owned by a different account, switch to using that for modifying the image
     if (resolvedAmi.ownerId != sourceCredentials.accountId) {
+      if (resolvedAmi.getRegion())
       sourceCredentials = accountCredentialsProvider.all.find { accountCredentials ->
         accountCredentials instanceof NetflixAmazonCredentials &&
           ((AmazonCredentials) accountCredentials).accountId == resolvedAmi.ownerId
@@ -85,7 +90,7 @@ class AllowLaunchAtomicOperation implements AtomicOperation<ResolvedAmiResult> {
     OperationPoller.retryWithBackoff({o ->
       sourceAmazonEC2.modifyImageAttribute(new ModifyImageAttributeRequest().withImageId(resolvedAmi.amiId).withLaunchPermission(
         new LaunchPermissionModifications().withAdd(new LaunchPermission().withUserId(targetCredentials.accountId))))
-    }, 500, 3);
+    }, 500, 3)
 
     if (sourceCredentials == targetCredentials) {
       task.updateStatus BASE_PHASE, "Tag replication not required"

clouddriver-aws/src/test/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/ops/AllowLaunchAtomicOperationUnitSpec.groovy

@@ -216,6 +216,44 @@ class AllowLaunchAtomicOperationUnitSpec extends Specification {
 
   }
 
+  void "should return resolved public AMI without performing additional operations"() {
+    setup:
+    def ownerCredentials = TestCredential.named('owner')
+    def targetCredentials = TestCredential.named('target')
+    def ownerAmazonEc2 = Mock(AmazonEC2)
+    def targetAmazonEc2 = Mock(AmazonEC2)
+
+    def description = new AllowLaunchDescription(account: 'target', amiName: 'ami-123456', region: 'us-west-1', credentials: ownerCredentials)
+    def op = new AllowLaunchAtomicOperation(description)
+    op.amazonClientProvider = Mock(AmazonClientProvider)
+    op.accountCredentialsProvider = Mock(AccountCredentialsProvider)
+
+    when:
+    op.operate([])
+
+    then:
+
+    with(op.accountCredentialsProvider) {
+      1 * getCredentials('target') >> targetCredentials
+    }
+
+    with(op.amazonClientProvider) {
+      1 * getAmazonEC2(targetCredentials, _, true) >> targetAmazonEc2
+      1 * getAmazonEC2(ownerCredentials, _, true) >> ownerAmazonEc2
+    }
+
+    with(ownerAmazonEc2) {
+      1 * describeImages(_) >> new DescribeImagesResult().withImages(
+        new Image()
+          .withImageId("ami-123456")
+          .withOwnerId(ownerCredentials.accountId)
+          .withPublic(true))
+    }
+
+    0 * _
+
+  }
+
   Closure<DescribeTagsResult> constructDescribeTagsResult = { Map tags ->
     new DescribeTagsResult(tags: tags.collect {new TagDescription(key: it.key, value: it.value) })
   }