From db868ea7c17a7924ee84e371a94233213a5cb0ac Mon Sep 17 00:00:00 2001 From: Grzegorz Grasza Date: Thu, 3 Jan 2019 13:20:22 +0100 Subject: [PATCH] Remove unneeded call to nova API, defer other API calls We get an instance ID directly from nova, which calls our API, consequently we don't need to call back to nova to double check if the instance ID realy exists. Additionally, defer calling keystone and glance APIs to the moment that the retrieved objects are realy needed. Change-Id: I64a20c88229490690798aaf75ca0d96d98032467 --- novajoin/join.py | 74 ++++++++++---------------- novajoin/tests/unit/api/v1/test_api.py | 67 ++--------------------- novajoin/tests/unit/fake_constants.py | 4 -- 3 files changed, 31 insertions(+), 114 deletions(-) diff --git a/novajoin/join.py b/novajoin/join.py index 37fa844..8980697 100644 --- a/novajoin/join.py +++ b/novajoin/join.py @@ -24,7 +24,6 @@ from novajoin import exception from novajoin.glance import get_default_image_service from novajoin.ipa import IPAClient from novajoin import keystone_client -from novajoin.nova import get_instance from novajoin import policy from novajoin import util @@ -119,48 +118,36 @@ class JoinController(Controller): except exception.PolicyNotAuthorized: raise base.Fault(webob.exc.HTTPForbidden()) - instance_id = body.get('instance-id') - image_id = body.get('image-id') - project_id = body.get('project-id') hostname_short = body.get('hostname') - metadata = body.get('metadata', {}) - - if not instance_id: - LOG.error('No instance-id in request') - raise base.Fault(webob.exc.HTTPBadRequest()) - if not hostname_short: LOG.error('No hostname in request') raise base.Fault(webob.exc.HTTPBadRequest()) - if not image_id: - LOG.error('No image-id in request') - raise base.Fault(webob.exc.HTTPBadRequest()) + metadata = body.get('metadata', {}) + enroll = metadata.get('ipa_enroll', '').lower() == 'true' - if not project_id: - LOG.error('No project-id in request') - raise base.Fault(webob.exc.HTTPBadRequest()) - - enroll = metadata.get('ipa_enroll', '') - - if enroll.lower() != 'true': - LOG.debug('IPA enrollment not requested in instance creation') - - image_service = get_default_image_service() image_metadata = {} - try: - image = image_service.show(context, image_id) - except (exception.ImageNotFound, exception.ImageNotAuthorized) as e: - msg = 'Failed to get image: %s' % e - LOG.error(msg) - raise base.Fault(webob.exc.HTTPBadRequest(explanation=msg)) - else: - image_metadata = image.get('properties', {}) + if not enroll: + LOG.debug('IPA enrollment not requested in instance creation') + # Check the image metadata to see if enrollment was requested - # Check the image metadata to see if enrollment was requested - if enroll.lower() != 'true': - enroll = image_metadata.get('ipa_enroll', '') - if enroll.lower() != 'true': + image_id = body.get('image-id') + if not image_id: + LOG.error('No image-id in request') + raise base.Fault(webob.exc.HTTPBadRequest()) + image_service = get_default_image_service() + try: + image = image_service.show(context, image_id) + except (exception.ImageNotFound, + exception.ImageNotAuthorized) as e: + msg = 'Failed to get image: %s' % e + LOG.error(msg) + raise base.Fault(webob.exc.HTTPBadRequest(explanation=msg)) + else: + image_metadata = image.get('properties', {}) + + enroll = image_metadata.get('ipa_enroll', '').lower() == 'true' + if not enroll: LOG.debug('IPA enrollment not requested in image') return {} else: @@ -168,22 +155,15 @@ class JoinController(Controller): else: LOG.debug('IPA enrollment requested as property') - # Ensure this instance exists in nova and retrieve the - # name of the user that requested it. - instance = get_instance(instance_id) - if instance is None: - msg = 'No such instance-id, %s' % instance_id - LOG.error(msg) - raise base.Fault(webob.exc.HTTPBadRequest(explanation=msg)) - - # TODO(rcritten): Eventually may check the user for permission - # as well using: - # user = keystone_client.get_user_name(instance.user_id) - hostclass = metadata.get('ipa_hostclass') if hostclass: # Only look up project_name when hostclass is requested to # save a round-trip with Keystone. + project_id = body.get('project-id') + if not project_id: + LOG.error('No project-id in request') + raise base.Fault(webob.exc.HTTPBadRequest()) + project_name = keystone_client.get_project_name(project_id) if project_name is None: msg = 'No such project-id, %s' % project_id diff --git a/novajoin/tests/unit/api/v1/test_api.py b/novajoin/tests/unit/api/v1/test_api.py index 2c5642b..6804c3c 100644 --- a/novajoin/tests/unit/api/v1/test_api.py +++ b/novajoin/tests/unit/api/v1/test_api.py @@ -70,27 +70,8 @@ class JoinTest(test.TestCase): else: assert(False) - def test_no_instanceid(self): - body = {"metadata": {"ipa_enroll": "True"}, - "image-id": fake.IMAGE_ID, - "project-id": fake.PROJECT_ID, - "hostname": "test"} - req = fakes.HTTPRequest.blank('/v1/') - req.method = 'POST' - req.content_type = "application/json" - - # Not using assertRaises because the exception is wrapped as - # a Fault - try: - self.join_controller.create(req, body) - except Fault as fault: - assert fault.status_int == 400 - else: - assert(False) - def test_no_imageid(self): - body = {"metadata": {"ipa_enroll": "True"}, - "instance-id": fake.INSTANCE_ID, + body = {"metadata": {"ipa_enroll": "False"}, "project-id": fake.PROJECT_ID, "hostname": "test"} req = fakes.HTTPRequest.blank('/v1/') @@ -108,7 +89,6 @@ class JoinTest(test.TestCase): def test_no_hostname(self): body = {"metadata": {"ipa_enroll": "True"}, - "instance-id": fake.INSTANCE_ID, "project-id": fake.PROJECT_ID, "image-id": fake.IMAGE_ID} req = fakes.HTTPRequest.blank('/v1/') @@ -125,8 +105,7 @@ class JoinTest(test.TestCase): assert(False) def test_no_project_id(self): - body = {"metadata": {"ipa_enroll": "True"}, - "instance-id": fake.INSTANCE_ID, + body = {"metadata": {"ipa_enroll": "True", "ipa_hostclass": "foo"}, "image-id": fake.IMAGE_ID, "hostname": "test"} req = fakes.HTTPRequest.blank('/v1/') @@ -146,7 +125,6 @@ class JoinTest(test.TestCase): def test_request_no_enrollment(self, mock_get_image): mock_get_image.return_value = FakeImageService() body = {"metadata": {"ipa_enroll": "False"}, - "instance-id": fake.INSTANCE_ID, "project-id": fake.PROJECT_ID, "image-id": fake.IMAGE_ID, "hostname": "test"} @@ -162,7 +140,6 @@ class JoinTest(test.TestCase): def test_request_invalid_image(self, mock_get_image): mock_get_image.side_effect = Fault(webob.exc.HTTPBadRequest()) body = {"metadata": {"ipa_enroll": "False"}, - "instance-id": fake.INSTANCE_ID, "project-id": fake.PROJECT_ID, "image-id": "invalid", "hostname": "test"} @@ -181,13 +158,11 @@ class JoinTest(test.TestCase): assert(False) @mock.patch('novajoin.ipa.SafeConfigParser') - @mock.patch('novajoin.join.get_instance') @mock.patch('novajoin.join.get_default_image_service') @mock.patch('novajoin.util.get_domain') def test_valid_request(self, mock_get_domain, mock_get_image, - mock_get_instance, mock_conf_parser): + mock_conf_parser): mock_get_image.return_value = FakeImageService() - mock_get_instance.return_value = fake.fake_instance mock_get_domain.return_value = "test" mock_conf_parser_instance = mock.MagicMock() @@ -196,7 +171,6 @@ class JoinTest(test.TestCase): mock_conf_parser.return_value = mock_conf_parser_instance body = {"metadata": {"ipa_enroll": "True"}, - "instance-id": fake.INSTANCE_ID, "project-id": fake.PROJECT_ID, "image-id": fake.IMAGE_ID, "hostname": "test"} @@ -221,15 +195,12 @@ class JoinTest(test.TestCase): @mock.patch('novajoin.ipa.SafeConfigParser') @mock.patch('novajoin.keystone_client.get_project_name') - @mock.patch('novajoin.join.get_instance') @mock.patch('novajoin.join.get_default_image_service') @mock.patch('novajoin.util.get_domain') def test_valid_hostclass_request(self, mock_get_domain, mock_get_image, - mock_get_instance, mock_get_project_name, mock_conf_parser): mock_get_image.return_value = FakeImageService() - mock_get_instance.return_value = fake.fake_instance mock_get_domain.return_value = "test" mock_get_project_name.return_value = "test" @@ -239,7 +210,6 @@ class JoinTest(test.TestCase): mock_conf_parser.return_value = mock_conf_parser_instance body = {"metadata": {"ipa_enroll": "True"}, - "instance-id": fake.INSTANCE_ID, "project-id": fake.PROJECT_ID, "image-id": fake.IMAGE_ID, "hostname": "test"} @@ -262,45 +232,16 @@ class JoinTest(test.TestCase): # because in all likelihood the keytab cannot be read (and # probably doesn't exist. This can be ignored. - @mock.patch('novajoin.join.get_instance') - @mock.patch('novajoin.join.get_default_image_service') - def test_invalid_instance_id(self, mock_get_image, mock_get_instance): - """Mock the instance to not exist so there is nothing to enroll.""" - mock_get_image.return_value = FakeImageService() - mock_get_instance.return_value = None - - body = {"metadata": {"ipa_enroll": "True"}, - "instance-id": "invalid", - "project-id": fake.PROJECT_ID, - "image-id": fake.IMAGE_ID, - "hostname": "test"} - req = fakes.HTTPRequest.blank('/v1') - req.method = 'POST' - req.content_type = "application/json" - req.body = jsonutils.dump_as_bytes(body) - - # Not using assertRaises because the exception is wrapped as - # a Fault - try: - self.join_controller.create(req, body) - except Fault as fault: - assert fault.status_int == 400 - else: - assert(False) - - @mock.patch('novajoin.join.get_instance') @mock.patch('novajoin.join.get_default_image_service') @mock.patch('novajoin.keystone_client.get_project_name') @mock.patch('novajoin.util.get_domain') def test_invalid_project_id(self, mock_get_domain, mock_get_project_name, - mock_get_image, mock_get_instance): + mock_get_image): mock_get_image.return_value = FakeImageService() - mock_get_instance.return_value = None mock_get_project_name.return_value = None mock_get_domain.return_value = "test" body = {"metadata": {"ipa_enroll": "True", "ipa_hostclass": "foo"}, - "instance-id": fake.INSTANCE_ID, "project-id": "invalid", "image-id": fake.IMAGE_ID, "hostname": "test"} diff --git a/novajoin/tests/unit/fake_constants.py b/novajoin/tests/unit/fake_constants.py index 2322769..e10033a 100644 --- a/novajoin/tests/unit/fake_constants.py +++ b/novajoin/tests/unit/fake_constants.py @@ -14,8 +14,4 @@ PROJECT_ID = '89afd400-b646-4bbc-b12b-c0a4d63e5bd3' USER_ID = 'c853ca26-e8ea-4797-8a52-ee124a013d0e' -INSTANCE_ID = 'e4274dc8-325a-409b-92fd-cfdfdd65ae8b' IMAGE_ID = 'b8c88e01-c820-40f6-b026-00926706e374' - -# In reality this should be a Server object. -fake_instance = {'instance_name': 'test', 'id': INSTANCE_ID}