Skip to content

Add BGP dynamic capability support for graceful restart #17

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

Open
heidinet2007 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add BGP dynamic capability support for graceful restart #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 8 additions & 3 deletions bgpd/bgp_open.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
/* Standard header for capability TLV */
struct capability_header {
uint8_t code;
uint8_t length;
uint8_t length; /* Note, extra one more oct in dynamic capabilities */
Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Dec 11, 2019

Choose a reason for hiding this comment

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

what 'oct' means here?

Copy link
Copy Markdown
Author

@heidinet2007 heidinet2007 Dec 13, 2019

Choose a reason for hiding this comment

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

FRR implemented the old version of dynamic capability draft, which has only 1 Octect for "length field". This field has been changed in the latest version to 2 Octects. Since the draft hasn't become RFC, and by talking to the authors, they are still trying to finalize the content, hence I didn't change the code, but left some comment there.

Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Dec 13, 2019

Choose a reason for hiding this comment

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

Since it's still a draft, what bgp implementations supports it? Why SONiC needs to support it?

Copy link
Copy Markdown
Author

@heidinet2007 heidinet2007 Dec 13, 2019

Choose a reason for hiding this comment

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

Almost all switch vendors with BGP implementation support dynamic capability - Cisco, Juniper, Arista, Huawei...

};

/* Generic MP capability data */
Expand All @@ -35,11 +35,16 @@ struct capability_mp_data {
};

struct graceful_restart_af {
afi_t afi;
safi_t safi;
uint16_t afi;
uint8_t safi;
Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Dec 11, 2019

Choose a reason for hiding this comment

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

why ?

Copy link
Copy Markdown
Author

@heidinet2007 heidinet2007 Dec 13, 2019

Choose a reason for hiding this comment

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

According to graceful restart RFC 4724
+--------------------------------------------------+
| Address Family Identifier (16 bits) |
+--------------------------------------------------+
| Subsequent Address Family Identifier (8 bits) |
+--------------------------------------------------+

Wrong size is defined in current FRR:
(gdb) p/x sizeof(afi_t)
$2 = 0x4
(gdb) p/x sizeof(safi_t)
$3 = 0x4

This causes failure in compatibility testing with other switches.

Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Dec 13, 2019

Choose a reason for hiding this comment

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

But this value is not what is being sent over line inside of the packet. FRR has special code which convert from aft_t, safi_t to a line format

Copy link
Copy Markdown
Author

@heidinet2007 heidinet2007 Dec 13, 2019

Choose a reason for hiding this comment

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

Do you mean the big/small endian convert or something else? Which piece of convert code do you refer to?

Copy link
Copy Markdown
Contributor

@pavel-shirshov pavel-shirshov Dec 18, 2019

Choose a reason for hiding this comment

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

https://github.com/FRRouting/frr/blob/0688fd81d006cbf4a0aedd771d769a497514ef4b/lib/iana_afi.h#L24
https://github.com/FRRouting/frr/blob/0688fd81d006cbf4a0aedd771d769a497514ef4b/lib/iana_afi.h#L65

Copy link
Copy Markdown
Author

@heidinet2007 heidinet2007 Jan 7, 2020

Choose a reason for hiding this comment

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

Here we just copy fields from wire directly, without conversion, similar to capability_mp_data. There were multiple problems fixed after the introduction of iana_xxx_t, such as
FRRouting/frr@a46a2e9

uint8_t flag;
};

struct capability_gr {
uint16_t restart_flag_time;
struct graceful_restart_af gr[];
};

/* Capability Code */
#define CAPABILITY_CODE_MP 1 /* Multiprotocol Extensions */
#define CAPABILITY_CODE_REFRESH 2 /* Route Refresh Capability */
Expand Down
191 changes: 187 additions & 4 deletions bgpd/bgp_packet.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -863,6 +863,65 @@ void bgp_route_refresh_send(struct peer *peer, afi_t afi, safi_t safi,
bgp_writes_on(peer);
}

