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

bridges: implement vlans, port-vlans, vlan-filtering and vlan-default-pvid options #540

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

p-strusiewiczsurmacki-mobica
Copy link

@p-strusiewiczsurmacki-mobica p-strusiewiczsurmacki-mobica commented Jan 31, 2025

Description

This PR is a followup to #127 .

As described in #127:
Implements the vlans & port-vlans option keys for bridges, as required by the "NetworkManager readwrite plugin" spec. Currently only supports the nm backend.

I've rebased the old PR onto the current main and it seems to work OK (at least the tests are passing). Additionally added the vlan-filtering bool that can be forced true even if no vlans are defined (as described by @chdxD1 in this comment here.

Right now I am starting to work on networkd backend. However decided to create this draft PR to enable an early review and to just let others know that I am working on this.

I've added vlan-default-pvid to set default PVID for a bridge and networkd backend so i think the PR is ready. :)

make check-coverage fails for me in codestyle test within netplan/_build/meson-private/pycompile.py, which is Meson's file so I believe this is out of scope here, therefore checking make check-coverage in the checklist as passed.

I can also see that GitHub action NetworkManager Autopkgtest / lxd-network-manager is failing due to libc6-dev : Depends: libc6 (= 2.39-0ubuntu8.3) but 2.39-0ubuntu8.4 is to be installed but it also seems to be an issue with Ubuntu repositories I think. I think Ubuntu's repositories were fixed already.

Checklist

  • Runs make check successfully.
  • Retains code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

NM:bridge:vlan: enable vlan-filtering on bridge, if vlans are set
@p-strusiewiczsurmacki-mobica p-strusiewiczsurmacki-mobica changed the title [WiP] bridges: implement vlans, port-vlans and vlan-filtering options [WIP] bridges: implement vlans, port-vlans and vlan-filtering options Jan 31, 2025
Signed-off-by: Patryk Strusiewicz-Surmacki <patryk-pawel.strusiewicz-surmacki@external.telekom.de>
@slyon slyon added the community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. label Feb 6, 2025
@p-strusiewiczsurmacki-mobica p-strusiewiczsurmacki-mobica force-pushed the bridge-vlans branch 2 times, most recently from 6407125 to 02564ed Compare February 6, 2025 16:02
@p-strusiewiczsurmacki-mobica p-strusiewiczsurmacki-mobica marked this pull request as ready for review February 7, 2025 14:01
@p-strusiewiczsurmacki-mobica p-strusiewiczsurmacki-mobica changed the title [WIP] bridges: implement vlans, port-vlans and vlan-filtering options bridges: implement vlans, port-vlans, vlan-filtering and vlan-default-pvid options Feb 7, 2025
@p-strusiewiczsurmacki-mobica
Copy link
Author

p-strusiewiczsurmacki-mobica commented Feb 7, 2025

Additional info: I've changed the code by @slyon a bit as it seems that for both NetworkManager and networkd only one PVID can be specified per interface. Networkd seems to specify this explicity in the docs. NetworkManager does not, but I've tried to configure multiple PVIDs with nmcli on Ubuntu 24.04 and it seems to not accept ranges. So I assume this is what is expected.

The PVID in networkd 'takes an VLAN ID or negative boolean value (e.g. "no")' - the 'negative boolean value' can be achieved with this PR by specifing vlans: [0 pvid] in the netplan configuration.

Signed-off-by: Patryk Strusiewicz-Surmacki <patryk-pawel.strusiewicz-surmacki@external.telekom.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants