From bbfe367648be30a315db2fd69e197e1d63393327 Mon Sep 17 00:00:00 2001 From: Christian Poessinger Date: Sun, 28 Feb 2021 18:14:18 +0100 Subject: vif: T3349: use fixed ordering when enabling parent and child interface When a VIF/VLAN interface is placed in admin down state but the lower interface, serving the vlan, is moved from admin down -> admin up, all its vlan interfaces will be placed in admin up state, too. This is bad as a VLAN interface will become admin up even if its specified as admin down after a reboot. To reproduce: set interfaces ethernet eth1 vif 20 disable set interfaces ethernet eth1 disable commit delete interfaces ethernet eth1 disable commit Now check the interface state and it returns UP,LOWER_UP 7: eth1.20@eth1: mtu 1500 qdisc noqueue state UP group default qlen 1000 link/ether 00:50:56:b3:09:07 brd ff:ff:ff:ff:ff:ff inet6 fe80::250:56ff:feb3:907/64 scope link valid_lft forever preferred_lft forever (cherry picked from commit 49bc3f1e3ff8416908fc986bb60b444a75a1722d) --- python/vyos/ifconfig/bond.py | 14 ++--------- python/vyos/ifconfig/bridge.py | 18 +++----------- python/vyos/ifconfig/dummy.py | 19 -------------- python/vyos/ifconfig/ethernet.py | 14 ++--------- python/vyos/ifconfig/geneve.py | 19 -------------- python/vyos/ifconfig/interface.py | 29 ++++++++------------- python/vyos/ifconfig/l2tpv3.py | 21 ---------------- python/vyos/ifconfig/loopback.py | 10 -------- python/vyos/ifconfig/macsec.py | 19 -------------- python/vyos/ifconfig/macvlan.py | 19 -------------- python/vyos/ifconfig/tunnel.py | 19 -------------- python/vyos/ifconfig/vtun.py | 19 -------------- python/vyos/ifconfig/vxlan.py | 19 -------------- python/vyos/ifconfig/wireguard.py | 11 -------- python/vyos/ifconfig/wireless.py | 21 ---------------- smoketest/scripts/cli/base_interfaces_test.py | 36 +++++++++++++++++++++++++++ 16 files changed, 53 insertions(+), 254 deletions(-) diff --git a/python/vyos/ifconfig/bond.py b/python/vyos/ifconfig/bond.py index 709222b09..199c69dde 100644 --- a/python/vyos/ifconfig/bond.py +++ b/python/vyos/ifconfig/bond.py @@ -343,9 +343,6 @@ class BondIf(Interface): if 'shutdown_required' in config: self.set_admin_state('down') - # call base class first - super().update(config) - # ARP monitor targets need to be synchronized between sysfs and CLI. # Unfortunately an address can't be send twice to sysfs as this will # result in the following exception: OSError: [Errno 22] Invalid argument. @@ -404,12 +401,5 @@ class BondIf(Interface): value = config.get('primary') if value: self.set_primary(value) - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) + # call base class first + super().update(config) diff --git a/python/vyos/ifconfig/bridge.py b/python/vyos/ifconfig/bridge.py index 76520f2ba..116ed22c0 100644 --- a/python/vyos/ifconfig/bridge.py +++ b/python/vyos/ifconfig/bridge.py @@ -235,11 +235,6 @@ class BridgeIf(Interface): interface setup code and provide a single point of entry when workin on any interface. """ - # call base class first - super().update(config) - - ifname = config['ifname'] - # Set ageing time value = config.get('aging') self.set_ageing_time(value) @@ -279,6 +274,7 @@ class BridgeIf(Interface): vlan_del = set() vlan_add = set() + ifname = config['ifname'] tmp = dict_search('member.interface', config) if tmp: if self.get_vlan_filter(): @@ -327,7 +323,6 @@ class BridgeIf(Interface): tmp = dict_search('allowed_vlan_removed', interface_config) - for vlan_id in (tmp or []): cmd = f'bridge vlan del dev {interface} vid {vlan_id}' self._cmd(cmd) @@ -373,12 +368,5 @@ class BridgeIf(Interface): self.set_vlan_filter(vlan_filter) - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) + # call base class first + super().update(config) diff --git a/python/vyos/ifconfig/dummy.py b/python/vyos/ifconfig/dummy.py index 19ef9d304..f2a4106e6 100644 --- a/python/vyos/ifconfig/dummy.py +++ b/python/vyos/ifconfig/dummy.py @@ -33,22 +33,3 @@ class DummyIf(Interface): 'prefixes': ['dum', ], }, } - - def update(self, config): - """ General helper function which works on a dictionary retrived by - get_config_dict(). It's main intention is to consolidate the scattered - interface setup code and provide a single point of entry when workin - on any interface. """ - - # call base class first - super().update(config) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) diff --git a/python/vyos/ifconfig/ethernet.py b/python/vyos/ifconfig/ethernet.py index 547b54b84..186a2d094 100644 --- a/python/vyos/ifconfig/ethernet.py +++ b/python/vyos/ifconfig/ethernet.py @@ -321,9 +321,6 @@ class EthernetIf(Interface): interface setup code and provide a single point of entry when workin on any interface. """ - # call base class first - super().update(config) - # disable ethernet flow control (pause frames) value = 'off' if 'disable_flow_control' in config else 'on' self.set_flow_control(value) @@ -357,12 +354,5 @@ class EthernetIf(Interface): for b_type in config['ring_buffer']: self.set_ring_buffer(b_type, config['ring_buffer'][b_type]) - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) + # call base class first + super().update(config) diff --git a/python/vyos/ifconfig/geneve.py b/python/vyos/ifconfig/geneve.py index 5c4597be8..0a3711dab 100644 --- a/python/vyos/ifconfig/geneve.py +++ b/python/vyos/ifconfig/geneve.py @@ -49,22 +49,3 @@ class GeneveIf(Interface): # interface is always A/D down. It needs to be enabled explicitly self.set_admin_state('down') - - def update(self, config): - """ General helper function which works on a dictionary retrived by - get_config_dict(). It's main intention is to consolidate the scattered - interface setup code and provide a single point of entry when workin - on any interface. """ - - # call base class first - super().update(config) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py index fea411319..59a87dcef 100644 --- a/python/vyos/ifconfig/interface.py +++ b/python/vyos/ifconfig/interface.py @@ -1252,6 +1252,16 @@ class Interface(Control): # configure port mirror self.set_mirror() + # Enable/Disable of an interface must always be done at the end of the + # derived class to make use of the ref-counting set_admin_state() + # function. We will only enable the interface if 'up' was called as + # often as 'down'. This is required by some interface implementations + # as certain parameters can only be changed when the interface is + # in admin-down state. This ensures the link does not flap during + # reconfiguration. + state = 'down' if 'disable' in config else 'up' + self.set_admin_state(state) + # remove no longer required 802.1ad (Q-in-Q VLANs) ifname = config['ifname'] for vif_s_id in config.get('vif_s_remove', {}): @@ -1380,22 +1390,3 @@ class VLANIf(Interface): def set_mirror(self): return - - def update(self, config): - """ General helper function which works on a dictionary retrived by - get_config_dict(). It's main intention is to consolidate the scattered - interface setup code and provide a single point of entry when workin - on any interface. """ - - # call base class first - super().update(config) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) diff --git a/python/vyos/ifconfig/l2tpv3.py b/python/vyos/ifconfig/l2tpv3.py index 8ed3d5afb..76d6e6311 100644 --- a/python/vyos/ifconfig/l2tpv3.py +++ b/python/vyos/ifconfig/l2tpv3.py @@ -94,24 +94,3 @@ class L2TPv3If(Interface): if self.config['tunnel_id']: cmd = 'ip l2tp del tunnel tunnel_id {tunnel_id}' self._cmd(cmd.format(**self.config)) - - - def update(self, config): - """ General helper function which works on a dictionary retrived by - get_config_dict(). It's main intention is to consolidate the scattered - interface setup code and provide a single point of entry when workin - on any interface. """ - - # call base class first - super().update(config) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) - diff --git a/python/vyos/ifconfig/loopback.py b/python/vyos/ifconfig/loopback.py index 0e632d826..e911ecbd9 100644 --- a/python/vyos/ifconfig/loopback.py +++ b/python/vyos/ifconfig/loopback.py @@ -70,13 +70,3 @@ class LoopbackIf(Interface): # call base class super().update(config) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) diff --git a/python/vyos/ifconfig/macsec.py b/python/vyos/ifconfig/macsec.py index 456686ea6..a229eaed5 100644 --- a/python/vyos/ifconfig/macsec.py +++ b/python/vyos/ifconfig/macsec.py @@ -55,22 +55,3 @@ class MACsecIf(Interface): # interface is always A/D down. It needs to be enabled explicitly self.set_admin_state('down') - - def update(self, config): - """ General helper function which works on a dictionary retrived by - get_config_dict(). It's main intention is to consolidate the scattered - interface setup code and provide a single point of entry when workin - on any interface. """ - - # call base class first - super().update(config) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) diff --git a/python/vyos/ifconfig/macvlan.py b/python/vyos/ifconfig/macvlan.py index 2447fec77..894215539 100644 --- a/python/vyos/ifconfig/macvlan.py +++ b/python/vyos/ifconfig/macvlan.py @@ -51,22 +51,3 @@ class MACVLANIf(Interface): ifname = self.config['ifname'] cmd = f'ip link set dev {ifname} type macvlan mode {mode}' return self._cmd(cmd) - - def update(self, config): - """ General helper function which works on a dictionary retrived by - get_config_dict(). It's main intention is to consolidate the scattered - interface setup code and provide a single point of entry when workin - on any interface. """ - - # call base class first - super().update(config) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) diff --git a/python/vyos/ifconfig/tunnel.py b/python/vyos/ifconfig/tunnel.py index 1af4f8e72..90dcac0f2 100644 --- a/python/vyos/ifconfig/tunnel.py +++ b/python/vyos/ifconfig/tunnel.py @@ -112,25 +112,6 @@ class _Tunnel(Interface): mac.dialect = mac_unix_expanded return str(mac) - def update(self, config): - """ General helper function which works on a dictionary retrived by - get_config_dict(). It's main intention is to consolidate the scattered - interface setup code and provide a single point of entry when workin - on any interface. """ - - # call base class first - super().update(config) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) - class GREIf(_Tunnel): """ GRE: Generic Routing Encapsulation diff --git a/python/vyos/ifconfig/vtun.py b/python/vyos/ifconfig/vtun.py index 99a592b3e..2c6e126d5 100644 --- a/python/vyos/ifconfig/vtun.py +++ b/python/vyos/ifconfig/vtun.py @@ -51,22 +51,3 @@ class VTunIf(Interface): def del_addr(self, addr): # IP addresses are managed by OpenVPN daemon pass - - def update(self, config): - """ General helper function which works on a dictionary retrived by - get_config_dict(). It's main intention is to consolidate the scattered - interface setup code and provide a single point of entry when workin - on any interface. """ - - # call base class first - super().update(config) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) diff --git a/python/vyos/ifconfig/vxlan.py b/python/vyos/ifconfig/vxlan.py index ad1f605ed..202e9d81c 100644 --- a/python/vyos/ifconfig/vxlan.py +++ b/python/vyos/ifconfig/vxlan.py @@ -94,22 +94,3 @@ class VXLANIf(Interface): cmd += ' {} {}'.format(self.mapping.get(key, key), value) self._cmd(cmd) - - def update(self, config): - """ General helper function which works on a dictionary retrived by - get_config_dict(). It's main intention is to consolidate the scattered - interface setup code and provide a single point of entry when workin - on any interface. """ - - # call base class first - super().update(config) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) diff --git a/python/vyos/ifconfig/wireguard.py b/python/vyos/ifconfig/wireguard.py index 9ee798ee8..33f59b57e 100644 --- a/python/vyos/ifconfig/wireguard.py +++ b/python/vyos/ifconfig/wireguard.py @@ -261,14 +261,3 @@ class WireGuardIf(Interface): # call base class super().update(config) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) - diff --git a/python/vyos/ifconfig/wireless.py b/python/vyos/ifconfig/wireless.py index 37703d242..0efe38d59 100644 --- a/python/vyos/ifconfig/wireless.py +++ b/python/vyos/ifconfig/wireless.py @@ -71,24 +71,3 @@ class WiFiIf(Interface): # re-add ourselves to any bridge we might have fallen out of if bridge_member: self.add_to_bridge(bridge_member) - - # Enable/Disable of an interface must always be done at the end of the - # derived class to make use of the ref-counting set_admin_state() - # function. We will only enable the interface if 'up' was called as - # often as 'down'. This is required by some interface implementations - # as certain parameters can only be changed when the interface is - # in admin-down state. This ensures the link does not flap during - # reconfiguration. - state = 'down' if 'disable' in config else 'up' - self.set_admin_state(state) - - -@Interface.register -class WiFiModemIf(WiFiIf): - definition = { - **WiFiIf.definition, - **{ - 'section': 'wirelessmodem', - 'prefixes': ['wlm', ], - } - } diff --git a/smoketest/scripts/cli/base_interfaces_test.py b/smoketest/scripts/cli/base_interfaces_test.py index f96739183..3e10f220e 100644 --- a/smoketest/scripts/cli/base_interfaces_test.py +++ b/smoketest/scripts/cli/base_interfaces_test.py @@ -292,6 +292,42 @@ class BasicInterfaceTest: self.assertEqual(tmp, self._mtu) self.assertEqual(Interface(vif).get_admin_state(), 'up') + def test_vif_8021q_lower_up_down(self): + # Testcase for https://phabricator.vyos.net/T3349 + if not self._test_vlan: + self.skipTest('not supported') + + for interface in self._interfaces: + base = self._base_path + [interface] + for option in self._options.get(interface, []): + self.session.set(base + option.split()) + + # disable the lower interface + self.session.set(base + ['disable']) + + for vlan in self._vlan_range: + vlan_base = self._base_path + [interface, 'vif', vlan] + # disable the vlan interface + self.session.set(vlan_base + ['disable']) + + self.session.commit() + + # re-enable all lower interfaces + for interface in self._interfaces: + base = self._base_path + [interface] + self.session.delete(base + ['disable']) + + self.session.commit() + + # verify that the lower interfaces are admin up and the vlan + # interfaces are all admin down + for interface in self._interfaces: + self.assertEqual(Interface(interface).get_admin_state(), 'up') + + for vlan in self._vlan_range: + ifname = f'{interface}.{vlan}' + self.assertEqual(Interface(ifname).get_admin_state(), 'down') + def test_vif_s_8021ad_vlan_interfaces(self): # XXX: This testcase is not allowed to run as first testcase, reason -- cgit v1.2.3