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

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

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)
 

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': []
         }
 

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):