-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat(helm): add location snippet to nginx config #18105
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
Conversation
|
Seems to be like a duplicate of #11348 Can we go forward with either one? |
jkroepke
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.
The other PR is closed.
Customizing the clientMaxBodySize would benefit us as well
Let do that in a distinct PR as well.
Could you also please provide an use-case for this?
|
Hi @jkroepke
I have provided a use case in the PR description. Do you mean adding it in the
Or do you mean regarding clientMaxBodySize. I basically have the same as @panzouh has in his initial PR #11348
But I didn't know until now that this was already solved: 809a024 |
Signed-off-by: Joshua Hügli <[email protected]>
Co-authored-by: Jan-Otto Kröpke <[email protected]> Signed-off-by: Joshua Hügli <[email protected]>
|
LGTM Thanks for your answers. |
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.
LGTM.
Thanks for your work. Please take note of the merge conflicts and add a note to changelog, including PR at the end of the note.
please take note of #18414 - I guess httpSnippet would work for you as well, since the inherited from the server block issue was resolved there.
yes just noticed #18414 aswell. This would indeed solve my issue. nevertheless I've updated the branch, implemented you proposed change and added the changelog |
|
LGTM. I didnt have merge power yet, but one of the internal loki team will merge this soon. |
|
@joschi36 please check CI looks like make helm-docs is missing |
Signed-off-by: nicolevanderhoeven <[email protected]>
|
Generated the reference doc to unblock this and resolved a merge conflict with Changelog. :) Just waiting for final checks, and otherwise LGTM and I'll merge! |
nicolevanderhoeven
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.
GTG!
What this PR does / why we need it:
The current Helm chart for the Loki gateway does not provide a way to inject Nginx configuration directives inside individual location blocks. This makes it impossible to implement common and important authentication or header manipulation schemes that rely on directives like proxy_set_header without overriding the entire Nginx configuration file.
A key use case is mTLS-based multi-tenancy, where an X-Scope-OrgID header must be set based on client certificate details. Due to Nginx's directive inheritance rules, setting this header in serverSnippet is overridden by any location block that defines its own proxy_set_header (e.g., for X-Query-Tags in the /loki/api/v1/ location or WebSocket upgrades in the /loki/api/v1/tail location). This results in failed authentication for some endpoints but not others.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This change is fully backward-compatible. If locationSnippet is not set, the template renders exactly as it did before, resulting in no change for existing users.
This was tested by implementing the mTLS multi-tenancy scheme described above, which was previously only possible via complex workarounds like overriding the entire nginx.conf file or using a post-renderer. With this change, the configuration becomes trivial and is managed cleanly through values.yaml. The snippet has been added to all location blocks for consistency and to ensure all API endpoints served by the gateway are covered.
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR