On Tue, May 18, 2021 at 02:06:15PM +0200, Claudio Jeker wrote:
> When I adjusted the capability negotiation to check both sides for
> presence I made the graceful restart capability lose all AFI/SAFI
> elements for the peer capabilities.
>
> This can be viewed with bgpctl show nei: e.g
>
> Description: beznau-1
> BGP version 4, remote router-id 192.168.0.252
> BGP state = Established, up for 02:11:07
> Last read 00:00:09, holdtime 90s, keepalive interval 30s
> Last write 00:00:06
> Neighbor capabilities:
> Multiprotocol extensions: IPv4 unicast
> 4-byte AS numbers
> Route Refresh
> Graceful Restart: Timeout: 90,
> Negotiated capabilities:
> Multiprotocol extensions: IPv4 unicast
> 4-byte AS numbers
> Route Refresh
>
> Message statistics:
>
> Instead of
>
> Description: beznau-1
> BGP version 4, remote router-id 192.168.0.252
> BGP state = Established, up for 00:02:31
> Last read 00:00:00, holdtime 90s, keepalive interval 30s
> Last write 00:00:00
> Neighbor capabilities:
> Multiprotocol extensions: IPv4 unicast
> 4-byte AS numbers
> Route Refresh
> Graceful Restart: Timeout: 90, restarted, IPv4 unicast
> Negotiated capabilities:
> Multiprotocol extensions: IPv4 unicast
> 4-byte AS numbers
> Route Refresh
>
> This is just a visual issue. In both cases the flush happens and graceful
> refresh remains disabled.
Actually lets go one step further and change what we announce. bgpd only
supports the "Procedures for the Receiving Speaker". It never keeps the
forwarding state over a restart. Because of this there is no need to
include any AFI/SAFI in the graceful restart capability.
>From the RFC:
When a sender of this capability does not include any <AFI, SAFI> in
the capability, it means that the sender is not capable of preserving
its forwarding state during BGP restart, but supports procedures for
the Receiving Speaker (as defined in Section 4.2 of this document).
In that case, the value of the "Restart Time" field advertised by the
sender is irrelevant.
I think not including any AFI/SAFI is the sensible thing to do.
--
:wq Claudio
Index: session.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
retrieving revision 1.415
diff -u -p -r1.415 session.c
--- session.c 16 May 2021 09:09:11 -0000 1.415
+++ session.c 20 May 2021 11:22:28 -0000
@@ -1443,41 +1443,20 @@ session_open(struct peer *p)
/* graceful restart and End-of-RIB marker, RFC 4724 */
if (p->capa.ann.grestart.restart) {
int rst = 0;
- u_int16_t hdr;
- u_int8_t grlen;
+ u_int16_t hdr = 0;
- if (mpcapa) {
- grlen = 2 + 4 * mpcapa;
- for (i = 0; i < AID_MAX; i++) {
- if (p->capa.neg.grestart.flags[i] &
- CAPA_GR_RESTARTING)
- rst++;
- }
- } else { /* AID_INET */
- grlen = 2 + 4;
- if (p->capa.neg.grestart.flags[AID_INET] &
- CAPA_GR_RESTARTING)
+ for (i = 0; i < AID_MAX; i++) {
+ if (p->capa.neg.grestart.flags[i] & CAPA_GR_RESTARTING)
rst++;
}
- hdr = conf->holdtime; /* default timeout */
- /* if client does graceful restart don't set R flag */
+ /* Only set the R-flag if no graceful restart is ongoing */
if (!rst)
hdr |= CAPA_GR_R_FLAG;
hdr = htons(hdr);
- errs += session_capa_add(opb, CAPA_RESTART, grlen);
+ errs += session_capa_add(opb, CAPA_RESTART, sizeof(hdr));
errs += ibuf_add(opb, &hdr, sizeof(hdr));
-
- if (mpcapa) {
- for (i = 0; i < AID_MAX; i++) {
- if (p->capa.ann.mp[i]) {
- errs += session_capa_add_gr(p, opb, i);
- }
- }
- } else { /* AID_INET */
- errs += session_capa_add_gr(p, opb, AID_INET);
- }
}
/* 4-bytes AS numbers, draft-ietf-idr-as4bytes-13 */
@@ -2585,24 +2564,24 @@ capa_neg_calc(struct peer *p)
int8_t negflags;
/* disable GR if the AFI/SAFI is not present */
- if (p->capa.ann.grestart.restart == 0 ||
- (p->capa.peer.grestart.flags[i] & CAPA_GR_PRESENT &&
+ if ((p->capa.peer.grestart.flags[i] & CAPA_GR_PRESENT &&
p->capa.neg.mp[i] == 0))
p->capa.peer.grestart.flags[i] = 0; /* disable */
/* look at current GR state and decide what to do */
negflags = p->capa.neg.grestart.flags[i];
p->capa.neg.grestart.flags[i] = p->capa.peer.grestart.flags[i];
if (negflags & CAPA_GR_RESTARTING) {
- if (!(p->capa.peer.grestart.flags[i] &
- CAPA_GR_FORWARD)) {
+ if (p->capa.ann.grestart.restart != 0 ||
+ p->capa.peer.grestart.flags[i] & CAPA_GR_FORWARD) {
+ p->capa.neg.grestart.flags[i] |=
+ CAPA_GR_RESTARTING;
+ } else {
if (imsg_rde(IMSG_SESSION_FLUSH, p->conf.id,
&i, sizeof(i)) == -1)
return (-1);
log_peer_warnx(&p->conf, "graceful restart of "
"%s, not restarted, flushing", aid2str(i));
- } else
- p->capa.neg.grestart.flags[i] |=
- CAPA_GR_RESTARTING;
+ }
}
}
p->capa.neg.grestart.timeout = p->capa.peer.grestart.timeout;
