-
Notifications
You must be signed in to change notification settings - Fork 3.9k
feat: add Gateway API Route CR to loki-gateway #16799
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: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Matias Charriere <[email protected]>
Signed-off-by: Quentin Bisson <[email protected]>
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.
Hey @mcharriere
The implementation is similar to grafana's own route.
I'm not fully convinced that the current simplicity and structure are sufficient in this case. While the Gateway object looks straightforward, the Ingress object at the root of the templates is quite complex.
If we want to introduce Gateway API support in the Loki Helm chart, the PR should at least include a Gateway API manifest that covers both:
- Gateway deployments
- Non-Gateway deployments (traditional ingress)
This is crucial to ensure the values.yaml structure works for both use cases. It would be a bad design decision to merge this PR, only to find out later that the current values setup doesn’t support additional HTTPRoute resources properly.
| - name: {{ include "loki.gatewayFullname" $ }} | ||
| port: {{ $.Values.service.port }} |
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.
| - name: {{ include "loki.gatewayFullname" $ }} | |
| port: {{ $.Values.service.port }} | |
| - name: {{ include "loki.gatewayFullname" $ }} | |
| port: {{ $.Values.service.port }} | |
| group: '' | |
| kind: Service | |
| weight: 1 |
ref:
|
@mcharriere Do we still need this PR? If yes, please check my comments. |
What this PR does / why we need it:
Add support for Gateway API Routes to loki's helm chart. The implementation is similar to grafana's own route.
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