fix(aws): Alter AllowLaunchDescription to not overload account (#4021)

spinnaker · Sep 11, 2019 · f2c0827 · f2c0827
1 parent 16b7528
commit f2c0827
Show file tree

Hide file tree

Showing 8 changed files with 66 additions and 23 deletions.
diff --git a/...vy/com/netflix/spinnaker/clouddriver/aws/deploy/description/AllowLaunchDescription.groovy b/...vy/com/netflix/spinnaker/clouddriver/aws/deploy/description/AllowLaunchDescription.groovy
@@ -17,7 +17,7 @@
 package com.netflix.spinnaker.clouddriver.aws.deploy.description
 
 class AllowLaunchDescription extends AbstractAmazonCredentialsDescription {
-  String account
+  String targetAccount
   String amiName
   String region
 

diff --git a/...groovy/com/netflix/spinnaker/clouddriver/aws/deploy/ops/AllowLaunchAtomicOperation.groovy b/...groovy/com/netflix/spinnaker/clouddriver/aws/deploy/ops/AllowLaunchAtomicOperation.groovy
@@ -58,7 +58,7 @@ class AllowLaunchAtomicOperation implements AtomicOperation<ResolvedAmiResult> {
     task.updateStatus BASE_PHASE, "Initializing Allow Launch Operation..."
 
     def sourceCredentials = description.credentials
-    def targetCredentials = accountCredentialsProvider.getCredentials(description.account) as NetflixAmazonCredentials
+    def targetCredentials = accountCredentialsProvider.getCredentials(description.targetAccount) as NetflixAmazonCredentials
     def sourceAmazonEC2 = amazonClientProvider.getAmazonEC2(description.credentials, description.region, true)
     def targetAmazonEC2 = amazonClientProvider.getAmazonEC2(targetCredentials, description.region, true)
 
@@ -94,7 +94,7 @@ class AllowLaunchAtomicOperation implements AtomicOperation<ResolvedAmiResult> {
     if (amiLocation == ResolvedAmiLocation.TARGET) {
       task.updateStatus BASE_PHASE, "AMI found in target account: skipping allow launch"
     } else {
-      task.updateStatus BASE_PHASE, "Allowing launch of $description.amiName from $description.account"
+      task.updateStatus BASE_PHASE, "Allowing launch of $description.amiName from $description.account/$description.region to $description.targetAccount"
 
       OperationPoller.retryWithBackoff({ o ->
         sourceAmazonEC2.modifyImageAttribute(new ModifyImageAttributeRequest().withImageId(resolvedAmi.amiId).withLaunchPermission(
@@ -136,7 +136,7 @@ class AllowLaunchAtomicOperation implements AtomicOperation<ResolvedAmiResult> {
       }
     }
 
-    task.updateStatus BASE_PHASE, "Done allowing launch of $description.amiName from $description.account."
+    task.updateStatus BASE_PHASE, "Done allowing launch of $description.amiName from $description.account/$description.region to $description.targetAccount."
     resolvedAmi
   }
 

diff --git a/...y/com/netflix/spinnaker/clouddriver/aws/deploy/preprocessors/AllowLaunchPreProcessor.java b/...y/com/netflix/spinnaker/clouddriver/aws/deploy/preprocessors/AllowLaunchPreProcessor.java
@@ -0,0 +1,38 @@
+/*
+ * Copyright 2019 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.netflix.spinnaker.clouddriver.aws.deploy.preprocessors;
+
+import com.netflix.spinnaker.clouddriver.aws.deploy.description.AllowLaunchDescription;
+import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperationDescriptionPreProcessor;
+import java.util.Map;
+import org.springframework.stereotype.Component;
+
+@Component
+public class AllowLaunchPreProcessor implements AtomicOperationDescriptionPreProcessor {
+  @Override
+  public boolean supports(Class descriptionClass) {
+    return AllowLaunchDescription.class.isAssignableFrom(descriptionClass);
+  }
+
+  @Override
+  public Map process(Map description) {
+    // Backwards-compatibility from when AllowLaunch used to overload "account" from the abstract
+    // AWS description.
+    description.putIfAbsent("targetAccount", description.get("account"));
+    description.put("account", description.get("credentials"));
+    return description;
+  }
+}
diff --git a/...ker/clouddriver/aws/deploy/preprocessors/CredentialsAccountNormalizationPreProcessor.java b/...ker/clouddriver/aws/deploy/preprocessors/CredentialsAccountNormalizationPreProcessor.java
@@ -15,19 +15,23 @@
  */
 package com.netflix.spinnaker.clouddriver.aws.deploy.preprocessors;
 
+import com.netflix.spinnaker.clouddriver.aws.deploy.description.AllowLaunchDescription;
 import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperationDescriptionPreProcessor;
 import java.util.Map;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.stereotype.Component;
 
-/** Normalizes the use of `account` vs `credentials`, ensuring that both are always set. */
+/**
+ * Normalizes the use of `account` vs `credentials`, ensuring that both are always set; prefers the
+ * value from `credentials`.
+ */
 @Slf4j
 @Component
 public class CredentialsAccountNormalizationPreProcessor
     implements AtomicOperationDescriptionPreProcessor {
   @Override
   public boolean supports(Class descriptionClass) {
-    return true;
+    return !AllowLaunchDescription.class.isAssignableFrom(descriptionClass);
   }
 
   @Override
@@ -40,6 +44,7 @@ public Map process(Map description) {
           "Passed 'account' ({}) and 'credentials' ({}), but values are not equal",
           account,
           credentials);
+      description.put("account", credentials);
     }
 
     if (credentials == null && account != null) {

diff --git a/...etflix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidator.groovy b/...etflix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidator.groovy
@@ -36,10 +36,10 @@ class AllowLaunchDescriptionValidator extends DescriptionValidator<AllowLaunchDe
     if (!description.region) {
       errors.rejectValue("region", "allowLaunchDescription.region.empty")
     }
-    if (!description.account) {
-      errors.rejectValue("account", "allowLaunchDescription.account.empty")
-    } else if (!accountCredentialsProvider.all.collect { it.name }.contains(description.account)) {
-      errors.rejectValue("account", "allowLaunchDescription.account.not.configured")
+    if (!description.targetAccount) {
+      errors.rejectValue("targetAccount", "allowLaunchDescription.targetAccount.empty")
+    } else if (!accountCredentialsProvider.all.collect { it.name }.contains(description.targetAccount)) {
+      errors.rejectValue("targetAccount", "allowLaunchDescription.targetAccount.not.configured")
     }
   }
 }
diff --git a/...om/netflix/spinnaker/clouddriver/aws/deploy/ops/AllowLaunchAtomicOperationUnitSpec.groovy b/...om/netflix/spinnaker/clouddriver/aws/deploy/ops/AllowLaunchAtomicOperationUnitSpec.groovy
@@ -59,7 +59,7 @@ class AllowLaunchAtomicOperationUnitSpec extends Specification {
     def creds = Stub(AccountCredentialsProvider) {
       getCredentials('target') >> target
     }
-    def op = new AllowLaunchAtomicOperation(new AllowLaunchDescription(amiName: 'super-awesome-ami', account: 'target', credentials: source))
+    def op = new AllowLaunchAtomicOperation(new AllowLaunchDescription(amiName: 'super-awesome-ami', targetAccount: 'target', credentials: source))
     op.accountCredentialsProvider = creds
     op.amazonClientProvider = provider
 
@@ -95,7 +95,7 @@ class AllowLaunchAtomicOperationUnitSpec extends Specification {
       describeImages(_) >> null
     }
     def provider = Mock(AmazonClientProvider)
-    def description = new AllowLaunchDescription(account: "prod", amiName: "ami-123456", region: "us-west-1", credentials: testCredentials)
+    def description = new AllowLaunchDescription(targetAccount: "prod", amiName: "ami-123456", region: "us-west-1", credentials: testCredentials)
     def op = new AllowLaunchAtomicOperation(description)
     op.amazonClientProvider = provider
     op.accountCredentialsProvider = Mock(AccountCredentialsProvider)
@@ -126,7 +126,7 @@ class AllowLaunchAtomicOperationUnitSpec extends Specification {
     def targetAmazonEc2 = Mock(AmazonEC2)
     def provider = Mock(AmazonClientProvider)
 
-    def description = new AllowLaunchDescription(account: "prod", amiName: "ami-123456", region: "us-west-1", credentials: testCredentials)
+    def description = new AllowLaunchDescription(targetAccount: "prod", amiName: "ami-123456", region: "us-west-1", credentials: testCredentials)
     def op = new AllowLaunchAtomicOperation(description)
     op.amazonClientProvider = provider
     op.accountCredentialsProvider = Mock(AccountCredentialsProvider)
@@ -161,7 +161,7 @@ class AllowLaunchAtomicOperationUnitSpec extends Specification {
     def targetAmazonEc2 = Mock(AmazonEC2)
     def provider = Mock(AmazonClientProvider)
 
-    def description = new AllowLaunchDescription(account: "test", amiName: "ami-123456", region: "us-west-1", credentials: testCredentials)
+    def description = new AllowLaunchDescription(targetAccount: "test", amiName: "ami-123456", region: "us-west-1", credentials: testCredentials)
     def op = new AllowLaunchAtomicOperation(description)
     op.amazonClientProvider = provider
     op.accountCredentialsProvider = Mock(AccountCredentialsProvider)
@@ -194,7 +194,7 @@ class AllowLaunchAtomicOperationUnitSpec extends Specification {
     def sourceAmazonEc2 = Mock(AmazonEC2)
     def targetAmazonEc2 = Mock(AmazonEC2)
 
-    def description = new AllowLaunchDescription(account: 'target', amiName: 'ami-123456', region: 'us-west-1', credentials: sourceCredentials)
+    def description = new AllowLaunchDescription(targetAccount: 'target', amiName: 'ami-123456', region: 'us-west-1', credentials: sourceCredentials)
     def op = new AllowLaunchAtomicOperation(description)
     op.amazonClientProvider = Mock(AmazonClientProvider)
     op.accountCredentialsProvider = Mock(AccountCredentialsProvider)
@@ -235,7 +235,7 @@ class AllowLaunchAtomicOperationUnitSpec extends Specification {
     def ownerAmazonEc2 = Mock(AmazonEC2)
     def targetAmazonEc2 = Mock(AmazonEC2)
 
-    def description = new AllowLaunchDescription(account: 'target', amiName: 'ami-123456', region: 'us-west-1', credentials: ownerCredentials)
+    def description = new AllowLaunchDescription(targetAccount: 'target', amiName: 'ami-123456', region: 'us-west-1', credentials: ownerCredentials)
     def op = new AllowLaunchAtomicOperation(description)
     op.amazonClientProvider = Mock(AmazonClientProvider)
     op.accountCredentialsProvider = Mock(AccountCredentialsProvider)
@@ -272,7 +272,7 @@ class AllowLaunchAtomicOperationUnitSpec extends Specification {
     def sourceAmazonEc2 = Mock(AmazonEC2)
     def targetAmazonEc2 = Mock(AmazonEC2)
 
-    def description = new AllowLaunchDescription(account: 'target', amiName: 'ami-123456', region: 'us-west-2', credentials: sourceCredentials)
+    def description = new AllowLaunchDescription(targetAccount: 'target', amiName: 'ami-123456', region: 'us-west-2', credentials: sourceCredentials)
     def op = new AllowLaunchAtomicOperation(description)
     op.amazonClientProvider = Mock(AmazonClientProvider)
     op.accountCredentialsProvider = Mock(AccountCredentialsProvider)
@@ -314,7 +314,7 @@ class AllowLaunchAtomicOperationUnitSpec extends Specification {
     def ownerAmazonEc2 = Mock(AmazonEC2)
     def targetAmazonEc2 = Mock(AmazonEC2)
 
-    def description = new AllowLaunchDescription(account: 'target', amiName: 'ami-123456', region: 'us-west-1', credentials: ownerCredentials)
+    def description = new AllowLaunchDescription(targetAccount: 'target', amiName: 'ami-123456', region: 'us-west-1', credentials: ownerCredentials)
     def op = new AllowLaunchAtomicOperation(description)
     op.amazonClientProvider = Mock(AmazonClientProvider)
     op.accountCredentialsProvider = Mock(AccountCredentialsProvider)

diff --git a/...ix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidatorSpec.groovy b/...ix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidatorSpec.groovy
@@ -36,22 +36,22 @@ class AllowLaunchDescriptionValidatorSpec extends Specification {
     then:
     1 * errors.rejectValue("amiName", _)
     1 * errors.rejectValue("region", _)
-    1 * errors.rejectValue("account", _)
+    1 * errors.rejectValue("targetAccount", _)
   }
 
   void "unconfigured account is rejected"() {
     setup:
     AllowLaunchDescriptionValidator validator = new AllowLaunchDescriptionValidator()
     def credentialsHolder = Mock(AccountCredentialsProvider)
     validator.accountCredentialsProvider = credentialsHolder
-    def description = new AllowLaunchDescription(account: "foo")
+    def description = new AllowLaunchDescription(targetAccount: "foo")
     def errors = Mock(Errors)
 
     when:
     validator.validate([], description, errors)
 
     then:
     1 * credentialsHolder.getAll() >> { [TestCredential.named('prod')] }
-    1 * errors.rejectValue("account", _)
+    1 * errors.rejectValue("targetAccount", _)
   }
 }
diff --git a/.../netflix/spinnaker/clouddriver/security/AbstractAtomicOperationsCredentialsSupport.groovy b/.../netflix/spinnaker/clouddriver/security/AbstractAtomicOperationsCredentialsSupport.groovy
@@ -39,7 +39,7 @@ abstract class AbstractAtomicOperationsCredentialsSupport implements AtomicOpera
 
   def <T extends AccountCredentials> T getCredentialsObject(String name) {
     if (name == null) {
-      throw new InvalidRequestException("credential name is required")
+      throw new InvalidRequestException("credentials are required")
     }
     T credential
     try {
@@ -49,7 +49,7 @@ abstract class AbstractAtomicOperationsCredentialsSupport implements AtomicOpera
       }
       credential = (T) repoCredential
     } catch (Exception e) {
-      throw new InvalidRequestException("credential not found (name: ${name}, names: ${accountCredentialsProvider.getAll()*.name})", e)
+      throw new InvalidRequestException("credentials not found (name: ${name}, names: ${accountCredentialsProvider.getAll()*.name})", e)
     }
 
     return credential