From 1a684df109c8278eacf0b4a4267c51d652a1fab2 Mon Sep 17 00:00:00 2001 From: Ganesh Nalawade Date: Thu, 30 Aug 2018 21:39:11 +0530 Subject: [PATCH] Fix iosxr cli_config module diff issue (#44898) * Fix iosxr cli_config module diff issue * Modify iosxr plugin to support configuration diff capability (get_diff()) within Ansible to be in sync with iosxr_config module. * Fix unit test case failure --- .../module_utils/network/iosxr/iosxr.py | 100 ++++++++++++ .../modules/network/iosxr/iosxr_config.py | 145 +++--------------- lib/ansible/plugins/cliconf/iosxr.py | 54 +++++-- .../network/iosxr/test_iosxr_config.py | 65 +++++++- 4 files changed, 220 insertions(+), 144 deletions(-) diff --git a/lib/ansible/module_utils/network/iosxr/iosxr.py b/lib/ansible/module_utils/network/iosxr/iosxr.py index d70c87fc30..23575ce79d 100644 --- a/lib/ansible/module_utils/network/iosxr/iosxr.py +++ b/lib/ansible/module_utils/network/iosxr/iosxr.py @@ -101,6 +101,41 @@ iosxr_top_spec = { } iosxr_argument_spec.update(iosxr_top_spec) +CONFIG_MISPLACED_CHILDREN = [ + re.compile(r'^end-\s*(.+)$') +] + +# Objects defined in Route-policy Language guide of IOS_XR. +# Reconfiguring these objects replace existing configurations. +# Hence these objects should be played direcly from candidate +# configurations +CONFIG_BLOCKS_FORCED_IN_DIFF = [ + { + 'start': re.compile(r'route-policy'), + 'end': re.compile(r'end-policy') + }, + { + 'start': re.compile(r'prefix-set'), + 'end': re.compile(r'end-set') + }, + { + 'start': re.compile(r'as-path-set'), + 'end': re.compile(r'end-set') + }, + { + 'start': re.compile(r'community-set'), + 'end': re.compile(r'end-set') + }, + { + 'start': re.compile(r'rd-set'), + 'end': re.compile(r'end-set') + }, + { + 'start': re.compile(r'extcommunity-set'), + 'end': re.compile(r'end-set') + } +] + def get_provider_argspec(): return iosxr_provider_spec @@ -458,3 +493,68 @@ def get_file(module, src, dst, proto='scp'): conn.get_file(source=src, destination=dst, proto=proto) except ConnectionError as exc: module.fail_json(msg=to_text(exc, errors='surrogate_then_replace')) + + +# A list of commands like {end-set, end-policy, ...} are part of configuration +# block like { prefix-set, as-path-set , ... } but they are not indented properly +# to be included with their parent. sanitize_config will add indentation to +# end-* commands so they are included with their parents +def sanitize_config(config, force_diff_prefix=None): + conf_lines = config.split('\n') + for regex in CONFIG_MISPLACED_CHILDREN: + for index, line in enumerate(conf_lines): + m = regex.search(line) + if m and m.group(0): + if force_diff_prefix: + conf_lines[index] = ' ' + m.group(0) + force_diff_prefix + else: + conf_lines[index] = ' ' + m.group(0) + conf = ('\n').join(conf_lines) + return conf + + +def mask_config_blocks_from_diff(config, candidate, force_diff_prefix): + conf_lines = config.split('\n') + candidate_lines = candidate.split('\n') + + for regex in CONFIG_BLOCKS_FORCED_IN_DIFF: + block_index_start_end = [] + for index, line in enumerate(candidate_lines): + startre = regex['start'].search(line) + if startre and startre.group(0): + start_index = index + else: + endre = regex['end'].search(line) + if endre and endre.group(0): + end_index = index + new_block = True + for prev_start, prev_end in block_index_start_end: + if start_index == prev_start: + # This might be end-set of another regex + # otherwise we would be having new start + new_block = False + break + if new_block: + block_index_start_end.append((start_index, end_index)) + + for start, end in block_index_start_end: + diff = False + if candidate_lines[start] in conf_lines: + run_conf_start_index = conf_lines.index(candidate_lines[start]) + else: + diff = False + continue + for i in range(start, end + 1): + if conf_lines[run_conf_start_index] == candidate_lines[i]: + run_conf_start_index = run_conf_start_index + 1 + else: + diff = True + break + if diff: + run_conf_start_index = conf_lines.index(candidate_lines[start]) + for i in range(start, end + 1): + conf_lines[run_conf_start_index] = conf_lines[run_conf_start_index] + force_diff_prefix + run_conf_start_index = run_conf_start_index + 1 + + conf = ('\n').join(conf_lines) + return conf diff --git a/lib/ansible/modules/network/iosxr/iosxr_config.py b/lib/ansible/modules/network/iosxr/iosxr_config.py index abba2986d6..bd8ec2268c 100644 --- a/lib/ansible/modules/network/iosxr/iosxr_config.py +++ b/lib/ansible/modules/network/iosxr/iosxr_config.py @@ -186,48 +186,15 @@ backup_path: """ import re +from ansible.module_utils._text import to_text from ansible.module_utils.basic import AnsibleModule -from ansible.module_utils.network.iosxr.iosxr import load_config, get_config +from ansible.module_utils.connection import ConnectionError +from ansible.module_utils.network.iosxr.iosxr import load_config, get_config, get_connection from ansible.module_utils.network.iosxr.iosxr import iosxr_argument_spec, copy_file from ansible.module_utils.network.common.config import NetworkConfig, dumps DEFAULT_COMMIT_COMMENT = 'configured by iosxr_config' -CONFIG_MISPLACED_CHILDREN = [ - re.compile(r'^end-\s*(.+)$') -] - -# Objects defined in Route-policy Language guide of IOS_XR. -# Reconfiguring these objects replace existing configurations. -# Hence these objects should be played direcly from candidate -# configurations -CONFIG_BLOCKS_FORCED_IN_DIFF = [ - { - 'start': re.compile(r'route-policy'), - 'end': re.compile(r'end-policy') - }, - { - 'start': re.compile(r'prefix-set'), - 'end': re.compile(r'end-set') - }, - { - 'start': re.compile(r'as-path-set'), - 'end': re.compile(r'end-set') - }, - { - 'start': re.compile(r'community-set'), - 'end': re.compile(r'end-set') - }, - { - 'start': re.compile(r'rd-set'), - 'end': re.compile(r'end-set') - }, - { - 'start': re.compile(r'extcommunity-set'), - 'end': re.compile(r'end-set') - } -] - def copy_file_to_node(module): """ Copy config file to IOS-XR node. We use SFTP because older IOS-XR versions don't handle SCP very well. @@ -265,90 +232,22 @@ def check_args(module, warnings): 'removed in the future') -# A list of commands like {end-set, end-policy, ...} are part of configuration -# block like { prefix-set, as-path-set , ... } but they are not indented properly -# to be included with their parent. sanitize_config will add indentation to -# end-* commands so they are included with their parents -def sanitize_config(config, force_diff_prefix=None): - conf_lines = config.split('\n') - for regex in CONFIG_MISPLACED_CHILDREN: - for index, line in enumerate(conf_lines): - m = regex.search(line) - if m and m.group(0): - if force_diff_prefix: - conf_lines[index] = ' ' + m.group(0) + force_diff_prefix - else: - conf_lines[index] = ' ' + m.group(0) - conf = ('\n').join(conf_lines) - return conf - - -def mask_config_blocks_from_diff(config, candidate, force_diff_prefix): - conf_lines = config.split('\n') - candidate_lines = candidate.split('\n') - - for regex in CONFIG_BLOCKS_FORCED_IN_DIFF: - block_index_start_end = [] - for index, line in enumerate(candidate_lines): - startre = regex['start'].search(line) - if startre and startre.group(0): - start_index = index - else: - endre = regex['end'].search(line) - if endre and endre.group(0): - end_index = index - new_block = True - for prev_start, prev_end in block_index_start_end: - if start_index == prev_start: - # This might be end-set of another regex - # otherwise we would be having new start - new_block = False - break - if new_block: - block_index_start_end.append((start_index, end_index)) - - for start, end in block_index_start_end: - diff = False - if candidate_lines[start] in conf_lines: - run_conf_start_index = conf_lines.index(candidate_lines[start]) - else: - diff = False - continue - for i in range(start, end + 1): - if conf_lines[run_conf_start_index] == candidate_lines[i]: - run_conf_start_index = run_conf_start_index + 1 - else: - diff = True - break - if diff: - run_conf_start_index = conf_lines.index(candidate_lines[start]) - for i in range(start, end + 1): - conf_lines[run_conf_start_index] = conf_lines[run_conf_start_index] + force_diff_prefix - run_conf_start_index = run_conf_start_index + 1 - - conf = ('\n').join(conf_lines) - return conf - - def get_running_config(module): contents = module.params['config'] if not contents: contents = get_config(module) - if module.params['src']: - contents = mask_config_blocks_from_diff(contents, module.params['src'], "ansible") - contents = sanitize_config(contents) - return NetworkConfig(indent=1, contents=contents) + return contents def get_candidate(module): - candidate = NetworkConfig(indent=1) + candidate = '' if module.params['src']: - config = module.params['src'] - config = sanitize_config(config) - candidate.load(config) + candidate = module.params['src'] elif module.params['lines']: + candidate_obj = NetworkConfig(indent=1) parents = module.params['parents'] or list() - candidate.add(module.params['lines'], parents=parents) + candidate_obj.add(module.params['lines'], parents=parents) + candidate = dumps(candidate_obj, 'raw') return candidate @@ -367,28 +266,27 @@ def run(module, result): commands = None replace_file_path = None + connection = get_connection(module) + try: + response = connection.get_diff(candidate=candidate_config, running=running_config, diff_match=match, path=path, diff_replace=replace) + except ConnectionError as exc: + module.fail_json(msg=to_text(exc, errors='surrogate_then_replace')) - if match != 'none' and replace != 'config': - commands = candidate_config.difference(running_config, path=path, match=match, replace=replace) - elif replace_config: - can_config = candidate_config.difference(running_config, path=path, match=match, replace=replace) - candidate = dumps(can_config, 'commands').split('\n') - run_config = running_config.difference(candidate_config, path=path, match=match, replace=replace) - running = dumps(run_config, 'commands').split('\n') + config_diff = response.get('config_diff') - if len(candidate) > 1 or len(running) > 1: + if replace_config: + running_base_diff_resp = connection.get_diff(candidate=running_config, running=candidate_config, diff_match=match, path=path, diff_replace=replace) + if config_diff or running_base_diff_resp['config_diff']: ret = copy_file_to_node(module) if not ret: module.fail_json(msg='Copy of config file to the node failed') commands = ['load harddisk:/ansible_config.txt'] replace_file_path = 'harddisk:/ansible_config.txt' - else: - commands = candidate_config.items - if commands: + if config_diff: if not replace_config: - commands = dumps(commands, 'commands').split('\n') + commands = config_diff.split('\n') if any((module.params['lines'], module.params['src'])): if module.params['before']: @@ -463,7 +361,8 @@ def main(): if module.params['backup']: result['__backup__'] = get_config(module) - run(module, result) + if any((module.params['src'], module.params['lines'])): + run(module, result) module.exit_json(**result) diff --git a/lib/ansible/plugins/cliconf/iosxr.py b/lib/ansible/plugins/cliconf/iosxr.py index b8826aa83b..aec9e15b18 100644 --- a/lib/ansible/plugins/cliconf/iosxr.py +++ b/lib/ansible/plugins/cliconf/iosxr.py @@ -26,7 +26,9 @@ import json from ansible.errors import AnsibleConnectionFailure from ansible.module_utils._text import to_text from ansible.module_utils.connection import ConnectionError +from ansible.module_utils.network.common.config import NetworkConfig, dumps from ansible.module_utils.network.common.utils import to_list +from ansible.module_utils.network.iosxr.iosxr import sanitize_config, mask_config_blocks_from_diff from ansible.plugins.cliconf import CliconfBase @@ -114,15 +116,37 @@ class Cliconf(CliconfBase): resp['response'] = results return resp - def get_diff(self, admin=False): - self.configure(admin=admin) + def get_diff(self, candidate=None, running=None, diff_match='line', diff_ignore_lines=None, path=None, diff_replace='line'): + diff = {} + device_operations = self.get_device_operations() + option_values = self.get_option_values() - diff = {'config_diff': None} - response = self.send_command('show commit changes diff') - for item in response.splitlines(): - if item and item[0] in ['<', '+', '-']: - diff['config_diff'] = response - break + if candidate is None and device_operations['supports_generate_diff']: + raise ValueError("candidate configuration is required to generate diff") + + if diff_match not in option_values['diff_match']: + raise ValueError("'match' value %s in invalid, valid values are %s" % (diff_match, ', '.join(option_values['diff_match']))) + + if diff_replace not in option_values['diff_replace']: + raise ValueError("'replace' value %s in invalid, valid values are %s" % (diff_replace, ', '.join(option_values['diff_replace']))) + + # prepare candidate configuration + sanitized_candidate = sanitize_config(candidate) + candidate_obj = NetworkConfig(indent=1, ignore_lines=diff_ignore_lines) + candidate_obj.load(sanitized_candidate) + + if running and diff_match != 'none': + # running configuration + running = mask_config_blocks_from_diff(running, candidate, "ansible") + running = sanitize_config(running) + + running_obj = NetworkConfig(indent=1, contents=running, ignore_lines=diff_ignore_lines) + configdiffobjs = candidate_obj.difference(running_obj, path=path, match=diff_match, replace=diff_replace) + + else: + configdiffobjs = candidate_obj.items + + diff['config_diff'] = dumps(configdiffobjs, 'commands') if configdiffobjs else '' return diff def get(self, command=None, prompt=None, answer=None, sendonly=False, newline=True, output=None): @@ -186,16 +210,16 @@ class Cliconf(CliconfBase): def get_device_operations(self): return { - 'supports_diff_replace': False, + 'supports_diff_replace': True, 'supports_commit': True, 'supports_rollback': False, 'supports_defaults': False, - 'supports_onbox_diff': True, + 'supports_onbox_diff': False, 'supports_commit_comment': True, 'supports_multiline_delimiter': False, - 'supports_diff_match': False, - 'supports_diff_ignore_lines': False, - 'supports_generate_diff': False, + 'supports_diff_match': True, + 'supports_diff_ignore_lines': True, + 'supports_generate_diff': True, 'supports_replace': True, 'supports_admin': True, 'supports_commit_label': True @@ -204,8 +228,8 @@ class Cliconf(CliconfBase): def get_option_values(self): return { 'format': ['text'], - 'diff_match': [], - 'diff_replace': [], + 'diff_match': ['line', 'strict', 'exact', 'none'], + 'diff_replace': ['line', 'block', 'config'], 'output': [] } diff --git a/test/units/modules/network/iosxr/test_iosxr_config.py b/test/units/modules/network/iosxr/test_iosxr_config.py index b4c4d0bad2..7d28e4a2af 100644 --- a/test/units/modules/network/iosxr/test_iosxr_config.py +++ b/test/units/modules/network/iosxr/test_iosxr_config.py @@ -20,8 +20,9 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type -from ansible.compat.tests.mock import patch +from ansible.compat.tests.mock import patch, MagicMock from ansible.modules.network.iosxr import iosxr_config +from ansible.plugins.cliconf.iosxr import Cliconf from units.modules.utils import set_module_args from .iosxr_module import TestIosxrModule, load_fixture @@ -35,14 +36,25 @@ class TestIosxrConfigModule(TestIosxrModule): self.patcher_get_config = patch('ansible.modules.network.iosxr.iosxr_config.get_config') self.mock_get_config = self.patcher_get_config.start() + self.patcher_exec_command = patch('ansible.modules.network.iosxr.iosxr_config.load_config') self.mock_exec_command = self.patcher_exec_command.start() + self.mock_get_connection = patch('ansible.modules.network.iosxr.iosxr_config.get_connection') + self.get_connection = self.mock_get_connection.start() + + self.conn = self.get_connection() + self.conn.edit_config = MagicMock() + + self.cliconf_obj = Cliconf(MagicMock()) + self.running_config = load_fixture('iosxr_config_config.cfg') + def tearDown(self): super(TestIosxrConfigModule, self).tearDown() self.patcher_get_config.stop() self.patcher_exec_command.stop() + self.mock_get_connection.stop() def load_fixtures(self, commands=None): config_file = 'iosxr_config_config.cfg' @@ -52,11 +64,13 @@ class TestIosxrConfigModule(TestIosxrModule): def test_iosxr_config_unchanged(self): src = load_fixture('iosxr_config_config.cfg') set_module_args(dict(src=src)) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff(src, src)) self.execute_module() def test_iosxr_config_src(self): src = load_fixture('iosxr_config_src.cfg') set_module_args(dict(src=src)) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff(src, self.running_config)) commands = ['hostname foo', 'interface GigabitEthernet0/0', 'no ip address'] self.execute_module(changed=True, commands=commands) @@ -67,34 +81,51 @@ class TestIosxrConfigModule(TestIosxrModule): self.assertIn('__backup__', result) def test_iosxr_config_lines_wo_parents(self): - set_module_args(dict(lines=['hostname foo'])) + lines = ['hostname foo'] + set_module_args(dict(lines=lines)) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff('\n'.join(lines), self.running_config)) commands = ['hostname foo'] self.execute_module(changed=True, commands=commands) def test_iosxr_config_lines_w_parents(self): - set_module_args(dict(lines=['shutdown'], parents=['interface GigabitEthernet0/0'])) + lines = ['shutdown'] + parents = ['interface GigabitEthernet0/0'] + candidate = parents + lines + set_module_args(dict(lines=lines, parents=parents)) + module = MagicMock() + module.params = {'lines': lines, 'parents': parents, 'src': None} + candidate_config = iosxr_config.get_candidate(module) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff(candidate_config, self.running_config)) commands = ['interface GigabitEthernet0/0', 'shutdown'] self.execute_module(changed=True, commands=commands) def test_iosxr_config_before(self): - set_module_args(dict(lines=['hostname foo'], before=['test1', 'test2'])) + lines = ['hostname foo'] + set_module_args(dict(lines=lines, before=['test1', 'test2'])) commands = ['test1', 'test2', 'hostname foo'] + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff('\n'.join(lines), self.running_config)) self.execute_module(changed=True, commands=commands, sort=False) def test_iosxr_config_after(self): - set_module_args(dict(lines=['hostname foo'], after=['test1', 'test2'])) + lines = ['hostname foo'] + set_module_args(dict(lines=lines, after=['test1', 'test2'])) commands = ['hostname foo', 'test1', 'test2'] + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff('\n'.join(lines), self.running_config)) self.execute_module(changed=True, commands=commands, sort=False) def test_iosxr_config_before_after_no_change(self): - set_module_args(dict(lines=['hostname router'], + lines = ['hostname router'] + set_module_args(dict(lines=lines, before=['test1', 'test2'], after=['test3', 'test4'])) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff('\n'.join(lines), self.running_config)) self.execute_module() def test_iosxr_config_config(self): config = 'hostname localhost' + lines = ['hostname router'] set_module_args(dict(lines=['hostname router'], config=config)) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff('\n'.join(lines), config)) commands = ['hostname router'] self.execute_module(changed=True, commands=commands) @@ -103,16 +134,23 @@ class TestIosxrConfigModule(TestIosxrModule): parents = ['interface GigabitEthernet0/0'] set_module_args(dict(lines=lines, replace='block', parents=parents)) commands = parents + lines + + module = MagicMock() + module.params = {'lines': lines, 'parents': parents, 'src': None} + candidate_config = iosxr_config.get_candidate(module) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff(candidate_config, self.running_config, diff_replace='block', path=parents)) self.execute_module(changed=True, commands=commands) def test_iosxr_config_force(self): lines = ['hostname router'] set_module_args(dict(lines=lines, force=True)) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff('\n'.join(lines), self.running_config, diff_match='none')) self.execute_module(changed=True, commands=lines) def test_iosxr_config_admin(self): lines = ['username admin', 'group root-system', 'secret P@ssw0rd'] set_module_args(dict(lines=lines, admin=True)) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff('\n'.join(lines), self.running_config)) self.execute_module(changed=True, commands=lines) def test_iosxr_config_match_none(self): @@ -120,6 +158,11 @@ class TestIosxrConfigModule(TestIosxrModule): parents = ['interface GigabitEthernet0/0'] set_module_args(dict(lines=lines, parents=parents, match='none')) commands = parents + lines + module = MagicMock() + module.params = {'lines': lines, 'parents': parents, 'src': None} + candidate_config = iosxr_config.get_candidate(module) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff(candidate_config, self.running_config, diff_match='none', path=parents)) + self.execute_module(changed=True, commands=commands, sort=False) def test_iosxr_config_match_strict(self): @@ -128,6 +171,11 @@ class TestIosxrConfigModule(TestIosxrModule): parents = ['interface GigabitEthernet0/0'] set_module_args(dict(lines=lines, parents=parents, match='strict')) commands = parents + ['shutdown'] + module = MagicMock() + module.params = {'lines': lines, 'parents': parents, 'src': None} + candidate_config = iosxr_config.get_candidate(module) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff(candidate_config, self.running_config, diff_match='strict', path=parents)) + self.execute_module(changed=True, commands=commands, sort=False) def test_iosxr_config_match_exact(self): @@ -136,6 +184,11 @@ class TestIosxrConfigModule(TestIosxrModule): parents = ['interface GigabitEthernet0/0'] set_module_args(dict(lines=lines, parents=parents, match='exact')) commands = parents + lines + module = MagicMock() + module.params = {'lines': lines, 'parents': parents, 'src': None} + candidate_config = iosxr_config.get_candidate(module) + self.conn.get_diff = MagicMock(return_value=self.cliconf_obj.get_diff(candidate_config, self.running_config, diff_match='exact', path=parents)) + self.execute_module(changed=True, commands=commands, sort=False) def test_iosxr_config_src_and_lines_fails(self):