bgp: Extend route policy matching with match types#3
bgp: Extend route policy matching with match types#3MitchLewis930 wants to merge 1 commit intopr_043_beforefrom
Conversation
Extends the internal route policy API with 3 specific prefix / neighbor match types (any / all / invert) to support more flexible route policies for future developments. Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
There was a problem hiding this comment.
Pull request overview
This PR extends BGP route policy matching capabilities by introducing match types (any, all, invert) for neighbors and prefixes, replacing the previous implicit "any" logic with explicit type-based matching.
Changes:
- Introduced
RoutePolicyMatchTypeenum with any/all/invert options - Refactored
RoutePolicyPrefixMatchand introducedRoutePolicyNeighborMatchstructs with explicit match types - Updated all route policy statement constructions to use the new match structures
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/bgp/types/bgp.go | Introduced match types and restructured neighbor/prefix match structures |
| pkg/bgp/types/utils.go | Updated type names from RoutePolicyPrefixMatch to RoutePolicyPrefix |
| pkg/bgp/types/zz_generated.deepequal.go | Generated DeepEqual methods for new match structures |
| pkg/bgp/types/bgp_test.go | Updated tests to use new match structures and verify match type string representations |
| pkg/bgp/types/test_fixtures.go | Updated test fixtures to use new match structures with explicit types |
| pkg/bgp/gobgp/conversions.go | Updated GoBGP conversions to handle match types and new structures |
| pkg/bgp/manager/reconciler/*.go | Updated reconcilers to create policies with new match structures |
| pkg/bgp/api/conversions.go | Updated API conversions for new match structures |
| pkg/bgp/api/printers.go | Updated table formatting to display match types |
| api/v1/models/*.go | Generated model files for new API structures |
| api/v1/openapi.yaml | Updated OpenAPI schema with new match type definitions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| s.Conditions.PrefixSet = &gobgp.MatchSet{ | ||
| Type: gobgp.MatchSet_ANY, // any of the configured prefixes | ||
| Type: toGoBGPPolicyMatchType(apiStatement.Conditions.MatchNeighbors.Type), |
There was a problem hiding this comment.
The prefix match set is using the neighbor match type instead of the prefix match type. This should be apiStatement.Conditions.MatchPrefixes.Type to correctly apply the prefix matching logic.
| Type: toGoBGPPolicyMatchType(apiStatement.Conditions.MatchNeighbors.Type), | |
| Type: toGoBGPPolicyMatchType(apiStatement.Conditions.MatchPrefixes.Type), |
| PrefixLenMin: 32, | ||
| PrefixLenMax: 32, | ||
| MatchNeighbors: &types.RoutePolicyNeighborMatch{ | ||
| Neighbors: []netip.Addr{netip.MustParseAddr("10.10.10.1")}, |
There was a problem hiding this comment.
Missing Type field in RoutePolicyNeighborMatch. All other instances in this file explicitly set Type to RoutePolicyMatchAny, but this one omits it, which may lead to inconsistent behavior or use of a zero-value type.
| PrefixLenMin: 128, | ||
| PrefixLenMax: 128, | ||
| MatchNeighbors: &types.RoutePolicyNeighborMatch{ | ||
| Neighbors: []netip.Addr{netip.MustParseAddr("10.10.10.1")}, |
There was a problem hiding this comment.
Missing Type field in RoutePolicyNeighborMatch. This instance should include Type: types.RoutePolicyMatchAny for consistency with other neighbor match definitions in this file.
| PrefixLenMin: 32, | ||
| PrefixLenMax: 32, | ||
| MatchNeighbors: &types.RoutePolicyNeighborMatch{ | ||
| Neighbors: []netip.Addr{netip.MustParseAddr("10.10.10.99")}, |
There was a problem hiding this comment.
Missing Type field in RoutePolicyNeighborMatch. This should include Type: types.RoutePolicyMatchAny to maintain consistency with other test cases.
| PrefixLenMin: 128, | ||
| PrefixLenMax: 128, | ||
| MatchNeighbors: &types.RoutePolicyNeighborMatch{ | ||
| Neighbors: []netip.Addr{netip.MustParseAddr("10.10.10.99")}, |
There was a problem hiding this comment.
Missing Type field in RoutePolicyNeighborMatch. This should include Type: types.RoutePolicyMatchAny for consistency.
| return condI.MatchPrefixes.Prefixes[0].PrefixLenMin > condJ.MatchPrefixes.Prefixes[0].PrefixLenMin | ||
| } | ||
| return true | ||
| return false |
There was a problem hiding this comment.
The sort comparison should return true when there are no prefixes to sort, not false. When one or both statements have no prefixes, the sort should maintain their relative order (return true for stable sort), not force them to the end of the list. This breaks the stable sort behavior.
2 participants
PR_043