From: Paul Michali Date: Wed, 12 Mar 2014 14:04:30 +0000 (+0000) Subject: Cisco VPN device driver post-merge cleanup X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=7f096512bbccee92da24e8f408f1d7f4f100cc95;p=openstack-build%2Fneutron-build.git Cisco VPN device driver post-merge cleanup During review for I-3 there were some minor comments regarding log level on message and asserts for mock calls. In addition, some debug logging enhancements were made between the service and device driver to better indicate the source of the shared "vpnservice_updated" RPC. Lastly, unit tests were updated, based on a newer Cisco CSR image, which had REST API fixes and behavior changes. Change-Id: I22277462270df0b2cd642ec576e8652c9df146b5 Closes-bug: 1288387 --- diff --git a/neutron/services/vpn/device_drivers/cisco_csr_rest_client.py b/neutron/services/vpn/device_drivers/cisco_csr_rest_client.py index 3cc3c435f..2e5599283 100644 --- a/neutron/services/vpn/device_drivers/cisco_csr_rest_client.py +++ b/neutron/services/vpn/device_drivers/cisco_csr_rest_client.py @@ -102,10 +102,9 @@ class CsrRestClient(object): if isinstance(te, r_exc.SSLError) else '', 'host': self.host}) self.status = requests.codes.REQUEST_TIMEOUT - except r_exc.ConnectionError as ce: - LOG.error(_("%(method)s: Unable to connect to CSR(%(host)s): " - "%(error)s"), - {'method': method, 'host': self.host, 'error': ce}) + except r_exc.ConnectionError: + LOG.exception(_("%(method)s: Unable to connect to CSR(%(host)s)"), + {'method': method, 'host': self.host}) self.status = requests.codes.NOT_FOUND except Exception as e: LOG.error(_("%(method)s: Unexpected error for CSR (%(host)s): " diff --git a/neutron/services/vpn/device_drivers/cisco_ipsec.py b/neutron/services/vpn/device_drivers/cisco_ipsec.py index c518491d3..5bbe5628b 100644 --- a/neutron/services/vpn/device_drivers/cisco_ipsec.py +++ b/neutron/services/vpn/device_drivers/cisco_ipsec.py @@ -16,7 +16,7 @@ import abc from collections import namedtuple -import httplib +import requests import netaddr from oslo.config import cfg @@ -221,7 +221,8 @@ class CiscoCsrIPsecDriver(device_drivers.DeviceDriver): def vpnservice_updated(self, context, **kwargs): """Handle VPNaaS service driver change notifications.""" - LOG.debug(_("Handling VPN service update notification")) + LOG.debug(_("Handling VPN service update notification '%s'"), + kwargs.get('reason', '')) self.sync(context, []) def create_vpn_service(self, service_data): @@ -675,7 +676,7 @@ class CiscoCsrIPSecConnection(object): def _check_create(self, resource, which): """Determine if REST create request was successful.""" - if self.csr.status == httplib.CREATED: + if self.csr.status == requests.codes.CREATED: LOG.debug("%(resource)s %(which)s is configured", {'resource': resource, 'which': which}) return @@ -701,7 +702,7 @@ class CiscoCsrIPSecConnection(object): def _verify_deleted(self, status, resource, which): """Determine if REST delete request was successful.""" - if status in (httplib.NO_CONTENT, httplib.NOT_FOUND): + if status in (requests.codes.NO_CONTENT, requests.codes.NOT_FOUND): LOG.debug("%(resource)s configuration %(which)s was removed", {'resource': resource, 'which': which}) else: diff --git a/neutron/services/vpn/service_drivers/__init__.py b/neutron/services/vpn/service_drivers/__init__.py index c932de716..9e7484ed5 100644 --- a/neutron/services/vpn/service_drivers/__init__.py +++ b/neutron/services/vpn/service_drivers/__init__.py @@ -76,7 +76,7 @@ class BaseIPsecVpnAgentApi(proxy.RpcProxy): active=True) for l3_agent in l3_agents: LOG.debug(_('Notify agent at %(topic)s.%(host)s the message ' - '%(method)s'), + '%(method)s %(args)s'), {'topic': self.to_agent_topic, 'host': l3_agent.host, 'method': method, @@ -86,6 +86,7 @@ class BaseIPsecVpnAgentApi(proxy.RpcProxy): version=version, topic='%s.%s' % (self.to_agent_topic, l3_agent.host)) - def vpnservice_updated(self, context, router_id): + def vpnservice_updated(self, context, router_id, **kwargs): """Send update event of vpnservices.""" - self._agent_notification(context, 'vpnservice_updated', router_id) + self._agent_notification(context, 'vpnservice_updated', router_id, + **kwargs) diff --git a/neutron/services/vpn/service_drivers/cisco_ipsec.py b/neutron/services/vpn/service_drivers/cisco_ipsec.py index 625fea936..4afd71b64 100644 --- a/neutron/services/vpn/service_drivers/cisco_ipsec.py +++ b/neutron/services/vpn/service_drivers/cisco_ipsec.py @@ -179,7 +179,8 @@ class CiscoCsrIPsecVPNDriver(service_drivers.VpnDriver): self.service_plugin.update_ipsec_site_conn_status( context, ipsec_site_connection['id'], constants.ERROR) csr_id_map.create_tunnel_mapping(context, ipsec_site_connection) - self.agent_rpc.vpnservice_updated(context, vpnservice['router_id']) + self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'], + reason='ipsec-conn-create') def update_ipsec_site_connection( self, context, old_ipsec_site_connection, ipsec_site_connection): @@ -190,7 +191,8 @@ class CiscoCsrIPsecVPNDriver(service_drivers.VpnDriver): def delete_ipsec_site_connection(self, context, ipsec_site_connection): vpnservice = self.service_plugin._get_vpnservice( context, ipsec_site_connection['vpnservice_id']) - self.agent_rpc.vpnservice_updated(context, vpnservice['router_id']) + self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'], + reason='ipsec-conn-delete') def create_ikepolicy(self, context, ikepolicy): pass @@ -214,10 +216,12 @@ class CiscoCsrIPsecVPNDriver(service_drivers.VpnDriver): pass def update_vpnservice(self, context, old_vpnservice, vpnservice): - self.agent_rpc.vpnservice_updated(context, vpnservice['router_id']) + self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'], + reason='vpn-service-update') def delete_vpnservice(self, context, vpnservice): - self.agent_rpc.vpnservice_updated(context, vpnservice['router_id']) + self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'], + reason='vpn-service-delete') def get_cisco_connection_mappings(self, conn_id, context): """Obtain persisted mappings for IDs related to connection.""" diff --git a/neutron/tests/unit/services/vpn/device_drivers/cisco_csr_mock.py b/neutron/tests/unit/services/vpn/device_drivers/cisco_csr_mock.py index 383030303..84f5d4ca5 100644 --- a/neutron/tests/unit/services/vpn/device_drivers/cisco_csr_mock.py +++ b/neutron/tests/unit/services/vpn/device_drivers/cisco_csr_mock.py @@ -29,7 +29,6 @@ from neutron.openstack.common import log as logging from neutron.tests.unit.services.vpn.device_drivers import httmock # TODO(pcm) Remove, once verified these have been fixed -FIXED_CSCum50512 = False FIXED_CSCum35484 = False FIXED_CSCul82396 = False FIXED_CSCum10324 = False @@ -236,7 +235,7 @@ def normal_get(url, request): u'outgoing-interface': u'GigabitEthernet1', u'admin-distance': 1} return httmock.response(requests.codes.OK, content=content) - if 'vpn-svc/site-to-site/active/sessions': + if 'vpn-svc/site-to-site/active/sessions' in url.path: # Only including needed fields for mock content = {u'kind': u'collection#vpn-active-sessions', u'items': [{u'status': u'DOWN-NEGOTIATING', @@ -318,26 +317,23 @@ def get_defaults(url, request): def get_unnumbered(url, request): if not request.headers.get('X-auth-token', None): return {'status_code': requests.codes.UNAUTHORIZED} - if FIXED_CSCum50512: - tunnel = url.path.split('/')[-1] - ipsec_policy_id = tunnel[6:] - content = {u'kind': u'object#vpn-site-to-site', - u'vpn-interface-name': u'%s' % tunnel, - u'ip-version': u'ipv4', - u'vpn-type': u'site-to-site', - u'ipsec-policy-id': u'%s' % ipsec_policy_id, - u'ike-profile-id': None, - u'mtu': 1500, - u'local-device': { - u'ip-address': u'unnumbered GigabitEthernet3', - u'tunnel-ip-address': u'10.10.10.10' - }, - u'remote-device': { - u'tunnel-ip-address': u'10.10.10.20' - }} - return httmock.response(requests.codes.OK, content=content) - else: - return httmock.response(requests.codes.INTERNAL_SERVER_ERROR) + tunnel = url.path.split('/')[-1] + ipsec_policy_id = tunnel[6:] + content = {u'kind': u'object#vpn-site-to-site', + u'vpn-interface-name': u'%s' % tunnel, + u'ip-version': u'ipv4', + u'vpn-type': u'site-to-site', + u'ipsec-policy-id': u'%s' % ipsec_policy_id, + u'ike-profile-id': None, + u'mtu': 1500, + u'local-device': { + u'ip-address': u'GigabitEthernet3', + u'tunnel-ip-address': u'10.10.10.10' + }, + u'remote-device': { + u'tunnel-ip-address': u'10.10.10.20' + }} + return httmock.response(requests.codes.OK, content=content) @filter_request(['get'], 'vpn-svc/site-to-site') diff --git a/neutron/tests/unit/services/vpn/device_drivers/notest_cisco_csr_rest.py b/neutron/tests/unit/services/vpn/device_drivers/notest_cisco_csr_rest.py index 920b9e403..776d4da46 100644 --- a/neutron/tests/unit/services/vpn/device_drivers/notest_cisco_csr_rest.py +++ b/neutron/tests/unit/services/vpn/device_drivers/notest_cisco_csr_rest.py @@ -919,17 +919,13 @@ class TestCsrRestIPSecConnectionCreate(base.BaseTestCase): location) # Check the hard-coded items that get set as well... content = self.csr.get_request(location, full_url=True) - if csr_request.FIXED_CSCum50512: - self.assertEqual(requests.codes.OK, self.csr.status) - expected_connection = {u'kind': u'object#vpn-site-to-site', - u'ip-version': u'ipv4'} - expected_connection.update(connection_info) - expected_connection[u'local-device'][u'ip-address'] = ( - u'unnumbered GigabitEthernet3') - self.assertEqual(expected_connection, content) - else: - self.assertEqual(requests.codes.INTERNAL_SERVER_ERROR, - self.csr.status) + self.assertEqual(requests.codes.OK, self.csr.status) + expected_connection = {u'kind': u'object#vpn-site-to-site', + u'ike-profile-id': None, + u'mtu': 1500, + u'ip-version': u'ipv4'} + expected_connection.update(connection_info) + self.assertEqual(expected_connection, content) def test_create_ipsec_connection_no_pre_shared_key(self): """Test of connection create without associated pre-shared key. @@ -1036,6 +1032,9 @@ class TestCsrRestIPSecConnectionCreate(base.BaseTestCase): u'remote-device': {u'tunnel-ip-address': u'10.10.10.20'} } self.csr.create_ipsec_connection(connection_info) + self.addCleanup(self._remove_resource_for_test, + self.csr.delete_ipsec_connection, + 'Tunnel%d' % tunnel_id) self.assertEqual(requests.codes.BAD_REQUEST, self.csr.status) def test_create_ipsec_connection_with_max_mtu(self): @@ -1080,6 +1079,9 @@ class TestCsrRestIPSecConnectionCreate(base.BaseTestCase): u'remote-device': {u'tunnel-ip-address': u'10.10.10.20'} } self.csr.create_ipsec_connection(connection_info) + self.addCleanup(self._remove_resource_for_test, + self.csr.delete_ipsec_connection, + 'Tunnel%d' % tunnel_id) self.assertEqual(requests.codes.BAD_REQUEST, self.csr.status) def test_status_when_no_tunnels_exist(self): diff --git a/neutron/tests/unit/services/vpn/device_drivers/test_cisco_ipsec.py b/neutron/tests/unit/services/vpn/device_drivers/test_cisco_ipsec.py index d2ff8796e..78a0bd84f 100644 --- a/neutron/tests/unit/services/vpn/device_drivers/test_cisco_ipsec.py +++ b/neutron/tests/unit/services/vpn/device_drivers/test_cisco_ipsec.py @@ -426,7 +426,7 @@ class TestCiscoCsrIPsecDeviceDriverSyncStatuses(base.BaseTestCase): self.assertFalse(connection.is_dirty) self.assertEqual(u'Tunnel0', connection.tunnel) self.assertEqual(constants.ACTIVE, connection.last_status) - self.assertEqual(0, self.conn_create.call_count) + self.assertFalse(self.conn_create.called) # TODO(pcm) FUTURE - handling for update (delete/create?) def test_update_of_unknown_ipsec_connection(self): @@ -468,7 +468,7 @@ class TestCiscoCsrIPsecDeviceDriverSyncStatuses(base.BaseTestCase): self.assertFalse(connection.is_dirty) self.assertEqual(u'Tunnel0', connection.tunnel) self.assertEqual(constants.ACTIVE, connection.last_status) - self.assertEqual(0, self.conn_create.call_count) + self.assertFalse(self.conn_create.called) def test_update_connection_admin_down(self): """Connection updated to admin down state - dirty.""" @@ -488,7 +488,7 @@ class TestCiscoCsrIPsecDeviceDriverSyncStatuses(base.BaseTestCase): self.assertTrue(connection.is_dirty) self.assertEqual(u'Tunnel0', connection.tunnel) self.assertEqual(constants.DOWN, connection.last_status) - self.assertEqual(0, self.conn_create.call_count) + self.assertFalse(self.conn_create.called) def test_update_missing_connection_admin_down(self): """Connection not present is in admin down state - nop. @@ -506,7 +506,7 @@ class TestCiscoCsrIPsecDeviceDriverSyncStatuses(base.BaseTestCase): connection = self.driver.update_connection(self.context, u'123', conn_data) self.assertIsNone(connection) - self.assertEqual(0, self.conn_create.call_count) + self.assertFalse(self.conn_create.called) def test_update_for_vpn_service_create(self): """Creation of new IPSec connection on new VPN service - create. @@ -586,7 +586,7 @@ class TestCiscoCsrIPsecDeviceDriverSyncStatuses(base.BaseTestCase): self.assertFalse(connection.is_dirty) self.assertEqual(u'Tunnel0', connection.tunnel) self.assertEqual(constants.ACTIVE, connection.last_status) - self.assertEqual(0, self.conn_create.call_count) + self.assertFalse(self.conn_create.called) def test_update_service_admin_down(self): """VPN service updated to admin down state - dirty. diff --git a/neutron/tests/unit/services/vpn/service_drivers/test_cisco_ipsec.py b/neutron/tests/unit/services/vpn/service_drivers/test_cisco_ipsec.py index 7c53e6fa7..50f3bc2c3 100644 --- a/neutron/tests/unit/services/vpn/service_drivers/test_cisco_ipsec.py +++ b/neutron/tests/unit/services/vpn/service_drivers/test_cisco_ipsec.py @@ -309,12 +309,12 @@ class TestCiscoIPsecDriver(base.BaseTestCase): mock.patch.object(csr_db, 'create_tunnel_mapping').start() self.context = n_ctx.Context('some_user', 'some_tenant') - def _test_update(self, func, args): + def _test_update(self, func, args, reason=None): with mock.patch.object(self.driver.agent_rpc, 'cast') as cast: func(self.context, *args) cast.assert_called_once_with( self.context, - {'args': {}, + {'args': reason, 'namespace': None, 'method': 'vpnservice_updated'}, version='1.0', @@ -322,7 +322,8 @@ class TestCiscoIPsecDriver(base.BaseTestCase): def test_create_ipsec_site_connection(self): self._test_update(self.driver.create_ipsec_site_connection, - [FAKE_VPN_CONNECTION]) + [FAKE_VPN_CONNECTION], + {'reason': 'ipsec-conn-create'}) def test_failure_validation_ipsec_connection(self): """Failure test of validation during IPSec site connection create. @@ -352,12 +353,15 @@ class TestCiscoIPsecDriver(base.BaseTestCase): def test_delete_ipsec_site_connection(self): self._test_update(self.driver.delete_ipsec_site_connection, - [FAKE_VPN_CONNECTION]) + [FAKE_VPN_CONNECTION], + {'reason': 'ipsec-conn-delete'}) def test_update_vpnservice(self): self._test_update(self.driver.update_vpnservice, - [FAKE_VPN_SERVICE, FAKE_VPN_SERVICE]) + [FAKE_VPN_SERVICE, FAKE_VPN_SERVICE], + {'reason': 'vpn-service-update'}) def test_delete_vpnservice(self): self._test_update(self.driver.delete_vpnservice, - [FAKE_VPN_SERVICE]) + [FAKE_VPN_SERVICE], + {'reason': 'vpn-service-delete'})