-
Notifications
You must be signed in to change notification settings - Fork 132
feat: Add ProxySettings user guide #1639
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
base: ngf-release-2.4
Are you sure you want to change the base?
Conversation
✅ Deploy Preview will be available once build job completes!
|
| @@ -0,0 +1,442 @@ | |||
| --- | |||
| title: Proxy Settings Policy API | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it looks like we're starting to follow this naming pattern with these API docs, could we update the title of the Snippets Filter doc?
| curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/coffee | ||
| ``` | ||
|
|
||
| The coffee application will still fail with a 502 Bad Gateway error because the Gateway-level policy's `bufferSize: "4k"` is not large enough to handle the coffee app's 10KB response headers. We'll fix this in the next section by applying a route-specific policy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that the intention is to show attaching a policy to a Gateway, but it's a bit weird to say "let's apply a policy, oh wait, it doesn't actually fix anything, so let's do it again."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fair
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess in my mind this would later show other proxy settings as well and then be less redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to remove or change this but looking at it I still think it makes sense to keep it here until we have other fields where we can more easily show the inheritance in action.
| For example, with `buffers: {number: 8, size: "4k"}` (32KB total), valid values for `busyBuffersSize` are between `bufferSize` (exclusive) and 28KB (exclusive). | ||
|
|
||
| **Validation limitation:** NGINX Gateway Fabric validates these constraints only when all fields are set in the same policy. If you set `busyBuffersSize` in one policy and `buffers` in another (via inheritance), you are responsible for ensuring the merged configuration satisfies NGINX's requirements. If the constraints are violated, NGINX will fail to reload and log an error. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doc is obviously very focused on buffering since that's all we support right now. However, eventually we're going to support different fields in this policy. This guide could get messy. So I wonder if the focus of the guides should be use-case instead of introducing the API. Does that mean we're going to have too many guides? Or can this doc be formatted in such a way to have different sections for each use case? I just don't want us to have to refactor this too much when we add more functionality to the API.
@ADubhlaoich thoughts on how we should structure it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this section is specifically called "Buffer configuration constraints" for this reason. I think having a guide per use case could make it very difficult to find the information you're looking for.
Once the API is expanded, this whole document can be rewritten to be more proxy_* focussed, but I think we need this specific section either way because of the NGINX restrictions. Other settings (like timeouts) are far more straight forward and won't require additional information sections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just linked to the end of the doc, didn't intend for my comment to be specific for this section. Should've just linked at the top haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it's ok to have specific sections - this is how to use the proxy api, and it has detailed sections on buffers - not only because it's the first functionality, but also because it's an area that's frequently misconfigured. If, when we add more fields later, we decide the document becomes too cluttered, we can just rename this to a use case guide instead of an api guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have sub-page dedicated to buffering cases?
salonichf5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me overall except for a few nits :) nice work 🚀
| ```yaml | ||
| kubectl apply -f https://raw.githubusercontent.com/nginx/nginx-gateway-fabric/v{{< version-ngf >}}/examples/proxy-settings-policy/httproutes.yaml | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: shouldn't these be shell?
| The settings in `ProxySettingsPolicy` correspond to the following NGINX directives: | ||
|
|
||
| - [`proxy_buffering`](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffering) | ||
| - [`proxy_buffer_size`](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffer_size) | ||
| - [`proxy_buffers`](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffers) | ||
| - [`proxy_busy_buffers_size`](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_busy_buffers_size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we also want to mention the field in the API these directives correspond to?
|
|
||
| Even when `disable: true` is set to disable response body buffering, NGINX still needs to buffer response headers. The `bufferSize` field controls the buffer size for headers and applies regardless of the `disable` setting. | ||
|
|
||
| If your upstream server sends large response headers (for example, many cookies or custom headers), you must ensure `bufferSize` is large enough to accommodate them, even when buffering is disabled: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! maybe i can remember it now!
Proposed changes
Add a user guide for the new
ProxySettingsPolicyAPIAlso rearranges the weights of the docs in the traffic management section, as several guides had the same weight, and some weights were missing
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. ↩