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 8829cf0ceb2058ddccd20e1efbb703a349b0c271..04ef87d693060cf02f6a84a024abfae20efa752d 100644 (file)
--- 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 b99900af2f21119323d5ed439b7f01f41f2f1b5f..3a031e0db35c90aeeb2bedd3016a47b68ecf632e 100644 (file)
--- 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 7e6f99757df4e6be60804b764a18d214e2693ddc..4e978a954c36a843c00ed08f9a390b02cb53a6b5 100644 (file)
--- 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 8d5997f041c5911ca783a1e3aeaaa71ac40ae0d1..e46dfa705bb07adbe2f31282584352c3541edea3 100644 (file)
--- 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 e8bb2eb4441d7329a83634e8124539d5f545e529..ee7b046202974a0804ef17b4ffaee496d714b8c2 100644 (file)
--- 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))