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

Proposal: http2 fallback to host header if authority is missing #1349

Closed
wants to merge 1 commit into from

Conversation

fredr
Copy link
Contributor

@fredr fredr commented Jan 22, 2019

Use the host header if :authority is not set

We use an nginx-ingress in kubernetes with the grpc-module. But for some reason the :authority header is not forwarded (I'm digging into why), but the host header is.
This causes cowlib and cowboy to reply with a protocol_error, and nginx will reply 502 Bad Gateway.

This was quite hard to debug, so even if you don't think cowboy/cowlib should handle this, this PR can act as some kind of documentation for others having the same problem.

Other http2 servers seems to fallback to use the host header if :authority was not set.

This one is dependent on ninenines/cowlib#73

@essen
Copy link
Member

essen commented Jan 22, 2019

OK with it but this needs tests and it'll need to use lists:keyfind instead of proplists, because the latter is slow.

@fredr fredr force-pushed the http2-authority-fallback branch from d38e45c to 26869f7 Compare January 23, 2019 14:52
@fredr
Copy link
Contributor Author

fredr commented Jan 23, 2019

Excellent!

I've added a test to verify that we fallback on the host header.
We still reject the request if both of host and :authority is missing.

Do you want any other tests? Also, I didn't know if I should update the cowlib dep to point to mentioned pr?

@fredr fredr force-pushed the http2-authority-fallback branch from 26869f7 to 059dfd3 Compare January 24, 2019 08:23
@fredr
Copy link
Contributor Author

fredr commented Jan 30, 2019

Hey @essen, any chanse this could be merged? or do you want me to make any changes?

@essen
Copy link
Member

essen commented Jan 30, 2019

I'll get back to you when I come back to Cowboy in a few weeks. PR looks fine otherwise.

@essen
Copy link
Member

essen commented Apr 1, 2019

Amended and merged, thanks!

@essen essen closed this Apr 1, 2019
@fredr
Copy link
Contributor Author

fredr commented Apr 1, 2019

Thank you! 🍻

@fredr
Copy link
Contributor Author

fredr commented Jul 3, 2019

For anyone who is interested, this has now also been fixed in the kubernetes ingress: kubernetes/ingress-nginx#4212

@essen
Copy link
Member

essen commented Jul 3, 2019

Glad to hear, 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.

2 participants