static void bgp_restart_dyn_capability_send (struct peer *peer,
int action,
struct stream *s)
{
afi_t afi;
safi_t safi;
iana_afi_t pkt_afi;
iana_safi_t pkt_safi;
uint8_t len;
unsigned long rcapp;
uint32_t restart_time;

stream_putc(s, action);
stream_putc(s, CAPABILITY_CODE_RESTART);

/* Copy from optional capability */
rcapp = stream_get_endp(s); /* Set Restart Capability Len Pointer */
stream_putc(s, 0);
restart_time = peer->bgp->restart_time;
if (peer->bgp->t_startup) {
SET_FLAG(restart_time, RESTART_R_BIT);
SET_FLAG(peer->cap, PEER_CAP_RESTART_BIT_ADV);
}
stream_putw(s, restart_time);

FOREACH_AFI_SAFI (afi, safi) {
if (peer->afc[afi][safi]) {
/* Convert AFI, SAFI to values for
* packet. */
bgp_map_afi_safi_int2iana(afi, safi, &pkt_afi,
&pkt_safi);
stream_putw(s, pkt_afi);
stream_putc(s, pkt_safi);
if (bgp_flag_check(peer->bgp,
BGP_FLAG_GR_PRESERVE_FWD)) {
stream_putc(s, RESTART_F_BIT);
} else {
stream_putc(s, 0);
}

if (bgp_debug_neighbor_events(peer))
zlog_debug(
"%s sending CAPABILITY has %s Graceful Restart CAP for afi/safi: %d/%d "
"is%spreserved",
peer->host,
action == CAPABILITY_ACTION_SET ? "Advertising"
: "Removing",
pkt_afi, pkt_safi,
bgp_flag_check(peer->bgp,
BGP_FLAG_GR_PRESERVE_FWD) ?
" " : "not");
}
}

/* Total Graceful restart capability Len. */
len = stream_get_endp(s) - rcapp - 1;
stream_putc_at(s, rcapp, len);
}

/*
* Create a BGP Capability packet and append it to the peer's output queue.
*
Expand Down Expand Up @@ -903,6 +962,8 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi,
action == CAPABILITY_ACTION_SET ? "Advertising"
: "Removing",
pkt_afi, pkt_safi);
} else if (capability_code == CAPABILITY_CODE_RESTART) {
bgp_restart_dyn_capability_send(peer, action, s);
}

/* Set packet size. */
Expand Down Expand Up @@ -2001,6 +2062,115 @@ static int bgp_route_refresh_receive(struct peer *peer, bgp_size_t size)
return BGP_PACKET_NOOP;
}

static int bgp_restart_dyn_capability_parse (struct peer *peer,
u_char action,
struct capability_header *caphdr,
u_char *end)
{
u_int16_t pkt_time, restart_flag_time;
u_char *pnt;

/* Verify length is a multiple of 4 */
if ((caphdr->length - 2) % 4) {
zlog_warn(
"Restart Cap: Received invalid length %d, non-multiple of 4",
caphdr->length);
return -1;
}

SET_FLAG(peer->cap, PEER_CAP_RESTART_RCV);
pnt = (u_char *)caphdr;
pnt += sizeof(struct capability_header); /* Adjust it to value of restart capability */

memcpy(&pkt_time, pnt, sizeof(u_int16_t));
restart_flag_time = ntohs(pkt_time);
pnt += sizeof(u_int16_t);

if (CHECK_FLAG(restart_flag_time, RESTART_R_BIT))
SET_FLAG(peer->cap, PEER_CAP_RESTART_BIT_RCV);

UNSET_FLAG(restart_flag_time, 0xF000);
peer->v_gr_restart = restart_flag_time;

if (bgp_debug_neighbor_events(peer)) {
zlog_debug("%s Capability has Graceful Restart capability",
peer->host);
zlog_debug("%s Peer has%srestarted. Restart Time : %d",
peer->host,
CHECK_FLAG(peer->cap, PEER_CAP_RESTART_BIT_RCV)
? " "
: " not ",
peer->v_gr_restart);
}


while (pnt + 4 <= end) {
struct graceful_restart_af grc;
iana_afi_t pkt_afi;
iana_safi_t pkt_safi;
uint8_t flag;
afi_t afi;
safi_t safi;

memcpy(&grc, pnt, sizeof(struct graceful_restart_af));
pnt += sizeof(struct graceful_restart_af);
pkt_afi = ntohs(grc.afi);
pkt_safi = grc.safi;
flag = grc.flag;

/* Convert AFI, SAFI to internal values, check. */
if (bgp_map_afi_safi_iana2int(pkt_afi, pkt_safi, &afi, &safi)) {
if (bgp_debug_neighbor_events(peer))
zlog_debug(
"%s Addr-family %d/%d(afi/safi) not supported."
" Ignore the Graceful Restart capability for this AFI/SAFI",
peer->host, pkt_afi, pkt_safi);
} else if (!peer->afc[afi][safi]) {
if (bgp_debug_neighbor_events(peer))
zlog_debug(
"%s Addr-family %d/%d(afi/safi) not enabled."
" Ignore the Graceful Restart capability",
peer->host, pkt_afi, pkt_safi);
} else {
if (bgp_debug_neighbor_events(peer))
zlog_debug(
"%s Address family %s was%spreserved",
peer->host, afi_safi_print(afi, safi),
CHECK_FLAG(peer->af_cap[afi][safi],
PEER_CAP_RESTART_AF_PRESERVE_RCV) ?
" " : " not ");

if (action == CAPABILITY_ACTION_SET) {
SET_FLAG(peer->af_cap[afi][safi],
PEER_CAP_RESTART_AF_RCV);
if (CHECK_FLAG(flag, RESTART_F_BIT)) {
SET_FLAG(peer->af_cap[afi][safi],
PEER_CAP_RESTART_AF_PRESERVE_RCV);
} else {
UNSET_FLAG(peer->af_cap[afi][safi],
PEER_CAP_RESTART_AF_PRESERVE_RCV);
}
} else { /*
* Reset AF specifict bit if removing
* graceful-restart capability dynamically
*/
UNSET_FLAG(peer->af_cap[afi][safi],
PEER_CAP_RESTART_AF_RCV);
UNSET_FLAG(peer->af_cap[afi][safi],
PEER_CAP_RESTART_AF_PRESERVE_RCV);
}
if (bgp_debug_neighbor_events(peer))
zlog_debug(
"%s Address family %s now is%spreserved",
peer->host, afi_safi_print(afi, safi),
CHECK_FLAG(peer->af_cap[afi][safi],
PEER_CAP_RESTART_AF_PRESERVE_RCV) ?
" " : " not ");
}
}
return 0;
}

/**
* Parse BGP CAPABILITY message for peer.
*
Expand All @@ -2011,7 +2181,7 @@ static int bgp_route_refresh_receive(struct peer *peer, bgp_size_t size)
static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt,
bgp_size_t length)
{
uint8_t *end;
uint8_t *end, *start_val;
struct capability_mp_data mpc;
struct capability_header *hdr;
uint8_t action;
Expand Down Expand Up @@ -2054,12 +2224,13 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt,
return BGP_Stop;
}

/* Fetch structure to the byte stream. */
memcpy(&mpc, pnt + 3, sizeof(struct capability_mp_data));
pnt += hdr->length + 3;
start_val = pnt + 3; /* Get start of value */
pnt = start_val + hdr->length; /* Move to next dynamic cap */

/* We know MP Capability Code. */
if (hdr->code == CAPABILITY_CODE_MP) {
/* Fetch structure to the byte stream. */
memcpy(&mpc, start_val, sizeof(struct capability_mp_data));
pkt_afi = ntohs(mpc.afi);
pkt_safi = mpc.safi;

Expand Down Expand Up @@ -2104,6 +2275,18 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt,
else
return BGP_Stop;
}
} else if (hdr->code == CAPABILITY_CODE_RESTART) {
if (bgp_debug_neighbor_events(peer))
zlog_debug(
"%s CAPABILITY has %s RESTART CAP",
peer->host,
action == CAPABILITY_ACTION_SET
? "Advertising"
: "Removing");

if (bgp_restart_dyn_capability_parse(peer, action, hdr, end) < 0) {
return BGP_Stop;
}
} else {
flog_warn(
EC_BGP_UNRECOGNIZED_CAPABILITY,
Expand Down
47 changes: 43 additions & 4 deletions bgpd/bgp_vty.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1946,6 +1946,23 @@ DEFUN (no_bgp_deterministic_med,
return CMD_SUCCESS;
}

static void bgp_restart_dyn_capability_update (struct bgp *bgp, int set)
{
struct peer *peer;
struct listnode *node, *nnode;

for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) {
if ((peer->status == Established) &&
(CHECK_FLAG(peer->cap, PEER_CAP_DYNAMIC_RCV))) {
bgp_capability_send(peer, AFI_MAX, SAFI_MAX,
CAPABILITY_CODE_RESTART,
set ?
CAPABILITY_ACTION_SET :
CAPABILITY_ACTION_UNSET);
}
}
}

/* "bgp graceful-restart" configuration. */
DEFUN (bgp_graceful_restart,
bgp_graceful_restart_cmd,
Expand All @@ -1954,7 +1971,10 @@ DEFUN (bgp_graceful_restart,
"Graceful restart capability parameters\n")
{
VTY_DECLVAR_CONTEXT(bgp, bgp);
bgp_flag_set(bgp, BGP_FLAG_GRACEFUL_RESTART);
if (!bgp_flag_check(bgp, BGP_FLAG_GRACEFUL_RESTART)) {
bgp_flag_set(bgp, BGP_FLAG_GRACEFUL_RESTART);
bgp_restart_dyn_capability_update(bgp, 1);
}
return CMD_SUCCESS;
}

Expand All @@ -1966,7 +1986,10 @@ DEFUN (no_bgp_graceful_restart,
"Graceful restart capability parameters\n")
{
VTY_DECLVAR_CONTEXT(bgp, bgp);
bgp_flag_unset(bgp, BGP_FLAG_GRACEFUL_RESTART);
if (bgp_flag_check(bgp, BGP_FLAG_GRACEFUL_RESTART)) {
bgp_flag_unset(bgp, BGP_FLAG_GRACEFUL_RESTART);
bgp_restart_dyn_capability_update(bgp, 0);
}
return CMD_SUCCESS;
}

Expand Down Expand Up @@ -2001,6 +2024,9 @@ DEFUN (bgp_graceful_restart_restart_time,

restart = strtoul(argv[idx_number]->arg, NULL, 10);
bgp->restart_time = restart;
if (bgp_flag_check(bgp, BGP_FLAG_GRACEFUL_RESTART)) {
bgp_restart_dyn_capability_update(bgp, 1);
}
return CMD_SUCCESS;
}

Expand Down Expand Up @@ -2031,6 +2057,9 @@ DEFUN (no_bgp_graceful_restart_restart_time,
VTY_DECLVAR_CONTEXT(bgp, bgp);

bgp->restart_time = BGP_DEFAULT_RESTART_TIME;
if (bgp_flag_check(bgp, BGP_FLAG_GRACEFUL_RESTART)) {
bgp_restart_dyn_capability_update(bgp, 1);
}
return CMD_SUCCESS;
}

Expand All @@ -2042,7 +2071,12 @@ DEFUN (bgp_graceful_restart_preserve_fw,
"Sets F-bit indication that fib is preserved while doing Graceful Restart\n")
{
VTY_DECLVAR_CONTEXT(bgp, bgp);
bgp_flag_set(bgp, BGP_FLAG_GR_PRESERVE_FWD);
if (!bgp_flag_check(bgp, BGP_FLAG_GR_PRESERVE_FWD)) {
bgp_flag_set(bgp, BGP_FLAG_GR_PRESERVE_FWD);
if (bgp_flag_check(bgp, BGP_FLAG_GRACEFUL_RESTART)) {
bgp_restart_dyn_capability_update(bgp, 1);
}
}
return CMD_SUCCESS;
}

Expand All @@ -2055,7 +2089,12 @@ DEFUN (no_bgp_graceful_restart_preserve_fw,
"Unsets F-bit indication that fib is preserved while doing Graceful Restart\n")
{
VTY_DECLVAR_CONTEXT(bgp, bgp);
bgp_flag_unset(bgp, BGP_FLAG_GR_PRESERVE_FWD);
if (bgp_flag_check(bgp, BGP_FLAG_GR_PRESERVE_FWD)) {
bgp_flag_unset(bgp, BGP_FLAG_GR_PRESERVE_FWD);
if (bgp_flag_check(bgp, BGP_FLAG_GRACEFUL_RESTART)) {
bgp_restart_dyn_capability_update(bgp, 1);
}
}
return CMD_SUCCESS;
}

Expand Down