bridge: add vlan trunk support #829

tjjh89017 · 2023-02-13T16:35:35Z

add vlan trunk support for veth
vlan trunk only support L2 only mode without any IPAM
refer ovs-cni design
https://github.com/k8snetworkplumbingwg/ovs-cni/blob/main/pkg/plugin/plugin.go

design:
origin "vlan" option will be PVID or untagged vlan for the network.
"vlanTrunk" will setup tagged vlan for veth.

entry type:
{ "id": 100 } will specify only tagged vlan 100
{ "minID": 100, "maxID": 120 } will specify tagged vlan from 100 to
120 (include 100 and 120)
vlanTrunk is a list of above entry type, so you can use this to add
tagged vlan
[ { "id": 100 }, { "minID": 1000, "maxID": 2000 } ]

complete config will be like this

{
  "cniVersion": "0.3.1",
  "name": "mynet",
  "type": "bridge",
  "bridge": "mynet0",
  "vlan": 100,
  "vlanTrunk": [
    { "id": 101 },
    { "minID": 1000, "maxID": 2000 },
    { "minID": 3000, "maxID": 4000 }
  ],
  "ipam": {}
}

Original PR: #689

Signed-off-by: Date Huang date.huang@suse.com

tjjh89017 · 2023-02-13T16:36:09Z

@squeed Please use this PR for #689
Thanks a lot

squeed · 2023-02-21T12:25:08Z

hey @SchSeba would you mind taking a quick look?

tjjh89017 · 2023-02-21T17:28:53Z

I found the test failed in "portmap" plugin.

SchSeba · 2023-03-28T13:08:05Z

I will take a look on this one.
also @mlguerrero12 if you have time please take a look :)

tjjh89017 · 2023-04-11T06:42:55Z

Hi @squeed and @SchSeba

I tried to fix up all lint issue and rebase to current main branch.
Could you help us to look on this PR?
Thanks a lot

squeed · 2023-04-17T15:53:42Z

@mlguerrero12, who agreed to review this.

maiqueb

some nits.

plugins/main/bridge/bridge.go

mlguerrero12 · 2023-04-19T07:25:06Z

I see a conflict with PVID here that has not been addressed.

When a port has PVID set (regular vlan id here), this becomes an access port. On the other hand, a trunk port has no PVID. Only tagged VIDs and no VID (default vlan). That's a check we need to add on this pr.

So, a use case for vlan trunk is a container with a trunk port on the bridge. This container can use multus for example to create additional vlan subinterfaces. Let's say two for vlan 100 and 200. Then, 3 containers can be created, one with vlan 100, the other one with 200 and the last one with no vlan. The subinterfaces on the first container should be able to talk to the containers with the specific vlan and untagged traffic should be forwarded thanks to the default vlan.

Now, regarding the default vlan, with the new parameter preserveDefaultVlan, it is possible to remove it. For instance, if a user only wants tagged traffic to go through the trunk port.

Hope this makes sense.

tjjh89017 · 2023-04-19T07:40:55Z

I see a conflict with PVID here that has not been addressed.

When a port has PVID set (regular vlan id here), this becomes an access port. On the other hand, a trunk port has no PVID. Only tagged VIDs and no VID (default vlan). That's a check we need to add on this pr.

In most of current switches, they could accept hybrid mode port which allow PVID and trunk at the same time.
The PVID here is called native vlan. (In Cisco switch, "trunk port" means "hybrid port" in other vendor switches.)
So we could have some case here.

pure access port: vlan != 0, preserveDefaultVlan == false, vlanTrunk == nil
pure trunk port: vlan == 0, preserveDefaultVlan == false, vlanTrunk != nil
hybrid port (with native vlan 1): vlan == 0, preserveDefaultVlan == true, vlanTrunk != nil
hybrid port (with special native vlan): vlan != 0, preserveDefaultVlan == false, vlanTrunk != nil

That is the reason I didn't check the vlan value before we configure vlanTrunk.
Users should configure those fields on their purpose.

But it seems I need to check if pvid is in vlanTrunks list, and skip it.

So, a use case for vlan trunk is a container with a trunk port on the bridge. This container can use multus for example to create additional vlan subinterfaces. Let's say two for vlan 100 and 200. Then, 3 containers can be created, one with vlan 100, the other one with 200 and the last one with no vlan. The subinterfaces on the first container should be able to talk to the containers with the specific vlan and untagged traffic should be forwarded thanks to the default vlan.

Now, regarding the default vlan, with the new parameter preserveDefaultVlan, it is possible to remove it. For instance, if a user only wants tagged traffic to go through the trunk port.

Hope this makes sense.

mlguerrero12 · 2023-04-19T08:09:08Z

Exactly, the pvid here is the native vlan, not the one coming from the parameter vlan. From you message, I think you believe the native vlan is configured with this parameter and it is not. We currently don't have a param to modify the native vlan of the bridge.

When we add the pvid (vlan param equal 100), the port has:
1 Egress untagged
100 PVID Egress untagged

Now, if you add trunk port. This will become.

1 Egress untagged
100 PVID Egress untagged
200
300

Native vlan is still 1 (this a configuration of the bridge itself, not the ports of the bridge).

With the above config, untagged ingress traffic will have tag 100 (clearly not the native vlan) and on egress, vid 100 and untagged traffic will output untagged.

I'm familiar with cisco switches as well and I don't remember a config to do this.

tjjh89017 · 2023-04-19T08:43:16Z

Exactly, the pvid here is the native vlan, not the one coming from the parameter vlan. From you message, I think you believe the native vlan is configured with this parameter and it is not. We currently don't have a param to modify the native vlan of the bridge.

When we add the pvid (vlan param equal 100), the port has: 1 Egress untagged 100 PVID Egress untagged

Now, if you add trunk port. This will become.

1 Egress untagged 100 PVID Egress untagged 200 300

Native vlan is still 1 (this a configuration of the bridge itself, not the ports of the bridge).

With the above config, untagged ingress traffic will have tag 100 (clearly not the native vlan) and on egress, vid 100 and untagged traffic will output untagged.

I'm familiar with cisco switches as well and I don't remember a config to do this.

So did it mean that we need to always remove the vid 1 when vlan is presented?

When we add the pvid (vlan param equal 100), the port has:
1 Egress untagged
100 PVID Egress untagged

In this config, vid 1 didn't take any effect and it might have some issue on this port.

Native vlan is still 1 (this a configuration of the bridge itself, not the ports of the bridge).

In this bridge cni, I think we never modify the bridge's vlan config.
but only port's vlan config.
So the native vlan that I mentioned before is always port's native vlan.

With the above config, untagged ingress traffic will have tag 100 (clearly not the native vlan) and on egress, vid 100 and untagged traffic will output untagged.

I'm not really confident that I understand this part.
ingress is ok, follow PVID config.
egress part.
If we have two port eth0 & eth1 and those ports are bridge ports.

eth0: 1 pvid egress untagged
eth1: 100 pvid egress untagged
      1-99

traffic from eth0 to eth1 will be captured with vid 1 tagged in eth1 egress direction.

The only config with problem will be like this

eth0: 1 pvid egress untagged
eth1: 1 egress untagged
      100 pvid egress untagged

traffic from eth0 to eth1 will be captured with untagged in eth1 egress direction.
but traffic from eth1 to eth0 will be catpured with tagged vid 100 in eth0 egress direction (so drop.)

I'm familiar with cisco switches as well and I don't remember a config to do this.

This is my config of my switch.
Cisco didn't have "bridge's vlan config" in Linux.
cisco only have port's vlan config.
So I'm not sure which config that is not in cisco switch.

c2960s

interface GigabitEthernet1/0/48
 description to-4100
 switchport trunk native vlan 101
 switchport trunk allowed vlan 100,101,999
 switchport mode trunk
!

So from my side, I will suggest to remove default vlan always when vlanID is presented.

Stay away from the config as this

// Bad 
1 Egress untagged
100 PVID Egress untagged

// Good (vlanID == 100)
100 PVID Egress untagged

// Good (vlanID == 0)
1 PVID Egress untagged

tjjh89017 · 2023-04-19T08:49:43Z

If we took this cisco config as example to impelment.

interface GigabitEthernet1/0/48
 description to-4100
 switchport trunk native vlan 101
 switchport trunk allowed vlan 100,101,999
 switchport mode trunk
!

vlanID := 101
vlanTrunk := [{ID: 100}, {ID: 101}, {ID: 999}]
BridgeName := br0

In Linux Bridge, we should get something like this below.

br0   1 PVID Egress Untagged
veth0 101 PVID Egress Untagged
      100
      999

without 1 Egress Untagged inside.

I want to ensure that we have the same config translation in this Cisco config.

mlguerrero12 · 2023-04-19T09:14:34Z

If you want to achieve

veth0 101 PVID Egress Untagged
100
999

you need to create a bridge with a default vlan equal to 101 and then add tags 100 and 999 with this pr.

