From 5fdd21714536a69348adbfdb97370c3201b401be Mon Sep 17 00:00:00 2001 From: Jernej Jakob Date: Tue, 5 May 2020 14:50:59 +0200 Subject: config: T2427: always return copies of lists Since lists in python are assigned by reference, taking the return value from these functions and modifying it will modify all other return values of functions that called the function before and did not explicitly copy it. To be safe, always make a copy of lists before returning them. --- python/vyos/config.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) (limited to 'python') 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) -- cgit v1.2.3 From 7a1c1fef7677e68d5a92c8538eda74a45b47a3c9 Mon Sep 17 00:00:00 2001 From: Jernej Jakob Date: Tue, 5 May 2020 15:48:47 +0200 Subject: configdict: T2427: do not remove all addresses when disabling interface Commit 3fdf0093a introduced code that removed all addresses from an interface when that interface is disabled. This is wrong, as other configured services may be listening on these addresses and may fail to start if their configured address isn't present. It also caused a commit error when applying dhcp-server configuration: DHCP server configuration error! None of configured DHCP subnets does not have appropriate primary IP address on any broadcast interface. This commit reverts it to prior behavior, which was to just put the interface admin down and leave all addresses configured, other than the IPv6 'fe80::EUI-64/64' link-local, which it deletes, as the interface may not have a MAC if it's put down. --- python/vyos/configdict.py | 55 ++++++++++++++++++----------------------------- 1 file changed, 21 insertions(+), 34 deletions(-) (limited to 'python') diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py index 5ab831258..ceacf86fc 100644 --- a/python/vyos/configdict.py +++ b/python/vyos/configdict.py @@ -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') + + # 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 what should stay or be removed + # 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 -- cgit v1.2.3 From 023b6b772ee6d6853e1c0e585b7f7d207e9926ce Mon Sep 17 00:00:00 2001 From: Jernej Jakob Date: Tue, 5 May 2020 15:57:31 +0200 Subject: vlan: T2427: convert vlan config variables from lists to dicts Previously all vlan configs, which are dicts, were appended to a simple list, with the distinguishing 'id' stored inside the dicts themselves. This worked, but wasn't ideal. This commit converts them to dicts, where the key is the VLAN ID and value the config dict of that VLAN. This makes it posible to access single VLANs by their ID (key) and we can for-loop and get both the ID and config with: 'for vif_id, vif in conf["vif"].items():' --- python/vyos/configdict.py | 12 ++++-------- python/vyos/ifconfig_vlan.py | 25 +++++++++++++++++-------- 2 files changed, 21 insertions(+), 16 deletions(-) (limited to 'python') diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py index ceacf86fc..666497389 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': '' } @@ -400,7 +400,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) @@ -411,12 +412,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! @@ -428,7 +425,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..54736ec6f 100644 --- a/python/vyos/ifconfig_vlan.py +++ b/python/vyos/ifconfig_vlan.py @@ -104,7 +104,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 +132,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 +163,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'] -- cgit v1.2.3 From 312cffce31cf3a411a87559bf99f78cde5facd60 Mon Sep 17 00:00:00 2001 From: Jernej Jakob Date: Tue, 5 May 2020 16:03:49 +0200 Subject: vlan: T2427: move code that applies VLANs to interface to common function --- python/vyos/ifconfig_vlan.py | 47 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) (limited to 'python') diff --git a/python/vyos/ifconfig_vlan.py b/python/vyos/ifconfig_vlan.py index 54736ec6f..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 -- cgit v1.2.3 From 0a3147631094dba44523bd439586086c1bf6a93b Mon Sep 17 00:00:00 2001 From: Jernej Jakob Date: Tue, 5 May 2020 16:04:54 +0200 Subject: configdict: T2427: clarify code comments --- python/vyos/configdict.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'python') diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py index 666497389..5ca369f66 100644 --- a/python/vyos/configdict.py +++ b/python/vyos/configdict.py @@ -375,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'] = [] @@ -389,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 -- cgit v1.2.3