From 7b1075ed7ff2aee7ecc7ade94f2ebde0c493c72a Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 11 Aug 2024 15:59:33 +0200 Subject: [PATCH 1/8] Allow to restrict api_info output. --- plugins/module_utils/_api_helper.py | 40 +++++ plugins/modules/api_info.py | 47 ++++++ tests/unit/plugins/modules/test_api_info.py | 166 ++++++++++++++++++++ 3 files changed, 253 insertions(+) create mode 100644 plugins/module_utils/_api_helper.py diff --git a/plugins/module_utils/_api_helper.py b/plugins/module_utils/_api_helper.py new file mode 100644 index 00000000..211077b5 --- /dev/null +++ b/plugins/module_utils/_api_helper.py @@ -0,0 +1,40 @@ +# -*- coding: utf-8 -*- +# Copyright (c) 2022, Felix Fontein (@felixfontein) +# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt) +# SPDX-License-Identifier: GPL-3.0-or-later + +# The data inside here is private to this collection. If you use this from outside the collection, +# you are on your own. There can be random changes to its format even in bugfix releases! + +from __future__ import absolute_import, division, print_function +__metaclass__ = type + + +def validate_restrict(module, path_info): + restrict = module.params['restrict'] + if restrict is None: + return + for entry in restrict: + field = entry['field'] + if field.startswith('!'): + module.fail_json(msg='restrict: the field name "{0}" must not start with "!"'.format(field)) + f = path_info.fields.get(field) + if f is None: + module.fail_json(msg='restrict: the field "{0}" does not exist for this path'.format(field)) + + +def entry_accepted(entry, path_info, module): + restrict = module.params['restrict'] + if restrict is None: + return True + for rule in restrict: + field = rule['field'] + field_info = path_info.fields[field] + value = entry.get(field) + if value is None: + value = field_info.default + if field not in entry and field_info.absent_value: + value = field_info.absent_value + if value not in rule['values']: + return False + return True diff --git a/plugins/modules/api_info.py b/plugins/modules/api_info.py index 4a5eff4d..14cf644e 100644 --- a/plugins/modules/api_info.py +++ b/plugins/modules/api_info.py @@ -301,6 +301,29 @@ type: bool default: false version_added: 2.10.0 + restrict: + description: + - Restrict output to entries matching the following criteria. + type: list + elements: dict + version_added: 2.18.0 + suboptions: + field: + description: + - The field whose values to restrict. + required: true + type: str + values: + description: + - The values of the field to limit to. + - "Note that the types of the values are important. If you provide a string V("0"), + and librouteros converts the value returned by the API to the integer V(0), + then this will not match. If you are not sure, better include both variants: + both the string and the integer." + - Use V(none) for disabled values. + required: true + type: list + elements: raw seealso: - module: community.routeros.api - module: community.routeros.api_facts @@ -318,6 +341,18 @@ path: ip address register: ip_addresses +- name: Print data for IP addresses + ansible.builtin.debug: + var: ip_addresses.result + +- name: Get IP addresses + community.routeros.api_info: + hostname: "{{ hostname }}" + password: "{{ password }}" + username: "{{ username }}" + path: ip address + register: ip_addresses + - name: Print data for IP addresses ansible.builtin.debug: var: ip_addresses.result @@ -358,6 +393,11 @@ split_path, ) +from ansible_collections.community.routeros.plugins.module_utils._api_helper import ( + entry_accepted, + validate_restrict, +) + try: from librouteros.exceptions import LibRouterosError except Exception: @@ -381,6 +421,10 @@ def main(): include_dynamic=dict(type='bool', default=False), include_builtin=dict(type='bool', default=False), include_read_only=dict(type='bool', default=False), + restrict=dict(type='list', elements='dict', options=dict( + field=dict(type='str', required=True), + values=dict(type='list', elements='raw', required=True), + )), ) module_args.update(api_argument_spec()) @@ -411,6 +455,7 @@ def main(): include_dynamic = module.params['include_dynamic'] include_builtin = module.params['include_builtin'] include_read_only = module.params['include_read_only'] + validate_restrict(module, path_info) try: api_path = compose_api_path(api, path) @@ -423,6 +468,8 @@ def main(): if not include_builtin: if entry.get('builtin', False): continue + if not entry_accepted(entry, path_info, module): + continue if not unfiltered: for k in list(entry): if k == '.id': diff --git a/tests/unit/plugins/modules/test_api_info.py b/tests/unit/plugins/modules/test_api_info.py index 3564668e..1c9d0141 100644 --- a/tests/unit/plugins/modules/test_api_info.py +++ b/tests/unit/plugins/modules/test_api_info.py @@ -822,3 +822,169 @@ def test_default_disable_2(self, mock_compose_api_path): 'comment': 'foo', }, ]) + + @patch('ansible_collections.community.routeros.plugins.modules.api_info.compose_api_path') + def test_restrict_1(self, mock_compose_api_path): + mock_compose_api_path.return_value = [ + { + 'chain': 'input', + 'in-interface-list': 'LAN', + 'dynamic': False, + '.id': '*1', + }, + { + 'chain': 'forward', + 'action': 'drop', + 'in-interface': 'sfp1', + '.id': '*2', + 'dynamic': False, + }, + ] + with self.assertRaises(AnsibleExitJson) as exc: + args = self.config_module_args.copy() + args.update({ + 'path': 'ip firewall filter', + 'handle_disabled': 'omit', + 'restrict': [], + }) + set_module_args(args) + self.module.main() + + result = exc.exception.args[0] + self.assertEqual(result['changed'], False) + self.assertEqual(result['result'], [ + { + 'chain': 'input', + 'in-interface-list': 'LAN', + '.id': '*1', + }, + { + 'chain': 'forward', + 'action': 'drop', + 'in-interface': 'sfp1', + '.id': '*2', + }, + ]) + + @patch('ansible_collections.community.routeros.plugins.modules.api_info.compose_api_path') + def test_restrict_2(self, mock_compose_api_path): + mock_compose_api_path.return_value = [ + { + 'chain': 'input', + 'in-interface-list': 'LAN', + 'dynamic': False, + '.id': '*1', + }, + { + 'chain': 'forward', + 'action': 'drop', + 'in-interface': 'sfp1', + '.id': '*2', + 'dynamic': False, + }, + { + 'chain': 'input', + 'action': 'drop', + 'dynamic': False, + '.id': '*3', + }, + ] + with self.assertRaises(AnsibleExitJson) as exc: + args = self.config_module_args.copy() + args.update({ + 'path': 'ip firewall filter', + 'handle_disabled': 'omit', + 'restrict': [{ + 'field': 'chain', + 'values': ['forward'], + }], + }) + set_module_args(args) + self.module.main() + + result = exc.exception.args[0] + self.assertEqual(result['changed'], False) + self.assertEqual(result['result'], [ + { + 'chain': 'forward', + 'action': 'drop', + 'in-interface': 'sfp1', + '.id': '*2', + }, + ]) + + @patch('ansible_collections.community.routeros.plugins.modules.api_info.compose_api_path') + def test_restrict_3(self, mock_compose_api_path): + mock_compose_api_path.return_value = [ + { + 'chain': 'input', + 'in-interface-list': 'LAN', + 'dynamic': False, + '.id': '*1', + }, + { + 'chain': 'forward', + 'action': 'drop', + 'in-interface': 'sfp1', + '.id': '*2', + 'dynamic': False, + }, + { + 'chain': 'input', + 'action': 'drop', + 'dynamic': False, + '.id': '*3', + }, + { + 'chain': 'input', + 'action': 'drop', + 'comment': 'Foo', + 'dynamic': False, + '.id': '*4', + }, + { + 'chain': 'input', + 'action': 'drop', + 'comment': 'Bar', + 'dynamic': False, + '.id': '*5', + }, + ] + with self.assertRaises(AnsibleExitJson) as exc: + args = self.config_module_args.copy() + args.update({ + 'path': 'ip firewall filter', + 'handle_disabled': 'omit', + 'restrict': [ + { + 'field': 'chain', + 'values': ['input', 'foobar'], + }, + { + 'field': 'action', + 'values': ['drop', 42], + }, + { + 'field': 'comment', + 'values': [None, 'Foo'], + }, + ], + }) + set_module_args(args) + self.module.main() + + result = exc.exception.args[0] + self.assertEqual(result['changed'], False) + self.assertEqual(result['result'], [ + { + 'chain': 'input', + 'action': 'drop', + '.id': '*3', + }, + { + 'chain': 'input', + 'action': 'drop', + 'comment': 'Foo', + '.id': '*4', + }, + ]) From 811323d11a9280df182eb2c1871ba16bdd3fba7d Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 11 Aug 2024 20:11:56 +0200 Subject: [PATCH 2/8] Allow to restrict what api_modify modifies. --- plugins/modules/api_modify.py | 77 ++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-) diff --git a/plugins/modules/api_modify.py b/plugins/modules/api_modify.py index 42abafb6..5a5bb200 100644 --- a/plugins/modules/api_modify.py +++ b/plugins/modules/api_modify.py @@ -333,6 +333,34 @@ - error default: create_only version_added: 2.10.0 + restrict: + description: + - Restrict operation to entries matching the following criteria. + - This can be useful together with O(handle_absent_entries=remove) to operate on a subset of + the values. + - For example, for O(path=ip firewall filter), you can set O(restrict[].field=chain) and + O(restrict[].values=input) to restrict operation to the input chain, and ignore the + forward and output chains. + type: list + elements: dict + version_added: 2.18.0 + suboptions: + field: + description: + - The field whose values to restrict. + required: true + type: str + values: + description: + - The values of the field to limit to. + - "Note that the types of the values are important. If you provide a string V("0"), + and librouteros converts the value returned by the API to the integer V(0), + then this will not match. If you are not sure, better include both variants: + both the string and the integer." + - Use V(none) for disabled values. + required: true + type: list + elements: raw seealso: - module: community.routeros.api - module: community.routeros.api_facts @@ -378,6 +406,23 @@ out-interface: to-addresses: ~ '!to-ports': + +- name: Block all incoming connections + community.routeros.api_modify: + hostname: "{{ hostname }}" + password: "{{ password }}" + username: "{{ username }}" + path: ip firewall filter + handle_absent_entries: remove + handle_entries_content: remove_as_much_as_possible + restrict: + # Do not touch any chain except the input chain + - field: chain + values: + - input + data: + - action: drop + chain: input ''' RETURN = ''' @@ -434,6 +479,11 @@ split_path, ) +from ansible_collections.community.routeros.plugins.module_utils._api_helper import ( + entry_accepted, + validate_restrict, +) + HAS_ORDEREDDICT = True try: from collections import OrderedDict @@ -699,18 +749,29 @@ def prepare_for_add(entry, path_info): return new_entry +def remove_rejected(data, path_info, module): + return [ + entry for entry in data + if entry_accepted(entry, path_info, module) + ] + + def sync_list(module, api, path, path_info): handle_absent_entries = module.params['handle_absent_entries'] handle_entries_content = module.params['handle_entries_content'] if handle_absent_entries == 'remove': if handle_entries_content == 'ignore': - module.fail_json('For this path, handle_absent_entries=remove cannot be combined with handle_entries_content=ignore') + module.fail_json( + msg='For this path, handle_absent_entries=remove cannot be combined with handle_entries_content=ignore' + ) stratify_keys = path_info.stratify_keys or () data = module.params['data'] stratified_data = defaultdict(list) for index, entry in enumerate(data): + if not entry_accepted(entry, path_info, module): + module.fail_json(msg='The element at index #{index} does not match `restrict`'.format(index=index + 1)) for stratify_key in stratify_keys: if stratify_key not in entry: module.fail_json( @@ -731,6 +792,7 @@ def sync_list(module, api, path, path_info): old_data = get_api_data(api_path, path_info) old_data = remove_dynamic(old_data) + old_data = remove_rejected(old_data, path_info, module) stratified_old_data = defaultdict(list) for index, entry in enumerate(old_data): sks = tuple(entry[stratify_key] for stratify_key in stratify_keys) @@ -843,6 +905,7 @@ def match(current_entry): # For sake of completeness, retrieve the full new data: if modify_list or create_list or reorder_list: new_data = remove_dynamic(get_api_data(api_path, path_info)) + new_data = remove_rejected(new_data, path_info, module) # Remove 'irrelevant' data for entry in old_data: @@ -881,6 +944,8 @@ def sync_with_primary_keys(module, api, path, path_info): data = module.params['data'] new_data_by_key = OrderedDict() for index, entry in enumerate(data): + if not entry_accepted(entry, path_info, module): + module.fail_json(msg='The element at index #{index} does not match `restrict`'.format(index=index + 1)) for primary_key in primary_keys: if primary_key not in entry: module.fail_json( @@ -912,6 +977,7 @@ def sync_with_primary_keys(module, api, path, path_info): old_data = get_api_data(api_path, path_info) old_data = remove_dynamic(old_data) + old_data = remove_rejected(old_data, path_info, module) old_data_by_key = OrderedDict() id_by_key = {} for entry in old_data: @@ -1038,6 +1104,7 @@ def sync_with_primary_keys(module, api, path, path_info): # For sake of completeness, retrieve the full new data: if modify_list or create_list or reorder_list: new_data = remove_dynamic(get_api_data(api_path, path_info)) + new_data = remove_rejected(new_data, path_info, module) # Remove 'irrelevant' data for entry in old_data: @@ -1065,6 +1132,8 @@ def sync_with_primary_keys(module, api, path, path_info): def sync_single_value(module, api, path, path_info): + if module.params['restrict'] is not None: + module.fail_json(msg='The restrict option cannot be used with this path, since there is precisely one entry.') data = module.params['data'] if len(data) != 1: module.fail_json(msg='Data must be a list with exactly one element.') @@ -1160,6 +1229,10 @@ def main(): ensure_order=dict(type='bool', default=False), handle_read_only=dict(type='str', default='error', choices=['ignore', 'validate', 'error']), handle_write_only=dict(type='str', default='create_only', choices=['create_only', 'always_update', 'error']), + restrict=dict(type='list', elements='dict', options=dict( + field=dict(type='str', required=True), + values=dict(type='list', elements='raw', required=True), + )), ) module_args.update(api_argument_spec()) @@ -1193,6 +1266,8 @@ def main(): if path_info is None or backend is None: module.fail_json(msg='Path /{path} is not yet supported'.format(path='/'.join(path))) + validate_restrict(module, path_info) + backend(module, api, path, path_info) From 0f73768c936ab7abd108b6f5e9cc35329ce7be0e Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 11 Aug 2024 20:14:33 +0200 Subject: [PATCH 3/8] Add changelog. --- changelogs/fragments/305-api-restrict.yml | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelogs/fragments/305-api-restrict.yml diff --git a/changelogs/fragments/305-api-restrict.yml b/changelogs/fragments/305-api-restrict.yml new file mode 100644 index 00000000..3b737ea5 --- /dev/null +++ b/changelogs/fragments/305-api-restrict.yml @@ -0,0 +1,3 @@ +minor_changes: + - "api_info - allow to restrict the output by limiting fields to specific values with the new ``restrict`` option (https://github.com/ansible-collections/community.routeros/pull/305)." + - "api_modify - allow to restrict what is updated by limiting fields to specific values with the new ``restrict`` option (https://github.com/ansible-collections/community.routeros/pull/305)." From 5ce4321d46ca75818c6c01ab920d85a6116d3dab Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 11 Aug 2024 20:19:21 +0200 Subject: [PATCH 4/8] Fix docs. --- plugins/modules/api_info.py | 9 +++++---- plugins/modules/api_modify.py | 9 +++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/plugins/modules/api_info.py b/plugins/modules/api_info.py index 14cf644e..76ba12a4 100644 --- a/plugins/modules/api_info.py +++ b/plugins/modules/api_info.py @@ -316,10 +316,11 @@ values: description: - The values of the field to limit to. - - "Note that the types of the values are important. If you provide a string V("0"), - and librouteros converts the value returned by the API to the integer V(0), - then this will not match. If you are not sure, better include both variants: - both the string and the integer." + - >- + Note that the types of the values are important. If you provide a string V("0"), + and librouteros converts the value returned by the API to the integer V(0), + then this will not match. If you are not sure, better include both variants: + both the string and the integer. - Use V(none) for disabled values. required: true type: list diff --git a/plugins/modules/api_modify.py b/plugins/modules/api_modify.py index 5a5bb200..0e268ee2 100644 --- a/plugins/modules/api_modify.py +++ b/plugins/modules/api_modify.py @@ -353,10 +353,11 @@ values: description: - The values of the field to limit to. - - "Note that the types of the values are important. If you provide a string V("0"), - and librouteros converts the value returned by the API to the integer V(0), - then this will not match. If you are not sure, better include both variants: - both the string and the integer." + - >- + Note that the types of the values are important. If you provide a string V("0"), + and librouteros converts the value returned by the API to the integer V(0), + then this will not match. If you are not sure, better include both variants: + both the string and the integer. - Use V(none) for disabled values. required: true type: list From 3f8c90cb12c3a670be258382d4100a9d8cfa460f Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 11 Aug 2024 21:23:50 +0200 Subject: [PATCH 5/8] Move shared code/docs to module utils and doc fragments. --- plugins/doc_fragments/api.py | 25 +++++++++++++++++++++++++ plugins/module_utils/_api_helper.py | 13 +++++++++++++ plugins/modules/api_info.py | 27 +++------------------------ plugins/modules/api_modify.py | 27 +++------------------------ 4 files changed, 44 insertions(+), 48 deletions(-) diff --git a/plugins/doc_fragments/api.py b/plugins/doc_fragments/api.py index 7f77e8fe..ef6228ca 100644 --- a/plugins/doc_fragments/api.py +++ b/plugins/doc_fragments/api.py @@ -95,3 +95,28 @@ class ModuleDocFragment(object): - ref: ansible_collections.community.routeros.docsite.api-guide description: How to connect to RouterOS devices with the RouterOS API ''' + + RESTRICT = r''' +options: + restrict: + type: list + elements: dict + suboptions: + field: + description: + - The field whose values to restrict. + required: true + type: str + values: + description: + - The values of the field to limit to. + - >- + Note that the types of the values are important. If you provide a string V("0"), + and librouteros converts the value returned by the API to the integer V(0), + then this will not match. If you are not sure, better include both variants: + both the string and the integer. + - Use V(none) for disabled values. + required: true + type: list + elements: raw +''' diff --git a/plugins/module_utils/_api_helper.py b/plugins/module_utils/_api_helper.py index 211077b5..52392766 100644 --- a/plugins/module_utils/_api_helper.py +++ b/plugins/module_utils/_api_helper.py @@ -38,3 +38,16 @@ def entry_accepted(entry, path_info, module): if value not in rule['values']: return False return True + + +def restrict_argument_spec(): + return dict( + restrict=dict( + type='list', + elements='dict', + options=dict( + field=dict(type='str', required=True), + values=dict(type='list', elements='raw', required=True), + ), + ), + ) diff --git a/plugins/modules/api_info.py b/plugins/modules/api_info.py index 76ba12a4..f1a56535 100644 --- a/plugins/modules/api_info.py +++ b/plugins/modules/api_info.py @@ -26,6 +26,7 @@ L(create an issue in the community.routeros Issue Tracker,https://github.com/ansible-collections/community.routeros/issues/). extends_documentation_fragment: - community.routeros.api + - community.routeros.api.restrict - community.routeros.attributes - community.routeros.attributes.actiongroup_api - community.routeros.attributes.info_module @@ -304,27 +305,7 @@ restrict: description: - Restrict output to entries matching the following criteria. - type: list - elements: dict version_added: 2.18.0 - suboptions: - field: - description: - - The field whose values to restrict. - required: true - type: str - values: - description: - - The values of the field to limit to. - - >- - Note that the types of the values are important. If you provide a string V("0"), - and librouteros converts the value returned by the API to the integer V(0), - then this will not match. If you are not sure, better include both variants: - both the string and the integer. - - Use V(none) for disabled values. - required: true - type: list - elements: raw seealso: - module: community.routeros.api - module: community.routeros.api_facts @@ -396,6 +377,7 @@ from ansible_collections.community.routeros.plugins.module_utils._api_helper import ( entry_accepted, + restrict_argument_spec, validate_restrict, ) @@ -422,12 +404,9 @@ def main(): include_dynamic=dict(type='bool', default=False), include_builtin=dict(type='bool', default=False), include_read_only=dict(type='bool', default=False), - restrict=dict(type='list', elements='dict', options=dict( - field=dict(type='str', required=True), - values=dict(type='list', elements='raw', required=True), - )), ) module_args.update(api_argument_spec()) + module_args.update(restrict_argument_spec()) module = AnsibleModule( argument_spec=module_args, diff --git a/plugins/modules/api_modify.py b/plugins/modules/api_modify.py index 0e268ee2..ca9ea590 100644 --- a/plugins/modules/api_modify.py +++ b/plugins/modules/api_modify.py @@ -32,6 +32,7 @@ - Needs L(ordereddict,https://pypi.org/project/ordereddict) for Python 2.6 extends_documentation_fragment: - community.routeros.api + - community.routeros.api.restrict - community.routeros.attributes - community.routeros.attributes.actiongroup_api attributes: @@ -341,27 +342,7 @@ - For example, for O(path=ip firewall filter), you can set O(restrict[].field=chain) and O(restrict[].values=input) to restrict operation to the input chain, and ignore the forward and output chains. - type: list - elements: dict version_added: 2.18.0 - suboptions: - field: - description: - - The field whose values to restrict. - required: true - type: str - values: - description: - - The values of the field to limit to. - - >- - Note that the types of the values are important. If you provide a string V("0"), - and librouteros converts the value returned by the API to the integer V(0), - then this will not match. If you are not sure, better include both variants: - both the string and the integer. - - Use V(none) for disabled values. - required: true - type: list - elements: raw seealso: - module: community.routeros.api - module: community.routeros.api_facts @@ -482,6 +463,7 @@ from ansible_collections.community.routeros.plugins.module_utils._api_helper import ( entry_accepted, + restrict_argument_spec, validate_restrict, ) @@ -1230,12 +1212,9 @@ def main(): ensure_order=dict(type='bool', default=False), handle_read_only=dict(type='str', default='error', choices=['ignore', 'validate', 'error']), handle_write_only=dict(type='str', default='create_only', choices=['create_only', 'always_update', 'error']), - restrict=dict(type='list', elements='dict', options=dict( - field=dict(type='str', required=True), - values=dict(type='list', elements='raw', required=True), - )), ) module_args.update(api_argument_spec()) + module_args.update(restrict_argument_spec()) module = AnsibleModule( argument_spec=module_args, From 6951f8cd01abbd2f341bd843577c2f429e37c8ce Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 11 Aug 2024 21:52:59 +0200 Subject: [PATCH 6/8] Refactor and allow to match by regex. --- plugins/doc_fragments/api.py | 9 ++++- plugins/module_utils/_api_helper.py | 56 +++++++++++++++++++++++------ plugins/modules/api_info.py | 8 ++--- plugins/modules/api_modify.py | 30 ++++++++-------- 4 files changed, 73 insertions(+), 30 deletions(-) diff --git a/plugins/doc_fragments/api.py b/plugins/doc_fragments/api.py index ef6228ca..c10d844f 100644 --- a/plugins/doc_fragments/api.py +++ b/plugins/doc_fragments/api.py @@ -116,7 +116,14 @@ class ModuleDocFragment(object): then this will not match. If you are not sure, better include both variants: both the string and the integer. - Use V(none) for disabled values. - required: true + - Either O(restrict[].values) or O(restrict[].regex), but not both, must be specified. type: list elements: raw + regex: + description: + - A regular expression matching values of the field to limit to. + - Note that all values will be converted to strings before matching. + - It is not possible to match disabled values with regular expressions. + - Either O(restrict[].values) or O(restrict[].regex), but not both, must be specified. + type: str ''' diff --git a/plugins/module_utils/_api_helper.py b/plugins/module_utils/_api_helper.py index 52392766..505397fb 100644 --- a/plugins/module_utils/_api_helper.py +++ b/plugins/module_utils/_api_helper.py @@ -9,25 +9,43 @@ from __future__ import absolute_import, division, print_function __metaclass__ = type +import re -def validate_restrict(module, path_info): +from ansible.module_utils.common.text.converters import to_text + + +def validate_and_prepare_restrict(module, path_info): restrict = module.params['restrict'] if restrict is None: - return - for entry in restrict: - field = entry['field'] + return None + restrict_data = [] + for rule in restrict: + field = rule['field'] if field.startswith('!'): module.fail_json(msg='restrict: the field name "{0}" must not start with "!"'.format(field)) f = path_info.fields.get(field) if f is None: module.fail_json(msg='restrict: the field "{0}" does not exist for this path'.format(field)) + new_rule = dict(field=field) + if rule['values'] is not None: + new_rule['values'] = rule['values'] + elif rule['regex'] is not None: + regex = rule['regex'] + try: + new_rule['regex'] = re.compile(regex) + new_rule['regex_source'] = regex + except Exception as exc: + module.fail_json(msg='restrict: invalid regular expression "{0}": {1}'.format(regex, exc)) + restrict_data.append(new_rule) + return restrict_data -def entry_accepted(entry, path_info, module): - restrict = module.params['restrict'] - if restrict is None: + +def restrict_entry_accepted(entry, path_info, restrict_data): + if restrict_data is None: return True - for rule in restrict: + for rule in restrict_data: + # Obtain field and value field = rule['field'] field_info = path_info.fields[field] value = entry.get(field) @@ -35,8 +53,19 @@ def entry_accepted(entry, path_info, module): value = field_info.default if field not in entry and field_info.absent_value: value = field_info.absent_value - if value not in rule['values']: + + # Actual test + if 'values' in rule and value not in rule['values']: return False + if 'regex' in rule: + if value is None: + # regex cannot match None + return False + value_str = to_text(value) + if isinstance(value, bool): + value_str = value_str.lower() + if rule['regex'].match(value_str): + return False return True @@ -47,7 +76,14 @@ def restrict_argument_spec(): elements='dict', options=dict( field=dict(type='str', required=True), - values=dict(type='list', elements='raw', required=True), + values=dict(type='list', elements='raw'), + regex=dict(type='str'), ), + mutually_exclusive=[ + ('values', 'regex'), + ], + required_one_of=[ + ('values', 'regex'), + ], ), ) diff --git a/plugins/modules/api_info.py b/plugins/modules/api_info.py index f1a56535..24fd8fed 100644 --- a/plugins/modules/api_info.py +++ b/plugins/modules/api_info.py @@ -376,9 +376,9 @@ ) from ansible_collections.community.routeros.plugins.module_utils._api_helper import ( - entry_accepted, restrict_argument_spec, - validate_restrict, + restrict_entry_accepted, + validate_and_prepare_restrict, ) try: @@ -435,7 +435,7 @@ def main(): include_dynamic = module.params['include_dynamic'] include_builtin = module.params['include_builtin'] include_read_only = module.params['include_read_only'] - validate_restrict(module, path_info) + restrict_data = validate_and_prepare_restrict(module, path_info) try: api_path = compose_api_path(api, path) @@ -448,7 +448,7 @@ def main(): if not include_builtin: if entry.get('builtin', False): continue - if not entry_accepted(entry, path_info, module): + if not restrict_entry_accepted(entry, path_info, restrict_data): continue if not unfiltered: for k in list(entry): diff --git a/plugins/modules/api_modify.py b/plugins/modules/api_modify.py index ca9ea590..a2a2c102 100644 --- a/plugins/modules/api_modify.py +++ b/plugins/modules/api_modify.py @@ -462,9 +462,9 @@ ) from ansible_collections.community.routeros.plugins.module_utils._api_helper import ( - entry_accepted, restrict_argument_spec, - validate_restrict, + restrict_entry_accepted, + validate_and_prepare_restrict, ) HAS_ORDEREDDICT = True @@ -732,14 +732,14 @@ def prepare_for_add(entry, path_info): return new_entry -def remove_rejected(data, path_info, module): +def remove_rejected(data, path_info, restrict_data): return [ entry for entry in data - if entry_accepted(entry, path_info, module) + if restrict_entry_accepted(entry, path_info, restrict_data) ] -def sync_list(module, api, path, path_info): +def sync_list(module, api, path, path_info, restrict_data): handle_absent_entries = module.params['handle_absent_entries'] handle_entries_content = module.params['handle_entries_content'] if handle_absent_entries == 'remove': @@ -753,7 +753,7 @@ def sync_list(module, api, path, path_info): data = module.params['data'] stratified_data = defaultdict(list) for index, entry in enumerate(data): - if not entry_accepted(entry, path_info, module): + if not restrict_entry_accepted(entry, path_info, restrict_data): module.fail_json(msg='The element at index #{index} does not match `restrict`'.format(index=index + 1)) for stratify_key in stratify_keys: if stratify_key not in entry: @@ -775,7 +775,7 @@ def sync_list(module, api, path, path_info): old_data = get_api_data(api_path, path_info) old_data = remove_dynamic(old_data) - old_data = remove_rejected(old_data, path_info, module) + old_data = remove_rejected(old_data, path_info, restrict_data) stratified_old_data = defaultdict(list) for index, entry in enumerate(old_data): sks = tuple(entry[stratify_key] for stratify_key in stratify_keys) @@ -888,7 +888,7 @@ def match(current_entry): # For sake of completeness, retrieve the full new data: if modify_list or create_list or reorder_list: new_data = remove_dynamic(get_api_data(api_path, path_info)) - new_data = remove_rejected(new_data, path_info, module) + new_data = remove_rejected(new_data, path_info, restrict_data) # Remove 'irrelevant' data for entry in old_data: @@ -915,7 +915,7 @@ def match(current_entry): ) -def sync_with_primary_keys(module, api, path, path_info): +def sync_with_primary_keys(module, api, path, path_info, restrict_data): primary_keys = path_info.primary_keys if path_info.fixed_entries: @@ -927,7 +927,7 @@ def sync_with_primary_keys(module, api, path, path_info): data = module.params['data'] new_data_by_key = OrderedDict() for index, entry in enumerate(data): - if not entry_accepted(entry, path_info, module): + if not restrict_entry_accepted(entry, path_info, restrict_data): module.fail_json(msg='The element at index #{index} does not match `restrict`'.format(index=index + 1)) for primary_key in primary_keys: if primary_key not in entry: @@ -960,7 +960,7 @@ def sync_with_primary_keys(module, api, path, path_info): old_data = get_api_data(api_path, path_info) old_data = remove_dynamic(old_data) - old_data = remove_rejected(old_data, path_info, module) + old_data = remove_rejected(old_data, path_info, restrict_data) old_data_by_key = OrderedDict() id_by_key = {} for entry in old_data: @@ -1087,7 +1087,7 @@ def sync_with_primary_keys(module, api, path, path_info): # For sake of completeness, retrieve the full new data: if modify_list or create_list or reorder_list: new_data = remove_dynamic(get_api_data(api_path, path_info)) - new_data = remove_rejected(new_data, path_info, module) + new_data = remove_rejected(new_data, path_info, restrict_data) # Remove 'irrelevant' data for entry in old_data: @@ -1114,7 +1114,7 @@ def sync_with_primary_keys(module, api, path, path_info): ) -def sync_single_value(module, api, path, path_info): +def sync_single_value(module, api, path, path_info, restrict_data): if module.params['restrict'] is not None: module.fail_json(msg='The restrict option cannot be used with this path, since there is precisely one entry.') data = module.params['data'] @@ -1246,9 +1246,9 @@ def main(): if path_info is None or backend is None: module.fail_json(msg='Path /{path} is not yet supported'.format(path='/'.join(path))) - validate_restrict(module, path_info) + restrict_data = validate_and_prepare_restrict(module, path_info) - backend(module, api, path, path_info) + backend(module, api, path, path_info, restrict_data) if __name__ == '__main__': From ae3b313d413d56e3b2f9fc74a82c6a7e1b82184a Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Mon, 12 Aug 2024 19:21:37 +0200 Subject: [PATCH 7/8] Simplify rules, allow to invert rule matcher. --- plugins/doc_fragments/api.py | 15 +++++++-- plugins/module_utils/_api_helper.py | 51 ++++++++++++++++++----------- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/plugins/doc_fragments/api.py b/plugins/doc_fragments/api.py index c10d844f..b0ad9799 100644 --- a/plugins/doc_fragments/api.py +++ b/plugins/doc_fragments/api.py @@ -107,6 +107,11 @@ class ModuleDocFragment(object): - The field whose values to restrict. required: true type: str + match_disabled: + description: + - Whether disabled or not provided values should match. + type: bool + default: false values: description: - The values of the field to limit to. @@ -115,8 +120,6 @@ class ModuleDocFragment(object): and librouteros converts the value returned by the API to the integer V(0), then this will not match. If you are not sure, better include both variants: both the string and the integer. - - Use V(none) for disabled values. - - Either O(restrict[].values) or O(restrict[].regex), but not both, must be specified. type: list elements: raw regex: @@ -124,6 +127,12 @@ class ModuleDocFragment(object): - A regular expression matching values of the field to limit to. - Note that all values will be converted to strings before matching. - It is not possible to match disabled values with regular expressions. - - Either O(restrict[].values) or O(restrict[].regex), but not both, must be specified. + Set O(restrict[].match_disabled=true) if you also want to match disabled values. type: str + invert: + description: + - Invert the condition. This affects O(restrict[].match_disabled), O(restrict[].values), + and O(restrict[].regex). + type: bool + default: false ''' diff --git a/plugins/module_utils/_api_helper.py b/plugins/module_utils/_api_helper.py index 505397fb..ffd897fb 100644 --- a/plugins/module_utils/_api_helper.py +++ b/plugins/module_utils/_api_helper.py @@ -27,10 +27,14 @@ def validate_and_prepare_restrict(module, path_info): if f is None: module.fail_json(msg='restrict: the field "{0}" does not exist for this path'.format(field)) - new_rule = dict(field=field) + new_rule = dict( + field=field, + match_disabled=rule['match_disabled'], + invert=rule['invert'], + ) if rule['values'] is not None: new_rule['values'] = rule['values'] - elif rule['regex'] is not None: + if rule['regex'] is not None: regex = rule['regex'] try: new_rule['regex'] = re.compile(regex) @@ -41,6 +45,25 @@ def validate_and_prepare_restrict(module, path_info): return restrict_data +def _value_to_str(value): + if value is None: + return None + value_str = to_text(value) + if isinstance(value, bool): + value_str = value_str.lower() + return value_str + + +def _test_rule_except_invert(value, rule): + if value is None and rule['match_disabled']: + return True + if 'values' in rule and value in rule['values']: + return True + if 'regex' in rule and value is not None and rule['regex'].match(_value_to_str(value)): + return True + return False + + def restrict_entry_accepted(entry, path_info, restrict_data): if restrict_data is None: return True @@ -54,18 +77,12 @@ def restrict_entry_accepted(entry, path_info, restrict_data): if field not in entry and field_info.absent_value: value = field_info.absent_value - # Actual test - if 'values' in rule and value not in rule['values']: + # Check + matches_rule = _test_rule_except_invert(value, rule) + if rule['invert']: + matches_rule = not matches_rule + if not matches_rule: return False - if 'regex' in rule: - if value is None: - # regex cannot match None - return False - value_str = to_text(value) - if isinstance(value, bool): - value_str = value_str.lower() - if rule['regex'].match(value_str): - return False return True @@ -76,14 +93,10 @@ def restrict_argument_spec(): elements='dict', options=dict( field=dict(type='str', required=True), + match_disabled=dict(type='bool', default=False), values=dict(type='list', elements='raw'), regex=dict(type='str'), + invert=dict(type='bool', default=False), ), - mutually_exclusive=[ - ('values', 'regex'), - ], - required_one_of=[ - ('values', 'regex'), - ], ), ) From 2b2a636ad7e8b7713e4b146bd5d26b91dff1a015 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Mon, 12 Aug 2024 20:05:41 +0200 Subject: [PATCH 8/8] Add more tests. --- .../plugins/module_utils/test__api_helper.py | 377 ++++++++++++++++++ 1 file changed, 377 insertions(+) create mode 100644 tests/unit/plugins/module_utils/test__api_helper.py diff --git a/tests/unit/plugins/module_utils/test__api_helper.py b/tests/unit/plugins/module_utils/test__api_helper.py new file mode 100644 index 00000000..b909f385 --- /dev/null +++ b/tests/unit/plugins/module_utils/test__api_helper.py @@ -0,0 +1,377 @@ +# -*- coding: utf-8 -*- + +# Copyright (c) 2021, Felix Fontein (@felixfontein) +# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt) +# SPDX-License-Identifier: GPL-3.0-or-later + +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +import re +import sys + +import pytest + +from ansible_collections.community.routeros.plugins.module_utils._api_data import ( + PATHS, +) + +from ansible_collections.community.routeros.plugins.module_utils._api_helper import ( + _value_to_str, + _test_rule_except_invert, + validate_and_prepare_restrict, + restrict_entry_accepted, +) + + +VALUE_TO_STR = [ + (None, None), + ('', u''), + ('foo', u'foo'), + (True, u'true'), + (False, u'false'), + ([], u'[]'), + ({}, u'{}'), + (1, u'1'), + (-42, u'-42'), + (1.5, u'1.5'), + (1.0, u'1.0'), +] + + +@pytest.mark.parametrize("value, expected", VALUE_TO_STR) +def test_value_to_str(value, expected): + result = _value_to_str(value) + print(repr(result)) + assert result == expected + + +TEST_RULE_EXCEPT_INVERT = [ + ( + None, + { + 'field': u'foo', + 'match_disabled': False, + 'invert': False, + }, + False, + ), + ( + None, + { + 'field': u'foo', + 'match_disabled': True, + 'invert': False, + }, + True, + ), + ( + 1, + { + 'field': u'foo', + 'match_disabled': False, + 'invert': False, + 'values': [1], + }, + True, + ), + ( + 1, + { + 'field': u'foo', + 'match_disabled': False, + 'invert': False, + 'values': ['1'], + }, + False, + ), + ( + 1, + { + 'field': u'foo', + 'match_disabled': False, + 'invert': False, + 'regex': re.compile(u'^1$'), + 'regex_source': u'^1$', + }, + True, + ), + ( + 1.10, + { + 'field': u'foo', + 'match_disabled': False, + 'invert': False, + 'regex': re.compile(u'^1\\.1$'), + 'regex_source': u'^1\\.1$', + }, + True, + ), + ( + 10, + { + 'field': u'foo', + 'match_disabled': False, + 'invert': False, + 'regex': re.compile(u'^1$'), + 'regex_source': u'^1$', + }, + False, + ), +] + + +@pytest.mark.parametrize("value, rule, expected", TEST_RULE_EXCEPT_INVERT) +def test_rule_except_invert(value, rule, expected): + result = _test_rule_except_invert(value, rule) + print(repr(result)) + assert result == expected + + +_test_path = PATHS[('ip', 'firewall', 'filter')] +_test_path.provide_version('7.0') +TEST_PATH = _test_path.get_data() + + +class FailJsonExc(Exception): + def __init__(self, msg, kwargs): + self.msg = msg + self.kwargs = kwargs + + +class FakeModule(object): + def __init__(self, restrict_value): + self.params = { + 'restrict': restrict_value, + } + + def fail_json(self, msg, **kwargs): + raise FailJsonExc(msg, kwargs) + + +TEST_VALIDATE_AND_PREPARE_RESTRICT = [ + ( + [{ + 'field': u'chain', + 'match_disabled': False, + 'values': None, + 'regex': None, + 'invert': False, + }], + [{ + 'field': u'chain', + 'match_disabled': False, + 'invert': False, + }], + ), + ( + [{ + 'field': u'comment', + 'match_disabled': True, + 'values': None, + 'regex': None, + 'invert': False, + }], + [{ + 'field': u'comment', + 'match_disabled': True, + 'invert': False, + }], + ), + ( + [{ + 'field': u'comment', + 'match_disabled': False, + 'values': None, + 'regex': None, + 'invert': True, + }], + [{ + 'field': u'comment', + 'match_disabled': False, + 'invert': True, + }], + ), +] + +if sys.version_info >= (2, 7, 17): + # Somewhere between Python 2.7.15 (used by Ansible 3.9) and 2.7.17 (used by ansible-base 2.10) + # something changed with ``==`` for ``re.Pattern``, at least for some patterns + # (my guess is: for ``re.compile(u'')``) + TEST_VALIDATE_AND_PREPARE_RESTRICT.extend([ + ( + [ + { + 'field': u'comment', + 'match_disabled': False, + 'values': [], + 'regex': None, + 'invert': False, + }, + { + 'field': u'comment', + 'match_disabled': False, + 'values': [None, 1, 42.0, True, u'foo', [], {}], + 'regex': None, + 'invert': False, + }, + { + 'field': u'chain', + 'match_disabled': False, + 'values': None, + 'regex': u'', + 'invert': True, + }, + { + 'field': u'chain', + 'match_disabled': False, + 'values': None, + 'regex': u'foo', + 'invert': False, + }, + ], + [ + { + 'field': u'comment', + 'match_disabled': False, + 'invert': False, + 'values': [], + }, + { + 'field': u'comment', + 'match_disabled': False, + 'invert': False, + 'values': [None, 1, 42.0, True, u'foo', [], {}], + }, + { + 'field': u'chain', + 'match_disabled': False, + 'invert': True, + 'regex': re.compile(u''), + 'regex_source': u'', + }, + { + 'field': u'chain', + 'match_disabled': False, + 'invert': False, + 'regex': re.compile(u'foo'), + 'regex_source': u'foo', + }, + ], + ), + ]) + + +@pytest.mark.parametrize("restrict_value, expected", TEST_VALIDATE_AND_PREPARE_RESTRICT) +def test_validate_and_prepare_restrict(restrict_value, expected): + fake_module = FakeModule(restrict_value) + result = validate_and_prepare_restrict(fake_module, TEST_PATH) + print(repr(result)) + assert result == expected + + +TEST_VALIDATE_AND_PREPARE_RESTRICT_FAIL = [ + ( + [{ + 'field': u'!foo', + 'match_disabled': False, + 'values': None, + 'regex': None, + 'invert': False, + }], + ['restrict: the field name "!foo" must not start with "!"'], + ), + ( + [{ + 'field': u'foo', + 'match_disabled': False, + 'values': None, + 'regex': None, + 'invert': False, + }], + ['restrict: the field "foo" does not exist for this path'], + ), + ( + [{ + 'field': u'chain', + 'match_disabled': False, + 'values': None, + 'regex': u'(', + 'invert': False, + }], + [ + 'restrict: invalid regular expression "(": missing ), unterminated subpattern at position 0', + 'restrict: invalid regular expression "(": unbalanced parenthesis', + ] + ), +] + + +@pytest.mark.parametrize("restrict_value, expected", TEST_VALIDATE_AND_PREPARE_RESTRICT_FAIL) +def test_validate_and_prepare_restrict_fail(restrict_value, expected): + fake_module = FakeModule(restrict_value) + with pytest.raises(FailJsonExc) as exc: + validate_and_prepare_restrict(fake_module, TEST_PATH) + print(repr(exc.value.msg)) + assert exc.value.msg in expected + + +TEST_RESTRICT_ENTRY_ACCEPTED = [ + ( + { + 'chain': 'input', + }, + [ + { + 'field': u'chain', + 'match_disabled': False, + 'invert': False, + }, + ], + False, + ), + ( + { + 'chain': 'input', + }, + [ + { + 'field': u'chain', + 'match_disabled': False, + 'invert': True, + }, + ], + True, + ), + ( + { + 'comment': 'foo', + }, + [ + { + 'field': u'comment', + 'match_disabled': True, + 'invert': False, + }, + ], + False, + ), + ( + {}, + [ + { + 'field': u'comment', + 'match_disabled': True, + 'invert': False, + }, + ], + True, + ), +] + + +@pytest.mark.parametrize("entry, restrict_data, expected", TEST_RESTRICT_ENTRY_ACCEPTED) +def test_restrict_entry_accepted(entry, restrict_data, expected): + result = restrict_entry_accepted(entry, TEST_PATH, restrict_data) + print(repr(result)) + assert result == expected