NGF: Add document for upstream http2#2021
Conversation
|
This is a shorter guide which is technically covering a Kubernetes concept (Service's appProtocol), however I couldn't find a suitable document to include this information in. So I decided to write a standalone document for it. |
| ```nginx | ||
| location /coffee { | ||
| ... | ||
| proxy_http_version 2; | ||
| proxy_pass http://default_coffee_80; | ||
| ... | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
so the curl doesn't return the protocol? or a different response to identify a http2 connection?
There was a problem hiding this comment.
The upstream application in this example doesn't respond back with http2 responses/ "doesn't speak http2" so it won't really work with a curl request. I don't really feel the need to show a response with a working http2-available backend, so I felt that just showing the nginx conf would suffice.
If you feel like its necessary to show a curl request, basically checking if the nginx conf proxy_http_version 2; works, then we can consider that, but I don't feel the need.
There was a problem hiding this comment.
A user guide implies end-to-end coverage. If this doesn't do that, it should be scoped down to a section within a larger doc, this feels incomplete to me.
could this be part of data plane configuration section as part of how to?
There was a problem hiding this comment.
we could do a small section to enable HTTP2 on a service section
There was a problem hiding this comment.
A user guide implies end-to-end coverage. If this doesn't do that, it should be scoped down to a section within a larger doc, this feels incomplete to me.
could this be part of data plane configuration section as part of how to?
I agree, I like to have curl requests and have sections verifying that traffic flows correctly, but that is essentially verifying that the NGINX product works correctly. I also think end-to-end can mean Kubernetes yaml -> nginx configuration generation. Users of NGF don't necessarily always need to validate nginx configuration works, as our product is mainly involved with generating correct nginx configuration.
The data plane configuration document primarily discusses the NginxProxy resource.
The data plane configuration is stored in the
NginxProxycustom resource...
So I think that would be out of place.
There already is precedence of the practice of just verifying nginx configuration in the Upstream settings policy document https://docs.nginx.com/nginx-gateway-fabric/traffic-management/upstream-settings/. In that document, the primary way the guide is showing to the user that the policies work, is by verifying nginx configuration.
There was a problem hiding this comment.
Yeah, i think in that case we should improve that document.
I am not convinced it should be standalone document. We could also just convert the disable HTTP2 section in the data plane configuration to HTTP2 only. That could talk about both how to enable and disable it.
Disabling using NginxProxy and enabled by adding a appProtocol: h2c field to a service and check for specific line in configuration.
There was a problem hiding this comment.
I think we have separate views on this, we should get a second opinion and see what makes the most sense to majority.
There was a problem hiding this comment.
Is my understanding correct that disabling HTTP/2 at the NGINX level affects all services referencing the route, or does it only apply to incoming connections to NGINX itself?
I'd assume there's still some dependency between the protocol allowed on the ingress side and what gets forwarded upstream even if NGINX can translate between protocols, the incoming connection type likely influences the end-to-end behavior. Want to make sure I understand the dependency
| ## Important Notes | ||
|
|
||
| - `kubernetes.io/h2c` is supported on HTTPRoutes and GRPCRoutes. It isn't supported on TLSRoutes. | ||
| - For NGINX to set `proxy_http_version 2` for a location, all valid backend references in the routing rule must have `appProtocol: kubernetes.io/h2c` set on their Service ports. If any valid backend doesn't use `kubernetes.io/h2c`, NGINX falls back to the default HTTP/1.1. |
There was a problem hiding this comment.
i think this could be in the troubleshooting section
There was a problem hiding this comment.
Would that be in a different file? I feel like this is relevant enough to this feature that I would want it all contained in this single file. I can put it in a different section in the file if you think that would be better
There was a problem hiding this comment.
no, just after other supported protocols section
There was a problem hiding this comment.
After some thought, I think its more suited for where it is currently. The statement isn't really the highlighting of a potential issue rather it is highlighting an important note / requirement to avoid an issue. Additionally, creating a new troubleshooting section with only one bullet point feels incomplete.
There was a problem hiding this comment.
We have a precedence of having troubleshooting section in our individual guides so its not really anything out of standard.
Its a troubleshooting pointer, it should be part of that section.
There was a problem hiding this comment.
I agree with you having troubleshooting sections is not anything out of standard.
However, I don't believe its a troubleshooting pointer.
The statement isn't really the highlighting of a potential issue rather it is highlighting an important note / requirement to avoid an issue.
The troubleshooting sections are usually "If you have these problems... here is the fix". This statement is more of a "Make sure you do these steps so you don't run into an issue".
There was a problem hiding this comment.
And my statement of
Additionally, creating a new troubleshooting section with only one bullet point feels incomplete.
still stands.
There was a problem hiding this comment.
The troubleshooting entry is actually validated by the steps themselves, if you skip setting the protocol, you hit that error. That's how you figured it required for this step
That's precisely the kind of thing worth documenting like here's the error you'll see, and here's how to fix it.
I am not convinced with the argument of not having single-item troubleshooting section.
|
hi @bjee19 can you please resolve the suggestions and re-request reviews in slack? |
4d1d001 to
87185c0
Compare
Proposed changes
Add document on supporting http2 to upstream through the appProtocol field on a Service.
Checklist
Before sharing this pull request, I completed the following checklist:
Footnotes
Potentially sensitive information includes personally identify information (PII), authentication credentials, and live URLs. Refer to the style guide for guidance about placeholder content. ↩