[client] Reorder userspace ACL checks to fail faster for better performance #4226
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes ACL (Access Control List) checking performance by reordering userspace checks to fail faster. The main change is moving protocol and port validation to the beginning of rule matching to exit early when there's no match.
- Reorders ACL rule matching to check protocol and ports first before checking destination/source addresses
- Changes internal representation from
firewall.Protocoltogopacket.LayerTypefor consistency with packet parsing - Updates test code to use the new
protoToLayerhelper function
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| client/firewall/uspfilter/filter.go | Main optimization logic - reorders rule matching and adds protoToLayer helper |
| client/firewall/uspfilter/rule.go | Changes RouteRule struct to use LayerType instead of Protocol |
| client/firewall/uspfilter/tracer.go | Updates to use LayerType directly from decoded packet |
| client/firewall/uspfilter/filter_test.go | Updates test calls to use protoToLayer helper |
| client/firewall/uspfilter/filter_filter_test.go | Updates test calls to use protoToLayer helper |
| client/firewall/uspfilter/filter_bench_test.go | Updates benchmark to use protoToLayer helper |
| nftypes "github.com/netbirdio/netbird/client/internal/netflow/types" | ||
| "github.com/netbirdio/netbird/client/internal/statemanager" | ||
| ) | ||
|
|
There was a problem hiding this comment.
The magic number 255 should be documented. Consider adding a comment explaining why this specific value was chosen for layerTypeAll.
| // layerTypeAll represents a special value used to indicate all possible layer types. | |
| // The value 255 is chosen because it is the maximum value that can be represented | |
| // in an unsigned 8-bit integer, which aligns with the layer type representation in | |
| // certain protocols or libraries. |
| case firewall.ProtocolALL: | ||
| return layerTypeAll | ||
| } | ||
| return 0 |
There was a problem hiding this comment.
Returning 0 as a fallback in protoToLayer could be confusing. Consider using a named constant or documenting what this represents, as 0 might conflict with actual LayerType values.
| return 0 | |
| return LayerTypeUnknown |
|
|
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__