From c29bae53af002c99f8f86126a5322f13d12a6f6f Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Sat, 3 Sep 2016 09:47:44 -0700 Subject: [PATCH 1/8] Adding ability to disable gRPC in Datastore. Also renaming GOOGLE_CLOUD_DISABLE_GAX to GOOGLE_CLOUD_DISABLE_GRPC to make it uniform across all APIs. --- docs/logging-usage.rst | 4 ++-- docs/pubsub-usage.rst | 4 ++-- google/cloud/datastore/connection.py | 6 +++++- google/cloud/environment_vars.py | 7 +++++++ google/cloud/logging/client.py | 3 ++- google/cloud/pubsub/client.py | 3 ++- unit_tests/datastore/test_connection.py | 8 ++++---- 7 files changed, 24 insertions(+), 11 deletions(-) diff --git a/docs/logging-usage.rst b/docs/logging-usage.rst index 7acf6d643ab8..4736924eae04 100644 --- a/docs/logging-usage.rst +++ b/docs/logging-usage.rst @@ -16,8 +16,8 @@ Authentication and Configuration - The library now enables the ``gRPC`` transport for the logging API by default, assuming that the required dependencies are installed and importable. To *disable* this transport, set the - :envvar:`GOOGLE_CLOUD_DISABLE_GAX` environment variable to a non-empty string, - e.g.: ``$ export GOOGLE_CLOUD_DISABLE_GAX=1``. + :envvar:`GOOGLE_CLOUD_DISABLE_GRPC` environment variable to a + non-empty string, e.g.: ``$ export GOOGLE_CLOUD_DISABLE_GRPC=true``. - After configuring your environment, create a :class:`Client ` diff --git a/docs/pubsub-usage.rst b/docs/pubsub-usage.rst index 00cedfb122ae..431f2784d050 100644 --- a/docs/pubsub-usage.rst +++ b/docs/pubsub-usage.rst @@ -15,8 +15,8 @@ Authentication / Configuration - The library now enables the ``gRPC`` transport for the pubsub API by default, assuming that the required dependencies are installed and importable. To *disable* this transport, set the - :envvar:`GOOGLE_CLOUD_DISABLE_GAX` environment variable to a non-empty string, - e.g.: ``$ export GOOGLE_CLOUD_DISABLE_GAX=1``. + :envvar:`GOOGLE_CLOUD_DISABLE_GRPC` environment variable to a + non-empty string, e.g.: ``$ export GOOGLE_CLOUD_DISABLE_GRPC=true``. - :class:`Client ` objects hold both a ``project`` and an authenticated connection to the PubSub service. diff --git a/google/cloud/datastore/connection.py b/google/cloud/datastore/connection.py index 66988a4c17ff..28a66b5076ea 100644 --- a/google/cloud/datastore/connection.py +++ b/google/cloud/datastore/connection.py @@ -20,6 +20,7 @@ from google.cloud._helpers import make_stub from google.cloud import connection as connection_module +from google.cloud.environment_vars import DISABLE_GRPC from google.cloud.environment_vars import GCD_HOST from google.cloud.exceptions import Conflict from google.cloud.exceptions import make_exception @@ -44,6 +45,9 @@ DATASTORE_API_PORT = 443 """Datastore API request port.""" +_DISABLE_GRPC = os.getenv(DISABLE_GRPC, False) +_USE_GRPC = _HAVE_GRPC and not _DISABLE_GRPC + class _DatastoreAPIOverHttp(object): """Helper mapping datastore API methods. @@ -381,7 +385,7 @@ def __init__(self, credentials=None, http=None, api_base_url=None): except KeyError: api_base_url = self.__class__.API_BASE_URL self.api_base_url = api_base_url - if _HAVE_GRPC: + if _USE_GRPC: self._datastore_api = _DatastoreAPIOverGRPC(self) else: self._datastore_api = _DatastoreAPIOverHttp(self) diff --git a/google/cloud/environment_vars.py b/google/cloud/environment_vars.py index 8beb66da24ac..599aa968d2af 100644 --- a/google/cloud/environment_vars.py +++ b/google/cloud/environment_vars.py @@ -35,3 +35,10 @@ CREDENTIALS = 'GOOGLE_APPLICATION_CREDENTIALS' """Environment variable defining location of Google credentials.""" + +DISABLE_GRPC = 'GOOGLE_CLOUD_DISABLE_GRPC' +"""Environment variable acting as flag to disable gRPC. + +To be used for APIs where both an HTTP and gRPC implementation +exist. +""" diff --git a/google/cloud/logging/client.py b/google/cloud/logging/client.py index 553f8929fd27..f16fe333aa4a 100644 --- a/google/cloud/logging/client.py +++ b/google/cloud/logging/client.py @@ -35,6 +35,7 @@ _HAVE_GAX = True from google.cloud.client import JSONClient +from google.cloud.environment_vars import DISABLE_GRPC from google.cloud.logging.connection import Connection from google.cloud.logging.connection import _LoggingAPI as JSONLoggingAPI from google.cloud.logging.connection import _MetricsAPI as JSONMetricsAPI @@ -47,7 +48,7 @@ from google.cloud.logging.sink import Sink -_DISABLE_GAX = os.getenv('GOOGLE_CLOUD_DISABLE_GAX', False) +_DISABLE_GAX = os.getenv(DISABLE_GRPC, False) _USE_GAX = _HAVE_GAX and not _DISABLE_GAX ASCENDING = 'timestamp asc' """Query string to order by ascending timestamps.""" diff --git a/google/cloud/pubsub/client.py b/google/cloud/pubsub/client.py index 9964bc9029d4..aed596e3d002 100644 --- a/google/cloud/pubsub/client.py +++ b/google/cloud/pubsub/client.py @@ -17,6 +17,7 @@ import os from google.cloud.client import JSONClient +from google.cloud.environment_vars import DISABLE_GRPC from google.cloud.pubsub.connection import Connection from google.cloud.pubsub.connection import _PublisherAPI as JSONPublisherAPI from google.cloud.pubsub.connection import _SubscriberAPI as JSONSubscriberAPI @@ -41,7 +42,7 @@ # pylint: enable=ungrouped-imports -_DISABLE_GAX = os.getenv('GOOGLE_CLOUD_DISABLE_GAX', False) +_DISABLE_GAX = os.getenv(DISABLE_GRPC, False) _USE_GAX = _HAVE_GAX and not _DISABLE_GAX diff --git a/unit_tests/datastore/test_connection.py b/unit_tests/datastore/test_connection.py index 478fd522a5cf..7146506c634f 100644 --- a/unit_tests/datastore/test_connection.py +++ b/unit_tests/datastore/test_connection.py @@ -292,10 +292,10 @@ def _make_query_pb(self, kind): return pb def _makeOne(self, credentials=None, http=None, - api_base_url=None, have_grpc=False): + api_base_url=None, use_grpc=False): from unit_tests._testing import _Monkey from google.cloud.datastore import connection as MUT - with _Monkey(MUT, _HAVE_GRPC=have_grpc): + with _Monkey(MUT, _USE_GRPC=use_grpc): return self._getTargetClass()(credentials=credentials, http=http, api_base_url=api_base_url) @@ -368,7 +368,7 @@ def mock_api(connection): return return_val with _Monkey(MUT, _DatastoreAPIOverHttp=mock_api): - conn = self._makeOne(have_grpc=False) + conn = self._makeOne(use_grpc=False) self.assertEqual(conn.credentials, None) self.assertIs(conn._datastore_api, return_val) @@ -386,7 +386,7 @@ def mock_api(connection): return return_val with _Monkey(MUT, _DatastoreAPIOverGRPC=mock_api): - conn = self._makeOne(have_grpc=True) + conn = self._makeOne(use_grpc=True) self.assertEqual(conn.credentials, None) self.assertIs(conn._datastore_api, return_val) From 0f3ef0d0d58ab3d326c37e170aebfe801794d95b Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Sat, 3 Sep 2016 10:02:31 -0700 Subject: [PATCH 2/8] Renaming make_stub() to make_secure_stub(). Changes mostly made with git grep -l make_stub | xargs sed -i s/make_stub/make_secure_stub/g Also updated the docstring and comments. --- google/cloud/_helpers.py | 8 ++--- google/cloud/bigtable/client.py | 27 +++++++++-------- google/cloud/datastore/connection.py | 9 +++--- unit_tests/bigtable/test_client.py | 40 ++++++++++++------------- unit_tests/datastore/test_connection.py | 4 +-- unit_tests/test__helpers.py | 6 ++-- 6 files changed, 48 insertions(+), 46 deletions(-) diff --git a/google/cloud/_helpers.py b/google/cloud/_helpers.py index 3f76827e1424..6440f2754d8b 100644 --- a/google/cloud/_helpers.py +++ b/google/cloud/_helpers.py @@ -612,8 +612,8 @@ def __call__(self, unused_context, callback): callback(headers, None) -def make_stub(credentials, user_agent, stub_class, host, port): - """Makes a stub for an RPC service. +def make_secure_stub(credentials, user_agent, stub_class, host, port): + """Makes a secure stub for an RPC service. Uses / depends on gRPC. @@ -636,8 +636,8 @@ def make_stub(credentials, user_agent, stub_class, host, port): :rtype: object, instance of ``stub_class`` :returns: The stub object used to make gRPC requests to a given API. """ - # Leaving the first argument to ssl_channel_credentials() as None - # loads root certificates from `grpc/_adapter/credentials/roots.pem`. + # ssl_channel_credentials() loads root certificates from + # `grpc/_adapter/credentials/roots.pem`. transport_creds = grpc.ssl_channel_credentials() custom_metadata_plugin = MetadataPlugin(credentials, user_agent) auth_creds = grpc.metadata_call_credentials( diff --git a/google/cloud/bigtable/client.py b/google/cloud/bigtable/client.py index 1a5a05336084..404c5ca6453d 100644 --- a/google/cloud/bigtable/client.py +++ b/google/cloud/bigtable/client.py @@ -29,7 +29,7 @@ from pkg_resources import get_distribution -from google.cloud._helpers import make_stub +from google.cloud._helpers import make_secure_stub from google.cloud.bigtable._generated import bigtable_instance_admin_pb2 from google.cloud.bigtable._generated import bigtable_pb2 from google.cloud.bigtable._generated import bigtable_table_admin_pb2 @@ -81,9 +81,9 @@ def _make_data_stub(client): :rtype: :class:`._generated.bigtable_pb2.BigtableStub` :returns: A gRPC stub object. """ - return make_stub(client.credentials, client.user_agent, - bigtable_pb2.BigtableStub, - DATA_API_HOST, DATA_API_PORT) + return make_secure_stub(client.credentials, client.user_agent, + bigtable_pb2.BigtableStub, + DATA_API_HOST, DATA_API_PORT) def _make_instance_stub(client): @@ -95,9 +95,10 @@ def _make_instance_stub(client): :rtype: :class:`.bigtable_instance_admin_pb2.BigtableInstanceAdminStub` :returns: A gRPC stub object. """ - return make_stub(client.credentials, client.user_agent, - bigtable_instance_admin_pb2.BigtableInstanceAdminStub, - INSTANCE_ADMIN_HOST, INSTANCE_ADMIN_PORT) + return make_secure_stub( + client.credentials, client.user_agent, + bigtable_instance_admin_pb2.BigtableInstanceAdminStub, + INSTANCE_ADMIN_HOST, INSTANCE_ADMIN_PORT) def _make_operations_stub(client): @@ -112,9 +113,9 @@ def _make_operations_stub(client): :rtype: :class:`._generated.operations_grpc_pb2.OperationsStub` :returns: A gRPC stub object. """ - return make_stub(client.credentials, client.user_agent, - operations_grpc_pb2.OperationsStub, - OPERATIONS_API_HOST, OPERATIONS_API_PORT) + return make_secure_stub(client.credentials, client.user_agent, + operations_grpc_pb2.OperationsStub, + OPERATIONS_API_HOST, OPERATIONS_API_PORT) def _make_table_stub(client): @@ -126,9 +127,9 @@ def _make_table_stub(client): :rtype: :class:`.bigtable_instance_admin_pb2.BigtableTableAdminStub` :returns: A gRPC stub object. """ - return make_stub(client.credentials, client.user_agent, - bigtable_table_admin_pb2.BigtableTableAdminStub, - TABLE_ADMIN_HOST, TABLE_ADMIN_PORT) + return make_secure_stub(client.credentials, client.user_agent, + bigtable_table_admin_pb2.BigtableTableAdminStub, + TABLE_ADMIN_HOST, TABLE_ADMIN_PORT) class Client(_ClientFactoryMixin, _ClientProjectMixin): diff --git a/google/cloud/datastore/connection.py b/google/cloud/datastore/connection.py index 28a66b5076ea..4e66fa68428b 100644 --- a/google/cloud/datastore/connection.py +++ b/google/cloud/datastore/connection.py @@ -18,7 +18,7 @@ from google.rpc import status_pb2 -from google.cloud._helpers import make_stub +from google.cloud._helpers import make_secure_stub from google.cloud import connection as connection_module from google.cloud.environment_vars import DISABLE_GRPC from google.cloud.environment_vars import GCD_HOST @@ -237,9 +237,10 @@ class _DatastoreAPIOverGRPC(object): """ def __init__(self, connection): - self._stub = make_stub(connection.credentials, connection.USER_AGENT, - datastore_grpc_pb2.DatastoreStub, - DATASTORE_API_HOST, DATASTORE_API_PORT) + self._stub = make_secure_stub(connection.credentials, + connection.USER_AGENT, + datastore_grpc_pb2.DatastoreStub, + DATASTORE_API_HOST, DATASTORE_API_PORT) def lookup(self, project, request_pb): """Perform a ``lookup`` request. diff --git a/unit_tests/bigtable/test_client.py b/unit_tests/bigtable/test_client.py index 7f9833314b60..799dd96fa93b 100644 --- a/unit_tests/bigtable/test_client.py +++ b/unit_tests/bigtable/test_client.py @@ -31,17 +31,17 @@ def test_it(self): client = _Client(credentials, user_agent) fake_stub = object() - make_stub_args = [] + make_secure_stub_args = [] - def mock_make_stub(*args): - make_stub_args.append(args) + def mock_make_secure_stub(*args): + make_secure_stub_args.append(args) return fake_stub - with _Monkey(MUT, make_stub=mock_make_stub): + with _Monkey(MUT, make_secure_stub=mock_make_secure_stub): result = self._callFUT(client) self.assertIs(result, fake_stub) - self.assertEqual(make_stub_args, [ + self.assertEqual(make_secure_stub_args, [ ( client.credentials, client.user_agent, @@ -67,17 +67,17 @@ def test_it(self): client = _Client(credentials, user_agent) fake_stub = object() - make_stub_args = [] + make_secure_stub_args = [] - def mock_make_stub(*args): - make_stub_args.append(args) + def mock_make_secure_stub(*args): + make_secure_stub_args.append(args) return fake_stub - with _Monkey(MUT, make_stub=mock_make_stub): + with _Monkey(MUT, make_secure_stub=mock_make_secure_stub): result = self._callFUT(client) self.assertIs(result, fake_stub) - self.assertEqual(make_stub_args, [ + self.assertEqual(make_secure_stub_args, [ ( client.credentials, client.user_agent, @@ -103,17 +103,17 @@ def test_it(self): client = _Client(credentials, user_agent) fake_stub = object() - make_stub_args = [] + make_secure_stub_args = [] - def mock_make_stub(*args): - make_stub_args.append(args) + def mock_make_secure_stub(*args): + make_secure_stub_args.append(args) return fake_stub - with _Monkey(MUT, make_stub=mock_make_stub): + with _Monkey(MUT, make_secure_stub=mock_make_secure_stub): result = self._callFUT(client) self.assertIs(result, fake_stub) - self.assertEqual(make_stub_args, [ + self.assertEqual(make_secure_stub_args, [ ( client.credentials, client.user_agent, @@ -139,17 +139,17 @@ def test_it(self): client = _Client(credentials, user_agent) fake_stub = object() - make_stub_args = [] + make_secure_stub_args = [] - def mock_make_stub(*args): - make_stub_args.append(args) + def mock_make_secure_stub(*args): + make_secure_stub_args.append(args) return fake_stub - with _Monkey(MUT, make_stub=mock_make_stub): + with _Monkey(MUT, make_secure_stub=mock_make_secure_stub): result = self._callFUT(client) self.assertIs(result, fake_stub) - self.assertEqual(make_stub_args, [ + self.assertEqual(make_secure_stub_args, [ ( client.credentials, client.user_agent, diff --git a/unit_tests/datastore/test_connection.py b/unit_tests/datastore/test_connection.py index 7146506c634f..fc43ec2e4fc5 100644 --- a/unit_tests/datastore/test_connection.py +++ b/unit_tests/datastore/test_connection.py @@ -123,11 +123,11 @@ def _makeOne(self, stub, connection=None, mock_args=None): if mock_args is None: mock_args = [] - def mock_make_stub(*args): + def mock_make_secure_stub(*args): mock_args.append(args) return stub - with _Monkey(MUT, make_stub=mock_make_stub): + with _Monkey(MUT, make_secure_stub=mock_make_secure_stub): return self._getTargetClass()(connection) def test_constructor(self): diff --git a/unit_tests/test__helpers.py b/unit_tests/test__helpers.py index 75d25982ad27..5f23b0dacec9 100644 --- a/unit_tests/test__helpers.py +++ b/unit_tests/test__helpers.py @@ -891,11 +891,11 @@ def callback(*args): self.assertEqual(len(credentials._tokens), 1) -class Test_make_stub(unittest.TestCase): +class Test_make_secure_stub(unittest.TestCase): def _callFUT(self, *args, **kwargs): - from google.cloud._helpers import make_stub - return make_stub(*args, **kwargs) + from google.cloud._helpers import make_secure_stub + return make_secure_stub(*args, **kwargs) def test_it(self): from unit_tests._testing import _Monkey From 4224f8091bd79d6dbdaee2267ea5c09d7d4536a5 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Sat, 3 Sep 2016 10:09:04 -0700 Subject: [PATCH 3/8] Adding make_insecure_stub(). --- google/cloud/_helpers.py | 23 +++++++++++++++++++++++ unit_tests/test__helpers.py | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/google/cloud/_helpers.py b/google/cloud/_helpers.py index 6440f2754d8b..c6ce06902159 100644 --- a/google/cloud/_helpers.py +++ b/google/cloud/_helpers.py @@ -649,6 +649,29 @@ def make_secure_stub(credentials, user_agent, stub_class, host, port): return stub_class(channel) +def make_insecure_stub(stub_class, host, port): + """Makes an insecure stub for an RPC service. + + Uses / depends on gRPC. + + :type stub_class: type + :param stub_class: A gRPC stub type for a given service. + + :type host: str + :param host: The host for the service. + + :type port: int + :param port: The port for the service. + + :rtype: object, instance of ``stub_class`` + :returns: The stub object used to make gRPC requests to a given API. + """ + # NOTE: This assumes port != http_client.HTTPS_PORT: + target = '%s:%d' % (host, port) + channel = grpc.insecure_channel(target) + return stub_class(channel) + + def exc_to_code(exc): """Retrieves the status code from a gRPC exception. diff --git a/unit_tests/test__helpers.py b/unit_tests/test__helpers.py index 5f23b0dacec9..4664ff25deae 100644 --- a/unit_tests/test__helpers.py +++ b/unit_tests/test__helpers.py @@ -969,6 +969,43 @@ def mock_plugin(*args): (target, COMPOSITE_CREDS)) +class Test_make_insecure_stub(unittest.TestCase): + + def _callFUT(self, *args, **kwargs): + from gcloud._helpers import make_insecure_stub + return make_insecure_stub(*args, **kwargs) + + def test_it(self): + from unit_tests._testing import _Monkey + from gcloud import _helpers as MUT + + mock_result = object() + stub_inputs = [] + CHANNEL = object() + + class _GRPCModule(object): + + def insecure_channel(self, *args): + self.insecure_channel_args = args + return CHANNEL + + grpc_mod = _GRPCModule() + + def mock_stub_class(channel): + stub_inputs.append(channel) + return mock_result + + host = 'HOST' + port = 1025 + with _Monkey(MUT, grpc=grpc_mod): + result = self._callFUT(mock_stub_class, host, port) + + self.assertTrue(result is mock_result) + self.assertEqual(stub_inputs, [CHANNEL]) + target = '%s:%d' % (host, port) + self.assertEqual(grpc_mod.insecure_channel_args, (target,)) + + class Test_exc_to_code(unittest.TestCase): def _callFUT(self, exc): From 1264624b0801579592407f9ab326f2c852bfdfa0 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Sat, 3 Sep 2016 10:31:23 -0700 Subject: [PATCH 4/8] Removing port from make_secure_stub(). Always use the HTTPS port when constructing the target. --- google/cloud/_helpers.py | 11 ++++------- google/cloud/bigtable/client.py | 16 ++++------------ google/cloud/datastore/connection.py | 4 +--- unit_tests/bigtable/test_client.py | 4 ---- unit_tests/datastore/test_connection.py | 1 - unit_tests/test__helpers.py | 10 +++++----- 6 files changed, 14 insertions(+), 32 deletions(-) diff --git a/google/cloud/_helpers.py b/google/cloud/_helpers.py index c6ce06902159..d4b09438321b 100644 --- a/google/cloud/_helpers.py +++ b/google/cloud/_helpers.py @@ -39,7 +39,7 @@ grpc = None _Rendezvous = Exception import six -from six.moves.http_client import HTTPConnection +from six.moves import http_client from six.moves import configparser # pylint: disable=ungrouped-imports @@ -269,7 +269,7 @@ def _compute_engine_id(): host = '169.254.169.254' uri_path = '/computeMetadata/v1/project/project-id' headers = {'Metadata-Flavor': 'Google'} - connection = HTTPConnection(host, timeout=0.1) + connection = http_client.HTTPConnection(host, timeout=0.1) try: connection.request('GET', uri_path, headers=headers) @@ -612,7 +612,7 @@ def __call__(self, unused_context, callback): callback(headers, None) -def make_secure_stub(credentials, user_agent, stub_class, host, port): +def make_secure_stub(credentials, user_agent, stub_class, host): """Makes a secure stub for an RPC service. Uses / depends on gRPC. @@ -630,9 +630,6 @@ def make_secure_stub(credentials, user_agent, stub_class, host, port): :type host: str :param host: The host for the service. - :type port: int - :param port: The port for the service. - :rtype: object, instance of ``stub_class`` :returns: The stub object used to make gRPC requests to a given API. """ @@ -644,7 +641,7 @@ def make_secure_stub(credentials, user_agent, stub_class, host, port): custom_metadata_plugin, name='google_creds') channel_creds = grpc.composite_channel_credentials( transport_creds, auth_creds) - target = '%s:%d' % (host, port) + target = '%s:%d' % (host, http_client.HTTPS_PORT) channel = grpc.secure_channel(target, channel_creds) return stub_class(channel) diff --git a/google/cloud/bigtable/client.py b/google/cloud/bigtable/client.py index 404c5ca6453d..8459c312368f 100644 --- a/google/cloud/bigtable/client.py +++ b/google/cloud/bigtable/client.py @@ -44,21 +44,14 @@ TABLE_ADMIN_HOST = 'bigtableadmin.googleapis.com' """Table Admin API request host.""" -TABLE_ADMIN_PORT = 443 -"""Table Admin API request port.""" INSTANCE_ADMIN_HOST = 'bigtableadmin.googleapis.com' """Cluster Admin API request host.""" -INSTANCE_ADMIN_PORT = 443 -"""Cluster Admin API request port.""" DATA_API_HOST = 'bigtable.googleapis.com' """Data API request host.""" -DATA_API_PORT = 443 -"""Data API request port.""" OPERATIONS_API_HOST = INSTANCE_ADMIN_HOST -OPERATIONS_API_PORT = INSTANCE_ADMIN_PORT ADMIN_SCOPE = 'https://www.googleapis.com/auth/bigtable.admin' """Scope for interacting with the Cluster Admin and Table Admin APIs.""" @@ -82,8 +75,7 @@ def _make_data_stub(client): :returns: A gRPC stub object. """ return make_secure_stub(client.credentials, client.user_agent, - bigtable_pb2.BigtableStub, - DATA_API_HOST, DATA_API_PORT) + bigtable_pb2.BigtableStub, DATA_API_HOST) def _make_instance_stub(client): @@ -98,7 +90,7 @@ def _make_instance_stub(client): return make_secure_stub( client.credentials, client.user_agent, bigtable_instance_admin_pb2.BigtableInstanceAdminStub, - INSTANCE_ADMIN_HOST, INSTANCE_ADMIN_PORT) + INSTANCE_ADMIN_HOST) def _make_operations_stub(client): @@ -115,7 +107,7 @@ def _make_operations_stub(client): """ return make_secure_stub(client.credentials, client.user_agent, operations_grpc_pb2.OperationsStub, - OPERATIONS_API_HOST, OPERATIONS_API_PORT) + OPERATIONS_API_HOST) def _make_table_stub(client): @@ -129,7 +121,7 @@ def _make_table_stub(client): """ return make_secure_stub(client.credentials, client.user_agent, bigtable_table_admin_pb2.BigtableTableAdminStub, - TABLE_ADMIN_HOST, TABLE_ADMIN_PORT) + TABLE_ADMIN_HOST) class Client(_ClientFactoryMixin, _ClientProjectMixin): diff --git a/google/cloud/datastore/connection.py b/google/cloud/datastore/connection.py index 4e66fa68428b..31e60982ae49 100644 --- a/google/cloud/datastore/connection.py +++ b/google/cloud/datastore/connection.py @@ -42,8 +42,6 @@ DATASTORE_API_HOST = 'datastore.googleapis.com' """Datastore API request host.""" -DATASTORE_API_PORT = 443 -"""Datastore API request port.""" _DISABLE_GRPC = os.getenv(DISABLE_GRPC, False) _USE_GRPC = _HAVE_GRPC and not _DISABLE_GRPC @@ -240,7 +238,7 @@ def __init__(self, connection): self._stub = make_secure_stub(connection.credentials, connection.USER_AGENT, datastore_grpc_pb2.DatastoreStub, - DATASTORE_API_HOST, DATASTORE_API_PORT) + DATASTORE_API_HOST) def lookup(self, project, request_pb): """Perform a ``lookup`` request. diff --git a/unit_tests/bigtable/test_client.py b/unit_tests/bigtable/test_client.py index 799dd96fa93b..95e64b0f0826 100644 --- a/unit_tests/bigtable/test_client.py +++ b/unit_tests/bigtable/test_client.py @@ -47,7 +47,6 @@ def mock_make_secure_stub(*args): client.user_agent, MUT.bigtable_pb2.BigtableStub, MUT.DATA_API_HOST, - MUT.DATA_API_PORT, ), ]) @@ -83,7 +82,6 @@ def mock_make_secure_stub(*args): client.user_agent, MUT.bigtable_instance_admin_pb2.BigtableInstanceAdminStub, MUT.INSTANCE_ADMIN_HOST, - MUT.INSTANCE_ADMIN_PORT, ), ]) @@ -119,7 +117,6 @@ def mock_make_secure_stub(*args): client.user_agent, MUT.operations_grpc_pb2.OperationsStub, MUT.OPERATIONS_API_HOST, - MUT.OPERATIONS_API_PORT, ), ]) @@ -155,7 +152,6 @@ def mock_make_secure_stub(*args): client.user_agent, MUT.bigtable_table_admin_pb2.BigtableTableAdminStub, MUT.TABLE_ADMIN_HOST, - MUT.TABLE_ADMIN_PORT, ), ]) diff --git a/unit_tests/datastore/test_connection.py b/unit_tests/datastore/test_connection.py index fc43ec2e4fc5..23891f513942 100644 --- a/unit_tests/datastore/test_connection.py +++ b/unit_tests/datastore/test_connection.py @@ -147,7 +147,6 @@ def test_constructor(self): conn.USER_AGENT, MUT.datastore_grpc_pb2.DatastoreStub, MUT.DATASTORE_API_HOST, - MUT.DATASTORE_API_PORT, )]) def test_lookup(self): diff --git a/unit_tests/test__helpers.py b/unit_tests/test__helpers.py index 4664ff25deae..41e955b7aa69 100644 --- a/unit_tests/test__helpers.py +++ b/unit_tests/test__helpers.py @@ -295,15 +295,15 @@ def _callFUT(self): return _compute_engine_id() def _monkeyConnection(self, connection): + from six.moves import http_client from unit_tests._testing import _Monkey - from google.cloud import _helpers def _connection_factory(host, timeout): connection.host = host connection.timeout = timeout return connection - return _Monkey(_helpers, HTTPConnection=_connection_factory) + return _Monkey(http_client, HTTPConnection=_connection_factory) def test_bad_status(self): connection = _HTTPConnection(404, None) @@ -898,6 +898,7 @@ def _callFUT(self, *args, **kwargs): return make_secure_stub(*args, **kwargs) def test_it(self): + from six.moves import http_client from unit_tests._testing import _Monkey from google.cloud import _helpers as MUT @@ -947,13 +948,12 @@ def mock_plugin(*args): return metadata_plugin host = 'HOST' - port = 1025 credentials = object() user_agent = 'USER_AGENT' with _Monkey(MUT, grpc=grpc_mod, MetadataPlugin=mock_plugin): result = self._callFUT(credentials, user_agent, - mock_stub_class, host, port) + mock_stub_class, host) self.assertTrue(result is mock_result) self.assertEqual(stub_inputs, [CHANNEL]) @@ -964,7 +964,7 @@ def mock_plugin(*args): self.assertEqual( grpc_mod.composite_channel_credentials_args, (SSL_CREDS, METADATA_CREDS)) - target = '%s:%d' % (host, port) + target = '%s:%d' % (host, http_client.HTTPS_PORT) self.assertEqual(grpc_mod.secure_channel_args, (target, COMPOSITE_CREDS)) From 60ed9e6f8fceb10410f856f71fe4f8aee6a0f71d Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Sat, 3 Sep 2016 10:48:30 -0700 Subject: [PATCH 5/8] Removing api_base_url from datastore Connection constructor. --- google/cloud/datastore/connection.py | 19 ++++++--------- unit_tests/datastore/test_connection.py | 31 ++----------------------- 2 files changed, 9 insertions(+), 41 deletions(-) diff --git a/google/cloud/datastore/connection.py b/google/cloud/datastore/connection.py index 31e60982ae49..69b6670ec156 100644 --- a/google/cloud/datastore/connection.py +++ b/google/cloud/datastore/connection.py @@ -355,10 +355,6 @@ class Connection(connection_module.Connection): :type http: :class:`httplib2.Http` or class that defines ``request()``. :param http: An optional HTTP object to make requests. - - :type api_base_url: string - :param api_base_url: The base of the API call URL. Defaults to - :attr:`API_BASE_URL`. """ API_BASE_URL = 'https://' + DATASTORE_API_HOST @@ -374,15 +370,14 @@ class Connection(connection_module.Connection): SCOPE = ('https://www.googleapis.com/auth/datastore',) """The scopes required for authenticating as a Cloud Datastore consumer.""" - def __init__(self, credentials=None, http=None, api_base_url=None): + def __init__(self, credentials=None, http=None): super(Connection, self).__init__(credentials=credentials, http=http) - if api_base_url is None: - try: - # gcd.sh has /datastore/ in the path still since it supports - # v1beta2 and v1beta3 simultaneously. - api_base_url = '%s/datastore' % (os.environ[GCD_HOST],) - except KeyError: - api_base_url = self.__class__.API_BASE_URL + try: + # gcd.sh has /datastore/ in the path still since it supports + # v1beta2 and v1beta3 simultaneously. + api_base_url = '%s/datastore' % (os.environ[GCD_HOST],) + except KeyError: + api_base_url = self.__class__.API_BASE_URL self.api_base_url = api_base_url if _USE_GRPC: self._datastore_api = _DatastoreAPIOverGRPC(self) diff --git a/unit_tests/datastore/test_connection.py b/unit_tests/datastore/test_connection.py index 23891f513942..65b67a8d0c67 100644 --- a/unit_tests/datastore/test_connection.py +++ b/unit_tests/datastore/test_connection.py @@ -290,13 +290,11 @@ def _make_query_pb(self, kind): pb.kind.add().name = kind return pb - def _makeOne(self, credentials=None, http=None, - api_base_url=None, use_grpc=False): + def _makeOne(self, credentials=None, http=None, use_grpc=False): from unit_tests._testing import _Monkey from google.cloud.datastore import connection as MUT with _Monkey(MUT, _USE_GRPC=use_grpc): - return self._getTargetClass()(credentials=credentials, http=http, - api_base_url=api_base_url) + return self._getTargetClass()(credentials=credentials, http=http) def _verifyProtobufCall(self, called_with, URI, conn): self.assertEqual(called_with['uri'], URI) @@ -326,31 +324,6 @@ def test_custom_url_from_env(self): self.assertNotEqual(conn.api_base_url, API_BASE_URL) self.assertEqual(conn.api_base_url, HOST + '/datastore') - def test_custom_url_from_constructor(self): - from google.cloud.connection import API_BASE_URL - - HOST = object() - conn = self._makeOne(api_base_url=HOST) - self.assertNotEqual(conn.api_base_url, API_BASE_URL) - self.assertEqual(conn.api_base_url, HOST) - - def test_custom_url_constructor_and_env(self): - import os - from unit_tests._testing import _Monkey - from google.cloud.connection import API_BASE_URL - from google.cloud.environment_vars import GCD_HOST - - HOST1 = object() - HOST2 = object() - fake_environ = {GCD_HOST: HOST1} - - with _Monkey(os, environ=fake_environ): - conn = self._makeOne(api_base_url=HOST2) - - self.assertNotEqual(conn.api_base_url, API_BASE_URL) - self.assertNotEqual(conn.api_base_url, HOST1) - self.assertEqual(conn.api_base_url, HOST2) - def test_ctor_defaults(self): conn = self._makeOne() self.assertEqual(conn.credentials, None) From 0cc607936ed8f0887149e65c4386be129c45946b Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Sat, 3 Sep 2016 11:39:55 -0700 Subject: [PATCH 6/8] Updating _DatastoreAPIOverGRPC to create an insecure stub. This was done since the local emulator doesn't use a secure connection. In the process, also - Using the DATASTORE_EMULATOR_HOST env. var. instead of DATASTORE_HOST since the former is just the host without the protocol (we do this to avoid having to parse anything) - Making "port" optional in make_insecure_stub() since the env. var. provided by the datastore emulator already has the port in it - Removing the "/datastore/" path from the GCD emulator URI (no longer needed) --- google/cloud/_helpers.py | 14 +++++--- google/cloud/datastore/connection.py | 31 +++++++++++------ google/cloud/environment_vars.py | 2 +- unit_tests/datastore/test_connection.py | 46 +++++++++++++++++++------ unit_tests/test__helpers.py | 17 ++++++--- 5 files changed, 78 insertions(+), 32 deletions(-) diff --git a/google/cloud/_helpers.py b/google/cloud/_helpers.py index d4b09438321b..da3e689ddc5e 100644 --- a/google/cloud/_helpers.py +++ b/google/cloud/_helpers.py @@ -646,7 +646,7 @@ def make_secure_stub(credentials, user_agent, stub_class, host): return stub_class(channel) -def make_insecure_stub(stub_class, host, port): +def make_insecure_stub(stub_class, host, port=None): """Makes an insecure stub for an RPC service. Uses / depends on gRPC. @@ -655,16 +655,20 @@ def make_insecure_stub(stub_class, host, port): :param stub_class: A gRPC stub type for a given service. :type host: str - :param host: The host for the service. + :param host: The host for the service. May also include the port + if ``port`` is unspecified. :type port: int - :param port: The port for the service. + :param port: (Optional) The port for the service. :rtype: object, instance of ``stub_class`` :returns: The stub object used to make gRPC requests to a given API. """ - # NOTE: This assumes port != http_client.HTTPS_PORT: - target = '%s:%d' % (host, port) + if port is None: + target = host + else: + # NOTE: This assumes port != http_client.HTTPS_PORT: + target = '%s:%d' % (host, port) channel = grpc.insecure_channel(target) return stub_class(channel) diff --git a/google/cloud/datastore/connection.py b/google/cloud/datastore/connection.py index 69b6670ec156..1f6416a1d39b 100644 --- a/google/cloud/datastore/connection.py +++ b/google/cloud/datastore/connection.py @@ -18,6 +18,7 @@ from google.rpc import status_pb2 +from google.cloud._helpers import make_insecure_stub from google.cloud._helpers import make_secure_stub from google.cloud import connection as connection_module from google.cloud.environment_vars import DISABLE_GRPC @@ -232,13 +233,20 @@ class _DatastoreAPIOverGRPC(object): :type connection: :class:`google.cloud.datastore.connection.Connection` :param connection: A connection object that contains helpful information for making requests. + + :type secure: bool + :param secure: Flag indicating if a secure stub connection is needed. """ - def __init__(self, connection): - self._stub = make_secure_stub(connection.credentials, - connection.USER_AGENT, - datastore_grpc_pb2.DatastoreStub, - DATASTORE_API_HOST) + def __init__(self, connection, secure): + if secure: + self._stub = make_secure_stub(connection.credentials, + connection.USER_AGENT, + datastore_grpc_pb2.DatastoreStub, + connection.host) + else: + self._stub = make_insecure_stub(datastore_grpc_pb2.DatastoreStub, + connection.host) def lookup(self, project, request_pb): """Perform a ``lookup`` request. @@ -373,14 +381,15 @@ class Connection(connection_module.Connection): def __init__(self, credentials=None, http=None): super(Connection, self).__init__(credentials=credentials, http=http) try: - # gcd.sh has /datastore/ in the path still since it supports - # v1beta2 and v1beta3 simultaneously. - api_base_url = '%s/datastore' % (os.environ[GCD_HOST],) + self.host = os.environ[GCD_HOST] + self.api_base_url = 'http://' + self.host + secure = False except KeyError: - api_base_url = self.__class__.API_BASE_URL - self.api_base_url = api_base_url + self.host = DATASTORE_API_HOST + self.api_base_url = self.__class__.API_BASE_URL + secure = True if _USE_GRPC: - self._datastore_api = _DatastoreAPIOverGRPC(self) + self._datastore_api = _DatastoreAPIOverGRPC(self, secure=secure) else: self._datastore_api = _DatastoreAPIOverHttp(self) diff --git a/google/cloud/environment_vars.py b/google/cloud/environment_vars.py index 599aa968d2af..0742d76916e8 100644 --- a/google/cloud/environment_vars.py +++ b/google/cloud/environment_vars.py @@ -27,7 +27,7 @@ GCD_DATASET = 'DATASTORE_DATASET' """Environment variable defining default dataset ID under GCD.""" -GCD_HOST = 'DATASTORE_HOST' +GCD_HOST = 'DATASTORE_EMULATOR_HOST' """Environment variable defining host for GCD dataset server.""" PUBSUB_EMULATOR = 'PUBSUB_EMULATOR_HOST' diff --git a/unit_tests/datastore/test_connection.py b/unit_tests/datastore/test_connection.py index 65b67a8d0c67..420eb2b0bce8 100644 --- a/unit_tests/datastore/test_connection.py +++ b/unit_tests/datastore/test_connection.py @@ -112,29 +112,35 @@ def _getTargetClass(self): from google.cloud.datastore.connection import _DatastoreAPIOverGRPC return _DatastoreAPIOverGRPC - def _makeOne(self, stub, connection=None, mock_args=None): + def _makeOne(self, stub, connection=None, secure=True, mock_args=None): from unit_tests._testing import _Monkey from google.cloud.datastore import connection as MUT if connection is None: connection = _Connection(None) connection.credentials = object() + connection.host = 'CURR_HOST' if mock_args is None: mock_args = [] - def mock_make_secure_stub(*args): + def mock_make_stub(*args): mock_args.append(args) return stub - with _Monkey(MUT, make_secure_stub=mock_make_secure_stub): - return self._getTargetClass()(connection) + if secure: + to_monkey = {'make_secure_stub': mock_make_stub} + else: + to_monkey = {'make_insecure_stub': mock_make_stub} + with _Monkey(MUT, **to_monkey): + return self._getTargetClass()(connection, secure) def test_constructor(self): from google.cloud.datastore import connection as MUT conn = _Connection(None) conn.credentials = object() + conn.host = 'CURR_HOST' stub = _GRPCStub() mock_args = [] @@ -146,7 +152,26 @@ def test_constructor(self): conn.credentials, conn.USER_AGENT, MUT.datastore_grpc_pb2.DatastoreStub, - MUT.DATASTORE_API_HOST, + conn.host, + )]) + + def test_constructor_insecure(self): + from gcloud.datastore import connection as MUT + + conn = _Connection(None) + conn.credentials = object() + conn.host = 'CURR_HOST:1234' + + stub = _GRPCStub() + mock_args = [] + datastore_api = self._makeOne(stub, connection=conn, + secure=False, + mock_args=mock_args) + self.assertIs(datastore_api._stub, stub) + + self.assertEqual(mock_args, [( + MUT.datastore_grpc_pb2.DatastoreStub, + conn.host, )]) def test_lookup(self): @@ -322,7 +347,7 @@ def test_custom_url_from_env(self): conn = self._makeOne() self.assertNotEqual(conn.api_base_url, API_BASE_URL) - self.assertEqual(conn.api_base_url, HOST + '/datastore') + self.assertEqual(conn.api_base_url, 'http://' + HOST) def test_ctor_defaults(self): conn = self._makeOne() @@ -350,11 +375,11 @@ def test_ctor_with_grpc(self): from unit_tests._testing import _Monkey from google.cloud.datastore import connection as MUT - connections = [] + api_args = [] return_val = object() - def mock_api(connection): - connections.append(connection) + def mock_api(connection, secure): + api_args.append((connection, secure)) return return_val with _Monkey(MUT, _DatastoreAPIOverGRPC=mock_api): @@ -362,7 +387,7 @@ def mock_api(connection): self.assertEqual(conn.credentials, None) self.assertIs(conn._datastore_api, return_val) - self.assertEqual(connections, [conn]) + self.assertEqual(api_args, [(conn, True)]) def test_ctor_explicit(self): class Creds(object): @@ -1065,6 +1090,7 @@ def request(self, **kw): class _Connection(object): + host = None USER_AGENT = 'you-sir-age-int' def __init__(self, api_url): diff --git a/unit_tests/test__helpers.py b/unit_tests/test__helpers.py index 41e955b7aa69..050bc821f6f5 100644 --- a/unit_tests/test__helpers.py +++ b/unit_tests/test__helpers.py @@ -975,7 +975,7 @@ def _callFUT(self, *args, **kwargs): from gcloud._helpers import make_insecure_stub return make_insecure_stub(*args, **kwargs) - def test_it(self): + def _helper(self, target, host, port=None): from unit_tests._testing import _Monkey from gcloud import _helpers as MUT @@ -995,16 +995,23 @@ def mock_stub_class(channel): stub_inputs.append(channel) return mock_result - host = 'HOST' - port = 1025 with _Monkey(MUT, grpc=grpc_mod): - result = self._callFUT(mock_stub_class, host, port) + result = self._callFUT(mock_stub_class, host, port=port) self.assertTrue(result is mock_result) self.assertEqual(stub_inputs, [CHANNEL]) - target = '%s:%d' % (host, port) self.assertEqual(grpc_mod.insecure_channel_args, (target,)) + def test_with_port_argument(self): + host = 'HOST' + port = 1025 + target = '%s:%d' % (host, port) + self._helper(target, host, port=port) + + def test_without_port_argument(self): + host = 'HOST:1114' + self._helper(host, host) + class Test_exc_to_code(unittest.TestCase): From 656bf4db02e66ec96fd380c2556431f90d92af15 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Sat, 3 Sep 2016 12:40:20 -0700 Subject: [PATCH 7/8] Updating emulator script for non-legacy datastore emulator. - Adding "--no-legacy" flag to "gcloud" CLI calls - Updating instructions in CONTRIBUTING document to showcase the flag and the GCLOUD_DISABLE_GRPC env. var. - Updating the datastore ready line for the server from the cloud-datastore-emulator (i.e. "--no-legacy") component, since it is different from the "gcd-emulator" component --- CONTRIBUTING.rst | 3 ++- system_tests/run_emulator.py | 13 ++++++++++--- tox.ini | 2 ++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 37686c3ad620..b4a9f92fd98a 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -208,13 +208,14 @@ Running System Tests ``datastore`` emulator:: $ tox -e datastore-emulator + $ GOOGLE_CLOUD_DISABLE_GRPC=true tox -e datastore-emulator This also requires that the ``gcloud`` command line tool is installed. If you'd like to run them directly (outside of a ``tox`` environment), first start the emulator and take note of the process ID:: - $ gcloud beta emulators datastore start 2>&1 > log.txt & + $ gcloud beta emulators datastore start --no-legacy 2>&1 > log.txt & [1] 33333 then determine the environment variables needed to interact with diff --git a/system_tests/run_emulator.py b/system_tests/run_emulator.py index 5d946a1aac83..2a1e5d41a53b 100644 --- a/system_tests/run_emulator.py +++ b/system_tests/run_emulator.py @@ -35,7 +35,10 @@ 'datastore': (GCD_DATASET, GCD_HOST), 'pubsub': (PUBSUB_EMULATOR,) } -_DS_READY_LINE = '[datastore] INFO: Dev App Server is now running\n' +EXTRA = { + 'datastore': ('--no-legacy',), +} +_DS_READY_LINE = '[datastore] Dev App Server is now running.\n' _PS_READY_LINE_PREFIX = '[pubsub] INFO: Server started, listening on ' @@ -62,7 +65,9 @@ def get_start_command(package): :rtype: tuple :returns: The arguments to be used, in a tuple. """ - return 'gcloud', 'beta', 'emulators', package, 'start' + result = ('gcloud', 'beta', 'emulators', package, 'start') + extra = EXTRA.get(package, ()) + return result + extra def get_env_init_command(package): @@ -74,7 +79,9 @@ def get_env_init_command(package): :rtype: tuple :returns: The arguments to be used, in a tuple. """ - return 'gcloud', 'beta', 'emulators', package, 'env-init' + result = ('gcloud', 'beta', 'emulators', package, 'env-init') + extra = EXTRA.get(package, ()) + return result + extra def datastore_wait_ready(popen): diff --git a/tox.ini b/tox.ini index 5128f7160987..01e473331b59 100644 --- a/tox.ini +++ b/tox.ini @@ -124,6 +124,8 @@ commands = python {toxinidir}/system_tests/run_emulator.py --package=datastore setenv = GOOGLE_CLOUD_NO_PRINT=true +passenv = + GOOGLE_CLOUD_DISABLE_GRPC deps = {[testenv]deps} psutil From 077d190d9f8ca724d0825b6601b4bfc646ddc99d Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Wed, 7 Sep 2016 10:37:46 -0700 Subject: [PATCH 8/8] Fixing gcloud->google.cloud imports. These were committed before the rename and fixed up after rebase. --- unit_tests/datastore/test_connection.py | 2 +- unit_tests/test__helpers.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/unit_tests/datastore/test_connection.py b/unit_tests/datastore/test_connection.py index 420eb2b0bce8..0b80815b4697 100644 --- a/unit_tests/datastore/test_connection.py +++ b/unit_tests/datastore/test_connection.py @@ -156,7 +156,7 @@ def test_constructor(self): )]) def test_constructor_insecure(self): - from gcloud.datastore import connection as MUT + from google.cloud.datastore import connection as MUT conn = _Connection(None) conn.credentials = object() diff --git a/unit_tests/test__helpers.py b/unit_tests/test__helpers.py index 050bc821f6f5..db25efb23c9e 100644 --- a/unit_tests/test__helpers.py +++ b/unit_tests/test__helpers.py @@ -972,12 +972,12 @@ def mock_plugin(*args): class Test_make_insecure_stub(unittest.TestCase): def _callFUT(self, *args, **kwargs): - from gcloud._helpers import make_insecure_stub + from google.cloud._helpers import make_insecure_stub return make_insecure_stub(*args, **kwargs) def _helper(self, target, host, port=None): from unit_tests._testing import _Monkey - from gcloud import _helpers as MUT + from google.cloud import _helpers as MUT mock_result = object() stub_inputs = []