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

blob/gcsblob: set defaults for SignURL #2800

Merged
merged 2 commits into from Jun 9, 2020
Merged

blob/gcsblob: set defaults for SignURL #2800

merged 2 commits into from Jun 9, 2020

Conversation

mark-kubacki
Copy link
Contributor

@mark-kubacki mark-kubacki commented May 26, 2020

This enables to use blob.SignURL out-of-the-box by setting the required values, primarily the GoogleAccessID and a PrivateKey—where available—, and a default signing behaviour.

By moving the IAM Credentials API client into the bucket, it's henceforth possible to use it in bucket.Options.SignBytes() without any external wrapper.

Previously users ran into SignURL working properly with the likes of S3 without doing anything special, and GCS has been the outlier that needed additional work degrading productivity and developer experience.

closes #2653

@googlebot googlebot added the cla: yes Google CLA has been signed! label May 26, 2020
@mark-kubacki
Copy link
Contributor Author

mark-kubacki commented May 26, 2020

I'd love to get some guidance on how to properly test this, or someone from the team to implement them given the elaborate infrastructure this uses.

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #2800 into master will increase coverage by 0.12%.
The diff coverage is 91.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2800      +/-   ##
==========================================
+ Coverage   68.24%   68.37%   +0.12%     
==========================================
  Files         115      116       +1     
  Lines       13348    13406      +58     
==========================================
+ Hits         9110     9167      +57     
- Misses       3582     3583       +1     
  Partials      656      656              
Impacted Files Coverage Δ
blob/gcsblob/gcsblob.go 87.93% <90.90%> (+0.91%) ⬆️
blob/gcsblob/iam.go 94.44% <94.44%> (ø)
pubsub/rabbitpubsub/rabbit.go 82.08% <0.00%> (+0.65%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f137c7...1d4204e. Read the comment docs.

@mark-kubacki
Copy link
Contributor Author

mark-kubacki commented May 27, 2020

I've dropped detection of the service account on GCE (as GoogleAccessID) from this PR and will file it separately once this is in. It required upgrading to cloud.google.com/go v0.57.0

Copy link
Contributor

@vangent vangent left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

blob/gcsblob/gcsblob.go Outdated Show resolved Hide resolved
blob/gcsblob/gcsblob.go Outdated Show resolved Hide resolved
blob/gcsblob/gcsblob.go Outdated Show resolved Hide resolved
blob/gcsblob/gcsblob.go Outdated Show resolved Hide resolved
blob/gcsblob/gcsblob.go Show resolved Hide resolved
blob/gcsblob/gcsblob.go Outdated Show resolved Hide resolved
@mark-kubacki mark-kubacki requested a review from vangent May 31, 2020 18:31
@mark-kubacki
Copy link
Contributor Author

Not sure what's outstanding – if there's something, please do kindly remind me.

Copy link
Contributor

@vangent vangent left a comment

Choose a reason for hiding this comment

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

Thanks for your patience with this, we're getting close.

blob/gcsblob/gcsblob.go Outdated Show resolved Hide resolved
blob/gcsblob/gcsblob.go Show resolved Hide resolved
blob/gcsblob/gcsblob.go Show resolved Hide resolved
blob/gcsblob/gcsblob.go Outdated Show resolved Hide resolved
blob/gcsblob/gcsblob.go Outdated Show resolved Hide resolved
blob/gcsblob/gcsblob.go Outdated Show resolved Hide resolved
This lifts gcsblob to function parity with other blob implementations
and enables users to skip setting those values in environments
such as AppEngine, where the file is in a well-known location, and
generally whenever a GOOGLE_APPLICATION_CREDENTIALS is set.
@mark-kubacki
Copy link
Contributor Author

Thanks for your patience negotiating this. Tests are in, and I believe we've arrived at an expanded functionality that opens up this to use-cases with improved security.

@mark-kubacki mark-kubacki requested a review from vangent June 7, 2020 20:25
blob/gcsblob/gcsblob.go Outdated Show resolved Hide resolved
blob/gcsblob/iam.go Outdated Show resolved Hide resolved
blob/gcsblob/iam.go Outdated Show resolved Hide resolved
blob/gcsblob/gcsblob.go Outdated Show resolved Hide resolved
blob/gcsblob/iam.go Show resolved Hide resolved
The IAM Service Account Credentials API can be used without a
private key, and to impersonate other accounts.
@mark-kubacki mark-kubacki requested a review from vangent June 9, 2020 00:18
@vangent
Copy link
Contributor

vangent commented Jun 9, 2020

Thanks again for your patience and for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA has been signed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blob/gcsblob: Make GoogleAccessID and PrivateKey optional
3 participants