From b0a8f709a56d83d347bb544f77ba78de8fd587b6 Mon Sep 17 00:00:00 2001 From: Francois Dumontet Date: Tue, 8 Nov 2022 16:25:15 +0100 Subject: [PATCH] bgp: fix case where confederation id same as member-as currently the following configuration dut: ! interface ntfp2 ip router isis 1 ! router bgp 200 no bgp ebgp-requires-policy bgp confederation identifier 300 bgp confederation peers 300 neighbor 192.168.1.1 remote-as 100 neighbor 192.168.2.2 remote-as 300 ! address-family ipv4 unicast neighbor 192.168.2.2 default-originate exit-address-family ! router isis 1 is-type level-2-only net 49.0001.0002.0002.0002.00 redistribute ipv4 connected level-2 ! end router: ! interface ntfp2 ip router isis 1 isis circuit-type level-2-only ! router bgp 300 no bgp ebgp-requires-policy bgp confederation identifier 300 bgp confederation peers 200 neighbor 192.168.2.1 remote-as 200 neighbor 192.168.3.2 remote-as 400 ! address-family ipv4 unicast network 3.3.3.0/24 exit-address-family ! router isis 1 is-type level-2-only net 49.0001.0003.0003.0003.00 redistribute ipv4 connected level-2 ! end on dut result of show bgp ipv4 unicast command is: show bgp ipv4 unicast BGP table version is 1, local router ID is 192.168.2.1, vrf id 0 Default local pref 100, local AS 200 Status codes: s suppressed, d damped, h history, * valid, > best, = multipath, i internal, r RIB-failure, S Stale, R Removed Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self Origin codes: i - IGP, e - EGP, ? - incomplete RPKI validation codes: V valid, I invalid, N Not found Network Next Hop Metric LocPrf Weight Path *> 1.1.1.0/24 192.168.1.1 0 0 100 i instead of sho bgp ipv4 unicast BGP table version is 3, local router ID is 192.168.2.1, vrf id 0 Default local pref 100, local AS 200 Status codes: s suppressed, d damped, h history, * valid, > best, = multipath, i internal, r RIB-failure, S Stale, R Removed Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self Origin codes: i - IGP, e - EGP, ? - incomplete RPKI validation codes: V valid, I invalid, N Not found Network Next Hop Metric LocPrf Weight Path *> 1.1.1.0/24 192.168.1.1 0 0 100 i *> 3.3.3.0/24 192.168.2.2 0 100 0 (300) i *> 4.4.4.0/24 192.168.3.2 0 100 0 (300) 400 i Displayed 3 routes and 3 total paths According to RFC 5065:the usage of one of the member AS number as the confederation identifier is not forbidden. fixes are the following in bgp_route.c: in bgp_update remove the test for presence of confederation id in as_path since, this case is allowed; in bgp_vty.c bgp_confederation_peers, remove the test on peer as value in bgpd.c bgp_confederation_peers_add remove the test on peer as value invert the order of setting peer->sort value and peer->local_as, since peer->sort is depending from current peer->local_as value bgp_confederation_peers_remove invert the order of setting peer->sort value and peer->local_as, since peer->sort is depending from current peer->local_as value Signed-off-by: Francois Dumontet --- bgpd/bgp_aspath.c | 27 +++++++++++++++++++++++++++ bgpd/bgp_aspath.h | 1 + bgpd/bgp_route.c | 17 ++++++++++++----- bgpd/bgp_vty.c | 7 ------- bgpd/bgpd.c | 6 +++--- 5 files changed, 43 insertions(+), 15 deletions(-) diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c index 06f6073781..85f09ccf0b 100644 --- a/bgpd/bgp_aspath.c +++ b/bgpd/bgp_aspath.c @@ -1187,6 +1187,33 @@ int aspath_loop_check(struct aspath *aspath, as_t asno) return count; } +/* AS path loop check. If aspath contains asno + * that is a confed id then return >= 1. + */ +int aspath_loop_check_confed(struct aspath *aspath, as_t asno) +{ + struct assegment *seg; + int count = 0; + + if (aspath == NULL || aspath->segments == NULL) + return 0; + + seg = aspath->segments; + + while (seg) { + unsigned int i; + + for (i = 0; i < seg->length; i++) + if (seg->type != AS_CONFED_SEQUENCE && + seg->type != AS_CONFED_SET && seg->as[i] == asno) + count++; + + seg = seg->next; + } + return count; +} + + /* When all of AS path is private AS return 1. */ bool aspath_private_as_check(struct aspath *aspath) { diff --git a/bgpd/bgp_aspath.h b/bgpd/bgp_aspath.h index 0b58e1adc4..97bc7c0aca 100644 --- a/bgpd/bgp_aspath.h +++ b/bgpd/bgp_aspath.h @@ -111,6 +111,7 @@ extern unsigned int aspath_key_make(const void *p); extern unsigned int aspath_get_first_as(struct aspath *aspath); extern unsigned int aspath_get_last_as(struct aspath *aspath); extern int aspath_loop_check(struct aspath *aspath, as_t asno); +extern int aspath_loop_check_confed(struct aspath *aspath, as_t asno); extern bool aspath_private_as_check(struct aspath *aspath); extern struct aspath *aspath_replace_specific_asn(struct aspath *aspath, as_t target_asn, diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 130a0b4abd..24ce78a9ac 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -2197,7 +2197,7 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi, /* If we're a CONFED we need to loop check the CONFED ID too */ if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION)) { - if (aspath_loop_check(piattr->aspath, bgp->confed_id)) { + if (aspath_loop_check_confed(piattr->aspath, bgp->confed_id)) { if (bgp_debug_update(NULL, p, subgrp->update_group, 0)) zlog_debug( "%pBP [Update:SEND] suppress announcement to peer AS %u is AS path.", @@ -4113,16 +4113,23 @@ int bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id, /* AS path loop check. */ if (do_loop_check) { - if (aspath_loop_check(attr->aspath, bgp->as) > allowas_in || - (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION) && - (aspath_loop_check(attr->aspath, bgp->confed_id) > - allowas_in))) { + if (aspath_loop_check(attr->aspath, bgp->as) > + peer->allowas_in[afi][safi]) { peer->stat_pfx_aspath_loop++; reason = "as-path contains our own AS;"; goto filtered; } } + /* If we're a CONFED we need to loop check the CONFED ID too */ + if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION) && do_loop_check) + if (aspath_loop_check_confed(attr->aspath, bgp->confed_id) > + peer->allowas_in[afi][safi]) { + peer->stat_pfx_aspath_loop++; + reason = "as-path contains our own confed AS;"; + goto filtered; + } + /* Route reflector originator ID check. If ACCEPT_OWN mechanism is * enabled, then take care of that too. */ diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 1f66080e93..95c81909ce 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -1917,13 +1917,6 @@ DEFUN (bgp_confederation_peers, for (i = idx_asn; i < argc; i++) { as = strtoul(argv[i]->arg, NULL, 10); - - if (bgp->as == as) { - vty_out(vty, - "%% Local member-AS not allowed in confed peer list\n"); - continue; - } - bgp_confederation_peers_add(bgp, as); } return CMD_SUCCESS; diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 6ad1cf2c06..b198cd560a 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -671,7 +671,7 @@ void bgp_confederation_peers_add(struct bgp *bgp, as_t as) struct peer *peer; struct listnode *node, *nnode; - if (bgp->as == as) + if (!bgp) return; if (bgp_confederation_peers_check(bgp, as)) @@ -687,8 +687,8 @@ void bgp_confederation_peers_add(struct bgp *bgp, as_t as) if (bgp_config_check(bgp, BGP_CONFIG_CONFEDERATION)) { for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) { if (peer->as == as) { - (void)peer_sort(peer); peer->local_as = bgp->as; + (void)peer_sort(peer); if (BGP_IS_VALID_STATE_FOR_NOTIF( peer->status)) { peer->last_reset = @@ -738,8 +738,8 @@ void bgp_confederation_peers_remove(struct bgp *bgp, as_t as) if (bgp_config_check(bgp, BGP_CONFIG_CONFEDERATION)) { for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) { if (peer->as == as) { - (void)peer_sort(peer); peer->local_as = bgp->confed_id; + (void)peer_sort(peer); if (BGP_IS_VALID_STATE_FOR_NOTIF( peer->status)) { peer->last_reset =