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

[0.9.0-beta.2] client-header-buffer-size doesn't help with http2 #376

Closed
iameli opened this issue Mar 6, 2017 · 2 comments · Fixed by #392
Closed

[0.9.0-beta.2] client-header-buffer-size doesn't help with http2 #376

iameli opened this issue Mar 6, 2017 · 2 comments · Fixed by #392

Comments

@iameli
Copy link

iameli commented Mar 6, 2017

I'm making some HTTP requests with large-ish (~5k) headers. Neither http1 nor http2 worked until I set client-header-buffer-size: 16k in the ConfigMap, and then http1 started working. http2 is still a problem though, I'm getting this:

2017/03/06 02:10:13 [info] 3908#3908: *436 client exceeded http2_max_field_size limit while processing HTTP/2 connection, client: 127.0.0.1, server: 0.0.0.0:442

Looks like http2_max_field_size defaults to 4k, just like client_header_buffer_size.

Would it be appropriate to have the client-header-buffer-size ConfigMap parameter also control the http2_max_field_size parameter, or would it be better to have a separate directive entirely? It seems to me like you'd always want those two parameters to have the same value. I can open a PR for this no problem, just want to make sure I'm doing the right thing first.

@iameli iameli changed the title [0.9.0-beta.2] client-header-buffer-size and such are ignored in http2 [0.9.0-beta.2] client-header-buffer-size doesn't help with http2 Mar 6, 2017
@rikatz
Copy link
Contributor

rikatz commented Mar 6, 2017

@iameli Have you tryied changing the http2_max_field_size to look like the https://github.com/kubernetes/ingress/blob/master/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl#L61

Did it worked fine? If so, you could open a PR to change / add this in the next releases.

@aledbf
Copy link
Member

aledbf commented Mar 7, 2017

@iameli we can start using the same configuration and change that in a future version if required

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 a pull request may close this issue.

3 participants