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.

Sign up for GitHub

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

Jump to bottom

Storage: ensure that 'Blob.reload' passes encryption headers. #7441

Merged
merged 4 commits into from
Mar 14, 2019
Merged

Storage: ensure that 'Blob.reload' passes encryption headers. #7441

merged 4 commits into from
Mar 14, 2019

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Feb 25, 2019

Closes #7440.

@tseaver
Define '_PropertyMixin._encryption_headers' helper method.
d110a4a 
Use it when reloading the instance.
@tseaver
Define 'Blob._encryption_headers'.
3c2748c 
Override to use the blob's '_encryption_key' attribute.

Closes #7440.
@tseaver
Use 'Blob.reload' in 'Bucket.get_blob'.
ab0d1ff
@tseaver tseaver added the api: storage Issues related to the Cloud Storage API. label Feb 25, 2019
@tseaver tseaver requested a review from frankyn February 25, 2019 22:24
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 25, 2019
@tseaver tseaver mentioned this pull request Feb 25, 2019
Merged
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 4, 2019
frankyn
frankyn reviewed Mar 11, 2019
View reviewed changes
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

LGTM, this will allow a .reload() on an encrypted object. Though, I do want to ask if it's reasonable to apply _encryption_headers() check to even buckets given they're not valid.

Future us may ask why it's being used for buckets as well.

@frankyn frankyn removed the 🚨 This issue needs some love. label Mar 11, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 11, 2019
@tseaver
Copy link
Contributor Author

tseaver commented Mar 11, 2019

@frankyn The _PropertyHelper base class defines reload, which calls the _encryption_headers method: it provides a no-op / empty implementation for Bucket (only Blob overrides it).

@frankyn
Copy link
Member

frankyn commented Mar 11, 2019

Thanks @tseaver, could you add a comment near the method stating that it's primary use is for a Blob and the given use case?

@tseaver
Copy link
Contributor Author

tseaver commented Mar 11, 2019

@frankyn

Could you add a comment near the method stating that it's primary use is for a Blob and the given use case?

0c5a4f7

frankyn
frankyn approved these changes Mar 11, 2019
View reviewed changes
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Thanks @tseaver. LGTM, I'd still like to get @crwilcox's review before merging if that's okay.

Chris, could you take a look over before this is merged?

@tseaver
Copy link
Contributor Author

tseaver commented Mar 12, 2019

Unrelated Firestore failure is from a known-flaky systest, see #7130.

@tseaver tseaver merged commit 4fdbcc5 into googleapis:master Mar 14, 2019
@tseaver tseaver deleted the 7440-storage-blob_reload_passes_enc_headers branch March 14, 2019 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crwilcox crwilcox crwilcox approved these changes

@frankyn frankyn frankyn approved these changes

Assignees

@crwilcox crwilcox

Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement. 🚨 This issue needs some love.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tseaver @frankyn @crwilcox @googlebot @yoshi-automation