feat: add Gateway API support in hydra - #889
PatTheSilent wants to merge 6 commits into
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThis PR adds Gateway API HTTPRoute support to the Hydra Helm chart. It adds public and admin route templates, disabled defaults with hostnames, and issuer URL selection from the public HTTPRoute hostname. ChangesHTTPRoute feature
Priority: ➖ Normal — Schedule the Gateway API support because it adds public and admin HTTPRoute resources across Hydra’s Helm chart, but no urgent external impact is indicated. Estimated code review effort: 2 (Simple) | ~12 minutes Merge Risk: 🟡 Moderate · up to Enabling the new public HTTPRoute can configure Hydra with an HTTP issuer even when users expose it over HTTPS through a Gateway. This can produce an incorrect issuer URL for clients and should be addressed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant Values as HTTPRoute values
participant Helm as Helm templates
participant Gateway as Kubernetes Gateway API
participant Issuer as hydra.config.urls.issuer
Values->>Helm: Enable public or admin HTTPRoute
Helm->>Gateway: Render HTTPRoute with service backend
Values->>Issuer: Provide public hostname
Issuer->>Issuer: Build http://<host> issuer URL
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
helm/charts/hydra/templates/httproute-admin.yaml (2)
17-20: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDefault empty
parentRefsproduces an invalid HTTPRoute.Same concern as flagged for
httproute-public.yamlandvalues.yaml.Also applies to: 27-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helm/charts/hydra/templates/httproute-admin.yaml` around lines 17 - 20, The httproute-admin template currently renders an empty parentRefs block when .Values.httproute.admin.parentRefs is unset, which makes the HTTPRoute invalid. Update the template around parentRefs in the httproute-admin YAML to only emit that section when there is at least one entry, using the same conditional pattern used for httproute-public.yaml, and ensure the related defaults in values.yaml do not leave parentRefs empty by default.
5-15: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame
labels/annotationsnesting bug ashttproute-public.yaml.Identical control-flow issue: the
if .Release.Namespace(line 7) is only closed at line 15, solabels:(9-10) andannotations:(11-14) are nested inside it, unlikeservice-admin.yaml's pattern where the namespace conditional closes immediately after thenamespace:line.🐛 Proposed fix
metadata: name: {{ $fullName }}-admin {{- if .Release.Namespace }} namespace: {{ .Release.Namespace }} + {{- end }} labels: {{- include "hydra.labels" . | nindent 4 }} {{- with .Values.httproute.admin.annotations }} annotations: {{- tpl (toYaml .) $ | nindent 4 }} {{- end }} - {{- end }} spec:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helm/charts/hydra/templates/httproute-admin.yaml` around lines 5 - 15, The metadata block in httproute-admin has the same nesting mistake as httproute-public: the .Release.Namespace conditional in the template keeps labels and annotations inside it instead of matching the service-admin pattern. Update the httproute-admin template so the namespace guard only wraps the namespace field, then render labels and annotations unconditionally afterward, using the existing metadata structure and symbols like .Release.Namespace, .Values.httproute.admin.annotations, and include "hydra.labels".helm/charts/hydra/templates/httproute-public.yaml (1)
17-20: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDefault empty
parentRefsproduces an invalid HTTPRoute.See corresponding comment on
values.yaml— with the defaultparentRefs: [], thiswithblock renders no content, leavingparentRefs:with a null value, which is invalid since Gateway API HTTPRoutes require at least oneparentRefto attach to a Gateway.Also applies to: 27-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helm/charts/hydra/templates/httproute-public.yaml` around lines 17 - 20, The HTTPRoute templates are rendering an empty parentRefs field when .Values.httproute.public.parentRefs (and the same pattern in the other affected template block) is left at its default empty list, which produces an invalid route. Update the templating around the parentRefs section in the HTTPRoute manifest so it either always renders at least one valid parentRef or conditionally omits the entire HTTPRoute when no parentRefs are configured, using the existing .Values.httproute.public.parentRefs lookup to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/charts/hydra/templates/httproute-public.yaml`:
- Around line 5-15: The `httproute-public.yaml` template has `labels` and
`annotations` incorrectly nested under the `.Release.Namespace` conditional.
Update the `metadata` block so the `if .Release.Namespace` in the
`httproute-public` template only wraps the `namespace:` line, then close it
before `labels:` and the `with .Values.httproute.public.annotations` block,
matching the pattern used in `service-public.yaml`.
---
Duplicate comments:
In `@helm/charts/hydra/templates/httproute-admin.yaml`:
- Around line 17-20: The httproute-admin template currently renders an empty
parentRefs block when .Values.httproute.admin.parentRefs is unset, which makes
the HTTPRoute invalid. Update the template around parentRefs in the
httproute-admin YAML to only emit that section when there is at least one entry,
using the same conditional pattern used for httproute-public.yaml, and ensure
the related defaults in values.yaml do not leave parentRefs empty by default.
- Around line 5-15: The metadata block in httproute-admin has the same nesting
mistake as httproute-public: the .Release.Namespace conditional in the template
keeps labels and annotations inside it instead of matching the service-admin
pattern. Update the httproute-admin template so the namespace guard only wraps
the namespace field, then render labels and annotations unconditionally
afterward, using the existing metadata structure and symbols like
.Release.Namespace, .Values.httproute.admin.annotations, and include
"hydra.labels".
In `@helm/charts/hydra/templates/httproute-public.yaml`:
- Around line 17-20: The HTTPRoute templates are rendering an empty parentRefs
field when .Values.httproute.public.parentRefs (and the same pattern in the
other affected template block) is left at its default empty list, which produces
an invalid route. Update the templating around the parentRefs section in the
HTTPRoute manifest so it either always renders at least one valid parentRef or
conditionally omits the entire HTTPRoute when no parentRefs are configured,
using the existing .Values.httproute.public.parentRefs lookup to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a0eed4a-142e-40ee-9910-937260d7825b
📒 Files selected for processing (4)
helm/charts/hydra/templates/_helpers.tplhelm/charts/hydra/templates/httproute-admin.yamlhelm/charts/hydra/templates/httproute-public.yamlhelm/charts/hydra/values.yaml
|
@Demonsthere @piotrmsc any chance I could get your attention? |
2 participants
Related Issue or Design Document
partially implements #845
Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact security@ory.com) from the maintainers to push the changes.
Further comments
This PR adds Gateway API HTTPRoutes in
hydrafor the API and proxy.The objective was to provide a drop-in replacement for Ingress resources.
One discepancy is in
_helpers.tpland thehydra.config.urls.issuerhelper. HTTPRoutes don't contain information about TLS in their spec so it's impossible to determine the protocol just from it. Since you can work around this by using.Values.hydra.config.urls.self.issuerI did not consider it a problem but I'm happy to get feedback.This PR is a bit selfish. I just need to get this done one way or another and thought that contributing back was the best way to get it done.
Summary by CodeRabbit
New Features
Documentation