In my view, you want to achieve the same with a hack. You want to add a vlan with PVID (access port) 101 and then delete the default vlan (with preserveDefaultVlan equal to false) and add tags 100 and 999.

Conceptually, this is not correct.

mlguerrero12 · 2023-04-19T09:48:39Z

All the issues come from not having removed the default vlan of the bridge from the ports since day 1.

If we allow users this kind of flexibility, there will be confusion. They can set a different default vlan by setting a PVID and having preserveDefaultVlan equal to false. But this is contradicting, they will have a default vlan but they are explicitly saying that they don't want to preserve the default vlan. It works, but it's not semantically correct.

Another issue, if they leave the default vlan of the bridge and set a PVID. They will have on port:
1 Egress Untagged
101 PVID Untagged

Untagged ingress traffic is fine, it will be tagged with 101, but egress traffic with tag 101 and 1 will be untagged. In a cisco switch, if the default vlan is set to 101, the trunk will not modify any other traffic. Traffic with vlan 1 will have tag 1. This will not be the case here.

In my opinion, we should stick to basic concepts, at least for now to avoid these issues. A trunk port should only allow tagged vids and the native vlan, the one configured on the bridge.

@maiqueb @squeed, what do you think? See my comment above also.

tjjh89017 · 2023-04-19T10:10:34Z

All the issues come from not having removed the default vlan of the bridge from the ports since day 1.

If we allow users this kind of flexibility, there will be confusion. They can set a different default vlan by setting a PVID and having preserveDefaultVlan equal to false. But this is contradicting, they will have a default vlan but they are explicitly saying that they don't want to preserve the default vlan. It works, but it's not semantically correct.

Another issue, if they leave the default vlan of the bridge and set a PVID. They will have on port: 1 Egress Untagged 101 PVID Untagged

Untagged ingress traffic is fine, it will be tagged with 101, but egress traffic with tag 101 and 1 will be untagged. In a cisco switch, if the default vlan is set to 101, the trunk will not modify any other traffic. Traffic with vlan 1 will have tag 1. This will not be the case here.

In my opinion, we should stick to basic concepts, at least for now to avoid these issues. A trunk port should only allow tagged vids and the native vlan, the one configured on the bridge.

@maiqueb @squeed, what do you think? See my comment above also.

As discussion with @mlguerrero12, I think we could make this PR with some limitation and leave something to the future version.
and I will file a ticket to deprecate some current behavior with breaking change to make sure there is no conflict in network scope and vlan/pvid/vids usage.

For now, I will modify the PR to support access port (untagged only) and trunk port (tagged only).
Leaving hybrid port support in the future.

by the way, I will suggest not to use the "default" vlan term.
Even Linux bridge could do some weird config like this [1]
But in current CNI config design, we only allow/accept common switch style config.
I will leave this to the next ticket to fix up and probably we need to remove preserveDefaultVlan option in the future.

[1]:

1 Egress Untagged
101 PVID Untagged

mlguerrero12 · 2023-04-19T11:10:24Z

cool, thanks @tjjh89017

maiqueb

Would it make sense to also unit-test the collectVlanTrunk function ? Especially it's "negative" scenario ? (e.g. check a trunk without maxID but with minID, etc).

plugins/main/bridge/bridge.go

maiqueb · 2023-04-19T11:36:04Z

All the issues come from not having removed the default vlan of the bridge from the ports since day 1.

If we allow users this kind of flexibility, there will be confusion. They can set a different default vlan by setting a PVID and having preserveDefaultVlan equal to false. But this is contradicting, they will have a default vlan but they are explicitly saying that they don't want to preserve the default vlan. It works, but it's not semantically correct.

Another issue, if they leave the default vlan of the bridge and set a PVID. They will have on port: 1 Egress Untagged 101 PVID Untagged

Untagged ingress traffic is fine, it will be tagged with 101, but egress traffic with tag 101 and 1 will be untagged. In a cisco switch, if the default vlan is set to 101, the trunk will not modify any other traffic. Traffic with vlan 1 will have tag 1. This will not be the case here.

In my opinion, we should stick to basic concepts, at least for now to avoid these issues. A trunk port should only allow tagged vids and the native vlan, the one configured on the bridge.

@maiqueb @squeed, what do you think? See my comment above also.

I agree w/ @mlguerrero12 - having a single, crystal clear flow regarding VLANs is preferable.

tjjh89017 · 2023-04-19T11:47:27Z

From my side,

A trunk port should only allow tagged vids and the native vlan, the one configured on the bridge.

I didn't agree with this one in the future.
But currently, it's ok.

