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

Allow custom endpoints with Google Cloud Storage. #648

Merged
merged 1 commit into from Nov 21, 2019

Conversation

martey
Copy link
Contributor

@martey martey commented Jan 11, 2019

Add new GS_CUSTOM_ENDPOINT setting to allow usage of custom domains/URLs with Google Cloud Storage, similar to AWS_S3_CUSTOM_DOMAIN.

@martey martey force-pushed the gcs-custom-endpoint branch 2 times, most recently from 5b42349 to cb0c0c4 Compare January 11, 2019 16:04
@sww314 sww314 added the google label Apr 3, 2019
@martey
Copy link
Contributor Author

martey commented Apr 20, 2019

I've updated this commit so that it is compatible with google-cloud-storage 1.15.0 (the addition of support for v4 signed URLs broke one of the imports added here). Because v4 signed URLs are still in beta, this commit still uses v2 signed URLs.

I've updated the google-cloud-storage minimum version to 1.15.0, but I can change it to be backwards compatible if necessary.

@martey
Copy link
Contributor Author

martey commented May 4, 2019

Hi @jschneier, can you let me know if there is anything else I should do to help this get merged?

@hiimdoublej
Copy link

+1 would like this as well @jschneier

@zjsvv
Copy link

zjsvv commented Aug 2, 2019

Hi, @sww314 and @jschneier, we are using this PR on production for a couple of months, and it works well. Can you give some advices to help this get merged? Thanks.

@martey
Copy link
Contributor Author

martey commented Sep 25, 2019

I've rebased this so that it can merge cleanly with master. I would still like this to be merged, and would appreciate feedback from this library's maintainers.

@andruten
Copy link

I'm also interested in this feature!

@martey
Copy link
Contributor Author

martey commented Nov 15, 2019

I've updated this to fix a bug with generating signed URLs and to stop using django.utils.six.

@@ -19,7 +19,7 @@ def read(filename):
'boto': ['boto>=2.32.0'],
'boto3': ['boto3>=1.4.4'],
'dropbox': ['dropbox>=7.2.1'],
'google': ['google-cloud-storage>=0.22.0'],
'google': ['google-cloud-storage>=1.15.0'],
Copy link
Owner

Choose a reason for hiding this comment

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

Is this change required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code in this pull request relies on the fact that Blob.generate_signed_url has a api_access_endpoint parameter (introduced in googleapis/google-cloud-python#7460 and google-cloud-storage 1.15.0).

It would be theoretically possible to update the code so that it is still compatible with google-cloud-storage 0.22.0, but because of the substantial differences in google-cloud-storage pre- and post-1.15.0, this would take a substantial amount of work and would significantly increase the complexity of this pull request's code.

Copy link
Owner

Choose a reason for hiding this comment

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

That change was merged in April. I'm not sure if I can unilaterally require that upgrade.

Is the major difference that non-public URLs wouldn't be able to be signed?

@jschneier
Copy link
Owner

I see there's quite a bit of demand for this, what did a previous iteration look like that didn't mandate the version upgrade? I want to merge some variant of this.

@martey
Copy link
Contributor Author

martey commented Nov 18, 2019

I see there's quite a bit of demand for this, what did a previous iteration look like that didn't mandate the version upgrade?

The "previous iteration" from January of this year (cb0c0c4) was created before google-cloud-storage 0.15.0 existed, and isn't compatible with any releases >= 0.15.0. It wouldn't be a valid option unless we wanted to require users have a version of google-cloud-storage before 0.15.0 or (as you mentioned in your earlier comment) were willing to prevent non-public URLs from working.

I am unsure about whether backporting these changes to google-cloud-storage < 0.15.0 would be successful. The version requirement on google-cloud-storage in setup.py encourages installing the most recent version of google-cloud-storage, so the automated tests don't cover using an older version of google-cloud-storage and it's possible that there are additional hidden bugs or problems in those older versions. It's also not clear that older versions like 0.22.0 are supported by Google (google-cloud-storage's changelog starts with version 1.6.0).

Personally, I don't think mandating a library upgrade is a huge ask for a new feature, especially when it would only affect people who have explicitly pinned google-cloud-storage. That said, I am still interested in having this merged, and will try to make whatever changes are necessary to have this happen.

@jschneier
Copy link
Owner

You make good points...especially that our version checking is loose see e.g the failing tests on the PR. That one would have been caught by running the tests every day though.

At some point I will need to rip the bandaid off and remove all of the Python 2 related and deprecated things. Since I have historically been willing to break backwards compat when I increment minor versions I don't think it's too much ask to require it for the next one but it does leave open a question of when that should be done.

Can you please rebase to fix the tests.

@martey martey force-pushed the gcs-custom-endpoint branch 3 times, most recently from ad3bab9 to 8aeda61 Compare November 18, 2019 18:58
Add new `GS_CUSTOM_ENDPOINT` setting to allow usage of custom
domains/URLs with Google Cloud Storage, similar to
`AWS_S3_CUSTOM_DOMAIN`.

This upgrades the minimum version of google-cloud-storage to 1.15.0.
@martey
Copy link
Contributor Author

martey commented Nov 19, 2019

I've rebased this and ensured that all tests pass.

@jschneier jschneier merged commit 888803f into jschneier:master Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants