From e3b6c9662e3210a3126aabde967f231af1b025ae Mon Sep 17 00:00:00 2001 From: Alexandru Coman Date: Tue, 6 Sep 2016 10:31:23 +0300 Subject: [PATCH] Refactor the CloudStack Metadata Service This patch changes the way that the service interacts with the Password Server in order to ease the service testing and to remove redundant code. Change-Id: I855899f6163465dfc2974047efb0dfd290a617ef Co-Authored-By: Stefan Caraiman Implements-Blueprint: cloudstack-password-port --- cloudbaseinit/conf/cloudstack.py | 4 + cloudbaseinit/metadata/services/cloudstack.py | 125 ++++++++++-------- .../metadata/services/test_cloudstack.py | 64 ++++----- 3 files changed, 97 insertions(+), 96 deletions(-) diff --git a/cloudbaseinit/conf/cloudstack.py b/cloudbaseinit/conf/cloudstack.py index a175f5dd..664a6e41 100644 --- a/cloudbaseinit/conf/cloudstack.py +++ b/cloudbaseinit/conf/cloudstack.py @@ -31,6 +31,10 @@ class CloudStackOptions(conf_base.Options): help="The base URL where the service looks for metadata", deprecated_name="cloudstack_metadata_ip", deprecated_group="DEFAULT"), + cfg.IntOpt( + "password_server_port", default=8080, + help="The port number used by the Password Server." + ), cfg.BoolOpt( "https_allow_insecure", default=False, help="Whether to disable the validation of HTTPS " diff --git a/cloudbaseinit/metadata/services/cloudstack.py b/cloudbaseinit/metadata/services/cloudstack.py index 4567ebf5..f13ff57b 100644 --- a/cloudbaseinit/metadata/services/cloudstack.py +++ b/cloudbaseinit/metadata/services/cloudstack.py @@ -27,25 +27,34 @@ from cloudbaseinit.utils import encoding CONF = cloudbaseinit_conf.CONF LOG = oslo_logging.getLogger(__name__) -BAD_REQUEST = b"bad_request" -SAVED_PASSWORD = b"saved_password" +BAD_REQUEST = "bad_request" +SAVED_PASSWORD = "saved_password" TIMEOUT = 10 class CloudStack(base.BaseHTTPMetadataService): + """Metadata service for Apache CloudStack. + + Apache CloudStack is an open source software designed to deploy and + manage large networks of virtual machines, as a highly available, + highly scalable Infrastructure as a Service (IaaS) cloud computing + platform. + """ + def __init__(self): - # Note: The base url used by the current metadata service will be - # updated later by the `_test_api` method. super(CloudStack, self).__init__( + # Note(alexcoman): The base url used by the current metadata + # service will be updated later by the `_test_api` method. base_url=None, https_allow_insecure=CONF.cloudstack.https_allow_insecure, https_ca_bundle=CONF.cloudstack.https_ca_bundle) self._osutils = osutils_factory.get_os_utils() - self._router_ip = None + self._metadata_host = None - def _get_path(self, resource, version="latest"): + @staticmethod + def _get_path(resource, version="latest"): """Get the relative path for the received resource.""" return posixpath.normpath( posixpath.join(version, "meta-data", resource)) @@ -68,7 +77,7 @@ class CloudStack(base.BaseHTTPMetadataService): LOG.debug('Available services: %s', response) netloc = urllib.parse.urlparse(metadata_url).netloc - self._router_ip = netloc.split(":")[0] + self._metadata_host = netloc.split(":")[0] return True def load(self): @@ -114,13 +123,37 @@ class CloudStack(base.BaseHTTPMetadataService): ssh_keys.append(ssh_key) return ssh_keys + def _password_client(self, body=None, headers=None, decode=True): + """Client for the Password Server.""" + port = CONF.cloudstack.password_server_port + with contextlib.closing(http_client.HTTPConnection( + self._metadata_host, port, timeout=TIMEOUT)) as connection: + try: + connection.request("GET", "/", body=body, headers=headers) + response = connection.getresponse() + except http_client.HTTPException as exc: + LOG.error("Request failed: %s", exc) + raise + + content = response.read() + if decode: + content = encoding.get_as_string(content) + + if response.status != 200: + raise http_client.HTTPException( + "%(status)s %(reason)s - %(message)r", + {"status": response.status, "reason": response.reason, + "message": content}) + + return content + def _get_password(self): """Get the password from the Password Server. The Password Server can be found on the DHCP_SERVER on the port 8080. .. note: The Password Server can return the following values: - * `bad_request`: the Password Server did not recognise + * `bad_request`: the Password Server did not recognize the request * `saved_password`: the password was already deleted from the Password Server @@ -132,45 +165,33 @@ class CloudStack(base.BaseHTTPMetadataService): headers = {"DomU_Request": "send_my_password"} password = None - with contextlib.closing(http_client.HTTPConnection( - self._router_ip, 8080, timeout=TIMEOUT)) as connection: - for _ in range(CONF.retry_count): - try: - connection.request("GET", "/", headers=headers) - response = connection.getresponse() - except http_client.HTTPException as exc: - LOG.exception(exc) - continue + for _ in range(CONF.retry_count): + try: + content = self._password_client(headers=headers).strip() + except http_client.HTTPConnection as exc: + LOG.error("Getting password failed: %s", exc) + continue - if response.status != 200: - LOG.warning("Getting password failed: %(status)s " - "%(reason)s - %(message)r", - {"status": response.status, - "reason": response.reason, - "message": response.read()}) - continue + if not content: + LOG.warning("The Password Server did not have any " + "password for the current instance.") + continue - content = response.read().strip() - if not content: - LOG.warning("The Password Server did not have any " - "password for the current instance.") - continue - - if content == BAD_REQUEST: - LOG.error("The Password Server did not recognise the " - "request.") - break - - if content == SAVED_PASSWORD: - LOG.warning("For this instance the password was already " - "taken from the Password Server.") - break - - LOG.info("The password server return a valid password " - "for the current instance.") - password = encoding.get_as_string(content) + if content == BAD_REQUEST: + LOG.error("The Password Server did not recognize the " + "request.") break + if content == SAVED_PASSWORD: + LOG.warning("The password was already taken from the " + "Password Server for the current instance.") + break + + LOG.info("The password server returned a valid password " + "for the current instance.") + password = content + break + return password def _delete_password(self): @@ -182,21 +203,15 @@ class CloudStack(base.BaseHTTPMetadataService): LOG.debug("Remove the password for this instance from the " "Password Server.") headers = {"DomU_Request": "saved_password"} - connection = http_client.HTTPConnection(self._router_ip, 8080, - timeout=TIMEOUT) + for _ in range(CONF.retry_count): - connection.request("GET", "/", headers=headers) - response = connection.getresponse() - if response.status != 200: - LOG.warning("Removing password failed: %(status)s " - "%(reason)s - %(message)r", - {"status": response.status, - "reason": response.reason, - "message": response.read()}) + try: + content = self._password_client(headers=headers).strip() + except http_client.HTTPConnection as exc: + LOG.error("Removing password failed: %s", exc) continue - content = response.read() - if content != BAD_REQUEST: # comparing bytes with bytes + if content != BAD_REQUEST: LOG.info("The password was removed from the Password Server.") break else: diff --git a/cloudbaseinit/tests/metadata/services/test_cloudstack.py b/cloudbaseinit/tests/metadata/services/test_cloudstack.py index 06d21282..a50b6b84 100644 --- a/cloudbaseinit/tests/metadata/services/test_cloudstack.py +++ b/cloudbaseinit/tests/metadata/services/test_cloudstack.py @@ -170,20 +170,15 @@ class CloudStackTest(unittest.TestCase): response = self._service.get_public_keys() self.assertEqual([], response) - @mock.patch('six.moves.http_client.HTTPConnection') - def test_get_password(self, mock_http_connection): + @mock.patch('cloudbaseinit.metadata.services.cloudstack.CloudStack' + '._password_client') + def test_get_password(self, mock_password_client): headers = {"DomU_Request": "send_my_password"} - mock_connection = mock.Mock() - mock_http_connection.return_value = mock_connection - mock_response = mock_connection.getresponse() - mock_request = mock_connection.request - mock_response.status = 200 - expected_password = b"password" - mock_response.read.side_effect = [expected_password] - self._service._router_ip = mock.sentinel.router_ip + expected_password = "password" + mock_password_client.return_value = expected_password expected_output = [ "Try to get password from the Password Server.", - "The password server return a valid password " + "The password server returned a valid password " "for the current instance." ] @@ -191,29 +186,22 @@ class CloudStackTest(unittest.TestCase): 'cloudstack') as snatcher: password = self._service._get_password() - mock_http_connection.assert_called_once_with( - mock.sentinel.router_ip, 8080, timeout=cloudstack.TIMEOUT) - mock_request.assert_called_once_with("GET", "/", headers=headers) - - self.assertEqual(expected_password.decode(), password) + mock_password_client.assert_called_once_with(headers=headers) + self.assertEqual(expected_password, password) self.assertEqual(expected_output, snatcher.output) - @mock.patch('six.moves.http_client.HTTPConnection') - def test_get_password_fail(self, mock_http_connection): - mock_connection = mock.Mock() - mock_http_connection.return_value = mock_connection - mock_response = mock_connection.getresponse() - mock_request = mock_connection.request - mock_response.status = 200 - mock_response.read.side_effect = [b"", cloudstack.BAD_REQUEST, - cloudstack.SAVED_PASSWORD] + @mock.patch('cloudbaseinit.metadata.services.cloudstack.CloudStack' + '._password_client') + def test_get_password_fail(self, mock_password_client): + mock_password_client.side_effect = ["", cloudstack.BAD_REQUEST, + cloudstack.SAVED_PASSWORD] expected_output = [ ["Try to get password from the Password Server.", - "For this instance the password was already taken from " - "the Password Server."], + "The password was already taken from the Password Server " + "for the current instance."], ["Try to get password from the Password Server.", - "The Password Server did not recognise the request."], + "The Password Server did not recognize the request."], ["Try to get password from the Password Server.", "The Password Server did not have any password for the " @@ -225,21 +213,16 @@ class CloudStackTest(unittest.TestCase): self.assertIsNone(self._service._get_password()) self.assertEqual(expected_output.pop(), snatcher.output) - self.assertEqual(3, mock_request.call_count) + self.assertEqual(3, mock_password_client.call_count) - @mock.patch('six.moves.http_client.HTTPConnection') - def test_delete_password(self, mock_http_connection): - mock_connection = mock.Mock() - mock_http_connection.return_value = mock_connection - mock_response = mock_connection.getresponse() - mock_request = mock_connection.request - mock_response.read.side_effect = [cloudstack.BAD_REQUEST, - cloudstack.SAVED_PASSWORD] - mock_response.status = 400 + @mock.patch('cloudbaseinit.metadata.services.cloudstack.CloudStack' + '._password_client') + def test_delete_password(self, mock_password_client): + mock_password_client.side_effect = [cloudstack.BAD_REQUEST, + cloudstack.SAVED_PASSWORD] expected_output = [ 'Remove the password for this instance from the ' 'Password Server.', - 'Removing password failed', 'Fail to remove the password from the Password Server.', 'Remove the password for this instance from the ' @@ -251,9 +234,8 @@ class CloudStackTest(unittest.TestCase): with testutils.LogSnatcher('cloudbaseinit.metadata.services.' 'cloudstack') as snatcher: self.assertIsNone(self._service._delete_password()) - mock_response.status = 200 self.assertIsNone(self._service._delete_password()) - self.assertEqual(2, mock_request.call_count) + self.assertEqual(2, mock_password_client.call_count) for expected, output in zip(expected_output, snatcher.output): self.assertTrue(output.startswith(expected))