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

Re-work bucket naming rules. #1279

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

blkmajik
Copy link

@blkmajik blkmajik commented Oct 6, 2022

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.

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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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]))
Copy link
Contributor

Choose a reason for hiding this comment

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

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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

@fviard
Copy link
Contributor

fviard commented Oct 9, 2022

Thank you very much for your PR.
This would be a worthwhile change.

I did a few reviews to improve this change, and afterward I will be happy to merge it.

@fviard fviard added this to the 2.4.0 milestone Oct 9, 2022
@fviard fviard modified the milestones: 2.4.0, 2.5.0 Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants