From 2168ae5b6554fe288a73f9d430a07ec87cfe205f Mon Sep 17 00:00:00 2001 From: Salvatore Orlando Date: Thu, 31 Jan 2013 08:42:13 -0800 Subject: [PATCH] Avoid sending names longer than 40 character to NVP Bug 1130192 NVP limits for name lenghts are much shorter than Quantum's. The quantum name is truncated before dispatching the NVP API operation. Change-Id: Idee568756a0df991003f59d498846ac9ff7fe095 --- .../nicira/nicira_nvp_plugin/nicira_qos_db.py | 4 ++ .../nicira/nicira_nvp_plugin/nvplib.py | 32 ++++++++++------ .../tests/unit/nicira/fake_nvpapiclient.py | 30 ++++++++++++++- .../tests/unit/nicira/test_nicira_plugin.py | 37 ++++++++++++++++++- 4 files changed, 90 insertions(+), 13 deletions(-) diff --git a/quantum/plugins/nicira/nicira_nvp_plugin/nicira_qos_db.py b/quantum/plugins/nicira/nicira_nvp_plugin/nicira_qos_db.py index c79412446e..8f1127ec4d 100644 --- a/quantum/plugins/nicira/nicira_nvp_plugin/nicira_qos_db.py +++ b/quantum/plugins/nicira/nicira_nvp_plugin/nicira_qos_db.py @@ -25,6 +25,7 @@ from quantum.db import models_v2 from quantum.openstack.common import uuidutils from quantum.plugins.nicira.nicira_nvp_plugin.extensions import (nvp_qos as ext_qos) +from quantum.plugins.nicira.nicira_nvp_plugin import nvplib class QoSQueue(model_base.BASEV2, models_v2.HasId, models_v2.HasTenant): @@ -297,4 +298,7 @@ class NVPQoSDbMixin(ext_qos.QueuePluginBase): for api_name, nvp_name in params.iteritems() if attr.is_attr_set(queue.get(api_name)) ) + if 'display_name' in nvp_queue: + nvp_queue['display_name'] = nvplib._check_and_truncate_name( + nvp_queue['display_name']) return nvp_queue diff --git a/quantum/plugins/nicira/nicira_nvp_plugin/nvplib.py b/quantum/plugins/nicira/nicira_nvp_plugin/nvplib.py index 2b81ff1495..dd2854e330 100644 --- a/quantum/plugins/nicira/nicira_nvp_plugin/nvplib.py +++ b/quantum/plugins/nicira/nicira_nvp_plugin/nvplib.py @@ -54,6 +54,8 @@ LROUTERNAT_RESOURCE = "nat/lrouter" LQUEUE_RESOURCE = "lqueue" GWSERVICE_RESOURCE = "gateway-service" QUANTUM_VERSION = "2013.1" +# Other constants for NVP resource +MAX_DISPLAY_NAME_LEN = 40 # Constants for NAT rules MATCH_KEYS = ["destination_ip_addresses", "destination_port_max", "destination_port_min", "source_ip_addresses", @@ -142,6 +144,14 @@ def _build_uri_path(resource, return uri_path +def _check_and_truncate_name(display_name): + if display_name and len(display_name) > MAX_DISPLAY_NAME_LEN: + LOG.debug(_("Specified name:'%s' exceeds maximum length. " + "It will be truncated on NVP"), display_name) + return display_name[:MAX_DISPLAY_NAME_LEN] + return display_name + + def get_cluster_version(cluster): """Return major/minor version #""" # Get control-cluster nodes @@ -286,7 +296,7 @@ def create_lswitch(cluster, tenant_id, display_name, cluster.default_tz_uuid), "transport_type": (nvp_binding_type or DEF_TRANSPORT_TYPE)} - lswitch_obj = {"display_name": display_name, + lswitch_obj = {"display_name": _check_and_truncate_name(display_name), "transport_zones": [transport_zone_config], "tags": [{"tag": tenant_id, "scope": "os_tid"}]} if nvp_binding_type == 'bridge' and vlan_id: @@ -315,9 +325,7 @@ def create_lswitch(cluster, tenant_id, display_name, def update_lswitch(cluster, lswitch_id, display_name, tenant_id=None, **kwargs): uri = _build_uri_path(LSWITCH_RESOURCE, resource_id=lswitch_id) - # TODO(salvatore-orlando): Make sure this operation does not remove - # any other important tag set on the lswtich object - lswitch_obj = {"display_name": display_name, + lswitch_obj = {"display_name": _check_and_truncate_name(display_name), "tags": [{"tag": tenant_id, "scope": "os_tid"}]} if "tags" in kwargs: lswitch_obj["tags"].extend(kwargs["tags"]) @@ -354,7 +362,7 @@ def create_l2_gw_service(cluster, tenant_id, display_name, devices): "device_id": device['interface_name'], "type": "L2Gateway"} for device in devices] gwservice_obj = { - "display_name": display_name, + "display_name": _check_and_truncate_name(display_name), "tags": tags, "gateways": gateways, "type": "L2GatewayServiceConfig" @@ -382,6 +390,7 @@ def create_lrouter(cluster, tenant_id, display_name, nexthop): with the NVP controller """ tags = [{"tag": tenant_id, "scope": "os_tid"}] + display_name = _check_and_truncate_name(display_name) lrouter_obj = { "display_name": display_name, "tags": tags, @@ -493,7 +502,7 @@ def update_l2_gw_service(cluster, gateway_id, display_name): if not display_name: # Nothing to update return gwservice_obj - gwservice_obj["display_name"] = display_name + gwservice_obj["display_name"] = _check_and_truncate_name(display_name) try: return json.loads(do_single_request("PUT", _build_uri_path(GWSERVICE_RESOURCE, @@ -513,7 +522,8 @@ def update_lrouter(cluster, lrouter_id, display_name, nexthop): # Nothing to update return lrouter_obj # It seems that this is faster than the doing an if on display_name - lrouter_obj["display_name"] = display_name or lrouter_obj["display_name"] + lrouter_obj["display_name"] = (_check_and_truncate_name(display_name) or + lrouter_obj["display_name"]) if nexthop: nh_element = lrouter_obj["routing_config"].get( "default_route_next_hop") @@ -705,16 +715,14 @@ def update_port(cluster, lswitch_uuid, lport_uuid, quantum_port_id, tenant_id, display_name, device_id, admin_status_enabled, mac_address=None, fixed_ips=None, port_security_enabled=None, security_profiles=None, queue_id=None): - # device_id can be longer than 40 so we rehash it hashed_device_id = hashlib.sha1(device_id).hexdigest() lport_obj = dict( admin_status_enabled=admin_status_enabled, - display_name=display_name, + display_name=_check_and_truncate_name(display_name), tags=[dict(scope='os_tid', tag=tenant_id), dict(scope='q_port_id', tag=quantum_port_id), dict(scope='vm_id', tag=hashed_device_id)]) - _configure_extensions(lport_obj, mac_address, fixed_ips, port_security_enabled, security_profiles, queue_id) @@ -741,6 +749,7 @@ def create_lport(cluster, lswitch_uuid, tenant_id, quantum_port_id, """ Creates a logical port on the assigned logical switch """ # device_id can be longer than 40 so we rehash it hashed_device_id = hashlib.sha1(device_id).hexdigest() + display_name = _check_and_truncate_name(display_name) lport_obj = dict( admin_status_enabled=admin_status_enabled, display_name=display_name, @@ -1048,8 +1057,9 @@ def create_security_profile(cluster, tenant_id, security_profile): 'ip_prefix': '0.0.0.0/0'}], 'logical_port_ingress_rules': []} try: + display_name = _check_and_truncate_name(security_profile.get('name')) body = mk_body( - tags=tags, display_name=security_profile.get('name'), + tags=tags, display_name=display_name, logical_port_ingress_rules=dhcp['logical_port_ingress_rules'], logical_port_egress_rules=dhcp['logical_port_egress_rules']) rsp = do_request(HTTP_POST, path, body, cluster=cluster) diff --git a/quantum/tests/unit/nicira/fake_nvpapiclient.py b/quantum/tests/unit/nicira/fake_nvpapiclient.py index 772343c566..0ddae75853 100644 --- a/quantum/tests/unit/nicira/fake_nvpapiclient.py +++ b/quantum/tests/unit/nicira/fake_nvpapiclient.py @@ -23,6 +23,17 @@ from quantum.plugins.nicira.nicira_nvp_plugin import NvpApiClient LOG = logging.getLogger(__name__) +MAX_NAME_LEN = 40 + + +def _validate_name(name): + if name and len(name) > MAX_NAME_LEN: + raise Exception("Logical switch name exceeds %d characters", + MAX_NAME_LEN) + + +def _validate_resource(body): + _validate_name(body.get('display_name')) class FakeClient: @@ -104,6 +115,15 @@ class FakeClient: _fake_lqueue_dict = {} _fake_gatewayservice_dict = {} + _validators = { + LSWITCH_RESOURCE: _validate_resource, + LSWITCH_LPORT_RESOURCE: _validate_resource, + LROUTER_LPORT_RESOURCE: _validate_resource, + SECPROF_RESOURCE: _validate_resource, + LQUEUE_RESOURCE: _validate_resource, + GWSERVICE_RESOURCE: _validate_resource + } + def __init__(self, fake_files_path): self.fake_files_path = fake_files_path @@ -436,6 +456,10 @@ class FakeClient: with open("%s/%s" % (self.fake_files_path, response_file)) as f: response_template = f.read() add_resource = getattr(self, '_add_%s' % res_type) + body_json = json.loads(body) + val_func = self._validators.get(res_type) + if val_func: + val_func(body_json) args = [body] if len(uuids): args.append(uuids[0]) @@ -456,9 +480,13 @@ class FakeClient: is_attachment = True res_type = res_type[:res_type.index('attachment')] res_dict = getattr(self, '_fake_%s_dict' % res_type) + body_json = json.loads(body) + val_func = self._validators.get(res_type) + if val_func: + val_func(body_json) resource = res_dict[uuids[-1]] if not is_attachment: - resource.update(json.loads(body)) + resource.update(body_json) else: relations = resource.get("_relations", {}) body_2 = json.loads(body) diff --git a/quantum/tests/unit/nicira/test_nicira_plugin.py b/quantum/tests/unit/nicira/test_nicira_plugin.py index a5791a39f8..e003bf27b4 100644 --- a/quantum/tests/unit/nicira/test_nicira_plugin.py +++ b/quantum/tests/unit/nicira/test_nicira_plugin.py @@ -150,6 +150,12 @@ class TestNiciraPortsV2(test_plugin.TestPortsV2, NiciraPluginV2TestCase): self.assertEqual(res['port']['fixed_ips'], data['port']['fixed_ips']) + def test_create_port_name_exceeds_40_chars(self): + name = 'this_is_a_port_whose_name_is_longer_than_40_chars' + with self.port(name=name) as port: + # Assert the Quantum name is not truncated + self.assertEqual(name, port['port']['name']) + class TestNiciraNetworksV2(test_plugin.TestNetworksV2, NiciraPluginV2TestCase): @@ -228,6 +234,12 @@ class TestNiciraNetworksV2(test_plugin.TestNetworksV2, # tenant must see a single network self.assertEqual(len(res['networks']), 1) + def test_create_network_name_exceeds_40_chars(self): + name = 'this_is_a_network_whose_name_is_longer_than_40_chars' + with self.network(name=name) as net: + # Assert Quantum name is not truncated + self.assertEqual(net['network']['name'], name) + class NiciraPortSecurityTestCase(psec.PortSecurityDBTestCase): @@ -285,7 +297,12 @@ class NiciraSecurityGroupsTestCase(ext_sg.SecurityGroupDBTestCase): class TestNiciraSecurityGroup(ext_sg.TestSecurityGroups, NiciraSecurityGroupsTestCase): - pass + + def test_create_security_group_name_exceeds_40_chars(self): + name = 'this_is_a_secgroup_whose_name_is_longer_than_40_chars' + with self.security_group(name=name) as sg: + # Assert Quantum name is not truncated + self.assertEqual(sg['security_group']['name'], name) class TestNiciraL3NatTestCase(test_l3_plugin.L3NatDBTestCase, @@ -411,6 +428,12 @@ class TestNiciraL3NatTestCase(test_l3_plugin.L3NatDBTestCase, def _nvp_metadata_teardown(self): cfg.CONF.set_override('enable_metadata_access_network', False, 'NVP') + def test_create_router_name_exceeds_40_chars(self): + name = 'this_is_a_router_whose_name_is_longer_than_40_chars' + with self.router(name=name) as rtr: + # Assert Quantum name is not truncated + self.assertEqual(rtr['router']['name'], name) + def test_router_add_interface_subnet_with_metadata_access(self): self._nvp_metadata_setup() self.test_router_add_interface_subnet() @@ -571,6 +594,12 @@ class TestNiciraQoSQueue(NiciraPluginV2TestCase): self.assertEqual(q['qos_queue']['qos_marking'], 'untrusted') self.assertFalse(q['qos_queue']['default']) + def test_create_qos_queue_name_exceeds_40_chars(self): + name = 'this_is_a_queue_whose_name_is_longer_than_40_chars' + with self.qos_queue(name=name) as queue: + # Assert Quantum name is not truncated + self.assertEqual(queue['qos_queue']['name'], name) + def test_create_qos_queue_default(self): with self.qos_queue(default=True) as q: self.assertTrue(q['qos_queue']['default']) @@ -842,6 +871,12 @@ class TestNiciraNetworkGateway(test_l2_gw.NetworkGatewayDbTestCase, cfg.CONF.set_override('api_extensions_path', ext_path) super(TestNiciraNetworkGateway, self).setUp() + def test_create_network_gateway_name_exceeds_40_chars(self): + name = 'this_is_a_gateway_whose_name_is_longer_than_40_chars' + with self._network_gateway(name=name) as nw_gw: + # Assert Quantum name is not truncated + self.assertEqual(nw_gw[self.resource]['name'], name) + def test_list_network_gateways(self): with self._network_gateway(name='test-gw-1') as gw1: with self._network_gateway(name='test_gw_2') as gw2: