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

@HemangChothani HemangChothani commented Apr 15, 2020

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
Copy link
Member

@frankyn frankyn left a comment

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

@frankyn frankyn Apr 16, 2020

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?

blob._API_ACCESS_ENDPOINT = api_endpoint
bucket._API_ACCESS_ENDPOINT = api_endpoint
Copy link
Member

@frankyn frankyn Apr 16, 2020

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 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

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

@frankyn frankyn Apr 17, 2020

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

@frankyn frankyn Apr 17, 2020

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

@HemangChothani HemangChothani Apr 20, 2020

_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 Apr 17, 2020
Copy link
Member

@frankyn frankyn left a comment

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

@frankyn frankyn Apr 22, 2020

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

Copy link
Member

@frankyn frankyn Apr 22, 2020

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

Copy link
Contributor Author

@HemangChothani HemangChothani Apr 23, 2020

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

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

@HemangChothani pending tests passing, but LGTM

@frankyn frankyn requested a review from crwilcox Apr 24, 2020
@frankyn
Copy link
Member

@frankyn 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
3 checks passed
cojenco added a commit to cojenco/python-storage that referenced this issue 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 added a commit to cojenco/python-storage that referenced this issue 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants