nxos_bgp updates (#24270)

* Remove invoke from nxos_bgp
* Try to simplify get_existing
* Streamline state_present
* More testing, squash & compact get_value & get_custom_value
* Streamline main
* nxapi has problems reading CustomNetworkConfig directly

lib/ansible/module_utils/netcfg.py

@@ -155,6 +155,9 @@ class NetworkConfig(object):
     def __str__(self):
         return '\n'.join([c.raw for c in self.items])
 
+    def __len__(self):
+        return len(self._items)
+
     def load(self, s):
         self._items = self.parse(s)
 
@@ -368,6 +371,9 @@ class NetworkConfig(object):
 
 class CustomNetworkConfig(NetworkConfig):
 
+    def items_text(self):
+        return [item.text for item in self.items]
+
     def expand_section(self, configobj, S=None):
         if S is None:
             S = list()

lib/ansible/modules/network/nxos/nxos_bgp.py

@@ -308,6 +308,7 @@ commands:
 '''
 
 import re
+
 from ansible.module_utils.nxos import get_config, load_config
 from ansible.module_utils.nxos import nxos_argument_spec, check_args
 from ansible.module_utils.basic import AnsibleModule
@@ -406,88 +407,54 @@ PARAM_TO_COMMAND_KEYMAP = {
 }
 
 
-def invoke(name, *args, **kwargs):
-    func = globals().get(name)
-    if func:
-        return func(*args, **kwargs)
+def get_value(arg, config):
+    command = PARAM_TO_COMMAND_KEYMAP.get(arg)
 
+    if command.split()[0] == 'event-history':
+        command_re = re.compile(r'\s+{0}\s*'.format(command), re.M)
 
-def get_custom_value(config, arg):
-    if arg.startswith('event_history'):
-        REGEX_SIZE = re.compile(r'(?:{0} size\s)(?P<value>.*)$'.format(PARAM_TO_COMMAND_KEYMAP[arg]), re.M)
-        REGEX = re.compile(r'\s+{0}\s*$'.format(PARAM_TO_COMMAND_KEYMAP[arg]), re.M)
+        size_re = re.compile(r'(?:{0} size\s)(?P<value>.*)'.format(command), re.M)
         value = False
 
-        if 'no {0}'.format(PARAM_TO_COMMAND_KEYMAP[arg]) in config:
-            pass
-        elif PARAM_TO_COMMAND_KEYMAP[arg] in config:
-            try:
-                value = REGEX_SIZE.search(config).group('value')
-            except AttributeError:
-                if REGEX.search(config):
-                    value = True
-
-    elif arg == 'enforce_first_as' or arg == 'fast_external_fallover':
-        REGEX = re.compile(r'no\s+{0}\s*$'.format(PARAM_TO_COMMAND_KEYMAP[arg]), re.M)
-        value = True
-        try:
-            if REGEX.search(config):
-                value = False
-        except TypeError:
-            value = True
-
-    elif arg == 'confederation_peers':
-        REGEX = re.compile(r'(?:confederation peers\s)(?P<value>.*)$', re.M)
-        value = ''
-        if 'confederation peers' in config:
-            value = REGEX.search(config).group('value').split()
-
-    elif arg == 'timer_bgp_keepalive':
-        REGEX = re.compile(r'(?:timers bgp\s)(?P<value>.*)$', re.M)
-        value = ''
-        if 'timers bgp' in config:
-            parsed = REGEX.search(config).group('value').split()
-            value = parsed[0]
-
-    elif arg == 'timer_bgp_hold':
-        REGEX = re.compile(r'(?:timers bgp\s)(?P<value>.*)$', re.M)
-        value = ''
-        if 'timers bgp' in config:
-            parsed = REGEX.search(config).group('value').split()
-            if len(parsed) == 2:
-                value = parsed[1]
+        if command_re.search(config):
+            search = size_re.search(config)
+            if search:
+                value = search.group('value')
+            else:
+                value = True
 
-    return value
+    elif arg in ['enforce_first_as', 'fast_external_fallover']:
+        no_command_re = re.compile(r'no\s+{0}\s*'.format(command), re.M)
+        value = True
 
+        if no_command_re.search(config):
+            value = False
 
-def get_value(arg, config):
-    custom = [
-        'event_history_cli',
-        'event_history_events',
-        'event_history_periodic',
-        'event_history_detail',
-        'confederation_peers',
-        'timer_bgp_hold',
-        'timer_bgp_keepalive',
-        'enforce_first_as',
-        'fast_external_fallover'
-    ]
-
-    if arg in custom:
-        value = get_custom_value(config, arg)
     elif arg in BOOL_PARAMS:
-        REGEX = re.compile(r'\s+{0}\s*$'.format(PARAM_TO_COMMAND_KEYMAP[arg]), re.M)
+        command_re = re.compile(r'\s+{0}\s*'.format(command), re.M)
         value = False
-        try:
-            if REGEX.search(config):
-                value = True
-        except TypeError:
-            value = False
+
+        if command_re.search(config):
+            value = True
     else:
-        REGEX = re.compile(r'(?:{0}\s)(?P<value>.*)$'.format(PARAM_TO_COMMAND_KEYMAP[arg]), re.M)
+        command_val_re = re.compile(r'(?:{0}\s)(?P<value>.*)'.format(command), re.M)
         value = ''
-        if PARAM_TO_COMMAND_KEYMAP[arg] in config:
-            value = REGEX.search(config).group('value')
+
+        has_command = command_val_re.search(config)
+        if has_command:
+            found_value = has_command.group('value')
+
+            if arg == 'confederation_peers':
+                value = found_value.split()
+            elif arg == 'timer_bgp_keepalive':
+                value = found_value.split()[0]
+            elif arg == 'timer_bgp_hold':
+                split_values = found_value.split()
+                if len(split_values) == 2:
+                    value = split_values[1]
+            elif found_value:
+                value = found_value
+
     return value
 
 
@@ -495,16 +462,13 @@ def get_existing(module, args, warnings):
     existing = {}
     netcfg = CustomNetworkConfig(indent=2, contents=get_config(module))
 
-    try:
-        asn_regex = r'.*router\sbgp\s(?P<existing_asn>\d+).*'
-        match_asn = re.match(asn_regex, str(netcfg), re.DOTALL)
-        existing_asn_group = match_asn.groupdict()
-        existing_asn = existing_asn_group['existing_asn']
-    except AttributeError:
-        existing_asn = ''
+    asn_re = re.compile(r'.*router\sbgp\s(?P<existing_asn>\d+).*', re.S)
+    asn_match = asn_re.match(str(netcfg))
 
-    if existing_asn:
+    if asn_match:
+        existing_asn = asn_match.group('existing_asn')
         bgp_parent = 'router bgp {0}'.format(existing_asn)
+
         if module.params['vrf'] != 'default':
             parents = [bgp_parent, 'vrf {0}'.format(module.params['vrf'])]
         else:
@@ -513,40 +477,28 @@ def get_existing(module, args, warnings):
         config = netcfg.get_section(parents)
         if config:
             for arg in args:
-                if arg != 'asn':
-                    if module.params['vrf'] != 'default':
-                        if arg not in GLOBAL_PARAMS:
-                            existing[arg] = get_value(arg, config)
-                    else:
-                        existing[arg] = get_value(arg, config)
+                if arg != 'asn' and (module.params['vrf'] == 'default' or
+                                     arg not in GLOBAL_PARAMS):
+                    existing[arg] = get_value(arg, config)
 
             existing['asn'] = existing_asn
             if module.params['vrf'] == 'default':
                 existing['vrf'] = 'default'
-        else:
-            if (module.params['state'] == 'present' and
-                    module.params['vrf'] != 'default'):
-                msg = ("VRF {0} doesn't exist. ".format(module.params['vrf']))
-                warnings.append(msg)
-    else:
-        if (module.params['state'] == 'present' and
-                module.params['vrf'] != 'default'):
-            msg = ("VRF {0} doesn't exist. ".format(module.params['vrf']))
-            warnings.append(msg)
+
+    if not existing and module.params['vrf'] != 'default' and module.params['state'] == 'present':
+        msg = ("VRF {0} doesn't exist.".format(module.params['vrf']))
+        warnings.append(msg)
 
     return existing
 
 
 def apply_key_map(key_map, table):
     new_dict = {}
-    for key, value in table.items():
+    for key in table:
         new_key = key_map.get(key)
         if new_key:
-            value = table.get(key)
-            if value:
-                new_dict[new_key] = value
-            else:
-                new_dict[new_key] = value
+            new_dict[new_key] = table.get(key)
+
     return new_dict
 
 
@@ -561,67 +513,56 @@ def state_present(module, existing, proposed, candidate):
         elif value is False:
             commands.append('no {0}'.format(key))
         elif value == 'default':
-            if key in PARAM_TO_DEFAULT_KEYMAP:
-                commands.append('{0} {1}'.format(key, PARAM_TO_DEFAULT_KEYMAP[key]))
-            elif existing_commands.get(key):
-                existing_value = existing_commands.get(key)
+            default_value = PARAM_TO_DEFAULT_KEYMAP.get(key)
+            existing_value = existing_commands.get(key)
+
+            if default_value:
+                commands.append('{0} {1}'.format(key, default_value))
+            elif existing_value:
                 if key == 'confederation peers':
-                    commands.append('no {0} {1}'.format(key, ' '.join(existing_value)))
-                else:
-                    commands.append('no {0} {1}'.format(key, existing_value))
-        else:
-            if key == 'confederation peers':
-                existing_confederation_peers = existing.get('confederation_peers')
-
-                if existing_confederation_peers:
-                    if not isinstance(existing_confederation_peers, list):
-                        existing_confederation_peers = [existing_confederation_peers]
-                else:
-                    existing_confederation_peers = []
-
-                values = value.split()
-                for each_value in values:
-                    if each_value not in existing_confederation_peers:
-                        existing_confederation_peers.append(each_value)
-                peer_string = ' '.join(existing_confederation_peers)
-                commands.append('{0} {1}'.format(key, peer_string))
-            elif key.startswith('timers bgp'):
-                command = 'timers bgp {0} {1}'.format(
-                    proposed['timer_bgp_keepalive'],
-                    proposed['timer_bgp_hold'])
-                if command not in commands:
-                    commands.append(command)
-            else:
-                if value.startswith('size'):
-                    value = value.replace('_', ' ')
-                command = '{0} {1}'.format(key, value)
+                    existing_value = ' '.join(existing_value)
+                commands.append('no {0} {1}'.format(key, existing_value))
+        elif key == 'confederation peers':
+            existing_confederation_peers = set(existing.get('confederation_peers', []))
+            new_values = set(value.split())
+            peer_string = ' '.join(existing_confederation_peers | new_values)
+            commands.append('{0} {1}'.format(key, peer_string))
+        elif key.startswith('timers bgp'):
+            command = 'timers bgp {0} {1}'.format(
+                proposed['timer_bgp_keepalive'],
+                proposed['timer_bgp_hold'])
+            if command not in commands:
                 commands.append(command)
+        else:
+            if value.startswith('size'):
+                value = value.replace('_', ' ')
+            command = '{0} {1}'.format(key, value)
+            commands.append(command)
 
+    parents = []
     if commands:
         commands = fix_commands(commands)
         parents = ['router bgp {0}'.format(module.params['asn'])]
         if module.params['vrf'] != 'default':
             parents.append('vrf {0}'.format(module.params['vrf']))
-        candidate.add(commands, parents=parents)
     elif proposed:
         if module.params['vrf'] != 'default':
             commands.append('vrf {0}'.format(module.params['vrf']))
             parents = ['router bgp {0}'.format(module.params['asn'])]
         else:
             commands.append('router bgp {0}'.format(module.params['asn']))
-            parents = []
-        candidate.add(commands, parents=parents)
+
+    candidate.add(commands, parents=parents)
 
 
-def state_absent(module, existing, proposed, candidate):
+def state_absent(module, existing, candidate):
     commands = []
     parents = []
     if module.params['vrf'] == 'default':
         commands.append('no router bgp {0}'.format(module.params['asn']))
-    else:
-        if existing.get('vrf') == module.params['vrf']:
-            commands.append('no vrf {0}'.format(module.params['vrf']))
-            parents = ['router bgp {0}'.format(module.params['asn'])]
+    elif existing.get('vrf') == module.params['vrf']:
+        commands.append('no vrf {0}'.format(module.params['vrf']))
+        parents = ['router bgp {0}'.format(module.params['asn'])]
 
     candidate.add(commands, parents=parents)
 
@@ -696,11 +637,7 @@ def main():
         timer_bgp_hold=dict(required=False, type='str'),
         timer_bgp_keepalive=dict(required=False, type='str'),
         state=dict(choices=['present', 'absent'], default='present', required=False),
-        include_defaults=dict(default=True),
-        config=dict(),
-        save=dict(type='bool', default=False)
     )
-
     argument_spec.update(nxos_argument_spec)
 
     module = AnsibleModule(argument_spec=argument_spec,
@@ -709,63 +646,22 @@ def main():
 
     warnings = list()
     check_args(module, warnings)
+    result = dict(changed=False, warnings=warnings)
 
     state = module.params['state']
-    args = [
-        "asn",
-        "bestpath_always_compare_med",
-        "bestpath_aspath_multipath_relax",
-        "bestpath_compare_neighborid",
-        "bestpath_compare_routerid",
-        "bestpath_cost_community_ignore",
-        "bestpath_med_confed",
-        "bestpath_med_missing_as_worst",
-        "bestpath_med_non_deterministic",
-        "cluster_id",
-        "confederation_id",
-        "confederation_peers",
-        "disable_policy_batching",
-        "disable_policy_batching_ipv4_prefix_list",
-        "disable_policy_batching_ipv6_prefix_list",
-        "enforce_first_as",
-        "event_history_cli",
-        "event_history_detail",
-        "event_history_events",
-        "event_history_periodic",
-        "fast_external_fallover",
-        "flush_routes",
-        "graceful_restart",
-        "graceful_restart_helper",
-        "graceful_restart_timers_restart",
-        "graceful_restart_timers_stalepath_time",
-        "isolate",
-        "local_as",
-        "log_neighbor_changes",
-        "maxas_limit",
-        "neighbor_down_fib_accelerate",
-        "reconnect_interval",
-        "router_id",
-        "shutdown",
-        "suppress_fib_pending",
-        "timer_bestpath_limit",
-        "timer_bgp_hold",
-        "timer_bgp_keepalive",
-        "vrf"
-    ]
 
     if module.params['vrf'] != 'default':
-        for param, inserted_value in module.params.items():
-            if param in GLOBAL_PARAMS and inserted_value:
-                module.fail_json(msg='Global params can be modified only'
-                                     ' under "default" VRF.',
+        for param in GLOBAL_PARAMS:
+            if module.params[param]:
+                module.fail_json(msg='Global params can be modified only under "default" VRF.',
                                  vrf=module.params['vrf'],
                                  global_param=param)
 
+    args = PARAM_TO_COMMAND_KEYMAP.keys()
     existing = get_existing(module, args, warnings)
 
-    if existing.get('asn'):
-        if (existing.get('asn') != module.params['asn'] and
-                state == 'present'):
+    if existing.get('asn') and state == 'present':
+        if existing.get('asn') != module.params['asn']:
             module.fail_json(msg='Another BGP ASN already exists.',
                              proposed_asn=module.params['asn'],
                              existing_asn=existing.get('asn'))
@@ -774,24 +670,23 @@ def main():
                          if v is not None and k in args)
     proposed = {}
     for key, value in proposed_args.items():
-        if key != 'asn' and key != 'vrf':
+        if key not in ['asn', 'vrf']:
             if str(value).lower() == 'default':
-                value = PARAM_TO_DEFAULT_KEYMAP.get(key)
-                if value is None:
-                    value = 'default'
-            if existing.get(key) or (not existing.get(key) and value):
+                value = PARAM_TO_DEFAULT_KEYMAP.get(key, 'default')
+            if existing.get(key) != value:
                 proposed[key] = value
 
-    result = dict(changed=False, warnings=warnings)
-
-    if state == 'present' or existing.get('asn') == module.params['asn']:
-        candidate = CustomNetworkConfig(indent=3)
-        invoke('state_%s' % state, module, existing, proposed, candidate)
-
-        if (candidate):
-            load_config(module, candidate)
-            result['changed'] = True
-        result['commands'] = [item.text for item in candidate.items]
+    candidate = CustomNetworkConfig(indent=3)
+    if state == 'present':
+        state_present(module, existing, proposed, candidate)
+    elif existing.get('asn') == module.params['asn']:
+        state_absent(module, existing, candidate)
+
+    if candidate:
+        candidate = candidate.items_text()
+        load_config(module, candidate)
+        result['changed'] = True
+        result['commands'] = candidate
     else:
         result['commands'] = []
 

test/units/modules/network/nxos/fixtures/nxos_bgp_config.cfg

@@ -0,0 +1,8 @@
+feature bgp
+
+router bgp 65535
+  router-id 192.168.1.1
+  event-history cli size medium
+  event-history detail
+  vrf test2
+    timers bgp 1 10

test/units/modules/network/nxos/test_nxos_bgp.py

@@ -42,9 +42,56 @@ class TestNxosBgpModule(TestNxosModule):
         self.mock_get_config.stop()
 
     def load_fixtures(self, commands=None):
+        self.get_config.return_value = load_fixture('nxos_bgp_config.cfg')
         self.load_config.return_value = None
 
     def test_nxos_bgp(self):
-        set_module_args(dict(asn=65535, vrf='test', router_id='1.1.1.1'))
+        set_module_args(dict(asn=65535, router_id='1.1.1.1'))
         result = self.execute_module(changed=True)
-        self.assertEqual(result['commands'], ['router bgp 65535', 'vrf test', 'router-id 1.1.1.1'])
+        self.assertEqual(result['commands'], ['router bgp 65535', 'router-id 1.1.1.1'])
+
+    def test_nxos_bgp_change_nothing(self):
+        set_module_args(dict(asn=65535, router_id='192.168.1.1'))
+        self.execute_module(changed=False)
+
+    def test_nxos_bgp_wrong_asn(self):
+        set_module_args(dict(asn=10, router_id='192.168.1.1'))
+        result = self.execute_module(failed=True)
+        self.assertEqual(result['msg'], 'Another BGP ASN already exists.')
+
+    def test_nxos_bgp_remove(self):
+        set_module_args(dict(asn=65535, state='absent'))
+        self.execute_module(changed=True, commands=['no router bgp 65535'])
+
+    def test_nxos_bgp_remove_vrf(self):
+        set_module_args(dict(asn=65535, vrf='test2', state='absent'))
+        self.execute_module(changed=True, commands=['router bgp 65535', 'no vrf test2'])
+
+    def test_nxos_bgp_remove_nonexistant_vrf(self):
+        set_module_args(dict(asn=65535, vrf='foo', state='absent'))
+        self.execute_module(changed=False)
+
+    def test_nxos_bgp_remove_wrong_asn(self):
+        set_module_args(dict(asn=10, state='absent'))
+        self.execute_module(changed=False)
+
+    def test_nxos_bgp_vrf(self):
+        set_module_args(dict(asn=65535, vrf='test', router_id='1.1.1.1'))
+        result = self.execute_module(changed=True, commands=['router bgp 65535', 'vrf test', 'router-id 1.1.1.1'])
+        self.assertEqual(result['warnings'], ["VRF test doesn't exist."])
+
+    def test_nxos_bgp_global_param(self):
+        set_module_args(dict(asn=65535, shutdown=True))
+        self.execute_module(changed=True, commands=['router bgp 65535', 'shutdown'])
+
+    def test_nxos_bgp_global_param_outside_default(self):
+        set_module_args(dict(asn=65535, vrf='test', shutdown=True))
+        result = self.execute_module(failed=True)
+        self.assertEqual(result['msg'], 'Global params can be modified only under "default" VRF.')
+
+    def test_nxos_bgp_default_value(self):
+        set_module_args(dict(asn=65535, graceful_restart_timers_restart='default'))
+        self.execute_module(
+            changed=True,
+            commands=['router bgp 65535', 'graceful-restart restart-time 120']
+        )