Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update gNOI.bgp.proto #214

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Update gNOI.bgp.proto #214

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc24c20
Update gNOI.bgp.proto
sachendras Sep 27, 2024
0ee80a1
Update gNOI.bgp.proto
sachendras Sep 27, 2024
a5ab33f
Update gNOIbgp.proto
sachendras Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion bgp/bgp.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,12 @@ message ClearBGPNeighborRequest {
enum Mode {
SOFT = 0; // Send route-refresh and reapply policy.
SOFTIN = 1; // Re-apply inbound policy on stored Adj-RIB-In.
HARD = 2; // Teardown and restart TCP connection.
HARD = 2; // Teardown, restart TCP connection, Send Cease
Copy link

@LimeHat LimeHat Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a (slightly) backward-incompatible change.
There could be existing implementations that send codes 4/9 unconditionally.

Copy link
Contributor Author

@sachendras sachendras Oct 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you please elaborate this with examples? What does unconditionally mean?
Error code 6 subcode 9 was introduced to handle GR cases when the "N" bit in the RESET flag of Capability 64 is set. So what other conditions would you send this subcode?

Following also from RFC8538 section#4:
A BGP speaker SHOULD NOT send a Hard Reset to a peer from which it
has not received the "N" bit. We note, however, that if it did so,
the effect would be as desired in any case because, according to
[RFC4271] and [RFC4724], any NOTIFICATION message, whether recognized
or not, results in a session reset. Thus, the only negative effect
to be expected from sending the Hard Reset to a peer that hasn't
advertised compliance to this specification would be that the peer
would be unable to properly log the associated information.

Copy link

Choose a reason for hiding this comment

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

Scenario 1) an implementation doesn't know about the RFC & code 9 and always uses code 4. (and receives the notification bit from its peer, which implies, according to the rules you specified, code 9)
Scenario 2) an implementation knows about the RFC and code 9 and still always uses code 4, because nobody said that it cannot do so for this specific RPC call
Scenario 3) an implementation knows about code 9 and always uses it, ignoring the quoted recommendation

There are a number of implementations in the wild, and any of these are possible. A safer way to introduce new (strict) requirements is to add a new option instead of redefining the existing one.

Copy link
Contributor Author

@sachendras sachendras Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Scenario 1) an implementation doesn't know about the RFC & code 9 and always uses code 4. (and receives the notification bit from its peer, which implies, according to the rules you specified, code 9)"

Response: Right, which means the receiving speaker either isnt supporting RFC8538 or isnt configured for one. Expectations are that when the implementation does so, do follow the RFC requirement for "HARD RESET" when the gNOI.ClearBGPNeighborRequest.HARD is executed.

Following from the RFC8538

A BGP speaker SHOULD NOT send a Hard Reset to a peer from which it has not received the "N" bit. We note, however, that if it did so, the effect would be as desired in any case because, according to [RFC4271] and [RFC4724], any NOTIFICATION message, whether recognized or not, results in a session reset. Thus, the only negative effect to be expected from sending the Hard Reset to a peer that hasn't advertised compliance to this specification would be that the peer would be unable to properly log the associated information.

Scenario 2) An implementation knows about the RFC and code 9 and still always uses code 4, because nobody said that it cannot do so for this specific RPC call.

Response: Understood, which is why additional guidance now. For your example, following can happen

a. The implementation doesnt support RFC8538 and hence hasnt set "N" bit in the initial Capability 64 negotiation. Therefore, the receipt of gNOI.ClearBGPNeighborRequest.HARD command would result in the implementation sending error code 6 subcode 4. This MUST result in TCP connection reset and route FLUSH. In this scenario, gNOI.ClearBGPNeighborRequest.GRACEFUL will also result in sending error code 6 subcode 4. Since bit "N" of the RESET flag is "0", the recieving speaker would handle the subcode4 receipt accordingly resulting in route flush.
b. If the "N" bit was set originally then for "gNOI.ClearBGPNeighborRequest.HARD", the expectations are that the original Code6 Subcode 4 is encapsulated inside the errorcode 6 subcode9. If the implementation doesnt do so and continues to send error code 6 subcode 4, then this wont be a HARD_RESET signal for the receiving Speaker of the NOTIFICATION message. In this case the TCP session can go down but the routes wouldnt be flushed. This is also the RFC 8538 expectation for Subcode 4.

Adding a 3rd option in the proto keeping "HARD" intact will be confusing. The issue is, the currently defined "HARD" wasnt comprehensive enough to cover for GR scenarios. This pull is intending to close that gap.

Scenario 3) an implementation knows about code 9 and always uses it, ignoring the quoted recommendation

Response: What does that mean? Subcode 9 was introduced in the RFC8538 for GR. If the implementation is using that all the time then they arent compliant with the original BGP CEASE message (rfc4486).

Copy link

Choose a reason for hiding this comment

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

Adding a 3rd option in the proto keeping "HARD" intact will be confusing. The issue is, the currently defined "HARD" wasnt comprehensive enough to cover for GR scenarios. This pull is intending to close that gap.

The previous option can be deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for my knowledge, how is that approach of deprecating "HARD" in the favor of a new attribute to handle GR and non-GR situation for "HARD" reset better than updating the existing "HARD" attribute itself?

Copy link

@LimeHat LimeHat Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't change the end goal, the issue is just a matter of backward-compatibility.

As you correctly noted, in the current proto the behavior is not fully defined, so there could be differences in existing implementations (we can debate what is correct and what is not correct according to the rfc and so on, but that's not the main point).

A new option (for example let's call it HARD_RFC8538) provides an API guarantee to a client: if the client calls it, he gets the exact, strictly defined behavior in all cases (subcode 9 if supported by the receiver, subcode 4 otherwise).

Updating the existing option doesn't provide such guarantees: the client will get this behavior if NOS was updated according to the latest changes in proto, but if it wasn't, the behavior is not really deterministic and maybe the NOS will continue sending subcode 4 while the gnoi client wanted to send code 9.

(if you have a strong opinion, I'm fine with updating the existing option. But in my view a new option is a bit cleaner.)

// notification (code 6) with subcode 4. However, if the "N" bit for
// Graceful-Restart" is set, then encapsulate the original Notification
// message in the data portion of the Subcode 9 message as per
// rfc8538#section-3.1 and also Flush the routes.
GRACEFUL = 4; // Teardown, restart TCP connection, Send Cease notification (code 6) with subcode 4.
}
Mode mode = 3; // Which mode to use for the clear operation.
}
Expand Down
Loading