Added vlan tag to the bridge cni plugin. #231

SchSeba · 2018-11-18T15:02:17Z

Allow the user to assign VLAN tag.

Updated netlink library to introduce the VlanFiltering variable into the bridge struct

squeed · 2018-11-19T11:02:01Z

Neat. Linux bridges never cease to surprise me :-). Some minor requests:

Can you split this in to two commits, one for the actual code and one for the vendor/ bump?
Can you add some documentation as to what this does? Especially, be clear that this sets the vlan on the container interface (I didn't immediately understand this)

And some larger questions:

Is there a use-case for setting a vlan and not enabling vlanFiltering? Otherwise, I'd rather just delete that knob.
Should we handle the case where an existing bridge doesn't have vlanFiltering enabled? Is that a setting that can be changed on-the-fly? ensureBridge should be as idempotent as reasonably possible.

SchSeba · 2018-11-19T13:24:36Z

@squeed

Can you split this in to two commits, one for the actual code and one for the vendor/ bump?

Done

Can you add some documentation as to what this does? Especially, be clear that this sets the vlan on the container interface (I didn't immediately understand this)

Done

Is there a use-case for setting a vlan and not enabling vlanFiltering? Otherwise, I'd rather just delete that knob.

I can't see a use-case like that, The tag will not work if the vlanFiltering is not configured.

Should we handle the case where an existing bridge doesn't have vlanFiltering enabled? Is that a setting that can be changed on-the-fly? ensureBridge should be as idempotent as reasonably possible.

I think we can always enable the vlanFiltering. untag traffic will work unrelated if the feature is enable or not.

squeed · 2018-11-21T21:01:42Z

So, we talked about this at the CNI maintainers meeting, and this came up: If a container is started in a vlan, how does it get connectivity? Presumably there's no equivalent vlan-100 "uplink" interface on the "host" side of the bridge (i.e. "br0.100").

What are you envisioning? Do you have a separate step that creates the "br0.100" interface? Or do you expect non-default vlans to be private / isolated?

It's generally expected, but by no means required, that a CNI plugin results in a container being "online." Of course, these are loose definitions, but they are helpful to guide decisions.

Should this plugin also create the "br0.100" interface if configured with a vlan?

phoracek · 2018-11-22T11:11:51Z

@squeed forgive me, I don't know much about setting of VLAN directly on a bridge, but my understanding was this:

If I connect two containers on the same host to bridge+vlan, they would be able to reach each other.

If I create a bridges on two nodes and connect them via a trunk interface. Then containers connected to this bridge+vlan can reach each other node-to-node.

Is that right or is more needed?

dankenigsberg · 2018-11-22T16:31:58Z

So, we talked about this at the CNI maintainers meeting, and this came up: If a container is started in a vlan, how does it get connectivity? Presumably there's no equivalent vlan-100 "uplink" interface on the "host" side of the bridge (i.e. "br0.100").

If properly set, the bridge has one trunk uplink (e.g. eth0) and multiple ports, one per Pod. Traffic arriving to eth0 tagged with vlanX is repeated (under usual bridge rules) to all ports belonging to this vlan, but with the tag stripped. The reverse happen to egress traffic.

So if I get what you're asking, the bridge port itself plays the role of br0.100 vlan device. They tag egress and strip ingress in the same way.

(/me slightly rephrasing @phoracek)

dankenigsberg · 2018-11-22T20:09:02Z

I don't know how it usually works on github, but @SchSeba maybe you can start you commit message by explaining what bridge vlan filtering is (include https://developers.redhat.com/blog/2017/09/14/vlan-filter-support-on-bridge/ ) and why it is useful here.

SchSeba · 2018-11-26T15:06:50Z

@squeed

Should this plugin also create the "br0.100" interface if configured with a vlan?

The code to allow the vlan to be tagged on the bridge interface. there is not need to create a br0.100 interface.

if n.Vlan != 0 {
		err = netlink.BridgeVlanAdd(br, uint16(n.Vlan), false, true, true, false)
		if err != nil {
			return nil, nil, fmt.Errorf("failed to setup vlan tag on the bridge interface %q: %v", br.Name, err)
		}
	}

SchSeba · 2018-12-04T14:15:09Z

@squeed there is any update on this PR?

squeed · 2018-12-12T17:43:45Z

Then I still don't understand it. Are you manually trunking in another physical interface? If I want to send a packet from the host to a container on vlan 100, how do I do it?

SchSeba · 2018-12-12T18:26:34Z

Hi @squeed Thanks for the comment.

So if the want so send a packet you need to allow the vlan on the bridge interface and configure ip address in the same subnet on that interface.

dankenigsberg · 2018-12-13T13:26:44Z

Maybe a little diagram would help explain what we'd like to achieve. The suggested change is aimed to allow multiple Pods, each one connected to a possibly-different vlan, share access to NIC that is connected to a trunk port of an external switch. If the node needs access to a vlan, it can connect to it just like any other namespace.

@squeed am I addressing your question?

    +---------------------------------------------------------------------+
    | node1                                                               |
    |                               +------------------+                  |        
    |    +---------+                |Pod3              |                  |        
    |    |Pod2     |                |                  |                  |        
    |    |         |     +----------+                  |                  |        
    |    |         |     |          |                  |                  |        
    |    |         |     |          +------------------+                  |        
    |    |         |     |                            +---------------+   |
    |    |         |     | vlan300                    | Pod1          |   |    
    |    +----+----+     |           +----------------+               |   |    
    |         |          |           |                |               |   |    
    |         |          |           | vlan200        |               |   |    
    |         |          |           |                +---------------+   |
    |         |          |           |                                    |        
    |         |     +----+-----------+--+                                 |        
    |         |     |                   |                                 |        
    |         |     |                   |                                 |        
    |         +-----+                   |                                 |        
    |    vlan200    |        br0        +---------+                       |        
    |               |                   |    vlan200                      |        
    |               +---------+---------+                                 |        
    |                         |                                           |
    |                         |eth7                                       |
    +---------------------------------------------------------------------+
                              |
                              |  trunk 200,300

rosenhouse · 2018-12-13T16:07:10Z

plugins/main/bridge/bridge.go

 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to create bridge %q: %v", n.BrName, err)
 	}

+	if n.Vlan != 0 {
+		err = netlink.BridgeVlanAdd(br, uint16(n.Vlan), false, true, true, false)


I don't see this vlan filter entry being removed on DEL. Are there any downsides to leaving it in place on the bridge even after all veths tagged with the vid have been removed?

Yeah I don't remove it from the bridge I can't find any good reason to remove it just link we are not removing the bridge if there is no veths connected to it.

rosenhouse · 2018-12-13T16:09:17Z

plugins/main/bridge/bridge_test.go

@@ -171,6 +180,7 @@ var counter uint
 // arguments for a test case.
 func (tc testCase) createCmdArgs(targetNS ns.NetNS, dataDir string) *skel.CmdArgs {
 	conf := tc.netConfJSON(dataDir)
+	fmt.Println(conf)


did you intend to leave this in the testCase?

Nope my bad sorry

SchSeba · 2019-01-09T11:34:39Z

Hi @squeed there is something that prevent this PR from be merged?

dcbw

mostly /lgtm
Just a couple README nits.

dcbw · 2019-01-09T15:53:49Z

plugins/main/bridge/README.md

@@ -52,3 +52,8 @@ If the bridge is missing, the plugin will create one on first use and, if gatewa
 * `hairpinMode` (boolean, optional): set hairpin mode for interfaces on the bridge. Defaults to false.
 * `ipam` (dictionary, required): IPAM configuration to be used for this network. For L2-only network, create empty dictionary.
 * `promiscMode` (boolean, optional): set promiscuous mode on the bridge. Defaults to false.
+* `vlan` (int, optional): assign VLAN tag. Defaults to none.
+
+*Note:* The VLAN parameter config the VLAN tag on the host end of the veth and also enable the vlan_filtering feature on the bridge interface.


"config" -> "configures"
"enable" -> "enables"

Done Thanks!

dcbw · 2019-01-09T15:55:21Z

plugins/main/bridge/README.md

+
+*Note:* The VLAN parameter config the VLAN tag on the host end of the veth and also enable the vlan_filtering feature on the bridge interface.
+
+*Note:* To configure up link for L2 network you need to allow the vlan on the up link interface by using the follow command ``` bridge vlan add vid VLAN_ID dev DEV``` 


"up link" -> "uplink"
"follow" -> "following"

And I assume DEV here is like eth0 or whatever the hosts physical NIC would usually be?

And I assume DEV here is like eth0 or whatever the hosts physical NIC would usually be?

Correct. A better text would be

To configure up link for L2 network you need to allow the vlan on the up link interface DEV by using the follow command

or maybe use the blog example of bridge vlan add dev bond0 vid 2 master

dcbw · 2019-01-09T16:06:36Z

So if the want so send a packet you need to allow the vlan on the bridge interface and configure ip address in the same subnet on that interface.

This isn't quite clear enough to me... what would be required on the host side to send a packet to a container on a given VLAN? Would you need to create another veth and attach it to the bridge using the same VLAN ID as the container?

SchSeba · 2019-01-10T08:24:43Z

This isn't quite clear enough to me... what would be required on the host side to send a packet to a container on a given VLAN? Would you need to create another veth and attach it to the bridge using the same VLAN ID as the container?

you don't need to create any additional interface, the bridge interface will get the vlan ID.
The bridge also will have ip address if this is a L3 connection just like before.
Then the host can send packets using the bridge interface to the container because they both have the same vlan ID.

squeed · 2019-01-16T15:18:39Z

I tested this with two configurations, same bridge:

{
        "cniVersion": "0.3.1",
        "name": "mybridge",
        "type": "bridge",
        "ipMasq": true,
        "ipam": {
                "type": "host-local",
                "subnet": "10.42.0.0/24"
        },
        "isDefaultGateway": true,
        "hairpinMode": true
}

and

{
        "cniVersion": "0.3.1",
        "name": "myvlanbridge",
        "type": "bridge",
        "ipMasq": true,
        "ipam": {
                "type": "host-local",
                "subnet": "10.52.0.0/24"
        },
        "isDefaultGateway": true,
        "hairpinMode": true,
        "vlan": 99
}

and got the following error:

$ sudo CNI_PATH=./bin ../cni/bin/cnitool add myvlanbridge /var/run/netns/vlan100 
failed to set bridge addr: "cni0" already has an IP address different from 10.52.0.1/24

squeed · 2019-01-23T15:01:21Z

Just to follow up on my previous comment: I would expect setting the vlan property on the bridge to work, even when users expect layer3 connectivity.

If I understand your use-case correctly, you only really care about layer2 connectivity, so that's not an issue. However, I would really like the plugins to continue to meet user expectations, and it's not at all clear to me as an end-user why adding vlan tag would require me to drop layer3.

So I'd like you to come up with a way for containers to have both a vlan tag and layer3 connectivity. If this means you have to create bridgename.100 on the host or not, I leave up to you.

squeed · 2019-02-20T16:57:00Z

Hi @SchSeba,
You are right, and my understanding of the spec was wrong (to be fair, the other maintainers were of the same opinion). I think you're right, and we should not include the IP addresses of the host-side interface. I'm very sorry for the confusion.

SchSeba · 2019-02-21T09:35:39Z

Hi @squeed thanks for the comment just to be sure you want me to remove the cni0.99 interface from the result.interfaces list?

SchSeba · 2019-02-26T15:41:48Z

@squeed ping?

dcbw · 2019-03-05T19:05:32Z

Hi @squeed thanks for the comment just to be sure you want me to remove the cni0.99 interface from the result.interfaces list?

@SchSeba not quite; leave the cni0.99 interface in the Interfaces array. But when there is an L3 address for cni0 or cni0.99 we don't need to include that in the IPs array because the existing bridge plugin does not include the cni0 address there either.

So I think what you have now is what Casey is asking for.

SchSeba · 2019-03-06T08:02:36Z

Thanks for the comment @dcbw so I think this PR is ready to get merge :)

dcbw · 2019-03-08T17:13:15Z

@SchSeba Casey is on PTO today but will do a final check on Monday.

dankenigsberg · 2019-03-13T13:16:23Z

@dcbw @squeed happy Wednesday!

SchSeba · 2019-03-21T10:09:41Z

Hi @dcbw @squeed any update on this PR?

SchSeba · 2019-03-21T13:34:21Z

Hi @rosenhouse maybe you can help me get this PR merge?

bboreham

Code looks OK; if Casey is ok with it I am too.

squeed · 2019-04-04T12:08:20Z

Code LGTM. Can you squash in to two commits (vendor and code), and I'll merge?

SchSeba · 2019-04-04T14:46:50Z

Hi @squeed please take a look on my last commit I was requested to add some lines to the test to fix them after the rebase.
If that's fine from your side I will squash it.

SchSeba · 2019-04-09T06:27:51Z

Hi @squeed any update?

squeed · 2019-04-10T15:53:55Z

@SchSeba Gotcha. looks good from my side. Squash this and I'll merge it :-)

With the VLAN filter, the Linux bridge acts more like a real switch, Allow to tag and untag vlan id's on every interface connected to the bridge. This PR also creates a veth interface for the bridge vlan interface on L3 configuration. Related to https://developers.redhat.com/blog/2017/09/14/vlan-filter-support-on-bridge/ post. Note: This feature was introduced in Linux kernel 3.8 and was added to RHEL in version 7.0.

SchSeba · 2019-04-10T19:14:42Z

@squeed done and also the tests looks good :)

rootfs · 2020-07-17T14:06:59Z

plugins/main/bridge/bridge.go

+	vlanFiltering := false
+	if n.Vlan != 0 {
+		vlanFiltering = true
+	}


what if vlan filtering is enabled in the begining (for those with Vlan IDs), and a new vlan, without vlan ID, is to be created. Wouldn't the new vlan disable vlan filtering?

CJCShadowsan · 2020-11-11T09:53:54Z

I know this sounds like an obvious question, but...

...How does this work?

I've tried creating a bridge interface but can't get pod-to-host networking working over VLANs.

dankenigsberg · 2020-11-11T10:05:16Z

I know this sounds like an obvious question, but...

...How does this work?

I've tried creating a bridge interface but can't get pod-to-host networking working over VLANs.

I believe you must first connect the host to a bridge port with the relevant Vlan ID enabled. You should be able to do so by running this CNI in the host networking namespace and set an IP address on the free veth.

CJCShadowsan · 2020-11-11T15:11:29Z

I know this sounds like an obvious question, but...
...How does this work?
I've tried creating a bridge interface but can't get pod-to-host networking working over VLANs.

I believe you must first connect the host to a bridge port with the relevant Vlan ID enabled. You should be able to do so by running this CNI in the host networking namespace and set an IP address on the free veth.

Ok - so my setup:

Host:

1x bond device
4x vlan devices (on 4 VLANS) attached to bond0 (bond0.101, bond0.102, etc)

Other hosts:

1x bond device
VLAN device bond0.101
IPMI device on VLAN 102

Via multus:

apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
  name: br1-vl101
spec:
  config: '{
      "cniVersion": "0.4.0",
      "type": "bridge",
      "bridge": "br1",
      "vlan": 101,
      "ipam": {
        "type": "host-local",
        "subnet": "10.8.0.0/24",
        "rangeStart": "10.8.0.220",
        "rangeEnd": "10.8.0.220"
      }
    }'

