Revert "Fix on_denied and on_missing bugs (#618) (#747)" (#764)

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

SUMMARY

This reverts commit 83a9db6 because it introduces a breaking change that does not need to be included in the current release.

ISSUE TYPE


Bugfix Pull Request
Docs Pull Request
Feature Pull Request
New Module Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Jill R <None>

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))