fix(cfn): do not add roleARN if empty or null (#4206) (#4220)

If no RoleARN is provided via Deck or in a templated pipeline, the creation/updating of stacks fails, as en empty value is not allowed by the AWS API. This patch handles this case by including this field into the API call only when it's a non-empty string.
spinnaker · Dec 16, 2019 · 22f8150 · 22f8150
1 parent 76ecdb5
commit 22f8150
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 12 deletions.
diff --git a/...com/netflix/spinnaker/clouddriver/aws/deploy/ops/DeployCloudFormationAtomicOperation.java b/...com/netflix/spinnaker/clouddriver/aws/deploy/ops/DeployCloudFormationAtomicOperation.java
@@ -30,6 +30,7 @@
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Qualifier;
+import org.springframework.util.StringUtils;
 
 @Slf4j
 public class DeployCloudFormationAtomicOperation implements AtomicOperation<Map> {
@@ -125,10 +126,12 @@ private String createStack(
         new CreateStackRequest()
             .withStackName(description.getStackName())
             .withParameters(parameters)
-            .withRoleARN(roleARN)
             .withTags(tags)
             .withTemplateBody(template)
             .withCapabilities(capabilities);
+    if (StringUtils.hasText(roleARN)) {
+      createStackRequest.setRoleARN(roleARN);
+    }
     task.updateStatus(BASE_PHASE, "Uploading CloudFormation Stack");
     CreateStackResult createStackResult = amazonCloudFormation.createStack(createStackRequest);
     return createStackResult.getStackId();
@@ -147,10 +150,12 @@ private String updateStack(
         new UpdateStackRequest()
             .withStackName(description.getStackName())
             .withParameters(parameters)
-            .withRoleARN(roleARN)
             .withTags(tags)
             .withTemplateBody(template)
             .withCapabilities(capabilities);
+    if (StringUtils.hasText(roleARN)) {
+      updateStackRequest.setRoleARN(roleARN);
+    }
     task.updateStatus(BASE_PHASE, "Uploading CloudFormation Stack");
     try {
       UpdateStackResult updateStackResult = amazonCloudFormation.updateStack(updateStackRequest);
@@ -176,11 +181,13 @@ private String createChangeSet(
             .withStackName(description.getStackName())
             .withChangeSetName(description.getChangeSetName())
             .withParameters(parameters)
-            .withRoleARN(roleARN)
             .withTags(tags)
             .withTemplateBody(template)
             .withCapabilities(capabilities)
             .withChangeSetType(changeSetType);
+    if (StringUtils.hasText(roleARN)) {
+      createChangeSetRequest.setRoleARN(roleARN);
+    }
     task.updateStatus(BASE_PHASE, "Uploading CloudFormation ChangeSet");
     try {
       CreateChangeSetResult createChangeSetResult =

diff --git a/...tflix/spinnaker/clouddriver/aws/deploy/ops/DeployCloudFormationAtomicOperationSpec.groovy b/...tflix/spinnaker/clouddriver/aws/deploy/ops/DeployCloudFormationAtomicOperationSpec.groovy
@@ -42,6 +42,7 @@ class DeployCloudFormationAtomicOperationSpec extends Specification {
     TaskRepository.threadLocalTask.set(Mock(Task))
   }
 
+  @Unroll
   void "should build a CreateStackRequest if stack doesn't exist and submit through aws client"() {
     given:
     def amazonClientProvider = Mock(AmazonClientProvider)
@@ -54,7 +55,7 @@ class DeployCloudFormationAtomicOperationSpec extends Specification {
           stackName: "stackTest",
           region: "eu-west-1",
           templateBody: '{"key":"value"}',
-          roleARN: "arn:aws:iam::123456789012:role/test",
+          roleARN: roleARN,
           parameters: [ key: "value"],
           tags: [ key: "value" ],
           capabilities: ["cap1", "cap2"],
@@ -74,15 +75,23 @@ class DeployCloudFormationAtomicOperationSpec extends Specification {
     1 * amazonCloudFormation.createStack(_) >> { CreateStackRequest request ->
       assert request.getStackName() == "stackTest"
       assert request.getTemplateBody() == '{"key":"value"}'
-      assert request.getRoleARN() == "arn:aws:iam::123456789012:role/test"
+      assert request.getRoleARN() == expectedRoleARN
       assert request.getParameters() == [ new Parameter().withParameterKey("key").withParameterValue("value") ]
       assert request.getTags() == [ new Tag().withKey("key").withValue("value") ]
       assert request.getCapabilities() == ["cap1", "cap2"]
       createStackResult
     }
     1 * createStackResult.getStackId() >> stackId
+
+    where:
+    roleARN                              || expectedRoleARN
+    "arn:aws:iam:123456789012:role/test" || "arn:aws:iam:123456789012:role/test"
+    ""                                   || null
+    "    "                               || null
+    null                                 || null
   }
 
+  @Unroll
   void "should build an UpdateStackRequest if stack exists and submit through aws client"() {
     given:
     def amazonClientProvider = Mock(AmazonClientProvider)
@@ -95,7 +104,7 @@ class DeployCloudFormationAtomicOperationSpec extends Specification {
           stackName: "stackTest",
           region: "eu-west-1",
           templateBody: '{"key":"value"}',
-          roleARN: "arn:aws:iam::123456789012:role/test",
+          roleARN: roleARN,
           parameters: [ key: "value" ],
           tags: [ key: "value" ],
           capabilities: ["cap1", "cap2"],
@@ -117,13 +126,20 @@ class DeployCloudFormationAtomicOperationSpec extends Specification {
     1 * amazonCloudFormation.updateStack(_) >> { UpdateStackRequest request ->
       assert request.getStackName() == "stackTest"
       assert request.getTemplateBody() == '{"key":"value"}'
-      assert request.getRoleARN() == "arn:aws:iam::123456789012:role/test"
+      assert request.getRoleARN() == expectedRoleARN
       assert request.getParameters() == [ new Parameter().withParameterKey("key").withParameterValue("value") ]
       assert request.getTags() == [ new Tag().withKey("key").withValue("value") ]
       assert request.getCapabilities() == ["cap1", "cap2"]
       updateStackRequest
     }
     1 * updateStackRequest.getStackId() >> stackId
+
+    where:
+    roleARN                              || expectedRoleARN
+    "arn:aws:iam:123456789012:role/test" || "arn:aws:iam:123456789012:role/test"
+    ""                                   || null
+    "    "                               || null
+    null                                 || null
   }
 
   @Unroll
@@ -139,7 +155,7 @@ class DeployCloudFormationAtomicOperationSpec extends Specification {
           stackName: "stackTest",
           region: "eu-west-1",
           templateBody: 'key: "value"',
-          roleARN: "arn:aws:iam::123456789012:role/test",
+          roleARN: roleARN,
           parameters: [ key: "value" ],
           tags: [ key: "value" ],
           capabilities: ["cap1", "cap2"],
@@ -167,7 +183,7 @@ class DeployCloudFormationAtomicOperationSpec extends Specification {
     1* amazonCloudFormation.createChangeSet(_) >> { CreateChangeSetRequest request ->
       assert request.getStackName() == "stackTest"
       assert request.getTemplateBody() == 'key: "value"'
-      assert request.getRoleARN() == "arn:aws:iam::123456789012:role/test"
+      assert request.getRoleARN() == expectedRoleARN
       assert request.getParameters() == [ new Parameter().withParameterKey("key").withParameterValue("value") ]
       assert request.getTags() == [ new Tag().withKey("key").withValue("value") ]
       assert request.getCapabilities() == ["cap1", "cap2"]
@@ -178,8 +194,11 @@ class DeployCloudFormationAtomicOperationSpec extends Specification {
     1 * createChangeSetResult.getStackId() >> stackId
 
     where:
-    existingStack || changeSetType
-    true          || ChangeSetType.UPDATE.toString()
-    false         || ChangeSetType.CREATE.toString()
+    roleARN                              | expectedRoleARN                      | existingStack || changeSetType
+    "arn:aws:iam:123456789012:role/test" | "arn:aws:iam:123456789012:role/test" | true          || ChangeSetType.UPDATE.toString()
+    ""                                   | null                                 | true          || ChangeSetType.UPDATE.toString()
+    "   "                                | null                                 | true          || ChangeSetType.UPDATE.toString()
+    "arn:aws:iam:123456789012:role/test" | "arn:aws:iam:123456789012:role/test" | true          || ChangeSetType.UPDATE.toString()
+    "arn:aws:iam:123456789012:role/test" | "arn:aws:iam:123456789012:role/test" | false         || ChangeSetType.CREATE.toString()
   }
 }