Fix AltStackInstances check for create (#119)

* Fix AltStackInstances check for create * Fix AltStackInstances check for creation * Delete unnecessary throw from unit test helper function
aws-cloudformation · Sep 18, 2023 · a44f123 · a44f123
1 parent 088c466
commit a44f123
Show file tree

Hide file tree

Showing 6 changed files with 108 additions and 65 deletions.
diff --git a/aws-cloudformation-stackset/docs/deploymenttargets.md b/aws-cloudformation-stackset/docs/deploymenttargets.md
@@ -11,7 +11,7 @@ To declare this entity in your AWS CloudFormation template, use the following sy
 <pre>
 {
     "<a href="#accounts" title="Accounts">Accounts</a>" : <i>[ String, ... ]</i>,
-    "<a href="#accountsUrl" title="AccountsUrl">AccountsUrl</a>" : <i>String</i>,
+    "<a href="#accountsurl" title="AccountsUrl">AccountsUrl</a>" : <i>String</i>,
     "<a href="#organizationalunitids" title="OrganizationalUnitIds">OrganizationalUnitIds</a>" : <i>[ String, ... ]</i>,
     "<a href="#accountfiltertype" title="AccountFilterType">AccountFilterType</a>" : <i>String</i>
 }
@@ -48,9 +48,11 @@ _Required_: No
 
 _Type_: String
 
-_Length Constraints_: Minimum length of 1. Maximum length of 5120.
+_Minimum Length_: <code>1</code>
 
-_Pattern_: `(s3://|http(s?)://).+`
+_Maximum Length_: <code>5120</code>
+
+_Pattern_: <code>(s3://|http(s?)://).+</code>
 
 _Update requires_: [No interruption](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html#update-no-interrupt)
 

diff --git a/aws-cloudformation-stackset/resource-role.yaml b/aws-cloudformation-stackset/resource-role.yaml
@@ -45,6 +45,7 @@ Resources:
                 - "cloudformation:UntagResource"
                 - "cloudformation:UpdateStackInstances"
                 - "cloudformation:UpdateStackSet"
+                - "iam:PassRole"
                 Resource: "*"
 Outputs:
   ExecutionRoleArn:

diff --git a/.../src/main/java/software/amazon/cloudformation/stackset/util/AltResourceModelAnalyzer.java b/.../src/main/java/software/amazon/cloudformation/stackset/util/AltResourceModelAnalyzer.java
@@ -9,6 +9,8 @@
 import lombok.Builder;
 import lombok.Data;
 import software.amazon.awssdk.utils.CollectionUtils;
+import software.amazon.cloudformation.exceptions.CfnInvalidRequestException;
+import software.amazon.cloudformation.stackset.Parameter;
 import software.amazon.cloudformation.stackset.ResourceModel;
 import software.amazon.cloudformation.stackset.StackInstances;
 
@@ -53,21 +55,53 @@ public void analyze(final StackInstancesPlaceHolder placeHolder) {
                 region -> stackInstancesToCreate.addAll(currentStackInstancesByRegion.get(region))
         );
 
+        HashMap<String, Set<Parameter>> ouDeploymentParametersMap = findDeploymentParametersForOUs(currentStackInstancesByRegion);
+
         setInter(currentRegions, previousRegions).forEach(
                 region -> new AltStackInstancesCalculator(region,
                         previousStackInstancesByRegion.get(region),
                         currentStackInstancesByRegion.get(region))
                         .calculate(
                                 stackInstancesToDelete,
                                 stackInstancesToCreate,
-                                stackInstancesToUpdate)
+                                stackInstancesToUpdate,
+                                ouDeploymentParametersMap)
         );
 
         placeHolder.setCreateStackInstances(new ArrayList<>(stackInstancesToCreate));
         placeHolder.setDeleteStackInstances(new ArrayList<>(stackInstancesToDelete));
         placeHolder.setUpdateStackInstances(new ArrayList<>(stackInstancesToUpdate));
     }
 
+    /*
+     *  If an OU is associated with different parameter sets, will raise an error.
+     *  1. This is the original process logic when ALT is not enabled.
+     *  2. Although users logically CAN associate an OU with different with ALT filter, but we cannot check if the input is valid
+     *  2.1 For example, (OU - account1) and (OU - account2). It's up to OU's structure if these two targets can be
+     *      associated with two parameters -- if OU is set(account1, account2, account3), then not valid.
+     *  2.2 So we chose to raise and error to align with previous implementation and avoid possible ambiguity
+     * */
+
+    private static HashMap<String, Set<Parameter>> findDeploymentParametersForOUs(final HashMap<String, Set<StackInstances>> currentStackInstancesByRegion) {
+        HashMap<String, Set<Parameter>> ouDeploymentParameters = new HashMap<>();
+
+        for (final String region : currentStackInstancesByRegion.keySet()) {
+            for (final StackInstances stackInstances : currentStackInstancesByRegion.get(region)) {
+                Set<Parameter> parameters = stackInstances.getParameterOverrides();
+
+                stackInstances.getDeploymentTargets().getOrganizationalUnitIds().forEach(
+                        ou -> {
+                            if (ouDeploymentParameters.containsKey(ou) && ouDeploymentParameters.get(ou) != parameters) {
+                                throw new CfnInvalidRequestException("An OrganizationalUnitIds cannot be associated with more than one Parameters set");
+                            }
+                            ouDeploymentParameters.put(ou, parameters);
+                        }
+                );
+            }
+        }
+        return ouDeploymentParameters;
+    }
+
     private static HashMap<String, Set<StackInstances>> regroupStackInstancesByRegion (final Collection<StackInstances> stackInstancesGroup) {
         HashMap<String, Set<StackInstances>> stackInstancesGroupsByRegion = new HashMap<>();
         if (CollectionUtils.isNullOrEmpty(stackInstancesGroup)) return stackInstancesGroupsByRegion;

diff --git a/...c/main/java/software/amazon/cloudformation/stackset/util/AltStackInstancesCalculator.java b/...c/main/java/software/amazon/cloudformation/stackset/util/AltStackInstancesCalculator.java
@@ -9,7 +9,6 @@
 import java.util.stream.Collectors;
 import lombok.Data;
 import software.amazon.awssdk.utils.CollectionUtils;
-import software.amazon.cloudformation.exceptions.CfnInvalidRequestException;
 import software.amazon.cloudformation.stackset.DeploymentTargets;
 import software.amazon.cloudformation.stackset.Parameter;
 import software.amazon.cloudformation.stackset.StackInstances;
@@ -30,7 +29,6 @@ public class AltStackInstancesCalculator {
 
     private final HashMap<List<String>, Set<String>> previousStackInstancesByOuFilter;
     private final HashMap<List<String>, Set<String>> currentStackInstancesByOuFilter;
-    private final HashMap<String, Set<Parameter>> ouDeploymentParametersMap;
 
     private final static String NONE = "NONE";
     private final static String INTER = "INTERSECTION";
@@ -52,12 +50,11 @@ public AltStackInstancesCalculator (String region, Set<StackInstances> previousS
         this.currentStackInstancesGroup = currentStackInstancesGroup;
         this.previousOUs = findAllOus(previousStackInstancesGroup);
         this.currentOUs = findAllOus(currentStackInstancesGroup);
-        this.ouDeploymentParametersMap = findDeploymentParametersForOUs(currentStackInstancesGroup);
         this.previousStackInstancesByOuFilter = mergeDifferentFilters(mergeSameFilters(previousStackInstancesGroup));
         this.currentStackInstancesByOuFilter = mergeDifferentFilters(mergeSameFilters(currentStackInstancesGroup));
     }
 
-    public void calculate (Set<StackInstances> instancesToDelete, Set<StackInstances> instancesToCreate, Set<StackInstances> instancesToUpdate) {
+    public void calculate (Set<StackInstances> instancesToDelete, Set<StackInstances> instancesToCreate, Set<StackInstances> instancesToUpdate, HashMap<String, Set<Parameter>> ouDeploymentParametersMap) {
         setDiff(previousOUs, currentOUs).forEach(ou -> {
             String filter = getFilterType(ou, previousStackInstancesByOuFilter);
             Set<String> accounts = previousStackInstancesByOuFilter.get(Arrays.asList(ou, filter));
@@ -85,33 +82,6 @@ public void calculate (Set<StackInstances> instancesToDelete, Set<StackInstances
         });
     }
 
-    /*
-    *  If an OU is associated with different parameter sets, will raise an error.
-    *  1. This is the original process logic when ALT is not enabled.
-    *  2. Although users logically CAN associate an OU with different with ALT filter, but we cannot check if the input is valid
-    *  2.1 For example, (OU - account1) and (OU - account2). It's up to OU's structure if these two targets can be
-    *      associated with two parameters -- if OU is set(account1, account2, account3), then not valid.
-    *  2.2 So we chose to raise and error to align with previous implementation and avoid possible ambiguity
-    * */
-
-    private static HashMap<String, Set<Parameter>> findDeploymentParametersForOUs(final Set<StackInstances> stackInstancesGroup) {
-        HashMap<String, Set<Parameter>> ouDeploymentParameters = new HashMap<>();
-
-        for (final StackInstances stackInstances : stackInstancesGroup) {
-            Set<Parameter> parameters = stackInstances.getParameterOverrides();
-
-            stackInstances.getDeploymentTargets().getOrganizationalUnitIds().forEach(
-                    ou -> {
-                        if (ouDeploymentParameters.containsKey(ou) && ouDeploymentParameters.get(ou) != parameters) {
-                            throw new CfnInvalidRequestException("An OrganizationalUnitIds cannot be associated with more than one Parameters set");
-                        }
-                        ouDeploymentParameters.put(ou, parameters);
-                    }
-            );
-        }
-        return ouDeploymentParameters;
-    }
-
     private static Set<String> findAllOus(final Set<StackInstances> stackInstancesGroup) {
         final HashSet<String> OUs = new HashSet<>();
         stackInstancesGroup.forEach(stackInstances -> {

diff --git a/.../test/java/software/amazon/cloudformation/stackset/util/AltResourceModelAnalyzerTest.java b/.../test/java/software/amazon/cloudformation/stackset/util/AltResourceModelAnalyzerTest.java
@@ -26,6 +26,8 @@
 import static software.amazon.cloudformation.stackset.util.AltTestUtils.generateInstances;
 import static software.amazon.cloudformation.stackset.util.AltTestUtils.generateInstancesWithRegions;
 import static software.amazon.cloudformation.stackset.util.AltTestUtils.generateModel;
+import static software.amazon.cloudformation.stackset.util.AltTestUtils.parameters_1;
+import static software.amazon.cloudformation.stackset.util.AltTestUtils.parameters_2;
 import static software.amazon.cloudformation.stackset.util.AltTestUtils.region_1;
 import static software.amazon.cloudformation.stackset.util.AltTestUtils.region_2;
 import static software.amazon.cloudformation.stackset.util.AltTestUtils.region_3;
@@ -280,4 +282,18 @@ public void test_Alt_In_Both_Models() {
         assertThat(Comparator.equals(new HashSet<>(placeHolder.getUpdateStackInstances()), desiredUpdateInstances)).isTrue();
     }
 
+    @Test
+    public void test_No_Alt_Filter_Multiple_Parameters_In_One_Group() {
+        ResourceModel currentModel = generateModel(new HashSet<>(Arrays.asList(
+                generateInstances(OU_1, parameters_1),
+                generateInstances(Arrays.asList(OU_1, OU_2), parameters_2)))
+        );
+        StackInstancesPlaceHolder placeHolder = new StackInstancesPlaceHolder();
+
+        Exception ex = assertThrows(
+                CfnInvalidRequestException.class,
+                () -> AltResourceModelAnalyzer.builder().currentModel(currentModel).build().analyze(placeHolder)
+        );
+        assertThat(ex.getMessage()).contains("An OrganizationalUnitIds cannot be associated with more than one Parameters set");
+    }
 }