From e86a4d5d8296c68f1cd922b3f51d1cb0cffc2274 Mon Sep 17 00:00:00 2001 From: Alina Buzachis Date: Tue, 5 Apr 2022 15:20:03 +0200 Subject: [PATCH] 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 Reviewed-by: Jill R --- ...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))