From 58abb3d9cc01a7c4888d0bd0c1fb96c78154c714 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Tue, 30 Aug 2016 16:18:33 +0200 Subject: [PATCH] Check MTU sanity of trunk port subports The MTU of VLAN subinterfaces must not be greater than MTU of the trunk port. This commit adds a check that verifies that this is indeed the case. This limitation comes from the restriction that Linux kernel imposes on MTUs of untagged interfaces and tagged subinterfaces of these interfaces. For details of the current state, see https://bugzilla.kernel.org/show_bug.cgi?id=13467, namely comments 7 and 8. Once this limitation is overcome, it would be possible to remove this check. Partially-Implements: blueprint vlan-aware-vms Change-Id: Ic11e9a2cde4336e8e1533c56b63ff7aaf2b7a510 --- neutron/services/trunk/exceptions.py | 5 ++ neutron/services/trunk/plugin.py | 18 +++-- neutron/services/trunk/rules.py | 44 ++++++++++- .../tests/unit/services/trunk/test_rules.py | 76 +++++++++++++++++++ 4 files changed, 133 insertions(+), 10 deletions(-) diff --git a/neutron/services/trunk/exceptions.py b/neutron/services/trunk/exceptions.py index ddeb6448930..b71adf29ae9 100644 --- a/neutron/services/trunk/exceptions.py +++ b/neutron/services/trunk/exceptions.py @@ -41,6 +41,11 @@ class ParentPortInUse(n_exc.InUse): "eligible for use as a parent port.") +class SubPortMtuGreaterThanTrunkPortMtu(n_exc.Conflict): + message = _("MTU %(port_mtu)s of subport %(port_id)s cannot be greater " + "than MTU %(trunk_mtu)s of trunk %(trunk_id)s.") + + class PortInUseAsTrunkParent(n_exc.InUse): message = _("Port %(port_id)s is currently a parent port " "for trunk %(trunk_id)s.") diff --git a/neutron/services/trunk/plugin.py b/neutron/services/trunk/plugin.py index 904fe9b8bea..976e84a9ab1 100644 --- a/neutron/services/trunk/plugin.py +++ b/neutron/services/trunk/plugin.py @@ -238,16 +238,18 @@ class TrunkPlugin(service_base.ServicePluginBase, @db_base_plugin_common.convert_result_to_dict def add_subports(self, context, trunk_id, subports): """Add one or more subports to trunk.""" - # Check for basic validation since the request body here is not - # automatically validated by the API layer. - subports = subports['sub_ports'] - subports_validator = rules.SubPortsValidator( - self._segmentation_types, subports) - subports = subports_validator.validate(context, basic_validation=True) - added_subports = [] - with db_api.autonested_transaction(context.session): trunk = self._get_trunk(context, trunk_id) + + # Check for basic validation since the request body here is not + # automatically validated by the API layer. + subports = subports['sub_ports'] + subports_validator = rules.SubPortsValidator( + self._segmentation_types, subports, trunk['port_id']) + subports = subports_validator.validate( + context, basic_validation=True) + added_subports = [] + rules.trunk_can_be_managed(context, trunk) original_trunk = copy.deepcopy(trunk) # NOTE(status_police): the trunk status should transition to diff --git a/neutron/services/trunk/rules.py b/neutron/services/trunk/rules.py index b7ad260a5cd..5f3191d3fc6 100644 --- a/neutron/services/trunk/rules.py +++ b/neutron/services/trunk/rules.py @@ -17,9 +17,11 @@ from neutron_lib.api import validators from neutron_lib import exceptions as n_exc from neutron._i18n import _ +from neutron.common import utils as n_utils from neutron.extensions import portbindings from neutron import manager from neutron.objects import trunk as trunk_objects +from neutron.plugins.ml2 import driver_api as api from neutron.services.trunk import exceptions as trunk_exc from neutron.services.trunk import utils @@ -131,18 +133,56 @@ class SubPortsValidator(object): msg = validators.validate_subports(self.subports) if msg: raise n_exc.InvalidInput(error_message=msg) + trunk_port_mtu = self._get_port_mtu(context, self.trunk_port_id) if trunk_validation: - return [self._validate(context, s) for s in self.subports] + return [self._validate(context, s, trunk_port_mtu) + for s in self.subports] else: return self.subports - def _validate(self, context, subport): + def _get_port_mtu(self, context, port_id): + """ + Return MTU for the network where the given port belongs to. + If the network or port cannot be obtained, or if MTU is not defined, + returns None. + """ + core_plugin = manager.NeutronManager.get_plugin() + + if not n_utils.is_extension_supported(core_plugin, 'net-mtu'): + return + + try: + port = core_plugin.get_port(context, port_id) + net = core_plugin.get_network(context, port['network_id']) + except (n_exc.PortNotFound, n_exc.NetworkNotFound): + # A concurrent request might have made the port or network + # disappear; though during DB insertion, the subport request + # will fail on integrity constraint, it is safer to return + # a None MTU here. + return + + return net[api.MTU] + + def _validate(self, context, subport, trunk_port_mtu): # Check that the subport doesn't reference the same port_id as a # trunk we may be in the middle of trying to create, in other words # make the validation idiot proof. if subport['port_id'] == self.trunk_port_id: raise trunk_exc.ParentPortInUse(port_id=subport['port_id']) + # Check MTU sanity - subport MTU must not exceed trunk MTU. + # If for whatever reason trunk_port_mtu is not available, + # the MTU sanity check cannot be enforced. + if trunk_port_mtu: + port_mtu = self._get_port_mtu(context, subport['port_id']) + if port_mtu and port_mtu > trunk_port_mtu: + raise trunk_exc.SubPortMtuGreaterThanTrunkPortMtu( + port_id=subport['port_id'], + port_mtu=port_mtu, + trunk_id=self.trunk_port_id, + trunk_mtu=trunk_port_mtu + ) + # If the segmentation details are missing, we will need to # figure out defaults when the time comes to support Ironic. # We can reasonably expect segmentation details to be provided diff --git a/neutron/tests/unit/services/trunk/test_rules.py b/neutron/tests/unit/services/trunk/test_rules.py index 78698cae79a..0787173827b 100644 --- a/neutron/tests/unit/services/trunk/test_rules.py +++ b/neutron/tests/unit/services/trunk/test_rules.py @@ -22,6 +22,7 @@ from oslo_utils import uuidutils from neutron import manager from neutron.plugins.common import utils +from neutron.plugins.ml2 import driver_api as api from neutron.services.trunk import constants from neutron.services.trunk import drivers from neutron.services.trunk import exceptions as trunk_exc @@ -40,6 +41,9 @@ class SubPortsValidatorTestCase(base.BaseTestCase): self.segmentation_types = {constants.VLAN: utils.is_valid_vlan_tag} self.context = mock.ANY + mock.patch.object(rules.SubPortsValidator, '_get_port_mtu', + return_value=None).start() + def test_validate_subport_subport_and_trunk_shared_port_id(self): shared_id = uuidutils.generate_uuid() validator = rules.SubPortsValidator( @@ -119,6 +123,78 @@ class SubPortsValidatorTestCase(base.BaseTestCase): self.context, basic_validation=True) +class SubPortsValidatorMtuSanityTestCase(test_plugin.Ml2PluginV2TestCase): + + def setUp(self): + super(SubPortsValidatorMtuSanityTestCase, self).setUp() + self.segmentation_types = {constants.VLAN: utils.is_valid_vlan_tag} + + def test_validate_subport_mtu_same_as_trunk(self): + self._test_validate_subport_trunk_mtu(1500, 1500) + + def test_validate_subport_mtu_smaller_than_trunks(self): + self._test_validate_subport_trunk_mtu(500, 1500) + + def test_validate_subport_mtu_greater_than_trunks(self): + self._test_validate_subport_trunk_mtu(1500, 500) + + def test_validate_subport_mtu_unset_trunks_set(self): + self._test_validate_subport_trunk_mtu(None, 500) + + def test_validate_subport_mtu_set_trunks_unset(self): + self._test_validate_subport_trunk_mtu(500, None) + + def test_validate_subport_mtu_set_trunks_net_exception(self): + self._test_validate_subport_trunk_mtu(1500, 'exc') + + def test_validate_subport_mtu_net_exception_trunks_set(self): + self._test_validate_subport_trunk_mtu('exc', 1500) + + def _test_validate_subport_trunk_mtu( + self, subport_net_mtu, trunk_net_mtu): + plugin = manager.NeutronManager.get_plugin() + orig_get_network = plugin.get_network + + def get_network_adjust_mtu(*args, **kwargs): + res = orig_get_network(*args, **kwargs) + if res['name'] == 'net_trunk': + if trunk_net_mtu == 'exc': + raise n_exc.NetworkNotFound(net_id='net-id') + res[api.MTU] = trunk_net_mtu + elif res['name'] == 'net_subport': + if subport_net_mtu == 'exc': + raise n_exc.NetworkNotFound(net_id='net-id') + res[api.MTU] = subport_net_mtu + return res + + with self.network('net_trunk') as trunk_net,\ + self.subnet(network=trunk_net) as trunk_subnet,\ + self.port(subnet=trunk_subnet) as trunk_port,\ + self.network('net_subport') as subport_net,\ + self.subnet(network=subport_net) as subport_subnet,\ + self.port(subnet=subport_subnet) as subport,\ + mock.patch.object(plugin, "get_network", + side_effect=get_network_adjust_mtu): + trunk = {'port_id': trunk_port['port']['id'], + 'tenant_id': 'test_tenant', + 'sub_ports': [{'port_id': subport['port']['id'], + 'segmentation_type': 'vlan', + 'segmentation_id': 2}]} + + validator = rules.SubPortsValidator( + self.segmentation_types, trunk['sub_ports'], trunk['port_id']) + + if subport_net_mtu is None or trunk_net_mtu is None: + validator.validate(self.context) + elif subport_net_mtu == 'exc' or trunk_net_mtu == 'exc': + validator.validate(self.context) + elif subport_net_mtu <= trunk_net_mtu: + validator.validate(self.context) + else: + self.assertRaises(trunk_exc.SubPortMtuGreaterThanTrunkPortMtu, + validator.validate, self.context) + + class TrunkPortValidatorTestCase(test_plugin.Ml2PluginV2TestCase): def setUp(self):