feat: RFC 7606 non-fatal NLRI parsing and treat-as-withdrawal support - #309
Open
digizeph wants to merge 5 commits into
Open
feat: RFC 7606 non-fatal NLRI parsing and treat-as-withdrawal support#309digizeph wants to merge 5 commits into
digizeph wants to merge 5 commits into
Conversation
- Malformed NLRI (withdrawn or announced) no longer causes the entire BGP UPDATE to be discarded. The parser returns partial data with a BgpValidationWarning::MalformedNlri containing the raw NLRI bytes. - parse_mrt_record now preserves raw MRT record bytes in ParserErrorWithBytes when the message body fails to parse. - BgpValidationWarning marked #[non_exhaustive] for forward compatibility. - Added examples/treat_as_withdrawal.rs demonstrating RFC 7606 error classification (attribute discard vs treat-as-withdrawal). - Added 12 fixture tests covering malformed NLRI recovery, raw byte preservation, the Qrator OTC incident scenario, and treat-as-withdrawal emulation. Closes #303
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements RFC 7606 revised error handling for BGP UPDATE parsing by making standard NLRI parsing non-fatal (emitting structured validation warnings instead) and by preserving raw MRT record bytes on body-parse failures to enable downstream diagnostics/forensics.
Changes:
- Convert withdrawn/announced NLRI parse failures in
parse_bgp_update_messageintoBgpValidationWarning::MalformedNlriwhile preserving attributes and the other NLRI section. - Preserve raw MRT record bytes in
ParserErrorWithByteswhenparse_mrt_recordfails while parsing the body. - Add fixture-style tests plus a runnable example demonstrating treat-as-withdrawal emulation and warning classification.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_rfc7606_error_handling.rs | Adds RFC 7606-focused regression tests for non-fatal NLRI parsing and raw-byte preservation. |
| src/parser/mrt/mrt_record.rs | Attaches raw MRT record bytes to ParserErrorWithBytes on body-parse errors. |
| src/parser/bgp/messages.rs | Implements non-fatal withdrawn/announced NLRI parsing with MalformedNlri warnings. |
| src/error.rs | Marks BgpValidationWarning as #[non_exhaustive] and adds MalformedNlri variant + Display. |
| examples/treat_as_withdrawal.rs | Demonstrates scanning MRT files and applying RFC 7606 treat-as-withdrawal logic using warnings. |
| examples/README.md | Indexes the new treat-as-withdrawal example. |
| CHANGELOG.md | Documents the new RFC 7606 NLRI handling and MRT raw-byte preservation behavior. |
Comments suppressed due to low confidence (1)
src/parser/bgp/messages.rs:507
- Same as withdrawn NLRI: a 1-byte announced NLRI currently becomes
Ok(vec![])insideread_nlri(no error), so noMalformedNlriwarning will be recorded. That prevents callers from reliably detecting malformed announced NLRI via warnings.
let (announced_prefixes, announced_nlri_error) = match read_nlri(input.clone(), &afi, add_path)
{
Ok(pfxs) => (pfxs, None),
Err(e) => (
Vec::new(),
Some(BgpValidationWarning::MalformedNlri {
nlri_type: "announced",
reason: e.to_string(),
raw_bytes: input.to_vec(),
}),
),
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+470
to
+481
| let (withdrawn_prefixes, withdrawn_nlri_error) = | ||
| match read_nlri(withdrawn_bytes.clone(), &afi, add_path) { | ||
| Ok(pfxs) => (pfxs, None), | ||
| Err(e) => ( | ||
| Vec::new(), | ||
| Some(BgpValidationWarning::MalformedNlri { | ||
| nlri_type: "withdrawn", | ||
| reason: e.to_string(), | ||
| raw_bytes: withdrawn_bytes.to_vec(), | ||
| }), | ||
| ), | ||
| }; |
Comment on lines
166
to
175
| pub fn parse_mrt_record(input: &mut impl Read) -> Result<MrtRecord, ParserErrorWithBytes> { | ||
| let raw_record = chunk_mrt_record(input)?; | ||
| let raw_bytes = raw_record.raw_bytes(); | ||
| match raw_record.parse() { | ||
| Ok(record) => Ok(record), | ||
| Err(e) => Err(ParserErrorWithBytes { | ||
| error: e, | ||
| bytes: None, | ||
| bytes: Some(raw_bytes.to_vec()), | ||
| }), | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #309 +/- ##
==========================================
+ Coverage 90.07% 90.15% +0.07%
==========================================
Files 91 91
Lines 19136 19159 +23
==========================================
+ Hits 17236 17272 +36
+ Misses 1900 1887 -13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Classify each validation warning into Notification / Treat-as-withdrawal / Attribute discard tiers following the Junos bgp-error-tolerance per-attribute mapping table. - Apply the 'most severe wins' rule when multiple attributes are malformed in a single UPDATE. - Show the router action (session reset, routes withdrawn, attribute discarded) for each finding.
- read_nlri: 1-byte NLRI field now returns Err instead of silently returning Ok(vec![]), so MalformedNlri warnings are correctly emitted for this edge case (both withdrawn and announced sections). - parse_mrt_record: build raw bytes Vec directly from header_bytes + message_bytes instead of going through raw_bytes() → to_vec(), avoiding an unnecessary intermediate Bytes allocation on the error path. - Added regression test for the 1-byte NLRI case.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/parser/bgp/messages.rs:514
- Mandatory-attribute validation currently keys off
announced_prefixes.is_empty(). When announced NLRI bytes are present but malformed (converted to aMalformedNlriwarning),announced_prefixesbecomes empty andcheck_mandatory_attributesmay be skipped, missing warnings for missing ORIGIN/AS_PATH/NEXT_HOP. Use the presence of announced NLRI bytes (or the NLRI error) rather than parsed prefixes to decide whether this UPDATE is an announcement / has standard NLRI.
// validate mandatory attributes
let is_announcement =
!announced_prefixes.is_empty() || attributes.has_attr(AttrType::MP_REACHABLE_NLRI);
let has_standard_nlri = !announced_prefixes.is_empty();
attributes.check_mandatory_attributes(is_announcement, has_standard_nlri);
Comment on lines
+438
to
+442
| if length == 1 { | ||
| // 1 byte does not make sense | ||
| // 1 byte does not make a valid NLRI encoding | ||
| warn!("seeing strange one-byte NLRI field (parsing NLRI in BGP UPDATE message)"); | ||
| input.advance(1); // skip the byte | ||
| return Ok(vec![]); | ||
| return Err(ParserError::ParseError( | ||
| "one-byte NLRI field is not a valid encoding".to_string(), |
Comment on lines
+375
to
+377
| // A 1-byte NLRI field in the announced section — previously this was | ||
| // silently skipped (Ok(vec![])), now it produces a MalformedNlri warning. | ||
| let one_byte_nlri = vec![0xFF]; |
Copilot correctly identified that a 1-byte NLRI with value 0x00 encodes the default route (prefix length 0, no prefix octets) — a valid encoding. The previous blanket rejection of all 1-byte NLRI would have incorrectly dropped valid 0.0.0.0/0 announcements. Now only 1-byte NLRI with a non-zero value is treated as malformed. Added test for both cases: invalid (0xFF) and valid default route (0x00).
Comment on lines
511
to
515
| // validate mandatory attributes | ||
| let is_announcement = | ||
| !announced_prefixes.is_empty() || attributes.has_attr(AttrType::MP_REACHABLE_NLRI); | ||
| let has_standard_nlri = !announced_prefixes.is_empty(); | ||
| attributes.check_mandatory_attributes(is_announcement, has_standard_nlri); |
Comment on lines
166
to
183
| pub fn parse_mrt_record(input: &mut impl Read) -> Result<MrtRecord, ParserErrorWithBytes> { | ||
| let raw_record = chunk_mrt_record(input)?; | ||
| // Capture raw bytes before parse() consumes raw_record. | ||
| let header_bytes = raw_record.header_bytes.clone(); | ||
| let message_bytes = raw_record.message_bytes.clone(); | ||
| match raw_record.parse() { | ||
| Ok(record) => Ok(record), | ||
| Err(e) => Err(ParserErrorWithBytes { | ||
| error: e, | ||
| bytes: None, | ||
| }), | ||
| Err(e) => { | ||
| // Build the raw bytes Vec directly from header + message bytes. | ||
| let mut bytes = Vec::with_capacity(header_bytes.len() + message_bytes.len()); | ||
| bytes.extend_from_slice(&header_bytes); | ||
| bytes.extend_from_slice(&message_bytes); | ||
| Err(ParserErrorWithBytes { | ||
| error: e, | ||
| bytes: Some(bytes), | ||
| }) | ||
| } | ||
| } |
- is_announcement/has_standard_nlri now based on wire-level NLRI byte presence (announced_bytes_present) rather than parse success. A malformed announced NLRI section still triggers mandatory-attribute validation (ORIGIN/AS_PATH/NEXT_HOP) because the UPDATE clearly intended to announce. - parse_mrt_record: use raw_record.clone().parse() + raw_bytes() instead of manual header/message reassembly, eliminating code duplication that could diverge from RawMrtRecord::raw_bytes(). - Added test: malformed NLRI with missing mandatory attrs still produces both MalformedNlri and MissingWellKnownAttribute warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #303
Summary
Implements RFC 7606 revised error handling for BGP UPDATE messages, enabling treat-as-withdrawal emulation. Two coupled changes:
1. Non-fatal NLRI parsing (
parse_bgp_update_message)Previously, any malformed byte in the withdrawn or announced NLRI caused the entire UPDATE to fail — destroying all attributes, all other prefixes, and returning
Err. This made RFC 7606 §5.3 treat-as-withdrawal impossible.Now, NLRI parse errors are caught inside
parse_bgp_update_messageand converted toBgpValidationWarning::MalformedNlri. The UPDATE is returned with partial prefix data (empty for the failed NLRI section, intact for the other). Attributes are fully preserved.Attribute framing errors (wrong length fields, truncated data) remain fatal, as they indicate structural corruption that makes the entire message untrustworthy.
2. Raw byte preservation (
parse_mrt_record)Body-parse failures now attach the original MRT record bytes to
ParserErrorWithBytes.bytes. Previously this was alwaysNonefor body-level failures, making forensic analysis and record export impossible from the fallible iterator.API surface
BgpValidationWarningmarked#[non_exhaustive]— exhaustive matches must add_ =>. This is the only breaking change and is expected at 0.x maturity.BgpValidationWarning::MalformedNlri { nlri_type: &'static str, reason: String, raw_bytes: Vec<u8> }Example
examples/treat_as_withdrawal.rsdemonstrates scanning an MRT file, classifying RFC 7606 validation issues by error-handling category, and extracting raw malformed NLRI bytes.Validation
cargo fmt --check✅cargo clippy --all-targets --all-features -- -D warnings✅cargo test --all-features: 664 existing tests + 12 new RFC 7606 fixture tests, all passing ✅cargo readmediff check ✅New tests cover: malformed announced/withdrawn NLRI, raw byte preservation in warnings, attribute survival across NLRI failure, partial prefix recovery, valid UPDATE still clean, framing errors still fatal, MRT raw byte preservation, Qrator OTC incident scenario, treat-as-withdrawal emulation pattern.