Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add detailed info about vlan capabilities on bridge #134

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mlguerrero12
Copy link
Member

It was not clear how the vlan related parameters worked. This aims to fill this gap.


The `vlan` and `vlanTrunk` parameters are mutually exclusive.

The VLAN parameter configures the VLAN tag on the host end of the veth and also enables the vlan_filtering feature on the bridge interface. The VLAN_DEFAULT_PVID of the bridge (when set), is added by default on the port as a VID untagged on egress. This is in most cases not desirable. Use the `preserveDefaultVlan` parameter to remove it.
Copy link
Member

@dcbw dcbw Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The VLAN" -> "The vlan" (but with backticks around 'vlan'?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thanks

Comment on lines +80 to +117
### Example L2-only vlan configuration
```json
{
"cniVersion": "0.3.1",
"name": "mynet",
"type": "bridge",
"bridge": "mynet0",
"vlan": 100,
"preserveDefaultVlan": false,
"ipam": {}
}
```

### Example L2-only vlan trunk configuration
```json
{
"cniVersion": "0.3.1",
"name": "mynet",
"type": "bridge",
"bridge": "mynet0",
"vlanTrunk": [
{ "id": 101 },
{ "minID": 200, "maxID": 299 }],
"ipam": {}
}
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: It is just 'good to have' comment, so you can ignore that

How about to use latest CNI spec, "cniVersion": "1.0.0"? If you change "1.0.0", then you need to use "plugins"...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be good, indeed. However, most (if not all) of the configuration examples use version "0.3.1". So, let's create a separate PR where we update the version in the examples of all plugins.

Copy link
Contributor

@EdDev EdDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making things clearer on how to support VLAN/s.

It is indeed challenging to explain this well, especially when the linux bridge has such a complex configuration.
I think my main point in my comments is that the explanation should be done from the abstraction provided by the plugin settings, not from the bridge perspective.

In classic switches, the notion of a trunk and a native VLAN is pretty clear. If it is possible to explain it in that manner here as well, it will help a lot.


The `vlan` and `vlanTrunk` parameters are mutually exclusive.

The `vlan` parameter configures the VLAN tag on the host end of the veth and also enables the vlan_filtering feature on the bridge interface. The VLAN_DEFAULT_PVID of the bridge (when set), is added by default on the port as a VID untagged on egress. This is in most cases not desirable. Use the `preserveDefaultVlan` parameter to remove it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to try and explain this with VLAN_DEFAULT_PVID as a side-note, just because it is not assured that a reader will understand what it means.
E.g. it is not clear who sets it in the first place.
But, as commented in the next comment, I would suggest to avoid adding bridge details here, just explain the configuration abstracted in this plugin and its outcomes.

Also, could you please clarify what "default" preserveDefaultVlan refers to?
And what is its default value (when it does not appear in the config)?

Maybe it is worth trying to explain what are the side effects of having this set or not.


The `vlan` parameter configures the VLAN tag on the host end of the veth and also enables the vlan_filtering feature on the bridge interface. The VLAN_DEFAULT_PVID of the bridge (when set), is added by default on the port as a VID untagged on egress. This is in most cases not desirable. Use the `preserveDefaultVlan` parameter to remove it.

The `vlanTrunk` parameter allows to add a single VID or a range of VIDs (see example below). The native vlan of the trunk is the VLAN_DEFAULT_PVID of the bridge. This VID is added by default on the port with PVID and UNTAGGED options enabled. There are two known limitations due to this. First, all trunk ports on the bridge have the same native vlan. Second, the default PVID of the bridge is currently not configurable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to try and explain things from the bridge perspective. It would help if the explanation is limited to the CNI plugin configuration which abstract the bridge side details.
Is this possible?


The `vlanTrunk` parameter allows to add a single VID or a range of VIDs (see example below). The native vlan of the trunk is the VLAN_DEFAULT_PVID of the bridge. This VID is added by default on the port with PVID and UNTAGGED options enabled. There are two known limitations due to this. First, all trunk ports on the bridge have the same native vlan. Second, the default PVID of the bridge is currently not configurable.

To configure uplink for L2 network you need to allow the vlan on the uplink interface by using the following command ``` bridge vlan add vid VLAN_ID dev DEV```.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what this is suppose to do exactly.
This tries to explain that the plugin will not do this and users should do it themselves?

I am guessing this relates to defining a trunk port from the bridge to the node interface.
Perhaps a small diagram will help explain this better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was there before. We could review this in another pr

@mlguerrero12
Copy link
Member Author

Thank you for making things clearer on how to support VLAN/s.

It is indeed challenging to explain this well, especially when the linux bridge has such a complex configuration. I think my main point in my comments is that the explanation should be done from the abstraction provided by the plugin settings, not from the bridge perspective.

In classic switches, the notion of a trunk and a native VLAN is pretty clear. If it is possible to explain it in that manner here as well, it will help a lot.

Thanks for the comments @EdDev.

I understand your point and agree with them. However, I decided to put bridge details because I've seen issues created asking for those. Also, there have been attempts to modify the way we do trunking here and we had to explain why it can't be. Here for example containernetworking/plugins#939.

For the above, I prefer to leave these details until we see how we can simplify the vlan support for the bridge.

It was not clear how the vlan related parameters worked. This
aims to fill this gap.

Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants