ifconfig: T2985: support on demand bridge creation

The current implementation for bridge based interfaces has an issue which is
caused by priority inheritance. We always assumed that the bridge interface will
be created last, but this may not be true in all cases, where some interfaces
will be created "on demand" - e.g. OpenVPN or late (VXLAN, GENEVE).

As we already have a bunch of verify steps in place we should not see a bridge
interface leak to the underlaying infrastructure code. This means, whenever an
interface will be member of a bridge, and the bridge does yet not exist, we will
create it in advance in the interface context, as the bridge code will be run
in the same commit but maybe sooner or later.

This will also be the solution for T2924.

9 files changed, 93 insertions, 124 deletions

diff --git a/interface-definitions/interfaces-bridge.xml.in b/interface-definitions/interfaces-bridge.xml.in
index 787e856d7..0a777865b 100644
--- a/interface-definitions/interfaces-bridge.xml.in
+++ b/interface-definitions/interfaces-bridge.xml.in
@@ -5,7 +5,7 @@
       <tagNode name="bridge" owner="${vyos_conf_scripts_dir}/interfaces-bridge.py">
         <properties>
           <help>Bridge Interface</help>
-          <priority>489</priority>
+          <priority>310</priority>
           <constraint>
             <regex>^br[0-9]+$</regex>
           </constraint>
diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py
index 02006465c..62df3334c 100644
--- a/python/vyos/configdict.py
+++ b/python/vyos/configdict.py
@@ -194,11 +194,9 @@ def is_member(conf, interface, intftype=None):
     interface name -> Interface is a member of this interface
     False -> interface type cannot have members
     """
-    from vyos.xml import is_tag
-    from vyos.xml import is_leaf
-
     ret_val = None
     intftypes = ['bonding', 'bridge']
+
     if intftype not in intftypes + [None]:
         raise ValueError((
             f'unknown interface type "{intftype}" or it cannot '
@@ -210,19 +208,13 @@ def is_member(conf, interface, intftype=None):
     old_level = conf.get_level()
     conf.set_level([])
 
-    for it in intftype:
-        base = ['interfaces', it]
+    for iftype in intftype:
+        base = ['interfaces', iftype]
         for intf in conf.list_nodes(base):
-            memberintf = base + [intf, 'member', 'interface']
-            if is_tag(memberintf):
-                if interface in conf.list_nodes(memberintf):
-                    ret_val = intf
-                    break
-            elif is_leaf(memberintf):
-                if ( conf.exists(memberintf) and
-                        interface in conf.return_values(memberintf) ):
-                    ret_val = intf
-                    break
+            member = base + [intf, 'member', 'interface', interface]
+            if conf.exists(member):
+                tmp = conf.get_config_dict(member, key_mangling=('-', '_'), get_first_key=True)
+                ret_val = {intf : tmp}
 
     old_level = conf.set_level(old_level)
     return ret_val
diff --git a/python/vyos/ifconfig/bridge.py b/python/vyos/ifconfig/bridge.py
index c133a56fc..7867a7387 100644
--- a/python/vyos/ifconfig/bridge.py
+++ b/python/vyos/ifconfig/bridge.py
@@ -234,25 +234,33 @@ class BridgeIf(Interface):
             if member in interfaces():
                 self.del_port(member)
 
-        STPBridgeIf = STP.enable(BridgeIf)
         tmp = vyos_dict_search('member.interface', config)
         if tmp:
             for interface, interface_config in tmp.items():
-                # if we've come here we already verified the interface
-                # does not have an addresses configured so just flush
-                # any remaining ones
-                Interface(interface).flush_addrs()
+                # if interface does yet not exist bail out early and
+                # add it later
+                if interface not in interfaces():
+                    continue
+
+                # Bridge lower "physical" interface
+                lower = Interface(interface)
+
+                # If we've come that far we already verified the interface does
+                # not have any addresses configured by CLI so just flush any
+                # remaining ones
+                lower.flush_addrs()
                 # enslave interface port to bridge
                 self.add_port(interface)
 
-                tmp = STPBridgeIf(interface)
                 # set bridge port path cost
-                value = interface_config.get('cost')
-                tmp.set_path_cost(value)
+                if 'cost' in interface_config:
+                    value = interface_config.get('cost')
+                    lower.set_path_cost(value)
 
                 # set bridge port path priority
-                value = interface_config.get('priority')
-                tmp.set_path_priority(value)
+                if 'priority' in interface_config:
+                    value = interface_config.get('priority')
+                    lower.set_path_priority(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()
diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py
index 47ec94bd3..c1f9d14af 100644
--- a/python/vyos/ifconfig/interface.py
+++ b/python/vyos/ifconfig/interface.py
@@ -46,6 +46,7 @@ from vyos.validate import assert_positive
 from vyos.validate import assert_range
 
 from vyos.ifconfig.control import Control
+from vyos.ifconfig.stp import STP
 from vyos.ifconfig.vrrp import VRRP
 from vyos.ifconfig.operational import Operational
 from vyos.ifconfig import Section
@@ -167,6 +168,18 @@ class Interface(Control):
             'validate': assert_positive,
             'location': '/proc/sys/net/ipv6/conf/{ifname}/dad_transmits',
         },
+        'path_cost': {
+            # XXX: we should set a maximum
+            'validate': assert_positive,
+            'location': '/sys/class/net/{ifname}/brport/path_cost',
+            'errormsg': '{ifname} is not a bridge port member'
+        },
+        'path_priority': {
+            # XXX: we should set a maximum
+            'validate': assert_positive,
+            'location': '/sys/class/net/{ifname}/brport/priority',
+            'errormsg': '{ifname} is not a bridge port member'
+        },
         'proxy_arp': {
             'validate': assert_boolean,
             'location': '/proc/sys/net/ipv4/conf/{ifname}/proxy_arp',
@@ -628,6 +641,28 @@ class Interface(Control):
             self._admin_state_down_cnt += 1
             return self.set_interface('admin_state', state)
 
+    def set_path_cost(self, cost):
+        """
+        Set interface path cost, only relevant for STP enabled interfaces
+
+        Example:
+
+        >>> from vyos.ifconfig import Interface
+        >>> Interface('eth0').set_path_cost(4)
+        """
+        self.set_interface('path_cost', cost)
+
+    def set_path_priority(self, priority):
+        """
+        Set interface path priority, only relevant for STP enabled interfaces
+
+        Example:
+
+        >>> from vyos.ifconfig import Interface
+        >>> Interface('eth0').set_path_priority(4)
+        """
+        self.set_interface('path_priority', priority)
+
     def set_proxy_arp(self, enable):
         """
         Set per interface proxy ARP configuration
@@ -809,24 +844,27 @@ class Interface(Control):
         # flush all addresses
         self._cmd(f'ip addr flush dev "{self.ifname}"')
 
-    def add_to_bridge(self, br):
+    def add_to_bridge(self, bridge_dict):
         """
         Adds the interface to the bridge with the passed port config.
 
         Returns False if bridge doesn't exist.
         """
 
-        # check if the bridge exists (on boot it doesn't)
-        if br not in Section.interfaces('bridge'):
-            return False
-
+        # drop all interface addresses first
         self.flush_addrs()
-        # add interface to bridge - use Section.klass to get BridgeIf class
-        Section.klass(br)(br, create=False).add_port(self.ifname)
 
-        # TODO: port config (STP)
+        for bridge, bridge_config in bridge_dict.items():
+            # add interface to bridge - use Section.klass to get BridgeIf class
+            Section.klass(bridge)(bridge, create=True).add_port(self.ifname)
 
-        return True
+            # set bridge port path cost
+            if 'cost' in bridge_config:
+                self.set_path_cost(bridge_config['cost'])
+
+            # set bridge port path priority
+            if 'priority' in bridge_config:
+                self.set_path_cost(bridge_config['priority'])
 
     def set_dhcp(self, enable):
         """
@@ -1047,8 +1085,8 @@ class Interface(Control):
 
         # re-add ourselves to any bridge we might have fallen out of
         if 'is_bridge_member' in config:
-            bridge = config.get('is_bridge_member')
-            self.add_to_bridge(bridge)
+            bridge_dict = config.get('is_bridge_member')
+            self.add_to_bridge(bridge_dict)
 
         # remove no longer required 802.1ad (Q-in-Q VLANs)
         ifname = config['ifname']
diff --git a/python/vyos/ifconfig/stp.py b/python/vyos/ifconfig/stp.py
deleted file mode 100644
index 5e83206c2..000000000
--- a/python/vyos/ifconfig/stp.py
+++ /dev/null
@@ -1,70 +0,0 @@
-# Copyright 2019 VyOS maintainers and contributors <maintainers@vyos.io>
-#
-# This library is free software; you can redistribute it and/or
-# modify it under the terms of the GNU Lesser General Public
-# License as published by the Free Software Foundation; either
-# version 2.1 of the License, or (at your option) any later version.
-#
-# This library is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-# Lesser General Public License for more details.
-#
-# You should have received a copy of the GNU Lesser General Public
-# License along with this library.  If not, see <http://www.gnu.org/licenses/>.
-
-
-from vyos.ifconfig.interface import Interface
-
-from vyos.validate import assert_positive
-
-
-class STP:
-    """
-    A spanning-tree capable interface. This applies only to bridge port member
-    interfaces!
-    """
-
-    @classmethod
-    def enable (cls, adaptee):
-        adaptee._sysfs_set = {**adaptee._sysfs_set, **cls._sysfs_set}
-        adaptee.set_path_cost = cls.set_path_cost
-        adaptee.set_path_priority = cls.set_path_priority
-        return adaptee
-
-    _sysfs_set = {
-        'path_cost': {
-            # XXX: we should set a maximum
-            'validate': assert_positive,
-            'location': '/sys/class/net/{ifname}/brport/path_cost',
-            'errormsg': '{ifname} is not a bridge port member'
-        },
-        'path_priority': {
-            # XXX: we should set a maximum
-            'validate': assert_positive,
-            'location': '/sys/class/net/{ifname}/brport/priority',
-            'errormsg': '{ifname} is not a bridge port member'
-        },
-    }
-
-    def set_path_cost(self, cost):
-        """
-        Set interface path cost, only relevant for STP enabled interfaces
-
-        Example:
-
-        >>> from vyos.ifconfig import Interface
-        >>> Interface('eth0').set_path_cost(4)
-        """
-        self.set_interface('path_cost', cost)
-
-    def set_path_priority(self, priority):
-        """
-        Set interface path priority, only relevant for STP enabled interfaces
-
-        Example:
-
-        >>> from vyos.ifconfig import Interface
-        >>> Interface('eth0').set_path_priority(4)
-        """
-        self.set_interface('path_priority', priority)
diff --git a/src/conf_mode/interfaces-bonding.py b/src/conf_mode/interfaces-bonding.py
index 9763620ac..ea9bd54d4 100755
--- a/src/conf_mode/interfaces-bonding.py
+++ b/src/conf_mode/interfaces-bonding.py
@@ -109,7 +109,7 @@ def get_config(config=None):
 
             # Check if member interface is already member of a bond
             tmp = is_member(conf, interface, 'bonding')
-            if tmp and tmp != bond['ifname']:
+            if tmp and bond['ifname'] not in tmp:
                 interface_config.update({'is_bond_member' : tmp})
 
             # Check if member interface is used as source-interface on another interface
@@ -162,11 +162,11 @@ def verify(bond):
                 raise ConfigError(error_msg + 'it does not exist!')
 
             if 'is_bridge_member' in interface_config:
-                tmp = interface_config['is_bridge_member']
+                tmp = next(iter(interface_config['is_bridge_member']))
                 raise ConfigError(error_msg + f'it is already a member of bridge "{tmp}"!')
 
             if 'is_bond_member' in interface_config:
-                tmp = interface_config['is_bond_member']
+                tmp = next(iter(interface_config['is_bond_member']))
                 raise ConfigError(error_msg + f'it is already a member of bond "{tmp}"!')
 
             if 'is_source_interface' in interface_config:
diff --git a/src/conf_mode/interfaces-bridge.py b/src/conf_mode/interfaces-bridge.py
index 485decb17..4aeb8fc67 100755
--- a/src/conf_mode/interfaces-bridge.py
+++ b/src/conf_mode/interfaces-bridge.py
@@ -24,6 +24,7 @@ from vyos.configdict import get_interface_dict
 from vyos.configdict import node_changed
 from vyos.configdict import is_member
 from vyos.configdict import is_source_interface
+from vyos.configdict import dict_merge
 from vyos.configverify import verify_dhcpv6
 from vyos.configverify import verify_vrf
 from vyos.ifconfig import BridgeIf
@@ -69,25 +70,26 @@ def get_config(config=None):
         # the default dictionary is not properly paged into the dict (see T2665)
         # thus we will ammend it ourself
         default_member_values = defaults(base + ['member', 'interface'])
-        for interface, interface_config in bridge['member']['interface'].items():
-            interface_config.update(default_member_values)
+        for interface in bridge['member']['interface']:
+            bridge['member']['interface'][interface] = dict_merge(
+                    default_member_values, bridge['member']['interface'][interface])
 
             # Check if member interface is already member of another bridge
             tmp = is_member(conf, interface, 'bridge')
-            if tmp and tmp != bridge['ifname']:
-                interface_config.update({'is_bridge_member' : tmp})
+            if tmp and bridge['ifname'] not in tmp:
+                bridge['member']['interface'][interface].update({'is_bridge_member' : tmp})
 
             # Check if member interface is already member of a bond
             tmp = is_member(conf, interface, 'bonding')
-            if tmp: interface_config.update({'is_bond_member' : tmp})
+            if tmp: bridge['member']['interface'][interface].update({'is_bond_member' : tmp})
 
             # Check if member interface is used as source-interface on another interface
             tmp = is_source_interface(conf, interface)
-            if tmp: interface_config.update({'is_source_interface' : tmp})
+            if tmp: bridge['member']['interface'][interface].update({'is_source_interface' : tmp})
 
             # Bridge members must not have an assigned address
             tmp = has_address_configured(conf, interface)
-            if tmp: interface_config.update({'has_address' : ''})
+            if tmp: bridge['member']['interface'][interface].update({'has_address' : ''})
 
     return bridge
 
@@ -105,15 +107,12 @@ def verify(bridge):
             if interface == 'lo':
                 raise ConfigError('Loopback interface "lo" can not be added to a bridge')
 
-            if interface not in interfaces():
-                raise ConfigError(error_msg + 'it does not exist!')
-
             if 'is_bridge_member' in interface_config:
-                tmp = interface_config['is_bridge_member']
+                tmp = next(iter(interface_config['is_bridge_member']))
                 raise ConfigError(error_msg + f'it is already a member of bridge "{tmp}"!')
 
             if 'is_bond_member' in interface_config:
-                tmp = interface_config['is_bond_member']
+                tmp = next(iter(interface_config['is_bond_member']))
                 raise ConfigError(error_msg + f'it is already a member of bond "{tmp}"!')
 
             if 'is_source_interface' in interface_config:
diff --git a/src/conf_mode/interfaces-openvpn.py b/src/conf_mode/interfaces-openvpn.py
index 518dbdc0e..f2b580c6f 100755
--- a/src/conf_mode/interfaces-openvpn.py
+++ b/src/conf_mode/interfaces-openvpn.py
@@ -208,7 +208,8 @@ def get_config(config=None):
     openvpn['auth_user_pass_file'] = f"/run/openvpn/{openvpn['intf']}.pw"
 
     # check if interface is member of a bridge
-    openvpn['is_bridge_member'] = is_member(conf, openvpn['intf'], 'bridge')
+    tmp = is_member(conf, openvpn['intf'], 'bridge')
+    if tmp: openvpn['is_bridge_member'] = next(iter(tmp))
 
     # Check if interface instance has been removed
     if not conf.exists('interfaces openvpn ' + openvpn['intf']):
diff --git a/src/conf_mode/interfaces-tunnel.py b/src/conf_mode/interfaces-tunnel.py
index f1d885b15..5561514bd 100755
--- a/src/conf_mode/interfaces-tunnel.py
+++ b/src/conf_mode/interfaces-tunnel.py
@@ -462,7 +462,8 @@ def get_config(config=None):
     options['tunnel'] = {}
 
     # check for bridges
-    options['bridge'] = is_member(config, ifname, 'bridge')
+    tmp = is_member(config, ifname, 'bridge')
+    if tmp: options['bridge'] = next(iter(tmp))
     options['interfaces'] = interfaces()
 
     for name in ct: