bridge: Disallow VLAN 1 when connecting to a VLAN

[phoracek]

The bridge CNI allows users to select a VLAN to which a Pod should be connected:

  {
    "cniVersion": "0.3.1",
    "name": "mynet",
    "type": "bridge",
    "bridge": "mynet0",
    "vlan": 100
  }

The expected behavior in such a case is that the Pod is isolated within the requested VLAN.

That however is not the case. Today, the underlying OS applies the default VLAN 1 to the bridge itself and to every veth connected to it. Due to that, the Pod connected to a VLAN is still receiving all the native traffic of the bridge.

That can be reproduced by:

• Using the configuration listed above,
• setting IP 11.11.11.1/24 on the bridge,
• setting IP 11.11.11.10/24 in the Pod,
• broadcasting ARP on the bridge: arping -A -I mynet0 11.11.11.1
• listening to ARP in the Pod: tcpdump -lni net1 arp

With this suggested fix, the implicitly added VLAN 1 is removed in case another VLAN ID is provided.

Fixes: https://github.com/containernetworking/plugins/issues/667

[phoracek]

/cc @dcbw @s1061123

Would you please take a look?

[maiqueb]

/cc @mccv1r0 could you chime in this ?

[maiqueb]

@phoracek shouldn't this have a unit test (or more...).

[phoracek]

Converting this to a draft: We cannot safely assume that the default PVID is 1. It can vary based on the bridge. The CNI should either check what the default is, or to strip all the IDs that were not explicitly requested.

[mlguerrero12]

yes, and also, there might be people relying on having a native vlan on the switch. It's a valid option. I'll create a pr in netlink to be able to set a native vlan, then we can take it from there. We can set 0 by default to achieve what you're proposing here. People can set other value if needed.

[maiqueb]

yes, and also, there might be people relying on having a native vlan on the switch. It's a valid option. I'll create a pr in netlink to be able to set a native vlan, then we can take it from there. We can set 0 by default to achieve what you're proposing here. People can set other value if needed.

Makes sense.

@phoracek would it make sense to add yet another knob for the default pvid ? Defaulting to 1, which is the current behavior iiuc ?

[mlguerrero12]

https://github.com/vishvananda/netlink/pull/858

I tested it setting default vlan to 0

[root@localhost plugins]# CNI_PATH=./bin ~/go/bin/cnitool add mynet /var/run/netns/testing { "cniVersion": "0.3.1", "interfaces": [ { "name": "mynet0", "mac": "6e:d4:4c:c8:bc:78" }, { "name": "veth3f8c4a16", "mac": "6e:d4:4c:c8:bc:78" }, { "name": "eth0", "mac": "e2:b6:5e:b7:df:3e", "sandbox": "/var/run/netns/testing" } ], "dns": {} }[root@localhost plugins]# ip a 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000 link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 inet 127.0.0.1/8 scope host lo valid_lft forever preferred_lft forever inet6 ::1/128 scope host valid_lft forever preferred_lft forever 2: enp1s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP group default qlen 1000 link/ether 52:54:00:02:df:2c brd ff:ff:ff:ff:ff:ff inet 192.168.122.20/24 brd 192.168.122.255 scope global dynamic noprefixroute enp1s0 valid_lft 2592sec preferred_lft 2592sec inet6 fe80::5054:ff:fe02:df2c/64 scope link noprefixroute valid_lft forever preferred_lft forever 12: mynet0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000 link/ether 96:6e:60:31:60:1c brd ff:ff:ff:ff:ff:ff inet6 fe80::701c:bfff:fe60:e6f/64 scope link tentative valid_lft forever preferred_lft forever 13: veth3f8c4a16@if7: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master mynet0 state UP group default link/ether fa:24:f5:a9:e3:34 brd ff:ff:ff:ff:ff:ff link-netns testing inet6 fe80::6cd4:4cff:fec8:bc78/64 scope link tentative valid_lft forever preferred_lft forever [root@localhost plugins]# bridge vlan show port vlan-id
veth3f8c4a16 100 PVID Egress Untagged

[phoracek]

@mlguerrero12 +1, I like that approach. Would you like to post a new PR utilizing the new vishvananda feature? I'd be happy to close this one.

@maiqueb I think it would be fair towards users in case any of them utilized this. I would not require it to be in the same PR as the fix though, since I really consider this a bug and an undocumented side-effect

[mlguerrero12]

sure, thanks

[mlguerrero12]

@phoracek, I created https://github.com/containernetworking/plugins/pull/865

[phoracek]

Thanks o/