From e889b1063f60f6b99f5d031f7e903f7be5f58900 Mon Sep 17 00:00:00 2001 From: Sam Doran Date: Wed, 18 Nov 2020 14:15:33 -0500 Subject: [PATCH] arg_spec - rework _check_arguments() (#72447) * Move _syslog_facitily to __init__ No good reason it should not be set for each object * Move internal property setting to private method * Create check_arguments() function * Remove unused import * Rename function to better match its behavior Change the behavior to return a set, either empty or populated, with unsupported keys. Accept legal_inputs as optional which will not required calling handle_aliases before calling get_unsupported_parameters(). * Add changelog * Rework function behavior and documentation I realized I missed the original intent of this method when moving it to a function. It is meant to compared the parameter keys to legal inputs always, not compare parameter keys to argument spec keys, even though the argument spec keys should be a subset of legal inputs. * Add tests * Fix typo. * Set internal properties when handling suboptions --- ...rg_spec-check_arguments-handle_aliases.yml | 2 + lib/ansible/module_utils/basic.py | 41 +++++++----- lib/ansible/module_utils/common/parameters.py | 25 +++++++ lib/ansible/module_utils/common/validation.py | 2 +- .../common/parameters/test_check_arguments.py | 65 +++++++++++++++++++ 5 files changed, 117 insertions(+), 18 deletions(-) create mode 100644 changelogs/fragments/arg_spec-check_arguments-handle_aliases.yml create mode 100644 test/units/module_utils/common/parameters/test_check_arguments.py diff --git a/changelogs/fragments/arg_spec-check_arguments-handle_aliases.yml b/changelogs/fragments/arg_spec-check_arguments-handle_aliases.yml new file mode 100644 index 00000000000..15754e119c6 --- /dev/null +++ b/changelogs/fragments/arg_spec-check_arguments-handle_aliases.yml @@ -0,0 +1,2 @@ +minor_changes: + - create ``get_unsupported_parameters`` validation function (https://github.com/ansible/ansible/pull/72447/files) diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index 61db4a30504..55b44e3e2db 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -156,6 +156,7 @@ from ansible.module_utils.common.sys_info import ( ) from ansible.module_utils.pycompat24 import get_exception, literal_eval from ansible.module_utils.common.parameters import ( + get_unsupported_parameters, handle_aliases, list_deprecations, list_no_log_values, @@ -692,6 +693,7 @@ class AnsibleModule(object): self._diff = False self._socket_path = None self._shell = None + self._syslog_facility = 'LOG_USER' self._verbosity = 0 # May be used to set modifications to the environment for any # run_command invocation @@ -728,6 +730,7 @@ class AnsibleModule(object): # a known valid (LANG=C) if it's an invalid/unavailable locale self._check_locale() + self._set_internal_properties() self._check_arguments() # check exclusive early @@ -1527,29 +1530,20 @@ class AnsibleModule(object): deprecate(message['msg'], version=message.get('version'), date=message.get('date'), collection_name=message.get('collection_name')) - def _check_arguments(self, spec=None, param=None, legal_inputs=None): - self._syslog_facility = 'LOG_USER' - unsupported_parameters = set() - if spec is None: - spec = self.argument_spec - if param is None: - param = self.params - if legal_inputs is None: - legal_inputs = self._legal_inputs - - for k in list(param.keys()): - - if k not in legal_inputs: - unsupported_parameters.add(k) + def _set_internal_properties(self, argument_spec=None, module_parameters=None): + if argument_spec is None: + argument_spec = self.argument_spec + if module_parameters is None: + module_parameters = self.params for k in PASS_VARS: # handle setting internal properties from internal ansible vars param_key = '_ansible_%s' % k - if param_key in param: + if param_key in module_parameters: if k in PASS_BOOLS: - setattr(self, PASS_VARS[k][0], self.boolean(param[param_key])) + setattr(self, PASS_VARS[k][0], self.boolean(module_parameters[param_key])) else: - setattr(self, PASS_VARS[k][0], param[param_key]) + setattr(self, PASS_VARS[k][0], module_parameters[param_key]) # clean up internal top level params: if param_key in self.params: @@ -1559,6 +1553,17 @@ class AnsibleModule(object): if not hasattr(self, PASS_VARS[k][0]): setattr(self, PASS_VARS[k][0], PASS_VARS[k][1]) + def _check_arguments(self, spec=None, param=None, legal_inputs=None): + unsupported_parameters = set() + if spec is None: + spec = self.argument_spec + if param is None: + param = self.params + if legal_inputs is None: + legal_inputs = self._legal_inputs + + unsupported_parameters = get_unsupported_parameters(spec, param, legal_inputs) + if unsupported_parameters: msg = "Unsupported parameters for (%s) module: %s" % (self._name, ', '.join(sorted(list(unsupported_parameters)))) if self._options_context: @@ -1571,6 +1576,7 @@ class AnsibleModule(object): supported_parameters.append(key) msg += " Supported parameters include: %s" % (', '.join(supported_parameters)) self.fail_json(msg=msg) + if self.check_mode and not self.supports_check_mode: self.exit_json(skipped=True, msg="remote module (%s) does not support check mode" % self._name) @@ -1817,6 +1823,7 @@ class AnsibleModule(object): options_legal_inputs = list(spec.keys()) + list(options_aliases.keys()) + self._set_internal_properties(spec, param) self._check_arguments(spec, param, options_legal_inputs) # check exclusive early diff --git a/lib/ansible/module_utils/common/parameters.py b/lib/ansible/module_utils/common/parameters.py index 4cf631e1e9d..4e263d932f5 100644 --- a/lib/ansible/module_utils/common/parameters.py +++ b/lib/ansible/module_utils/common/parameters.py @@ -195,3 +195,28 @@ def handle_aliases(argument_spec, params, alias_warnings=None): params[k] = params[alias] return aliases_results, legal_inputs + + +def get_unsupported_parameters(argument_spec, module_parameters, legal_inputs=None): + """Check keys in module_parameters against those provided in legal_inputs + to ensure they contain legal values. If legal_inputs are not supplied, + they will be generated using the argument_spec. + + :arg argument_spec: Dictionary of parameters, their type, and valid values. + :arg module_parameters: Dictionary of module parameters. + :arg legal_inputs: List of valid key names property names. Overrides values + in argument_spec. + + :returns: Set of unsupported parameters. Empty set if no unsupported parameters + are found. + """ + + if legal_inputs is None: + aliases, legal_inputs = handle_aliases(argument_spec, module_parameters) + + unsupported_parameters = set() + for k in module_parameters.keys(): + if k not in legal_inputs: + unsupported_parameters.add(k) + + return unsupported_parameters diff --git a/lib/ansible/module_utils/common/validation.py b/lib/ansible/module_utils/common/validation.py index fc13f4d0aab..a53a39458c0 100644 --- a/lib/ansible/module_utils/common/validation.py +++ b/lib/ansible/module_utils/common/validation.py @@ -9,7 +9,7 @@ import os import re from ast import literal_eval -from ansible.module_utils._text import to_native, to_text +from ansible.module_utils._text import to_native from ansible.module_utils.common._json_compat import json from ansible.module_utils.common.collections import is_iterable from ansible.module_utils.common.text.converters import jsonify diff --git a/test/units/module_utils/common/parameters/test_check_arguments.py b/test/units/module_utils/common/parameters/test_check_arguments.py new file mode 100644 index 00000000000..48bbfe7d716 --- /dev/null +++ b/test/units/module_utils/common/parameters/test_check_arguments.py @@ -0,0 +1,65 @@ +# -*- coding: utf-8 -*- +# Copyright (c) 2020 Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import absolute_import, division, print_function +__metaclass__ = type + + +import pytest + +from ansible.module_utils.common.parameters import get_unsupported_parameters + + +@pytest.fixture +def argument_spec(): + return { + 'state': {'aliases': ['status']}, + 'enabled': {}, + } + + +def mock_handle_aliases(*args): + aliases = {} + legal_inputs = [ + '_ansible_check_mode', + '_ansible_debug', + '_ansible_diff', + '_ansible_keep_remote_files', + '_ansible_module_name', + '_ansible_no_log', + '_ansible_remote_tmp', + '_ansible_selinux_special_fs', + '_ansible_shell_executable', + '_ansible_socket', + '_ansible_string_conversion_action', + '_ansible_syslog_facility', + '_ansible_tmpdir', + '_ansible_verbosity', + '_ansible_version', + 'state', + 'status', + 'enabled', + ] + + return aliases, legal_inputs + + +@pytest.mark.parametrize( + ('module_parameters', 'legal_inputs', 'expected'), + ( + ({'fish': 'food'}, ['state', 'enabled'], set(['fish'])), + ({'state': 'enabled', 'path': '/var/lib/path'}, None, set(['path'])), + ({'state': 'enabled', 'path': '/var/lib/path'}, ['state', 'path'], set()), + ({'state': 'enabled', 'path': '/var/lib/path'}, ['state'], set(['path'])), + ({}, None, set()), + ({'state': 'enabled'}, None, set()), + ({'status': 'enabled', 'enabled': True, 'path': '/var/lib/path'}, None, set(['path'])), + ({'status': 'enabled', 'enabled': True}, None, set()), + ) +) +def test_check_arguments(argument_spec, module_parameters, legal_inputs, expected, mocker): + mocker.patch('ansible.module_utils.common.parameters.handle_aliases', side_effect=mock_handle_aliases) + result = get_unsupported_parameters(argument_spec, module_parameters, legal_inputs) + + assert result == expected