Revert "Fix on_denied and on_missing bugs (ansible-collections#618) (a…

…nsible-collections#747)"

This reverts commit 83a9db6.

plugins/lookup/aws_ssm.py

@@ -23,15 +23,18 @@
     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 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).
+  - 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)
   - 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 lookup will generate an error.
+    If there is no parameter under that path then the return will be successful but the
+    dictionary will be empty.
   - If the lookup fails due to lack of permissions or due to an AWS client error then the aws_ssm
-    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
+    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
     of that variable with a default.  See the examples below.
 
 options:
@@ -78,26 +81,26 @@
 - name: lookup ssm parameter store in the current region
   debug: msg="{{ lookup('aws_ssm', 'Hello' ) }}"
 
-- name: lookup ssm parameter store in specified region
+- name: lookup ssm parameter store in nominated region
   debug: msg="{{ lookup('aws_ssm', 'Hello', region='us-east-2' ) }}"
 
-- name: lookup ssm parameter store without decryption
+- name: lookup ssm parameter store without decrypted
   debug: msg="{{ lookup('aws_ssm', 'Hello', decrypt=False ) }}"
 
-- name: lookup ssm parameter store using a specified aws profile
+- name: lookup ssm parameter store in nominated 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 ssm parameter and fail if missing
-  debug: msg="{{ lookup('aws_ssm', 'missing-parameter') }}"
+- name: lookup a key which doesn't exist, returns ""
+  debug: msg="{{ lookup('aws_ssm', 'NoKey') }}"
 
 - name: lookup a key which doesn't exist, returning a default ('root')
-  debug: msg="{{ lookup('aws_ssm', 'AdminID', on_missing="skip") | default('root', true) }}"
+  debug: msg="{{ lookup('aws_ssm', 'AdminID') | default('root', true) }}"
 
 - name: lookup a key which doesn't exist failing to store it in a fact
   set_fact:
@@ -121,6 +124,9 @@
   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" ) }}"
 '''
@@ -167,8 +173,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="error",
-            on_denied="error"):
+            bypath=False, shortnames=False, recursive=False, decrypt=True, on_missing="skip",
+            on_denied="skip"):
         '''
             :arg terms: a list of lookups to run.
                 e.g. ['parameter_name', 'parameter_name_too' ]
@@ -214,53 +220,40 @@ 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))
-
-                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.
+                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.
                 if shortnames:
                     for x in paramlist:
                         x['Name'] = x['Name'][x['Name'].rfind('/') + 1:]
 
                 display.vvvv("AWS_ssm path lookup returned: %s" % str(paramlist))
-
-                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.
+                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.
         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,51 +128,43 @@ def test_path_lookup_variable(mocker):
     lookup._load_name = "aws_ssm"
 
     boto3_double = mocker.MagicMock()
-    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
+    get_path_fn = boto3_double.Session.return_value.client.return_value.get_parameters_by_path
+    get_path_fn.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["recursive"] = True
+    args["bypath"] = '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_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()
+    get_path_fn.assert_called_with(Path="/testpath", Recursive=False, WithDecryption=True)
 
 
-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.
+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.
 
+    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)
-    args = copy(dummy_credentials)
-    args["on_missing"] = 'warn'
-    retval = lookup.run(["simple", "missing_variable", "/testpath/won", "simple"], {}, **args)
+    retval = lookup.run(["missing_variable"], {}, **dummy_credentials)
     assert(isinstance(retval, list))
-    assert(retval == ["simple_value", None, "simple_value_won", "simple_value"])
-
+    assert(retval[0] is None)
 
-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.
 
+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.
     """
     lookup = aws_ssm.LookupModule()
     lookup._load_name = "aws_ssm"
@@ -182,9 +174,7 @@ def test_skip_on_missing_match_retvals_to_call_params_with_some_missing_variable
     boto3_double.Session.return_value.client.return_value.get_parameter.side_effect = mock_get_parameter
 
     mocker.patch.object(boto3, 'session', boto3_double)
-    args = copy(dummy_credentials)
-    args["on_missing"] = 'skip'
-    retval = lookup.run(["simple", "missing_variable", "/testpath/won", "simple"], {}, **args)
+    retval = lookup.run(["simple", "missing_variable", "/testpath/won", "simple"], {}, **dummy_credentials)
     assert(isinstance(retval, list))
     assert(retval == ["simple_value", None, "simple_value_won", "simple_value"])
 
@@ -256,7 +246,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'] = "skip"
+    missing_credentials['on_missing'] = "warn"
     mocker.patch.object(boto3, 'session', boto3_double)
     retval = lookup.run(["missing_variable"], {}, **missing_credentials)
     assert(isinstance(retval, list))
@@ -317,7 +307,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'] = "skip"
+    denied_credentials['on_denied'] = "warn"
     mocker.patch.object(boto3, 'session', boto3_double)
     retval = lookup.run(["denied_variable"], {}, **denied_credentials)
     assert(isinstance(retval, list))