acl-plugin: unapply/reapply the classifier-based inacls when performing macip_acl_add_replace on an existing MACIP ACL

The classifier tables layout might (and most always will) change during the MACIP ACL modification.
Furthermore, vnet_set_input_acl_intfc() is quite a picky creature - it quietly does nothing
if there is an existing inacl applied, even if the number is different, so a simple "reapply"
does not work. So, cleanly remove inacl, then reapply when the new tables are ready.

Also, fix the testcase which was supposed to test this exact behavior.

Thanks to Jon Loeliger for spotting this issue.

Change-Id: I7e4bd8023d9de7e914448bb4466c1b0ef6940f58
Signed-off-by: Andrew Yourtchenko <ayourtch@gmail.com>

2 files changed, 44 insertions, 20 deletions

diff --git a/src/plugins/acl/acl.c b/src/plugins/acl/acl.c
index dbb740abc55..286b2e96d1e 100644
--- a/src/plugins/acl/acl.c
+++ b/src/plugins/acl/acl.c
@@ -1503,6 +1503,27 @@ macip_destroy_classify_tables (acl_main_t * am, u32 macip_acl_index)
 }
 
 static int
+macip_maybe_apply_unapply_classifier_tables (acl_main_t * am, u32 acl_index,
+					     int is_apply)
+{
+  int rv = 0;
+  int rv0 = 0;
+  int i;
+  macip_acl_list_t *a = pool_elt_at_index (am->macip_acls, acl_index);
+
+  for (i = 0; i < vec_len (am->macip_acl_by_sw_if_index); i++)
+    if (vec_elt (am->macip_acl_by_sw_if_index, i) == acl_index)
+      {
+	rv0 = vnet_set_input_acl_intfc (am->vlib_main, i, a->ip4_table_index,
+					a->ip6_table_index, a->l2_table_index,
+					is_apply);
+	/* return the first unhappy outcome but make try to plough through. */
+	rv = rv || rv0;
+      }
+  return rv;
+}
+
+static int
 macip_acl_add_list (u32 count, vl_api_macip_acl_rule_t rules[],
 		    u32 * acl_list_index, u8 * tag)
 {
@@ -1511,6 +1532,7 @@ macip_acl_add_list (u32 count, vl_api_macip_acl_rule_t rules[],
   macip_acl_rule_t *r;
   macip_acl_rule_t *acl_new_rules = 0;
   int i;
+  int rv = 0;
 
   if (*acl_list_index != ~0)
     {
@@ -1531,6 +1553,9 @@ macip_acl_add_list (u32 count, vl_api_macip_acl_rule_t rules[],
 	("acl-plugin-warning: Trying to create empty MACIP ACL (tag %s)",
 	 tag);
     }
+  /* if replacing the ACL, unapply the classifier tables first - they will be gone.. */
+  if (~0 != *acl_list_index)
+    rv = macip_maybe_apply_unapply_classifier_tables (am, *acl_list_index, 0);
   void *oldheap = acl_set_heap (am);
   /* Create and populate the rules */
   if (count > 0)
@@ -1575,7 +1600,10 @@ macip_acl_add_list (u32 count, vl_api_macip_acl_rule_t rules[],
   /* Create and populate the classifer tables */
   macip_create_classify_tables (am, *acl_list_index);
   clib_mem_set_heap (oldheap);
-  return 0;
+  /* If the ACL was already applied somewhere, reapply the newly created tables */
+  rv = rv
+    || macip_maybe_apply_unapply_classifier_tables (am, *acl_list_index, 1);
+  return rv;
 }
 
 
diff --git a/test/test_acl_plugin_macip.py b/test/test_acl_plugin_macip.py
index d27458d7df0..bf87e02ecd9 100644
--- a/test/test_acl_plugin_macip.py
+++ b/test/test_acl_plugin_macip.py
@@ -588,7 +588,8 @@ class MethodHolder(VppTestCase):
 
     def run_traffic(self, mac_type, ip_type, traffic, is_ip6, packets,
                     do_not_expected_capture=False, tags=None,
-                    apply_rules=True, isMACIP=True, permit_tags=PERMIT_TAGS):
+                    apply_rules=True, isMACIP=True, permit_tags=PERMIT_TAGS,
+                    try_replace=False):
         self.reset_packet_infos()
 
         if tags is None:
@@ -650,6 +651,15 @@ class MethodHolder(VppTestCase):
             self.vapi.macip_acl_interface_add_del(
                 sw_if_index=tx_if.sw_if_index,
                 acl_index=0)
+        if try_replace:
+            if isMACIP:
+                reply = self.vapi.macip_acl_add_replace(
+                                                   test_dict['macip_rules'],
+                                                   acl_index)
+            else:
+                reply = self.vapi.acl_add_replace(acl_index=acl_index,
+                                                  r=test_dict['acl_rules'])
+            self.assertEqual(reply.retval, 0)
 
         if not isinstance(src_if, VppSubInterface):
             tx_if.add_stream(test_dict['stream'])
@@ -812,16 +822,9 @@ class TestMACIP_IP4(MethodHolder):
         """ MACIP replace ACL with IP4 traffic
         """
         self.run_traffic(self.OUI_MAC, self.SUBNET_IP,
-                         self.BRIDGED, self.IS_IP4, 9)
-
-        r = self.create_rules()
-        # replace acls #2, #3 with new
-        reply = self.vapi.macip_acl_add_replace(r[0], 0)
-        self.assertEqual(reply.retval, 0)
-        self.assertEqual(reply.acl_index, 0)
-
+                         self.BRIDGED, self.IS_IP4, 9, try_replace=True)
         self.run_traffic(self.EXACT_MAC, self.EXACT_IP,
-                         self.BRIDGED, self.IS_IP4, 9, True)
+                         self.BRIDGED, self.IS_IP4, 9, try_replace=True)
 
 
 class TestMACIP_IP6(MethodHolder):
@@ -957,16 +960,9 @@ class TestMACIP_IP6(MethodHolder):
         """ MACIP replace ACL with IP6 traffic
         """
         self.run_traffic(self.OUI_MAC, self.SUBNET_IP,
-                         self.BRIDGED, self.IS_IP6, 9)
-
-        r = self.create_rules()
-        # replace acls #2, #3 with new
-        reply = self.vapi.macip_acl_add_replace(r[0], 0)
-        self.assertEqual(reply.retval, 0)
-        self.assertEqual(reply.acl_index, 0)
-
+                         self.BRIDGED, self.IS_IP6, 9, try_replace=True)
         self.run_traffic(self.EXACT_MAC, self.EXACT_IP,
-                         self.BRIDGED, self.IS_IP6, 9, True)
+                         self.BRIDGED, self.IS_IP6, 9, try_replace=True)
 
 
 class TestMACIP(MethodHolder):