Merge "network: make nova to handle port_security_enabled=False"

nova/network/neutronv2/api.py

@@ -69,6 +69,8 @@ soft_external_network_attach_authorize = extensions.soft_core_authorizer(
 _SESSION = None
 _ADMIN_AUTH = None
 
+DEFAULT_SECGROUP = 'default'
+
 
 def list_opts():
     opts = copy.deepcopy(_neutron_options)
@@ -442,6 +444,17 @@ class API(base_api.NetworkAPI):
 
         return ports, net_ids, ordered_networks, available_macs
 
+    def _clean_security_groups(self, security_groups):
+        """Cleans security groups requested from Nova API
+
+        Neutron already passes a 'default' security group when
+        creating ports so it's not necessary to specify it to the
+        request.
+        """
+        if DEFAULT_SECGROUP in security_groups:
+            security_groups.remove(DEFAULT_SECGROUP)
+        return security_groups
+
     def _process_security_groups(self, instance, neutron, security_groups):
         """Processes and validates requested security groups for allocation.
 
@@ -589,7 +602,8 @@ class API(base_api.NetworkAPI):
         #                available net which is permitted bug/1364344
         self._check_external_network_attach(context, nets)
 
-        security_groups = kwargs.get('security_groups', [])
+        security_groups = self._clean_security_groups(
+            kwargs.get('security_groups', []))
         security_group_ids = self._process_security_groups(
                                     instance, neutron, security_groups)
 
@@ -610,17 +624,20 @@ class API(base_api.NetworkAPI):
                 continue
 
             nets_in_requested_order.append(network)
-            # If security groups are requested on an instance then the
-            # network must has a subnet associated with it. Some plugins
-            # implement the port-security extension which requires
-            # 'port_security_enabled' to be True for security groups.
-            # That is why True is returned if 'port_security_enabled'
-            # is not found.
-            if (security_groups and not (
-                    network['subnets']
-                    and network.get('port_security_enabled', True))):
 
-                raise exception.SecurityGroupCannotBeApplied()
+            port_security_enabled = network.get('port_security_enabled', True)
+            if port_security_enabled:
+                if not network.get('subnets'):
+                    # Neutron can't apply security groups to a port
+                    # for a network without L3 assignements.
+                    raise exception.SecurityGroupCannotBeApplied()
+            else:
+                if security_group_ids:
+                    # We don't want to apply security groups on port
+                    # for a network defined with
+                    # 'port_security_enabled=False'.
+                    raise exception.SecurityGroupCannotBeApplied()
+
             zone = 'compute:%s' % instance.availability_zone
             port_req_body = {'port': {'device_id': instance.uuid,
                                       'device_owner': zone}}

nova/tests/unit/network/test_neutronv2.py

@@ -216,6 +216,7 @@ class TestNeutronv2Base(test.TestCase):
                            'tenant_id': 'my_tenantid'})
         self.nets3 = self.nets2 + [{'id': 'my_netid3',
                                     'name': 'my_netname3',
+                                    'subnets': ['mysubnid3'],
                                     'tenant_id': 'my_tenantid'}]
         self.nets4 = [{'id': 'his_netid4',
                       'name': 'his_netname4',
@@ -223,6 +224,7 @@ class TestNeutronv2Base(test.TestCase):
         # A network request with external networks
         self.nets5 = self.nets1 + [{'id': 'the-external-one',
                                     'name': 'out-of-this-world',
+                                    'subnets': ['mysubnid5'],
                                     'router:external': True,
                                     'tenant_id': 'should-be-an-admin'}]
         # A network request with a duplicate
@@ -241,7 +243,8 @@ class TestNeutronv2Base(test.TestCase):
         self.nets9 = []
         # A network that is both shared and external
         self.nets10 = [{'id': 'net_id', 'name': 'net_name',
-                        'router:external': True, 'shared': True}]
+                        'router:external': True, 'shared': True,
+                        'subnets': ['mysubnid10']}]
         # A network with non-blank dns_domain to test _update_port_dns_name
         self.nets11 = [{'id': 'my_netid1',
                       'name': 'my_netname1',
@@ -1098,6 +1101,16 @@ class TestNeutronv2(TestNeutronv2Base):
         self._allocate_for_instance(net_idx=3,
                                     requested_networks=requested_networks)
 
+    def test_allocate_for_instance_with_no_subnet_defined(self):
+        # net_id=4 does not specify subnet and does not set the option
+        # port_security_disabled to True, so Neutron will not been
+        # able to associate the default security group to the port
+        # requested to be created. We expect an exception to be
+        # raised.
+        self.assertRaises(exception.SecurityGroupCannotBeApplied,
+                          self._allocate_for_instance, net_idx=4,
+                          _break='post_list_networks')
+
     def test_allocate_for_instance_with_invalid_network_id(self):
         requested_networks = objects.NetworkRequestList(
             objects=[objects.NetworkRequest(network_id='invalid_id')])
@@ -3605,7 +3618,8 @@ class TestNeutronv2WithMock(test.TestCase):
             objects = [objects.NetworkRequest(port_id='fake-port1'),
                        objects.NetworkRequest(port_id='fake-port2'),
                        objects.NetworkRequest(port_id='fail-port')])
-        mock_avail_nets.return_value = [{'id': 'net-1'}]
+        mock_avail_nets.return_value = [{'id': 'net-1',
+                                         'subnets': ['subnet1']}]
 
         self.api.allocate_for_instance(mock.sentinel.ctx,
                                   mock_inst,
@@ -3722,7 +3736,8 @@ class TestNeutronv2WithMock(test.TestCase):
         mock_inst = mock.Mock(project_id="proj-1",
                               availability_zone='zone-1',
                               uuid='inst-1')
-        mock_avail_nets.return_value = [{'id': 'net-1'}]
+        mock_avail_nets.return_value = [{'id': 'net-1',
+                                         'subnets': ['subnet1']}]
         mock_nc.create_port.return_value = {'port': {'id': 'fake_id',
                             'tenant_id': mock_inst.project_id,
                             'binding:vif_type': 'binding_failed'}}
@@ -4272,3 +4287,233 @@ class TestNeutronClientForAdminScenarios(test.NoDBTestCase):
 
     def test_get_client_for_admin_context_with_id(self):
         self._test_get_client_for_admin(use_id=True, admin_context=True)
+
+
+class TestNeutronPortSecurity(test.NoDBTestCase):
+
+    @mock.patch.object(neutronapi.API, 'get_instance_nw_info')
+    @mock.patch.object(neutronapi.API, '_update_port_dns_name')
+    @mock.patch.object(neutronapi.API, '_create_port')
+    @mock.patch.object(neutronapi.API, '_populate_neutron_extension_values')
+    @mock.patch.object(neutronapi.API, '_check_external_network_attach')
+    @mock.patch.object(neutronapi.API, '_process_security_groups')
+    @mock.patch.object(neutronapi.API, '_get_available_networks')
+    @mock.patch.object(neutronapi.API, '_process_requested_networks')
+    @mock.patch.object(neutronapi.API, '_has_port_binding_extension')
+    @mock.patch.object(neutronapi, 'get_client')
+    def test_no_security_groups_requested(
+            self, mock_get_client, mock_has_port_binding_extension,
+            mock_process_requested_networks, mock_get_available_networks,
+            mock_process_security_groups, mock_check_external_network_attach,
+            mock_populate_neutron_extension_values, mock_create_port,
+            mock_update_port_dns_name, mock_get_instance_nw_info):
+        nets = [
+            {'id': 'net1',
+             'name': 'net_name1',
+             'subnets': ['mysubnid1'],
+             'port_security_enabled': True},
+            {'id': 'net2',
+             'name': 'net_name2',
+             'subnets': ['mysubnid2'],
+             'port_security_enabled': True}]
+        onets = objects.NetworkRequestList(objects=[
+            objects.NetworkRequest(network_id='net1'),
+            objects.NetworkRequest(network_id='net2')])
+
+        instance = objects.Instance(
+            project_id=1, availability_zone='nova', uuid='uuid1')
+        secgroups = ['default']  # Nova API provides the 'default'
+
+        mock_process_requested_networks.return_value = [
+            None, ['net1', 'net2'], onets, None]
+        mock_get_available_networks.return_value = nets
+        mock_process_security_groups.return_value = []
+
+        api = neutronapi.API()
+        api.allocate_for_instance(
+            'context', instance, requested_networks=onets,
+            security_groups=secgroups)
+
+        mock_process_security_groups.assert_called_once_with(
+            instance, mock.ANY, [])
+        mock_create_port.assert_has_calls([
+            mock.call(
+                mock.ANY, instance,
+                u'net1', {'port':
+                          {'device_owner': u'compute:nova',
+                           'device_id': 'uuid1'}},
+                None, [], None, None),
+            mock.call(
+                mock.ANY, instance,
+                u'net2', {'port':
+                          {'device_owner': u'compute:nova',
+                           'device_id': 'uuid1'}},
+                None, [], None, None)])
+
+    @mock.patch.object(neutronapi.API, 'get_instance_nw_info')
+    @mock.patch.object(neutronapi.API, '_update_port_dns_name')
+    @mock.patch.object(neutronapi.API, '_create_port')
+    @mock.patch.object(neutronapi.API, '_populate_neutron_extension_values')
+    @mock.patch.object(neutronapi.API, '_check_external_network_attach')
+    @mock.patch.object(neutronapi.API, '_process_security_groups')
+    @mock.patch.object(neutronapi.API, '_get_available_networks')
+    @mock.patch.object(neutronapi.API, '_process_requested_networks')
+    @mock.patch.object(neutronapi.API, '_has_port_binding_extension')
+    @mock.patch.object(neutronapi, 'get_client')
+    def test_security_groups_requested(
+            self, mock_get_client, mock_has_port_binding_extension,
+            mock_process_requested_networks, mock_get_available_networks,
+            mock_process_security_groups, mock_check_external_network_attach,
+            mock_populate_neutron_extension_values, mock_create_port,
+            mock_update_port_dns_name, mock_get_instance_nw_info):
+        nets = [
+            {'id': 'net1',
+             'name': 'net_name1',
+             'subnets': ['mysubnid1'],
+             'port_security_enabled': True},
+            {'id': 'net2',
+             'name': 'net_name2',
+             'subnets': ['mysubnid2'],
+             'port_security_enabled': True}]
+        onets = objects.NetworkRequestList(objects=[
+            objects.NetworkRequest(network_id='net1'),
+            objects.NetworkRequest(network_id='net2')])
+
+        instance = objects.Instance(
+            project_id=1, availability_zone='nova', uuid='uuid1')
+        secgroups = ['default', 'secgrp1', 'secgrp2']
+
+        mock_process_requested_networks.return_value = [
+            None, ['net1', 'net2'], onets, None]
+        mock_get_available_networks.return_value = nets
+        mock_process_security_groups.return_value = ['secgrp-uuid1',
+                                                     'secgrp-uuid2']
+
+        api = neutronapi.API()
+        api.allocate_for_instance(
+            'context', instance, requested_networks=onets,
+            security_groups=secgroups)
+
+        mock_process_security_groups.assert_called_once_with(
+            instance, mock.ANY, ['secgrp1', 'secgrp2'])
+        mock_create_port.assert_has_calls([
+            mock.call(
+                mock.ANY, instance,
+                u'net1', {'port':
+                          {'device_owner': u'compute:nova',
+                           'device_id': 'uuid1'}},
+                None, ['secgrp-uuid1', 'secgrp-uuid2'], None, None),
+            mock.call(
+                mock.ANY, instance,
+                u'net2', {'port':
+                          {'device_owner': u'compute:nova',
+                           'device_id': 'uuid1'}},
+                None, ['secgrp-uuid1', 'secgrp-uuid2'], None, None)])
+
+    @mock.patch.object(neutronapi.API, 'get_instance_nw_info')
+    @mock.patch.object(neutronapi.API, '_update_port_dns_name')
+    @mock.patch.object(neutronapi.API, '_create_port')
+    @mock.patch.object(neutronapi.API, '_populate_neutron_extension_values')
+    @mock.patch.object(neutronapi.API, '_check_external_network_attach')
+    @mock.patch.object(neutronapi.API, '_process_security_groups')
+    @mock.patch.object(neutronapi.API, '_get_available_networks')
+    @mock.patch.object(neutronapi.API, '_process_requested_networks')
+    @mock.patch.object(neutronapi.API, '_has_port_binding_extension')
+    @mock.patch.object(neutronapi, 'get_client')
+    def test_port_security_disabled_no_security_groups_requested(
+            self, mock_get_client, mock_has_port_binding_extension,
+            mock_process_requested_networks, mock_get_available_networks,
+            mock_process_security_groups, mock_check_external_network_attach,
+            mock_populate_neutron_extension_values, mock_create_port,
+            mock_update_port_dns_name, mock_get_instance_nw_info):
+        nets = [
+            {'id': 'net1',
+             'name': 'net_name1',
+             'subnets': ['mysubnid1'],
+             'port_security_enabled': False},
+            {'id': 'net2',
+             'name': 'net_name2',
+             'subnets': ['mysubnid2'],
+             'port_security_enabled': False}]
+        onets = objects.NetworkRequestList(objects=[
+            objects.NetworkRequest(network_id='net1'),
+            objects.NetworkRequest(network_id='net2')])
+
+        instance = objects.Instance(
+            project_id=1, availability_zone='nova', uuid='uuid1')
+        secgroups = ['default']  # Nova API provides the 'default'
+
+        mock_process_requested_networks.return_value = [
+            None, ['net1', 'net2'], onets, None]
+        mock_get_available_networks.return_value = nets
+        mock_process_security_groups.return_value = []
+
+        api = neutronapi.API()
+        api.allocate_for_instance(
+            'context', instance, requested_networks=onets,
+            security_groups=secgroups)
+
+        mock_process_security_groups.assert_called_once_with(
+            instance, mock.ANY, [])
+        mock_create_port.assert_has_calls([
+            mock.call(
+                mock.ANY, instance,
+                u'net1', {'port':
+                          {'device_owner': u'compute:nova',
+                           'device_id': 'uuid1'}},
+                None, [], None, None),
+            mock.call(
+                mock.ANY, instance,
+                u'net2', {'port':
+                          {'device_owner': u'compute:nova',
+                           'device_id': 'uuid1'}},
+                None, [], None, None)])
+
+    @mock.patch.object(neutronapi.API, 'get_instance_nw_info')
+    @mock.patch.object(neutronapi.API, '_update_port_dns_name')
+    @mock.patch.object(neutronapi.API, '_create_port')
+    @mock.patch.object(neutronapi.API, '_populate_neutron_extension_values')
+    @mock.patch.object(neutronapi.API, '_check_external_network_attach')
+    @mock.patch.object(neutronapi.API, '_process_security_groups')
+    @mock.patch.object(neutronapi.API, '_get_available_networks')
+    @mock.patch.object(neutronapi.API, '_process_requested_networks')
+    @mock.patch.object(neutronapi.API, '_has_port_binding_extension')
+    @mock.patch.object(neutronapi, 'get_client')
+    def test_port_security_disabled_and_security_groups_requested(
+            self, mock_get_client, mock_has_port_binding_extension,
+            mock_process_requested_networks, mock_get_available_networks,
+            mock_process_security_groups, mock_check_external_network_attach,
+            mock_populate_neutron_extension_values, mock_create_port,
+            mock_update_port_dns_name, mock_get_instance_nw_info):
+        nets = [
+            {'id': 'net1',
+             'name': 'net_name1',
+             'subnets': ['mysubnid1'],
+             'port_security_enabled': True},
+            {'id': 'net2',
+             'name': 'net_name2',
+             'subnets': ['mysubnid2'],
+             'port_security_enabled': False}]
+        onets = objects.NetworkRequestList(objects=[
+            objects.NetworkRequest(network_id='net1'),
+            objects.NetworkRequest(network_id='net2')])
+
+        instance = objects.Instance(
+            project_id=1, availability_zone='nova', uuid='uuid1')
+        secgroups = ['default', 'secgrp1', 'secgrp2']
+
+        mock_process_requested_networks.return_value = [
+            None, ['net1', 'net2'], onets, None]
+        mock_get_available_networks.return_value = nets
+        mock_process_security_groups.return_value = ['secgrp-uuid1',
+                                                     'secgrp-uuid2']
+
+        api = neutronapi.API()
+        self.assertRaises(
+            exception.SecurityGroupCannotBeApplied,
+            api.allocate_for_instance,
+            'context', instance, requested_networks=onets,
+            security_groups=secgroups)
+
+        mock_process_security_groups.assert_called_once_with(
+            instance, mock.ANY, ['secgrp1', 'secgrp2'])

releasenotes/notes/known-issue-on-api-1efca45440136f3e.yaml

@@ -0,0 +1,8 @@
+---
+issues:
+  - When using Neutron extension 'port_security' and booting an
+    instance on a network with 'port_security_enabled=False' the Nova
+    API response says there is a 'default' security group attached to
+    the instance which is incorrect. However when listing security groups
+    for the instance there are none listed, which is correct. The API
+    response will be fixed separately with a microversion.