bgpd: bmp unset v6 flag + address PR#14188 comments

use CHECK_FLAG
fix comment spaces
change zlog_debug to zlog_warn
safeguard on updated_route
added doc/developer/bmp.rst to subdir.am
other qol changes

Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
This commit is contained in:
Maxence Younsi 2023-09-11 14:51:45 +02:00
parent fa17129752
commit 9607070acd
3 changed files with 47 additions and 33 deletions

View File

@ -285,7 +285,7 @@ static inline int bmp_get_peer_distinguisher(struct bmp *bmp, afi_t afi,
static void bmp_common_hdr(struct stream *s, uint8_t ver, uint8_t type) static void bmp_common_hdr(struct stream *s, uint8_t ver, uint8_t type)
{ {
stream_putc(s, ver); stream_putc(s, ver);
stream_putl(s, 0); /*dummy message length. will be set later. */ stream_putl(s, 0); /* dummy message length. will be set later. */
stream_putc(s, type); stream_putc(s, type);
} }
@ -305,7 +305,7 @@ static void bmp_per_peer_hdr(struct stream *s, struct bgp *bgp,
stream_putc(s, peer_type_flag); stream_putc(s, peer_type_flag);
/* Peer Flags */ /* Peer Flags */
if (peer->connection->su.sa.sa_family == AF_INET6) if (!is_locrib && peer->connection->su.sa.sa_family == AF_INET6)
SET_FLAG(flags, BMP_PEER_FLAG_V); SET_FLAG(flags, BMP_PEER_FLAG_V);
else else
UNSET_FLAG(flags, BMP_PEER_FLAG_V); UNSET_FLAG(flags, BMP_PEER_FLAG_V);
@ -325,7 +325,7 @@ static void bmp_per_peer_hdr(struct stream *s, struct bgp *bgp,
stream_putl(s, 0); stream_putl(s, 0);
stream_putl(s, 0); stream_putl(s, 0);
} else if (peer->connection->su.sa.sa_family == AF_INET6) } else if (peer->connection->su.sa.sa_family == AF_INET6)
stream_put(s, &peer->connection->su.sin6.sin6_addr, 16); stream_put(s, &peer->connection->su.sin6.sin6_addr, IPV6_MAX_BYTELEN);
else if (peer->connection->su.sa.sa_family == AF_INET) { else if (peer->connection->su.sa.sa_family == AF_INET) {
stream_putl(s, 0); stream_putl(s, 0);
stream_putl(s, 0); stream_putl(s, 0);
@ -398,7 +398,7 @@ static int bmp_send_initiation(struct bmp *bmp)
bmp_put_info_tlv(s, BMP_INFO_TYPE_SYSNAME, cmd_hostname_get()); bmp_put_info_tlv(s, BMP_INFO_TYPE_SYSNAME, cmd_hostname_get());
len = stream_get_endp(s); len = stream_get_endp(s);
stream_putl_at(s, BMP_LENGTH_POS, len); /*message length is set. */ stream_putl_at(s, BMP_LENGTH_POS, len); /* message length is set. */
pullwr_write_stream(bmp->pullwr, s); pullwr_write_stream(bmp->pullwr, s);
stream_free(s); stream_free(s);
@ -534,7 +534,7 @@ static struct stream *bmp_peerstate(struct peer *peer, bool down)
} }
len = stream_get_endp(s); len = stream_get_endp(s);
stream_putl_at(s, BMP_LENGTH_POS, len); /*message length is set. */ stream_putl_at(s, BMP_LENGTH_POS, len); /* message length is set. */
return s; return s;
} }
@ -884,7 +884,7 @@ static void bmp_eor(struct bmp *bmp, afi_t afi, safi_t safi, uint8_t flags,
/* skip this message if peer distinguisher is not available */ /* skip this message if peer distinguisher is not available */
if (bmp_get_peer_distinguisher(bmp, afi, peer_type_flag, if (bmp_get_peer_distinguisher(bmp, afi, peer_type_flag,
&peer_distinguisher)) { &peer_distinguisher)) {
zlog_debug( zlog_warn(
"skipping bmp message for reason: can't get peer distinguisher"); "skipping bmp message for reason: can't get peer distinguisher");
continue; continue;
} }
@ -1012,7 +1012,7 @@ static void bmp_monitor(struct bmp *bmp, struct peer *peer, uint8_t flags,
/* skip this message if peer distinguisher is not available */ /* skip this message if peer distinguisher is not available */
if (bmp_get_peer_distinguisher(bmp, afi, peer_type_flag, if (bmp_get_peer_distinguisher(bmp, afi, peer_type_flag,
&peer_distinguisher)) { &peer_distinguisher)) {
zlog_debug( zlog_warn(
"skipping bmp message for reason: can't get peer distinguisher"); "skipping bmp message for reason: can't get peer distinguisher");
return; return;
} }
@ -1155,8 +1155,10 @@ afibreak:
prefix_copy(&bmp->syncpos, bgp_dest_get_prefix(bn)); prefix_copy(&bmp->syncpos, bgp_dest_get_prefix(bn));
} }
if (bmp->targets->afimon[afi][safi] & BMP_MON_POSTPOLICY || if (CHECK_FLAG(bmp->targets->afimon[afi][safi],
bmp->targets->afimon[afi][safi] & BMP_MON_LOC_RIB) { BMP_MON_POSTPOLICY) ||
CHECK_FLAG(bmp->targets->afimon[afi][safi],
BMP_MON_LOC_RIB)) {
for (bpiter = bgp_dest_get_bgp_path_info(bn); bpiter; for (bpiter = bgp_dest_get_bgp_path_info(bn); bpiter;
bpiter = bpiter->next) { bpiter = bpiter->next) {
if (!CHECK_FLAG(bpiter->flags, if (!CHECK_FLAG(bpiter->flags,
@ -1173,7 +1175,8 @@ afibreak:
bpi = bpiter; bpi = bpiter;
} }
} }
if (bmp->targets->afimon[afi][safi] & BMP_MON_PREPOLICY) { if (CHECK_FLAG(bmp->targets->afimon[afi][safi],
BMP_MON_PREPOLICY)) {
for (adjiter = bn->adj_in; adjiter; for (adjiter = bn->adj_in; adjiter;
adjiter = adjiter->next) { adjiter = adjiter->next) {
if (adjiter->peer->qobj_node.nid if (adjiter->peer->qobj_node.nid
@ -1303,8 +1306,6 @@ static bool bmp_wrqueue_locrib(struct bmp *bmp, struct pullwr *pullwr)
/* currently syncing & haven't reached this prefix yet /* currently syncing & haven't reached this prefix yet
* => it'll be sent as part of the table sync, no need here * => it'll be sent as part of the table sync, no need here
*/ */
zlog_info(
"bmp: skipping direct monitor msg bc will be sent with sync :)");
goto out; goto out;
case BMP_AFI_LIVE: case BMP_AFI_LIVE:
break; break;
@ -1332,8 +1333,7 @@ static bool bmp_wrqueue_locrib(struct bmp *bmp, struct pullwr *pullwr)
struct bgp_path_info *bpi; struct bgp_path_info *bpi;
for (bpi = bn ? bgp_dest_get_bgp_path_info(bn) : NULL; bpi; for (bpi = bgp_dest_get_bgp_path_info(bn); bpi; bpi = bpi->next) {
bpi = bpi->next) {
if (!CHECK_FLAG(bpi->flags, BGP_PATH_SELECTED)) if (!CHECK_FLAG(bpi->flags, BGP_PATH_SELECTED))
continue; continue;
if (bpi->peer == peer) if (bpi->peer == peer)
@ -1402,10 +1402,10 @@ static bool bmp_wrqueue(struct bmp *bmp, struct pullwr *pullwr)
bn = bgp_safi_node_lookup(bmp->targets->bgp->rib[afi][safi], safi, bn = bgp_safi_node_lookup(bmp->targets->bgp->rib[afi][safi], safi,
&bqe->p, prd); &bqe->p, prd);
if (bmp->targets->afimon[afi][safi] & BMP_MON_POSTPOLICY) { if (CHECK_FLAG(bmp->targets->afimon[afi][safi], BMP_MON_POSTPOLICY)) {
struct bgp_path_info *bpi; struct bgp_path_info *bpi;
for (bpi = bn ? bgp_dest_get_bgp_path_info(bn) : NULL; bpi; for (bpi = bgp_dest_get_bgp_path_info(bn); bpi;
bpi = bpi->next) { bpi = bpi->next) {
if (!CHECK_FLAG(bpi->flags, BGP_PATH_VALID)) if (!CHECK_FLAG(bpi->flags, BGP_PATH_VALID))
continue; continue;
@ -1420,7 +1420,7 @@ static bool bmp_wrqueue(struct bmp *bmp, struct pullwr *pullwr)
written = true; written = true;
} }
if (bmp->targets->afimon[afi][safi] & BMP_MON_PREPOLICY) { if (CHECK_FLAG(bmp->targets->afimon[afi][safi], BMP_MON_PREPOLICY)) {
struct bgp_adj_in *adjin; struct bgp_adj_in *adjin;
for (adjin = bn ? bn->adj_in : NULL; adjin; for (adjin = bn ? bn->adj_in : NULL; adjin;
@ -1546,7 +1546,7 @@ static int bmp_process(struct bgp *bgp, afi_t afi, safi_t safi,
/* check if any monitoring is enabled (ignoring loc-rib since it /* check if any monitoring is enabled (ignoring loc-rib since it
* uses another hook & queue * uses another hook & queue
*/ */
if (!(bt->afimon[afi][safi] & ~BMP_MON_LOC_RIB)) if (!CHECK_FLAG(bt->afimon[afi][safi], ~BMP_MON_LOC_RIB))
continue; continue;
struct bmp_queue_entry *last_item = struct bmp_queue_entry *last_item =
@ -2455,6 +2455,9 @@ DEFPY(bmp_stats_cfg,
return CMD_SUCCESS; return CMD_SUCCESS;
} }
#define BMP_POLICY_IS_LOCRIB(str) ((str)[0] == 'l') /* __l__oc-rib */
#define BMP_POLICY_IS_PRE(str) ((str)[1] == 'r') /* p__r__e-policy */
DEFPY(bmp_monitor_cfg, bmp_monitor_cmd, DEFPY(bmp_monitor_cfg, bmp_monitor_cmd,
"[no] bmp monitor <ipv4|ipv6|l2vpn> <unicast|multicast|evpn|vpn> <pre-policy|post-policy|loc-rib>$policy", "[no] bmp monitor <ipv4|ipv6|l2vpn> <unicast|multicast|evpn|vpn> <pre-policy|post-policy|loc-rib>$policy",
NO_STR BMP_STR NO_STR BMP_STR
@ -2475,9 +2478,9 @@ DEFPY(bmp_monitor_cfg, bmp_monitor_cmd,
argv_find_and_parse_afi(argv, argc, &index, &afi); argv_find_and_parse_afi(argv, argc, &index, &afi);
argv_find_and_parse_safi(argv, argc, &index, &safi); argv_find_and_parse_safi(argv, argc, &index, &safi);
if (policy[0] == 'l') if (BMP_POLICY_IS_LOCRIB(policy))
flag = BMP_MON_LOC_RIB; flag = BMP_MON_LOC_RIB;
else if (policy[1] == 'r') else if (BMP_POLICY_IS_PRE(policy))
flag = BMP_MON_PREPOLICY; flag = BMP_MON_PREPOLICY;
else else
flag = BMP_MON_POSTPOLICY; flag = BMP_MON_POSTPOLICY;
@ -2615,15 +2618,17 @@ DEFPY(show_bmp,
continue; continue;
const char *pre_str = const char *pre_str =
afimon_flag & BMP_MON_PREPOLICY CHECK_FLAG(afimon_flag,
BMP_MON_PREPOLICY)
? "pre-policy " ? "pre-policy "
: ""; : "";
const char *post_str = const char *post_str =
afimon_flag & BMP_MON_POSTPOLICY CHECK_FLAG(afimon_flag,
BMP_MON_POSTPOLICY)
? "post-policy " ? "post-policy "
: ""; : "";
const char *locrib_str = const char *locrib_str =
afimon_flag & BMP_MON_LOC_RIB CHECK_FLAG(afimon_flag, BMP_MON_LOC_RIB)
? "loc-rib" ? "loc-rib"
: ""; : "";
@ -2750,14 +2755,16 @@ static int bmp_config_write(struct bgp *bgp, struct vty *vty)
vty_out(vty, " bmp mirror\n"); vty_out(vty, " bmp mirror\n");
FOREACH_AFI_SAFI (afi, safi) { FOREACH_AFI_SAFI (afi, safi) {
if (bt->afimon[afi][safi] & BMP_MON_PREPOLICY) if (CHECK_FLAG(bt->afimon[afi][safi],
BMP_MON_PREPOLICY))
vty_out(vty, " bmp monitor %s %s pre-policy\n", vty_out(vty, " bmp monitor %s %s pre-policy\n",
afi2str_lower(afi), safi2str(safi)); afi2str_lower(afi), safi2str(safi));
if (bt->afimon[afi][safi] & BMP_MON_POSTPOLICY) if (CHECK_FLAG(bt->afimon[afi][safi],
BMP_MON_POSTPOLICY))
vty_out(vty, vty_out(vty,
" bmp monitor %s %s post-policy\n", " bmp monitor %s %s post-policy\n",
afi2str_lower(afi), safi2str(safi)); afi2str_lower(afi), safi2str(safi));
if (bt->afimon[afi][safi] & BMP_MON_LOC_RIB) if (CHECK_FLAG(bt->afimon[afi][safi], BMP_MON_LOC_RIB))
vty_out(vty, " bmp monitor %s %s loc-rib\n", vty_out(vty, " bmp monitor %s %s loc-rib\n",
afi2str(afi), safi2str(safi)); afi2str(afi), safi2str(safi));
} }
@ -2808,7 +2815,6 @@ static int bgp_bmp_init(struct event_loop *tm)
return 0; return 0;
} }
/* TODO remove "bn", redundant with updated_route->net ? */
static int bmp_route_update(struct bgp *bgp, afi_t afi, safi_t safi, static int bmp_route_update(struct bgp *bgp, afi_t afi, safi_t safi,
struct bgp_dest *bn, struct bgp_dest *bn,
struct bgp_path_info *old_route, struct bgp_path_info *old_route,
@ -2818,17 +2824,24 @@ static int bmp_route_update(struct bgp *bgp, afi_t afi, safi_t safi,
bool is_withdraw = old_route && !new_route; bool is_withdraw = old_route && !new_route;
struct bgp_path_info *updated_route = struct bgp_path_info *updated_route =
is_withdraw ? old_route : new_route; is_withdraw ? old_route : new_route;
/* this should never happen */
if (!updated_route) {
zlog_warn("%s: no updated route found!", __func__);
return 0;
}
struct bmp_bgp *bmpbgp = bmp_bgp_get(bgp); struct bmp_bgp *bmpbgp = bmp_bgp_get(bgp);
struct peer *peer = updated_route->peer; struct peer *peer = updated_route->peer;
struct bmp_targets *bt; struct bmp_targets *bt;
struct bmp *bmp; struct bmp *bmp;
frr_each (bmp_targets, &bmpbgp->targets, bt) { frr_each (bmp_targets, &bmpbgp->targets, bt) {
is_locribmon_enabled |= if (CHECK_FLAG(bt->afimon[afi][safi], BMP_MON_LOC_RIB)) {
(bt->afimon[afi][safi] & BMP_MON_LOC_RIB); is_locribmon_enabled = true;
if (is_locribmon_enabled)
break; break;
}
} }
if (!is_locribmon_enabled) if (!is_locribmon_enabled)
@ -2847,7 +2860,7 @@ static int bmp_route_update(struct bgp *bgp, afi_t afi, safi_t safi,
monotime(NULL); monotime(NULL);
frr_each (bmp_targets, &bmpbgp->targets, bt) { frr_each (bmp_targets, &bmpbgp->targets, bt) {
if (bt->afimon[afi][safi] & BMP_MON_LOC_RIB) { if (CHECK_FLAG(bt->afimon[afi][safi], BMP_MON_LOC_RIB)) {
struct bmp_queue_entry *last_item = bmp_process_one( struct bmp_queue_entry *last_item = bmp_process_one(
bt, &bt->locupdhash, &bt->locupdlist, bgp, afi, bt, &bt->locupdhash, &bt->locupdlist, bgp, afi,

View File

@ -5,6 +5,7 @@
dev_RSTFILES = \ dev_RSTFILES = \
doc/developer/bgp-typecodes.rst \ doc/developer/bgp-typecodes.rst \
doc/developer/bgpd.rst \ doc/developer/bgpd.rst \
doc/developer/bmp.rst \
doc/developer/building-frr-for-alpine.rst \ doc/developer/building-frr-for-alpine.rst \
doc/developer/building-frr-for-archlinux.rst \ doc/developer/building-frr-for-archlinux.rst \
doc/developer/building-frr-for-centos6.rst \ doc/developer/building-frr-for-centos6.rst \

View File

@ -36,7 +36,7 @@ The `BMP` implementation in FRR has the following properties:
successfully. OPEN messages for failed sessions cannot currently be successfully. OPEN messages for failed sessions cannot currently be
mirrored. mirrored.
- **route monitoring** is available for IPv4 and IPv6 AFIs, unicast, multicast - **route monitoring** is available for IPv4 and IPv6 AFIs, unicast, multicast,
EVPN and VPN SAFIs. Other SAFIs (VPN, Labeled-Unicast, Flowspec, etc.) are not EVPN and VPN SAFIs. Other SAFIs (VPN, Labeled-Unicast, Flowspec, etc.) are not
currently supported. currently supported.