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

fix(storage): anonymous credentials for private bucket #107

Merged
merged 12 commits into from Apr 25, 2020

Conversation

HemangChothani
Copy link
Contributor

Fixes #102

  • We can override the private variables of blob class related to api_endpoint, upload_uri, download_uri.

  • Still confusion with these two parameters:

    1. blob._API_ACCESS_ENDPOINT
    2. bucket._API_ACCESS_ENDPOINT

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 15, 2020
@frankyn frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2020
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 @HemangChothani left some comments.

@@ -122,6 +124,7 @@ def __init__(
if client_options.api_endpoint:
api_endpoint = client_options.api_endpoint
kw_args["api_endpoint"] = api_endpoint
_override_private_url_variables(api_endpoint)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for putting this together. I think the code needs to be refactored for maintainability but the this is a good start.

The URLs in URL Templates could be refactored to be pulled in from the client which is passed around the library.

For example:

base_url = _MULTIPART_URL_TEMPLATE.format(bucket_path=self.bucket.path)

Could be updated to:

_BASE_UPLOAD_TEMPLATE =  u"{hostname}/upload/storage/v1{bucket_path}/o?uploadType="
_MULTIPART_URL_TEMPLATE = _BASE_UPLOAD_TEMPLATE + u"multipart"
# truncated....
base_url = _MULTIPART_URL_TEMPLATE.format(hostname=storage_client._connection.API_BASE_URL,bucket_path=self.bucket.path)

WDYT?

Comment on lines 1071 to 1072
blob._API_ACCESS_ENDPOINT = api_endpoint
bucket._API_ACCESS_ENDPOINT = api_endpoint
Copy link
Member

Choose a reason for hiding this comment

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

These URLs are used for Signature based requests like Signed URLs. These can be left alone because the method has a way to update the hostname because they don't use the same API (Download and Upload use JSON while Signature requests use XML API).

@HemangChothani
Copy link
Contributor Author

HemangChothani commented Apr 16, 2020

@frankyn still we can refactor, need your suggestion.

  • We can remove _get_transport_method() from blob class as we are calling _require_client() method just above _get_transport_method() call, so no need of that method and we can also directly pass client._http.

OR

  • We can modify return type of _get_transport_method method, as of now it returns transport object but also need to return client object as well, so no need to call _require_client() in the respective method where we are using *_URL_TEMPLATE global variables.

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.

Let's keep it the way you have it with one minor nit. This looks much better than before.

Thanks @HemangChothani

@@ -650,19 +648,24 @@ def _get_transport(self, client):
client = self._require_client(client)
return client._http

def _get_download_url(self):
def _get_download_url(self, client):
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this could you use client() that's already available here: https://github.com/googleapis/python-storage/pull/107/files#diff-5d3277a5f0f072a447a1eb89f9fa1ae0R273-R275

Copy link
Member

Choose a reason for hiding this comment

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

this was a bad call, you'll need to do this along with _require_client() otherwise you can end up in a bad state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_require_client() has been call by _get_transport method so moving _get_download_url method call after that.

One system test has failed so fixed that.

@HemangChothani HemangChothani marked this pull request as ready for review April 17, 2020 11:56
@HemangChothani HemangChothani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 21, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 21, 2020
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.

1 more question. Thanks for fixing this @HemangChothani!

@@ -530,7 +530,7 @@ def download_blob_to_file(self, blob_or_uri, file_obj, start=None, end=None):
try:
blob_or_uri.download_to_file(file_obj, client=self, start=start, end=end)
except AttributeError:
blob = Blob.from_string(blob_or_uri)
blob = Blob.from_string(blob_or_uri, client=self)
Copy link
Member

Choose a reason for hiding this comment

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

was this an accidental miss in implementation? Seems like it but want to confirm.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at it again, client is passed below to download_to_file(). The change doesn't look necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the problem arise here , if i didn't pass the client so bucket class has received None

bucket = Bucket(client, name=netloc)

so when call _require_client() method it find the client object which passed in download_to_file() so hostname=self.client._connection.API_BASE_URL line hit the error as self.client found None

Copy link
Member

@frankyn frankyn Apr 24, 2020

Choose a reason for hiding this comment

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

Gotcha, when I raised that you should revert my last comment, I was referring to updating the method _get_download_url() to accept client parameter because even though _get_transport calls _require_client() it doesn't update self.client with a value if it's None.

I think you may want to pass client returned from _require_client() to _get_download_url().

Thank you for your patience.

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.

@HemangChothani pending tests passing, but LGTM

@frankyn frankyn requested a review from crwilcox April 24, 2020 15:41
@frankyn
Copy link
Member

frankyn commented Apr 24, 2020

@HemangChothani asked @crwilcox to take another pass to confirm.

@frankyn frankyn merged commit 6152ab4 into googleapis:master Apr 25, 2020
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* fix(storage): anonymous credentials for private bucket

* fix(storage): use API_BASE_URL for hostname

* fix(storage): nit

* fix(storage): fix system test

* fix(storage): revert changes

Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* fix(storage): anonymous credentials for private bucket

* fix(storage): use API_BASE_URL for hostname

* fix(storage): nit

* fix(storage): fix system test

* fix(storage): revert changes

Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
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.

"Anonymous credentials cannot be refreshed" with fake-gcs-server
5 participants