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

nginx: calculation of server_names_hash_bucket_size not right #623

Closed
justinsb opened this issue Apr 19, 2017 · 3 comments
Closed

nginx: calculation of server_names_hash_bucket_size not right #623

justinsb opened this issue Apr 19, 2017 · 3 comments
Assignees

Comments

@justinsb
Copy link
Member

justinsb commented Apr 19, 2017

The calculation of server_names_hash_bucket_size underestimates the required size.

With a long name (49 characters) I hit:

2017/04/19 02:01:08 [emerg] 28#28: could not build server_names_hash, you should increase server_names_hash_bucket_size: 64
nginx: [emerg] could not build server_names_hash, you should increase server_names_hash_bucket_size: 64
nginx: configuration file /tmp/nginx-cfg540721084 test failed

I confirmed in that temp file that server_names_hash_bucket_size is indeed set to 64.

Here is the relevant code:
https://github.com/nginx/nginx/blob/master/src/core/ngx_hash.c#L269

The correct calculation is NGX_HASH_ELT_SIZE(&names[n]) + sizeof(void *)

where NGX_HASH_ELT_SIZE(name) = (sizeof(void *) + ngx_align((name)->key.len + 2, sizeof(void *)))

So for us, assuming 64 bit, we want to do

8 + 8 + align8(len(s) + 2)

My 49 characters therefore, is 16 + align8(51) = 16 + 56 = 72 (and of course we then have to round up to the next power of 2, i.e. 128)

@justinsb
Copy link
Member Author

(Working on a fix now)

@justinsb justinsb self-assigned this Apr 19, 2017
@aledbf
Copy link
Member

aledbf commented Apr 19, 2017

@justinsb I changed this a couple of days ago #607

justinsb added a commit to justinsb/ingress that referenced this issue Apr 19, 2017
There were some edge cases where we did not calculate hash_bucket_size
correctly.

Fix kubernetes#623
@justinsb
Copy link
Member Author

justinsb commented Apr 19, 2017

@aledbf right, but I think there's some extra nginx padding in the hash table that isn't accounted for. This is just on the length (i.e. key-size) as well - I haven't hit the total hash table size yet.

If in doubt, try creating an ingress with a host with 49 characters e.g.

minimal-ruby-app-staging.useast1a.k8s.example.com

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

No branches or pull requests

2 participants