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 support for filtering VNTag frames #1480

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

Conversation

stefan-baranoff
Copy link

Add a "vntag" keyword similar to the existing "vlan" and "mpls" keywords to filter on VNTag frames. VNTag was proposed as 802.1Qbh but has been replaced in the standards by 802.1Qbr although some vendors are still using VNTag.

Because VNTag is always followed by VLAN and the following VLAN has the next EtherType, both the VNTag and VLAN have to be present to match and both have to be skipped in processing. So, this commit also adds an optional vlan [vlan_id] after the vntag keyword to easily filter on the VLAN ID following the VNTag.

Fixes #770


First, sorry about having to close the other PR. I had two branches on my side (a misnamed nvtag and this vntag) and just missed on the PR with the wrong one! I had already started some tests in the vntag branch. This PR replaced #1479

The PCAP file is a real PCAP manually adjusted with Scapy to include a VNTag+VLAN header. It parses properly in Wireshark as VNTag, giving it some legitimacy. I do not have direct access to native VNTag traffic but am asking the team I'm helping out if they can get one. I'm not sure it's necessary, though, given Wireshark agreeing:
wireshark-vntag-parse

@stefan-baranoff
Copy link
Author

I think maybe it's worth revisiting if VLAN should be implicit. Re-reading the PCAPs and thinking on this today, the VNTag header is like the VLAN header which gets injected between dest addr and ethertype. I always forget that VLAN (and apparently VNTag) "splits" the ethernet header.

I'll revise this to remove the implicit VLAN and see how it looks. It'll probably still work and be a little simpler (and more explicit, which I think is good).

@stefan-baranoff
Copy link
Author

Updated with implicit VLAN removed; this looks much cleaner!

@infrastation
Copy link
Member

Thank you, please have some patience because there is a number of other work items that need to be resolved and are in progress already.

@infrastation
Copy link
Member

In the next revision of these changes please also add a line to the "packet filtering" section of the CHANGES file.

@stefan-baranoff
Copy link
Author

stefan-baranoff commented Mar 3, 2025

Reminder to self: rebase and squash before merging!

@stefan-baranoff
Copy link
Author

Thank you, please have some patience because there is a number of other work items that need to be resolved and are in progress already.

Completely understood! No pressure or rush! I'm mostly trying to figure out how to contribute meaningfully in your project's style and cadence. I've been walled up in my own team's process for so long, it's been a while since I worked on other teams. Please let me know if there's anything I can do/change to make the process easier/smoother for you.

Thanks for all your hard work maintaining such a great library!

@stefan-baranoff stefan-baranoff force-pushed the vntag-filter branch 2 times, most recently from b4a1cf4 to 18de787 Compare March 12, 2025 04:02
Add a "vntag" keyword similar to the existing "vlan" and "mpls" keywords to filter on VNTag frames. VNTag was proposed as 802.1Qbh but has been replaced in the standards by 802.1Qbr although some vendors are still using VNTag. Skips the 6 bytes of the VNTag header similar to the 4 byte skip of VLAN. They both inject themselves between Ethernet dest addr and EtherType.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

vn-tag support required for bpf
2 participants