Page MenuHomePhabricator

bridge: Bridge adding non existing interfaces is allowed but does not work
Closed, ResolvedPublicFEATURE REQUEST

Description

when adding interfaces to the bridge it is possible to add interfaces no currently created.
and when the interface is created it is not tied into the bridge.

Steps to replicate:

  1. Create a interface with a vif
# set interface ethernet eth0 vif 1
# commit
  1. Create the bridge and add two interfaces. one of those interfaces is currently not existing
vyos@vyos# show int
# set interfaces bridge br1 address 10.3.4.5/24
# set interfaces bridge br1 member interface eth0.1
# set interfaces bridge br1 member interface eth1.1
# commit
  1. Create the missing interface
# set interface ethernet eth1 vif 1
# commit
  1. The vif is created, but is not tied to the bridge group
vyos@vyos# show int eth eth1
 ...
 vif 1 {
 }

vyos@vyos# show int bridge br1
 address 10.3.4.5/24
 member {
     interface eth0.1 {
     }
     interface eth1.1 {
     }
 }

vyos@vyos# ip a s eth1.1
6: eth1.1@eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether 52:54:05:12:34:57 brd ff:ff:ff:ff:ff:ff
    inet6 fe80::5054:5ff:fe12:3457/64 scope link
       valid_lft forever preferred_lft forever

vyos@vyos# brctl show
bridge name	bridge id		STP enabled	interfaces
br1		8000.525405123456	yes		eth0.1         <-- interface eth1.1 is not added to the bridge

To fix this two posibillities exists.

  1. Do not allow additions of un-initialized interfaces . this also means that deletion of an interface in a bridge-group also should be disallowed.
  1. On interface creation, make it check if it should be in a bridge-group and run the bridge-script to re-add the interface to the bridge-group

out of these two methods i would prefer if solution 2 is implemented.

I am still not 100% sure that this syntax is better then having the bridge mapping on the interface itself. because this is actually a function of the interface and not of the bridge. and will be added/removed with the interface. so this is something that also could have been in the ethernet-script

Details

Difficulty level
Normal (likely a few hours)
Version
-
Why the issue appeared?
Will be filled on close

Event Timeline

runar renamed this task from bridge: possible to add non-created interfaces. to bridge: Bridge adding non existing interfaces is allowed but does not work.Fri, Aug 23, 8:42 AM
runar created this task.
c-po added a comment.Fri, Aug 23, 8:49 AM

If the bridge priority is higher then the ethernet vif this should work out of the box

runar updated the task description. (Show Details)Fri, Aug 23, 8:50 AM

i've updated the description. the bridge is created and comitted prior to the creation of the interface.

runar added a comment.EditedFri, Aug 23, 8:53 AM

the current only way to get the interface added to the group is to remove and readd it to the group.

vyos@vyos# brctl show
bridge name	bridge id		STP enabled	interfaces
br1		8000.525405123456	yes		eth0.1

vyos@vyos# show int bridge br1
 address 10.3.4.5/24
 member {
     interface eth0.1 {
     }
     interface eth1.1 {
     }
 }

vyos@vyos# del int bri br1 mem interface eth1.1
vyos@vyos# commit
[ interfaces bridge br1 ]
device eth1.1 is not a slave of br1

vyos@vyos# set int bri br1 mem interface eth1.1
vyos@vyos# commit

vyos@vyos# brctl show
bridge name	bridge id		STP enabled	interfaces
br1		8000.525405123456	yes		eth0.1
							eth1.1
runar added a comment.Fri, Aug 23, 9:29 AM

Full console dump to reproduce with comments:

##
## Start without anything

vyos@vyos# show
 interfaces {
     ethernet eth0 {
         duplex auto
         hw-id 52:54:05:12:34:56
         smp-affinity auto
         speed auto
     }
     ethernet eth1 {
         duplex auto
         hw-id 52:54:05:12:34:57
         smp-affinity auto
         speed auto
     }
     loopback lo {
     }
 }
....






##
## Create a vif on eth0 and set up the bridge
[edit]
vyos@vyos# set int eth eth0 vif 1
[edit]
vyos@vyos# set int bri br1 address 10.3.4.5/24
[edit]
vyos@vyos# set int bri br1 member int eth0.1
[edit]
vyos@vyos# set int bri br1 member int eth1.1
[edit]
vyos@vyos# commit
[ interfaces bridge br1 ]
interface eth1.1 does not exist!
Assigning 10.3.4.5/24 to br1

## The script complains about you trying to add an non-existant interface, but the config is allowed
## From now on, the CLI and brctl is out of sync config-wise 

[edit]
vyos@vyos# show int
 bridge br1 {
     address 10.3.4.5/24
     member {
         interface eth0.1 {
         }
         interface eth1.1 {
         }
     }
 }
 ethernet eth0 {
     duplex auto
     hw-id 52:54:05:12:34:56
     smp-affinity auto
     speed auto
     vif 1 {
     }
 }
 ethernet eth1 {
     duplex auto
     hw-id 52:54:05:12:34:57
     smp-affinity auto
     speed auto
 }
[edit]
vyos@vyos# brctl show
bridge name	bridge id		STP enabled	interfaces
br1		8000.525405123456	no		eth0.1
[edit]
vyos@vyos# ip a | egrep -e 'eth[0123456789]'
2: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
7: eth0.1@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br1 state UP group default qlen 1000
8: br1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000





## Then create the interface missing on eth1.1
[edit]
vyos@vyos# set int eth eth1 vif 1
[edit]
vyos@vyos# commit
[edit]





## Now the interface is added but is not in the bridge-group
vyos@vyos# brctl show
bridge name	bridge id		STP enabled	interfaces
br1		8000.525405123456	no		eth0.1
[edit]

vyos@vyos# ip a | egrep -e 'eth[0123456789]'
2: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
7: eth0.1@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br1 state UP group default qlen 1000
9: eth1.1@eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000








## To force it into the bridge, it needs to be readded. 
## CLI complains when deleting because the CLI and brctl is out of sync

[edit]
vyos@vyos# del int bri br1 mem int eth1.1
[edit]
vyos@vyos# commit
[ interfaces bridge br1 ]
device eth1.1 is not a slave of br1

[edit]
vyos@vyos# set int bri br1 mem int eth1.1
[edit]
vyos@vyos# commit
[edit]






## Now, the interface is part of the bridge again.

vyos@vyos# brctl show
bridge name	bridge id		STP enabled	interfaces
br1		8000.525405123456	no		eth0.1
							eth1.1
[edit]
vyos@vyos#
c-po claimed this task.Fri, Aug 23, 9:49 AM
c-po added a comment.EditedFri, Aug 23, 10:15 AM

This can be solved using multiple ways:

  • add option to the interfaces-bridge.py script to recreate the bridge when triggered externally
    • advantage: the conde is already there
    • disadvantage: the script can easily grow to a wastebin of code executed from 100 places
  • add a dedicated bridge-group-sync.py script which synchronizes the bridge groups and interfaces, called whenever a interface is added to the system (e.g OpenVPN vtun or ethernet vif). We will walk through all available bridges configured and add possibly missing interfaces
    • advantage: code is contained in a dedicated scirpt
    • disadvantage: more and more scripts might evolve
  • Write a vyos-interfaced which acts as message receiver and will handle tasks as adding/removing IP addresses, adding/removing bridge and bond members
    • advantage: most generic and single source
    • disadvantage: most complex

I think I would start of with bridge-group-sync.py and see how this scales. In the end I think we once will definately require a vyos-interfaced or even a complete vyosd

c-po triaged this task as High priority.Fri, Aug 23, 10:17 AM
c-po changed Difficulty level from Unknown (require assessment) to Hard (possibly days).
jjakob added a subscriber: jjakob.Sat, Aug 24, 9:20 PM

Would something like https://openwrt.org/docs/techref/netifd be useful? The drawback that it's dependent on ubus https://openwrt.org/docs/techref/ubus

c-po added a comment.Mon, Aug 26, 2:02 PM

Turns out this can be done with the following code:

#!/usr/bin/env python3
#
# Copyright (C) 2019 VyOS maintainers and contributors
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License version 2 or later as
# published by the Free Software Foundation.
#
# This program 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 General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.
#

# Script is used to synchronize configured bridge interfaces.
# one can add a non existing interface to a bridge group (e.g. VLAN)
# but the vlan interface itself does yet not exist. It should be added
# to the bridge automatically once it's available

import argparse
import subprocess

from sys import exit
from time import sleep
from vyos.config import Config

def subprocess_cmd(command):
    process = subprocess.Popen(command,stdout=subprocess.PIPE, shell=True)
    proc_stdout = process.communicate()[0].strip()
    pass

if __name__ == '__main__':
    parser = argparse.ArgumentParser()
    parser.add_argument('-i', '--interface', action='store', help='Interface name which should be added to bridge it is configured for', required=True)
    args, unknownargs = parser.parse_known_args()

    conf = Config()
    if not conf.list_nodes('interfaces bridge'):
        #  no bridge interfaces exist .. bail out early
        exit(0)
    else:
        for bridge in conf.list_nodes('interfaces bridge'):
            for member_if in conf.list_nodes('interfaces bridge {} member interface'.format(bridge)):
                if args.interface == member_if:
                    cmd = 'brctl addif "{}" "{}"'.format(bridge, args.interface)
                    # let interfaces etc. settle - especially required for OpenVPN bridged interfaces
                    sleep(4)
                    subprocess_cmd(cmd)

    exit(0)

The problem is, when using an OpenVPN --up script it is executed but the interface is unfortuntely not added to the bridge. Calling the same commandline manually works. Looks like some sort of internal synchronisation when adding interfaces to the system.

On subsequent thoughts this is "bug" is no problem at all (as of now) b/c in VyOS 1.2.0 or 1.1.8 it is not possible to add an interface to a bridge which does not exist:

vyos@vyos# set interfaces ethernet eth1 bridge-group bridge br1
[edit]
vyos@vyos# set interfaces ethernet eth2 bridge-group bridge br1
[edit]
vyos@vyos# commit
[ interfaces ethernet eth1 bridge-group bridge br1 ]
bridge interface br1 does not exist on system

[[interfaces ethernet eth1]] failed
[ interfaces ethernet eth2 bridge-group bridge br1 ]
bridge interface br1 does not exist on system

[[interfaces ethernet eth2]] failed
Commit failed

Adding an interface to a non existing bridge is almost the same as adding an non existing interface to a bridge. Both make no sense and should be forbidden.

c-po closed this task as Resolved.Mon, Aug 26, 2:21 PM
c-po changed Difficulty level from Hard (possibly days) to Normal (likely a few hours).