Merge "ovsfw: Fix overlapping MAC addresses on integration bridge"

doc/source/contributor/internals/openvswitch_firewall.rst

@@ -146,8 +146,8 @@ connections into separate conntrack zones.
 
  table=0,  priority=100,in_port=1 actions=load:0x1->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,71)
  table=0,  priority=100,in_port=2 actions=load:0x2->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,71)
- table=0,  priority=90,dl_dst=fa:16:3e:a4:22:10 actions=load:0x1->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,81)
+ table=0,  priority=90,dl_vlan=0x284,dl_dst=fa:16:3e:a4:22:10 actions=load:0x1->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,81)
- table=0,  priority=90,dl_dst=fa:16:3e:24:57:c7 actions=load:0x2->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,81)
+ table=0,  priority=90,dl_vlan=0x284,dl_dst=fa:16:3e:24:57:c7 actions=load:0x2->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,81)
  table=0,  priority=0 actions=NORMAL
 
 Following ``table 71`` implements arp spoofing protection, ip spoofing
@@ -264,8 +264,8 @@ remaining egress connections are sent to normal switching.
 
 ::
 
- table=73, priority=100,dl_dst=fa:16:3e:a4:22:10 actions=load:0x1->NXM_NX_REG5[],resubmit(,81)
+ table=73, priority=100,reg6=0x284,dl_dst=fa:16:3e:a4:22:10 actions=load:0x1->NXM_NX_REG5[],resubmit(,81)
- table=73, priority=100,dl_dst=fa:16:3e:24:57:c7 actions=load:0x2->NXM_NX_REG5[],resubmit(,81)
+ table=73, priority=100,reg6=0x284,dl_dst=fa:16:3e:24:57:c7 actions=load:0x2->NXM_NX_REG5[],resubmit(,81)
  table=73, priority=90,ct_state=+new-est,reg5=0x1 actions=ct(commit,zone=NXM_NX_REG6[0..15]),NORMAL
  table=73, priority=90,ct_state=+new-est,reg5=0x2 actions=ct(commit,zone=NXM_NX_REG6[0..15]),NORMAL
  table=73, priority=80,reg5=0x1 actions=NORMAL

neutron/agent/linux/openvswitch_firewall/firewall.py

@@ -408,6 +408,15 @@ class OVSFirewallDriver(firewall.FirewallDriver):
         for table in ovs_consts.OVS_FIREWALL_TABLES:
             self.int_br.br.add_flow(table=table, priority=0, actions='drop')
 
+    def get_ovs_port(self, port_id):
+        ovs_port = self.int_br.br.get_vif_port_by_id(port_id)
+        if not ovs_port:
+            raise exceptions.OVSFWPortNotFound(port_id=port_id)
+        return ovs_port
+
+    def _get_port_vlan_tag(self, port_name):
+        return get_tag_from_other_config(self.int_br.br, port_name)
+
     def get_ofport(self, port):
         port_id = port['device']
         return self.sg_port_map.ports.get(port_id)
@@ -418,15 +427,11 @@ class OVSFirewallDriver(firewall.FirewallDriver):
         If ofport is nonexistent, create and return one.
         """
         port_id = port['device']
-        ovs_port = self.int_br.br.get_vif_port_by_id(port_id)
+        ovs_port = self.get_ovs_port(port_id)
-        if not ovs_port:
-            raise exceptions.OVSFWPortNotFound(port_id=port_id)
-
         try:
             of_port = self.sg_port_map.ports[port_id]
         except KeyError:
-            port_vlan_id = get_tag_from_other_config(
+            port_vlan_id = self._get_port_vlan_tag(ovs_port.port_name)
-                self.int_br.br, ovs_port.port_name)
             of_port = OFPort(port, ovs_port, port_vlan_id)
             self.sg_port_map.create_port(of_port, port)
         else:
@@ -442,6 +447,7 @@ class OVSFirewallDriver(firewall.FirewallDriver):
 
     def prepare_port_filter(self, port):
         if not firewall.port_sec_enabled(port):
+            self._initialize_egress_no_port_security(port['device'])
             return
         old_of_port = self.get_ofport(port)
         of_port = self.get_or_create_ofport(port)
@@ -462,8 +468,10 @@ class OVSFirewallDriver(firewall.FirewallDriver):
         """
         if not firewall.port_sec_enabled(port):
             self.remove_port_filter(port)
+            self._initialize_egress_no_port_security(port['device'])
             return
         elif not self.is_port_managed(port):
+            self._remove_egress_no_port_security(port['device'])
             self.prepare_port_filter(port)
             return
         old_of_port = self.get_ofport(port)
@@ -519,6 +527,15 @@ class OVSFirewallDriver(firewall.FirewallDriver):
             self.conj_ip_manager.sg_removed(sg_id)
             self.sg_port_map.delete_sg(sg_id)
 
+    def process_trusted_ports(self, port_ids):
+        """Pass packets from these ports directly to ingress pipeline."""
+        for port_id in port_ids:
+            self._initialize_egress_no_port_security(port_id)
+
+    def remove_trusted_ports(self, port_ids):
+        for port_id in port_ids:
+            self._remove_egress_no_port_security(port_id)
+
     def filter_defer_apply_on(self):
         self._deferred = True
 
@@ -559,6 +576,7 @@ class OVSFirewallDriver(firewall.FirewallDriver):
             table=ovs_consts.TRANSIENT_TABLE,
             priority=90,
             dl_dst=port.mac,
+            dl_vlan='0x%x' % port.vlan_tag,
             actions='set_field:{:d}->reg{:d},'
                     'set_field:{:d}->reg{:d},'
                     'strip_vlan,resubmit(,{:d})'.format(
@@ -585,6 +603,50 @@ class OVSFirewallDriver(firewall.FirewallDriver):
                 actions='normal'
             )
 
+    def _initialize_egress_no_port_security(self, port_id):
+        ovs_port = self.get_ovs_port(port_id)
+        try:
+            vlan_tag = self._get_port_vlan_tag(ovs_port.port_name)
+        except exceptions.OVSFWTagNotFound:
+            # It's a patch port, don't set anything
+            return
+        self._add_flow(
+            table=ovs_consts.TRANSIENT_TABLE,
+            priority=100,
+            in_port=ovs_port.ofport,
+            actions='set_field:%d->reg%d,'
+                    'set_field:%d->reg%d,'
+                    'resubmit(,%d)' % (
+                        ovs_port.ofport,
+                        ovsfw_consts.REG_PORT,
+                        vlan_tag,
+                        ovsfw_consts.REG_NET,
+                        ovs_consts.ACCEPT_OR_INGRESS_TABLE)
+        )
+        self._add_flow(
+            table=ovs_consts.ACCEPT_OR_INGRESS_TABLE,
+            priority=80,
+            reg_port=ovs_port.ofport,
+            actions='normal'
+        )
+
+    def _remove_egress_no_port_security(self, port_id):
+        ovs_port = self.get_ovs_port(port_id)
+        try:
+            # Test if it's a patch port
+            self._get_port_vlan_tag(ovs_port.port_name)
+        except exceptions.OVSFWTagNotFound:
+            # It's a patch port, don't do anything
+            return
+        self._delete_flows(
+            table=ovs_consts.TRANSIENT_TABLE,
+            in_port=ovs_port.ofport
+        )
+        self._delete_flows(
+            table=ovs_consts.ACCEPT_OR_INGRESS_TABLE,
+            reg_port=ovs_port.ofport
+        )
+
     def _initialize_egress(self, port):
         """Identify egress traffic and send it to egress base"""
         self._initialize_egress_ipv6_icmp(port)
@@ -695,6 +757,7 @@ class OVSFirewallDriver(firewall.FirewallDriver):
             table=ovs_consts.ACCEPT_OR_INGRESS_TABLE,
             priority=100,
             dl_dst=port.mac,
+            reg_net=port.vlan_tag,
             actions='set_field:{:d}->reg{:d},resubmit(,{:d})'.format(
                 port.ofport,
                 ovsfw_consts.REG_PORT,
@@ -935,14 +998,15 @@ class OVSFirewallDriver(firewall.FirewallDriver):
         self._strict_delete_flow(
             priority=90,
             table=ovs_consts.TRANSIENT_TABLE,
-            dl_dst=port.mac)
+            dl_dst=port.mac,
+            dl_vlan=port.vlan_tag)
         self._strict_delete_flow(
             priority=100,
             table=ovs_consts.TRANSIENT_TABLE,
             in_port=port.ofport)
         self._delete_flows(reg_port=port.ofport)
         self._delete_flows(table=ovs_consts.ACCEPT_OR_INGRESS_TABLE,
-                           dl_dst=port.mac)
+                           dl_dst=port.mac, reg_net=port.vlan_tag)
 
     def delete_flows_for_ip_addresses(
             self, ip_addresses, direction, ethertype, vlan_tag):

