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

Truncate GCE load balancer names to 63 chars #7609

Merged
merged 1 commit into from May 1, 2015

Conversation

Projects
None yet
5 participants
@brendandburns
Copy link
Contributor

commented May 1, 2015

@googlebot googlebot added the cla: yes label May 1, 2015

@brendandburns brendandburns force-pushed the brendandburns:lb branch from fe7e82a to 40549dc May 1, 2015

// Hash and truncate
hash := md5.Sum([]byte(name))
truncated := name[0 : LOAD_BALANCER_NAME_MAX_LENGTH-6]
shortHash := hash[0:6]

This comment has been minimized.

Copy link
@roberthbailey

roberthbailey May 1, 2015

Member

nit: can just do [:6] here

@a-robinson

This comment has been minimized.

Copy link
Member

commented May 1, 2015

LGTM

a-robinson added a commit that referenced this pull request May 1, 2015

Merge pull request #7609 from brendandburns/lb
Truncate GCE load balancer names to 63 chars

@a-robinson a-robinson merged commit eab6020 into kubernetes:master May 1, 2015

2 of 4 checks passed

Shippable Builds failed on Shippable
Details
coverage/coveralls Coverage decreased (-0.01%) to 49.13%
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@caesarxuchao

This comment has been minimized.

Copy link
Member

commented May 1, 2015

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

j3ffml added a commit that referenced this pull request May 2, 2015

Merge pull request #7635 from a-robinson/length
Revert #7145 now that #7609 is in, and fix the tests that were relying on it

cjcullen added a commit to cjcullen/kubernetes that referenced this pull request May 2, 2015

ghost pushed a commit that referenced this pull request May 3, 2015

Quinton Hoole

ghost pushed a commit that referenced this pull request May 3, 2015

Quinton Hoole
Merge pull request #7681 from GoogleCloudPlatform/revert-7635-length
Revert "Revert #7145 now that #7609 is in, and fix the tests that were r...
@ghost

This comment has been minimized.

Copy link

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 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.