Re-work bucket naming rules. #1279
Conversation
Add bucket_name_quirks config option (boolean). This allows you to use a colon in the bucket name for Ceph compatibility with tenants. Since it's after March 1, 2018 apply the bucket naming rules for bucket creation across the board. Still allow dns_strict to be used if we ever check bucket names for not creation events. See https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html for details.
| raise S3.Exceptions.ParameterError("Bucket name '%s' must end with a letter or a digit" % bucket) | ||
| if len(bucket) > max_length: | ||
| raise S3.Exceptions.ParameterError("Bucket name '%s' is too long (max %d characters)" % (bucket, max_length)) | ||
| if re.search("-\.", bucket, re.UNICODE): |
There was a problem hiding this comment.
You can profit of the rework to simplify this to:
if '-.' in bucket
| raise S3.Exceptions.ParameterError("Bucket name '%s' is too long (max %d characters)" % (bucket, max_length)) | ||
| if re.search("-\.", bucket, re.UNICODE): | ||
| raise S3.Exceptions.ParameterError("Bucket name '%s' must not contain sequence '-.' for DNS compatibility" % bucket) | ||
| if re.search("\.\.", bucket, re.UNICODE): |
There was a problem hiding this comment.
Same a previous one, you could probably simplify that in:
if '..' in bucket
And maybe merge the 2 tests in fact!
| raise S3.Exceptions.ParameterError("Bucket name '%s' contains disallowed character '%s'. The only supported ones are: us-ascii letters (a-z, A-Z), digits (0-9), dot (.), hyphen (-), colon (:), and underscore (_)." % (bucket, invalid.groups()[0])) | ||
| elif dns_strict: | ||
| # This is the default | ||
| max_length = 63 | ||
| invalid = re.search("([^a-z0-9\.-])", bucket, re.UNICODE) | ||
| if invalid: | ||
| raise S3.Exceptions.ParameterError("Bucket name '%s' contains disallowed character '%s'. The only supported ones are: lowercase us-ascii letters (a-z), digits (0-9), dot (.) and hyphen (-)." % (bucket, invalid.groups()[0])) |
There was a problem hiding this comment.
Maybe you can profit of your PR to change the following in this file:
import S3.Exceptions into from S3.Exceptions import ParameterError
And so simplify all of its usage here.
| @@ -234,8 +234,30 @@ def time_to_epoch(t): | |||
| raise S3.Exceptions.ParameterError('Unable to convert %r to an epoch time. Pass an epoch time. Try `date -d \'now + 1 year\' +%%s` (shell) or time.mktime (Python).' % t) | |||
|
|
|||
|
|
|||
| def check_bucket_name(bucket, dns_strict=True): | |||
| if dns_strict: | |||
| def check_bucket_name(bucket, dns_strict=True, name_quirks=False): | |||
There was a problem hiding this comment.
Now that us-east-1 also doesn't support the previous set of extended chars, I think that it is ok to not add your "name_quirks" and just add the ":" inside the char list checked when not in "dns_strict" mode.
btw, can you escape the "-" inside the regexp, to be able to have it before the additional characters, and also avoid errors in the future?
| @@ -133,6 +133,8 @@ class Config(object): | |||
| force = False | |||
| server_side_encryption = False | |||
| enable = None | |||
| # Used to allow colons in bucket names for Ceph compatibility | |||
| bucket_name_quirks = False | |||
There was a problem hiding this comment.
I don't like so much "quirks" and to restrict that to colons and Ceph.
Maybe you can call that something like: "bucket_name_extended", "extended_bucket_name", "bucket_name_relaxed", "bucket_name_lenient".
or invert the condition like:
"bucket_name_strict = True",
And in the comment say that it relax rules for bucket name for S3 compatible services that supports path style requests.
| else: | ||
| check_bucket_name(bucket, dns_strict = False) | ||
| if bucket_location: |
There was a problem hiding this comment.
The CreateBucketConfiguration should be set in all cases, that we use "name quirks" or not.
For the moment, keep the existing code but remove the "check_bucket_name" of this block.
(In the future, I will change the EU/US thing to avoid some specific issues, but it can be done after).
After the previous block, add a new block of code:
if self.config.bucket_name_quirks:
checkbucket(dns_strict = False)
else:
check_bucket(dns_strict= True)
|
Thank you very much for your PR. I did a few reviews to improve this change, and afterward I will be happy to merge it. |
Add bucket_name_quirks config option (boolean). This allows you to use a colon in the bucket name for Ceph compatibility with tenants.
Since it's after March 1, 2018 apply the bucket naming rules for bucket creation across the board. Still allow dns_strict to be used if we ever check bucket names for not creation events. See
https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html
for details.