Use more precise does S3 bucket exist method #34123
Conversation
We are using a deprecated method for checking if an S3 bucket exists. This deprecated method has a limitation that it can not distinguish between invalid credentials and a lack of permissions. This commit switches to using a method that correctly surfaces if invalid credentials are supplied when checking for the existence of a bucket.
jasontedor
added
review
:Distributed/Snapshot/Restore
|
Pinging @elastic/es-distributed |
There was a problem hiding this comment.
I don't think that we can or that we should be doing this change: doesBucketExist() is implemented as a HEAD bucket request while doesBucketExistV2() is implemented as GET bucket acl request, which requires different bucket permissions than what we've been recommending. The reason why doesBucketExistV2() got introduced can be found here. I'm not sure if this is the method that we actually want to check. We are less interested in knowing whether the bucket exists (but we might not be able to access it), but rather interested whether it exists and we can access it. The request to check this is just the plain old headBucket request. If you want to improve this code section, I therefore recommend changing it to a plain clientReference.client().headBucket(new HeadBucketRequest(bucket)), then catch the AmazonServiceException and wrap it with an exception saying something along the lines of bucket does not exist or cannot be accessed, and the more detailed cause will then be provided by the AmazonServiceException.
|
Two important things:
|
|
@ywelsch I pushed. |
We are using a deprecated method for checking if an S3 bucket exists. This deprecated method has a limitation that it can not distinguish between invalid credentials and a lack of permissions. This commit switches to using a method that correctly surfaces if invalid credentials are supplied when checking for the existence of a bucket.
* master: Use more precise does S3 bucket exist method (elastic#34123) LLREST: Introduce a strict mode (elastic#33708) [CCR] Adjust list retryable errors (elastic#33985) Fix AggregationFactories.Builder equality and hash regarding order (elastic#34005) MINOR: Remove some deadcode in NodeEnv and Related (elastic#34133) Rest-Api-Spec: Correct spelling in filter_path description (elastic#33154) Core: Don't rely on java time for epoch seconds formatting (elastic#34086) Retry errors when fetching follower global checkpoint. (elastic#34019) Watcher: Reenable watcher stats REST tests (elastic#34107) Remove special-casing of Synonym filters in AnalysisRegistry (elastic#34034) Rename CCR APIs (elastic#34027) Fixed CCR stats api serialization issues and (elastic#33983) Support 'string'-style queries on metadata fields when reasonable. (elastic#34089) Logging: Drop Settings from security logger get calls (elastic#33940) SQL: Internal refactoring of operators as functions (elastic#34097)
We are using a deprecated method for checking if an S3 bucket exists. This deprecated method has a limitation that it can not distinguish between invalid credentials and a lack of permissions. This commit switches to using a method that correctly surfaces if invalid credentials are supplied when checking for the existence of a bucket.
We are using a deprecated method for checking if an S3 bucket exists. This deprecated method has a limitation that it can not distinguish between invalid credentials and a lack of permissions. This commit switches to using a method that correctly surfaces if invalid credentials are supplied when checking for the existence of a bucket.