neutron/tests/common/conn_testers.py

@@ -22,6 +22,8 @@ from oslo_utils import uuidutils
 from neutron.agent import firewall
 from neutron.common import constants as n_consts
 from neutron.common import utils as common_utils
+from neutron.plugins.ml2.drivers.openvswitch.agent.common import (
+    constants as ovs_consts)
 from neutron.plugins.ml2.drivers.openvswitch.agent.openflow.ovs_ofctl import (
     br_int)
 from neutron.tests.common import machine_fixtures
@@ -360,6 +362,12 @@ class ConnectionTester(fixtures.Fixture):
                 "At least one packet got reply from %s namespace to %s "
                 "address." % self._get_namespace_and_address(direction)))
 
+    def set_peer_port_as_patch_port(self):
+        pass
+
+    def set_peer_port_as_vm_port(self):
+        pass
+
 
 class OVSBaseConnectionTester(ConnectionTester):
 
@@ -419,9 +427,47 @@ class OVSConnectionTester(OVSBaseConnectionTester):
 
     def set_vm_tag(self, tag):
         self.set_tag(self._vm.port.name, self.bridge, tag)
+        self._vm.port.vlan_tag = tag
 
     def set_peer_tag(self, tag):
         self.set_tag(self._peer.port.name, self.bridge, tag)
+        self._peer.port.vlan_tag = tag
+
+    def set_peer_port_as_patch_port(self):
+        """As packets coming from tunneling bridges are always tagged with
+        local VLAN tag, this flows will simulate the behavior.
+        """
+        self.bridge.add_flow(
+            table=ovs_consts.LOCAL_SWITCHING,
+            priority=110,
+            vlan_tci=0,
+            in_port=self.bridge.get_port_ofport(self._peer.port.name),
+            actions='mod_vlan_vid:0x%x,'
+                    'resubmit(,%d)' % (
+                        self._peer.port.vlan_tag,
+                        ovs_consts.LOCAL_SWITCHING)
+        )
+        self.bridge.add_flow(
+            table=ovs_consts.TRANSIENT_TABLE,
+            priority=4,
+            dl_vlan='0x%x' % self._peer.port.vlan_tag,
+            actions='strip_vlan,normal'
+        )
+
+    def set_peer_port_as_vm_port(self):
+        """Remove flows simulating traffic from tunneling bridges.
+
+        This method is opposite to set_peer_port_as_patch_port().
+        """
+        self.bridge.delete_flows(
+            table=ovs_consts.LOCAL_SWITCHING,
+            vlan_tci=0,
+            in_port=self.bridge.get_port_ofport(self._peer.port.name),
+        )
+        self.bridge.delete_flows(
+            table=ovs_consts.TRANSIENT_TABLE,
+            dl_vlan='0x%x' % self._peer.port.vlan_tag,
+        )
 
 
 class OVSTrunkConnectionTester(OVSBaseConnectionTester):

neutron/tests/common/net_helpers.py

@@ -737,6 +737,7 @@ class OVSPortFixture(PortFixture):
                  hybrid_plug=False):
         super(OVSPortFixture, self).__init__(bridge, namespace, mac, port_id)
         self.hybrid_plug = hybrid_plug
+        self.vlan_tag = None
 
     def _create_bridge_fixture(self):
         return OVSBridgeFixture()

neutron/tests/fullstack/test_securitygroup.py

@@ -111,6 +111,8 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest):
              5. a remote security group member addition works, and
              6. an established connection stops by deleting a SG rule.
              7. test other protocol functionality by using SCTP protocol
