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

Add a validator for ELB names #615

Merged
merged 3 commits into from
Nov 15, 2016
Merged

Conversation

chrisgilmerproj
Copy link
Contributor

The LoadBalancerName has some constraints that we can validate for ahead of time:

This name must be unique within your set of load balancers for the region, must have a maximum of 32 characters, must contain only alphanumeric characters or hyphens, and cannot begin or end with a hyphen.

From: http://docs.aws.amazon.com/elasticloadbalancing/2012-06-01/APIReference/API_CreateLoadBalancer.html

My current implementation of the regex doesn't quite work. A one character name ought to work but I have created a regex that will fail on names of one character. I could use some help on it.

@@ -73,6 +73,14 @@ def test_s3_bucket_name(self):
with self.assertRaises(ValueError):
s3_bucket_name(b)

def test_elb_name(self):
for b in ['a', 'a'*32, 'wick3d-elb-name', 'Wick3d-ELB-Name']:
Copy link
Member

Choose a reason for hiding this comment

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

Given the regex you might want to add:

a-a
aa
aaa

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 9b55342

def test_elb_name(self):
for b in ['a', 'a'*32, 'wick3d-elb-name', 'Wick3d-ELB-Name']:
elb_name(b)
for b in ['a'*63, 'invalid_elb', '-invalid-elb',
Copy link
Member

Choose a reason for hiding this comment

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

Probably want 'a'*33 instead of 63. Add 'a-' as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Copy pasta. Fixed and expanded in 9b55342

@@ -73,6 +73,14 @@ def s3_bucket_name(b):
raise ValueError("%s is not a valid s3 bucket name" % b)


def elb_name(b):
elb_name_re = compile(r'^[a-zA-Z0-9][a-zA-Z0-9\-]{0,30}[a-zA-Z0-9]{1}$')
Copy link
Member

Choose a reason for hiding this comment

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

Try something like this:

^[a-zA-Z0-9](?:[a-zA-Z0-9\-]{0,30}[a-zA-Z0-9]{1})?$

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This worked! Nice call. I've added it here: 9b55342

I tested manually a bit on http://pythex.org/ just for context.

@chrisgilmerproj
Copy link
Contributor Author

@markpeek - Thanks for the help on this. It looks to have passed the tests. Do you want me to squash the commits before you accept it?

@markpeek
Copy link
Member

@chrisgilmerproj thanks for making the changes. I'll have time to review further and commit later. I'll squash as part of the github commit flow.

@chrisgilmerproj
Copy link
Contributor Author

I appreciate it. Thanks so much.

@markpeek markpeek merged commit c03b18c into cloudtools:master Nov 15, 2016
@markpeek
Copy link
Member

Thanks!

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.

None yet

2 participants