bgpd: 'set comm-list delete' stops as soon as it hits a community-list entry with a deny

'set comm-list delete' stops as soon as it hits a community-list entry with
a deny

Reviewed By: sharpd@cumulusnetworks.com
Testing Done:

'set comm-list FOO delete' stops evaluating the community-list as soon as
we hit
the first "delete" statement. This makes it impossible to use
community-lists
where you deny some subset of communities to delete and then permit all of
the
others.

This patch changes the behavior so that we no longer exit the
community-list at
the first delete statement. Here is our baseline, we are receiving multiple
communities from 10.1.1.2 for the 10.1.3.0/24 prefix.

qct-ly6-04# show ip bgp 10.1.3.0/24
  BGP routing table entry for 10.1.3.0/24
  Paths: (1 available, best #1, table Default-IP-Routing-Table)
  Not advertised to any peer
  Local
  10.1.1.2 (metric 20) from 10.1.1.2 (10.1.1.2)
    Origin IGP, metric 0, localpref 100, valid, internal, best
    Community: 1:1 1:2 1:3 20:1 20:2 20:3 99:1
    Last update: Wed Mar 4 13:50:36 2015

qct-ly6-04#

We apply the following FOO route-map inbound to this peer and soft clear
the peer
!
ip community-list expanded BAD_COMMS permit 99:.*
ip community-list expanded BAD_COMMS deny 1:.*
ip community-list expanded BAD_COMMS permit 20.*
!
route-map FOO permit 10
set comm-list BAD_COMMS delete
!
router bgp 10
neighbor 10.1.1.2 route-map FOO in
!

qct-ly6-04# clear ip bgp * soft in
qct-ly6-04# show ip bgp 10.1.3.0/24
  BGP routing table entry for 10.1.3.0/24
  Paths: (1 available, best #1, table Default-IP-Routing-Table)
  Not advertised to any peer
  Local
  10.1.1.2 (metric 20) from 10.1.1.2 (10.1.1.2)
    Origin IGP, metric 0, localpref 100, valid, internal, best
    Community: 1:1 1:2 1:3
    Last update: Wed Mar 4 13:51:12 2015

qct-ly6-04#
qct-ly6-04#

We deleted all communities flagged as "permit" by the BAD_COMMS
community-list
while leaving the ones matched by "deny 1:.*" alone.

 #endif /* _QUAGGA_BGP_COMMUNITY_H */

'set comm-list delete' stops as soon as it hits a community-list entry with a deny

Ticket: CM-3513
Reviewed By: sharpd@cumulusnetworks.com
Testing Done:

'set comm-list FOO delete' stops evaluating the community-list as soon as we hit
the first "delete" statement. This makes it impossible to use community-lists
where you deny some subset of communities to delete and then permit all of the
others.

This patch changes the behavior so that we no longer exit the community-list at
the first delete statement. Here is our baseline, we are receiving multiple
communities from 10.1.1.2 for the 10.1.3.0/24 prefix.

qct-ly6-04# show ip bgp 10.1.3.0/24
  BGP routing table entry for 10.1.3.0/24
  Paths: (1 available, best #1, table Default-IP-Routing-Table)
  Not advertised to any peer
  Local
  10.1.1.2 (metric 20) from 10.1.1.2 (10.1.1.2)
    Origin IGP, metric 0, localpref 100, valid, internal, best
    Community: 1:1 1:2 1:3 20:1 20:2 20:3 99:1
    Last update: Wed Mar 4 13:50:36 2015

qct-ly6-04#

We apply the following FOO route-map inbound to this peer and soft clear the peer
!
ip community-list expanded BAD_COMMS permit 99:.*
ip community-list expanded BAD_COMMS deny 1:.*
ip community-list expanded BAD_COMMS permit 20.*
!
route-map FOO permit 10
set comm-list BAD_COMMS delete
!
router bgp 10
neighbor 10.1.1.2 route-map FOO in
!

qct-ly6-04# clear ip bgp * soft in
qct-ly6-04# show ip bgp 10.1.3.0/24
  BGP routing table entry for 10.1.3.0/24
  Paths: (1 available, best #1, table Default-IP-Routing-Table)
  Not advertised to any peer
  Local
  10.1.1.2 (metric 20) from 10.1.1.2 (10.1.1.2)
    Origin IGP, metric 0, localpref 100, valid, internal, best
    Community: 1:1 1:2 1:3
    Last update: Wed Mar 4 13:51:12 2015

qct-ly6-04#
qct-ly6-04#

We deleted all communities flagged as "permit" by the BAD_COMMS community-list
while leaving the ones matched by "deny 1:.*" alone.

3 files changed, 139 insertions, 76 deletions

diff --git a/bgpd/bgp_clist.c b/bgpd/bgp_clist.c
index b91ab81f..db2cfa48 100644
--- a/bgpd/bgp_clist.c
+++ b/bgpd/bgp_clist.c
@@ -330,6 +330,94 @@ community_list_entry_lookup (struct community_list *list, const void *arg,
   return NULL;
 }
 
+static char *
+community_str_get (struct community *com, int i)
+{
+  int len;
+  u_int32_t comval;
+  u_int16_t as;
+  u_int16_t val;
+  char *str;
+  char *pnt;
+
+  memcpy (&comval, com_nthval (com, i), sizeof (u_int32_t));
+  comval = ntohl (comval);
+
+  switch (comval)
+    {
+      case COMMUNITY_INTERNET:
+        len = strlen (" internet");
+        break;
+      case COMMUNITY_NO_EXPORT:
+        len = strlen (" no-export");
+        break;
+      case COMMUNITY_NO_ADVERTISE:
+        len = strlen (" no-advertise");
+        break;
+      case COMMUNITY_LOCAL_AS:
+        len = strlen (" local-AS");
+        break;
+      default:
+        len = strlen (" 65536:65535");
+        break;
+    }
+
+  /* Allocate memory.  */
+  str = pnt = XMALLOC (MTYPE_COMMUNITY_STR, len);
+
+  switch (comval)
+    {
+      case COMMUNITY_INTERNET:
+        strcpy (pnt, "internet");
+        pnt += strlen ("internet");
+        break;
+      case COMMUNITY_NO_EXPORT:
+        strcpy (pnt, "no-export");
+        pnt += strlen ("no-export");
+        break;
+      case COMMUNITY_NO_ADVERTISE:
+        strcpy (pnt, "no-advertise");
+        pnt += strlen ("no-advertise");
+        break;
+      case COMMUNITY_LOCAL_AS:
+        strcpy (pnt, "local-AS");
+        pnt += strlen ("local-AS");
+        break;
+      default:
+        as = (comval >> 16) & 0xFFFF;
+        val = comval & 0xFFFF;
+        sprintf (pnt, "%u:%d", as, val);
+        pnt += strlen (pnt);
+        break;
+    }
+
+  *pnt = '\0';
+
+  return str;
+}
+
+/* Internal function to perform regular expression match for
+ *  * a single community. */
+static int
+community_regexp_include (regex_t * reg, struct community *com, int i)
+{
+  const char *str;
+
+  /* When there is no communities attribute it is treated as empty
+ *      string.  */
+  if (com == NULL || com->size == 0)
+    str = "";
+  else
+    str = community_str_get (com, i);
+
+  /* Regular expression match.  */
+  if (regexec (reg, str, 0, NULL, 0) == 0)
+    return 1;
+
+  /* No match.  */
+  return 0;
+}
+
 /* Internal function to perform regular expression match for community
    attribute.  */
 static int
@@ -372,54 +460,6 @@ ecommunity_regexp_match (struct ecommunity *ecom, regex_t * reg)
   return 0;
 }
 
-/* Delete community attribute using regular expression match.  Return
-   modified communites attribute.  */
-static struct community *
-community_regexp_delete (struct community *com, regex_t * reg)
-{
-  int i;
-  u_int32_t comval;
-  /* Maximum is "65535:65535" + '\0'. */
-  char c[12];
-  const char *str;
-
-  if (!com)
-    return NULL;
-
-  i = 0;
-  while (i < com->size)
-    {
-      memcpy (&comval, com_nthval (com, i), sizeof (u_int32_t));
-      comval = ntohl (comval);
-
-      switch (comval)
-        {
-        case COMMUNITY_INTERNET:
-          str = "internet";
-          break;
-        case COMMUNITY_NO_EXPORT:
-          str = "no-export";
-          break;
-        case COMMUNITY_NO_ADVERTISE:
-          str = "no-advertise";
-          break;
-        case COMMUNITY_LOCAL_AS:
-          str = "local-AS";
-          break;
-        default:
-          sprintf (c, "%d:%d", (comval >> 16) & 0xFFFF, comval & 0xFFFF);
-          str = c;
-          break;
-        }
-
-      if (regexec (reg, str, 0, NULL, 0) == 0)
-        community_del_val (com, com_nthval (com, i));
-      else
-        i++;
-    }
-  return com;
-}
-
 /* When given community attribute matches to the community-list return
    1 else return 0.  */
 int
@@ -509,41 +549,63 @@ community_list_match_delete (struct community *com,
                              struct community_list *list)
 {
   struct community_entry *entry;
+  u_int32_t val;
+  u_int32_t com_index_to_delete[com->size];
+  int delete_index = 0;
+  int i;
 
-  for (entry = list->head; entry; entry = entry->next)
+  /* Loop over each community value and evaluate each against the
+   * community-list.  If we need to delete a community value add its index to
+   * com_index_to_delete.
+   */
+  for (i = 0; i < com->size; i++)
     {
-      if (entry->any)
+      val = community_val_get (com, i);
+
+      for (entry = list->head; entry; entry = entry->next)
         {
-          if (entry->direct == COMMUNITY_PERMIT) 
+          if (entry->any)
             {
-              /* This is a tricky part.  Currently only
-               * route_set_community_delete() uses this function.  In the
-               * function com->size is zero, it free the community
-               * structure.  
-               */
-              com->size = 0;
+              if (entry->direct == COMMUNITY_PERMIT)
+                {
+                  com_index_to_delete[delete_index] = i;
+                  delete_index++;
+                }
+              break;
             }
-          return com;
-        }
 
-      if ((entry->style == COMMUNITY_LIST_STANDARD) 
-          && (community_include (entry->u.com, COMMUNITY_INTERNET)
-              || community_match (com, entry->u.com) ))
-        {
+          else if ((entry->style == COMMUNITY_LIST_STANDARD)
+                   && (community_include (entry->u.com, COMMUNITY_INTERNET)
+                       || community_include (entry->u.com, val) ))
+            {
               if (entry->direct == COMMUNITY_PERMIT)
-                community_delete (com, entry->u.com);
-              else
-                break;
-        }
-      else if ((entry->style == COMMUNITY_LIST_EXPANDED)
-               && community_regexp_match (com, entry->reg))
-        {
-          if (entry->direct == COMMUNITY_PERMIT)
-            community_regexp_delete (com, entry->reg);
-          else
-            break;
-        }
+                {
+                  com_index_to_delete[delete_index] = i;
+                  delete_index++;
+                }
+              break;
+            }
+
+          else if ((entry->style == COMMUNITY_LIST_EXPANDED)
+                   && community_regexp_include (entry->reg, com, i))
+            {
+              if (entry->direct == COMMUNITY_PERMIT)
+                {
+                  com_index_to_delete[delete_index] = i;
+                  delete_index++;
+                }
+              break;
+            }
+         }
+     }
+
+  /* Delete all of the communities we flagged for deletion */
+  for (i = delete_index-1; i >= 0; i--)
+    {
+      val = community_val_get (com, com_index_to_delete[i]);
+      community_del_val (com, &val);
     }
+
   return com;
 }
 
diff --git a/bgpd/bgp_community.c b/bgpd/bgp_community.c
index 1bd2dd84..f1997bd9 100644
--- a/bgpd/bgp_community.c
+++ b/bgpd/bgp_community.c
@@ -144,7 +144,7 @@ community_include (struct community *com, u_int32_t val)
   return 0;
 }
 
-static u_int32_t
+u_int32_t
 community_val_get (struct community *com, int i)
 {
   u_char *p;
diff --git a/bgpd/bgp_community.h b/bgpd/bgp_community.h
index 9e483770..c73dab30 100644
--- a/bgpd/bgp_community.h
+++ b/bgpd/bgp_community.h
@@ -70,5 +70,6 @@ extern int community_include (struct community *, u_int32_t);
 extern void community_del_val (struct community *, u_int32_t *);
 extern unsigned long community_count (void);
 extern struct hash *community_hash (void);
+extern u_int32_t community_val_get (struct community *com, int i);
 
 #endif /* _QUAGGA_BGP_COMMUNITY_H */