When I attach this to a POD (incidentally a KubeVirt instance), the address comes up and I see the device show up on a bridge vlan command on the physical host... But can't ping a 10.8.0.5 address which is on the same VLAN, on a physical host (not the host this Pod is running on).

Yet... with the real bond0.101 device I can ping 10.8.0.5.

What am I missing? I was hoping there would be some sort of example to make this work considering this merge request enables a VLAN tag on a bridge plugin but i'm just coming up agonisingly short.

dankenigsberg · 2020-11-11T16:01:47Z

Kudos for using KubeVirt ;-) You're most welcome to share your use case on kubevirt-dev

Can you ping between two pods on the same node over their br1-vl101 network? You should!
I don't see how you connect br1 to the external world. I assume it is connected to bond0. IIRC Linux vlan devices such as your bond0.101 grab ethernet frames with the 101 tag before they reach to a bridge connected to bond0. To let the both host and bridge see bidirectional vlan 101 traffic, I suggest again that you create a veth pair, with one side attached to a bridge port tagged with 101 and the other with ip address, both on the host network namespace.

If my guess is off, you'd need a bit of tcpdump sleuthing to find out where your packets are lost.

CJCShadowsan · 2020-11-11T17:15:00Z

I'm betting this is where i'm falling down here. So there's more that needs to be done in order to make this work? Lol!