plugins/main/bridge/bridge.go

maiqueb

just some opinionated nits.

plugins/main/bridge/bridge.go

maiqueb

missing descriptions in the table Entrys and also missing the "error" scenario for checking collectVlanTrunk fails when provided wrong input.

plugins/main/bridge/bridge_test.go

tjjh89017 · 2023-04-19T14:13:18Z

@maiqueb I think this version of unit test should work.
Please help me to review
Thanks a lot!

github action in my repo

maiqueb

Nice, I'm just missing a few tests.

Also missing tests for the vlanTrunk.ID attribute; I would say:

positive tests: a vlan trunk w/ just the ID
positive tests: a vlan trunk w/ a range + ID
negative test: a a vlan trunk where the ID has a negative value
negative test: a a vlan trunk where the ID == 10000

plugins/main/bridge/bridge_test.go

maiqueb

Thank you.

Please squash all commits into one once the PR is ready.

@mlguerrero12 can you take another look ?

tjjh89017 · 2023-04-19T14:40:12Z

Thank you.

Please squash all commits into one once the PR is ready.

@mlguerrero12 can you take another look ?

I will squash commit in the last moment.
Please notify me if all reviewers accept and it's ready to merge.
Thanks a lot.

mlguerrero12 · 2023-04-19T14:53:39Z

thank you @tjjh89017

add vlan trunk support for veth vlan trunk only support L2 only mode without any IPAM refer ovs-cni design https://github.com/k8snetworkplumbingwg/ovs-cni/blob/main/pkg/plugin/plugin.go design: origin "vlan" option will be PVID or untagged vlan for the network. "vlanTrunk" will setup tagged vlan for veth. entry type: `{ "id": 100 }` will specify only tagged vlan 100 `{ "minID": 100, "maxID": 120 }` will specify tagged vlan from 100 to 120 (include 100 and 120) vlanTrunk is a list of above entry type, so you can use this to add tagged vlan `[ { "id": 100 }, { "minID": 1000, "maxID": 2000 } ]` complete config will be like this { "cniVersion": "0.3.1", "name": "mynet", "type": "bridge", "bridge": "mynet0", "vlan": 100, "vlanTrunk": [ { "id": 101 }, { "minID": 1000, "maxID": 2000 }, { "minID": 3000, "maxID": 4000 } ], "ipam": {} } Signed-off-by: Date Huang <date.huang@suse.com>

tjjh89017 · 2023-04-19T14:55:59Z

Ok thanks all of you guys!
I already squashed all commits into one.
Please check it.
Thanks again

mlguerrero12 · 2023-04-19T15:01:37Z

please don't forget to update the docs, the example sets both vlan and vlantrunk

tjjh89017 · 2023-04-19T15:02:51Z

please don't forget to update the docs, the example sets both vlan and vlantrunk

Sure, I will!

tjjh89017 mentioned this pull request Feb 13, 2023

bridge: add vlan trunk support #689

Closed

tjjh89017 force-pushed the bridge_vlan_trunk branch 8 times, most recently from 5f9df94 to 43a475f Compare April 11, 2023 06:32

tjjh89017 force-pushed the bridge_vlan_trunk branch 3 times, most recently from c85b58a to 534f610 Compare April 18, 2023 07:15

maiqueb reviewed Apr 18, 2023

View reviewed changes

plugins/main/bridge/bridge.go Outdated Show resolved Hide resolved

plugins/main/bridge/bridge.go Show resolved Hide resolved

plugins/main/bridge/bridge.go Show resolved Hide resolved

plugins/main/bridge/bridge.go Outdated Show resolved Hide resolved

mlguerrero12 reviewed Apr 18, 2023

View reviewed changes

plugins/main/bridge/bridge.go Outdated Show resolved Hide resolved

mlguerrero12 reviewed Apr 18, 2023

View reviewed changes

plugins/main/bridge/bridge.go Show resolved Hide resolved

tjjh89017 force-pushed the bridge_vlan_trunk branch 2 times, most recently from 40f87d8 to 5f9ae6e Compare April 18, 2023 15:24

tjjh89017 requested a review from mlguerrero12 April 19, 2023 07:53

maiqueb requested changes Apr 19, 2023

View reviewed changes

plugins/main/bridge/bridge.go Outdated Show resolved Hide resolved

plugins/main/bridge/bridge.go Show resolved Hide resolved

mlguerrero12 reviewed Apr 19, 2023

View reviewed changes

plugins/main/bridge/bridge.go Outdated Show resolved Hide resolved

maiqueb reviewed Apr 19, 2023

View reviewed changes

plugins/main/bridge/bridge.go Show resolved Hide resolved

plugins/main/bridge/bridge.go Show resolved Hide resolved

maiqueb requested changes Apr 19, 2023

View reviewed changes

plugins/main/bridge/bridge_test.go Outdated Show resolved Hide resolved

maiqueb requested changes Apr 19, 2023

View reviewed changes

plugins/main/bridge/bridge_test.go Outdated Show resolved Hide resolved

plugins/main/bridge/bridge_test.go Outdated Show resolved Hide resolved

tjjh89017 force-pushed the bridge_vlan_trunk branch from e185d05 to d869ebd Compare April 19, 2023 14:12

maiqueb requested changes Apr 19, 2023

View reviewed changes

plugins/main/bridge/bridge_test.go Show resolved Hide resolved

maiqueb approved these changes Apr 19, 2023

View reviewed changes

mlguerrero12 approved these changes Apr 19, 2023

View reviewed changes

tjjh89017 force-pushed the bridge_vlan_trunk branch from beae87d to 090af7d Compare April 19, 2023 14:55

tjjh89017 mentioned this pull request Apr 20, 2023

Remove vlanTrunk example with vlanID containernetworking/cni.dev#120

Open

squeed approved these changes Apr 24, 2023

View reviewed changes

dcbw merged commit 9cf1a09 into containernetworking:main Apr 24, 2023
4 of 5 checks passed

maiqueb mentioned this pull request Apr 24, 2023

Bump linux bridge CNI kubevirt/cluster-network-addons-operator#1544

Merged

guangbochen mentioned this pull request Apr 27, 2023

[FEATURE] Add VLAN network for VM to support multiple or all VLANID harvester/harvester#766

Open

phoracek mentioned this pull request Apr 28, 2023

[release-0.85] Bump linux bridge CNI kubevirt/cluster-network-addons-operator#1545

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bridge: add vlan trunk support #829

bridge: add vlan trunk support #829

tjjh89017 commented Feb 13, 2023

tjjh89017 commented Feb 13, 2023

squeed commented Feb 21, 2023

tjjh89017 commented Feb 21, 2023

SchSeba commented Mar 28, 2023

tjjh89017 commented Apr 11, 2023 •

edited

squeed commented Apr 17, 2023

maiqueb left a comment

mlguerrero12 commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023 •

edited

mlguerrero12 commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023 •

edited

tjjh89017 commented Apr 19, 2023 •

edited

mlguerrero12 commented Apr 19, 2023

mlguerrero12 commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023

mlguerrero12 commented Apr 19, 2023

maiqueb left a comment

maiqueb commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023

maiqueb left a comment

maiqueb left a comment

tjjh89017 commented Apr 19, 2023 •

edited

maiqueb left a comment

maiqueb left a comment

tjjh89017 commented Apr 19, 2023

mlguerrero12 commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023

mlguerrero12 commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023

bridge: add vlan trunk support #829

bridge: add vlan trunk support #829

Conversation

tjjh89017 commented Feb 13, 2023

tjjh89017 commented Feb 13, 2023

squeed commented Feb 21, 2023

tjjh89017 commented Feb 21, 2023

SchSeba commented Mar 28, 2023

tjjh89017 commented Apr 11, 2023 • edited

squeed commented Apr 17, 2023

maiqueb left a comment

Choose a reason for hiding this comment

mlguerrero12 commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023 • edited

mlguerrero12 commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023 • edited

tjjh89017 commented Apr 19, 2023 • edited

mlguerrero12 commented Apr 19, 2023

mlguerrero12 commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023

mlguerrero12 commented Apr 19, 2023

maiqueb left a comment

Choose a reason for hiding this comment

maiqueb commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023

maiqueb left a comment

Choose a reason for hiding this comment

maiqueb left a comment

Choose a reason for hiding this comment

tjjh89017 commented Apr 19, 2023 • edited

maiqueb left a comment

Choose a reason for hiding this comment

maiqueb left a comment

Choose a reason for hiding this comment

tjjh89017 commented Apr 19, 2023

mlguerrero12 commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023

mlguerrero12 commented Apr 19, 2023

tjjh89017 commented Apr 19, 2023

tjjh89017 commented Apr 11, 2023 •

edited

tjjh89017 commented Apr 19, 2023 •

edited

tjjh89017 commented Apr 19, 2023 •

edited

tjjh89017 commented Apr 19, 2023 •

edited

tjjh89017 commented Apr 19, 2023 •

edited