Merge "Make sure "dead vlan" ports cannot transmit packets"

neutron/agent/common/ovs_lib.py

@@ -371,13 +371,7 @@ class OVSBridge(BaseOVS):
         with self.ovsdb.transaction() as txn:
             txn.add(self.ovsdb.add_port(self.br_name, port_name,
                                         may_exist=False))
-            # NOTE(mangelajo): Port is added to dead vlan (4095) by default
+            self._set_port_dead(port_name, txn)
-            # until it's handled by the neutron-openvswitch-agent. Otherwise it
-            # becomes a trunk port on br-int (receiving traffic for all vlans),
-            # and also triggers issues on ovs-vswitchd related to the
-            # datapath flow revalidator thread, see lp#1767422
-            txn.add(self.ovsdb.db_set(
-                    'Port', port_name, ('tag', constants.DEAD_VLAN_TAG)))
 
             # TODO(mangelajo): We could accept attr tuples for the Port too
             # but, that could potentially break usage of this function in
@@ -388,22 +382,27 @@ class OVSBridge(BaseOVS):
                 txn.add(self.ovsdb.db_set('Interface', port_name,
                                           *interface_attr_tuples))
 
-        # NOTE(bence romsics): We are after the ovsdb transaction therefore
+    def _set_port_dead(self, port_name, txn):
-        # there's still a short time window between the port created and
+        # NOTE(mangelajo): Port is added to dead vlan (4095) by default
-        # the flow added in which the dead vlan tag is not pushed onto the
+        # until it's handled by the neutron-openvswitch-agent. Otherwise it
-        # frames arriving at these ports and because of that those frames may
+        # may trigger issues on ovs-vswitchd related to the
-        # get through. However before the transaction we cannot create the
+        # datapath flow revalidator thread, see lp#1767422
-        # flow because we don't have the ofport. And I'm not aware of a
+        txn.add(self.ovsdb.db_set(
-        # combined ovsdb+openflow transaction to do it inside the transaction.
+            'Port', port_name, ('tag', constants.DEAD_VLAN_TAG)))
-        if (self.br_name == cfg.CONF.OVS.integration_bridge):
+        # Just setting 'tag' to 4095 is not enough to prevent any traffic
-            self.add_flow(
+        # to/from new port because "access" ports do not have 802.1Q header
-                table=constants.LOCAL_SWITCHING,
+        # and hence are not matched by default 4095-dropping rule.
-                priority=constants.OPENFLOW_MAX_PRIORITY - 1,
+        # So we also set "vlan_mode" attribute to "trunk" and "trunks"=[4095]
-                in_port=self.get_port_ofport(port_name),
+        # With this OVS normal pipeline will allow only packets tagged with
-                actions='mod_vlan_vid:{:d},'
+        # 4095 from such ports, which normally not happens,
-                        'resubmit(,{:d})'.format(
+        # but even if it does - default rule in br-int will drop them anyway.
-                            constants.DEAD_VLAN_TAG,
+        # Thus untagged packets from such ports will also be dropped until
-                            constants.LOCAL_SWITCHING))
+        # ovs agent sets proper VLAN tag and clears vlan_mode to default
+        # ("access"). See lp#1930414 for details.
+        txn.add(self.ovsdb.db_set(
+            'Port', port_name, ('vlan_mode', 'trunk')))
+        txn.add(self.ovsdb.db_set(
+            'Port', port_name, ('trunks', constants.DEAD_VLAN_TAG)))
 
     def delete_port(self, port_name):
         self.ovsdb.del_port(port_name, self.br_name).execute()

neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py

@@ -1228,19 +1228,19 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin,
                 self.setup_arp_spoofing_protection(self.int_br,
                                                    port, port_detail)
             if cur_tag != lvm.vlan:
-                self.int_br.set_db_attribute(
+                ovsdb = self.int_br.ovsdb
-                    "Port", port.port_name, "tag", lvm.vlan)
+                with ovsdb.transaction() as txn:
-                # When changing the port's tag from DEAD_VLAN_TAG to
+                    txn.add(ovsdb.db_set(
-                # something else, also delete the previously dead port's
+                        'Port', port.port_name, ('tag', lvm.vlan)))
-                # push DEAD_VLAN_TAG flow which we installed from
+                    # When changing the port's tag from DEAD_VLAN_TAG to
-                # ovs_lib.OVSBridge.replace_port().
+                    # something else, also clear port's vlan_mode and trunks,
-                if (cur_tag == constants.DEAD_VLAN_TAG and port.ofport != -1):
+                    # which were set to make sure all packets are dropped.
-                    self.int_br.delete_flows(
+                    if (cur_tag == constants.DEAD_VLAN_TAG and
-                        cookie=ovs_lib.COOKIE_ANY,
+                            port.ofport != ovs_lib.INVALID_OFPORT):
-                        table=constants.LOCAL_SWITCHING,
+                        txn.add(ovsdb.db_clear(
-                        priority=constants.OPENFLOW_MAX_PRIORITY - 1,
+                            'Port', port.port_name, 'vlan_mode'))
-                        in_port=port.ofport,
+                        txn.add(ovsdb.db_clear(
-                        strict=True)
+                            'Port', port.port_name, 'trunks'))
 
             # update plugin about port status
             # FIXME(salv-orlando): Failures while updating device status

neutron/tests/common/conn_testers.py

@@ -20,7 +20,6 @@ from neutron_lib import constants
 from oslo_config import cfg
 from oslo_utils import uuidutils
 
-from neutron.agent.common import ovs_lib
 from neutron.common import utils as common_utils
 from neutron.plugins.ml2.drivers.openvswitch.agent.common import (
     constants as ovs_consts)
@@ -386,13 +385,6 @@ class OVSBaseConnectionTester(ConnectionTester):
             txn.add(
                 ovsdb.db_add(
                     'Port', port_name, 'other_config', {'tag': str(tag)}))
-        bridge.delete_flows(
-            cookie=ovs_lib.COOKIE_ANY,
-            table=ovs_consts.LOCAL_SWITCHING,
-            priority=ovs_consts.OPENFLOW_MAX_PRIORITY - 1,
-            in_port=bridge.get_port_ofport(port_name),
-            strict=True,
-        )
 
 
 class OVSConnectionTester(OVSBaseConnectionTester):

neutron/tests/common/net_helpers.py

@@ -858,6 +858,9 @@ class OVSPortFixture(PortFixture):
         # on ports that we intend to use as fake vm interfaces, they
         # need to be flat. This is related to lp#1767422
         self.bridge.clear_db_attribute("Port", port_name, "tag")
+        # Clear vlan_mode that is added for each new port. lp#1930414
+        self.bridge.clear_db_attribute("Port", port_name, "vlan_mode")
+        self.bridge.clear_db_attribute("Port", port_name, "trunks")
         self.addCleanup(self.bridge.delete_port, port_name)
         self.port = ip_lib.IPDevice(port_name, self.namespace)
 

neutron/tests/functional/agent/l2/base.py

@@ -465,13 +465,16 @@ class OVSAgentTestFramework(base.BaseOVSLinuxTestCase, OVSOFControllerHelper):
         self._plug_ports(network, ports, self.agent, bridge=phys_br,
                          namespace=namespace)
 
-        if network_type == 'flat':
+        for port in ports:
-            # NOTE(slaweq): for OVS implementations remove the DEAD VLAN tag
+            if network_type == 'flat':
-            # on ports that belongs to flat network. DEAD VLAN tag is added
+                # NOTE(slaweq): for OVS implementations remove the DEAD VLAN
-            # to each newly created port. This is related to lp#1767422
+                # tag on ports that belongs to flat network. DEAD VLAN tag is
-            for port in ports:
+                # added to each newly created port.
+                # This is related to lp#1767422
                 phys_br.clear_db_attribute("Port", port['vif_name'], "tag")
-        elif phys_segmentation_id and network_type == 'vlan':
+            elif phys_segmentation_id and network_type == 'vlan':
-            for port in ports:
                 phys_br.set_db_attribute(
                     "Port", port['vif_name'], "tag", phys_segmentation_id)
+            # Clear vlan_mode that is added for each new port. lp#1930414
+            phys_br.clear_db_attribute("Port", port['vif_name'], "vlan_mode")
+            phys_br.clear_db_attribute("Port", port['vif_name'], "trunks")

neutron/tests/functional/agent/l3/framework.py

@@ -98,6 +98,8 @@ class L3AgentTestFramework(base.BaseSudoTestCase):
         self.mock_plugin_api = mock.patch(
             'neutron.agent.l3.agent.L3PluginApi').start().return_value
         mock.patch('neutron.agent.rpc.PluginReportStateAPI').start()
+        mock.patch('neutron.agent.common.ovs_lib.'
+                   'OVSBridge._set_port_dead').start()
         l3_config.register_l3_agent_config_opts(l3_config.OPTS, cfg.CONF)
         self.conf = self._configure_agent('agent1')
         self.agent = neutron_l3_agent.L3NATAgentWithStateReport('agent1',

neutron/tests/functional/agent/test_dhcp_agent.py

@@ -35,7 +35,6 @@ from neutron.agent.linux import utils
 from neutron.agent.metadata import driver as metadata_driver
 from neutron.common import utils as common_utils
 from neutron.conf.agent import common as config
-from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants
 from neutron.tests.common import net_helpers
 from neutron.tests.functional.agent.linux import helpers
 from neutron.tests.functional import base
@@ -74,9 +73,8 @@ class DHCPAgentOVSTestFramework(base.BaseSudoTestCase):
             'interface_driver',
             'neutron.agent.linux.interface.OVSInterfaceDriver')
         self.conf.set_override('report_interval', 0, 'AGENT')
-        self.br_int = self.useFixture(net_helpers.OVSBridgeFixture()).bridge
+        br_int = self.useFixture(net_helpers.OVSBridgeFixture()).bridge
-        self.conf.set_override(
+        self.conf.set_override('integration_bridge', br_int.br_name, 'OVS')
-            'integration_bridge', self.br_int.br_name, 'OVS')
 
         self.mock_plugin_api = mock.patch(
             'neutron.agent.dhcp.agent.DhcpPluginApi').start().return_value
@@ -87,6 +85,9 @@ class DHCPAgentOVSTestFramework(base.BaseSudoTestCase):
 
         self.conf.set_override('check_child_processes_interval', 1, 'AGENT')
 
+        mock.patch('neutron.agent.common.ovs_lib.'
+                   'OVSBridge._set_port_dead').start()
+
     def network_dict_for_dhcp(self, dhcp_enabled=True,
                               ip_version=lib_const.IP_VERSION_4,
                               prefix_override=None):
@@ -293,6 +294,13 @@ class DHCPAgentOVSTestCase(DHCPAgentOVSTestFramework):
         dev = ip_lib.IPDevice(iface_name, network.namespace)
         self.assertEqual(789, dev.link.mtu)
 
+    def test_good_address_allocation(self):
+        network, port = self._get_network_port_for_allocation_test()
+        network.ports.append(port)
+        self.configure_dhcp_for_network(network=network)
+        self._plug_port_for_dhcp_request(network, port)
+        self.assert_good_allocation_for_port(network, port)
+
     def test_bad_address_allocation(self):
         network, port = self._get_network_port_for_allocation_test()
         network.ports.append(port)
@@ -443,25 +451,3 @@ class DHCPAgentOVSTestCase(DHCPAgentOVSTestFramework):
         else:
             self.assertEqual(agent.DHCP_PROCESS_GREENLET_MAX,
                              self.agent._pool.size)
-
-
-class DHCPAgentOVSTestCaseOwnBridge(DHCPAgentOVSTestFramework):
-    """Class for a single test that needs its own bridge.
-
-    This way the ofport numbers are predictable and we can fake ovs-agent's
-    flow deletions with hardcoded ofport numbers.
-    """
-
-    def test_good_address_allocation(self):
-        network, port = self._get_network_port_for_allocation_test()
-        network.ports.append(port)
-        self.configure_dhcp_for_network(network=network)
-        self._plug_port_for_dhcp_request(network, port)
-        for ofport in (1, 2):
-            self.br_int.delete_flows(
-                cookie=ovs_lib.COOKIE_ANY,
-                table=constants.LOCAL_SWITCHING,
-                priority=constants.OPENFLOW_MAX_PRIORITY - 1,
-                in_port=ofport,
-                strict=True)
-        self.assert_good_allocation_for_port(network, port)

neutron/tests/functional/agent/test_firewall.py

@@ -19,6 +19,7 @@
 
 import copy
 import functools
+from unittest import mock
 
 import netaddr
 from neutron_lib import constants
@@ -139,6 +140,8 @@ class BaseFirewallTestCase(linux_base.BaseOVSLinuxTestCase):
             conn_testers.OVSConnectionTester(self.ip_cidr,
                                              self.of_helper.br_int_cls))
         firewall_drv = openvswitch_firewall.OVSFirewallDriver(tester.bridge)
+        mock.patch('neutron.agent.common.ovs_lib.'
+                   'OVSBridge._set_port_dead').start()
         return tester, firewall_drv
 
     def assign_vlan_to_peers(self):

neutron/tests/functional/agent/test_ovs_lib.py

@@ -92,6 +92,10 @@ class OVSBridgeTestCase(OVSBridgeTestBase):
                                                     'external_ids')['test'])
         self.assertEqual(agent_const.DEAD_VLAN_TAG,
                          self.br.db_get_val('Port', port_name, 'tag'))
+        self.assertEqual("trunk",
+                         self.br.db_get_val('Port', port_name, 'vlan_mode'))
+        self.assertEqual(4095,
+                         self.br.db_get_val('Port', port_name, 'trunks'))
 
     def test_attribute_lifecycle(self):
         (port_name, ofport) = self.create_ovs_port()

neutron/tests/functional/services/trunk/drivers/openvswitch/agent/test_trunk_manager.py

@@ -163,6 +163,8 @@ class TrunkManagerTestCase(base.BaseSudoTestCase):
             self.tester.bridge)
         self.trunk = trunk_manager.TrunkParentPort(
             trunk_id, uuidutils.generate_uuid())
+        mock.patch('neutron.agent.common.ovs_lib.'
+                   'OVSBridge._set_port_dead').start()
 
     def test_connectivity(self):
         """Test connectivity with trunk and sub ports.