From: Donald Sharp Date: Fri, 12 Jun 2015 14:59:10 +0000 (-0700) Subject: Here we have an unsual confederations config, "router bgp X" and X-Git-Tag: frr-2.0-rc1~1330 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=66b199b2ff2a8b39064f913489fe331cbbec4383;p=matthieu%2Ffrr.git Here we have an unsual confederations config, "router bgp X" and "bgp confederation id X" are the same value. router bgp 1 bgp router-id 10.1.1.1 bgp confederation identifier 1 bgp confederation peers 24 35 neighbor 10.1.1.2 remote-as 24 neighbor 10.1.1.2 update-source lo neighbor 10.1.1.3 remote-as 1 neighbor 10.1.1.3 update-source lo The customer does this because they want to peer to 10.1.1.2 as a confed-external peer but peer with 10.1.1.3 as a normal iBGP peer. The bug was that we thought 10.1.1.3 was an EBGP peer so we did not send him LOCALPREF which caused the Juniper to send us a NOTIFICATION. I confirmed that quagga also sends a NOTIFICATION in this scenario. The fix is to add a check to see if router bgp X and bgp confederation identifier X are equal because that is a factor in determining if a peer is EBGP or IBGP Additional issues fixed in the this patch: We were not properly removing all AS_CONFED_SEQUENCEs/SETs from the aspath when advertising a route to an ebgp peer. This was due to two issues: We only called aspath_delete_confed_seq() if confederations were configured. We can RX as aspath with CONFED segments even if confederations are not configured. aspath_delete_confed_seq() was implemented based on the original confed RFC 3065 which basically said "remove all of the leading AS_CONFED_SEQUENCEs/SETs" where the new confed RFC 5065 says "remove ALL of the AS_CONFED_SEQUENCEs/SETs" peer-groups did not work for confed-external peers. peer_calc_sort() always returned BGP_PEER_EBGP for a confederations where the remote-as was not specified. The reason was the peer->as_type was AS_UNSPECIFIED but we checked if (peer->as_type != AS_SPECIFIED) return (peer->as_type == AS_INTERNAL ? BGP_PEER_IBGP : BGP_PEER_EBGP); After fixing that I found that when we got to the else where we checked for peer1 we could only possibly return BGP_PEER_IBGP or BGP_PEER_EBGP, we need to also be able to return BGP_PEER_CONFED. I changed this to return peer1->sort. "show ip bgp x.x.x.x" would always display "Local" for the aspath. This is because we were calling aspath_counts_hop() to determine if the aspath was empty. This is wrong though because CONFED segments do not count towards aspath hopcount. The fix is to null check aspath->segments to determine if the aspath is actually empty. "show ip bgp x.x.x.x" and "show ip bgp neighbor" always displayed "internal" or "external" and never "confed-internal" or "confed-external". This made troubleshooting difficult because I couldn't tell exactly what kind of peer I was dealing with. I added the confed-internal and confed-external output...also added a "peer-type" field in the json output for 'show ip bgp x.x.x.x' "show ip bgp peer-group" did not list the peer-group name if we hadn't determined the "type" (internal, external, etc) for the peer-group --- diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c index 8829cf0ceb..04ef87d693 100644 --- a/bgpd/bgp_aspath.c +++ b/bgpd/bgp_aspath.c @@ -1718,36 +1718,52 @@ aspath_cmp_left_confed (const struct aspath *aspath1, const struct aspath *aspat return 0; } -/* Delete all leading AS_CONFED_SEQUENCE/SET segments from aspath. - * See RFC3065, 6.1 c1 */ +/* Delete all AS_CONFED_SEQUENCE/SET segments from aspath. + * RFC 5065 section 4.1.c.1 + * + * 1) if any path segments of the AS_PATH are of the type + * AS_CONFED_SEQUENCE or AS_CONFED_SET, those segments MUST be + * removed from the AS_PATH attribute, leaving the sanitized + * AS_PATH attribute to be operated on by steps 2, 3 or 4. + */ struct aspath * aspath_delete_confed_seq (struct aspath *aspath) { - struct assegment *seg; + struct assegment *seg, *prev, *next; + char removed_confed_segment; if (!(aspath && aspath->segments)) return aspath; seg = aspath->segments; + removed_confed_segment = 0; + next = NULL; + prev = NULL; - /* "if the first path segment of the AS_PATH is - * of type AS_CONFED_SEQUENCE," - */ - if (aspath->segments->type != AS_CONFED_SEQUENCE) - return aspath; - - /* "... that segment and any immediately following segments - * of the type AS_CONFED_SET or AS_CONFED_SEQUENCE are removed - * from the AS_PATH attribute," - */ - while (seg && - (seg->type == AS_CONFED_SEQUENCE || seg->type == AS_CONFED_SET)) + while (seg) { - aspath->segments = seg->next; - assegment_free (seg); - seg = aspath->segments; + next = seg->next; + + if (seg->type == AS_CONFED_SEQUENCE || seg->type == AS_CONFED_SET) + { + /* This is the first segment in the aspath */ + if (aspath->segments == seg) + aspath->segments = seg->next; + else + prev->next = seg->next; + + assegment_free (seg); + removed_confed_segment = 1; + } + else + prev = seg; + + seg = next; } - aspath_str_update (aspath); + + if (removed_confed_segment) + aspath_str_update (aspath); + return aspath; } diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index b99900af2f..3a031e0db3 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -2287,11 +2287,13 @@ bgp_packet_attribute (struct bgp *bgp, struct peer *peer, { aspath = aspath_dup (attr->aspath); + /* Even though we may not be configured for confederations we may have + * RXed an AS_PATH with AS_CONFED_SEQUENCE or AS_CONFED_SET */ + aspath = aspath_delete_confed_seq (aspath); + if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION)) { - /* Strip the confed info, and then stuff our path CONFED_ID - on the front */ - aspath = aspath_delete_confed_seq (aspath); + /* Stuff our path CONFED_ID on the front */ aspath = aspath_add_seq (aspath, bgp->confed_id); } else diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 7e6f99757d..4e978a954c 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -6692,7 +6692,7 @@ route_vty_out (struct vty *vty, struct prefix *p, { if (json_paths) { - if (!attr->aspath->str || aspath_count_hops (attr->aspath) == 0) + if (!attr->aspath->str || !attr->aspath->segments) json_string = json_object_new_string("Local"); else json_string = json_object_new_string(attr->aspath->str); @@ -6986,7 +6986,7 @@ route_vty_out_detail (struct vty *vty, struct bgp *bgp, struct prefix *p, if (!json_paths) vty_out (vty, " "); - if (aspath_count_hops (attr->aspath) == 0) + if (!attr->aspath->segments) { if (json_paths) json_string = json_object_new_string("Local"); @@ -7344,26 +7344,56 @@ route_vty_out_detail (struct vty *vty, struct bgp *bgp, struct prefix *p, { if (binfo->peer->as == binfo->peer->local_as) { - if (json_paths) - json_object_object_add(json_path, "internal", json_boolean_true); + if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION)) + { + if (json_paths) + { + json_string = json_object_new_string("confed-internal"); + json_object_object_add(json_path, "peer-type", json_string); + } + else + { + vty_out (vty, ", confed-internal"); + } + } else - vty_out (vty, ", internal"); + { + if (json_paths) + { + json_string = json_object_new_string("internal"); + json_object_object_add(json_path, "peer-type", json_string); + } + else + { + vty_out (vty, ", internal"); + } + } } - else + else { if (bgp_confederation_peers_check(bgp, binfo->peer->as)) { if (json_paths) - json_object_object_add(json_path, "confed-external", json_boolean_true); + { + json_string = json_object_new_string("confed-external"); + json_object_object_add(json_path, "peer-type", json_string); + } else - vty_out (vty, ", confed-external"); + { + vty_out (vty, ", confed-external"); + } } else { if (json_paths) - json_object_object_add(json_path, "external", json_boolean_true); + { + json_string = json_object_new_string("external"); + json_object_object_add(json_path, "peer-type", json_string); + } else - vty_out (vty, ", external"); + { + vty_out (vty, ", external"); + } } } } diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 8d5997f041..e46dfa705b 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -9403,9 +9403,22 @@ bgp_show_peer (struct vty *vty, struct peer *p) " no-prepend" : "", CHECK_FLAG (p->flags, PEER_FLAG_LOCAL_AS_REPLACE_AS) ? " replace-as" : ""); - vty_out (vty, "%s link%s", - p->as == p->local_as ? "internal" : "external", - VTY_NEWLINE); + + /* peer type internal, external, confed-internal or confed-external */ + if (p->as == p->local_as) + { + if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION)) + vty_out (vty, "confed-internal link%s", VTY_NEWLINE); + else + vty_out (vty, "internal link%s", VTY_NEWLINE); + } + else + { + if (bgp_confederation_peers_check(bgp, p->as)) + vty_out (vty, "confed-external link%s", VTY_NEWLINE); + else + vty_out (vty, "external link%s", VTY_NEWLINE); + } /* Description. */ if (p->desc) @@ -10864,11 +10877,14 @@ bgp_show_one_peer_group (struct vty *vty, struct peer_group *group) if (conf->as_type == AS_SPECIFIED || conf->as_type == AS_EXTERNAL) { - vty_out (vty, "%sBGP peer-group %s, remote AS %d%s", + vty_out (vty, "%sBGP peer-group %s, remote AS %d%s", VTY_NEWLINE, group->name, conf->as, VTY_NEWLINE); } else if (conf->as_type == AS_INTERNAL) { - vty_out (vty, "%sBGP peer-group %s, remote AS %d%s", + vty_out (vty, "%sBGP peer-group %s, remote AS %d%s", VTY_NEWLINE, group->name, group->bgp->as, VTY_NEWLINE); + } else { + vty_out (vty, "%sBGP peer-group %s%s", + VTY_NEWLINE, group->name, VTY_NEWLINE); } if ((group->bgp->as == conf->as) || (conf->as_type == AS_INTERNAL)) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index e8bb2eb444..ee7b046202 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -830,17 +830,22 @@ peer_calc_sort (struct peer *peer) /* Peer-group */ if (CHECK_FLAG (peer->sflags, PEER_STATUS_GROUP)) { - if (peer->as_type != AS_SPECIFIED) - return (peer->as_type == AS_INTERNAL ? BGP_PEER_IBGP : BGP_PEER_EBGP); - else if (peer->as) + if (peer->as_type == AS_INTERNAL) + return BGP_PEER_IBGP; + + else if (peer->as_type == AS_EXTERNAL) + return BGP_PEER_EBGP; + + else if (peer->as_type == AS_SPECIFIED && peer->as) return (bgp->as == peer->as ? BGP_PEER_IBGP : BGP_PEER_EBGP); + else { struct peer *peer1; peer1 = listnode_head (peer->group->peer); + if (peer1) - return (peer1->local_as == peer1->as - ? BGP_PEER_IBGP : BGP_PEER_EBGP); + return peer1->sort; } return BGP_PEER_INTERNAL; } @@ -853,10 +858,20 @@ peer_calc_sort (struct peer *peer) if (peer->local_as == peer->as) { - if (peer->local_as == bgp->confed_id) - return BGP_PEER_EBGP; - else - return BGP_PEER_IBGP; + if (bgp->as == bgp->confed_id) + { + if (peer->local_as == bgp->as) + return BGP_PEER_IBGP; + else + return BGP_PEER_EBGP; + } + else + { + if (peer->local_as == bgp->confed_id) + return BGP_PEER_EBGP; + else + return BGP_PEER_IBGP; + } } if (bgp_confederation_peers_check (bgp, peer->as))