Auto-detect account regional namespace buckets in s3 mb#10212
Auto-detect account regional namespace buckets in s3 mb#10212AndrewAsseily wants to merge 4 commits intov2from
Conversation
…tNamespace header
aemous
left a comment
There was a problem hiding this comment.
Left some comments.
Also, consider adding ~1 integration test. We are making some assumption on server behavior so it may be worth baking that into tests. Such as, attempt to create a bucket with name xyz-an, and expect s3 to return an error that the formatting is wrong.
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "type": "enhancement", | |||
| "category": "s3", | |||
There was a problem hiding this comment.
| "category": "s3", | |
| "category": "``S3``", |
| { | ||
| "type": "enhancement", | ||
| "category": "s3", | ||
| "description": "Add support for creating S3 account regional namespace buckets with aws s3 mb. The command now automatically detects bucket names matching the account-regional naming pattern and sets the required x-amz-bucket-namespace header." |
There was a problem hiding this comment.
| "description": "Add support for creating S3 account regional namespace buckets with aws s3 mb. The command now automatically detects bucket names matching the account-regional naming pattern and sets the required x-amz-bucket-namespace header." | |
| "description": "Add support for creating S3 account regional namespace buckets with ``aws s3 mb``. The command now automatically detects bucket names matching the account-regional naming pattern and sets the required ``x-amz-bucket-namespace`` header." |
| ) | ||
|
|
||
| _S3_ACCOUNT_REGIONAL_NAMESPACE_REGEX = re.compile( | ||
| r'^.+-\d{12}-[a-z]{2}(-[a-z]+-\d+)?-an$' |
There was a problem hiding this comment.
I have concerns for this regex.
According to the S3 docs:
Bucket names can only end with the suffix -an when you are creating buckets in your account regional namespace. For more information, see Namespaces for general purpose buckets.
If our mb command is making a best-effort attempt at creating buckets, then I think we should always add the regional namespace header when the suffix is -an. This would match Console behavior, the front-end does client-side validation that if it ends with -an it must be for regional account namespace.
S3 will not allow users to create a bucket that ends with -an unless the header is provided, so I think it is on us to provide the header whenever that suffix is provided.
For example, I pulled down your code and tried this command:
$ aws s3 mb s3://xyz-an
make_bucket failed: s3://xyz-an An error occurred (MissingNamespaceHeader) when calling the CreateBucket operation: The requested bucket is an account-regional namespace bucket, but your request is missing the required x-amz-bucket-namespace header.
I feel this error should not be encountered by our users, since we are in charge of supplying headers.
If the rest of the formatting doesn't match the expected format (e.g user gives bucket name xyz-an), S3 will give an API error that the name doesn't match the format. In my view that's ideal since they should remain the authoritative source of truth on the right format, rather than us trying to duplicate their formatting logic in the client.
This would also match how we handle S3 express buckets, we only check for the -x-s3 suffix, even though S3 requires a zone too.
|
|
||
| def test_account_regional_namespace_bucket(self): | ||
| bucket = 'amzn-s3-demo-bucket-111122223333-us-west-2-an' | ||
| command = self.prefix + 's3://%s --region us-west-2' % bucket |
There was a problem hiding this comment.
can we use f-strings on new code instead of formatting operator?
| } | ||
| self.assert_params_for_cmd(command, expected_params) | ||
|
|
||
| def test_regular_bucket_no_namespace(self): |
There was a problem hiding this comment.
worth adding a test for bucket name xyz-an.
Issue #, if available:
Description of changes:
The s3 mb command now automatically detects account regional namespace bucket names (matching *---an) and sets the x-amz-bucket-namespace: account-regional header on the CreateBucket request. Previously, users had to fall back to s3api create-bucket --bucket-namespace for
this workflow.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.