From 6ec0cb0dde81e686ba2cb18b276fdeefdddf62c9 Mon Sep 17 00:00:00 2001 From: Alina Buzachis Date: Thu, 31 Mar 2022 13:56:25 +0200 Subject: [PATCH] Revert "Fix on_denied and on_missing bugs (#618) (#747)" This reverts commit 83a9db63d725cab9ce172c611e897db15a90ad0e. --- ...ing-and-on-denied-now-default-to-error.yml | 2 - plugins/lookup/aws_ssm.py | 93 +++++++++---------- tests/unit/plugins/lookup/test_aws_ssm.py | 44 ++++----- 3 files changed, 60 insertions(+), 79 deletions(-) delete mode 100644 changelogs/fragments/617-aws_ssm-on_missing-and-on-denied-now-default-to-error.yml diff --git a/changelogs/fragments/617-aws_ssm-on_missing-and-on-denied-now-default-to-error.yml b/changelogs/fragments/617-aws_ssm-on_missing-and-on-denied-now-default-to-error.yml deleted file mode 100644 index 452d38c8411..00000000000 --- a/changelogs/fragments/617-aws_ssm-on_missing-and-on-denied-now-default-to-error.yml +++ /dev/null @@ -1,2 +0,0 @@ -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). diff --git a/plugins/lookup/aws_ssm.py b/plugins/lookup/aws_ssm.py index 2333dc53f1f..89cfe379465 100644 --- a/plugins/lookup/aws_ssm.py +++ b/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,22 +220,33 @@ 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: @@ -237,30 +254,6 @@ def run(self, terms, variables=None, boto_profile=None, aws_profile=None, 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: diff --git a/tests/unit/plugins/lookup/test_aws_ssm.py b/tests/unit/plugins/lookup/test_aws_ssm.py index d54f1dc62fb..bb7e138fa80 100644 --- a/tests/unit/plugins/lookup/test_aws_ssm.py +++ b/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))