Merge pull request #393 from jjakob/disable-address-fix-T2427

T2427: fix interface addressing, bugfix config library, migrate VLAN config to dicts, move VLAN adding to common function

3 files changed, 98 insertions, 61 deletions

diff --git a/python/vyos/config.py b/python/vyos/config.py
index 75055a603..0bc6be12a 100644
--- a/python/vyos/config.py
+++ b/python/vyos/config.py
@@ -155,7 +155,7 @@ class Config(object):
         ``exists("system name-server"`` without ``set_level``.
 
         Args:
-            path (str): relative config path
+            path (str|list): relative config path
         """
         # Make sure there's always a space between default path (level)
         # and path supplied as method argument
@@ -166,7 +166,7 @@ class Config(object):
             else:
                 self._level = []
         elif isinstance(path, list):
-            self._level = path
+            self._level = path.copy()
         else:
             raise TypeError("Level path must be either a whitespace-separated string or a list")
 
@@ -177,7 +177,7 @@ class Config(object):
         Returns:
             str: current edit level
         """
-        return(self._level)
+        return(self._level.copy())
 
     def exists(self, path):
         """
@@ -386,7 +386,7 @@ class Config(object):
             values = []
 
         if not values:
-            return(default)
+            return(default.copy())
         else:
             return(values)
 
@@ -407,7 +407,7 @@ class Config(object):
             nodes = []
 
         if not nodes:
-            return(default)
+            return(default.copy())
         else:
             return(nodes)
 
@@ -448,7 +448,6 @@ class Config(object):
         else:
             return(value)
 
-
     def return_effective_values(self, path, default=[]):
         """
         Retrieve all values of a multi-value node in a running (effective) config
@@ -465,7 +464,7 @@ class Config(object):
             values = []
 
         if not values:
-            return(default)
+            return(default.copy())
         else:
             return(values)
 
@@ -488,6 +487,6 @@ class Config(object):
             nodes = []
 
         if not nodes:
-            return(default)
+            return(default.copy())
         else:
             return(nodes)
diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py
index 5ab831258..5ca369f66 100644
--- a/python/vyos/configdict.py
+++ b/python/vyos/configdict.py
@@ -132,7 +132,7 @@ vlan_default = {
     'is_bridge_member': False,
     'mac': '',
     'mtu': 1500,
-    'vif_c': [],
+    'vif_c': {},
     'vif_c_remove': [],
     'vrf': ''
 }
@@ -201,10 +201,6 @@ def intf_to_dict(conf, default):
     intf = deepcopy(default)
     intf['intf'] = ifname_from_config(conf)
 
-    # retrieve configured interface addresses
-    if conf.exists('address'):
-        intf['address'] = conf.return_values('address')
-
     # retrieve interface description
     if conf.exists('description'):
         intf['description'] = conf.return_value('description')
@@ -258,10 +254,6 @@ def intf_to_dict(conf, default):
     if conf.exists('ipv6 address autoconf'):
         intf['ipv6_autoconf'] = 1
 
-    # Get prefixes for IPv6 addressing based on MAC address (EUI-64)
-    if conf.exists('ipv6 address eui64'):
-        intf['ipv6_eui64_prefix'] = conf.return_values('ipv6 address eui64')
-
     # Disable IPv6 forwarding on this interface
     if conf.exists('ipv6 disable-forwarding'):
         intf['ipv6_forwarding'] = 0
@@ -304,49 +296,44 @@ def intf_to_dict(conf, default):
         if intf['ingress_qos'] != conf.return_effective_value('ingress-qos'):
             intf['ingress_qos_changed'] = True
 
-    disabled = disable_state(conf)
+    # Get the interface addresses
+    intf['address'] = conf.return_values('address')
 
-    # Get the interface IPs
-    eff_addr = conf.return_effective_values('address')
-    act_addr = conf.return_values('address')
+    # addresses to remove - difference between effective and working config
+    intf['address_remove'] = list_diff(
+            conf.return_effective_values('address'),
+            intf['address']
+            )
 
     # Get prefixes for IPv6 addressing based on MAC address (EUI-64)
-    eff_eui = conf.return_effective_values('ipv6 address eui64')
-    act_eui = conf.return_values('ipv6 address eui64')
+    intf['ipv6_eui64_prefix'] = conf.return_values('ipv6 address eui64')
 
-    # Determine what should stay or be removed
+    # EUI64 to remove - difference between effective and working config
+    intf['ipv6_eui64_prefix_remove'] = list_diff(
+            conf.return_effective_values('ipv6 address eui64'),
+            intf['ipv6_eui64_prefix']
+            )
+
+    # Determine if the interface should be disabled
+    disabled = disable_state(conf)
     if disabled == disable.both:
         # was and is still disabled
         intf['disable'] = True
-        intf['address'] = []
-        intf['address_remove'] = []
-        intf['ipv6_eui64_prefix'] = []
-        intf['ipv6_eui64_prefix_remove'] = []
     elif disabled == disable.now:
         # it is now disable but was not before
         intf['disable'] = True
-        intf['address'] = []
-        intf['address_remove'] = eff_addr
-        intf['ipv6_eui64_prefix'] = []
-        intf['ipv6_eui64_prefix_remove'] = eff_eui
     elif disabled == disable.was:
         # it was disable but not anymore
         intf['disable'] = False
-        intf['address'] = act_addr
-        intf['address_remove'] = []
-        intf['ipv6_eui64_prefix'] = act_eui
-        intf['ipv6_eui64_prefix_remove'] = []
     else:
         # normal change
         intf['disable'] = False
-        intf['address'] = act_addr
-        intf['address_remove'] = list_diff(eff_addr, act_addr)
-        intf['ipv6_eui64_prefix'] = act_eui
-        intf['ipv6_eui64_prefix_remove'] = list_diff(eff_eui, act_eui)
 
-    # Remove the default link-local address if set or if member of a bridge
+    # Remove the default link-local address if no-default-link-local is set,
+    # if member of a bridge or if disabled (it may not have a MAC if it's down)
     if ( conf.exists('ipv6 address no-default-link-local')
-            or intf.get('is_bridge_member') ):
+            or intf.get('is_bridge_member')
+            or intf['disable'] ):
         intf['ipv6_eui64_prefix_remove'].append('fe80::/64')
     else:
         # add the link-local by default to make IPv6 work
@@ -358,7 +345,7 @@ def intf_to_dict(conf, default):
         if intf['mac'] and intf['mac'] != interface.get_mac():
             intf['ipv6_eui64_prefix_remove'] += intf['ipv6_eui64_prefix']
     except Exception:
-        # If the interface does not exists, it can not have changed
+        # If the interface does not exist, it could not have changed
         pass
 
     return intf, disable
@@ -388,8 +375,7 @@ def add_to_dict(conf, disabled, ifdict, section, key):
     # the section to parse for vlan
     sections = []
 
-    # Determine interface addresses (currently effective) - to determine which
-    # address is no longer valid and needs to be removed from the bond
+    # determine which interfaces to add or remove based on disable state
     if disabled == disable.both:
         # was and is still disabled
         ifdict[f'{key}_remove'] = []
@@ -402,7 +388,7 @@ def add_to_dict(conf, disabled, ifdict, section, key):
         sections = active
     else:
         # normal change
-        # get vif-s interfaces (currently effective) - to determine which vif-s
+        # get interfaces (currently effective) - to determine which
         # interface is no longer present and needs to be removed
         ifdict[f'{key}_remove'] = list_diff(effect, active)
         sections = active
@@ -413,7 +399,8 @@ def add_to_dict(conf, disabled, ifdict, section, key):
     for s in sections:
         # set config level to vif interface
         conf.set_level(current_level + [section, s])
-        ifdict[f'{key}'].append(vlan_to_dict(conf))
+        # add the vlan config as a key (vlan id) - value (config) pair
+        ifdict[key][s] = vlan_to_dict(conf)
 
     # re-set configuration level to leave things as found
     conf.set_level(current_level)
@@ -424,12 +411,8 @@ def add_to_dict(conf, disabled, ifdict, section, key):
 def vlan_to_dict(conf, default=vlan_default):
     vlan, disabled = intf_to_dict(conf, default)
 
-    level = conf.get_level()
-    # get the '100' in 'interfaces bonding bond0 vif-s 100'
-    vlan['id'] = level[-1]
-
     # if this is a not within vif-s node, we are done
-    if level[-2] != 'vif-s':
+    if conf.get_level()[-2] != 'vif-s':
         return vlan
 
     # ethertype is mandatory on vif-s nodes and only exists here!
@@ -441,7 +424,6 @@ def vlan_to_dict(conf, default=vlan_default):
 
     # check if there is a Q-in-Q vlan customer interface
     # and call this function recursively
-
     add_to_dict(conf, disable, vlan, 'vif-c', 'vif_c')
 
     return vlan
diff --git a/python/vyos/ifconfig_vlan.py b/python/vyos/ifconfig_vlan.py
index eb7a369ec..09fb8c802 100644
--- a/python/vyos/ifconfig_vlan.py
+++ b/python/vyos/ifconfig_vlan.py
@@ -16,6 +16,53 @@
 from netifaces import interfaces
 from vyos import ConfigError
 
