[Storage] Fix public bucket source check in SkyPilot Storage #1087
Conversation
| # These buckets are known to be private, but may need to be updated if | ||
| # they are removed by their owners. | ||
| with pytest.raises(sky.exceptions.StorageBucketGetError, | ||
| match='the bucket name is taken'): |
There was a problem hiding this comment.
match=backend_utils._BUCKET_FAIL_TO_CONNECT_MESSAGE.format(...)
There was a problem hiding this comment.
Ahh private buckets trigger this message on gcp, so the common message is the bucket name is taken:
Lines 1129 to 1133 in 6196552
There was a problem hiding this comment.
In this case, can we move this message as an variable such as PRIVATE_BUCKET_ERROR?
| @@ -820,7 +820,7 @@ def test_new_bucket_creation_and_deletion(self, tmp_local_storage_obj, | |||
|
|
|||
| @pytest.mark.parametrize( | |||
| 'tmp_public_storage_obj, store_type', | |||
| [('s3://tcga-2-open', storage_lib.StoreType.S3), | |||
There was a problem hiding this comment.
Can we keep this bucket in the test? For coverage.
There was a problem hiding this comment.
Great point - added!
tests/test_smoke.py
Outdated
| else: | ||
| retry_count += 1 | ||
| if retry_count > 3: | ||
| raise RuntimeError('Could find a nonexistent bucket to use.' |
There was a problem hiding this comment.
good catch! fixed
|
@michaelzhiluo ready for another look! |
|
This PR looks good to me, as long as above comment is resolved! |
|
@michaelzhiluo - regarding refactoring error strings, I'll create another PR to fix that since that's tangential to this bugfix - would that be ok? |
|
Sounds good, in that case, feel free to merge! |
Closes #977.
GetPublicAccessBlockis no longer a reliable method to check if a s3 bucket is public. Buckets can be publicly readable yet not allow access toGetPublicAccessBlock. I'm not sure what caused it (and I can't find a clear reason).Some buckets still allow it (e.g.,
s3://tcga-2-open, which is used in our smoke tests and thus was passing), but others have blocked it (e.g.,s3://digitalcorpora,s3://imagenet-bucket).Using
head_bucketAPI now, as recommended by boto3 docs.Also added storage tests for non-existent and private buckets.
Tested:
pytest test_smoke.py::TestStorageWithCredentials