Repository: Sceptre/sceptre

Issue:
`Sceptre diff` creates false-positives when Stack parameter ends with linebreak
### Subject of the issue
Right now, running `sceptre diff` will report that there _is_ a difference when there isn't when the parameter value ends with a line break, such as when you use the !file_contents resolver for a parameter value.

This is because we're using `rstrip('\n')` on the _deployed_ stack parameter values, but not on the `generated` stack parameter values.
### Your environment
* version of sceptre: 3.1.0
* version of python: 3.9.4
* MacOS

### Steps to reproduce
1. Create a StackConfig with a !file_contents resolver populating a stack parameter where the file ends with a line break (as all files should). 
2. Deploy that stack.
3. Run diff on that stack. 

You'll see a diff for that parameter, despite there not being any difference.
### Expected behaviour
There shouldn't be a diff at all.

### Actual behaviour
There's a diff, even though the value hasn't changed!


Discussion:

Patch:
diff --git a/sceptre/diffing/stack_differ.py b/sceptre/diffing/stack_differ.py
index 69011c6..87931e1 100644
--- a/sceptre/diffing/stack_differ.py
+++ b/sceptre/diffing/stack_differ.py
@@ -1,7 +1,7 @@
 import difflib
 import logging
 from abc import abstractmethod
-from typing import NamedTuple, Dict, List, Optional, Callable, Tuple, Generic, TypeVar
+from typing import NamedTuple, Dict, List, Optional, Callable, Tuple, Generic, TypeVar, Union
 
 import cfn_flip
 import deepdiff
@@ -20,7 +20,7 @@ logger = logging.getLogger(__name__)
 class StackConfiguration(NamedTuple):
     """A data container to represent the comparable parts of a Stack."""
     stack_name: str
-    parameters: Dict[str, str]
+    parameters: Dict[str, Union[str, List[str]]]
     stack_tags: Dict[str, str]
     notifications: List[str]
     role_arn: Optional[str]
@@ -128,7 +128,7 @@ class StackDiffer(Generic[DiffType]):
         )
 
     def _create_generated_config(self, stack: Stack) -> StackConfiguration:
