Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(bigquery): expose customer managed encryption key for ML models #9302

Conversation

HemangChothani
Copy link
Contributor

@HemangChothani HemangChothani commented Sep 26, 2019

Fixes: #9251

@HemangChothani HemangChothani requested a review from a team September 26, 2019 11:18
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 26, 2019
@@ -25,6 25,7 @@
from google.api_core import datetime_helpers
from google.cloud.bigquery import _helpers
from google.cloud.bigquery_v2 import types
from google.cloud.bigquery.table import EncryptionConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems wrong for EncryptionConfiguration to live in the table module now that it's used for tables and models. Let's create a new encryption_configuration model for this to live. (Similar to external_config.)

We can make an alias in the table module to keep this from being a breaking change.

@@ -21,6 21,8 @@
import google.cloud._helpers
from google.cloud.bigquery_v2.gapic import enums

KMS_KEY_NAME = "projects/1/locations/global/keyRings/1/cryptoKeys/1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a unit test, but we're supposed to discourage use of global location. Let's change this to us.

@HemangChothani HemangChothani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 1, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 1, 2019
@HemangChothani
Copy link
Contributor Author

@tswast PTAL.

@@ -70,99 70,6 @@ def _verifySchema(self, schema, resource):
self._verify_field(field, r_field)


class TestEncryptionConfiguration(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please keep one test to ensure that from google.cloud.bigquery.table import EncryptionConfiguration continues to work? I don't want to make this feature into breaking change.

@@ -1118,7 1046,9 @@ def test_clustering_fields_setter_w_none_noop(self):
self.assertFalse("clustering" in table._properties)

def test_encryption_configuration_setter(self):
from google.cloud.bigquery.table import EncryptionConfiguration
from google.cloud.bigquery.encryption_configuration import (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that we need to test that the old location needs to continue to work.

Suggested change
from google.cloud.bigquery.encryption_configuration import (
# Previously, the EncryptionConfiguration class was in the table module, not the
# encryption_configuration module. It was moved to support models encryption.
# This test import from the table module to ensure that the previous location
# continues to function as an alias.
from google.cloud.bigquery.table import (

@tswast tswast merged commit 1dd4731 into googleapis:master Oct 16, 2019
@HemangChothani HemangChothani deleted the Bigquery_expose_CMEK_functionality_for_ML_models branch December 17, 2019 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BigQuery: expose CMEK functionality for ML models
4 participants