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: add preconditions and retry config support to ACL patch operationss #586

Merged
merged 9 commits into from Sep 15, 2021

Conversation

@cojenco
Copy link
Contributor

@cojenco cojenco commented Sep 13, 2021

Add preconditions and retry config support to the ACL patch operations. The PATCH API requests for ACL in python-storage utilize storage.buckets.patch or storage.objects.patch

  • ACL patch methods in ACL (_save /save /save_predefined /clear)
  • ACL patch methods in Bucket (make_public /make_private)
  • ACL patch methods in Blob (make_public /make_private)
  • Add and update tests
  • Update doscstrings

Note: IAM configurations storage.buckets.getIamPolicy storage.buckets.setIamPolicy are highly encouraged to control access instead

Fixes #582

@google-cla google-cla bot added the cla: yes label Sep 13, 2021
@cojenco cojenco marked this pull request as ready for review Sep 13, 2021
@cojenco cojenco requested review from as code owners Sep 13, 2021
Copy link
Contributor

@tritone tritone left a comment

Thanks for adding these!

Copy link
Contributor

@tseaver tseaver left a comment

We patch ACLs using the objectAccessControls.pach and defaultObjectAccessControls.pach methods, which do not AFAICT take the metageneration / generation into account: instead, they rely on the resource's Etag for concurrency. Note the absence of the if_*_match parameters on those docs page.

@tseaver
Copy link
Contributor

@tseaver tseaver commented Sep 14, 2021

At a minimum, if the back-end docs are incomplete, and the back-end actually supports these semantics, we need to add one or more system tests which exercise them.

@cojenco
Copy link
Contributor Author

@cojenco cojenco commented Sep 14, 2021

@tseaver We patch ACLs here using storage.buckets.patch and storage.objects.patch, instead of ACL specific endpoints (such as storage.object_acl.patch). See here that we're using bucket and object paths for patching resources.

You are right about storage.object_acl.patch storage.default_object_acl.patch relying on the resource's Etag for concurrency.

That is actually one of the considerations why storage.buckets.patch and storage.objects.patch are used here instead. If you recall our previous discussion, the If-Match header that specifies an entity tag does not work for patch operations (internal tracking bug).

Since we are using storage.buckets.patch and storage.objects.patch under the hood, it makes sense to expose the related if_*_match parameters.

@tseaver
Copy link
Contributor

@tseaver tseaver commented Sep 14, 2021

My bad -- I had talked myself into "remembering" the need to back out some similar changes for ACLs in past PRs. You are indeed correct that we do not use the ACL specific APIs, and so should be free to use the if-*-match headers.

@tseaver
Copy link
Contributor

@tseaver tseaver commented Sep 14, 2021

I do think we should add a system test which exercises the new headers, though.

@cojenco cojenco merged commit 4333caf into googleapis:main Sep 15, 2021
11 checks passed
cojenco added a commit to cojenco/python-storage that referenced this issue Sep 15, 2021
…nss (googleapis#586)

* add preconditions and retry config support to ACL patch operations

* update existing unit tests

* add unit tests

* add preconditions and retry config to bucket make public/private

* add preconditions and retry config to blob make public/private

* update docstrings

* add system tests acl with metegeneration match

* revise to use permitted group email
@cojenco cojenco deleted the update-acl branch Sep 17, 2021
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
…nss (googleapis#586)

* add preconditions and retry config support to ACL patch operations

* update existing unit tests

* add unit tests

* add preconditions and retry config to bucket make public/private

* add preconditions and retry config to blob make public/private

* update docstrings

* add system tests acl with metegeneration match

* revise to use permitted group email
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
…nss (googleapis#586)

* add preconditions and retry config support to ACL patch operations

* update existing unit tests

* add unit tests

* add preconditions and retry config to bucket make public/private

* add preconditions and retry config to blob make public/private

* update docstrings

* add system tests acl with metegeneration match

* revise to use permitted group email
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants