Storage: add support for KMS keys by tseaver · Pull Request #5259 · googleapis/google-cloud-python

tseaver · 2018-04-27T21:50:04Z

Merge is blocked for public beta of the KMS support. We will need to adjust the system tests at that time to switch from using keys in the global location to keys in the same location as the bucket.

Sign in to view

+    :param kms_key_name:
+        Optional resource name of Cloud KMS key used to encrypt the blob's
+        contents.
+        See https://cloud.google.com/storage/docs/encryption#kms.


Sign in to view

+        # We can't verify it, but ideally we would check that the following
+        # URL was resolvable with our credentals
+        # KEY_URL = 'https://cloudkms.googleapis.com/v1/{}'.format(
+        #     kms_key_name)


Sign in to view

+        download_url = blob._get_download_url()
+        expected_url = (
+            'https://www.googleapis.com/download/storage/v1/b/'
+            'buhkit/o/bzzz-fly.txt?alt=media')  # kmsKeyName *not* expected


Sign in to view

+            key_name,
+        )
+
+    def test_blob_w_explicit_kms_key_name(self):


tseaver · 2018-04-30T17:23:33Z

@frankyn PTAL.

frankyn

LGTM, but do not merge and still pending switching to regional keys.

tseaver · 2018-05-07T17:11:40Z

@frankyn FYI I just ran the KMS system tests without changing to regional keys, and they passed. When I switch to regional keys, they fail.

frankyn · 2018-05-07T17:13:28Z

Thanks @tseaver, what is the error you're seeing?

tseaver · 2018-05-07T17:25:22Z

@frankyn The Cloud KMS key resides in a region that is not allowed.

frankyn · 2018-05-07T19:45:51Z

Synced with @tseaver and will look into reproducing error and syncing with GCS team.

googlebot · 2018-05-08T00:30:20Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-05-09T00:01:49Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

frankyn · 2018-05-09T00:03:52Z

I made commits and caused the CLA issue. I'm okay with my contributions being added to the repo.

tseaver · 2018-05-09T20:25:40Z

@frankyn We are good to merge this branch today, right?

frankyn · 2018-05-09T20:49:09Z

I saw the failures in tests were related to logging, and tests for storage weren't ran. I ran tests locally and they passed, but I want to make sure they're not broken for repo system tests.

tseaver · 2018-05-09T20:56:24Z

@frankyn The logging system tests shouldn't even run against this branch. I've run them locally using the same credentials we use on CircleCI, and they pass. I will try rebuilding them.

Backed by '_properties'.

* Plumb 'kms_key_name' param through to 'Bucket.blob'.

Don't highlight absence of 'kmsKeyName' in expected GET request URL.

…blobs'.

googlebot · 2018-05-09T20:58:36Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

tseaver · 2018-05-09T21:00:04Z

I've rebased the branch against current master, hoping that the fix in #5264 will exclude the irrelevant system tests.

googlebot · 2018-05-09T21:01:07Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

frankyn · 2018-05-09T21:01:33Z

Same state as before, but CLA: yes was removed again..

* Add support for blob-level KMS encryption keys (#5221) * Add 'Bucket.default_kms_key_name' property. (#5222) * Expose 'Blob.kms_key_name' as read-only property (#5249) * Add system tests for GCS-KMS integration (#5257)

tseaver added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. api: storage Issues related to the Cloud Storage API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Apr 27, 2018

tseaver requested a review from frankyn April 27, 2018 21:50

tseaver requested a review from lukesneeringer as a code owner April 27, 2018 21:50

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 27, 2018

tseaver mentioned this pull request Apr 27, 2018

Add system tests for GCS-KMS integration #5257

Merged

frankyn suggested changes Apr 28, 2018

View reviewed changes

frankyn approved these changes Apr 30, 2018

View reviewed changes

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels May 8, 2018

frankyn added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels May 9, 2018

tseaver added 6 commits May 9, 2018 16:58

Add support for blob-level KMS encryption keys (#5221)

c7c87f5

Add 'Bucket.default_kms_key_name' property. (#5222)

8d3d3db

Expose 'Blob.kms_key_name' as read-only property (#5249)

16c5f86

Backed by '_properties'.

Add system tests for GCS-KMS integration (#5257)

49cc315

* Plumb 'kms_key_name' param through to 'Bucket.blob'.

Remove link to not-yet-extant GCS-KMS docs.

f44a7a3

Don't highlight absence of 'kmsKeyName' in expected GET request URL.

Ensure 'kms_key_name' is available in blobs fetched via 'Bucket.list_…

512a8fc

…blobs'.

Frank Natividad added 2 commits May 9, 2018 16:58

From global kms keyring to regional kms keyring

5880583

Remove ending forward-slash in resource name

d7843d1

tseaver force-pushed the storage-support_kms_keys-wip branch from 200df7c to d7843d1 Compare May 9, 2018 20:58

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels May 9, 2018

frankyn added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels May 9, 2018

tseaver merged commit 38c3e9a into master May 9, 2018

tseaver deleted the storage-support_kms_keys-wip branch May 9, 2018 21:50

Conversation

tseaver commented Apr 27, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

tseaver commented Apr 30, 2018

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

tseaver commented May 7, 2018

Uh oh!

frankyn commented May 7, 2018

Uh oh!

tseaver commented May 7, 2018

Uh oh!

frankyn commented May 7, 2018

Uh oh!

googlebot commented May 8, 2018

Uh oh!

googlebot commented May 9, 2018

Uh oh!

frankyn commented May 9, 2018

Uh oh!

tseaver commented May 9, 2018

Uh oh!

frankyn commented May 9, 2018

Uh oh!

tseaver commented May 9, 2018

Uh oh!

googlebot commented May 9, 2018

Uh oh!

tseaver commented May 9, 2018

Uh oh!

googlebot commented May 9, 2018

Uh oh!

frankyn commented May 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants