From 22a684d503990a89bb27b03408c97854227efc0b Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Mon, 15 Mar 2021 14:29:43 -0700 Subject: [PATCH 01/13] fix: address issue in establishing an emulator connection --- google/cloud/bigtable/client.py | 94 +++++++++++++++++++++++---------- 1 file changed, 67 insertions(+), 27 deletions(-) diff --git a/google/cloud/bigtable/client.py b/google/cloud/bigtable/client.py index 5e49934d0..217872425 100644 --- a/google/cloud/bigtable/client.py +++ b/google/cloud/bigtable/client.py @@ -32,6 +32,7 @@ import grpc from google.api_core.gapic_v1 import client_info +import google.auth from google.cloud import bigtable_v2 from google.cloud import bigtable_admin_v2 @@ -66,20 +67,14 @@ READ_ONLY_SCOPE = "https://www.googleapis.com/auth/bigtable.data.readonly" """Scope for reading table data.""" - def _create_gapic_client(client_class, client_options=None, transport=None): def inner(self): - if self._emulator_host is None: - return client_class( + return client_class( credentials=None, client_info=self._client_info, client_options=client_options, transport=transport, ) - else: - return client_class( - channel=self._emulator_channel, client_info=self._client_info - ) return inner @@ -166,16 +161,6 @@ def __init__( self._admin = bool(admin) self._client_info = client_info self._emulator_host = os.getenv(BIGTABLE_EMULATOR) - self._emulator_channel = None - - if self._emulator_host is not None: - self._emulator_channel = grpc.insecure_channel( - target=self._emulator_host, - options={ - "grpc.keepalive_time_ms": 30000, - "grpc.keepalive_timeout_ms": 10000, - }.items(), - ) if channel is not None: warnings.warn( @@ -208,22 +193,77 @@ def _get_scopes(self): return scopes + def _emulator_channel(self, transport, options): + """ + Creates a channel using self._credentials in a similar way to grpc.secure_channel but + using grpc.local_channel_credentials() rather than grpc.ssh_channel_credentials() + to allow easy connection to a local emulator. + :return: grpc.Channel or grpc.aio.Channel + """ + # TODO: Implement a special credentials type for emulator and use + # "transport.create_channel" to create gRPC channels once google-auth + # extends it's allowed credentials types. + # Note: this code also exists in the firestore client. + if "GrpcAsyncIOTransport" in str(transport.__name__): + return grpc.aio.secure_channel( + self._emulator_host, self._local_composite_credentials(), options=options + ) + else: + return grpc.secure_channel( + self._emulator_host, self._local_composite_credentials(), options=options + ) + + def _local_composite_credentials(self): + """ + Creates the credentials for the local emulator channel + :return: grpc.ChannelCredentials + """ + credentials = google.auth.credentials.with_scopes_if_required( + self._credentials, None + ) + request = google.auth.transport.requests.Request() + + # Create the metadata plugin for inserting the authorization header. + metadata_plugin = google.auth.transport.grpc.AuthMetadataPlugin( + credentials, request + ) + + # Create a set of grpc.CallCredentials using the metadata plugin. + google_auth_credentials = grpc.metadata_call_credentials(metadata_plugin) + + # Using the local_credentials to allow connection to emulator + local_credentials = grpc.local_channel_credentials() + + # Combine the local credentials and the authorization credentials. + return grpc.composite_channel_credentials( + local_credentials, google_auth_credentials + ) + def _create_gapic_client_channel(self, client_class, grpc_transport): + options = { + "grpc.max_send_message_length": -1, + "grpc.max_receive_message_length": -1, + "grpc.keepalive_time_ms": 30000, + "grpc.keepalive_timeout_ms": 10000, + }.items() if self._client_options and self._client_options.api_endpoint: api_endpoint = self._client_options.api_endpoint else: api_endpoint = client_class.DEFAULT_ENDPOINT - channel = grpc_transport.create_channel( - host=api_endpoint, - credentials=self._credentials, - options={ - "grpc.max_send_message_length": -1, - "grpc.max_receive_message_length": -1, - "grpc.keepalive_time_ms": 30000, - "grpc.keepalive_timeout_ms": 10000, - }.items(), - ) + channel = None + if self._emulator_host is not None: + api_endpoint = self._emulator_host + # channel = self._emulator_channel(transport) + # channel = self._emulator_channel(transport) + # transport = transport(host=self._target, channel=channel) + channel = self._emulator_channel(grpc_transport, options) + else: + channel = grpc_transport.create_channel( + host=api_endpoint, + credentials=self._credentials, + options=options, + ) transport = grpc_transport(channel=channel, host=api_endpoint) return transport From 9eea81422faee47b77dcba6cc5f430040b9bbdfc Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Mon, 15 Mar 2021 14:32:04 -0700 Subject: [PATCH 02/13] chore: remove commented out lines --- google/cloud/bigtable/client.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/google/cloud/bigtable/client.py b/google/cloud/bigtable/client.py index 217872425..3165ac83b 100644 --- a/google/cloud/bigtable/client.py +++ b/google/cloud/bigtable/client.py @@ -254,9 +254,6 @@ def _create_gapic_client_channel(self, client_class, grpc_transport): channel = None if self._emulator_host is not None: api_endpoint = self._emulator_host - # channel = self._emulator_channel(transport) - # channel = self._emulator_channel(transport) - # transport = transport(host=self._target, channel=channel) channel = self._emulator_channel(grpc_transport, options) else: channel = grpc_transport.create_channel( From ed19cc21799aa7012f75984072dfbd950c5ac082 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Tue, 16 Mar 2021 15:08:00 -0700 Subject: [PATCH 03/13] test: update tests --- google/cloud/bigtable/client.py | 33 ++++++++++++++------------ tests/unit/test_client.py | 42 ++++++++++++++++++++------------- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/google/cloud/bigtable/client.py b/google/cloud/bigtable/client.py index 3165ac83b..be536f295 100644 --- a/google/cloud/bigtable/client.py +++ b/google/cloud/bigtable/client.py @@ -67,14 +67,15 @@ READ_ONLY_SCOPE = "https://www.googleapis.com/auth/bigtable.data.readonly" """Scope for reading table data.""" + def _create_gapic_client(client_class, client_options=None, transport=None): def inner(self): return client_class( - credentials=None, - client_info=self._client_info, - client_options=client_options, - transport=transport, - ) + credentials=None, + client_info=self._client_info, + client_options=client_options, + transport=transport, + ) return inner @@ -206,11 +207,15 @@ def _emulator_channel(self, transport, options): # Note: this code also exists in the firestore client. if "GrpcAsyncIOTransport" in str(transport.__name__): return grpc.aio.secure_channel( - self._emulator_host, self._local_composite_credentials(), options=options + self._emulator_host, + self._local_composite_credentials(), + options=options, ) else: return grpc.secure_channel( - self._emulator_host, self._local_composite_credentials(), options=options + self._emulator_host, + self._local_composite_credentials(), + options=options, ) def _local_composite_credentials(self): @@ -241,11 +246,11 @@ def _local_composite_credentials(self): def _create_gapic_client_channel(self, client_class, grpc_transport): options = { - "grpc.max_send_message_length": -1, - "grpc.max_receive_message_length": -1, - "grpc.keepalive_time_ms": 30000, - "grpc.keepalive_timeout_ms": 10000, - }.items() + "grpc.max_send_message_length": -1, + "grpc.max_receive_message_length": -1, + "grpc.keepalive_time_ms": 30000, + "grpc.keepalive_timeout_ms": 10000, + }.items() if self._client_options and self._client_options.api_endpoint: api_endpoint = self._client_options.api_endpoint else: @@ -257,9 +262,7 @@ def _create_gapic_client_channel(self, client_class, grpc_transport): channel = self._emulator_channel(grpc_transport, options) else: channel = grpc_transport.create_channel( - host=api_endpoint, - credentials=self._credentials, - options=options, + host=api_endpoint, credentials=self._credentials, options=options, ) transport = grpc_transport(channel=channel, host=api_endpoint) return transport diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 5f2d7db26..c39b05571 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -67,16 +67,24 @@ def test_w_emulator(self): client_class = mock.Mock() emulator_host = emulator_channel = object() credentials = _make_credentials() + + client_options = mock.Mock() + transport = mock.Mock() + client = _Client( credentials, emulator_host=emulator_host, emulator_channel=emulator_channel ) client_info = client._client_info = mock.Mock() - - result = self._invoke_client_factory(client_class)(client) + result = self._invoke_client_factory( + client_class, client_options=client_options, transport=transport + )(client) self.assertIs(result, client_class.return_value) client_class.assert_called_once_with( - channel=client._emulator_channel, client_info=client_info + credentials=None, + client_info=client_info, + client_options=client_options, + transport=transport, ) @@ -121,7 +129,6 @@ def test_constructor_defaults(self): self.assertIs(client._client_info, _CLIENT_INFO) self.assertIsNone(client._channel) self.assertIsNone(client._emulator_host) - self.assertIsNone(client._emulator_channel) self.assertEqual(client.SCOPE, (DATA_SCOPE,)) def test_constructor_explicit(self): @@ -167,22 +174,23 @@ def test_constructor_with_emulator_host(self): credentials = _make_credentials() emulator_host = "localhost:8081" - with mock.patch("os.getenv") as getenv: - getenv.return_value = emulator_host - with mock.patch("grpc.insecure_channel") as factory: - getenv.return_value = emulator_host + with mock.patch("os.environ", {BIGTABLE_EMULATOR: emulator_host}): + with mock.patch("grpc.secure_channel") as factory: client = self._make_one(project=self.PROJECT, credentials=credentials) + # don't test local_composite_credentials + client._local_composite_credentials = lambda: credentials + # channels are formed when needed, so access a client + # create a gapic channel + client.table_data_client self.assertEqual(client._emulator_host, emulator_host) - self.assertIs(client._emulator_channel, factory.return_value) - factory.assert_called_once_with( - target=emulator_host, - options={ - "grpc.keepalive_time_ms": 30000, - "grpc.keepalive_timeout_ms": 10000, - }.items(), - ) - getenv.assert_called_once_with(BIGTABLE_EMULATOR) + options = { + "grpc.max_send_message_length": -1, + "grpc.max_receive_message_length": -1, + "grpc.keepalive_time_ms": 30000, + "grpc.keepalive_timeout_ms": 10000, + }.items() + factory.assert_called_once_with(emulator_host, credentials, options=options) def test__get_scopes_default(self): from google.cloud.bigtable.client import DATA_SCOPE From f7fe18a9484f07f7fcda293397665cdccd2b23a6 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Thu, 18 Mar 2021 09:20:54 -0700 Subject: [PATCH 04/13] test: add test run for emulated testing --- noxfile.py | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/noxfile.py b/noxfile.py index 72b387570..8360e5c17 100644 --- a/noxfile.py +++ b/noxfile.py @@ -33,6 +33,7 @@ # 'docfx' is excluded since it only needs to run in 'docs-presubmit' nox.options.sessions = [ "unit", + "system_emulated", "system", "cover", "lint", @@ -110,13 +111,38 @@ def unit(session): """Run the unit test suite.""" default(session) +@nox.session(python="3.8") +def system_emulated(session): + import subprocess + + try: + subprocess.call(["gcloud", '--version']) + except OSError: + session.skip("gcloud not found but required for emulator support") + + hostport = "localhost:8789" + p = subprocess.Popen([ + "gcloud", "beta", "emulators", "bigtable", "start", + "--host-port", hostport + ]) + + system(session, emulator=hostport) + + # Stop Emulator + p.send_signal(1) @nox.session(python=SYSTEM_TEST_PYTHON_VERSIONS) -def system(session): +def system(session, emulator=False): """Run the system test suite.""" system_test_path = os.path.join("tests", "system.py") system_test_folder_path = os.path.join("tests", "system") + if emulator: + os.environ["BIGTABLE_EMULATOR_HOST"] = emulator + elif os.environ.get("BIGTABLE_EMULATOR_HOST"): + del os.environ["BIGTABLE_EMULATOR_HOST"] + + # Check the value of `RUN_SYSTEM_TESTS` env var. It defaults to true. if os.environ.get("RUN_SYSTEM_TESTS", "true") == "false": session.skip("RUN_SYSTEM_TESTS is set to false, skipping") From ced96202d7e129247ee304dea198d290e5f06b38 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Thu, 18 Mar 2021 09:52:18 -0700 Subject: [PATCH 05/13] test: simplify emulator wrapping, ensure env var is passed --- noxfile.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/noxfile.py b/noxfile.py index 8360e5c17..8698a8e11 100644 --- a/noxfile.py +++ b/noxfile.py @@ -114,6 +114,7 @@ def unit(session): @nox.session(python="3.8") def system_emulated(session): import subprocess + import signal try: subprocess.call(["gcloud", '--version']) @@ -126,23 +127,18 @@ def system_emulated(session): "--host-port", hostport ]) - system(session, emulator=hostport) + session.env["BIGTABLE_EMULATOR_HOST"] = hostport + system(session) # Stop Emulator - p.send_signal(1) + os.killpg(os.getpgid(p.pid), signal.SIGTERM) @nox.session(python=SYSTEM_TEST_PYTHON_VERSIONS) -def system(session, emulator=False): +def system(session): """Run the system test suite.""" system_test_path = os.path.join("tests", "system.py") system_test_folder_path = os.path.join("tests", "system") - if emulator: - os.environ["BIGTABLE_EMULATOR_HOST"] = emulator - elif os.environ.get("BIGTABLE_EMULATOR_HOST"): - del os.environ["BIGTABLE_EMULATOR_HOST"] - - # Check the value of `RUN_SYSTEM_TESTS` env var. It defaults to true. if os.environ.get("RUN_SYSTEM_TESTS", "true") == "false": session.skip("RUN_SYSTEM_TESTS is set to false, skipping") From a05f2d346c8b4ce1ead85f1eb425a8a99c5966fe Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Thu, 18 Mar 2021 10:00:23 -0700 Subject: [PATCH 06/13] test(temporary?): use normal cred flow, not emulatorCreds --- tests/system.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/system.py b/tests/system.py index 84f9977e1..28937ce70 100644 --- a/tests/system.py +++ b/tests/system.py @@ -115,8 +115,12 @@ def setUpModule(): Config.IN_EMULATOR = os.getenv(BIGTABLE_EMULATOR) is not None if Config.IN_EMULATOR: - credentials = EmulatorCreds() - Config.CLIENT = Client(admin=True, credentials=credentials) + # I expect users won't always pass creds, just use the usual cred flow + # on creation. + # credentials = EmulatorCreds() + # Config.CLIENT = Client(admin=True, credentials=credentials) + Config.CLIENT = Client(admin=True) + else: Config.CLIENT = Client(admin=True) From 7e4bb8e3db570370832e32a18c20adf48e3285d4 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Thu, 18 Mar 2021 10:19:30 -0700 Subject: [PATCH 07/13] test: install beta gcloud component --- noxfile.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/noxfile.py b/noxfile.py index 8698a8e11..5bdddfd91 100644 --- a/noxfile.py +++ b/noxfile.py @@ -121,6 +121,9 @@ def system_emulated(session): except OSError: session.skip("gcloud not found but required for emulator support") + # Currently, CI/CD doesn't have beta component of gcloud. + subprocess.call(["gcloud", "components", "install", "beta"]) + hostport = "localhost:8789" p = subprocess.Popen([ "gcloud", "beta", "emulators", "bigtable", "start", From 34eee818881ef6792dd8e81d3cb32803582996d8 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Thu, 18 Mar 2021 10:42:11 -0700 Subject: [PATCH 08/13] test: install bigtable gcloud component --- noxfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noxfile.py b/noxfile.py index 5bdddfd91..3b36c9325 100644 --- a/noxfile.py +++ b/noxfile.py @@ -122,7 +122,7 @@ def system_emulated(session): session.skip("gcloud not found but required for emulator support") # Currently, CI/CD doesn't have beta component of gcloud. - subprocess.call(["gcloud", "components", "install", "beta"]) + subprocess.call(["gcloud", "components", "install", "beta", "bigtable"]) hostport = "localhost:8789" p = subprocess.Popen([ From 4133c065f72ed64cbda8a6ab314519d0525fa232 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Thu, 18 Mar 2021 10:58:26 -0700 Subject: [PATCH 09/13] test: skip backups on emulator --- tests/system.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/system.py b/tests/system.py index 28937ce70..0f4871aef 100644 --- a/tests/system.py +++ b/tests/system.py @@ -844,6 +844,7 @@ def test_delete_column_family(self): self.assertEqual(temp_table.list_column_families(), {}) def test_backup(self): + self._maybe_emulator_skip("backups are not supported in the emulator") from google.cloud._helpers import _datetime_to_pb_timestamp temp_table_id = "test-backup-table" From 3e4c72ccee3c0b19bc05b7ea8e07a6a3250849ba Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Thu, 18 Mar 2021 11:12:38 -0700 Subject: [PATCH 10/13] test: skip backups on emulator --- tests/system.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/system.py b/tests/system.py index 0f4871aef..7faa07981 100644 --- a/tests/system.py +++ b/tests/system.py @@ -844,7 +844,9 @@ def test_delete_column_family(self): self.assertEqual(temp_table.list_column_families(), {}) def test_backup(self): - self._maybe_emulator_skip("backups are not supported in the emulator") + if Config.IN_EMULATOR: + self.skipTest("backups are not supported in the emulator") + from google.cloud._helpers import _datetime_to_pb_timestamp temp_table_id = "test-backup-table" From b1f8c6c4cb3d02f828cf604b6485504835bd5047 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Thu, 18 Mar 2021 11:27:46 -0700 Subject: [PATCH 11/13] chore: linter --- noxfile.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/noxfile.py b/noxfile.py index 3b36c9325..84fbd0583 100644 --- a/noxfile.py +++ b/noxfile.py @@ -111,13 +111,14 @@ def unit(session): """Run the unit test suite.""" default(session) + @nox.session(python="3.8") def system_emulated(session): import subprocess import signal - + try: - subprocess.call(["gcloud", '--version']) + subprocess.call(["gcloud", "--version"]) except OSError: session.skip("gcloud not found but required for emulator support") @@ -125,10 +126,9 @@ def system_emulated(session): subprocess.call(["gcloud", "components", "install", "beta", "bigtable"]) hostport = "localhost:8789" - p = subprocess.Popen([ - "gcloud", "beta", "emulators", "bigtable", "start", - "--host-port", hostport - ]) + p = subprocess.Popen( + ["gcloud", "beta", "emulators", "bigtable", "start", "--host-port", hostport] + ) session.env["BIGTABLE_EMULATOR_HOST"] = hostport system(session) @@ -136,6 +136,7 @@ def system_emulated(session): # Stop Emulator os.killpg(os.getpgid(p.pid), signal.SIGTERM) + @nox.session(python=SYSTEM_TEST_PYTHON_VERSIONS) def system(session): """Run the system test suite.""" From ea6042e41b36da98ad74ac39cee659ef7459ccaa Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Thu, 18 Mar 2021 11:54:00 -0700 Subject: [PATCH 12/13] chore: linter --- tests/system.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/system.py b/tests/system.py index 7faa07981..d3c015dbd 100644 --- a/tests/system.py +++ b/tests/system.py @@ -24,7 +24,8 @@ from google.cloud.environment_vars import BIGTABLE_EMULATOR from test_utils.retry import RetryErrors from test_utils.retry import RetryResult -from test_utils.system import EmulatorCreds + +# from test_utils.system import EmulatorCreds from test_utils.system import unique_resource_id from google.cloud._helpers import _datetime_from_microseconds From 057e329f1fb6396083db4806674bfc66dd3ade95 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Fri, 19 Mar 2021 10:47:49 -0700 Subject: [PATCH 13/13] chore: cleanup --- tests/system.py | 12 +++--------- tests/unit/test_client.py | 1 - 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/tests/system.py b/tests/system.py index d3c015dbd..21a39eb29 100644 --- a/tests/system.py +++ b/tests/system.py @@ -115,15 +115,9 @@ def setUpModule(): Config.IN_EMULATOR = os.getenv(BIGTABLE_EMULATOR) is not None - if Config.IN_EMULATOR: - # I expect users won't always pass creds, just use the usual cred flow - # on creation. - # credentials = EmulatorCreds() - # Config.CLIENT = Client(admin=True, credentials=credentials) - Config.CLIENT = Client(admin=True) - - else: - Config.CLIENT = Client(admin=True) + # Previously we created clients using a mock EmulatorCreds when targeting + # an emulator. + Config.CLIENT = Client(admin=True) Config.INSTANCE = Config.CLIENT.instance(INSTANCE_ID, labels=LABELS) Config.CLUSTER = Config.INSTANCE.cluster( diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index c39b05571..f6b8eb5bc 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -67,7 +67,6 @@ def test_w_emulator(self): client_class = mock.Mock() emulator_host = emulator_channel = object() credentials = _make_credentials() - client_options = mock.Mock() transport = mock.Mock()