So - dummies guide - If we just use the physical bond, bond0 (which carries all VLANS):

brctl addbr br1 bond0
ip link set br1 type bridge vlan_filtering 1
ip link set bond0 type bridge vlan_filtering 1
bridge vlan add dev bond0 vid 101 master

apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
  name: br1-vl101
spec:
  config: '{
      "cniVersion": "0.4.0",
      "type": "bridge",
      "bridge": "br1",
      "master": "bond0",
      "vlan": 101,
      "ipam": {
        "type": "host-local",
        "subnet": "10.8.0.0/24",
        "rangeStart": "10.8.0.220",
        "rangeEnd": "10.8.0.220"
      }
    }'

This creates a veth with VLAN 101, bond0 bridge carries 101... And br1 gets created that links to bond0 if it's created. Is that correct?

gaby · 2021-07-20T04:19:55Z

Can someone provide an example of how is this suppose to work? We can use docker ipvlan and get vlan tagged traffic. When using this plugin we can't ping/reach anything using vlans.

SchSeba force-pushed the add-vlan-tag-to-bridge branch from d1d2fca to 9ff5e57 Compare November 19, 2018 13:02

SchSeba force-pushed the add-vlan-tag-to-bridge branch 4 times, most recently from db0fcec to f81be1f Compare November 26, 2018 14:37

rosenhouse reviewed Dec 13, 2018

View reviewed changes

SchSeba force-pushed the add-vlan-tag-to-bridge branch from f81be1f to a1438fc Compare December 13, 2018 19:22

dcbw mentioned this pull request Dec 19, 2018

cni vlan plugin not suppor 2 pod #243

Closed

dcbw reviewed Jan 9, 2019

View reviewed changes

SchSeba force-pushed the add-vlan-tag-to-bridge branch from a1438fc to a9ab503 Compare January 10, 2019 08:25

SchSeba force-pushed the add-vlan-tag-to-bridge branch 3 times, most recently from 22df5b9 to 0502f71 Compare February 3, 2019 18:55

SchSeba mentioned this pull request Apr 1, 2019

VM using Multus with DHCP not able to renew lease kubevirt/kubevirt#2163

Closed

bboreham approved these changes Apr 3, 2019

View reviewed changes

SchSeba force-pushed the add-vlan-tag-to-bridge branch from 19363f7 to 67c1036 Compare April 4, 2019 12:15

vendor folder bump.

dc53699

SchSeba force-pushed the add-vlan-tag-to-bridge branch from 67c1036 to 58a7125 Compare April 4, 2019 14:33

squeed added this to the v0.8.0 milestone Apr 10, 2019