+             8. test two vms with same mac on the same host in different
+                networks
         """
         index_to_sg = [0, 0, 1]
         if self.firewall_driver == 'iptables_hybrid':
@@ -272,3 +274,100 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest):
         self.assert_connection(
             vms[1].namespace, vms[0].namespace, vms[0].ip, 3366,
             net_helpers.NetcatTester.SCTP)
+
+        # 8. test two vms with same mac on the same host in different networks
+        self._test_overlapping_mac_addresses()
+
+    def _create_vm_on_host(
+            self, project_id, network_id, sg_id, host, mac_address=None):
+        if mac_address:
+            port = self.safe_client.create_port(
+                project_id, network_id, host.hostname,
+                security_groups=[sg_id], mac_address=mac_address)
+        else:
+            port = self.safe_client.create_port(
+                project_id, network_id, host.hostname,
+                security_groups=[sg_id])
+
+        return self.useFixture(
+            machine.FakeFullstackMachine(
+                host, network_id, project_id, self.safe_client,
+                neutron_port=port))
+
+    def _create_three_vms_first_has_static_mac(
+            self, project_id, allowed_port, subnet_cidr):
+        """Create three vms.
+
+        First VM has a static mac and is placed on first host. Second VM is
+        placed on the first host and third VM is placed on second host.
+        """
+        network = self.safe_client.create_network(project_id)
+        self.safe_client.create_subnet(
+            project_id, network['id'], subnet_cidr)
+        sg = self.safe_client.create_security_group(project_id)
+
+        self.safe_client.create_security_group_rule(
+            project_id, sg['id'],
+            direction='ingress',
+            ethertype=constants.IPv4,
+            protocol=constants.PROTO_NAME_TCP,
+            port_range_min=allowed_port, port_range_max=allowed_port)
+
+        vms = [self._create_vm_on_host(
+            project_id, network['id'], sg['id'], self.environment.hosts[0],
+            mac_address="fa:16:3e:de:ad:fe")]
+
+        if self.firewall_driver == 'iptables_hybrid':
+            # iptables lack isolation between agents, use only a single host
+            vms.extend([
+                self._create_vm_on_host(
+                    project_id, network['id'], sg['id'],
+                    self.environment.hosts[0])
+                for _ in range(2)])
+        else:
+            vms.extend([
+                self._create_vm_on_host(
+                    project_id, network['id'], sg['id'], host)
+                for host in self.environment.hosts[:2]])
+
+        map(lambda vm: vm.block_until_boot(), vms)
+        return vms
+
+    def verify_connectivity_between_vms(self, src_vm, dst_vm, protocol, port):
+        self.assert_connection(
+            src_vm.namespace, dst_vm.namespace, dst_vm.ip, port,
+            protocol)
+
+    def verify_no_connectivity_between_vms(
+            self, src_vm, dst_vm, protocol, port):
+        self.assert_no_connection(
+            src_vm.namespace, dst_vm.namespace, dst_vm.ip, port, protocol)
+
+    def _test_overlapping_mac_addresses(self):
+        project1 = uuidutils.generate_uuid()
+        p1_allowed = 4444
+
+        project2 = uuidutils.generate_uuid()
+        p2_allowed = 4445
+
+        p1_vms = self._create_three_vms_first_has_static_mac(
+            project1, p1_allowed, '20.0.2.0/24')
+        p2_vms = self._create_three_vms_first_has_static_mac(
+            project2, p2_allowed, '20.0.3.0/24')
+
+        have_connectivity = [
+            (p1_vms[0], p1_vms[1], p1_allowed),
+            (p1_vms[1], p1_vms[2], p1_allowed),
+            (p2_vms[0], p2_vms[1], p2_allowed),
+            (p2_vms[1], p2_vms[2], p2_allowed),
+        ]
+
+        for vm1, vm2, port in have_connectivity:
+            self.verify_connectivity_between_vms(
+                vm1, vm2, net_helpers.NetcatTester.TCP, port)
+            self.verify_connectivity_between_vms(
+                vm2, vm1, net_helpers.NetcatTester.TCP, port)
+            self.verify_no_connectivity_between_vms(
+                vm1, vm2, net_helpers.NetcatTester.TCP, port + 1)
+            self.verify_no_connectivity_between_vms(
+                vm2, vm1, net_helpers.NetcatTester.TCP, port + 1)

neutron/tests/functional/agent/test_firewall.py

@@ -111,6 +111,8 @@ class BaseFirewallTestCase(base.BaseSudoTestCase):
         # FIXME(jlibosva): We should consider to call prepare_port_filter with
         # deferred bridge depending on its performance
         self.firewall.prepare_port_filter(self.src_port_desc)
+        # Traffic coming from patch-port is always VLAN tagged
+        self.tester.set_peer_port_as_patch_port()
 
     def initialize_iptables(self):
         cfg.CONF.set_override('enable_ipset', self.enable_ipset,
@@ -542,6 +544,8 @@ class FirewallTestCase(BaseFirewallTestCase):
         self.assertEqual(packets_received, 0)
 
     def test_remote_security_groups(self):
+        self.tester.set_peer_port_as_vm_port()
+
         remote_sg_id = 'remote_sg_id'
         peer_port_desc = self._create_port_description(
             self.tester.peer_port_id,

neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py

@@ -476,6 +476,7 @@ class TestOVSFirewallDriver(base.BaseTestCase):
                         self.port_ofport, TESTING_VLAN_TAG,
                         ovs_consts.BASE_INGRESS_TABLE),
             dl_dst=self.port_mac,
+            dl_vlan='0x%x' % TESTING_VLAN_TAG,
             priority=90,
             table=ovs_consts.TRANSIENT_TABLE)
         filter_rule = mock.call(
@@ -498,8 +499,10 @@ class TestOVSFirewallDriver(base.BaseTestCase):
                      'security_groups': [1],
                      'port_security_enabled': False}
         self._prepare_security_group()
-        self.firewall.prepare_port_filter(port_dict)
+        with mock.patch.object(
-        self.assertFalse(self.mock_bridge.br.add_flow.called)
+                self.firewall, 'initialize_port_flows') as m_init_flows:
+            self.firewall.prepare_port_filter(port_dict)
+        self.assertFalse(m_init_flows.called)
 
     def test_prepare_port_filter_initialized_port(self):
         port_dict = {'device': 'port-id',
@@ -602,3 +605,51 @@ class TestOVSFirewallDriver(base.BaseTestCase):
             self.firewall._cleanup_stale_sg()
             sg_removed_mock.assert_called_once_with(1)
             delete_sg_mock.assert_called_once_with(1)
+
+    def test_get_ovs_port(self):
+        ovs_port = self.firewall.get_ovs_port('port_id')
+        self.assertEqual(self.fake_ovs_port, ovs_port)
+
+    def test_get_ovs_port_non_existent(self):
+        self.mock_bridge.br.get_vif_port_by_id.return_value = None
+        with testtools.ExpectedException(exceptions.OVSFWPortNotFound):
+            self.firewall.get_ovs_port('port_id')
+
+    def test__initialize_egress_no_port_security_sends_to_egress(self):
+        self.mock_bridge.br.db_get_val.return_value = {'tag': TESTING_VLAN_TAG}
+        self.firewall._initialize_egress_no_port_security('port_id')
+        expected_call = mock.call(
+            table=ovs_consts.TRANSIENT_TABLE,
+            priority=100,
+            in_port=self.fake_ovs_port.ofport,
+            actions='set_field:%d->reg%d,'
+                    'set_field:%d->reg%d,'
+                    'resubmit(,%d)' % (
+                        self.fake_ovs_port.ofport,
+                        ovsfw_consts.REG_PORT,
+                        TESTING_VLAN_TAG,
+                        ovsfw_consts.REG_NET,
+                        ovs_consts.ACCEPT_OR_INGRESS_TABLE)
+        )
+        calls = self.mock_bridge.br.add_flow.call_args_list
+        self.assertIn(expected_call, calls)
+
+    def test__initialize_egress_no_port_security_no_tag(self):
+        self.mock_bridge.br.db_get_val.return_value = {}
+        self.firewall._initialize_egress_no_port_security('port_id')
+        self.assertFalse(self.mock_bridge.br.add_flow.called)
+
+    def test__remove_egress_no_port_security_deletes_flow(self):
+        self.mock_bridge.br.db_get_val.return_value = {'tag': TESTING_VLAN_TAG}
+        self.firewall._remove_egress_no_port_security('port_id')
+        expected_call = mock.call(
+            table=ovs_consts.TRANSIENT_TABLE,
+            in_port=self.fake_ovs_port.ofport,
+        )
+        calls = self.mock_bridge.br.delete_flows.call_args_list
+        self.assertIn(expected_call, calls)
+
+    def test__remove_egress_no_port_security_no_tag(self):
+        self.mock_bridge.br.db_get_val.return_value = {}
+        self.firewall._remove_egress_no_port_security('port_id')
+        self.assertFalse(self.mock_bridge.br.delete_flows.called)