-        parameters = self._extract_parameters_dict(stack)
+        parameters = self._extract_parameters_from_generated_stack(stack)
         stack_configuration = StackConfiguration(
             stack_name=stack.external_name,
             parameters=parameters,
@@ -139,13 +139,9 @@ class StackDiffer(Generic[DiffType]):
 
         return stack_configuration
 
-    def _extract_parameters_dict(self, stack: Stack) -> dict:
-        """Extracts a USABLE dict of parameters from the stack.
-
-        Because stack parameters often contain resolvers referencing stacks that might not exist yet
-        (such as when producing a diff on a yet-to-be-deployed stack), we cannot always resolve the
-        stack parameters. Therefore, we need to resolve them (if possible) and fall back to a
-        different representation of that resolver, if necessary.
+    def _extract_parameters_from_generated_stack(self, stack: Stack) -> dict:
+        """Extracts a usable dict of parameters from the stack, performing some minor transformations
+        to match the what CloudFormation does on its end.
 
         :param stack: The stack to extract the parameters from
         :return: A dictionary of stack parameters to be compared.
@@ -153,8 +149,8 @@ class StackDiffer(Generic[DiffType]):
         formatted_parameters = {}
         for key, value in stack.parameters.items():
             if isinstance(value, list):
-                value = ','.join(value)
-            formatted_parameters[key] = value
+                value = ','.join(item.rstrip('\n') for item in value)
+            formatted_parameters[key] = value.rstrip('\n')
 
         return formatted_parameters
 
@@ -170,7 +166,7 @@ class StackDiffer(Generic[DiffType]):
                 return None
             return StackConfiguration(
                 parameters={
-                    param['ParameterKey']: param['ParameterValue'].rstrip('\n')
+                    param['ParameterKey']: param['ParameterValue']
                     for param
                     in stack.get('Parameters', [])
                 },
@@ -193,6 +189,14 @@ class StackDiffer(Generic[DiffType]):
         generated_template_summary = stack_actions.fetch_local_template_summary()
 
         if deployed_config is not None:
+            # Trailing linebreaks sometimes get removed by CloudFormation in certain circumstances
+            # and can sometimes come from using the !file_contents resolver, but ultimately they
+            # shouldn't affect the diff. We'll ignore all trailing linebreaks.
+            self._remove_terminating_linebreaks_from_deployed_parameters(
+                deployed_template_summary,
+                deployed_config
+            )
+
             # If the parameter is not passed by Sceptre and the value on the deployed parameter is
             # the default value, we'll actually remove it from the deployed parameters list so it
             # doesn't show up as a false positive.
@@ -207,6 +211,27 @@ class StackDiffer(Generic[DiffType]):
             # parameter values will be masked.
             self._mask_no_echo_parameters(generated_template_summary, generated_config)
 
+    def _remove_terminating_linebreaks_from_deployed_parameters(
+        self,
+        template_summary: Optional[dict],
+        deployed_config: StackConfiguration
+    ):
+        if template_summary is None:
+            return
+
+        parameter_types = {
+            parameter['ParameterKey']: parameter['ParameterType']
+            for parameter in template_summary['Parameters']
+        }
+
+        for key, value in deployed_config.parameters.items():
+            parameter_type = parameter_types[key]
+            if parameter_type == 'CommaDelimitedList':
+                # If it's a list of strings, remove trailing linebreaks for each item
+                value = ','.join([item.rstrip('\n') for item in value.split(',')])
+
+            deployed_config.parameters[key] = value.rstrip('\n')
+
     def _remove_deployed_default_parameters_that_arent_passed(
         self,
         template_summary: dict,


Test patch:
diff --git a/tests/test_diffing/test_stack_differ.py b/tests/test_diffing/test_stack_differ.py
index 8392bfd..a416499 100644
--- a/tests/test_diffing/test_stack_differ.py
+++ b/tests/test_diffing/test_stack_differ.py
@@ -353,6 +353,49 @@ class TestStackDiffer:
             self.expected_generated_config
         )
 
+    def test_diff__parameter_has_identical_string_linebreak__compares_identical_configs(self):
+        self.deployed_parameters['param'] = self.deployed_parameters['param'] + '\n'
+        self.parameters_on_stack_config['param'] = self.parameters_on_stack_config['param'] + '\n'
+
+        self.differ.diff(self.actions)
+        generated_config = deepcopy(self.expected_generated_config._asdict())
+        generated_parameters = generated_config.pop('parameters')
+        expected_config = StackConfiguration(
+            parameters={
+                key: value.rstrip('\n')
+                for key, value in generated_parameters.items()
+            },
+            **generated_config,
+        )
+
+        self.command_capturer.compare_stack_configurations.assert_called_with(
+            expected_config,
+            expected_config
+        )
+
+    def test_diff__parameter_has_identical_list_linebreaks__compares_identical_configs(self):
+        self.deployed_parameter_types['param'] = 'CommaDelimitedList'
+        self.deployed_parameters['param'] = 'testing\n,this\n,out\n'
+        self.parameters_on_stack_config['param'] = [
+            'testing\n',
+            'this\n',
+            'out\n'
+        ]
+
+        self.differ.diff(self.actions)
+        generated_config = deepcopy(self.expected_generated_config._asdict())
+        generated_parameters = generated_config.pop('parameters')
+        generated_parameters['param'] = 'testing,this,out'
+        expected_config = StackConfiguration(
+            parameters=generated_parameters,
+            **generated_config,
+        )
+
+        self.command_capturer.compare_stack_configurations.assert_called_with(
+            expected_config,
+            expected_config
+        )
+
     def test_diff__no_echo_default_parameter__generated_stack_doesnt_pass_parameter__compares_identical_configs(self):
         self.deployed_parameters['new'] = '****'
         self.deployed_parameter_defaults['new'] = 'default value'