Fix on_denied and on_missing bugs (#618) (#747)

[PR #618/e85e4209 backport][stable-3] Fix on_denied and on_missing bugs

This is a backport of PR #618 as merged into main (e85e420).
SUMMARY
This pull request:

Changes the default value of on_denied to be error, so that it agrees with what is stated in the documentation.
Changes the default value of on_missing to be error, and updates the documentation to explain this.

Fixes #617.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
aws_ssm lookup

changelogs/fragments/617-aws_ssm-on_missing-and-on-denied-now-default-to-error.yml

@@ -0,0 +1,2 @@
+breaking_changes:
+- aws_ssm - on_denied and on_missing now both default to error, for consistency with both aws_secret and the base Lookup class (https://github.com/ansible-collections/amazon.aws/issues/617).

plugins/lookup/aws_ssm.py

@@ -23,18 +23,15 @@
     The first argument you pass the lookup can either be a parameter name or a hierarchy of
     parameters. Hierarchies start with a forward slash and end with the parameter name. Up to
     5 layers may be specified.
-  - If looking up an explicitly listed parameter by name which does not exist then the lookup will
-    return a None value which will be interpreted by Jinja2 as an empty string.  You can use the
-    ```default``` filter to give a default value in this case but must set the second parameter to
-    true (see examples below)
+  - If looking up an explicitly listed parameter by name which does not exist then the lookup
+    will generate an error. You can use the ```default``` filter to give a default value in
+    this case but must set the ```on_missing``` parameter to ```skip``` or ```warn```. You must
+    also set the second parameter of the ```default``` filter to ```true``` (see examples below).
   - When looking up a path for parameters under it a dictionary will be returned for each path.
-    If there is no parameter under that path then the return will be successful but the
-    dictionary will be empty.
+    If there is no parameter under that path then the lookup will generate an error.
   - If the lookup fails due to lack of permissions or due to an AWS client error then the aws_ssm
-    will generate an error, normally crashing the current ansible task.  This is normally the right
-    thing since ignoring a value that IAM isn't giving access to could cause bigger problems and
-    wrong behaviour or loss of data.  If you want to continue in this case then you will have to set
-    up two ansible tasks, one which sets a variable and ignores failures one which uses the value
+    will generate an error. If you want to continue in this case then you will have to set up
+    two ansible tasks, one which sets a variable and ignores failures and one which uses the value
     of that variable with a default.  See the examples below.
 
 options:
@@ -81,26 +78,26 @@
 - name: lookup ssm parameter store in the current region
   debug: msg="{{ lookup('aws_ssm', 'Hello' ) }}"
 
-- name: lookup ssm parameter store in nominated region
+- name: lookup ssm parameter store in specified region
   debug: msg="{{ lookup('aws_ssm', 'Hello', region='us-east-2' ) }}"
 
-- name: lookup ssm parameter store without decrypted
+- name: lookup ssm parameter store without decryption
   debug: msg="{{ lookup('aws_ssm', 'Hello', decrypt=False ) }}"
 
-- name: lookup ssm parameter store in nominated aws profile
+- name: lookup ssm parameter store using a specified aws profile
   debug: msg="{{ lookup('aws_ssm', 'Hello', aws_profile='myprofile' ) }}"
 
 - name: lookup ssm parameter store using explicit aws credentials
   debug: msg="{{ lookup('aws_ssm', 'Hello', aws_access_key=my_aws_access_key, aws_secret_key=my_aws_secret_key, aws_security_token=my_security_token ) }}"
 
-- name: lookup ssm parameter store with all options.
+- name: lookup ssm parameter store with all options
   debug: msg="{{ lookup('aws_ssm', 'Hello', decrypt=false, region='us-east-2', aws_profile='myprofile') }}"
 
-- name: lookup a key which doesn't exist, returns ""
-  debug: msg="{{ lookup('aws_ssm', 'NoKey') }}"
+- name: lookup ssm parameter and fail if missing
+  debug: msg="{{ lookup('aws_ssm', 'missing-parameter') }}"
 
 - name: lookup a key which doesn't exist, returning a default ('root')
-  debug: msg="{{ lookup('aws_ssm', 'AdminID') | default('root', true) }}"
+  debug: msg="{{ lookup('aws_ssm', 'AdminID', on_missing="skip") | default('root', true) }}"
 
 - name: lookup a key which doesn't exist failing to store it in a fact
   set_fact:
@@ -124,9 +121,6 @@
   debug: msg='Path contains {{ item }}'
   loop: '{{ lookup("aws_ssm", "/demo/", "/demo1/", bypath=True)}}'
 
-- name: lookup ssm parameter and fail if missing
-  debug: msg="{{ lookup('aws_ssm', 'missing-parameter', on_missing="error" ) }}"
-
 - name: lookup ssm parameter warn if access is denied
   debug: msg="{{ lookup('aws_ssm', 'missing-parameter', on_denied="warn" ) }}"
 '''
@@ -173,8 +167,8 @@ def _boto3_conn(region, credentials):
 class LookupModule(LookupBase):
     def run(self, terms, variables=None, boto_profile=None, aws_profile=None,
             aws_secret_key=None, aws_access_key=None, aws_security_token=None, region=None,
-            bypath=False, shortnames=False, recursive=False, decrypt=True, on_missing="skip",
-            on_denied="skip"):
+            bypath=False, shortnames=False, recursive=False, decrypt=True, on_missing="error",
+            on_denied="error"):
         '''
             :arg terms: a list of lookups to run.
                 e.g. ['parameter_name', 'parameter_name_too' ]
@@ -220,40 +214,53 @@ def run(self, terms, variables=None, boto_profile=None, aws_profile=None,
         if bypath:
             ssm_dict['Recursive'] = recursive
             for term in terms:
-                ssm_dict["Path"] = term
                 display.vvv("AWS_ssm path lookup term: %s in region: %s" % (term, region))
-                try:
-                    response = client.get_parameters_by_path(**ssm_dict)
-                except botocore.exceptions.ClientError as e:
-                    raise AnsibleError("SSM lookup exception: {0}".format(to_native(e)))
-                paramlist = list()
-                paramlist.extend(response['Parameters'])
-
-                # Manual pagination, since boto doesn't support it yet for get_parameters_by_path
-                while 'NextToken' in response:
-                    response = client.get_parameters_by_path(NextToken=response['NextToken'], **ssm_dict)
-                    paramlist.extend(response['Parameters'])
-
-                # shorten parameter names. yes, this will return duplicate names with different values.
+
+                paramlist = self.get_path_parameters(client, ssm_dict, term, on_missing.lower(), on_denied.lower())
+                # Shorten parameter names. Yes, this will return
+                # duplicate names with different values.
                 if shortnames:
                     for x in paramlist:
                         x['Name'] = x['Name'][x['Name'].rfind('/') + 1:]
 
                 display.vvvv("AWS_ssm path lookup returned: %s" % str(paramlist))
-                if len(paramlist):
-                    ret.append(boto3_tag_list_to_ansible_dict(paramlist,
-                                                              tag_name_key_name="Name",
-                                                              tag_value_key_name="Value"))
-                else:
-                    ret.append({})
-            # Lookup by parameter name - always returns a list with one or no entry.
+
+                ret.append(boto3_tag_list_to_ansible_dict(paramlist,
+                                                          tag_name_key_name="Name",
+                                                          tag_value_key_name="Value"))
+        # Lookup by parameter name - always returns a list with one or
+        # no entry.
         else:
             display.vvv("AWS_ssm name lookup term: %s" % terms)
             for term in terms:
                 ret.append(self.get_parameter_value(client, ssm_dict, term, on_missing.lower(), on_denied.lower()))
         display.vvvv("AWS_ssm path lookup returning: %s " % str(ret))
         return ret
 
+    def get_path_parameters(self, client, ssm_dict, term, on_missing, on_denied):
+        ssm_dict["Path"] = term
+        paginator = client.get_paginator('get_parameters_by_path')
+        try:
+            paramlist = paginator.paginate(**ssm_dict).build_full_result()['Parameters']
+        except is_boto3_error_code('AccessDeniedException'):
+            if on_denied == 'error':
+                raise AnsibleError("Failed to access SSM parameter path %s (AccessDenied)" % term)
+            elif on_denied == 'warn':
+                self._display.warning('Skipping, access denied for SSM parameter path %s' % term)
+                paramlist = [{}]
+            elif on_denied == 'skip':
+                paramlist = [{}]
+        except botocore.exceptions.ClientError as e:  # pylint: disable=duplicate-except
+            raise AnsibleError("SSM lookup exception: {0}".format(to_native(e)))
+
+        if not len(paramlist):
+            if on_missing == "error":
+                raise AnsibleError("Failed to find SSM parameter path %s (ResourceNotFound)" % term)
+            elif on_missing == "warn":
+                self._display.warning('Skipping, did not find SSM parameter path %s' % term)
+
+        return paramlist
+
     def get_parameter_value(self, client, ssm_dict, term, on_missing, on_denied):
         ssm_dict["Name"] = term
         try:

tests/unit/plugins/lookup/test_aws_ssm.py

@@ -128,43 +128,51 @@ def test_path_lookup_variable(mocker):
     lookup._load_name = "aws_ssm"
 
     boto3_double = mocker.MagicMock()
-    get_path_fn = boto3_double.Session.return_value.client.return_value.get_parameters_by_path
-    get_path_fn.return_value = path_success_response
+    get_paginator_fn = boto3_double.Session.return_value.client.return_value.get_paginator
+    paginator = get_paginator_fn.return_value
+    paginator.paginate.return_value.build_full_result.return_value = path_success_response
     boto3_client_double = boto3_double.Session.return_value.client
 
     mocker.patch.object(boto3, 'session', boto3_double)
     args = copy(dummy_credentials)
-    args["bypath"] = 'true'
+    args["bypath"] = True
+    args["recursive"] = True
     retval = lookup.run(["/testpath"], {}, **args)
     assert(retval[0]["/testpath/won"] == "simple_value_won")
     assert(retval[0]["/testpath/too"] == "simple_value_too")
     boto3_client_double.assert_called_with('ssm', 'eu-west-1', aws_access_key_id='notakey',
                                            aws_secret_access_key="notasecret", aws_session_token=None)
-    get_path_fn.assert_called_with(Path="/testpath", Recursive=False, WithDecryption=True)
+    get_paginator_fn.assert_called_with('get_parameters_by_path')
+    paginator.paginate.assert_called_with(Path="/testpath", Recursive=True, WithDecryption=True)
+    paginator.paginate.return_value.build_full_result.assert_called_with()
 
 
-def test_return_none_for_missing_variable(mocker):
-    """
-    during jinja2 templates, we can't shouldn't normally raise exceptions since this blocks the ability to use defaults.
+def test_warn_on_missing_match_retvals_to_call_params_with_some_missing_variables(mocker):
+    """If we get a complex list of variables with some missing and some
+    not, and on_missing is warn, we still have to return a list which
+    matches with the original variable list.
 
-    for this reason we return ```None``` for missing variables
     """
     lookup = aws_ssm.LookupModule()
     lookup._load_name = "aws_ssm"
 
     boto3_double = mocker.MagicMock()
+
     boto3_double.Session.return_value.client.return_value.get_parameter.side_effect = mock_get_parameter
 
     mocker.patch.object(boto3, 'session', boto3_double)
-    retval = lookup.run(["missing_variable"], {}, **dummy_credentials)
+    args = copy(dummy_credentials)
+    args["on_missing"] = 'warn'
+    retval = lookup.run(["simple", "missing_variable", "/testpath/won", "simple"], {}, **args)
     assert(isinstance(retval, list))
-    assert(retval[0] is None)
+    assert(retval == ["simple_value", None, "simple_value_won", "simple_value"])
 
 
-def test_match_retvals_to_call_params_even_with_some_missing_variables(mocker):
-    """
-    If we get a complex list of variables with some missing and some not, we still have to return a
-    list which matches with the original variable list.
+def test_skip_on_missing_match_retvals_to_call_params_with_some_missing_variables(mocker):
+    """If we get a complex list of variables with some missing and some
+    not, and on_missing is skip, we still have to return a list which
+    matches with the original variable list.
+
     """
     lookup = aws_ssm.LookupModule()
     lookup._load_name = "aws_ssm"
@@ -174,7 +182,9 @@ def test_match_retvals_to_call_params_even_with_some_missing_variables(mocker):
     boto3_double.Session.return_value.client.return_value.get_parameter.side_effect = mock_get_parameter
 
     mocker.patch.object(boto3, 'session', boto3_double)
-    retval = lookup.run(["simple", "missing_variable", "/testpath/won", "simple"], {}, **dummy_credentials)
+    args = copy(dummy_credentials)
+    args["on_missing"] = 'skip'
+    retval = lookup.run(["simple", "missing_variable", "/testpath/won", "simple"], {}, **args)
     assert(isinstance(retval, list))
     assert(retval == ["simple_value", None, "simple_value_won", "simple_value"])
 
@@ -246,7 +256,7 @@ def test_skip_on_missing_variable(mocker):
     boto3_double.Session.return_value.client.return_value.get_parameter.side_effect = mock_get_parameter
 
     missing_credentials = copy(dummy_credentials)
-    missing_credentials['on_missing'] = "warn"
+    missing_credentials['on_missing'] = "skip"
     mocker.patch.object(boto3, 'session', boto3_double)
     retval = lookup.run(["missing_variable"], {}, **missing_credentials)
     assert(isinstance(retval, list))
@@ -307,7 +317,7 @@ def test_skip_on_denied_variable(mocker):
     boto3_double.Session.return_value.client.return_value.get_parameter.side_effect = mock_get_parameter
 
     denied_credentials = copy(dummy_credentials)
-    denied_credentials['on_denied'] = "warn"
+    denied_credentials['on_denied'] = "skip"
     mocker.patch.object(boto3, 'session', boto3_double)
     retval = lookup.run(["denied_variable"], {}, **denied_credentials)
     assert(isinstance(retval, list))