From 1f4ce5cef9c862d4ec883dad36f74f231d10db40 Mon Sep 17 00:00:00 2001 From: kroudy Date: Tue, 21 Apr 2026 02:15:19 +0000 Subject: [PATCH] bgp/policy: fix match-prefix-set to accept covered sub-prefixes 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. --- holo-bgp/src/policy.rs | 8 +- holo-utils/src/policy.rs | 174 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+), 7 deletions(-) diff --git a/holo-bgp/src/policy.rs b/holo-bgp/src/policy.rs index ee1770207..01f7908e7 100644 --- a/holo-bgp/src/policy.rs +++ b/holo-bgp/src/policy.rs @@ -110,8 +110,6 @@ pub(crate) fn redistribute_apply( }); } -// ===== helper functions ===== - // Processes routing policies for a specific route and returns the policy // result. fn process_policies( @@ -182,11 +180,7 @@ fn process_stmt_condition( PolicyCondition::MatchPrefixSet(value) => { let af = prefix.address_family(); match match_sets.prefixes.get(&(value.clone(), af)) { - Some(set) => set.prefixes.iter().any(|range| { - prefix.ip() == range.prefix.ip() - && prefix.prefix() >= range.masklen_lower - && prefix.prefix() <= range.masklen_upper - }), + Some(set) => set.matches(prefix), None => false, } } diff --git a/holo-utils/src/policy.rs b/holo-utils/src/policy.rs index 5522f3e55..88ef749a4 100644 --- a/holo-utils/src/policy.rs +++ b/holo-utils/src/policy.rs @@ -153,6 +153,31 @@ pub struct PrefixSet { pub prefixes: BTreeSet, } +impl PrefixSet { + // Returns `true` if `prefix` matches any range in the set. + // + // A range matches when the candidate `prefix` is tree-wise covered by + // the range's `ip-prefix` AND the candidate's length falls within + // `[masklen_lower, masklen_upper]`. + // + // Containment (`IpNetwork::contains(prefix.ip())`) is the test used by + // FRR (`lib/plist.c`), BIRD (`filter/tree.c`), Cisco IOS + // `ip prefix-list`, and Juniper `prefix-list-filter`: a more-specific + // prefix matches a less-specific covering prefix when its network + // address lies within that covering prefix. RFC 9067 ("A YANG Data + // Model for Routing Policy") ยง3.3 and the `ietf-routing-policy` module + // (grouping `prefix`) use `ip-prefix` plus a mask-length range to + // express the covering pattern; the range reduces to an exact-length + // match when `masklen_lower == masklen_upper`. + pub fn matches(&self, prefix: &IpNetwork) -> bool { + self.prefixes.iter().any(|range| { + range.prefix.contains(prefix.ip()) + && prefix.prefix() >= range.masklen_lower + && prefix.prefix() <= range.masklen_upper + }) + } +} + // List of IPv4 or IPv6 neighbors that can be matched in a routing policy. #[derive(Clone, Debug)] #[derive(Deserialize, Serialize)] @@ -756,3 +781,152 @@ impl BgpEqOperator { } } } + +// ===== tests ===== + +#[cfg(test)] +mod tests { + use std::collections::BTreeSet; + use std::str::FromStr; + + use super::*; + + fn net(s: &str) -> IpNetwork { + IpNetwork::from_str(s).unwrap() + } + + fn range(cidr: &str, lower: u8, upper: u8) -> IpPrefixRange { + IpPrefixRange { + prefix: net(cidr), + masklen_lower: lower, + masklen_upper: upper, + } + } + + fn set_from(ranges: impl IntoIterator) -> PrefixSet { + let mut prefixes = BTreeSet::new(); + prefixes.extend(ranges); + PrefixSet { + name: "TEST".to_owned(), + mode: AddressFamily::Ipv4, + prefixes, + } + } + + #[test] + fn exact_length_matches_only_that_length() { + let set = set_from([range("192.0.2.0/24", 24, 24)]); + assert!(set.matches(&net("192.0.2.0/24"))); + // Same covering prefix, different length โ€” rejected by length bound. + assert!(!set.matches(&net("192.0.2.0/25"))); + // Sub-network with same length, different network-address โ€” rejected. + assert!(!set.matches(&net("192.0.2.0/26"))); + // Completely unrelated network. + assert!(!set.matches(&net("198.51.100.0/24"))); + } + + #[test] + fn range_accepts_covered_subprefixes() { + // `203.0.113.0/24 [24, 32]` must accept any prefix tree-wise + // covered by 203.0.113.0/24 whose length is in [24, 32]. + let set = set_from([range("203.0.113.0/24", 24, 32)]); + assert!(set.matches(&net("203.0.113.0/24"))); + assert!(set.matches(&net("203.0.113.0/25"))); + assert!(set.matches(&net("203.0.113.128/25"))); + assert!(set.matches(&net("203.0.113.0/26"))); + assert!(set.matches(&net("203.0.113.64/27"))); + assert!(set.matches(&net("203.0.113.1/32"))); + } + + #[test] + fn range_rejects_length_outside_bounds() { + let set = set_from([range("203.0.113.0/24", 26, 28)]); + // Length below `masklen_lower`. + assert!(!set.matches(&net("203.0.113.0/24"))); + assert!(!set.matches(&net("203.0.113.0/25"))); + // Length inside bounds. + assert!(set.matches(&net("203.0.113.0/26"))); + assert!(set.matches(&net("203.0.113.0/27"))); + assert!(set.matches(&net("203.0.113.0/28"))); + // Length above `masklen_upper`. + assert!(!set.matches(&net("203.0.113.0/29"))); + assert!(!set.matches(&net("203.0.113.0/32"))); + } + + #[test] + fn range_rejects_supernets_and_disjoint_prefixes() { + let set = set_from([range("203.0.113.0/24", 8, 32)]); + // Supernet of the covering prefix โ€” not contained. + assert!(!set.matches(&net("203.0.0.0/16"))); + assert!(!set.matches(&net("203.0.112.0/23"))); + // Sibling /24 at the same tree depth. + assert!(!set.matches(&net("203.0.112.0/24"))); + // Completely disjoint address space. + assert!(!set.matches(&net("198.51.100.0/24"))); + } + + #[test] + fn multiple_entries_match_any() { + let set = set_from([ + range("192.0.2.0/24", 24, 24), + range("203.0.113.0/24", 24, 32), + ]); + assert!(set.matches(&net("192.0.2.0/24"))); + assert!(set.matches(&net("203.0.113.64/27"))); + assert!(!set.matches(&net("192.0.2.64/27"))); + assert!(!set.matches(&net("198.51.100.0/24"))); + } + + #[test] + fn ipv6_ranges_match_like_ipv4() { + let set = PrefixSet { + name: "TEST6".to_owned(), + mode: AddressFamily::Ipv6, + prefixes: [range("2001:db8::/32", 32, 128)].into_iter().collect(), + }; + assert!(set.matches(&net("2001:db8::/32"))); + assert!(set.matches(&net("2001:db8::/48"))); + assert!(set.matches(&net("2001:db8:1::/48"))); + assert!(set.matches(&net("2001:db8::1/128"))); + assert!(!set.matches(&net("2001:db9::/48"))); + assert!(!set.matches(&net("2001:db8::/31"))); + } + + #[test] + fn default_zero_prefix_matches_any_in_range() { + // `0.0.0.0/0 [0, 32]` is the "match any IPv4 prefix" pattern + // commonly used to redistribute everything of the configured + // address family. + let set = set_from([range("0.0.0.0/0", 0, 32)]); + assert!(set.matches(&net("0.0.0.0/0"))); + assert!(set.matches(&net("10.0.0.0/8"))); + assert!(set.matches(&net("192.0.2.0/24"))); + assert!(set.matches(&net("198.51.100.1/32"))); + } + + #[test] + fn empty_prefix_set_never_matches() { + let set = PrefixSet { + name: "EMPTY".to_owned(), + mode: AddressFamily::Ipv4, + prefixes: BTreeSet::new(), + }; + assert!(!set.matches(&net("192.0.2.0/24"))); + assert!(!set.matches(&net("0.0.0.0/0"))); + } + + #[test] + fn non_canonical_anchor_is_interpreted_as_its_covering_network() { + // Holo's northbound does not canonicalize `ip-prefix` on + // ingest, so a configured range like `192.0.2.1/24` is stored + // verbatim with host bits set. Under containment semantics the + // stored anchor is interpreted as the covering network + // (`192.0.2.0/24`), because `IpNetwork::contains` masks the + // candidate before comparing. This test pins that behavior. + let set = set_from([range("192.0.2.1/24", 24, 32)]); + assert!(set.matches(&net("192.0.2.0/24"))); + assert!(set.matches(&net("192.0.2.64/27"))); + assert!(set.matches(&net("192.0.2.255/32"))); + assert!(!set.matches(&net("198.51.100.0/24"))); + } +}