From 4c945dacb9ff9da731301feb26b1edb4ac00e8bd Mon Sep 17 00:00:00 2001 From: Andrew Yourtchenko Date: Thu, 15 Aug 2019 12:26:17 +0000 Subject: acl: fix stats-segment counters validation on acl update The stats-segment validation/clear logic for acl counters was wrong, fix it. Also add the code to the unittests to cover that case, add a vat command to enable/disable counters, clean up the unnecessary endian conversion and remove the stray clib_warning() Change-Id: I421297a92e4aeb885c468c72a97cec25981df615 Type: fix Ticket: VPP-1744 Fixes: f995c7122ba0d024b17bc3232e8edd18d5e25088 Signed-off-by: Andrew Yourtchenko --- src/plugins/acl/acl.c | 15 ++++++++---- src/plugins/acl/acl_test.c | 39 +++++++++++++++++++++++++++++--- test/test_acl_plugin_l2l3.py | 54 +++++++++++++++++++++++++++++--------------- 3 files changed, 82 insertions(+), 26 deletions(-) diff --git a/src/plugins/acl/acl.c b/src/plugins/acl/acl.c index 24dd53bd749..6b120563c9b 100644 --- a/src/plugins/acl/acl.c +++ b/src/plugins/acl/acl.c @@ -391,14 +391,19 @@ validate_and_reset_acl_counters (acl_main_t * am, u32 acl_index) /* filled in once only */ am->combined_acl_counters[i].stat_segment_name = (void *) format (0, "/acl/%d/matches%c", i, 0); - clib_warning ("add stats segment: %s", - am->combined_acl_counters[i].stat_segment_name); - i32 rule_count = vec_len (am->acls[acl_index].rules); + i32 rule_count = vec_len (am->acls[i].rules); /* Validate one extra so we always have at least one counter for an ACL */ vlib_validate_combined_counter (&am->combined_acl_counters[i], rule_count); - vlib_zero_combined_counter (&am->combined_acl_counters[i], rule_count); + vlib_clear_combined_counters (&am->combined_acl_counters[i]); } + + /* (re)validate for the actual ACL that is getting added/updated */ + i32 rule_count = vec_len (am->acls[acl_index].rules); + /* Validate one extra so we always have at least one counter for an ACL */ + vlib_validate_combined_counter (&am->combined_acl_counters[acl_index], + rule_count); + vlib_clear_combined_counters (&am->combined_acl_counters[acl_index]); acl_plugin_counter_unlock (am); } @@ -1945,7 +1950,7 @@ static void vl_api_acl_stats_intf_counters_enable_reply_t *rmp; int rv; - rv = acl_stats_intf_counters_enable_disable (am, ntohl (mp->enable)); + rv = acl_stats_intf_counters_enable_disable (am, mp->enable); REPLY_MACRO (VL_API_ACL_DEL_REPLY); } diff --git a/src/plugins/acl/acl_test.c b/src/plugins/acl/acl_test.c index 9185cc12329..ec951ad7057 100644 --- a/src/plugins/acl/acl_test.c +++ b/src/plugins/acl/acl_test.c @@ -68,7 +68,8 @@ _(acl_interface_add_del_reply) \ _(macip_acl_interface_add_del_reply) \ _(acl_interface_set_acl_list_reply) \ _(acl_interface_set_etype_whitelist_reply) \ -_(macip_acl_del_reply) +_(macip_acl_del_reply) \ +_(acl_stats_intf_counters_enable_reply) #define foreach_reply_retval_aclindex_handler \ _(acl_add_replace_reply) \ @@ -310,7 +311,8 @@ _(MACIP_ACL_INTERFACE_ADD_DEL_REPLY, macip_acl_interface_add_del_reply) \ _(MACIP_ACL_INTERFACE_GET_REPLY, macip_acl_interface_get_reply) \ _(ACL_PLUGIN_CONTROL_PING_REPLY, acl_plugin_control_ping_reply) \ _(ACL_PLUGIN_GET_VERSION_REPLY, acl_plugin_get_version_reply) \ -_(ACL_PLUGIN_GET_CONN_TABLE_MAX_ENTRIES_REPLY,acl_plugin_get_conn_table_max_entries_reply) +_(ACL_PLUGIN_GET_CONN_TABLE_MAX_ENTRIES_REPLY,acl_plugin_get_conn_table_max_entries_reply) \ +_(ACL_STATS_INTF_COUNTERS_ENABLE_REPLY, acl_stats_intf_counters_enable_reply) static int api_acl_plugin_get_version (vat_main_t * vam) { @@ -574,6 +576,36 @@ static int api_acl_plugin_get_conn_table_max_entries (vat_main_t * vam) return ret; } +static int api_acl_stats_intf_counters_enable (vat_main_t * vam) +{ + acl_test_main_t * sm = &acl_test_main; + unformat_input_t * i = vam->input; + vl_api_acl_stats_intf_counters_enable_t * mp; + u32 msg_size = sizeof(*mp); + int ret; + + vam->result_ready = 0; + mp = vl_msg_api_alloc_as_if_client(msg_size); + memset (mp, 0, msg_size); + mp->_vl_msg_id = ntohs (VL_API_ACL_STATS_INTF_COUNTERS_ENABLE + sm->msg_id_base); + mp->client_index = vam->my_client_index; + mp->enable = 1; + + while (unformat_check_input (i) != UNFORMAT_END_OF_INPUT) { + if (unformat (i, "disable")) + mp->enable = 0; + else + break; + } + + /* send it... */ + S(mp); + + /* Wait for a reply... */ + W (ret); + return ret; +} + /* * Read the series of ACL entries from file in the following format: @@ -1485,7 +1517,8 @@ _(macip_acl_del, "")\ _(macip_acl_dump, "[]") \ _(macip_acl_interface_add_del, " | sw_if_index [add|del] acl ") \ _(macip_acl_interface_get, "") \ -_(acl_plugin_get_conn_table_max_entries, "") +_(acl_plugin_get_conn_table_max_entries, "") \ +_(acl_stats_intf_counters_enable, "[disable]") static diff --git a/test/test_acl_plugin_l2l3.py b/test/test_acl_plugin_l2l3.py index 6b4ea476eb4..31b4058fc69 100644 --- a/test/test_acl_plugin_l2l3.py +++ b/test/test_acl_plugin_l2l3.py @@ -103,9 +103,11 @@ class TestACLpluginL2L3(VppTestCase): half = cls.remote_hosts_count // 2 cls.pg0.remote_hosts = cls.loop0.remote_hosts[:half] cls.pg1.remote_hosts = cls.loop0.remote_hosts[half:] + reply = cls.vapi.papi.acl_stats_intf_counters_enable(enable=1) @classmethod def tearDownClass(cls): + reply = cls.vapi.papi.acl_stats_intf_counters_enable(enable=0) super(TestACLpluginL2L3, cls).tearDownClass() def tearDown(self): @@ -471,6 +473,7 @@ class TestACLpluginL2L3(VppTestCase): acls=[acl_idx['L2']]) self.applied_acl_shuffle(self.pg0.sw_if_index) self.applied_acl_shuffle(self.pg2.sw_if_index) + return {'L2': acl_idx['L2'], 'L3': acl_idx['L3']} def apply_acl_ip46_both_directions_reflect(self, primary_is_bridged_to_routed, @@ -531,13 +534,21 @@ class TestACLpluginL2L3(VppTestCase): def apply_acl_ip46_routed_to_bridged(self, test_l2_deny, is_ip6, is_reflect, add_eh): - self.apply_acl_ip46_x_to_y(False, test_l2_deny, is_ip6, - is_reflect, add_eh) + return self.apply_acl_ip46_x_to_y(False, test_l2_deny, is_ip6, + is_reflect, add_eh) def apply_acl_ip46_bridged_to_routed(self, test_l2_deny, is_ip6, is_reflect, add_eh): - self.apply_acl_ip46_x_to_y(True, test_l2_deny, is_ip6, - is_reflect, add_eh) + return self.apply_acl_ip46_x_to_y(True, test_l2_deny, is_ip6, + is_reflect, add_eh) + + def verify_acl_packet_count(self, acl_idx, packet_count): + matches = self.statistics.get_counter('/acl/%d/matches' % acl_idx) + self.logger.info("stat seg for ACL %d: %s" % (acl_idx, repr(matches))) + total_count = 0 + for p in matches[0]: + total_count = total_count + p['packets'] + self.assertEqual(total_count, packet_count) def run_traffic_ip46_x_to_y(self, bridged_to_routed, test_l2_deny, is_ip6, @@ -560,34 +571,41 @@ class TestACLpluginL2L3(VppTestCase): packet_count = self.get_packet_count_for_if_idx(self.loop0.sw_if_index) rcvd1 = rx_if.get_capture(packet_count) self.verify_capture(self.loop0, self.pg2, rcvd1, bridged_to_routed) + return len(stream) def run_traffic_ip46_routed_to_bridged(self, test_l2_deny, is_ip6, is_reflect, is_established, add_eh, stateful_icmp=False): - self.run_traffic_ip46_x_to_y(False, test_l2_deny, is_ip6, - is_reflect, is_established, add_eh, - stateful_icmp) + return self.run_traffic_ip46_x_to_y(False, test_l2_deny, is_ip6, + is_reflect, is_established, add_eh, + stateful_icmp) def run_traffic_ip46_bridged_to_routed(self, test_l2_deny, is_ip6, is_reflect, is_established, add_eh, stateful_icmp=False): - self.run_traffic_ip46_x_to_y(True, test_l2_deny, is_ip6, - is_reflect, is_established, add_eh, - stateful_icmp) + return self.run_traffic_ip46_x_to_y(True, test_l2_deny, is_ip6, + is_reflect, is_established, add_eh, + stateful_icmp) def run_test_ip46_routed_to_bridged(self, test_l2_deny, is_ip6, is_reflect, add_eh): - self.apply_acl_ip46_routed_to_bridged(test_l2_deny, - is_ip6, is_reflect, add_eh) - self.run_traffic_ip46_routed_to_bridged(test_l2_deny, is_ip6, - is_reflect, False, add_eh) + acls = self.apply_acl_ip46_routed_to_bridged(test_l2_deny, + is_ip6, is_reflect, + add_eh) + pkts = self.run_traffic_ip46_routed_to_bridged(test_l2_deny, is_ip6, + is_reflect, False, + add_eh) + self.verify_acl_packet_count(acls['L3'], pkts) def run_test_ip46_bridged_to_routed(self, test_l2_deny, is_ip6, is_reflect, add_eh): - self.apply_acl_ip46_bridged_to_routed(test_l2_deny, - is_ip6, is_reflect, add_eh) - self.run_traffic_ip46_bridged_to_routed(test_l2_deny, is_ip6, - is_reflect, False, add_eh) + acls = self.apply_acl_ip46_bridged_to_routed(test_l2_deny, + is_ip6, is_reflect, + add_eh) + pkts = self.run_traffic_ip46_bridged_to_routed(test_l2_deny, is_ip6, + is_reflect, False, + add_eh) + self.verify_acl_packet_count(acls['L2'], pkts) def run_test_ip46_routed_to_bridged_and_back(self, test_l2_action, is_ip6, add_eh, -- cgit 1.2.3-korg