SchSeba force-pushed the add-vlan-tag-to-bridge branch from 58a7125 to b69a2dd Compare April 10, 2019 19:00

dcbw merged commit e9e1d37 into containernetworking:master Apr 11, 2019

rootfs reviewed Jul 17, 2020

View reviewed changes


		Note: The VLAN parameter config the VLAN tag on the host end of the veth and also enable the vlan_filtering feature on the bridge interface.

		Note: To configure up link for L2 network you need to allow the vlan on the up link interface by using the follow command ``` bridge vlan add vid VLAN_ID dev DEV```

Added vlan tag to the bridge cni plugin. #231

Added vlan tag to the bridge cni plugin. #231

Conversation

SchSeba commented Nov 18, 2018 • edited Loading

squeed commented Nov 19, 2018

SchSeba commented Nov 19, 2018

squeed commented Nov 21, 2018 • edited Loading

phoracek commented Nov 22, 2018

dankenigsberg commented Nov 22, 2018

dankenigsberg commented Nov 22, 2018

SchSeba commented Nov 26, 2018

SchSeba commented Dec 4, 2018

squeed commented Dec 12, 2018

SchSeba commented Dec 12, 2018

dankenigsberg commented Dec 13, 2018

rosenhouse Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchSeba commented Jan 9, 2019

dcbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbw commented Jan 9, 2019

SchSeba commented Jan 10, 2019

squeed commented Jan 16, 2019

squeed commented Jan 23, 2019

squeed commented Feb 20, 2019 • edited Loading

SchSeba commented Feb 21, 2019

SchSeba commented Feb 26, 2019

dcbw commented Mar 5, 2019

SchSeba commented Mar 6, 2019

dcbw commented Mar 8, 2019

dankenigsberg commented Mar 13, 2019 • edited Loading

SchSeba commented Mar 21, 2019

SchSeba commented Mar 21, 2019

bboreham left a comment

Choose a reason for hiding this comment

squeed commented Apr 4, 2019

SchSeba commented Apr 4, 2019

SchSeba commented Apr 9, 2019

squeed commented Apr 10, 2019

SchSeba commented Apr 10, 2019

Choose a reason for hiding this comment

CJCShadowsan commented Nov 11, 2020

dankenigsberg commented Nov 11, 2020

CJCShadowsan commented Nov 11, 2020

dankenigsberg commented Nov 11, 2020

CJCShadowsan commented Nov 11, 2020

gaby commented Jul 20, 2021

SchSeba commented Nov 18, 2018 •

edited

Loading

squeed commented Nov 21, 2018 •

edited

Loading

rosenhouse Dec 13, 2018 •

edited

Loading

squeed commented Feb 20, 2019 •

edited

Loading

dankenigsberg commented Mar 13, 2019 •

edited

Loading