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

Remove GCS Bucket Exists Check #60899

Merged

Conversation

original-brownbear
Copy link
Member

Same as #43288 for GCS.
We don't need to do the bucket exists check before using the repo, that just needlessly
increases the necessary permissions for using the GCS repository.
The change in what LIST API to use also reduces the number of API calls the SDK makes for each listing by one GET.

Slightly adjusted the way verification exceptions are handled also because the GCS SDK does throw RuntimeException instead of an IOException if a bucket does not exist and we should still bubble up a RepositoryException in this case during verification I think.

Same as elastic#43288 for GCS.
We don't need to do the bucket exists check before using the repo, that just needlessly
increases the necessary permissions for using the GCS repository.
@original-brownbear original-brownbear added >non-issue :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.10.0 labels Aug 10, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team label Aug 10, 2020
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM

@ywelsch ywelsch added the v7.9.1 label Aug 10, 2020
@original-brownbear
Copy link
Member Author

Thanks Yannick!

@original-brownbear original-brownbear merged commit 5829a99 into elastic:master Aug 10, 2020
@original-brownbear original-brownbear deleted the drop-bucket-check-gcs branch August 10, 2020 16:00
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Aug 10, 2020
Same as elastic#43288 for GCS.
We don't need to do the bucket exists check before using the repo, that just needlessly
increases the necessary permissions for using the GCS repository.
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Aug 10, 2020
Same as elastic#43288 for GCS.
We don't need to do the bucket exists check before using the repo, that just needlessly
increases the necessary permissions for using the GCS repository.
original-brownbear added a commit that referenced this pull request Aug 11, 2020
Same as #43288 for GCS.
We don't need to do the bucket exists check before using the repo, that just needlessly
increases the necessary permissions for using the GCS repository.
original-brownbear added a commit that referenced this pull request Aug 11, 2020
Same as #43288 for GCS.
We don't need to do the bucket exists check before using the repo, that just needlessly
increases the necessary permissions for using the GCS repository.
@haizaar
Copy link

haizaar commented Aug 14, 2020

Thanks for implementing this! When it's released I hope we can finally only assign roles/storage.objectAdmin on the bucket level and stop using legacy GCS ASLs (today roles/storage.legacyBucketWriter is the minimum role required).

Do you know if it will make to the next 7.8.x release (if any)?

@original-brownbear
Copy link
Member Author

Hey @haizaar

no problem! I'm afraid this will only make it into 7.9.x though.

@original-brownbear original-brownbear restored the drop-bucket-check-gcs branch December 6, 2020 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >non-issue Team:Distributed Meta label for distributed team v7.9.1 v7.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants