server: expose raw NOTIFICATION error code/subcode on PeerState - #3543
Open
Sreedhar-cp wants to merge 2 commits into
Open
server: expose raw NOTIFICATION error code/subcode on PeerState#3543Sreedhar-cp wants to merge 2 commits into
Sreedhar-cp wants to merge 2 commits into
Conversation
…valid-msg handleOpen() builds the actual outgoing NOTIFICATION in notif via bgp.NewBGPNotificationMessage() and correctly sends it on the wire (sendNotification(fsm.conn, notif)), but passed the received OPEN message (m, bound by the outer type switch) into newfsmStateReason(...) instead of notif, for both the bad-peer-AS and generic invalid-message paths. This means fsmStateReason.BGPNotification held a *bgp.BGPOpen rather than a *bgp.BGPNotification for these two transitions, silently diverging from every other FSM path in this file (hold-timer-expired, admin-down) which correctly pass the actual notification. Besides being simply wrong, this also meant bmp.go's BMP Peer Down Notification embedded the wrong message content for a peer torn down via a bad-peer-AS or invalid-message OPEN rejection. Pass notif instead, matching the convention used everywhere else in this file.
DisconnectReason/DisconnectMessage (added for peer disconnect reason reporting) describe why a session went down as an enum plus a formatted string, but don't expose the underlying RFC 4271 §6 NOTIFICATION error code/subcode as typed integers -- callers that need the raw numeric values have to fragile-parse the message string. Add PeerState.notification_code / notification_subcode (proto + generated Go), threaded through apiutil.PeerState and the WatchEvent peer-event builder via a new extractNotificationCodeSubcode helper. This is intentionally not gated on any particular DisconnectReason: fsmBadPeerAS, fsmInvalidMsg, fsmHoldTimerExpired, and fsmAdminDown can all carry a real notification, so any of them should surface a numeric code/subcode when one is actually present. Both fields are 0/0 when no notification was involved. Adds a test driving a genuine bad-peer-AS NOTIFICATION exchange between two live *BgpServer instances and asserting the resulting PeerState carries the real RFC 4271 OPEN Message Error / Bad Peer AS code/subcode.
fujita
reviewed
Aug 30, 2026
| // disconnect_reason/disconnect_message alone cannot expose these as typed | ||
| // integers without fragile string-parsing. | ||
| uint32 notification_code = 24; | ||
| uint32 notification_subcode = 25; |
Member
There was a problem hiding this comment.
// A BGP NOTIFICATION message, as defined in RFC 4271 section 4.5.
message BgpNotification {
uint32 error_code = 1;
uint32 error_subcode = 2;
bytes data = 3;
}
Let's create a new type for notification because we might need data later.
Member
|
Thanks! The first commit is a fix. So I've already merged it. |
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
2 participants
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.
Summary
PeerState.DisconnectReason/DisconnectMessage(added for peer disconnect reason reporting) describe why a session went down as an enum plus a formatted string, but don't expose the underlying RFC 4271 §6 NOTIFICATION error code/subcode as typed integers. A caller that needs the raw numeric values today has to fragile-parse the message string.This PR adds
PeerState.notification_code/notification_subcode, threaded through from the FSM's underlying*bgp.BGPNotificationwhenever one is present. This is intentionally not limited to a specificDisconnectReason(e.g.NOTIFICATION_SENT/_RECEIVED) —fsmBadPeerAS,fsmInvalidMsg,fsmHoldTimerExpired, andfsmAdminDowncan all carry a real notification too, so any of them should surface a numeric code/subcode when one is actually present. Both fields are0/0when no notification was involved.Commits
server: fix wrong message wired into fsmStateReason on bad-peer-AS/invalid-msg— while adding a test for the feature above, found thathandleOpen()builds the actual outgoing NOTIFICATION innotifand correctly sends it on the wire, but passes the received OPEN message (m, bound by the outer type switch) intonewfsmStateReason(...)instead ofnotif, for both the bad-peer-AS and generic invalid-message paths. Every other FSM path in this file (hold-timer-expired, admin-down) correctly passes the actual notification. Besides being simply wrong, this also meansbmp.go's BMP Peer Down Notification embeds the wrong message content for a peer torn down via a bad-peer-AS or invalid-message OPEN rejection. Fixed to match the convention used everywhere else in the file.api, server: expose raw NOTIFICATION error code/subcode on PeerState— the actual feature: new proto fields + regenerated Go,apiutil.PeerStatefields, and theextractNotificationCodeSubcodehelper wired intoWatchEvent's peer-event builder.Test plan
TestWatchEventPeerNotificationCodeSubcodedrives a genuine bad-peer-AS NOTIFICATION exchange between two live*BgpServerinstances and asserts the resultingPeerStatecarries the real RFC 4271 OPEN Message Error / Bad Peer AS code/subcode.go build ./...,go vet ./...clean.go test ./...passes across every package (two pre-existing tests,TestEBGPRouteStuckandTestRTCDeferralTimerRaceCondition, require loopback IP aliases not configured on macOS by default and are unaffected by this change — verified they fail identically on unmodifiedmaster).