diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index 4f2bd8a5c3e..6ccca4ebd41 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -69,6 +69,8 @@ _SESSION = None _ADMIN_AUTH = None +DEFAULT_SECGROUP = 'default' + def list_opts(): opts = copy.deepcopy(_neutron_options) @@ -442,6 +444,17 @@ def _process_requested_networks(self, context, instance, neutron, 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 @@ def allocate_for_instance(self, context, instance, **kwargs): # 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 @@ def allocate_for_instance(self, context, instance, **kwargs): 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}} diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py index e1628739642..f703d036723 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -216,6 +216,7 @@ def setUp(self): '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 @@ def setUp(self): # 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 @@ def setUp(self): 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 @@ def test_allocate_for_instance_with_requested_networks(self): 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 @@ def show_port(port_id): 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 @@ def test_port_binding_failed_created_port(self, mock_ntrn, 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 @@ def test_get_client_for_admin_context(self): 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']) diff --git a/releasenotes/notes/known-issue-on-api-1efca45440136f3e.yaml b/releasenotes/notes/known-issue-on-api-1efca45440136f3e.yaml new file mode 100644 index 00000000000..ae8c3665162 --- /dev/null +++ b/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. \ No newline at end of file