Repository: cloudtools/stacker

Issue:
Update of a Raw Blueprint with Transform fails
Hi,

I believe I have discovered a bug in Stacker. The bug seems to be triggered by trying to update a raw template which uses Transforms. I'm using master at the time of this report (d1353b3a942731ada1532f0e1422d64673988c1a) with python 3.6.5 on macOS 10.13.6. Stacker is installed in a virtualenv.

Config to reproduce:
config.yaml:
```yaml
---
namespace: 'whatever'
stacks:
    - name: 'dummy'
      template_path: dummy.json
```
dummy.json:
```json
{
    "Transform": "AWS::Serverless-2016-10-31",
    "Resources": {
        "DummyParameter": {
            "Type": "AWS::SSM::Parameter",
            "Properties": {
                "Type": "String",
                "Value": "foobar"
            }
        }
    }
}
```

Steps to reproduce: run `build` twice:
```
% stacker build config.yml
[2018-08-28T15:56:44] Using default AWS provider mode
[2018-08-28T15:56:50] dummy: submitted (creating new stack)
[2018-08-28T15:57:20] dummy: complete (creating new stack)
```
```
% stacker build config.yml
[2018-08-28T16:00:56] Using default AWS provider mode
[2018-08-28T16:00:57] create_change_set() got an unexpected keyword argument 'stack_policy'
Traceback (most recent call last):
  File "<stripped>/stacker/venv/lib/python3.6/site-packages/stacker/plan.py", line 93, in _run_once
    status = self.fn(self.stack, status=self.status)
  File "<stripped>/stacker/venv/lib/python3.6/site-packages/stacker/actions/build.py", line 353, in _launch_stack
    stack_policy=stack_policy,
  File "<stripped>/stacker/venv/lib/python3.6/site-packages/stacker/providers/aws/default.py", line 805, in update_stack
    stack_policy=stack_policy, **kwargs)
  File "<stripped>/stacker/venv/lib/python3.6/site-packages/stacker/providers/aws/default.py", line 891, in noninteractive_changeset_update
    'UPDATE', service_role=self.service_role, **kwargs
TypeError: create_change_set() got an unexpected keyword argument 'stack_policy'
[2018-08-28T16:00:57] dummy: failed (create_change_set() got an unexpected keyword argument 'stack_policy')
[2018-08-28T16:00:57] The following steps failed: dummy
```

Discussion:

Patch:
diff --git a/examples/cross-account/stacker.yaml b/examples/cross-account/stacker.yaml
index 123a509..6c39662 100644
--- a/examples/cross-account/stacker.yaml
+++ b/examples/cross-account/stacker.yaml
@@ -14,7 +14,5 @@ stacks:
     profile: master
     template_path: templates/stacker-bucket.yaml
     variables:
-      Roles:
-        # Change these to the correct AWS account IDs
-        - arn:aws:iam::<prod account id>:role/Stacker
-        - arn:aws:iam::<stage account id>:role/Stacker
+      # Change these to the correct AWS account IDs, must be comma seperated list
+      Roles: arn:aws:iam::<prod account id>:role/Stacker, arn:aws:iam::<stage account id>:role/Stacker
diff --git a/stacker/lookups/handlers/kms.py b/stacker/lookups/handlers/kms.py
index 9ff6a52..b5f654d 100644
--- a/stacker/lookups/handlers/kms.py
+++ b/stacker/lookups/handlers/kms.py
@@ -52,5 +52,12 @@ def handler(value, **kwargs):
         region, value = value.split("@", 1)
 
     kms = get_session(region).client('kms')
-    decoded = codecs.decode(value.encode(), 'base64').decode()
+
+    # encode str value as an utf-8 bytestring for use with codecs.decode.
+    value = value.encode('utf-8')
+
+    # get raw but still encrypted value from base64 version.
+    decoded = codecs.decode(value, 'base64')
+
+    # decrypt and return the plain text raw value.
     return kms.decrypt(CiphertextBlob=decoded)["Plaintext"]
diff --git a/stacker/providers/aws/default.py b/stacker/providers/aws/default.py
index a8ad679..885f744 100644
--- a/stacker/providers/aws/default.py
+++ b/stacker/providers/aws/default.py
@@ -397,6 +397,8 @@ def generate_cloudformation_args(stack_name, parameters, tags, template,
             with create_change_set.
         service_role (str, optional): An optional service role to use when
             interacting with Cloudformation.
+        stack_policy (:class:`stacker.providers.base.Template`): A template
+            object representing a stack policy.
         change_set_name (str, optional): An optional change set name to use
             with create_change_set.
 
@@ -434,6 +436,16 @@ def generate_cloudformation_args(stack_name, parameters, tags, template,
 
 
 def generate_stack_policy_args(stack_policy=None):
+    """ Converts a stack policy object into keyword args.
+
+    Args:
+        stack_policy (:class:`stacker.providers.base.Template`): A template
+            object representing a stack policy.
+
+    Returns:
+        dict: A dictionary of keyword arguments to be used elsewhere.
+    """
+
     args = {}
     if stack_policy:
         logger.debug("Stack has a stack policy")
@@ -666,6 +678,8 @@ class Provider(BaseProvider):
             tags (list): A list of dictionaries that defines the tags
                 that should be applied to the Cloudformation stack.
             force_change_set (bool): Whether or not to force change set use.
+            stack_policy (:class:`stacker.providers.base.Template`): A template
+                object representing a stack policy.
         """
 
         logger.debug("Attempting to create stack %s:.", fqn)
@@ -813,6 +827,8 @@ class Provider(BaseProvider):
                 not. False will follow the behavior of the provider.
             force_change_set (bool): A flag that indicates whether the update
                 must be executed with a change set.
+            stack_policy (:class:`stacker.providers.base.Template`): A template
+                object representing a stack policy.
         """
         logger.debug("Attempting to update stack %s:", fqn)
         logger.debug("    parameters: %s", parameters)
@@ -824,11 +840,28 @@ class Provider(BaseProvider):
         update_method = self.select_update_method(force_interactive,
                                                   force_change_set)
 
-        return update_method(fqn, template, old_parameters, parameters, tags,
-                             stack_policy=stack_policy, **kwargs)
+        return update_method(fqn, template, old_parameters, parameters,
+                             stack_policy=stack_policy, tags=tags, **kwargs)
+
+    def deal_with_changeset_stack_policy(self, fqn, stack_policy):
+        """ Set a stack policy when using changesets.
+
+        ChangeSets don't allow you to set stack policies in the same call to
+        update them. This sets it before executing the changeset if the
+        stack policy is passed in.
+
+        Args:
+            stack_policy (:class:`stacker.providers.base.Template`): A template
+                object representing a stack policy.
+        """
+        if stack_policy:
+            kwargs = generate_stack_policy_args(stack_policy)
+            kwargs["StackName"] = fqn
+            logger.debug("Setting stack policy on %s.", fqn)
+            self.cloudformation.set_stack_policy(**kwargs)
 
     def interactive_update_stack(self, fqn, template, old_parameters,
-                                 parameters, tags, stack_policy=None,
+                                 parameters, stack_policy, tags,
                                  **kwargs):
         """Update a Cloudformation stack in interactive mode.
 
@@ -840,6 +873,8 @@ class Provider(BaseProvider):
                 parameter list on the existing Cloudformation stack.
             parameters (list): A list of dictionaries that defines the
                 parameter list to be applied to the Cloudformation stack.
+            stack_policy (:class:`stacker.providers.base.Template`): A template
+                object representing a stack policy.
             tags (list): A list of dictionaries that defines the tags
                 that should be applied to the Cloudformation stack.
         """
@@ -878,19 +913,15 @@ class Provider(BaseProvider):
             finally:
                 ui.unlock()
 
-        # ChangeSets don't support specifying a stack policy inline, like
-        # CreateStack/UpdateStack, so we just SetStackPolicy if there is one.
-        if stack_policy:
-            kwargs = generate_stack_policy_args(stack_policy)
-            kwargs["StackName"] = fqn
-            self.cloudformation.set_stack_policy(**kwargs)
+        self.deal_with_changeset_stack_policy(fqn, stack_policy)
 
         self.cloudformation.execute_change_set(
             ChangeSetName=change_set_id,
         )
 
     def noninteractive_changeset_update(self, fqn, template, old_parameters,
-                                        parameters, tags, **kwargs):
+                                        parameters, stack_policy, tags,
+                                        **kwargs):
         """Update a Cloudformation stack using a change set.
 
         This is required for stacks with a defined Transform (i.e. SAM), as the
@@ -904,6 +935,8 @@ class Provider(BaseProvider):
                 parameter list on the existing Cloudformation stack.
             parameters (list): A list of dictionaries that defines the
                 parameter list to be applied to the Cloudformation stack.
+            stack_policy (:class:`stacker.providers.base.Template`): A template
+                object representing a stack policy.
             tags (list): A list of dictionaries that defines the tags
                 that should be applied to the Cloudformation stack.
         """
@@ -914,6 +947,8 @@ class Provider(BaseProvider):
             'UPDATE', service_role=self.service_role, **kwargs
         )
 
+        self.deal_with_changeset_stack_policy(fqn, stack_policy)
+
         self.cloudformation.execute_change_set(
             ChangeSetName=change_set_id,
         )
@@ -932,6 +967,8 @@ class Provider(BaseProvider):
                 parameter list to be applied to the Cloudformation stack.
             tags (list): A list of dictionaries that defines the tags
                 that should be applied to the Cloudformation stack.
+            stack_policy (:class:`stacker.providers.base.Template`): A template
+                object representing a stack policy.
         """
 
         logger.debug("Using default provider mode for %s.", fqn)


Test patch:
diff --git a/stacker/tests/providers/aws/test_default.py b/stacker/tests/providers/aws/test_default.py
index d1de9a7..fcc1404 100644
--- a/stacker/tests/providers/aws/test_default.py
+++ b/stacker/tests/providers/aws/test_default.py
@@ -500,6 +500,64 @@ class TestProviderDefaultMode(unittest.TestCase):
             self.assertFalse(
                 self.provider.prepare_stack_for_update(stack, []))
 
+    def test_noninteractive_changeset_update_no_stack_policy(self):
+        stack_name = "MockStack"
+
+        self.stubber.add_response(
+            "create_change_set",
+            {'Id': 'CHANGESETID', 'StackId': 'STACKID'}
+        )
+        changes = []
+        changes.append(generate_change())
+
+        self.stubber.add_response(
+            "describe_change_set",
+            generate_change_set_response(
+                status="CREATE_COMPLETE", execution_status="AVAILABLE",
+                changes=changes,
+            )
+        )
+
+        self.stubber.add_response("execute_change_set", {})
+
+        with self.stubber:
+            self.provider.noninteractive_changeset_update(
+                fqn=stack_name,
+                template=Template(url="http://fake.template.url.com/"),
+                old_parameters=[],
+                parameters=[], stack_policy=None, tags=[],
+            )
+
+    def test_noninteractive_changeset_update_with_stack_policy(self):
+        stack_name = "MockStack"
+
+        self.stubber.add_response(
+            "create_change_set",
+            {'Id': 'CHANGESETID', 'StackId': 'STACKID'}
+        )
+        changes = []
+        changes.append(generate_change())
+
+        self.stubber.add_response(
+            "describe_change_set",
+            generate_change_set_response(
+                status="CREATE_COMPLETE", execution_status="AVAILABLE",
+                changes=changes,
+            )
+        )
+
+        self.stubber.add_response("set_stack_policy", {})
+
+        self.stubber.add_response("execute_change_set", {})
+
+        with self.stubber:
+            self.provider.noninteractive_changeset_update(
+                fqn=stack_name,
+                template=Template(url="http://fake.template.url.com/"),
+                old_parameters=[],
+                parameters=[], stack_policy=Template(body="{}"), tags=[],
+            )
+
 
 class TestProviderInteractiveMode(unittest.TestCase):
     def setUp(self):
@@ -516,7 +574,8 @@ class TestProviderInteractiveMode(unittest.TestCase):
         self.assertEqual(p.replacements_only, replacements)
 
     @patch("stacker.providers.aws.default.ask_for_approval")
-    def test_update_stack_execute_success(self, patched_approval):
+    def test_update_stack_execute_success_no_stack_policy(self,
+                                                          patched_approval):
         stack_name = "my-fake-stack"
 
         self.stubber.add_response(
@@ -550,6 +609,45 @@ class TestProviderInteractiveMode(unittest.TestCase):
 
         self.assertEqual(patched_approval.call_count, 1)
 
+    @patch("stacker.providers.aws.default.ask_for_approval")
+    def test_update_stack_execute_success_with_stack_policy(self,
+                                                            patched_approval):
+        stack_name = "my-fake-stack"
+
+        self.stubber.add_response(
+            "create_change_set",
+            {'Id': 'CHANGESETID', 'StackId': 'STACKID'}
+        )
+        changes = []
+        changes.append(generate_change())
+
+        self.stubber.add_response(
+            "describe_change_set",
+            generate_change_set_response(
+                status="CREATE_COMPLETE", execution_status="AVAILABLE",
+                changes=changes,
+            )
+        )
+
+        self.stubber.add_response("set_stack_policy", {})
+
+        self.stubber.add_response("execute_change_set", {})
+
+        with self.stubber:
+            self.provider.update_stack(
+                fqn=stack_name,
+                template=Template(url="http://fake.template.url.com/"),
+                old_parameters=[],
+                parameters=[], tags=[],
+                stack_policy=Template(body="{}"),
+            )
+
+        patched_approval.assert_called_with(full_changeset=changes,
+                                            params_diff=[],
+                                            include_verbose=True)
+
+        self.assertEqual(patched_approval.call_count, 1)
+
     def test_select_update_method(self):
         for i in [[{'force_interactive': False,
                     'force_change_set': False},