From f2c08275cadee31720dce1d20679af6b50c16edb Mon Sep 17 00:00:00 2001 From: Rob Zienert Date: Wed, 11 Sep 2019 13:33:43 -0700 Subject: [PATCH] fix(aws): Alter AllowLaunchDescription to not overload account (#4021) --- .../description/AllowLaunchDescription.groovy | 2 +- .../ops/AllowLaunchAtomicOperation.groovy | 6 +-- .../AllowLaunchPreProcessor.java | 38 +++++++++++++++++++ ...tialsAccountNormalizationPreProcessor.java | 9 ++++- .../AllowLaunchDescriptionValidator.groovy | 8 ++-- .../AllowLaunchAtomicOperationUnitSpec.groovy | 16 ++++---- ...AllowLaunchDescriptionValidatorSpec.groovy | 6 +-- ...tAtomicOperationsCredentialsSupport.groovy | 4 +- 8 files changed, 66 insertions(+), 23 deletions(-) create mode 100644 clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/preprocessors/AllowLaunchPreProcessor.java diff --git a/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/description/AllowLaunchDescription.groovy b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/description/AllowLaunchDescription.groovy index 81a3f1ce034..92ae09862b7 100644 --- a/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/description/AllowLaunchDescription.groovy +++ b/clouddriver-aws/src/main/groovy/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/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/ops/AllowLaunchAtomicOperation.groovy b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/ops/AllowLaunchAtomicOperation.groovy index 4dc27c6016e..2b390d23f8f 100644 --- a/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/ops/AllowLaunchAtomicOperation.groovy +++ b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/ops/AllowLaunchAtomicOperation.groovy @@ -58,7 +58,7 @@ class AllowLaunchAtomicOperation implements AtomicOperation { 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 { 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 { } } - 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/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/preprocessors/AllowLaunchPreProcessor.java b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/preprocessors/AllowLaunchPreProcessor.java new file mode 100644 index 00000000000..2a953ca7156 --- /dev/null +++ b/clouddriver-aws/src/main/groovy/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/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/preprocessors/CredentialsAccountNormalizationPreProcessor.java b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/preprocessors/CredentialsAccountNormalizationPreProcessor.java index 5bb26f6f66f..db4c45aff20 100644 --- a/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/preprocessors/CredentialsAccountNormalizationPreProcessor.java +++ b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/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/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidator.groovy b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidator.groovy index f8c230a6bde..c32e209cc91 100644 --- a/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidator.groovy +++ b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidator.groovy @@ -36,10 +36,10 @@ class AllowLaunchDescriptionValidator extends DescriptionValidator> 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/clouddriver-aws/src/test/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidatorSpec.groovy b/clouddriver-aws/src/test/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidatorSpec.groovy index 27ba1e5c925..1350806303e 100644 --- a/clouddriver-aws/src/test/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidatorSpec.groovy +++ b/clouddriver-aws/src/test/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/validators/AllowLaunchDescriptionValidatorSpec.groovy @@ -36,7 +36,7 @@ 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"() { @@ -44,7 +44,7 @@ class AllowLaunchDescriptionValidatorSpec extends Specification { 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: @@ -52,6 +52,6 @@ class AllowLaunchDescriptionValidatorSpec extends Specification { then: 1 * credentialsHolder.getAll() >> { [TestCredential.named('prod')] } - 1 * errors.rejectValue("account", _) + 1 * errors.rejectValue("targetAccount", _) } } diff --git a/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/security/AbstractAtomicOperationsCredentialsSupport.groovy b/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/security/AbstractAtomicOperationsCredentialsSupport.groovy index fb7b47e08b8..352b105b5d9 100644 --- a/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/security/AbstractAtomicOperationsCredentialsSupport.groovy +++ b/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/security/AbstractAtomicOperationsCredentialsSupport.groovy @@ -39,7 +39,7 @@ abstract class AbstractAtomicOperationsCredentialsSupport implements AtomicOpera def 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