From cba2192b6784892b23b9069ae89ec997c0caa210 Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Wed, 11 Sep 2019 16:03:04 -0700 Subject: [PATCH 01/13] Add IMDS backup url --- azurelinuxagent/common/protocol/imds.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/azurelinuxagent/common/protocol/imds.py b/azurelinuxagent/common/protocol/imds.py index d10bd2b746..3125f68443 100644 --- a/azurelinuxagent/common/protocol/imds.py +++ b/azurelinuxagent/common/protocol/imds.py @@ -11,6 +11,7 @@ from azurelinuxagent.common.utils.flexible_version import FlexibleVersion IMDS_ENDPOINT = '169.254.169.254' +IMDS_ENDPOINT_BACKUP = '168.63.129.16' APIVERSION = '2018-02-01' BASE_URI = "http://{0}/metadata/instance/{1}?api-version={2}" @@ -248,10 +249,18 @@ def __init__(self, version=APIVERSION): def compute_url(self): return BASE_URI.format(IMDS_ENDPOINT, 'compute', self._api_version) + @property + def compute_url_backup(self): + return BASE_URI.format(IMDS_ENDPOINT_BACKUP, 'compute', self._api_version) + @property def instance_url(self): return BASE_URI.format(IMDS_ENDPOINT, '', self._api_version) + @property + def instance_url_backup(self): + return BASE_URI.format(IMDS_ENDPOINT_BACKUP, '', self._api_version) + def get_compute(self): """ Fetch compute information. @@ -263,7 +272,9 @@ def get_compute(self): resp = restutil.http_get(self.compute_url, headers=self._headers) if restutil.request_failed(resp): - raise HttpError("{0} - GET: {1}".format(resp.status, self.compute_url)) + resp = restutil.http_get(self.compute_url_backup, headers=self._headers) + if restutil.request_failed(resp): + raise HttpError("{0} - GET: {1}".format(resp.status, self.compute_url)) data = resp.read() data = json.loads(ustr(data, encoding="utf-8")) @@ -286,7 +297,9 @@ def validate(self): # ensure we get a 200 resp = restutil.http_get(self.instance_url, headers=self._health_headers) if restutil.request_failed(resp): - return False, "{0}".format(restutil.read_response_error(resp)) + resp = restutil.http_get(self.instance_url_backup, headers=self._health_headers) + if restutil.request_failed(resp): + return False, "{0}".format(restutil.read_response_error(resp)) # ensure the response is valid json data = resp.read() From e417b36e543991fe7858c95d819ac690977b61a0 Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Thu, 12 Sep 2019 13:19:46 -0700 Subject: [PATCH 02/13] Update imds tests --- tests/protocol/test_imds.py | 38 ++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/tests/protocol/test_imds.py b/tests/protocol/test_imds.py index 327aa020cf..fd68999305 100644 --- a/tests/protocol/test_imds.py +++ b/tests/protocol/test_imds.py @@ -235,44 +235,53 @@ def _setup_image_origin_assert(publisher, offer, sku, version): return compute_info.image_origin def test_response_validation(self): + # expect extra "fallback" calls on error status codes + # invalid json or empty response self._assert_validation(http_status_code=200, http_response='', expected_valid=False, - expected_response='JSON parsing failed') + expected_response='JSON parsing failed', + expected_fallback=False) self._assert_validation(http_status_code=200, http_response=None, expected_valid=False, - expected_response='JSON parsing failed') + expected_response='JSON parsing failed', + expected_fallback=False) self._assert_validation(http_status_code=200, http_response='{ bad json ', expected_valid=False, - expected_response='JSON parsing failed') + expected_response='JSON parsing failed', + expected_fallback=False) # 500 response self._assert_validation(http_status_code=500, http_response='error response', expected_valid=False, - expected_response='[HTTP Failed] [500: reason] error response') + expected_response='[HTTP Failed] [500: reason] error response', + expected_fallback=True) # 429 response self._assert_validation(http_status_code=429, http_response='server busy', expected_valid=False, - expected_response='[HTTP Failed] [429: reason] server busy') + expected_response='[HTTP Failed] [429: reason] server busy', + expected_fallback=True) # valid json self._assert_validation(http_status_code=200, http_response=self._imds_response('valid'), expected_valid=True, - expected_response='') + expected_response='', + expected_fallback=False) # unicode self._assert_validation(http_status_code=200, http_response=self._imds_response('unicode'), expected_valid=True, - expected_response='') + expected_response='', + expected_fallback=False) def test_field_validation(self): # TODO: compute fields (#1249) @@ -322,7 +331,7 @@ def _imds_response(f): with open(path, "rb") as fh: return fh.read() - def _assert_validation(self, http_status_code, http_response, expected_valid, expected_response): + def _assert_validation(self, http_status_code, http_response, expected_valid, expected_response, expected_fallback): test_subject = imds.ImdsClient() with patch("azurelinuxagent.common.utils.restutil.http_get") as mock_http_get: mock_http_get.return_value = ResponseMock(status=http_status_code, @@ -330,15 +339,22 @@ def _assert_validation(self, http_status_code, http_response, expected_valid, ex response=http_response) validate_response = test_subject.validate() - self.assertEqual(1, mock_http_get.call_count) + if expected_fallback: + self.assertEqual(2, mock_http_get.call_count) + else: + self.assertEqual(1, mock_http_get.call_count) positional_args, kw_args = mock_http_get.call_args self.assertTrue('User-Agent' in kw_args['headers']) self.assertEqual(restutil.HTTP_USER_AGENT_HEALTH, kw_args['headers']['User-Agent']) self.assertTrue('Metadata' in kw_args['headers']) self.assertEqual(True, kw_args['headers']['Metadata']) - self.assertEqual('http://169.254.169.254/metadata/instance/?api-version=2018-02-01', - positional_args[0]) + if expected_fallback: + self.assertEqual('http://168.63.129.16/metadata/instance/?api-version=2018-02-01', + positional_args[0]) + else: + self.assertEqual('http://169.254.169.254/metadata/instance/?api-version=2018-02-01', + positional_args[0]) self.assertEqual(expected_valid, validate_response[0]) self.assertTrue(expected_response in validate_response[1], "Expected: '{0}', Actual: '{1}'" From c8112635524a38f3a1572f90e9d358531a5db9bb Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Thu, 12 Sep 2019 13:21:02 -0700 Subject: [PATCH 03/13] Update imds tests --- tests/protocol/test_imds.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/protocol/test_imds.py b/tests/protocol/test_imds.py index fd68999305..b7048b00a9 100644 --- a/tests/protocol/test_imds.py +++ b/tests/protocol/test_imds.py @@ -302,7 +302,8 @@ def _assert_field(self, *fields): self._assert_validation(http_status_code=200, http_response=altered_response, expected_valid=False, - expected_response='Empty field: [{0}]'.format(fields[-1])) + expected_response='Empty field: [{0}]'.format(fields[-1]), + expected_fallback=False) # assert missing value self._update_field(response_obj, fields, None) @@ -310,7 +311,8 @@ def _assert_field(self, *fields): self._assert_validation(http_status_code=200, http_response=altered_response, expected_valid=False, - expected_response='Missing field: [{0}]'.format(fields[-1])) + expected_response='Missing field: [{0}]'.format(fields[-1]), + expected_fallback=False) def _update_field(self, obj, fields, val): if isinstance(obj, list): From 1c8da0dc040995e009a40b3c89a9ad41cb9d1ad8 Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Thu, 19 Sep 2019 14:17:56 -0700 Subject: [PATCH 04/13] Use endpoint from dhcp instead of hardcoded value --- azurelinuxagent/common/protocol/imds.py | 8 ++++---- azurelinuxagent/common/protocol/util.py | 6 +++--- tests/protocol/test_imds.py | 14 ++++++++++---- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/azurelinuxagent/common/protocol/imds.py b/azurelinuxagent/common/protocol/imds.py index 3125f68443..9e0e8bf6ab 100644 --- a/azurelinuxagent/common/protocol/imds.py +++ b/azurelinuxagent/common/protocol/imds.py @@ -8,10 +8,10 @@ from azurelinuxagent.common.future import ustr import azurelinuxagent.common.logger as logger from azurelinuxagent.common.protocol.restapi import DataContract, set_properties +from azurelinuxagent.common.protocol.util import get_protocol_util from azurelinuxagent.common.utils.flexible_version import FlexibleVersion IMDS_ENDPOINT = '169.254.169.254' -IMDS_ENDPOINT_BACKUP = '168.63.129.16' APIVERSION = '2018-02-01' BASE_URI = "http://{0}/metadata/instance/{1}?api-version={2}" @@ -243,7 +243,7 @@ def __init__(self, version=APIVERSION): 'User-Agent': restutil.HTTP_USER_AGENT_HEALTH, 'Metadata': True, } - pass + self.protocol_util = get_protocol_util() @property def compute_url(self): @@ -251,7 +251,7 @@ def compute_url(self): @property def compute_url_backup(self): - return BASE_URI.format(IMDS_ENDPOINT_BACKUP, 'compute', self._api_version) + return BASE_URI.format(self.protocol_util.get_wireserver_endpoint(), 'compute', self._api_version) @property def instance_url(self): @@ -259,7 +259,7 @@ def instance_url(self): @property def instance_url_backup(self): - return BASE_URI.format(IMDS_ENDPOINT_BACKUP, '', self._api_version) + return BASE_URI.format(self.protocol_util.get_wireserver_endpoint(), '', self._api_version) def get_compute(self): """ diff --git a/azurelinuxagent/common/protocol/util.py b/azurelinuxagent/common/protocol/util.py index 186ff76ba5..692b54890c 100644 --- a/azurelinuxagent/common/protocol/util.py +++ b/azurelinuxagent/common/protocol/util.py @@ -152,7 +152,7 @@ def _get_tag_file_path(self): conf.get_lib_dir(), TAG_FILE_NAME) - def _get_wireserver_endpoint(self): + def get_wireserver_endpoint(self): try: file_path = os.path.join(conf.get_lib_dir(), ENDPOINT_FILE_NAME) return fileutil.read_file(file_path) @@ -182,7 +182,7 @@ def _detect_wire_protocol(self): endpoint = self.dhcp_handler.endpoint else: logger.info("_detect_wire_protocol: DHCP not available") - endpoint = self._get_wireserver_endpoint() + endpoint = self.get_wireserver_endpoint() if endpoint == None: endpoint = conf_endpoint logger.info("Using hardcoded WireServer endpoint {0}", endpoint) @@ -239,7 +239,7 @@ def _get_protocol(self): protocol_name = fileutil.read_file(protocol_file_path) if protocol_name == prots.WireProtocol: - endpoint = self._get_wireserver_endpoint() + endpoint = self.get_wireserver_endpoint() return WireProtocol(endpoint) elif protocol_name == prots.MetadataProtocol: return MetadataProtocol() diff --git a/tests/protocol/test_imds.py b/tests/protocol/test_imds.py index b7048b00a9..28328cb20c 100644 --- a/tests/protocol/test_imds.py +++ b/tests/protocol/test_imds.py @@ -8,6 +8,7 @@ from azurelinuxagent.common.future import ustr from azurelinuxagent.common.protocol.restapi import set_properties from azurelinuxagent.common.utils import restutil +from azurelinuxagent.common.protocol.util import ProtocolUtil, get_protocol_util from tests.ga.test_update import ResponseMock from tests.tools import * @@ -45,9 +46,13 @@ def test_get(self, mock_http_get): self.assertEqual(True, kw_args['headers']['Metadata']) @patch("azurelinuxagent.ga.update.restutil.http_get") - def test_get_bad_request(self, mock_http_get): + @patch("azurelinuxagent.common.protocol.util.ProtocolUtil") + def test_get_bad_request(self, mock_http_get, ProtocolUtil): mock_http_get.return_value = ResponseMock(status=restutil.httpclient.BAD_REQUEST) + ProtocolUtil = get_protocol_util() + ProtocolUtil.get_wireserver_endpoint = MagicMock() + test_subject = imds.ImdsClient() self.assertRaises(HttpError, test_subject.get_compute) @@ -234,8 +239,9 @@ def _setup_image_origin_assert(publisher, offer, sku, version): return compute_info.image_origin - def test_response_validation(self): - # expect extra "fallback" calls on error status codes + @patch("azurelinuxagent.common.protocol.util.ProtocolUtil") + def test_response_validation(self, ProtocolUtil): + ProtocolUtil().get_wireserver_endpoint.return_value = "foo.bar" # invalid json or empty response self._assert_validation(http_status_code=200, @@ -352,7 +358,7 @@ def _assert_validation(self, http_status_code, http_response, expected_valid, ex self.assertTrue('Metadata' in kw_args['headers']) self.assertEqual(True, kw_args['headers']['Metadata']) if expected_fallback: - self.assertEqual('http://168.63.129.16/metadata/instance/?api-version=2018-02-01', + self.assertEqual('http://foo.bar/metadata/instance/?api-version=2018-02-01', positional_args[0]) else: self.assertEqual('http://169.254.169.254/metadata/instance/?api-version=2018-02-01', From 1215b40b4759343e5393cb83e4bd28f2acbd8434 Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Fri, 20 Sep 2019 16:14:11 -0700 Subject: [PATCH 05/13] Add logging for IMDS connection failures --- azurelinuxagent/common/protocol/imds.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/azurelinuxagent/common/protocol/imds.py b/azurelinuxagent/common/protocol/imds.py index 9e0e8bf6ab..a71a6b6378 100644 --- a/azurelinuxagent/common/protocol/imds.py +++ b/azurelinuxagent/common/protocol/imds.py @@ -269,11 +269,13 @@ def get_compute(self): :rtype: ComputeInfo """ + # ensure we get a 200 resp = restutil.http_get(self.compute_url, headers=self._headers) - if restutil.request_failed(resp): + logger.warn("Unable to connect to primary IMDS endpoint at {0}", self.compute_url) resp = restutil.http_get(self.compute_url_backup, headers=self._headers) if restutil.request_failed(resp): + logger.warn("Unable to connect to backup IMDS endpoint at {0}", self.compute_url_backup) raise HttpError("{0} - GET: {1}".format(resp.status, self.compute_url)) data = resp.read() @@ -297,8 +299,10 @@ def validate(self): # ensure we get a 200 resp = restutil.http_get(self.instance_url, headers=self._health_headers) if restutil.request_failed(resp): + logger.warn("Unable to connect to primary IMDS endpoint at {0}", self.instance_url) resp = restutil.http_get(self.instance_url_backup, headers=self._health_headers) if restutil.request_failed(resp): + logger.warn("Unable to connect to backup IMDS endpoint at {0}", self.instance_url_backup) return False, "{0}".format(restutil.read_response_error(resp)) # ensure the response is valid json From 5f3214cb211a49171d7015886b14fd5208ac236c Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Fri, 20 Sep 2019 16:14:59 -0700 Subject: [PATCH 06/13] Bypass proxy for IMDS connections --- azurelinuxagent/common/protocol/imds.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/azurelinuxagent/common/protocol/imds.py b/azurelinuxagent/common/protocol/imds.py index a71a6b6378..aa5dc3bf76 100644 --- a/azurelinuxagent/common/protocol/imds.py +++ b/azurelinuxagent/common/protocol/imds.py @@ -270,10 +270,10 @@ def get_compute(self): """ # ensure we get a 200 - resp = restutil.http_get(self.compute_url, headers=self._headers) + resp = restutil.http_get(self.compute_url, headers=self._headers, use_proxy=False) if restutil.request_failed(resp): logger.warn("Unable to connect to primary IMDS endpoint at {0}", self.compute_url) - resp = restutil.http_get(self.compute_url_backup, headers=self._headers) + resp = restutil.http_get(self.compute_url_backup, headers=self._headers, use_proxy=False) if restutil.request_failed(resp): logger.warn("Unable to connect to backup IMDS endpoint at {0}", self.compute_url_backup) raise HttpError("{0} - GET: {1}".format(resp.status, self.compute_url)) @@ -297,10 +297,10 @@ def validate(self): """ # ensure we get a 200 - resp = restutil.http_get(self.instance_url, headers=self._health_headers) + resp = restutil.http_get(self.instance_url, headers=self._health_headers, use_proxy=False) if restutil.request_failed(resp): logger.warn("Unable to connect to primary IMDS endpoint at {0}", self.instance_url) - resp = restutil.http_get(self.instance_url_backup, headers=self._health_headers) + resp = restutil.http_get(self.instance_url_backup, headers=self._health_headers, use_proxy=False) if restutil.request_failed(resp): logger.warn("Unable to connect to backup IMDS endpoint at {0}", self.instance_url_backup) return False, "{0}".format(restutil.read_response_error(resp)) From 6dbd7f1ace4776858913d90c3555207efa08bf47 Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Mon, 23 Sep 2019 11:22:42 -0700 Subject: [PATCH 07/13] Fix fallback but need to fix unit tests --- azurelinuxagent/common/protocol/imds.py | 72 +++++++----- tests/protocol/test_imds.py | 149 +++++++++++++++++------- 2 files changed, 149 insertions(+), 72 deletions(-) diff --git a/azurelinuxagent/common/protocol/imds.py b/azurelinuxagent/common/protocol/imds.py index aa5dc3bf76..f0bf075b68 100644 --- a/azurelinuxagent/common/protocol/imds.py +++ b/azurelinuxagent/common/protocol/imds.py @@ -13,7 +13,7 @@ IMDS_ENDPOINT = '169.254.169.254' APIVERSION = '2018-02-01' -BASE_URI = "http://{0}/metadata/instance/{1}?api-version={2}" +BASE_URI = "http://{0}/metadata/{1}?api-version={2}" IMDS_IMAGE_ORIGIN_UNKNOWN = 0 IMDS_IMAGE_ORIGIN_CUSTOM = 1 @@ -243,23 +243,41 @@ def __init__(self, version=APIVERSION): 'User-Agent': restutil.HTTP_USER_AGENT_HEALTH, 'Metadata': True, } + self._regex_imds_ioerror = re.compile(r".*HTTP Failed. GET http://[^ ]+ -- IOError timed out -- [0-9]+ attempts made") self.protocol_util = get_protocol_util() - @property - def compute_url(self): - return BASE_URI.format(IMDS_ENDPOINT, 'compute', self._api_version) - - @property - def compute_url_backup(self): - return BASE_URI.format(self.protocol_util.get_wireserver_endpoint(), 'compute', self._api_version) + def _get_metadata_url(self, endpoint, resource_path): + return BASE_URI.format(endpoint, resource_path, self._api_version) - @property - def instance_url(self): - return BASE_URI.format(IMDS_ENDPOINT, '', self._api_version) + def get_metadata(self, resource_path, is_health): + """ + Get metadata from IMDS, falling back to Wireserver endpoint if necessary. - @property - def instance_url_backup(self): - return BASE_URI.format(self.protocol_util.get_wireserver_endpoint(), '', self._api_version) + :param str resource_path: path of IMDS resource + :param bool is_health: True if for health/heartbeat, False otherwise + :return: Tuple + is_request_success: True when connection succeeds, False otherwise + response: response from IMDS on request success, failure message otherwise + """ + headers = self._health_headers if is_health else self._headers + try: + url = self._get_metadata_url(IMDS_ENDPOINT, resource_path) + resp = restutil.http_get(url, headers=headers, use_proxy=False) + except HttpError as e: + logger.warn("Unable to connect to primary IMDS endpoint at {0}", url) + if not self._regex_imds_ioerror.match(str(e)): + raise e + try: + url = self._get_metadata_url(self.protocol_util.get_wireserver_endpoint(), resource_path) + resp = restutil.http_get(url, headers=headers, use_proxy=False) + except HttpError as e: + logger.warn("Unable to connect to backup IMDS endpoint at {0}", url) + if not self._regex_imds_ioerror.match(str(e)): + raise e + return False, "IMDS {0}: Unable to connect to endpoint".format(resource_path) + if restutil.request_failed(resp): + return False, "IMDS {0} [{1}]: {2}".format(resource_path, resp.status, restutil.read_response_error(resp)) + return True, resp.read() def get_compute(self): """ @@ -270,16 +288,11 @@ def get_compute(self): """ # ensure we get a 200 - resp = restutil.http_get(self.compute_url, headers=self._headers, use_proxy=False) - if restutil.request_failed(resp): - logger.warn("Unable to connect to primary IMDS endpoint at {0}", self.compute_url) - resp = restutil.http_get(self.compute_url_backup, headers=self._headers, use_proxy=False) - if restutil.request_failed(resp): - logger.warn("Unable to connect to backup IMDS endpoint at {0}", self.compute_url_backup) - raise HttpError("{0} - GET: {1}".format(resp.status, self.compute_url)) + success, resp = self.get_metadata('instance/compute', is_health=False) + if not success: + raise HttpError(resp) - data = resp.read() - data = json.loads(ustr(data, encoding="utf-8")) + data = json.loads(ustr(resp, encoding="utf-8")) compute_info = ComputeInfo() set_properties('compute', compute_info, data) @@ -297,18 +310,13 @@ def validate(self): """ # ensure we get a 200 - resp = restutil.http_get(self.instance_url, headers=self._health_headers, use_proxy=False) - if restutil.request_failed(resp): - logger.warn("Unable to connect to primary IMDS endpoint at {0}", self.instance_url) - resp = restutil.http_get(self.instance_url_backup, headers=self._health_headers, use_proxy=False) - if restutil.request_failed(resp): - logger.warn("Unable to connect to backup IMDS endpoint at {0}", self.instance_url_backup) - return False, "{0}".format(restutil.read_response_error(resp)) + success, resp = self.get_metadata('instance', is_health=True) + if not success: + return False, resp # ensure the response is valid json - data = resp.read() try: - json_data = json.loads(ustr(data, encoding="utf-8")) + json_data = json.loads(ustr(resp, encoding="utf-8")) except Exception as e: return False, "JSON parsing failed: {0}".format(ustr(e)) diff --git a/tests/protocol/test_imds.py b/tests/protocol/test_imds.py index 28328cb20c..0dd7847cf7 100644 --- a/tests/protocol/test_imds.py +++ b/tests/protocol/test_imds.py @@ -5,10 +5,9 @@ import azurelinuxagent.common.protocol.imds as imds from azurelinuxagent.common.exception import HttpError -from azurelinuxagent.common.future import ustr +from azurelinuxagent.common.future import ustr, httpclient from azurelinuxagent.common.protocol.restapi import set_properties from azurelinuxagent.common.utils import restutil -from azurelinuxagent.common.protocol.util import ProtocolUtil, get_protocol_util from tests.ga.test_update import ResponseMock from tests.tools import * @@ -46,13 +45,9 @@ def test_get(self, mock_http_get): self.assertEqual(True, kw_args['headers']['Metadata']) @patch("azurelinuxagent.ga.update.restutil.http_get") - @patch("azurelinuxagent.common.protocol.util.ProtocolUtil") - def test_get_bad_request(self, mock_http_get, ProtocolUtil): + def test_get_bad_request(self, mock_http_get): mock_http_get.return_value = ResponseMock(status=restutil.httpclient.BAD_REQUEST) - ProtocolUtil = get_protocol_util() - ProtocolUtil.get_wireserver_endpoint = MagicMock() - test_subject = imds.ImdsClient() self.assertRaises(HttpError, test_subject.get_compute) @@ -239,55 +234,106 @@ def _setup_image_origin_assert(publisher, offer, sku, version): return compute_info.image_origin - @patch("azurelinuxagent.common.protocol.util.ProtocolUtil") - def test_response_validation(self, ProtocolUtil): - ProtocolUtil().get_wireserver_endpoint.return_value = "foo.bar" - + def test_response_validation(self): # invalid json or empty response self._assert_validation(http_status_code=200, http_response='', expected_valid=False, - expected_response='JSON parsing failed', - expected_fallback=False) + expected_response='JSON parsing failed') self._assert_validation(http_status_code=200, http_response=None, expected_valid=False, - expected_response='JSON parsing failed', - expected_fallback=False) + expected_response='JSON parsing failed') self._assert_validation(http_status_code=200, http_response='{ bad json ', expected_valid=False, - expected_response='JSON parsing failed', - expected_fallback=False) + expected_response='JSON parsing failed') # 500 response self._assert_validation(http_status_code=500, http_response='error response', expected_valid=False, - expected_response='[HTTP Failed] [500: reason] error response', - expected_fallback=True) + expected_response='[HTTP Failed] [500: reason] error response') # 429 response self._assert_validation(http_status_code=429, http_response='server busy', expected_valid=False, - expected_response='[HTTP Failed] [429: reason] server busy', - expected_fallback=True) + expected_response='[HTTP Failed] [429: reason] server busy') # valid json self._assert_validation(http_status_code=200, http_response=self._imds_response('valid'), expected_valid=True, - expected_response='', - expected_fallback=False) + expected_response='') # unicode self._assert_validation(http_status_code=200, http_response=self._imds_response('unicode'), expected_valid=True, - expected_response='', - expected_fallback=False) + expected_response='') + + @patch("azurelinuxagent.common.protocol.util.ProtocolUtil") + @patch("azurelinuxagent.common.utils.restutil._http_request") + def test_endpoint_fallback(self, ProtocolUtil, mock_http_req): + # http error status codes are tested in test_response_validation, none of which + # should trigger a fallback. This is confirmed as _assert_validation will count + # http GET calls and enforces a single GET call (fallback would cause 2) and + # checks the url called. + + test_subject = imds.ImdsClient() + test_conn = MockHttpConn() + + ProtocolUtil().get_wireserver_endpoint.return_value = "foo.bar" + mock_http_req.side_effect = self.mock_http_req_no_imds_endpoint + + conn_success, response = test_subject.get_metadata('something', False) + #self.assertTrue(conn_success) + self.assertEqual('Mock response', response) + + # calls when both endpoints are unreachable + #with patch("azurelinuxagent.common.utils.restutil.http_get") as mock_http_get: + # mock_http_get.side_effect = self.mock_http_get_no_imds_endpoint + # with patch("azurelinuxagent.common.future.httpclient.HTTPConnection") as mock_conn: + # + # mock_conn.getresponse.side_effect = httpclient.NotConnected() + # self.assertRaises(HttpError, test_subject.validate) + # self.assertEqual(2, mock_conn.getresponse.call_count) + # + # positional_args, kw_args = mock_conn.getresponse.call_args + # self.assertTrue('User-Agent' in kw_args['headers']) + # self.assertEqual(restutil.HTTP_USER_AGENT_HEALTH, kw_args['headers']['User-Agent']) + # self.assertTrue('Metadata' in kw_args['headers']) + # self.assertEqual(True, kw_args['headers']['Metadata']) + # self.assertEqual('http://169.254.169.254/metadata/instance?api-version=2018-02-01', + # positional_args[0]) + # + # # calls when both endpoints are unreachable + # with patch("azurelinuxagent.common.future.httpclient.getresponse") as mock_http_get: + # mock_http_get.side_effect = httpclient.NotConnected() + # self.assertRaises(HttpError, test_subject.get_compute) + # self.assertEqual(2, mock_http_get.call_count) + # + # positional_args, kw_args = mock_http_get.call_args + + # self.assertTrue('User-Agent' in kw_args['headers']) + # self.assertEqual(restutil.HTTP_USER_AGENT, kw_args['headers']['User-Agent']) + # self.assertTrue('Metadata' in kw_args['headers']) + # self.assertEqual(True, kw_args['headers']['Metadata']) + # self.assertEqual('http://169.254.169.254/metadata/instance/compute?api-version=2018-02-01', + # positional_args[0]) + + def mock_http_req_no_imds_endpoint(self, *args, **kwargs): + resp = MagicMock() + resp.status = httpclient.OK + resp.read = 'Mock response' + + if "169.254.169.254" in kwargs['host']: + raise httpclient.NotConnected() + elif "foo.bar" in kwargs['host']: + return resp + raise Exception("Unexpected url called") def test_field_validation(self): # TODO: compute fields (#1249) @@ -308,8 +354,7 @@ def _assert_field(self, *fields): self._assert_validation(http_status_code=200, http_response=altered_response, expected_valid=False, - expected_response='Empty field: [{0}]'.format(fields[-1]), - expected_fallback=False) + expected_response='Empty field: [{0}]'.format(fields[-1])) # assert missing value self._update_field(response_obj, fields, None) @@ -317,8 +362,7 @@ def _assert_field(self, *fields): self._assert_validation(http_status_code=200, http_response=altered_response, expected_valid=False, - expected_response='Missing field: [{0}]'.format(fields[-1]), - expected_fallback=False) + expected_response='Missing field: [{0}]'.format(fields[-1])) def _update_field(self, obj, fields, val): if isinstance(obj, list): @@ -339,7 +383,7 @@ def _imds_response(f): with open(path, "rb") as fh: return fh.read() - def _assert_validation(self, http_status_code, http_response, expected_valid, expected_response, expected_fallback): + def _assert_validation(self, http_status_code, http_response, expected_valid, expected_response): test_subject = imds.ImdsClient() with patch("azurelinuxagent.common.utils.restutil.http_get") as mock_http_get: mock_http_get.return_value = ResponseMock(status=http_status_code, @@ -347,27 +391,52 @@ def _assert_validation(self, http_status_code, http_response, expected_valid, ex response=http_response) validate_response = test_subject.validate() - if expected_fallback: - self.assertEqual(2, mock_http_get.call_count) - else: - self.assertEqual(1, mock_http_get.call_count) + self.assertEqual(1, mock_http_get.call_count) positional_args, kw_args = mock_http_get.call_args self.assertTrue('User-Agent' in kw_args['headers']) self.assertEqual(restutil.HTTP_USER_AGENT_HEALTH, kw_args['headers']['User-Agent']) self.assertTrue('Metadata' in kw_args['headers']) self.assertEqual(True, kw_args['headers']['Metadata']) - if expected_fallback: - self.assertEqual('http://foo.bar/metadata/instance/?api-version=2018-02-01', - positional_args[0]) - else: - self.assertEqual('http://169.254.169.254/metadata/instance/?api-version=2018-02-01', - positional_args[0]) + self.assertEqual('http://169.254.169.254/metadata/instance?api-version=2018-02-01', + positional_args[0]) self.assertEqual(expected_valid, validate_response[0]) self.assertTrue(expected_response in validate_response[1], "Expected: '{0}', Actual: '{1}'" .format(expected_response, validate_response[1])) +class MockHttpConn: + def __init__(self): + self.method = 'method' + self.url = 'url' + self.body = 'body' + self.headers = 'headers' + + def request(self, method, url, body=None, headers=None): + self.method = method + self.url = url + self.body = body + self.headers = headers + raise Exception('inside request()') + + def getresponse(self): + raise Exception('inside getresponse({0})'.format(self.url)) + if "169.254.169.254" in self.url: + raise httpclient.RemoteDisconnected() + elif "foo.bar" in self.url: + resp = MagicMock() + resp.status = httpclient.OK + resp.reason = 'reason' + resp.response = 'Mock response' + return resp + else: + raise Exception("Unexpected url requested") + + @staticmethod + def _imds_response(f): + path = os.path.join(data_dir, "imds", "{0}.json".format(f)) + with open(path, "rb") as fh: + return fh.read() if __name__ == '__main__': unittest.main() From 3c3e80f6b40bddd9051a7745ee82cd5c79544578 Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Tue, 24 Sep 2019 21:44:01 -0700 Subject: [PATCH 08/13] Fix unit tests --- azurelinuxagent/common/protocol/imds.py | 23 +-- tests/protocol/test_imds.py | 185 +++++++++++++----------- 2 files changed, 111 insertions(+), 97 deletions(-) diff --git a/azurelinuxagent/common/protocol/imds.py b/azurelinuxagent/common/protocol/imds.py index f0bf075b68..dc0cbdb236 100644 --- a/azurelinuxagent/common/protocol/imds.py +++ b/azurelinuxagent/common/protocol/imds.py @@ -249,6 +249,11 @@ def __init__(self, version=APIVERSION): def _get_metadata_url(self, endpoint, resource_path): return BASE_URI.format(endpoint, resource_path, self._api_version) + def _http_get(self, endpoint, resource_path, is_health): + url = self._get_metadata_url(endpoint, resource_path) + headers = self._health_headers if is_health else self._headers + return restutil.http_get(url, headers=headers, use_proxy=False) + def get_metadata(self, resource_path, is_health): """ Get metadata from IMDS, falling back to Wireserver endpoint if necessary. @@ -259,24 +264,24 @@ def get_metadata(self, resource_path, is_health): is_request_success: True when connection succeeds, False otherwise response: response from IMDS on request success, failure message otherwise """ - headers = self._health_headers if is_health else self._headers + endpoint = IMDS_ENDPOINT try: - url = self._get_metadata_url(IMDS_ENDPOINT, resource_path) - resp = restutil.http_get(url, headers=headers, use_proxy=False) + resp = self._http_get(endpoint=endpoint, resource_path=resource_path, is_health=is_health) except HttpError as e: - logger.warn("Unable to connect to primary IMDS endpoint at {0}", url) + logger.warn("Unable to connect to primary IMDS endpoint {0}", endpoint) if not self._regex_imds_ioerror.match(str(e)): raise e + endpoint = self.protocol_util.get_wireserver_endpoint() try: - url = self._get_metadata_url(self.protocol_util.get_wireserver_endpoint(), resource_path) - resp = restutil.http_get(url, headers=headers, use_proxy=False) + resp = self._http_get(endpoint=endpoint, resource_path=resource_path, is_health=is_health) except HttpError as e: - logger.warn("Unable to connect to backup IMDS endpoint at {0}", url) + logger.warn("Unable to connect to backup IMDS endpoint {0}", endpoint) if not self._regex_imds_ioerror.match(str(e)): raise e - return False, "IMDS {0}: Unable to connect to endpoint".format(resource_path) + return False, "IMDS error in /metadata/{0}: Unable to connect to endpoint".format(resource_path) if restutil.request_failed(resp): - return False, "IMDS {0} [{1}]: {2}".format(resource_path, resp.status, restutil.read_response_error(resp)) + return False, "IMDS error in /metadata/{0}: {1}".format( + resource_path, restutil.read_response_error(resp)) return True, resp.read() def get_compute(self): diff --git a/tests/protocol/test_imds.py b/tests/protocol/test_imds.py index 0dd7847cf7..2935605696 100644 --- a/tests/protocol/test_imds.py +++ b/tests/protocol/test_imds.py @@ -274,67 +274,6 @@ def test_response_validation(self): expected_valid=True, expected_response='') - @patch("azurelinuxagent.common.protocol.util.ProtocolUtil") - @patch("azurelinuxagent.common.utils.restutil._http_request") - def test_endpoint_fallback(self, ProtocolUtil, mock_http_req): - # http error status codes are tested in test_response_validation, none of which - # should trigger a fallback. This is confirmed as _assert_validation will count - # http GET calls and enforces a single GET call (fallback would cause 2) and - # checks the url called. - - test_subject = imds.ImdsClient() - test_conn = MockHttpConn() - - ProtocolUtil().get_wireserver_endpoint.return_value = "foo.bar" - mock_http_req.side_effect = self.mock_http_req_no_imds_endpoint - - conn_success, response = test_subject.get_metadata('something', False) - #self.assertTrue(conn_success) - self.assertEqual('Mock response', response) - - # calls when both endpoints are unreachable - #with patch("azurelinuxagent.common.utils.restutil.http_get") as mock_http_get: - # mock_http_get.side_effect = self.mock_http_get_no_imds_endpoint - # with patch("azurelinuxagent.common.future.httpclient.HTTPConnection") as mock_conn: - # - # mock_conn.getresponse.side_effect = httpclient.NotConnected() - # self.assertRaises(HttpError, test_subject.validate) - # self.assertEqual(2, mock_conn.getresponse.call_count) - # - # positional_args, kw_args = mock_conn.getresponse.call_args - # self.assertTrue('User-Agent' in kw_args['headers']) - # self.assertEqual(restutil.HTTP_USER_AGENT_HEALTH, kw_args['headers']['User-Agent']) - # self.assertTrue('Metadata' in kw_args['headers']) - # self.assertEqual(True, kw_args['headers']['Metadata']) - # self.assertEqual('http://169.254.169.254/metadata/instance?api-version=2018-02-01', - # positional_args[0]) - # - # # calls when both endpoints are unreachable - # with patch("azurelinuxagent.common.future.httpclient.getresponse") as mock_http_get: - # mock_http_get.side_effect = httpclient.NotConnected() - # self.assertRaises(HttpError, test_subject.get_compute) - # self.assertEqual(2, mock_http_get.call_count) - # - # positional_args, kw_args = mock_http_get.call_args - - # self.assertTrue('User-Agent' in kw_args['headers']) - # self.assertEqual(restutil.HTTP_USER_AGENT, kw_args['headers']['User-Agent']) - # self.assertTrue('Metadata' in kw_args['headers']) - # self.assertEqual(True, kw_args['headers']['Metadata']) - # self.assertEqual('http://169.254.169.254/metadata/instance/compute?api-version=2018-02-01', - # positional_args[0]) - - def mock_http_req_no_imds_endpoint(self, *args, **kwargs): - resp = MagicMock() - resp.status = httpclient.OK - resp.read = 'Mock response' - - if "169.254.169.254" in kwargs['host']: - raise httpclient.NotConnected() - elif "foo.bar" in kwargs['host']: - return resp - raise Exception("Unexpected url called") - def test_field_validation(self): # TODO: compute fields (#1249) @@ -405,38 +344,108 @@ def _assert_validation(self, http_status_code, http_response, expected_valid, ex "Expected: '{0}', Actual: '{1}'" .format(expected_response, validate_response[1])) -class MockHttpConn: - def __init__(self): - self.method = 'method' - self.url = 'url' - self.body = 'body' - self.headers = 'headers' - - def request(self, method, url, body=None, headers=None): - self.method = method - self.url = url - self.body = body - self.headers = headers - raise Exception('inside request()') - - def getresponse(self): - raise Exception('inside getresponse({0})'.format(self.url)) - if "169.254.169.254" in self.url: - raise httpclient.RemoteDisconnected() - elif "foo.bar" in self.url: + @patch("azurelinuxagent.common.protocol.util.ProtocolUtil") + def test_endpoint_fallback(self, ProtocolUtil): + # http error status codes are tested in test_response_validation, none of which + # should trigger a fallback. This is confirmed as _assert_validation will count + # http GET calls and enforces a single GET call (fallback would cause 2) and + # checks the url called. + + test_subject = imds.ImdsClient() + ProtocolUtil().get_wireserver_endpoint.return_value = "foo.bar" + + # both endpoints unreachable + test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_both) + conn_success, response = test_subject.get_metadata('something', False) + self.assertFalse(conn_success) + self.assertEqual('IMDS error in /metadata/something: Unable to connect to endpoint', response) + self.assertEqual(2, test_subject._http_get.call_count) + + # primary IMDS endpoint unreachable and success in secondary IMDS endpoint + test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_ok) + conn_success, response = test_subject.get_metadata('something', False) + self.assertTrue(conn_success) + self.assertEqual('Mock success response', response) + self.assertEqual(2, test_subject._http_get.call_count) + + # primary IMDS endpoint unreachable and http error in secondary IMDS endpoint + test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_fail) + conn_success, response = test_subject.get_metadata('something', False) + self.assertFalse(conn_success) + self.assertEqual('IMDS error in /metadata/something: [HTTP Failed] [404: reason] Mock not found', response) + self.assertEqual(2, test_subject._http_get.call_count) + + # primary IMDS endpoint unreachable and http error in secondary IMDS endpoint + test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_fail) + conn_success, response = test_subject.get_metadata('something', False) + self.assertFalse(conn_success) + self.assertEqual('IMDS error in /metadata/something: [HTTP Failed] [404: reason] Mock not found', response) + self.assertEqual(2, test_subject._http_get.call_count) + + # primary IMDS endpoint with non-fallback HTTPError + test_subject._http_get = Mock(side_effect=self.mock_http_get_nonfallback_primary) + try: + test_subject.get_metadata('something', False) + self.assertTrue(False, 'Expected HttpError but no except raised') + except HttpError as e: + self.assertEqual('[HttpError] HTTP Failed. GET http://169.254.169.254/metadata/something -- IOError incomplete read -- 6 attempts made', str(e)) + self.assertEqual(1, test_subject._http_get.call_count) + except Exception as e: + self.assertTrue(False, 'Expected HttpError but got {0}'.format(str(e))) + + # primary IMDS endpoint unreachable and non-timeout HTTPError in secondary IMDS endpoint + test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_except) + try: + test_subject.get_metadata('something', False) + self.assertTrue(False, 'Expected HttpError but no except raised') + except HttpError as e: + self.assertEqual('[HttpError] HTTP Failed. GET http://foo.bar/metadata/something -- IOError incomplete read -- 6 attempts made', str(e)) + self.assertEqual(2, test_subject._http_get.call_count) + except Exception as e: + self.assertTrue(False, 'Expected HttpError but got {0}'.format(str(e))) + + def mock_http_get_unreachable_both(self, *_, **kwargs): + raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError timed out -- 6 attempts made" + .format(kwargs['endpoint'], kwargs['resource_path'])) + + def mock_http_get_unreachable_primary_with_ok(self, *_, **kwargs): + if "169.254.169.254" == kwargs['endpoint']: + raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError timed out -- 6 attempts made" + .format(kwargs['endpoint'], kwargs['resource_path'])) + elif "foo.bar" == kwargs['endpoint']: resp = MagicMock() resp.status = httpclient.OK + resp.read.return_value = 'Mock success response' + return resp + raise Exception("Unexpected endpoint called") + + def mock_http_get_unreachable_primary_with_fail(self, *_, **kwargs): + if "169.254.169.254" == kwargs['endpoint']: + raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError timed out -- 6 attempts made" + .format(kwargs['endpoint'], kwargs['resource_path'])) + elif "foo.bar" == kwargs['endpoint']: + resp = MagicMock() + resp.status = httpclient.NOT_FOUND resp.reason = 'reason' - resp.response = 'Mock response' + resp.read.return_value = 'Mock not found' return resp - else: - raise Exception("Unexpected url requested") + raise Exception("Unexpected endpoint called") + + def mock_http_get_nonfallback_primary(self, *_, **kwargs): + if "169.254.169.254" == kwargs['endpoint']: + raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError incomplete read -- 6 attempts made" + .format(kwargs['endpoint'], kwargs['resource_path'])) + raise Exception("Unexpected endpoint called") + + def mock_http_get_unreachable_primary_with_except(self, *_, **kwargs): + if "169.254.169.254" == kwargs['endpoint']: + raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError timed out -- 6 attempts made" + .format(kwargs['endpoint'], kwargs['resource_path'])) + elif "foo.bar" == kwargs['endpoint']: + raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError incomplete read -- 6 attempts made" + .format(kwargs['endpoint'], kwargs['resource_path'])) + raise Exception("Unexpected endpoint called") - @staticmethod - def _imds_response(f): - path = os.path.join(data_dir, "imds", "{0}.json".format(f)) - with open(path, "rb") as fh: - return fh.read() if __name__ == '__main__': unittest.main() From 5a153e30a50478c1bac3cd3118acf130bc392f74 Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Wed, 25 Sep 2019 11:07:32 -0700 Subject: [PATCH 09/13] Improve code coverage --- azurelinuxagent/common/protocol/imds.py | 8 +- tests/protocol/test_imds.py | 101 +++++++++++++----------- 2 files changed, 57 insertions(+), 52 deletions(-) diff --git a/azurelinuxagent/common/protocol/imds.py b/azurelinuxagent/common/protocol/imds.py index dc0cbdb236..6b5498627f 100644 --- a/azurelinuxagent/common/protocol/imds.py +++ b/azurelinuxagent/common/protocol/imds.py @@ -249,9 +249,8 @@ def __init__(self, version=APIVERSION): def _get_metadata_url(self, endpoint, resource_path): return BASE_URI.format(endpoint, resource_path, self._api_version) - def _http_get(self, endpoint, resource_path, is_health): + def _http_get(self, endpoint, resource_path, headers): url = self._get_metadata_url(endpoint, resource_path) - headers = self._health_headers if is_health else self._headers return restutil.http_get(url, headers=headers, use_proxy=False) def get_metadata(self, resource_path, is_health): @@ -264,16 +263,17 @@ def get_metadata(self, resource_path, is_health): is_request_success: True when connection succeeds, False otherwise response: response from IMDS on request success, failure message otherwise """ + headers = self._health_headers if is_health else self._headers endpoint = IMDS_ENDPOINT try: - resp = self._http_get(endpoint=endpoint, resource_path=resource_path, is_health=is_health) + resp = self._http_get(endpoint=endpoint, resource_path=resource_path, headers=headers) except HttpError as e: logger.warn("Unable to connect to primary IMDS endpoint {0}", endpoint) if not self._regex_imds_ioerror.match(str(e)): raise e endpoint = self.protocol_util.get_wireserver_endpoint() try: - resp = self._http_get(endpoint=endpoint, resource_path=resource_path, is_health=is_health) + resp = self._http_get(endpoint=endpoint, resource_path=resource_path, headers=headers) except HttpError as e: logger.warn("Unable to connect to backup IMDS endpoint {0}", endpoint) if not self._regex_imds_ioerror.match(str(e)): diff --git a/tests/protocol/test_imds.py b/tests/protocol/test_imds.py index 2935605696..5671ccfcdb 100644 --- a/tests/protocol/test_imds.py +++ b/tests/protocol/test_imds.py @@ -354,55 +354,60 @@ def test_endpoint_fallback(self, ProtocolUtil): test_subject = imds.ImdsClient() ProtocolUtil().get_wireserver_endpoint.return_value = "foo.bar" - # both endpoints unreachable - test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_both) - conn_success, response = test_subject.get_metadata('something', False) - self.assertFalse(conn_success) - self.assertEqual('IMDS error in /metadata/something: Unable to connect to endpoint', response) - self.assertEqual(2, test_subject._http_get.call_count) - - # primary IMDS endpoint unreachable and success in secondary IMDS endpoint - test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_ok) - conn_success, response = test_subject.get_metadata('something', False) - self.assertTrue(conn_success) - self.assertEqual('Mock success response', response) - self.assertEqual(2, test_subject._http_get.call_count) - - # primary IMDS endpoint unreachable and http error in secondary IMDS endpoint - test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_fail) - conn_success, response = test_subject.get_metadata('something', False) - self.assertFalse(conn_success) - self.assertEqual('IMDS error in /metadata/something: [HTTP Failed] [404: reason] Mock not found', response) - self.assertEqual(2, test_subject._http_get.call_count) - - # primary IMDS endpoint unreachable and http error in secondary IMDS endpoint - test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_fail) - conn_success, response = test_subject.get_metadata('something', False) - self.assertFalse(conn_success) - self.assertEqual('IMDS error in /metadata/something: [HTTP Failed] [404: reason] Mock not found', response) - self.assertEqual(2, test_subject._http_get.call_count) - - # primary IMDS endpoint with non-fallback HTTPError - test_subject._http_get = Mock(side_effect=self.mock_http_get_nonfallback_primary) - try: - test_subject.get_metadata('something', False) - self.assertTrue(False, 'Expected HttpError but no except raised') - except HttpError as e: - self.assertEqual('[HttpError] HTTP Failed. GET http://169.254.169.254/metadata/something -- IOError incomplete read -- 6 attempts made', str(e)) - self.assertEqual(1, test_subject._http_get.call_count) - except Exception as e: - self.assertTrue(False, 'Expected HttpError but got {0}'.format(str(e))) - - # primary IMDS endpoint unreachable and non-timeout HTTPError in secondary IMDS endpoint - test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_except) - try: - test_subject.get_metadata('something', False) - self.assertTrue(False, 'Expected HttpError but no except raised') - except HttpError as e: - self.assertEqual('[HttpError] HTTP Failed. GET http://foo.bar/metadata/something -- IOError incomplete read -- 6 attempts made', str(e)) + # ensure user-agent gets set correctly + for is_health, expected_useragent in [(False, 'guestagent'), (True, 'guestagent+health')]: + # set a different resource path for health query to make debugging unit test easier + resource_path = 'something/health' if is_health else 'something' + + # both endpoints unreachable + test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_both) + conn_success, response = test_subject.get_metadata(resource_path=resource_path, is_health=is_health) + self.assertFalse(conn_success) + self.assertEqual('IMDS error in /metadata/{0}: Unable to connect to endpoint'.format(resource_path), response) self.assertEqual(2, test_subject._http_get.call_count) - except Exception as e: - self.assertTrue(False, 'Expected HttpError but got {0}'.format(str(e))) + + # primary IMDS endpoint unreachable and success in secondary IMDS endpoint + test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_ok) + conn_success, response = test_subject.get_metadata(resource_path=resource_path, is_health=is_health) + self.assertTrue(conn_success) + self.assertEqual('Mock success response', response) + self.assertEqual(2, test_subject._http_get.call_count) + + # primary IMDS endpoint unreachable and http error in secondary IMDS endpoint + test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_fail) + conn_success, response = test_subject.get_metadata(resource_path=resource_path, is_health=is_health) + self.assertFalse(conn_success) + self.assertEqual('IMDS error in /metadata/{0}: [HTTP Failed] [404: reason] Mock not found'.format(resource_path), response) + self.assertEqual(2, test_subject._http_get.call_count) + + # primary IMDS endpoint unreachable and http error in secondary IMDS endpoint + test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_fail) + conn_success, response = test_subject.get_metadata(resource_path=resource_path, is_health=is_health) + self.assertFalse(conn_success) + self.assertEqual('IMDS error in /metadata/{0}: [HTTP Failed] [404: reason] Mock not found'.format(resource_path), response) + self.assertEqual(2, test_subject._http_get.call_count) + + # primary IMDS endpoint with non-fallback HTTPError + test_subject._http_get = Mock(side_effect=self.mock_http_get_nonfallback_primary) + try: + test_subject.get_metadata(resource_path=resource_path, is_health=is_health) + self.assertTrue(False, 'Expected HttpError but no except raised') + except HttpError as e: + self.assertEqual('[HttpError] HTTP Failed. GET http://169.254.169.254/metadata/{0} -- IOError incomplete read -- 6 attempts made'.format(resource_path), str(e)) + self.assertEqual(1, test_subject._http_get.call_count) + except Exception as e: + self.assertTrue(False, 'Expected HttpError but got {0}'.format(str(e))) + + # primary IMDS endpoint unreachable and non-timeout HTTPError in secondary IMDS endpoint + test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_except) + try: + test_subject.get_metadata(resource_path=resource_path, is_health=is_health) + self.assertTrue(False, 'Expected HttpError but no except raised') + except HttpError as e: + self.assertEqual('[HttpError] HTTP Failed. GET http://foo.bar/metadata/{0} -- IOError incomplete read -- 6 attempts made'.format(resource_path), str(e)) + self.assertEqual(2, test_subject._http_get.call_count) + except Exception as e: + self.assertTrue(False, 'Expected HttpError but got {0}'.format(str(e))) def mock_http_get_unreachable_both(self, *_, **kwargs): raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError timed out -- 6 attempts made" From aaf3554a8ab9e33c49c18f3be82c78cf0eabca6c Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Wed, 25 Sep 2019 11:34:49 -0700 Subject: [PATCH 10/13] Actually check user-agent --- tests/protocol/test_imds.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/protocol/test_imds.py b/tests/protocol/test_imds.py index 5671ccfcdb..0ce4d09b01 100644 --- a/tests/protocol/test_imds.py +++ b/tests/protocol/test_imds.py @@ -355,7 +355,7 @@ def test_endpoint_fallback(self, ProtocolUtil): ProtocolUtil().get_wireserver_endpoint.return_value = "foo.bar" # ensure user-agent gets set correctly - for is_health, expected_useragent in [(False, 'guestagent'), (True, 'guestagent+health')]: + for is_health, expected_useragent in [(False, restutil.HTTP_USER_AGENT), (True, restutil.HTTP_USER_AGENT_HEALTH)]: # set a different resource path for health query to make debugging unit test easier resource_path = 'something/health' if is_health else 'something' @@ -365,6 +365,9 @@ def test_endpoint_fallback(self, ProtocolUtil): self.assertFalse(conn_success) self.assertEqual('IMDS error in /metadata/{0}: Unable to connect to endpoint'.format(resource_path), response) self.assertEqual(2, test_subject._http_get.call_count) + for _, kwargs in test_subject._http_get.call_args_list: + self.assertTrue('User-Agent' in kwargs['headers']) + self.assertEqual(expected_useragent, kwargs['headers']['User-Agent']) # primary IMDS endpoint unreachable and success in secondary IMDS endpoint test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_ok) @@ -372,6 +375,9 @@ def test_endpoint_fallback(self, ProtocolUtil): self.assertTrue(conn_success) self.assertEqual('Mock success response', response) self.assertEqual(2, test_subject._http_get.call_count) + for _, kwargs in test_subject._http_get.call_args_list: + self.assertTrue('User-Agent' in kwargs['headers']) + self.assertEqual(expected_useragent, kwargs['headers']['User-Agent']) # primary IMDS endpoint unreachable and http error in secondary IMDS endpoint test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_fail) @@ -379,6 +385,9 @@ def test_endpoint_fallback(self, ProtocolUtil): self.assertFalse(conn_success) self.assertEqual('IMDS error in /metadata/{0}: [HTTP Failed] [404: reason] Mock not found'.format(resource_path), response) self.assertEqual(2, test_subject._http_get.call_count) + for _, kwargs in test_subject._http_get.call_args_list: + self.assertTrue('User-Agent' in kwargs['headers']) + self.assertEqual(expected_useragent, kwargs['headers']['User-Agent']) # primary IMDS endpoint unreachable and http error in secondary IMDS endpoint test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_fail) @@ -386,6 +395,9 @@ def test_endpoint_fallback(self, ProtocolUtil): self.assertFalse(conn_success) self.assertEqual('IMDS error in /metadata/{0}: [HTTP Failed] [404: reason] Mock not found'.format(resource_path), response) self.assertEqual(2, test_subject._http_get.call_count) + for _, kwargs in test_subject._http_get.call_args_list: + self.assertTrue('User-Agent' in kwargs['headers']) + self.assertEqual(expected_useragent, kwargs['headers']['User-Agent']) # primary IMDS endpoint with non-fallback HTTPError test_subject._http_get = Mock(side_effect=self.mock_http_get_nonfallback_primary) @@ -395,6 +407,9 @@ def test_endpoint_fallback(self, ProtocolUtil): except HttpError as e: self.assertEqual('[HttpError] HTTP Failed. GET http://169.254.169.254/metadata/{0} -- IOError incomplete read -- 6 attempts made'.format(resource_path), str(e)) self.assertEqual(1, test_subject._http_get.call_count) + for _, kwargs in test_subject._http_get.call_args_list: + self.assertTrue('User-Agent' in kwargs['headers']) + self.assertEqual(expected_useragent, kwargs['headers']['User-Agent']) except Exception as e: self.assertTrue(False, 'Expected HttpError but got {0}'.format(str(e))) @@ -406,6 +421,9 @@ def test_endpoint_fallback(self, ProtocolUtil): except HttpError as e: self.assertEqual('[HttpError] HTTP Failed. GET http://foo.bar/metadata/{0} -- IOError incomplete read -- 6 attempts made'.format(resource_path), str(e)) self.assertEqual(2, test_subject._http_get.call_count) + for _, kwargs in test_subject._http_get.call_args_list: + self.assertTrue('User-Agent' in kwargs['headers']) + self.assertEqual(expected_useragent, kwargs['headers']['User-Agent']) except Exception as e: self.assertTrue(False, 'Expected HttpError but got {0}'.format(str(e))) From e2a6a3e29697df7adbeffd89f0a2391cfbeeb5a6 Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Wed, 25 Sep 2019 11:51:07 -0700 Subject: [PATCH 11/13] Fix ws --- tests/protocol/test_imds.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/protocol/test_imds.py b/tests/protocol/test_imds.py index 0ce4d09b01..5e237d8e3e 100644 --- a/tests/protocol/test_imds.py +++ b/tests/protocol/test_imds.py @@ -381,7 +381,7 @@ def test_endpoint_fallback(self, ProtocolUtil): # primary IMDS endpoint unreachable and http error in secondary IMDS endpoint test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_fail) - conn_success, response = test_subject.get_metadata(resource_path=resource_path, is_health=is_health) + conn_success, response = test_subject.get_metadata(resource_path=resource_path, is_health=is_health) self.assertFalse(conn_success) self.assertEqual('IMDS error in /metadata/{0}: [HTTP Failed] [404: reason] Mock not found'.format(resource_path), response) self.assertEqual(2, test_subject._http_get.call_count) From f89b1ea0d9c69f05d1fe9a32a504df5ffd474324 Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Thu, 26 Sep 2019 10:53:42 -0700 Subject: [PATCH 12/13] PR feedback --- azurelinuxagent/common/protocol/imds.py | 12 ++++++------ tests/protocol/test_imds.py | 22 +++++++++++----------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/azurelinuxagent/common/protocol/imds.py b/azurelinuxagent/common/protocol/imds.py index 6b5498627f..061e4e4e6e 100644 --- a/azurelinuxagent/common/protocol/imds.py +++ b/azurelinuxagent/common/protocol/imds.py @@ -13,7 +13,7 @@ IMDS_ENDPOINT = '169.254.169.254' APIVERSION = '2018-02-01' -BASE_URI = "http://{0}/metadata/{1}?api-version={2}" +BASE_METADATA_URI = "http://{0}/metadata/{1}?api-version={2}" IMDS_IMAGE_ORIGIN_UNKNOWN = 0 IMDS_IMAGE_ORIGIN_CUSTOM = 1 @@ -244,10 +244,10 @@ def __init__(self, version=APIVERSION): 'Metadata': True, } self._regex_imds_ioerror = re.compile(r".*HTTP Failed. GET http://[^ ]+ -- IOError timed out -- [0-9]+ attempts made") - self.protocol_util = get_protocol_util() + self._protocol_util = get_protocol_util() def _get_metadata_url(self, endpoint, resource_path): - return BASE_URI.format(endpoint, resource_path, self._api_version) + return BASE_METADATA_URI.format(endpoint, resource_path, self._api_version) def _http_get(self, endpoint, resource_path, headers): url = self._get_metadata_url(endpoint, resource_path) @@ -270,14 +270,14 @@ def get_metadata(self, resource_path, is_health): except HttpError as e: logger.warn("Unable to connect to primary IMDS endpoint {0}", endpoint) if not self._regex_imds_ioerror.match(str(e)): - raise e - endpoint = self.protocol_util.get_wireserver_endpoint() + raise + endpoint = self._protocol_util.get_wireserver_endpoint() try: resp = self._http_get(endpoint=endpoint, resource_path=resource_path, headers=headers) except HttpError as e: logger.warn("Unable to connect to backup IMDS endpoint {0}", endpoint) if not self._regex_imds_ioerror.match(str(e)): - raise e + raise return False, "IMDS error in /metadata/{0}: Unable to connect to endpoint".format(resource_path) if restutil.request_failed(resp): return False, "IMDS error in /metadata/{0}: {1}".format( diff --git a/tests/protocol/test_imds.py b/tests/protocol/test_imds.py index 5e237d8e3e..adabcf40fa 100644 --- a/tests/protocol/test_imds.py +++ b/tests/protocol/test_imds.py @@ -360,7 +360,7 @@ def test_endpoint_fallback(self, ProtocolUtil): resource_path = 'something/health' if is_health else 'something' # both endpoints unreachable - test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_both) + test_subject._http_get = Mock(side_effect=self._mock_http_get_unreachable_both) conn_success, response = test_subject.get_metadata(resource_path=resource_path, is_health=is_health) self.assertFalse(conn_success) self.assertEqual('IMDS error in /metadata/{0}: Unable to connect to endpoint'.format(resource_path), response) @@ -370,7 +370,7 @@ def test_endpoint_fallback(self, ProtocolUtil): self.assertEqual(expected_useragent, kwargs['headers']['User-Agent']) # primary IMDS endpoint unreachable and success in secondary IMDS endpoint - test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_ok) + test_subject._http_get = Mock(side_effect=self._mock_http_get_unreachable_primary_with_ok) conn_success, response = test_subject.get_metadata(resource_path=resource_path, is_health=is_health) self.assertTrue(conn_success) self.assertEqual('Mock success response', response) @@ -380,7 +380,7 @@ def test_endpoint_fallback(self, ProtocolUtil): self.assertEqual(expected_useragent, kwargs['headers']['User-Agent']) # primary IMDS endpoint unreachable and http error in secondary IMDS endpoint - test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_fail) + test_subject._http_get = Mock(side_effect=self._mock_http_get_unreachable_primary_with_fail) conn_success, response = test_subject.get_metadata(resource_path=resource_path, is_health=is_health) self.assertFalse(conn_success) self.assertEqual('IMDS error in /metadata/{0}: [HTTP Failed] [404: reason] Mock not found'.format(resource_path), response) @@ -390,7 +390,7 @@ def test_endpoint_fallback(self, ProtocolUtil): self.assertEqual(expected_useragent, kwargs['headers']['User-Agent']) # primary IMDS endpoint unreachable and http error in secondary IMDS endpoint - test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_fail) + test_subject._http_get = Mock(side_effect=self._mock_http_get_unreachable_primary_with_fail) conn_success, response = test_subject.get_metadata(resource_path=resource_path, is_health=is_health) self.assertFalse(conn_success) self.assertEqual('IMDS error in /metadata/{0}: [HTTP Failed] [404: reason] Mock not found'.format(resource_path), response) @@ -400,7 +400,7 @@ def test_endpoint_fallback(self, ProtocolUtil): self.assertEqual(expected_useragent, kwargs['headers']['User-Agent']) # primary IMDS endpoint with non-fallback HTTPError - test_subject._http_get = Mock(side_effect=self.mock_http_get_nonfallback_primary) + test_subject._http_get = Mock(side_effect=self._mock_http_get_nonfallback_primary) try: test_subject.get_metadata(resource_path=resource_path, is_health=is_health) self.assertTrue(False, 'Expected HttpError but no except raised') @@ -414,7 +414,7 @@ def test_endpoint_fallback(self, ProtocolUtil): self.assertTrue(False, 'Expected HttpError but got {0}'.format(str(e))) # primary IMDS endpoint unreachable and non-timeout HTTPError in secondary IMDS endpoint - test_subject._http_get = Mock(side_effect=self.mock_http_get_unreachable_primary_with_except) + test_subject._http_get = Mock(side_effect=self._mock_http_get_unreachable_primary_with_except) try: test_subject.get_metadata(resource_path=resource_path, is_health=is_health) self.assertTrue(False, 'Expected HttpError but no except raised') @@ -427,11 +427,11 @@ def test_endpoint_fallback(self, ProtocolUtil): except Exception as e: self.assertTrue(False, 'Expected HttpError but got {0}'.format(str(e))) - def mock_http_get_unreachable_both(self, *_, **kwargs): + def _mock_http_get_unreachable_both(self, *_, **kwargs): raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError timed out -- 6 attempts made" .format(kwargs['endpoint'], kwargs['resource_path'])) - def mock_http_get_unreachable_primary_with_ok(self, *_, **kwargs): + def _mock_http_get_unreachable_primary_with_ok(self, *_, **kwargs): if "169.254.169.254" == kwargs['endpoint']: raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError timed out -- 6 attempts made" .format(kwargs['endpoint'], kwargs['resource_path'])) @@ -442,7 +442,7 @@ def mock_http_get_unreachable_primary_with_ok(self, *_, **kwargs): return resp raise Exception("Unexpected endpoint called") - def mock_http_get_unreachable_primary_with_fail(self, *_, **kwargs): + def _mock_http_get_unreachable_primary_with_fail(self, *_, **kwargs): if "169.254.169.254" == kwargs['endpoint']: raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError timed out -- 6 attempts made" .format(kwargs['endpoint'], kwargs['resource_path'])) @@ -454,13 +454,13 @@ def mock_http_get_unreachable_primary_with_fail(self, *_, **kwargs): return resp raise Exception("Unexpected endpoint called") - def mock_http_get_nonfallback_primary(self, *_, **kwargs): + def _mock_http_get_nonfallback_primary(self, *_, **kwargs): if "169.254.169.254" == kwargs['endpoint']: raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError incomplete read -- 6 attempts made" .format(kwargs['endpoint'], kwargs['resource_path'])) raise Exception("Unexpected endpoint called") - def mock_http_get_unreachable_primary_with_except(self, *_, **kwargs): + def _mock_http_get_unreachable_primary_with_except(self, *_, **kwargs): if "169.254.169.254" == kwargs['endpoint']: raise HttpError("HTTP Failed. GET http://{0}/metadata/{1} -- IOError timed out -- 6 attempts made" .format(kwargs['endpoint'], kwargs['resource_path'])) From 35c845107587cfbf9dd159206da42e73e57933ab Mon Sep 17 00:00:00 2001 From: Jack Williams Date: Wed, 2 Oct 2019 10:13:42 -0700 Subject: [PATCH 13/13] Make log messages periodic --- azurelinuxagent/common/protocol/imds.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/azurelinuxagent/common/protocol/imds.py b/azurelinuxagent/common/protocol/imds.py index 061e4e4e6e..3e03a077f0 100644 --- a/azurelinuxagent/common/protocol/imds.py +++ b/azurelinuxagent/common/protocol/imds.py @@ -228,7 +228,8 @@ def image_origin(self): return IMDS_IMAGE_ORIGIN_PLATFORM except Exception as e: - logger.warn("Could not determine the image origin from IMDS: {0}", str(e)) + logger.periodic_warn(logger.EVERY_FIFTEEN_MINUTES, + "[PERIODIC] Could not determine the image origin from IMDS: {0}".format(str(e))) return IMDS_IMAGE_ORIGIN_UNKNOWN @@ -268,14 +269,16 @@ def get_metadata(self, resource_path, is_health): try: resp = self._http_get(endpoint=endpoint, resource_path=resource_path, headers=headers) except HttpError as e: - logger.warn("Unable to connect to primary IMDS endpoint {0}", endpoint) + logger.periodic_warn(logger.EVERY_FIFTEEN_MINUTES, + "[PERIODIC] Unable to connect to primary IMDS endpoint {0}".format(endpoint)) if not self._regex_imds_ioerror.match(str(e)): raise endpoint = self._protocol_util.get_wireserver_endpoint() try: resp = self._http_get(endpoint=endpoint, resource_path=resource_path, headers=headers) except HttpError as e: - logger.warn("Unable to connect to backup IMDS endpoint {0}", endpoint) + logger.periodic_warn(logger.EVERY_FIFTEEN_MINUTES, + "[PERIODIC] Unable to connect to backup IMDS endpoint {0}".format(endpoint)) if not self._regex_imds_ioerror.match(str(e)): raise return False, "IMDS error in /metadata/{0}: Unable to connect to endpoint".format(resource_path)