From 7c3a4e7e528bcc39b622496415b4c5d039bc20d3 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Tue, 22 Aug 2017 18:14:24 -0700 Subject: [PATCH 1/6] Use metadata server to detect GKE environment --- logging/google/cloud/logging/_helpers.py | 34 ++++++++++++ logging/google/cloud/logging/client.py | 9 ++-- logging/tests/unit/test__helpers.py | 67 ++++++++++++++++++++++++ logging/tests/unit/test_client.py | 16 +++--- 4 files changed, 116 insertions(+), 10 deletions(-) diff --git a/logging/google/cloud/logging/_helpers.py b/logging/google/cloud/logging/_helpers.py index 8e17a9538e76..35d70dd68115 100644 --- a/logging/google/cloud/logging/_helpers.py +++ b/logging/google/cloud/logging/_helpers.py @@ -14,11 +14,17 @@ """Common logging helpers.""" +import requests from google.cloud.logging.entries import ProtobufEntry from google.cloud.logging.entries import StructEntry from google.cloud.logging.entries import TextEntry +METADATA_URL = 'http://metadata/computeMetadata/v1/' +METADATA_HEADERS = { + 'Metadata-Flavor': 'Google' +} + def entry_from_resource(resource, client, loggers): """Detect correct entry type from resource and instantiate. @@ -46,3 +52,31 @@ def entry_from_resource(resource, client, loggers): return ProtobufEntry.from_api_repr(resource, client, loggers) raise ValueError('Cannot parse log entry resource.') + + +def retrieve_metadata_server(metadata_key): + """Retrieve the metadata key in the metadata server. + + See: https://cloud.google.com/compute/docs/storing-retrieving-metadata + + :type metadata_key: str + :param metadata_key: Key of the metadata which will form the url. + + :rtype: str + :returns: The value of the metadata key returned by the metadata server. + """ + url = METADATA_URL + metadata_key + + try: + response = requests.get(url, headers=METADATA_HEADERS) + + if response.status_code == requests.codes.ok: + return response.text + else: + return None + except requests.exceptions.RequestException: + # Ignore the exception, connection failed means the attribute does not + # exist in the metadata server. + pass + + return None diff --git a/logging/google/cloud/logging/client.py b/logging/google/cloud/logging/client.py index 23ec84ec67d0..ae20dd48fcf6 100644 --- a/logging/google/cloud/logging/client.py +++ b/logging/google/cloud/logging/client.py @@ -31,6 +31,7 @@ from google.cloud.client import ClientWithProject from google.cloud.environment_vars import DISABLE_GRPC +from google.cloud.logging._helpers import retrieve_metadata_server from google.cloud.logging._http import Connection from google.cloud.logging._http import _LoggingAPI as JSONLoggingAPI from google.cloud.logging._http import _MetricsAPI as JSONMetricsAPI @@ -55,8 +56,8 @@ _APPENGINE_FLEXIBLE_ENV_FLEX = 'GAE_INSTANCE' """Environment variable set in App Engine when env:flex is set.""" -_CONTAINER_ENGINE_ENV = 'KUBERNETES_SERVICE' -"""Environment variable set in a Google Container Engine environment.""" +_GKE_CLUSTER_NAME = 'instance/attributes/cluster-name' +"""Attribute in metadata server when in GKE environment.""" class Client(ClientWithProject): @@ -301,10 +302,12 @@ def get_default_handler(self): :rtype: :class:`logging.Handler` :returns: The default log handler based on the environment """ + gke_cluster_name = retrieve_metadata_server(_GKE_CLUSTER_NAME) + if (_APPENGINE_FLEXIBLE_ENV_VM in os.environ or _APPENGINE_FLEXIBLE_ENV_FLEX in os.environ): return AppEngineHandler(self) - elif _CONTAINER_ENGINE_ENV in os.environ: + elif gke_cluster_name is not None: return ContainerEngineHandler() else: return CloudLoggingHandler(self) diff --git a/logging/tests/unit/test__helpers.py b/logging/tests/unit/test__helpers.py index 7cc2d392514c..388e0d9f394a 100644 --- a/logging/tests/unit/test__helpers.py +++ b/logging/tests/unit/test__helpers.py @@ -15,6 +15,8 @@ import unittest +import mock + class Test_entry_from_resource(unittest.TestCase): @@ -53,6 +55,71 @@ def test_proto_payload(self): self._payload_helper('protoPayload', 'ProtobufEntry') +class Test_retrieve_metadata_server(unittest.TestCase): + + @staticmethod + def _call_fut(metadata_key): + from google.cloud.logging._helpers import retrieve_metadata_server + + return retrieve_metadata_server(metadata_key) + + def test_metadata_exists(self): + status_code_ok = 200 + response_text = 'my-gke-cluster' + metadata_key = 'test_key' + + response_mock = mock.Mock() + response_mock.status_code = status_code_ok + response_mock.text = response_text + + requests_mock = mock.Mock() + requests_mock.get.return_value = response_mock + requests_mock.codes.ok = status_code_ok + + patch = mock.patch( + 'google.cloud.logging._helpers.requests', + requests_mock) + + with patch: + metadata = self._call_fut(metadata_key) + + self.assertEqual(metadata, response_text) + + def test_metadata_does_not_exist(self): + status_code_ok = 200 + status_code_not_found = 404 + metadata_key = 'test_key' + + response_mock = mock.Mock() + response_mock.status_code = status_code_not_found + + requests_mock = mock.Mock() + requests_mock.get.return_value = response_mock + requests_mock.codes.ok = status_code_ok + + patch = mock.patch( + 'google.cloud.logging._helpers.requests', + requests_mock) + + with patch: + metadata = self._call_fut(metadata_key) + + self.assertIsNone(metadata) + + def test_request_exception(self): + metadata_key = 'test_url_cannot_connect' + metadata_url = 'invalid_url' + + patch = mock.patch( + 'google.cloud.logging._helpers.METADATA_URL', + new=metadata_url) + + with patch: + metadata = self._call_fut(metadata_key) + + self.assertIsNone(metadata) + + class EntryMock(object): def __init__(self): diff --git a/logging/tests/unit/test_client.py b/logging/tests/unit/test_client.py index 37bfc5c18214..bb16e85c7ae5 100644 --- a/logging/tests/unit/test_client.py +++ b/logging/tests/unit/test_client.py @@ -581,16 +581,18 @@ def test_get_default_handler_app_engine(self): self.assertIsInstance(handler, AppEngineHandler) def test_get_default_handler_container_engine(self): - import os - from google.cloud._testing import _Monkey - from google.cloud.logging.client import _CONTAINER_ENGINE_ENV from google.cloud.logging.handlers import ContainerEngineHandler - client = self._make_one(project=self.PROJECT, - credentials=_make_credentials(), - _use_grpc=False) + client = self._make_one( + project=self.PROJECT, + credentials=_make_credentials(), + _use_grpc=False) + + patch = mock.patch( + 'google.cloud.logging.client.retrieve_metadata_server', + return_value='test-gke-cluster') - with _Monkey(os, environ={_CONTAINER_ENGINE_ENV: 'True'}): + with patch: handler = client.get_default_handler() self.assertIsInstance(handler, ContainerEngineHandler) From 70a4ffbd9d3eb2617b86df51445500660fdd203d Mon Sep 17 00:00:00 2001 From: Angela Li Date: Wed, 23 Aug 2017 12:24:22 -0700 Subject: [PATCH 2/6] Use raise_for_status to throw exception when request failed --- logging/google/cloud/logging/_helpers.py | 12 +++++++----- logging/tests/unit/test__helpers.py | 22 +++++++++++++++------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/logging/google/cloud/logging/_helpers.py b/logging/google/cloud/logging/_helpers.py index 35d70dd68115..c3f97a73207e 100644 --- a/logging/google/cloud/logging/_helpers.py +++ b/logging/google/cloud/logging/_helpers.py @@ -15,6 +15,7 @@ """Common logging helpers.""" import requests +from requests import exceptions from google.cloud.logging.entries import ProtobufEntry from google.cloud.logging.entries import StructEntry @@ -70,11 +71,12 @@ def retrieve_metadata_server(metadata_key): try: response = requests.get(url, headers=METADATA_HEADERS) - if response.status_code == requests.codes.ok: - return response.text - else: - return None - except requests.exceptions.RequestException: + # Exception will raise if the HTTP request returned an unsuccessful + # status code. + response.raise_for_status() + + return response.text + except (exceptions.RequestException, exceptions.HTTPError): # Ignore the exception, connection failed means the attribute does not # exist in the metadata server. pass diff --git a/logging/tests/unit/test__helpers.py b/logging/tests/unit/test__helpers.py index 388e0d9f394a..094498c3555e 100644 --- a/logging/tests/unit/test__helpers.py +++ b/logging/tests/unit/test__helpers.py @@ -68,13 +68,11 @@ def test_metadata_exists(self): response_text = 'my-gke-cluster' metadata_key = 'test_key' - response_mock = mock.Mock() - response_mock.status_code = status_code_ok + response_mock = ResponseMock(status_code=status_code_ok) response_mock.text = response_text requests_mock = mock.Mock() requests_mock.get.return_value = response_mock - requests_mock.codes.ok = status_code_ok patch = mock.patch( 'google.cloud.logging._helpers.requests', @@ -86,16 +84,13 @@ def test_metadata_exists(self): self.assertEqual(metadata, response_text) def test_metadata_does_not_exist(self): - status_code_ok = 200 status_code_not_found = 404 metadata_key = 'test_key' - response_mock = mock.Mock() - response_mock.status_code = status_code_not_found + response_mock = ResponseMock(status_code=status_code_not_found) requests_mock = mock.Mock() requests_mock.get.return_value = response_mock - requests_mock.codes.ok = status_code_ok patch = mock.patch( 'google.cloud.logging._helpers.requests', @@ -129,3 +124,16 @@ def __init__(self): def from_api_repr(self, resource, client, loggers): self.called = (resource, client, loggers) return self.sentinel + + +class ResponseMock(object): + + def __init__(self, status_code, text='test_response_text'): + self.status_code = status_code + self.text = text + + def raise_for_status(self): + from requests.exceptions import HTTPError + + if self.status_code >= 400: + raise HTTPError('test_error_msg', response=self) From dacba18dd88d7df3e47becaa08d282c23f125367 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Wed, 23 Aug 2017 13:00:36 -0700 Subject: [PATCH 3/6] Only return response when status_code is 200 --- logging/google/cloud/logging/_helpers.py | 4 +++- logging/tests/unit/test__helpers.py | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/logging/google/cloud/logging/_helpers.py b/logging/google/cloud/logging/_helpers.py index c3f97a73207e..8a19065de013 100644 --- a/logging/google/cloud/logging/_helpers.py +++ b/logging/google/cloud/logging/_helpers.py @@ -73,9 +73,11 @@ def retrieve_metadata_server(metadata_key): # Exception will raise if the HTTP request returned an unsuccessful # status code. + if response.status_code == requests.codes.ok: + return response.text + response.raise_for_status() - return response.text except (exceptions.RequestException, exceptions.HTTPError): # Ignore the exception, connection failed means the attribute does not # exist in the metadata server. diff --git a/logging/tests/unit/test__helpers.py b/logging/tests/unit/test__helpers.py index 094498c3555e..1e2f943229f7 100644 --- a/logging/tests/unit/test__helpers.py +++ b/logging/tests/unit/test__helpers.py @@ -73,6 +73,7 @@ def test_metadata_exists(self): requests_mock = mock.Mock() requests_mock.get.return_value = response_mock + requests_mock.codes.ok = status_code_ok patch = mock.patch( 'google.cloud.logging._helpers.requests', @@ -84,6 +85,7 @@ def test_metadata_exists(self): self.assertEqual(metadata, response_text) def test_metadata_does_not_exist(self): + status_code_ok = 200 status_code_not_found = 404 metadata_key = 'test_key' @@ -91,6 +93,7 @@ def test_metadata_does_not_exist(self): requests_mock = mock.Mock() requests_mock.get.return_value = response_mock + requests_mock.codes.ok = status_code_ok patch = mock.patch( 'google.cloud.logging._helpers.requests', From 67a48dcdc19d6a3a285bd69c0e696a8beb8c53c8 Mon Sep 17 00:00:00 2001 From: Angela Li Date: Wed, 23 Aug 2017 13:05:53 -0700 Subject: [PATCH 4/6] Fix stuff --- logging/google/cloud/logging/_helpers.py | 5 +---- logging/tests/unit/test__helpers.py | 6 ------ 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/logging/google/cloud/logging/_helpers.py b/logging/google/cloud/logging/_helpers.py index 8a19065de013..311b614c6bad 100644 --- a/logging/google/cloud/logging/_helpers.py +++ b/logging/google/cloud/logging/_helpers.py @@ -15,7 +15,6 @@ """Common logging helpers.""" import requests -from requests import exceptions from google.cloud.logging.entries import ProtobufEntry from google.cloud.logging.entries import StructEntry @@ -76,9 +75,7 @@ def retrieve_metadata_server(metadata_key): if response.status_code == requests.codes.ok: return response.text - response.raise_for_status() - - except (exceptions.RequestException, exceptions.HTTPError): + except requests.exceptions.RequestException: # Ignore the exception, connection failed means the attribute does not # exist in the metadata server. pass diff --git a/logging/tests/unit/test__helpers.py b/logging/tests/unit/test__helpers.py index 1e2f943229f7..5a2d9efd565e 100644 --- a/logging/tests/unit/test__helpers.py +++ b/logging/tests/unit/test__helpers.py @@ -134,9 +134,3 @@ class ResponseMock(object): def __init__(self, status_code, text='test_response_text'): self.status_code = status_code self.text = text - - def raise_for_status(self): - from requests.exceptions import HTTPError - - if self.status_code >= 400: - raise HTTPError('test_error_msg', response=self) From b79d6b945f7f0bf8eeb6c9d9b838355a74bf5dca Mon Sep 17 00:00:00 2001 From: Angela Li Date: Wed, 23 Aug 2017 13:17:55 -0700 Subject: [PATCH 5/6] Remove unused comments --- logging/google/cloud/logging/_helpers.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/logging/google/cloud/logging/_helpers.py b/logging/google/cloud/logging/_helpers.py index 311b614c6bad..b8876c428988 100644 --- a/logging/google/cloud/logging/_helpers.py +++ b/logging/google/cloud/logging/_helpers.py @@ -70,8 +70,6 @@ def retrieve_metadata_server(metadata_key): try: response = requests.get(url, headers=METADATA_HEADERS) - # Exception will raise if the HTTP request returned an unsuccessful - # status code. if response.status_code == requests.codes.ok: return response.text From 2b430721b4fdfbe2a5a12167e97d4837beab13be Mon Sep 17 00:00:00 2001 From: Angela Li Date: Wed, 23 Aug 2017 14:32:26 -0700 Subject: [PATCH 6/6] Address comments --- logging/google/cloud/logging/_helpers.py | 4 +++- logging/tests/unit/test__helpers.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/logging/google/cloud/logging/_helpers.py b/logging/google/cloud/logging/_helpers.py index b8876c428988..c7fab41bc4e8 100644 --- a/logging/google/cloud/logging/_helpers.py +++ b/logging/google/cloud/logging/_helpers.py @@ -60,7 +60,9 @@ def retrieve_metadata_server(metadata_key): See: https://cloud.google.com/compute/docs/storing-retrieving-metadata :type metadata_key: str - :param metadata_key: Key of the metadata which will form the url. + :param metadata_key: Key of the metadata which will form the url. You can + also supply query parameters after the metadata key. + e.g. "tags?alt=json" :rtype: str :returns: The value of the metadata key returned by the metadata server. diff --git a/logging/tests/unit/test__helpers.py b/logging/tests/unit/test__helpers.py index 5a2d9efd565e..93532eed0c05 100644 --- a/logging/tests/unit/test__helpers.py +++ b/logging/tests/unit/test__helpers.py @@ -106,7 +106,7 @@ def test_metadata_does_not_exist(self): def test_request_exception(self): metadata_key = 'test_url_cannot_connect' - metadata_url = 'invalid_url' + metadata_url = 'http://metadata.invalid/' patch = mock.patch( 'google.cloud.logging._helpers.METADATA_URL',