Do not apply security groups to logical ports

Security groups rules were applied to logical ports and this caused
Floating IP not to work correctly.
Correct some typo.

Change-Id: I174e60f8eb8a4d00b71fffc127e2d2d36836835d
Closes-Bug: #1221336

neutron/plugins/midonet/agent/midonet_driver.py

@@ -120,10 +120,11 @@ class MidonetInterfaceDriver(interface.LinuxInterfaceDriver):
                           host_uuid)
                 raise e
             try:
-                self.mido_api.host.add_host_interface_port(
+                self.mido_api.add_host_interface_port(
                     host, vport_id, host_dev_name)
-            except w_exc.HTTPError as e:
+            except w_exc.HTTPError:
-                LOG.warn(_('Faild binding vport=%(vport) to device=%(device)'),
+                LOG.warn(_(
+                         'Faild binding vport=%(vport)s to device=%(device)s'),
                          {"vport": vport_id, "device": host_dev_name})
         else:
             LOG.warn(_("Device %s already exists"), device_name)

neutron/plugins/midonet/common/net_util.py

@@ -56,6 +56,10 @@ def get_protocol_value(protocol):
     """Convert string representation of protocol to the numerical."""
     if protocol is None:
         return None
+
+    if isinstance(protocol, int):
+        return protocol
+
     mapping = {
         'tcp': constants.TCP_PROTOCOL,
         'udp': constants.UDP_PROTOCOL,

neutron/plugins/midonet/plugin.py

@@ -295,10 +295,11 @@ class MidonetPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
 
     def _bind_port_to_sgs(self, context, port, sg_ids):
         self._process_port_create_security_group(context, port, sg_ids)
-        for sg_id in sg_ids:
+        if sg_ids is not None:
-            pg_name = _sg_port_group_name(sg_id)
+            for sg_id in sg_ids:
-            self.client.add_port_to_port_group_by_name(port["tenant_id"],
+                pg_name = _sg_port_group_name(sg_id)
-                                                       pg_name, port["id"])
+                self.client.add_port_to_port_group_by_name(
+                    port["tenant_id"], pg_name, port["id"])
 
     def _unbind_port_from_sgs(self, context, port_id):
         self._delete_port_security_group_bindings(context, port_id)
@@ -477,7 +478,6 @@ class MidonetPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
     def create_port(self, context, port):
         """Create a L2 port in Neutron/MidoNet."""
         LOG.debug(_("MidonetPluginV2.create_port called: port=%r"), port)
-
         port_data = port['port']
 
         # Create a bridge port in MidoNet and set the bridge port ID as the
@@ -493,39 +493,43 @@ class MidonetPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
                 new_port = super(MidonetPluginV2, self).create_port(context,
                                                                     port)
                 port_data.update(new_port)
-
+                self._ensure_default_security_group_on_port(context,
-                # Bind security groups to the port
+                                                            port)
-                sg_ids = self._get_security_groups_on_port(context, port)
+                if _is_vif_port(port_data):
-                if sg_ids:
+                    # Bind security groups to the port
+                    sg_ids = self._get_security_groups_on_port(context, port)
                     self._bind_port_to_sgs(context, port_data, sg_ids)
-                port_data[ext_sg.SECURITYGROUPS] = sg_ids
 
-                # Create port chains
+                    # Create port chains
-                port_chains = {}
+                    port_chains = {}
-                for d, name in _port_chain_names(new_port["id"]).iteritems():
+                    for d, name in _port_chain_names(
-                    port_chains[d] = self.client.create_chain(tenant_id, name)
+                        new_port["id"]).iteritems():
+                        port_chains[d] = self.client.create_chain(tenant_id,
+                                                                  name)
 
-                self._initialize_port_chains(port_data, port_chains['inbound'],
+                    self._initialize_port_chains(port_data,
-                                             port_chains['outbound'], sg_ids)
+                                                 port_chains['inbound'],
+                                                 port_chains['outbound'],
+                                                 sg_ids)
 
-                # Update the port with the chain
+                    # Update the port with the chain
-                self.client.update_port_chains(
+                    self.client.update_port_chains(
-                    bridge_port, port_chains["inbound"].get_id(),
+                        bridge_port, port_chains["inbound"].get_id(),
-                    port_chains["outbound"].get_id())
+                        port_chains["outbound"].get_id())
 
-                if _is_dhcp_port(port_data):
-                    # For DHCP port, add a metadata route
-                    for cidr, ip in self._metadata_subnets(
-                        context, port_data["fixed_ips"]):
-                        self.client.add_dhcp_route_option(bridge, cidr, ip,
-                                                          METADATA_DEFAULT_IP)
-                elif _is_vif_port(port_data):
                     # DHCP mapping is only for VIF ports
                     for cidr, ip, mac in self._dhcp_mappings(
                             context, port_data["fixed_ips"],
                             port_data["mac_address"]):
                         self.client.add_dhcp_host(bridge, cidr, ip, mac)
 
+                elif _is_dhcp_port(port_data):
+                    # For DHCP port, add a metadata route
+                    for cidr, ip in self._metadata_subnets(
+                        context, port_data["fixed_ips"]):
+                        self.client.add_dhcp_route_option(bridge, cidr, ip,
+                                                          METADATA_DEFAULT_IP)
+
         except Exception as ex:
             # Try removing the MidoNet port before raising an exception.
             with excutils.save_and_reraise_exception():
@@ -629,8 +633,8 @@ class MidonetPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
                     self._check_update_has_security_groups(port)):
                 self._unbind_port_from_sgs(context, p["id"])
                 sg_ids = self._get_security_groups_on_port(context, port)
-                if sg_ids:
+                self._bind_port_to_sgs(context, p, sg_ids)
-                    self._bind_port_to_sgs(context, p, sg_ids)
+
         return p
 
     def create_router(self, context, router):
@@ -884,12 +888,12 @@ class MidonetPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
             # Not all VM images supports DHCP option 121.  Add a route for the
             # Metadata server in the router to forward the packet to the bridge
             # that will send them to the Metadata Proxy.
-            net_addr, net_len = net_util.net_addr(METADATA_DEFAULT_IP)
+            md_net_addr, md_net_len = net_util.net_addr(METADATA_DEFAULT_IP)
             self.client.add_router_route(
                 router, type='Normal', src_network_addr=net_addr,
                 src_network_length=net_len,
-                dst_network_addr=net_addr,
+                dst_network_addr=md_net_addr,
-                dst_network_length=32,
+                dst_network_length=md_net_len,
                 next_hop_port=router_port.get_id(),
                 next_hop_gateway=metadata_gw_ip)
 

neutron/tests/unit/midonet/test_midonet_driver.py

@@ -26,7 +26,6 @@ sys.modules["midonetclient"] = mock.Mock()
 from neutron.agent.common import config
 from neutron.agent.linux import interface
 from neutron.agent.linux import ip_lib
-from neutron.agent.linux import utils
 from neutron.openstack.common import uuidutils
 import neutron.plugins.midonet.agent.midonet_driver as driver
 from neutron.tests import base
@@ -43,10 +42,7 @@ class MidoInterfaceDriverTestCase(base.BaseTestCase):
         self.ip = self.ip_p.start()
         self.device_exists_p = mock.patch.object(ip_lib, 'device_exists')
         self.device_exists = self.device_exists_p.start()
-
+        self.device_exists.return_value = False
-        self.api_p = mock.patch.object(sys.modules["midonetclient"].api,
-                                       'MidonetApi')
-        self.api = self.api_p.start()
         self.addCleanup(mock.patch.stopall)
         midonet_opts = [
             cfg.StrOpt('midonet_uri',
@@ -64,6 +60,12 @@ class MidoInterfaceDriverTestCase(base.BaseTestCase):
         ]
         self.conf.register_opts(midonet_opts, "MIDONET")
         self.driver = driver.MidonetInterfaceDriver(self.conf)
+        self.root_dev = mock.Mock()
+        self.ns_dev = mock.Mock()
+        self.ip().add_veth = mock.Mock(return_value=(
+            self.root_dev, self.ns_dev))
+        self.driver._get_host_uuid = mock.Mock(
+            return_value=uuidutils.generate_uuid())
         self.network_id = uuidutils.generate_uuid()
         self.port_id = uuidutils.generate_uuid()
         self.device_name = "tap0"
@@ -73,20 +75,10 @@ class MidoInterfaceDriverTestCase(base.BaseTestCase):
         super(MidoInterfaceDriverTestCase, self).setUp()
 
     def test_plug(self):
-        def device_exists(dev, root_helper=None, namespace=None):
+        self.driver.plug(
-            return False
+            self.network_id, self.port_id,
-
+            self.device_name, self.mac_address,
-        self.device_exists.side_effect = device_exists
+            self.bridge, self.namespace)
-        root_dev = mock.Mock()
-        ns_dev = mock.Mock()
-        self.ip().add_veth = mock.Mock(return_value=(root_dev, ns_dev))
-        self.driver._get_host_uuid = mock.Mock(
-            return_value=uuidutils.generate_uuid())
-        with mock.patch.object(utils, 'execute'):
-            self.driver.plug(
-                self.network_id, self.port_id,
-                self.device_name, self.mac_address,
-                self.bridge, self.namespace)
 
         expected = [mock.call(), mock.call('sudo'),
                     mock.call().add_veth(self.device_name,
@@ -95,19 +87,15 @@ class MidoInterfaceDriverTestCase(base.BaseTestCase):
                     mock.call().ensure_namespace(self.namespace),
                     mock.call().ensure_namespace().add_device_to_namespace(
                         mock.ANY)]
-        ns_dev.assert_has_calls(
+        self.ns_dev.assert_has_calls(
             [mock.call.link.set_address(self.mac_address)])
 
-        root_dev.assert_has_calls([mock.call.link.set_up()])
+        self.root_dev.assert_has_calls([mock.call.link.set_up()])
-        ns_dev.assert_has_calls([mock.call.link.set_up()])
+        self.ns_dev.assert_has_calls([mock.call.link.set_up()])
         self.ip.assert_has_calls(expected, True)
-        host = mock.Mock()
-        self.api().get_host = mock.Mock(return_value=host)
-        self.api.assert_has_calls([mock.call().add_host_interface_port])
 
     def test_unplug(self):
-        with mock.patch.object(utils, 'execute'):
+        self.driver.unplug(self.device_name, self.bridge, self.namespace)
-            self.driver.unplug(self.device_name, self.bridge, self.namespace)
 
         self.ip_dev.assert_has_calls([
             mock.call(self.device_name, self.driver.root_helper,

neutron/tests/unit/midonet/test_midonet_plugin.py

@@ -27,6 +27,7 @@ import sys
 import neutron.common.test_lib as test_lib
 import neutron.tests.unit.midonet.mock_lib as mock_lib
 import neutron.tests.unit.test_db_plugin as test_plugin
+import neutron.tests.unit.test_extension_security_group as sg
 
 
 MIDOKURA_PKG_PATH = "neutron.plugins.midonet.plugin"
@@ -59,6 +60,30 @@ class MidonetPluginV2TestCase(test_plugin.NeutronDbPluginV2TestCase):
 
 class TestMidonetNetworksV2(test_plugin.TestNetworksV2,
                             MidonetPluginV2TestCase):
+
+    pass
+
+
+class TestMidonetSecurityGroupsTestCase(sg.SecurityGroupDBTestCase):
+
+    _plugin_name = ('%s.MidonetPluginV2' % MIDOKURA_PKG_PATH)
+
+    def setUp(self):
+        self.mock_api = mock.patch(
+            'neutron.plugins.midonet.midonet_lib.MidoClient')
+        etc_path = os.path.join(os.path.dirname(__file__), 'etc')
+        test_lib.test_config['config_files'] = [os.path.join(
+            etc_path, 'midonet.ini.test')]
+
+        self.instance = self.mock_api.start()
+        mock_cfg = mock_lib.MidonetLibMockConfig(self.instance.return_value)
+        mock_cfg.setup()
+        super(TestMidonetSecurityGroupsTestCase, self).setUp(self._plugin_name)
+
+
+class TestMidonetSecurityGroup(sg.TestSecurityGroups,
+                               TestMidonetSecurityGroupsTestCase):
+
     pass
 
 
@@ -120,4 +145,4 @@ class TestMidonetPortsV2(test_plugin.TestPortsV2,
     # tests that attempt to create them.
 
     def test_overlapping_subnets(self):
-            pass
+        pass