+def apply_all_vlans(intf, intfconfig):
+    """
+    Function applies all VLANs to the passed interface.
+
+    intf: object of Interface class
+    intfconfig: dict with interface configuration
+    """
+    # remove no longer required service VLAN interfaces (vif-s)
+    for vif_s in intfconfig['vif_s_remove']:
+        intf.del_vlan(vif_s)
+
+    # create service VLAN interfaces (vif-s)
+    for vif_s_id, vif_s in intfconfig['vif_s'].items():
+        s_vlan = intf.add_vlan(vif_s_id, ethertype=vif_s['ethertype'])
+        apply_vlan_config(s_vlan, vif_s)
+
+        # remove no longer required client VLAN interfaces (vif-c)
+        # on lower service VLAN interface
+        for vif_c in intfconfig['vif_c_remove']:
+            s_vlan.del_vlan(vif_c)
+
+        # create client VLAN interfaces (vif-c)
+        # on lower service VLAN interface
+        for vif_c_id, vif_c in vif_s['vif_c'].items():
+            c_vlan = s_vlan.add_vlan(vif_c_id)
+            apply_vlan_config(c_vlan, vif_c)
+
+    # remove no longer required VLAN interfaces (vif)
+    for vif in intfconfig['vif_remove']:
+        intf.del_vlan(vif)
+
+    # create VLAN interfaces (vif)
+    for vif_id, vif in intfconfig['vif'].items():
+        # QoS priority mapping can only be set during interface creation
+        # so we delete the interface first if required.
+        if vif['egress_qos_changed'] or vif['ingress_qos_changed']:
+            try:
+                # on system bootup the above condition is true but the interface
+                # does not exists, which throws an exception, but that's legal
+                intf.del_vlan(vif_id)
+            except:
+                pass
+
+        vlan = intf.add_vlan(vif_id, ingress_qos=vif['ingress_qos'], egress_qos=vif['egress_qos'])
+        apply_vlan_config(vlan, vif)
+
+
 def apply_vlan_config(vlan, config):
     """
     Generic function to apply a VLAN configuration from a dictionary
@@ -104,7 +151,8 @@ def verify_vlan_config(config):
     implementing this function in multiple places use single source \o/
     """
 
-    for vif in config['vif']:
+    # config['vif'] is a dict with ids as keys and config dicts as values
+    for vif in config['vif'].values():
         # DHCPv6 parameters-only and temporary address are mutually exclusive
         if vif['dhcpv6_prm_only'] and vif['dhcpv6_temporary']:
             raise ConfigError('DHCPv6 temporary and parameters-only options are mutually exclusive!')
@@ -131,14 +179,19 @@ def verify_vlan_config(config):
     if 'vif_s' not in config.keys():
         return
 
-    for vif_s in config['vif_s']:
-        for vif in config['vif']:
-            if vif['id'] == vif_s['id']:
-                raise ConfigError('Can not use identical ID on vif and vif-s interface')
+    # config['vif_s'] is a dict with ids as keys and config dicts as values
+    for vif_s_id, vif_s in config['vif_s'].items():
+        for vif_id, vif in config['vif'].items():
+            if vif_id == vif_s_id:
+                raise ConfigError((
+                    f'Cannot use identical ID on vif "{vif["intf"]}" '
+                    f'and vif-s "{vif_s}"'))
 
         # DHCPv6 parameters-only and temporary address are mutually exclusive
         if vif_s['dhcpv6_prm_only'] and vif_s['dhcpv6_temporary']:
-            raise ConfigError('DHCPv6 temporary and parameters-only options are mutually exclusive!')
+            raise ConfigError((
+                'DHCPv6 temporary and parameters-only options are mutually '
+                'exclusive!'))
 
         if ( vif_s['is_bridge_member']
                 and ( vif_s['address']
@@ -157,10 +210,13 @@ def verify_vlan_config(config):
                     f'vif-s {vif_s["intf"]} cannot be member of VRF {vif_s["vrf"]} '
                     f'and bridge {vif_s["is_bridge_member"]} at the same time!'))
 
-        for vif_c in vif_s['vif_c']:
+        # vif_c is a dict with ids as keys and config dicts as values
+        for vif_c in vif_s['vif_c'].values():
             # DHCPv6 parameters-only and temporary address are mutually exclusive
             if vif_c['dhcpv6_prm_only'] and vif_c['dhcpv6_temporary']:
-                raise ConfigError('DHCPv6 temporary and parameters-only options are mutually exclusive!')
+                raise ConfigError((
+                    'DHCPv6 temporary and parameters-only options are '
+                    'mutually exclusive!'))
 
             if ( vif_c['is_bridge_member']
                     and ( vif_c['address']