From 4a2009a6fa2017013102d3d43e670334ebc60d9f Mon Sep 17 00:00:00 2001 From: Sean Dague Date: Fri, 24 Feb 2017 13:56:03 -0500 Subject: [PATCH] Be more tolerant of keystone catalog configuration The previous attempt at validating against keystone assumed that users had an unversioned endpoint in their catalog. Most do not. This uses the keystoneauth discovery magic to do the right thing whether or not they have the versioned or unversioned endpoint, as long as they have a working v3 installation. It is also much more verbose on log messages when things go wrong. Change-Id: I509d406ca1f1f7b064aaca88645ad17685827267 Related-Bug: #1667679 Closes-Bug: #1693228 --- nova/api/openstack/identity.py | 20 +++-- .../openstack/compute/test_flavor_access.py | 6 ++ .../unit/api/openstack/compute/test_quotas.py | 14 ++++ nova/tests/unit/test_identity.py | 81 ++++++++++++++++--- 4 files changed, 107 insertions(+), 14 deletions(-) diff --git a/nova/api/openstack/identity.py b/nova/api/openstack/identity.py index 6931f3bb1df5..4bcc0a026a0d 100644 --- a/nova/api/openstack/identity.py +++ b/nova/api/openstack/identity.py @@ -30,10 +30,22 @@ def verify_project_id(context, project_id): """ sess = session.Session(auth=context.get_auth_plugin()) + failure = webob.exc.HTTPBadRequest( + explanation=_("Project ID %s is not a valid project.") % + project_id) try: - resp = sess.get('/v3/projects/%s' % project_id, - endpoint_filter={'service_type': 'identity'}, + resp = sess.get('/projects/%s' % project_id, + endpoint_filter={ + 'service_type': 'identity', + 'version': (3, 0) + }, raise_exc=False) + except kse.EndpointNotFound: + LOG.error( + "Keystone identity service version 3.0 was not found. This might " + "be because your endpoint points to the v2.0 versioned endpoint " + "which is not supported. Please fix this.") + raise failure except kse.ClientException: # something is wrong, like there isn't a keystone v3 endpoint, # we'll take the pass and default to everything being ok. @@ -45,9 +57,7 @@ def verify_project_id(context, project_id): return True elif resp.status_code == 404: # we got access, and we know this project is not there - raise webob.exc.HTTPBadRequest( - explanation=_("Project ID %s is not a valid project.") % - project_id) + raise failure elif resp.status_code == 403: # we don't have enough permission to verify this, so default # to "it's ok". diff --git a/nova/tests/unit/api/openstack/compute/test_flavor_access.py b/nova/tests/unit/api/openstack/compute/test_flavor_access.py index 2495eb1fda50..5f1852911d7a 100644 --- a/nova/tests/unit/api/openstack/compute/test_flavor_access.py +++ b/nova/tests/unit/api/openstack/compute/test_flavor_access.py @@ -136,6 +136,12 @@ class FlavorAccessTestV21(test.NoDBTestCase): def setUp(self): super(FlavorAccessTestV21, self).setUp() self.flavor_controller = flavors_api.FlavorsController() + # We need to stub out verify_project_id so that it doesn't + # generate an EndpointNotFound exception and result in a + # server error. + self.stub_out('nova.api.openstack.identity.verify_project_id', + lambda ctx, project_id: True) + self.req = FakeRequest() self.req.environ = {"nova.context": context.RequestContext('fake_user', 'fake')} diff --git a/nova/tests/unit/api/openstack/compute/test_quotas.py b/nova/tests/unit/api/openstack/compute/test_quotas.py index d1a61fa312bc..d87184c883f2 100644 --- a/nova/tests/unit/api/openstack/compute/test_quotas.py +++ b/nova/tests/unit/api/openstack/compute/test_quotas.py @@ -41,6 +41,14 @@ def quota_set(id, include_server_group_quotas=True): class BaseQuotaSetsTest(test.TestCase): + def setUp(self): + super(BaseQuotaSetsTest, self).setUp() + # We need to stub out verify_project_id so that it doesn't + # generate an EndpointNotFound exception and result in a + # server error. + self.stub_out('nova.api.openstack.identity.verify_project_id', + lambda ctx, project_id: True) + def get_delete_status_int(self, res): # NOTE: on v2.1, http status code is set as wsgi_code of API # method instead of status_int in a response object. @@ -531,6 +539,12 @@ class QuotaSetsTestV236(test.NoDBTestCase): def setUp(self): super(QuotaSetsTestV236, self).setUp() + # We need to stub out verify_project_id so that it doesn't + # generate an EndpointNotFound exception and result in a + # server error. + self.stub_out('nova.api.openstack.identity.verify_project_id', + lambda ctx, project_id: True) + self.flags(enable_network_quota=True) tenant_networks._register_network_quota() self.old_req = fakes.HTTPRequest.blank('', version='2.1') diff --git a/nova/tests/unit/test_identity.py b/nova/tests/unit/test_identity.py index e2c12832cc6b..bd2285ba7781 100644 --- a/nova/tests/unit/test_identity.py +++ b/nova/tests/unit/test_identity.py @@ -47,46 +47,109 @@ class FakeResponse(object): class IdentityValidationTest(test.NoDBTestCase): + """Unit tests for our validation of keystone projects. + + There are times when Nova stores keystone project_id and user_id + in our database as strings. Until the Pike release none of this + data was validated, so it was very easy for adminstrators to think + they were adjusting quota for a project (by name) when instead + they were just inserting keys in a database that would not get used. + + This is only tested in unit tests through mocking out keystoneauth + responses because a functional test would need a real keystone or + keystone simulator. + + The functional code works by using the existing keystone + credentials and trying to make a /v3/projects/{id} get call. It + will return a 403 if the user doesn't have enough permissions to + ask about other projects, a 404 if it does and that project does + not exist. + + """ @mock.patch('keystoneauth1.session.Session.get') def test_good_id(self, get): + """Test response 200. + + This indicates we have permissions, and we have definitively + found the project exists. + + """ get.return_value = FakeResponse(200) self.assertTrue(identity.verify_project_id(mock.MagicMock(), "foo")) get.assert_called_once_with( - '/v3/projects/foo', - endpoint_filter={'service_type': 'identity'}, + '/projects/foo', + endpoint_filter={'service_type': 'identity', 'version': (3, 0)}, raise_exc=False) @mock.patch('keystoneauth1.session.Session.get') def test_no_project(self, get): + """Test response 404. + + This indicates that we have permissions, and we have + definitively found the project does not exist. + + """ get.return_value = FakeResponse(404) self.assertRaises(webob.exc.HTTPBadRequest, identity.verify_project_id, mock.MagicMock(), "foo") get.assert_called_once_with( - '/v3/projects/foo', - endpoint_filter={'service_type': 'identity'}, + '/projects/foo', + endpoint_filter={'service_type': 'identity', 'version': (3, 0)}, raise_exc=False) @mock.patch('keystoneauth1.session.Session.get') def test_unknown_id(self, get): + """Test response 403. + + This indicates we don't have permissions. We fail open here + and assume the project exists. + + """ get.return_value = FakeResponse(403) self.assertTrue(identity.verify_project_id(mock.MagicMock(), "foo")) get.assert_called_once_with( - '/v3/projects/foo', - endpoint_filter={'service_type': 'identity'}, + '/projects/foo', + endpoint_filter={'service_type': 'identity', 'version': (3, 0)}, raise_exc=False) @mock.patch('keystoneauth1.session.Session.get') def test_unknown_error(self, get): + """Test some other return from keystone. + + If we got anything else, something is wrong on the keystone + side. We don't want to fail on our side. + + """ get.return_value = FakeResponse(500, "Oh noes!") self.assertTrue(identity.verify_project_id(mock.MagicMock(), "foo")) get.assert_called_once_with( - '/v3/projects/foo', - endpoint_filter={'service_type': 'identity'}, + '/projects/foo', + endpoint_filter={'service_type': 'identity', 'version': (3, 0)}, raise_exc=False) @mock.patch('keystoneauth1.session.Session.get') def test_early_fail(self, get): - get.side_effect = kse.EndpointNotFound() + """Test if we get a keystoneauth exception. + + If we get a random keystoneauth exception, fall back and + assume the project exists. + + """ + get.side_effect = kse.ConnectionError() self.assertTrue(identity.verify_project_id(mock.MagicMock(), "foo")) + + @mock.patch('keystoneauth1.session.Session.get') + def test_wrong_version(self, get): + """Test endpoint not found. + + EndpointNotFound will be made when the keystone v3 API is not + found in the service catalog, or if the v2.0 endpoint was + registered as the root endpoint. We treat this the same as 404. + + """ + get.side_effect = kse.EndpointNotFound() + self.assertRaises(webob.exc.HTTPBadRequest, + identity.verify_project_id, + mock.MagicMock(), "foo")