Skip to content

bgp/policy: fix match-prefix-set to accept covered sub-prefixes#122

Open
kroudy wants to merge 1 commit into
holo-routing:masterfrom
kroudy:fix/bgp-prefix-set-containment
Open

bgp/policy: fix match-prefix-set to accept covered sub-prefixes#122
kroudy wants to merge 1 commit into
holo-routing:masterfrom
kroudy:fix/bgp-prefix-set-containment

Conversation

@kroudy

@kroudy kroudy commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

The MatchPrefixSet condition currently requires exact network-address equality between the candidate prefix and the prefix-set entry's ip-prefix. This rejects sub-prefixes that are tree-wise covered by the entry's ip-prefix even when their length falls within the mask-length range.

The ietf-routing-policy YANG module shipped with Holo (published as RFC 9067, "A YANG Data Model for Routing Policy") describes the prefix grouping as expressing a mask-length range for a covering ip-prefix. The YANG text's worked example is ambiguous on whether "range" means literal same-network-address-with-varying-length or tree-wise containment, but peer implementations converge on the containment reading: FRR (lib/plist.c), BIRD (filter/tree.c), Cisco IOS ip prefix-list ... le N, and Juniper prefix-list-filter all treat the entry as a covering pattern. That is also what operators reach for by default, so containment is the principle of least surprise.

Switch to IpNetwork::contains(prefix.ip()), which returns true when the candidate's network address lies within the range's covering prefix. The match logic lives as a PrefixSet::matches method in holo-utils so it can be unit-tested directly and reused by other protocol crates; holo-bgp's MatchPrefixSet condition calls set.matches(prefix).

Semantics change disclosure: this is strictly more permissive. Existing mask-length-lower == mask-length-upper entries (the IETF "exact-match" pattern, which the existing topo1-1 conformance tests use) are unaffected because the length constraint still rules out any prefix whose length differs from the anchor. Ranged entries start matching covered sub-prefixes as intended. Non-canonical ip-prefix entries (stored verbatim by the northbound) are now interpreted as their covering network rather than matching only a literal host IP; this is consistent with how FRR/BIRD/Cisco/Juniper treat the same input.

Nine unit tests cover exact-length entries, ranged entries, length-bound rejection, supernet and disjoint rejection, multi-entry OR semantics, IPv6 parity, the 0.0.0.0/0 [0, 32] match-any pattern, empty prefix-sets, and non-canonical anchor behavior.

@rwestphal rwestphal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@kroudy This fix looks good to me. Ideally, we could move prefix_set_contains into a method of PrefixSet, renamed to PrefixSet::matches, so that other protocol crates can reuse it in the future. What do you think?

The MatchPrefixSet condition currently requires exact network-address
equality between the candidate prefix and the prefix-set entry's
ip-prefix. This rejects sub-prefixes that are tree-wise covered by
the entry's ip-prefix even when their length falls within the
mask-length range.

The ietf-routing-policy YANG module shipped with Holo (published as
RFC 9067, "A YANG Data Model for Routing Policy") describes the
`prefix` grouping as expressing a mask-length range for a covering
ip-prefix. The YANG text's worked example is ambiguous on whether
"range" means literal same-network-address-with-varying-length or
tree-wise containment, but peer implementations converge on the
containment reading: FRR (lib/plist.c), BIRD (filter/tree.c), Cisco
IOS `ip prefix-list ... le N`, and Juniper `prefix-list-filter`
all treat the entry as a covering pattern. That is also what
operators reach for by default, so containment is the principle of
least surprise.

Switch to IpNetwork::contains(prefix.ip()), which returns true when
the candidate's network address lies within the range's covering
prefix. The match logic lives as a `PrefixSet::matches` method in
holo-utils so it can be unit-tested directly and reused by other
protocol crates; holo-bgp's MatchPrefixSet condition calls
`set.matches(prefix)`.

Semantics change disclosure: this is strictly more permissive.
Existing `mask-length-lower == mask-length-upper` entries (the
IETF "exact-match" pattern, which the existing topo1-1 conformance
tests use) are unaffected because the length constraint still rules
out any prefix whose length differs from the anchor. Ranged entries
start matching covered sub-prefixes as intended. Non-canonical
ip-prefix entries (stored verbatim by the northbound) are now
interpreted as their covering network rather than matching only a
literal host IP; this is consistent with how FRR/BIRD/Cisco/Juniper
treat the same input.

Nine unit tests cover exact-length entries, ranged entries,
length-bound rejection, supernet and disjoint rejection, multi-entry
OR semantics, IPv6 parity, the `0.0.0.0/0 [0, 32]` match-any
pattern, empty prefix-sets, and non-canonical anchor behavior.
@kroudy kroudy force-pushed the fix/bgp-prefix-set-containment branch from 27a561a to 1f4ce5c Compare June 1, 2026 02:02
@kroudy

kroudy commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Good call — done. The logic now lives as PrefixSet::matches(&self, prefix: &IpNetwork) -> bool in holo-utils/src/policy.rs, right next to the PrefixSet definition, so any protocol crate can call set.matches(prefix). holo-bgp's MatchPrefixSet arm just calls set.matches(prefix) now and drops the now-unused PrefixSet import. The nine unit tests moved with it into holo-utils and exercise the method directly.

Verified: cargo test -p holo-utils (9 prefix-set tests) and cargo test -p holo-bgp (20) both green, and cargo build --release --bin holod --features "isis,ospf,bgp,bfd,ldp,rip" succeeds. Amended into the single commit and force-pushed.

@kroudy kroudy requested a review from rwestphal June 13, 2026 22:50
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.

2 participants