x/vmware-nsx
Code Issues Proposed changes

Improvements to API validation logic.

Browse Source 
Do not automatically map generic exceptions like AttributeError to
http errors (instead they should be handled closer to where they occur
so that they can be "intelligently" converted to the appropriate
error).

Fix up some expected error codes in the unit tests.
Improve some of the validation messages.
Remove all use of locals() in attributes.py

Fixes: bug #1076813
Change-Id: Iabf8808a840e927307bbcae4cd41790af3d79a9e
This commit is contained in:
Henry Gessau 2013-02-07 16:01:28 -05:00
parent c2886fce30
commit 7ba71c4887
6 changed files with 68 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
quantum/api/v2/attributes.py
View File

@ -33,13 +33,16 @@ SHARED = 'shared'
def _verify_dict_keys(expected_keys, target_dict): def _verify_dict_keys(expected_keys, target_dict):
if not isinstance(target_dict, dict): if not isinstance(target_dict, dict):
msg = _("Invalid input. %s must be a dictionary.") % target_dict msg = (_("Invalid input. '%(target_dict)s' must be a dictionary "
"with keys: %(expected_keys)s") %
dict(target_dict=target_dict, expected_keys=expected_keys))
return msg return msg
provided_keys = target_dict.keys() provided_keys = target_dict.keys()
if set(expected_keys) != set(provided_keys): if set(expected_keys) != set(provided_keys):
msg = (_("Expected keys not found. Expected: %(expected_keys)s " msg = (_("Expected keys not found. Expected: '%(expected_keys)s', "
"Provided: %(provided_keys)s") % locals()) "Provided: '%(provided_keys)s'") %
dict(expected_keys=expected_keys, provided_keys=provided_keys))
return msg return msg
@ -49,7 +52,8 @@ def is_attr_set(attribute):
def _validate_values(data, valid_values=None): def _validate_values(data, valid_values=None):
if data not in valid_values: if data not in valid_values:
msg = _("'%(data)s' is not in %(valid_values)s") % locals() msg = (_("'%(data)s' is not in %(valid_values)s") %
dict(data=data, valid_values=valid_values))
LOG.debug(msg) LOG.debug(msg)
return msg return msg
@ -61,7 +65,8 @@ def _validate_string(data, max_len=None):
return msg return msg
if max_len is not None and len(data) > max_len: if max_len is not None and len(data) > max_len:
msg = _("'%(data)s' exceeds maximum length of %(max_len)s") % locals() msg = (_("'%(data)s' exceeds maximum length of %(max_len)s") %
dict(data=data, max_len=max_len))
LOG.debug(msg) LOG.debug(msg)
return msg return msg
@ -71,7 +76,9 @@ def _validate_range(data, valid_values=None):
max_value = valid_values[1] max_value = valid_values[1]
if not min_value <= data <= max_value: if not min_value <= data <= max_value:
msg = _("'%(data)s' is not in range %(min_value)s through " msg = _("'%(data)s' is not in range %(min_value)s through "
"%(max_value)s") % locals() "%(max_value)s") % dict(data=data,
min_value=min_value,
max_value=max_value)
LOG.debug(msg) LOG.debug(msg)
return msg return msg
@ -101,7 +108,7 @@ def _validate_ip_pools(data, valid_values=None):
""" """
if not isinstance(data, list): if not isinstance(data, list):
msg = _("'%s' is not a valid IP pool") % data msg = _("Invalid data format for IP pool: '%s'") % data
LOG.debug(msg) LOG.debug(msg)
return msg return msg
@ -120,18 +127,22 @@ def _validate_ip_pools(data, valid_values=None):
def _validate_fixed_ips(data, valid_values=None): def _validate_fixed_ips(data, valid_values=None):
if not isinstance(data, list): if not isinstance(data, list):
msg = _("'%s' is not a valid fixed IP") % data msg = _("Invalid data format for fixed IP: '%s'") % data
LOG.debug(msg) LOG.debug(msg)
return msg return msg
ips = [] ips = []
for fixed_ip in data: for fixed_ip in data:
if not isinstance(fixed_ip, dict):
msg = _("Invalid data format for fixed IP: '%s'") % fixed_ip
LOG.debug(msg)
return msg
if 'ip_address' in fixed_ip: if 'ip_address' in fixed_ip:
# Ensure that duplicate entries are not set - just checking IP # Ensure that duplicate entries are not set - just checking IP
# suffices. Duplicate subnet_id's are legitimate. # suffices. Duplicate subnet_id's are legitimate.
fixed_ip_address = fixed_ip['ip_address'] fixed_ip_address = fixed_ip['ip_address']
if fixed_ip_address in ips: if fixed_ip_address in ips:
msg = _("Duplicate entry %s") % fixed_ip msg = _("Duplicate IP address '%s'") % fixed_ip_address
else: else:
msg = _validate_ip_address(fixed_ip_address) msg = _validate_ip_address(fixed_ip_address)
if msg: if msg:
@ -147,7 +158,7 @@ def _validate_fixed_ips(data, valid_values=None):
def _validate_nameservers(data, valid_values=None): def _validate_nameservers(data, valid_values=None):
if not hasattr(data, '__iter__'): if not hasattr(data, '__iter__'):
msg = _("'%s' is not a valid nameserver") % data msg = _("Invalid data format for nameserver: '%s'") % data
LOG.debug(msg) LOG.debug(msg)
return msg return msg
@ -162,7 +173,7 @@ def _validate_nameservers(data, valid_values=None):
LOG.debug(msg) LOG.debug(msg)
return msg return msg
if ip in ips: if ip in ips:
msg = _("Duplicate nameserver %s") % ip msg = _("Duplicate nameserver '%s'") % ip
LOG.debug(msg) LOG.debug(msg)
return msg return msg
ips.append(ip) ips.append(ip)
@ -170,7 +181,7 @@ def _validate_nameservers(data, valid_values=None):
def _validate_hostroutes(data, valid_values=None): def _validate_hostroutes(data, valid_values=None):
if not isinstance(data, list): if not isinstance(data, list):
msg = _("'%s' is not a valid hostroute") % data msg = _("Invalid data format for hostroute: '%s'") % data
LOG.debug(msg) LOG.debug(msg)
return msg return msg
@ -190,7 +201,7 @@ def _validate_hostroutes(data, valid_values=None):
LOG.debug(msg) LOG.debug(msg)
return msg return msg
if hostroute in hostroutes: if hostroute in hostroutes:
msg = _("Duplicate hostroute %s") % hostroute msg = _("Duplicate hostroute '%s'") % hostroute
LOG.debug(msg) LOG.debug(msg)
return msg return msg
hostroutes.append(hostroute) hostroutes.append(hostroute)
@ -252,7 +263,7 @@ def _validate_uuid_list(data, valid_values=None):
return msg return msg
if len(set(data)) != len(data): if len(set(data)) != len(data):
msg = _("Duplicate items in the list: %s") % ', '.join(data) msg = _("Duplicate items in the list: '%s'") % ', '.join(data)
LOG.debug(msg) LOG.debug(msg)
return msg return msg

8
quantum/api/v2/base.py
View File

@ -36,8 +36,6 @@ FAULT_MAP = {exceptions.NotFound: webob.exc.HTTPNotFound,
exceptions.ServiceUnavailable: webob.exc.HTTPServiceUnavailable, exceptions.ServiceUnavailable: webob.exc.HTTPServiceUnavailable,
exceptions.NotAuthorized: webob.exc.HTTPForbidden, exceptions.NotAuthorized: webob.exc.HTTPForbidden,
netaddr.AddrFormatError: webob.exc.HTTPBadRequest, netaddr.AddrFormatError: webob.exc.HTTPBadRequest,
AttributeError: webob.exc.HTTPBadRequest,
ValueError: webob.exc.HTTPBadRequest,
} }
@ -359,7 +357,11 @@ class Controller(object):
def update(self, request, id, body=None, **kwargs): def update(self, request, id, body=None, **kwargs):
"""Updates the specified entity's attributes""" """Updates the specified entity's attributes"""
parent_id = kwargs.get(self._parent_id_name) parent_id = kwargs.get(self._parent_id_name)
payload = body.copy() try:
payload = body.copy()
except AttributeError:
msg = _("Invalid format: %s") % request.body
raise exceptions.BadRequest(resource='body', msg=msg)
payload['id'] = id payload['id'] = id
notifier_api.notify(request.context, notifier_api.notify(request.context,
self._publisher_id, self._publisher_id,

3
quantum/api/v2/resource.py
View File

@ -80,8 +80,7 @@ def Resource(controller, faults=None, deserializers=None, serializers=None):
method = getattr(controller, action) method = getattr(controller, action)
result = method(request=request, **args) result = method(request=request, **args)
except (ValueError, AttributeError, except (exceptions.QuantumException,
exceptions.QuantumException,
netaddr.AddrFormatError) as e: netaddr.AddrFormatError) as e:
LOG.exception(_('%s failed'), action) LOG.exception(_('%s failed'), action)
body = serializer.serialize({'QuantumError': str(e)}) body = serializer.serialize({'QuantumError': str(e)})

12
quantum/tests/unit/cisco/test_network_plugin.py
View File

@ -92,7 +92,7 @@ class TestCiscoPortsV2(CiscoNetworkPluginV2TestCase,
'test', 'test',
True) True)
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'ports') self._validate_behavior_on_bulk_failure(res, 'ports', 500)
def test_create_ports_bulk_native_plugin_failure(self): def test_create_ports_bulk_native_plugin_failure(self):
if self._skip_native_bulk: if self._skip_native_bulk:
@ -112,7 +112,7 @@ class TestCiscoPortsV2(CiscoNetworkPluginV2TestCase,
res = self._create_port_bulk(self.fmt, 2, net['network']['id'], res = self._create_port_bulk(self.fmt, 2, net['network']['id'],
'test', True, context=ctx) 'test', True, context=ctx)
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'ports') self._validate_behavior_on_bulk_failure(res, 'ports', 500)
class TestCiscoNetworksV2(CiscoNetworkPluginV2TestCase, class TestCiscoNetworksV2(CiscoNetworkPluginV2TestCase,
@ -140,7 +140,7 @@ class TestCiscoNetworksV2(CiscoNetworkPluginV2TestCase,
res = self._create_network_bulk(self.fmt, 2, 'test', True) res = self._create_network_bulk(self.fmt, 2, 'test', True)
LOG.debug("response is %s" % res) LOG.debug("response is %s" % res)
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'networks') self._validate_behavior_on_bulk_failure(res, 'networks', 500)
def test_create_networks_bulk_native_plugin_failure(self): def test_create_networks_bulk_native_plugin_failure(self):
if self._skip_native_bulk: if self._skip_native_bulk:
@ -157,7 +157,7 @@ class TestCiscoNetworksV2(CiscoNetworkPluginV2TestCase,
patched_plugin.side_effect = side_effect patched_plugin.side_effect = side_effect
res = self._create_network_bulk(self.fmt, 2, 'test', True) res = self._create_network_bulk(self.fmt, 2, 'test', True)
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'networks') self._validate_behavior_on_bulk_failure(res, 'networks', 500)
class TestCiscoSubnetsV2(CiscoNetworkPluginV2TestCase, class TestCiscoSubnetsV2(CiscoNetworkPluginV2TestCase,
@ -189,7 +189,7 @@ class TestCiscoSubnetsV2(CiscoNetworkPluginV2TestCase,
net['network']['id'], net['network']['id'],
'test') 'test')
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'subnets') self._validate_behavior_on_bulk_failure(res, 'subnets', 500)
def test_create_subnets_bulk_native_plugin_failure(self): def test_create_subnets_bulk_native_plugin_failure(self):
if self._skip_native_bulk: if self._skip_native_bulk:
@ -209,7 +209,7 @@ class TestCiscoSubnetsV2(CiscoNetworkPluginV2TestCase,
'test') 'test')
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'subnets') self._validate_behavior_on_bulk_failure(res, 'subnets', 500)
class TestCiscoPortsV2XML(TestCiscoPortsV2): class TestCiscoPortsV2XML(TestCiscoPortsV2):

32
quantum/tests/unit/test_attributes.py
View File

@ -139,17 +139,32 @@ class TestAttributes(unittest2.TestCase):
self.assertIsNone(msg) self.assertIsNone(msg)
def test_validate_fixed_ips(self): def test_validate_fixed_ips(self):
fixed_ips = [[{'subnet_id': '00000000-ffff-ffff-ffff-000000000000', fixed_ips = [
{'data': [{'subnet_id': '00000000-ffff-ffff-ffff-000000000000',
'ip_address': '1111.1.1.1'}], 'ip_address': '1111.1.1.1'}],
[{'subnet_id': 'invalid'}], 'error_msg': "'1111.1.1.1' is not a valid IP address"},
None, {'data': [{'subnet_id': 'invalid',
[{'subnet_id': '00000000-0fff-ffff-ffff-000000000000', 'ip_address': '1.1.1.1'}],
'error_msg': "'invalid' is not a valid UUID"},
{'data': None,
'error_msg': "Invalid data format for fixed IP: 'None'"},
{'data': "1.1.1.1",
'error_msg': "Invalid data format for fixed IP: '1.1.1.1'"},
{'data': ['00000000-ffff-ffff-ffff-000000000000', '1.1.1.1'],
'error_msg': "Invalid data format for fixed IP: "
"'00000000-ffff-ffff-ffff-000000000000'"},
{'data': [['00000000-ffff-ffff-ffff-000000000000', '1.1.1.1']],
'error_msg': "Invalid data format for fixed IP: "
"'['00000000-ffff-ffff-ffff-000000000000', "
"'1.1.1.1']'"},
{'data': [{'subnet_id': '00000000-0fff-ffff-ffff-000000000000',
'ip_address': '1.1.1.1'}, 'ip_address': '1.1.1.1'},
{'subnet_id': '00000000-ffff-ffff-ffff-000000000000', {'subnet_id': '00000000-ffff-ffff-ffff-000000000000',
'ip_address': '1.1.1.1'}]] 'ip_address': '1.1.1.1'}],
'error_msg': "Duplicate IP address '1.1.1.1'"}]
for fixed in fixed_ips: for fixed in fixed_ips:
msg = attributes._validate_fixed_ips(fixed) msg = attributes._validate_fixed_ips(fixed['data'])
self.assertIsNotNone(msg) self.assertEqual(msg, fixed['error_msg'])
fixed_ips = [[{'subnet_id': '00000000-ffff-ffff-ffff-000000000000', fixed_ips = [[{'subnet_id': '00000000-ffff-ffff-ffff-000000000000',
'ip_address': '1.1.1.1'}], 'ip_address': '1.1.1.1'}],
@ -402,7 +417,8 @@ class TestAttributes(unittest2.TestCase):
'f3eeab00-8367-4524-b662-55e64d4cacb5', 'f3eeab00-8367-4524-b662-55e64d4cacb5',
'e5069610-744b-42a7-8bd8-ceac1a229cd4'] 'e5069610-744b-42a7-8bd8-ceac1a229cd4']
msg = attributes._validate_uuid_list(duplicate_uuids) msg = attributes._validate_uuid_list(duplicate_uuids)
error = "Duplicate items in the list: %s" % ', '.join(duplicate_uuids) error = ("Duplicate items in the list: "
"'%s'" % ', '.join(duplicate_uuids))
self.assertEquals(msg, error) self.assertEquals(msg, error)
# check valid uuid lists # check valid uuid lists

14
quantum/tests/unit/test_db_plugin.py
View File

@ -411,7 +411,7 @@ class QuantumDbPluginV2TestCase(testlib_api.WebTestCase):
def _do_side_effect(self, patched_plugin, orig, *args, **kwargs): def _do_side_effect(self, patched_plugin, orig, *args, **kwargs):
""" Invoked by test cases for injecting failures in plugin """ """ Invoked by test cases for injecting failures in plugin """
def second_call(*args, **kwargs): def second_call(*args, **kwargs):
raise AttributeError raise q_exc.QuantumException
patched_plugin.side_effect = second_call patched_plugin.side_effect = second_call
return orig(*args, **kwargs) return orig(*args, **kwargs)
@ -740,7 +740,7 @@ class TestPortsV2(QuantumDbPluginV2TestCase):
'test', 'test',
True) True)
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'ports') self._validate_behavior_on_bulk_failure(res, 'ports', 500)
def test_create_ports_bulk_native_plugin_failure(self): def test_create_ports_bulk_native_plugin_failure(self):
if self._skip_native_bulk: if self._skip_native_bulk:
@ -759,7 +759,7 @@ class TestPortsV2(QuantumDbPluginV2TestCase):
res = self._create_port_bulk(self.fmt, 2, net['network']['id'], res = self._create_port_bulk(self.fmt, 2, net['network']['id'],
'test', True, context=ctx) 'test', True, context=ctx)
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'ports') self._validate_behavior_on_bulk_failure(res, 'ports', 500)
def test_list_ports(self): def test_list_ports(self):
# for this test we need to enable overlapping ips # for this test we need to enable overlapping ips
@ -1731,7 +1731,7 @@ class TestNetworksV2(QuantumDbPluginV2TestCase):
patched_plugin.side_effect = side_effect patched_plugin.side_effect = side_effect
res = self._create_network_bulk(self.fmt, 2, 'test', True) res = self._create_network_bulk(self.fmt, 2, 'test', True)
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'networks') self._validate_behavior_on_bulk_failure(res, 'networks', 500)
def test_create_networks_bulk_native_plugin_failure(self): def test_create_networks_bulk_native_plugin_failure(self):
if self._skip_native_bulk: if self._skip_native_bulk:
@ -1747,7 +1747,7 @@ class TestNetworksV2(QuantumDbPluginV2TestCase):
patched_plugin.side_effect = side_effect patched_plugin.side_effect = side_effect
res = self._create_network_bulk(self.fmt, 2, 'test', True) res = self._create_network_bulk(self.fmt, 2, 'test', True)
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'networks') self._validate_behavior_on_bulk_failure(res, 'networks', 500)
def test_list_networks(self): def test_list_networks(self):
with contextlib.nested(self.network(), with contextlib.nested(self.network(),
@ -1976,7 +1976,7 @@ class TestSubnetsV2(QuantumDbPluginV2TestCase):
net['network']['id'], net['network']['id'],
'test') 'test')
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'subnets') self._validate_behavior_on_bulk_failure(res, 'subnets', 500)
def test_create_subnets_bulk_native_plugin_failure(self): def test_create_subnets_bulk_native_plugin_failure(self):
if self._skip_native_bulk: if self._skip_native_bulk:
@ -1995,7 +1995,7 @@ class TestSubnetsV2(QuantumDbPluginV2TestCase):
'test') 'test')
# We expect a 500 as we injected a fault in the plugin # We expect a 500 as we injected a fault in the plugin
self._validate_behavior_on_bulk_failure(res, 'subnets') self._validate_behavior_on_bulk_failure(res, 'subnets', 500)
def test_delete_subnet(self): def test_delete_subnet(self):
gateway_ip = '10.0.0.1' gateway_ip = '10.0.0.1'