Skip to content

Conversation

bleader
Copy link
Contributor

@bleader bleader commented Oct 6, 2025

Previous implementation did not take how VLANs are configured into account, this lead to a pretty hefty change.

VLANs use a fake bridge, which is then included in a parent bridge. While packets remain within this bridge, they are not tagged, so the vlanid cannot be used to match packets in the OVS datapath. The only workaround is to create rules for each port. However, this means that rules for untagged traffic, as previously implemented, will also apply to VLAN ports. Therefore, we must apply rules to each matching port in all cases, and create a rule on the uplink ports that matches accordingly.

Changes summary:

  • Refactored to improve clarity and error handling
  • Added update_args_from_ovs() to dynamically gather bridge, VLAN, and port info from OVS.
  • Changed ip_range to ipRange that XO plugin is using
  • Replaced single rule building with per-port rule, as well as matching on VLAN for uplink ports
  • Updated tests to match the refactor
  • Added tests for update_args_from_ovs()
  • Mocked OVS command calls in tests for more accurate validation.

@bleader bleader requested review from gthvn1 and last-genius October 6, 2025 15:09


def update_args_from_ovs(args):
# get parent bridge to apply ruels to
Copy link
Contributor

Choose a reason for hiding this comment

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

typo "ruels"

Copy link
Contributor

@gthvn1 gthvn1 left a comment

Choose a reason for hiding this comment

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

LGTM. You should probably update the README.md as well.

@last-genius last-genius requested a review from psafont October 7, 2025 09:48
Copy link

@last-genius last-genius left a comment

Choose a reason for hiding this comment

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

Really hard to fully wrap the head around everything here, but given the thoroughness of the tests it looks good to me as well.

Copy link

@psafont psafont left a comment

Choose a reason for hiding this comment

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

Lots of care obviously went here, dudging by the tests and the commit message. Like Andriy, I can't follow all the reasonings, but itś obvious this is an improvement.

Previous implementation did not take how VLANs are configured into
account, this lead to a pretty hefty change.

VLANs use a fake bridge, which is then included in a parent bridge.
While packets remain within this bridge, they are not tagged, so the
vlanid cannot be used to match packets in the OVS datapath. The only
workaround is to create rules for each port. However, this means that
rules for untagged traffic, as previously implemented, will also apply
to VLAN ports. Therefore, we must apply rules to each matching port in
all cases, and create a rule on the uplink ports that matches
accordingly.

Changes summary:
- Refactored to improve clarity and error handling
- Added update_args_from_ovs() to dynamically gather bridge, VLAN, and port info from OVS.
- Changed ip_range to ipRange that XO plugin is using
- Replaced single rule building with per-port rule, as well as matching on VLAN for uplink ports
- Updated tests to match the refactor
- Added tests for update_args_from_ovs()
- Mocked OVS command calls in tests for more accurate validation.

Signed-off-by: David Morel <[email protected]>
@bleader bleader force-pushed the dml-traffic-rules-fixing branch from ba5d447 to 24ad916 Compare October 8, 2025 15:20
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.

4 participants