Skip to content

Add missing referencegrants for auth handlers#775

Merged
ensonic merged 1 commit into
mainfrom
ensonic/refgrant
Jul 3, 2026
Merged

Add missing referencegrants for auth handlers#775
ensonic merged 1 commit into
mainfrom
ensonic/refgrant

Conversation

@ensonic

@ensonic ensonic commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

This makes the "-auth" http-routes work.

I am still not happy that we need to managed hard-coded lists.

@ensonic ensonic requested a review from Tobias-Pe July 3, 2026 10:32
@ensonic

ensonic commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

I am not sure, how this is supposed to scale though, as it feels upside down. I'll research.

This makes the "-auth" http-routes work.
@ensonic ensonic force-pushed the ensonic/refgrant branch from 9767069 to 2966f03 Compare July 3, 2026 11:01
@ensonic ensonic changed the title Add missing referencegrant for oauth2-proxy Add missing referencegrants for auth handlers Jul 3, 2026
@ensonic ensonic requested a review from csieber July 3, 2026 11:02

@Tobias-Pe Tobias-Pe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval to fix this for now. Let's iterate on how to make this better

Feel free to resolve the AI review comments without action if you disagree or add them into a followup

type: PathPrefix
value: /grafana
{{- if eq . "-auth" }}
{{- $authUrl := urlParse (tpl $.Values.gf_ingress_auth_url $) }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ARCHITECTURE, STYLE] Consider defining a shared Helm helper template (e.g., in _helpers.tpl using define) to extract the backend service name, namespace, and port from auth URLs. Encapsulating this URL parsing and service destination lookup logic avoids repeating identical template logic across both grafana-http-route.yaml and prometheus-http-route.yaml.

Additionally, consider placing distinct template statements on separate lines (such as on line 27) to improve code readability.

🤖 AI Reviewer (Jetski) | Traces: architecture: http://go/traj/7ef5cb6a-c322-4436-b205-9691752f849d, style: http://go/traj/6066be5b-eac5-4040-a40c-0c0682f51e23

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRs are welcome ;)

Comment thread src/app_charts/prometheus/cloud/grafana-http-route.yaml
path:
type: ReplaceFullPath
replaceFullPath: {{ tpl $.Values.gf_ingress_auth_url $ }}
replaceFullPath: {{ $authUrl.path }}{{ if $authUrl.query }}?{{ $authUrl.query }}{{ end }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CORRECTNESS] Omit query parameters (?{{ $authUrl.query }}) from replaceFullPath and provide a default path (such as /) when $authUrl.path is empty, because Gateway API URLRewrite expects an RFC 3986 path component and will either reject or percent-encode question marks (this also applies to prometheus-http-route.yaml).

🤖 AI Reviewer (Jetski) | Traces: correctness: http://go/traj/82c48c05-bb91-453d-8c56-c99db974d19c

@ensonic ensonic merged commit 3966cb8 into main Jul 3, 2026
7 checks passed
@ensonic ensonic deleted the ensonic/refgrant branch July 3, 2026 11:17
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