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

Jump to bottom

Truncate GCE load balancer names to 63 chars #7609

Merged
merged 1 commit into from May 1, 2015
Merged

Truncate GCE load balancer names to 63 chars #7609

merged 1 commit into from May 1, 2015

Conversation

brendandburns
Copy link
Contributor

roberthbailey
roberthbailey reviewed May 1, 2015
View reviewed changes
pkg/cloudprovider/gce/gce.go
// Hash and truncate
hash := md5.Sum([]byte(name))
truncated := name[0 : LOAD_BALANCER_NAME_MAX_LENGTH-6]
shortHash := hash[0:6]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can just do [:6] here

@a-robinson
Copy link
Contributor

LGTM

a-robinson added a commit that referenced this pull request May 1, 2015
@a-robinson
Merge pull request #7609 from brendandburns/lb
eab6020 
Truncate GCE load balancer names to 63 chars
@a-robinson a-robinson merged commit eab6020 into kubernetes:master May 1, 2015
@caesarxuchao
Copy link
Member

Maybe I've missed something. The change makes sense to me, but are you going to revert the changes in #7145? If not, the origName will always be shorter than 32 bytes because of the logic in GetLoadBalancerName(), thus the hash function will never be called.

a-robinson added a commit to a-robinson/kubernetes that referenced this pull request May 1, 2015
@a-robinson
Revert kubernetes#7145 now that kubernetes#7609 is in, and fix the te…
890ff7b 
…sts that were relying on it.
j3ffml added a commit that referenced this pull request May 2, 2015
@j3ffml
Merge pull request #7635 from a-robinson/length
b447dc0 
Revert #7145 now that #7609 is in, and fix the tests that were relying on it
cjcullen pushed a commit to cjcullen/kubernetes that referenced this pull request May 2, 2015
@a-robinson @cjcullen
Revert kubernetes#7145 now that kubernetes#7609 is in, and fix the te…
ce7dca4 
…sts that were relying on it.
@ghost ghost mentioned this pull request May 3, 2015
Closed
ghost pushed a commit that referenced this pull request May 3, 2015
Revert "Revert #7145 now that #7609 is in, and fix the tests that wer…
83b70c4 
…e relying on it"
ghost pushed a commit that referenced this pull request May 3, 2015
Merge pull request #7681 from GoogleCloudPlatform/revert-7635-length
86751e8 
Revert "Revert #7145 now that #7609 is in, and fix the tests that were r...
@ghost
Copy link

ghost commented May 3, 2015

I'd propose rolling this PR back until we've thought this through and tested it better.
I've had to roll back #7635 which attempted to roll back #7145, the former because it simply didn't work, and broke e2e tests.
Which means that this PR actually just introduces dead code, as per @caesarxuchao's comment above.
I also have some other concerns that I'd like to bring up in the review of the reformulation of this PR, on a week day :-)

This was referenced May 3, 2015
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@a-robinson a-robinson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Limit length of load balancer names
5 participants
@brendandburns @a-robinson @caesarxuchao @roberthbailey @googlebot