Merge pull request #3529 from anarkiwi/dot1xmanage

Remove stack ref from standalone, clean up and deflake 8021x.

Author: anarkiwi

faucet/valve.py

@@ -1783,20 +1783,16 @@ def reload_config(self, _now, new_dp):
         self.notify({'CONFIG_CHANGE': {'restart_type': restart_type}})
         return ofmsgs
 
-    def _del_native_vlan(self, port):
-        vlan_table = self.dp.tables['vlan']
-        ofmsg = vlan_table.flowdel(
-            vlan_table.match(in_port=port.number, vlan=port.native_vlan),
-            priority=self.dp.low_priority,
-        )
-        return [ofmsg]
-
-    def _warm_reconfig_port_vlans(self, port, vlans):
+    def _warm_reconfig_port_native_vlans(self, port, new_dyn_dot1x_native_vlan):
         ofmsgs = []
+        old_vlan = port.dyn_dot1x_native_vlan
         ofmsgs.extend(self.switch_manager.del_port(port))
+        port.dyn_dot1x_native_vlan = new_dyn_dot1x_native_vlan
+        for vlan in (old_vlan,) + (port.dyn_dot1x_native_vlan, port.native_vlan):
+            if vlan is not None:
+                vlan.reset_ports(self.dp.ports.values())
+                ofmsgs.extend(self.switch_manager.update_vlan(vlan))
         ofmsgs.extend(self.switch_manager.add_port(port))
-        for vlan in vlans:
-            ofmsgs.extend(self.switch_manager.update_vlan(vlan))
         return ofmsgs
 
     def add_dot1x_native_vlan(self, port_num, vlan_name):
@@ -1805,27 +1801,14 @@ def add_dot1x_native_vlan(self, port_num, vlan_name):
         vlans = [vlan for vlan in self.dp.vlans.values() if vlan.name == vlan_name]
         if vlans:
             vlan = vlans[0]
-            port.dyn_dot1x_native_vlan = vlan
-            vlan.reset_ports(self.dp.ports.values())
-            ofmsgs.extend(self._del_native_vlan(port))
-            ofmsgs.extend(self._warm_reconfig_port_vlans(
-                port, (port.dyn_dot1x_native_vlan, port.native_vlan)))
+            ofmsgs.extend(self._warm_reconfig_port_native_vlans(port, vlan))
         return ofmsgs
 
     def del_dot1x_native_vlan(self, port_num):
         ofmsgs = []
         port = self.dp.ports[port_num]
         if port.dyn_dot1x_native_vlan is not None:
-            dyn_vlan = port.dyn_dot1x_native_vlan
-            port.dyn_dot1x_native_vlan = None
-            dyn_vlan.reset_ports(self.dp.ports.values())
-            # Delete any existing native VLAN rule.
-            vlan_table = self.dp.tables['vlan']
-            ofmsgs.append(vlan_table.flowdel(
-                vlan_table.match(in_port=port.number, vlan=NullVLAN()),
-                priority=self.dp.low_priority))
-            ofmsgs.extend(self._warm_reconfig_port_vlans(
-                port, (dyn_vlan, port.native_vlan)))
+            ofmsgs.extend(self._warm_reconfig_port_native_vlans(port, None))
         return ofmsgs
 
     def router_vlan_for_ip_gw(self, vlan, ip_gw):

faucet/valve_switch_stack.py

@@ -467,6 +467,13 @@ def add_port(self, port):
                 inst=self.pipeline.accept_to_classification()))
         return ofmsgs
 
+    def del_port(self, port):
+        ofmsgs = super(ValveSwitchStackManagerBase, self).del_port(port)
+        if port.stack:
+            for vlan in self.vlans.values():
+                vlan.clear_cache_hosts_on_port(port)
+        return ofmsgs
+
 
 class ValveSwitchStackManagerNoReflection(ValveSwitchStackManagerBase):
     """Stacks of size 2 - all switches directly connected to root.

faucet/valve_switch_standalone.py

@@ -344,6 +344,12 @@ def _port_add_vlan_rules(self, port, vlan, mirror_act, push_vlan=True):
             self.vlan_table.match(in_port=port.number, vlan=match_vlan),
             priority=self.low_priority, inst=inst)
 
+    def _native_vlan(self, port):
+        for native_vlan in (port.dyn_dot1x_native_vlan, port.native_vlan):
+            if native_vlan is not None:
+                return native_vlan
+        return None
+
     def add_port(self, port):
         if port.vlans():
             mirror_act = port.mirror_actions()
@@ -352,11 +358,10 @@ def add_port(self, port):
                 tagged_ofmsgs.append(self._port_add_vlan_rules(
                     port, vlan, mirror_act, push_vlan=False))
             untagged_ofmsgs = []
-            for native_vlan in (port.dyn_dot1x_native_vlan, port.native_vlan):
-                if native_vlan is not None:
-                    untagged_ofmsgs.append(self._port_add_vlan_rules(
-                        port, native_vlan, mirror_act))
-                    break
+            native_vlan = self._native_vlan(port)
+            if native_vlan is not None:
+                untagged_ofmsgs.append(self._port_add_vlan_rules(
+                    port, native_vlan, mirror_act))
             # If no untagged VLANs, add explicit drop rule for untagged packets.
             if port.count_untag_vlan_miss and not untagged_ofmsgs:
                 untagged_ofmsgs.append(self.vlan_table.flowmod(
@@ -374,11 +379,13 @@ def del_port(self, port):
                 # per OF 1.3.5 B.6.23, the OFA will match flows
                 # that have an action targeting this port.
                 ofmsgs.append(table.flowdel(out_port=port.number))
-        vlans = port.vlans()
-        if port.stack:
-            vlans = self.vlans.values()
-        for vlan in vlans:
+        for vlan in port.vlans():
             vlan.clear_cache_hosts_on_port(port)
+        native_vlan = self._native_vlan(port)
+        if native_vlan is not None:
+            ofmsgs.append(self.vlan_table.flowdel(
+                self.vlan_table.match(in_port=port.number, vlan=port.native_vlan),
+                priority=self.low_priority))
         return ofmsgs
 
     def build_flood_rules(self, vlan, modify=False):

tests/integration/mininet_tests.py

@@ -362,9 +362,20 @@ def insert_dynamic_values(dot1x_expected_events):
                                 msg='expected event: {} not in events_that_happened {}'.format(
                                     expected_event, events_that_happened))
 
+    def _eapol_filter(self, fields):
+        return '(' + ' and '.join(('ether proto 0x888e',) + fields) + ')'
+
+    def _success_eapol_filter(self, expect_success):
+        eap_code = '0x04'
+        if expect_success:
+            eap_code = '0x03'
+        return self._eapol_filter(('ether[14:4] == 0x01000004', 'ether[18] == %s' % eap_code))
+
+    def _logoff_eapol_filter(self):
+        return self._eapol_filter(('ether[14:4] == 0x01020000',))
+
     def try_8021x(self, host, port_num, conf, and_logoff=False, terminate_wpasupplicant=False,
-                  wpasup_timeout=180, tcpdump_timeout=15, tcpdump_packets=10,
-                  expect_success=True):
+                  wpasup_timeout=180, tcpdump_timeout=30, expect_success=True):
         if expect_success:
             self.wait_8021x_flows(port_num)
         port_labels = self.port_labels(port_num)
@@ -380,7 +391,11 @@ def try_8021x(self, host, port_num, conf, and_logoff=False, terminate_wpasupplic
             'dp_dot1x_failure_total', default=0)
         dp_logoff_total = self.scrape_prometheus_var(
             'dp_dot1x_logoff_total', default=0)
-        tcpdump_filter = 'ether proto 0x888e'
+        tcpdump_filters = [self._success_eapol_filter(expect_success)]
+        if and_logoff:
+            tcpdump_filters.append(self._logoff_eapol_filter())
+        tcpdump_packets = len(tcpdump_filters)
+        tcpdump_filter = ' or '.join(tcpdump_filters)
         tcpdump_txt = self.tcpdump_helper(
             host, tcpdump_filter, [
                 lambda: self.wpa_supplicant_callback(
@@ -393,6 +408,8 @@ def try_8021x(self, host, port_num, conf, and_logoff=False, terminate_wpasupplic
             if not and_logoff:
                 self.wait_8021x_success_flows(host, port_num)
         success = 'Success' in tcpdump_txt
+        if expect_success != success:
+            return False
         new_success_total = self.scrape_prometheus_var(
             'port_dot1x_success_total', labels=port_labels, default=0)
         new_failure_total = self.scrape_prometheus_var(
@@ -405,8 +422,6 @@ def try_8021x(self, host, port_num, conf, and_logoff=False, terminate_wpasupplic
             'dp_dot1x_failure_total', default=0)
         new_dp_logoff_total = self.scrape_prometheus_var(
             'dp_dot1x_logoff_total', default=0)
-        if expect_success != success:
-            return False
         if expect_success and success:
             self.assertGreater(new_success_total, success_total)
             self.assertGreater(new_dp_success_total, dp_success_total)
@@ -735,11 +750,10 @@ class Faucet8021XIdentityOnPortUpTest(Faucet8021XBaseTest):
     def test_untagged(self):
         port_no1 = self.port_map['port_1']
 
-        # start wpa sup, logon, then send id request. should then be 2 success.
+        # start wpa sup, logon, then send id request.
         self.set_port_up(port_no1)
         self.assertTrue(self.try_8021x(
-            self.eapol1_host, port_no1, self.wpasupplicant_conf_1, and_logoff=False,
-            tcpdump_timeout=180, tcpdump_packets=6))
+            self.eapol1_host, port_no1, self.wpasupplicant_conf_1, and_logoff=False))
         self.set_port_down(port_no1)
         self.one_ipv4_ping(
             self.eapol1_host, self.ping_host.IP(),
@@ -749,15 +763,18 @@ def port_up(port):
             self.set_port_up(port)
             self.wait_8021x_flows(port)
 
-        tcpdump_filter = 'ether proto 0x888e'
+        username = 'user'
+        username_bytes = ''.join(('%2x' % ord(c) for c in username))
+        tcpdump_filter = ' or '.join((
+            self._success_eapol_filter(True),
+            self._eapol_filter(('ether[23:4] == 0x%s' % username_bytes,))))
         tcpdump_txt = self.tcpdump_helper(
             self.eapol1_host, tcpdump_filter, [
                 lambda: port_up(port_no1)],
-            timeout=80, vflags='-vvv', packets=10)
+            timeout=30, vflags='-vvv', packets=2)
         for req_str in (
-                'len 5, Request (1)', # assume that this is the identity request
-                'Identity: user', # supplicant replies with username
-                'Success', # supplicant success
+                'Identity: %s' % username,  # supplicant replies with username
+                'Success',  # supplicant success
                 ):
             self.assertTrue(req_str in tcpdump_txt, msg='%s not in %s' % (req_str, tcpdump_txt))