Skip to content

feat: HTTP Route Support#2293

Merged
theSuess merged 5 commits intomasterfrom
push-wrysouumqzvt
Nov 17, 2025
Merged

feat: HTTP Route Support#2293
theSuess merged 5 commits intomasterfrom
push-wrysouumqzvt

Conversation

@theSuess
Copy link
Copy Markdown
Collaborator

@theSuess theSuess commented Nov 12, 2025

This PR combines the best parts of #2283 and #2026.

Only adds management of HTTPRoute resources. Decisions on how to handle PreferIngress are to be made in #2284.

@theSuess theSuess force-pushed the push-wrysouumqzvt branch 3 times, most recently from 8acb2a0 to 87a6cac Compare November 12, 2025 07:58
@github-actions github-actions Bot added the documentation Issues relating to documentation, missing, non-clear etc. label Nov 12, 2025
@theSuess theSuess force-pushed the push-wrysouumqzvt branch 2 times, most recently from 0991d9b to 64bf7c7 Compare November 12, 2025 08:13
@theSuess theSuess marked this pull request as ready for review November 12, 2025 08:13
This was referenced Nov 12, 2025
Closed
Closed
@theSuess
Copy link
Copy Markdown
Collaborator Author

theSuess commented Nov 12, 2025

E2E tests do not offer any advantage to integration tests in this case as we can fully cover everything in there so I omitted them for now

weisdd
weisdd approved these changes Nov 12, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@weisdd weisdd left a comment

Choose a reason for hiding this comment

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

I've added a couple of minor recommendations (not affecting the functionality itself), the rest is great.

Comment thread controllers/reconcilers/grafana/ingress_reconciler.go
Comment thread main.go
@weisdd
Copy link
Copy Markdown
Collaborator

weisdd commented Nov 15, 2025

@theSuess in #2299, I've added conditional event tracking for Routes, so you might want to implement a similar change here or under another PR. The tracking has to be conditional, otherwise runtime will keep on retrying to subscribe to events, resulting in errors every 10 seconds.

@weisdd
Copy link
Copy Markdown
Collaborator

weisdd commented Nov 15, 2025

Sorry for flooding you with comments :) I've added #2298 and #2301 to clean up dangling resources. We should implement a similar change for HTTPRoutes either here or in a separate PR.

theSuess and others added 3 commits November 17, 2025 07:55
@theSuess @lexfrei @hkmdxlftjf
add reconciliation logic for HTTPRoutes
50f6f57 
Co-Authored-By: Aleksei Sviridkin <f@lex.la>
Co-Authored-By: sure <bigfemonkey@gmail.com>
@theSuess
add HTTPRoute example
5c0286b
@theSuess
mark HTTPRoute as owned
8ae5f74
@theSuess theSuess force-pushed the push-wrysouumqzvt branch 5 times, most recently from ca68bf3 to c54f713 Compare November 17, 2025 07:33
Baarsgaard
Baarsgaard approved these changes Nov 17, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Baarsgaard Baarsgaard left a comment

Choose a reason for hiding this comment

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

Looks good, just a minor oversight here 😄

Comment thread controllers/reconcilers/grafana/ingress_reconciler.go Outdated
}

func (r *IngressReconciler) deleteHTTPRouteIfNil(ctx context.Context, cr *v1beta1.Grafana, scheme *runtime.Scheme) error {
if cr.Spec.Ingress != nil {
Copy link
Copy Markdown
Collaborator

@Baarsgaard Baarsgaard Nov 17, 2025

Choose a reason for hiding this comment

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

Suggested change
if cr.Spec.Ingress != nil {
if cr.Spec.HTTPRoute != nil {

Copy link
Copy Markdown
Collaborator Author

@theSuess theSuess Nov 17, 2025

Choose a reason for hiding this comment

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

Huh, weird that the test case didn't catch this but I guess it makes sense based on the ordering of operations (and the fact that we only run the reconciliation once)

Copy link
Copy Markdown
Collaborator

@weisdd weisdd Nov 17, 2025

Choose a reason for hiding this comment

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

Yeah, I think you're right, it's about ordering: Delete (doesn't exist yet, no error) -> Create -> Get (exists) -> Delete (actual deletion) -> Get (removed). We just don't have a test that would cover both Ingress and HTTPRoute.

weisdd
weisdd approved these changes Nov 17, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@weisdd weisdd left a comment

Choose a reason for hiding this comment

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

In addition to Steffen's comment, the log line needs to also be adjusted. The rest LGTM :)

Comment thread controllers/reconcilers/grafana/ingress_reconciler.go Outdated
@theSuess theSuess enabled auto-merge November 17, 2025 09:51
@theSuess theSuess added this pull request to the merge queue Nov 17, 2025
Merged via the queue into master with commit f4a6c95 Nov 17, 2025
16 checks passed
@theSuess theSuess deleted the push-wrysouumqzvt branch November 17, 2025 10:01
@theSuess theSuess mentioned this pull request Nov 17, 2025
Closed
@theSuess theSuess added feature this PR introduces a new feature and removed documentation Issues relating to documentation, missing, non-clear etc. labels Dec 15, 2025
@chriskvik chriskvik mentioned this pull request Dec 16, 2025
Closed
@lexfrei lexfrei mentioned this pull request Feb 6, 2026
Open
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@weisdd weisdd weisdd approved these changes

@Baarsgaard Baarsgaard Baarsgaard approved these changes

@nissessenap nissessenap Awaiting requested review from nissessenap

@ishanjainn ishanjainn Awaiting requested review from ishanjainn ishanjainn is a code owner

@hubeadmin hubeadmin Awaiting requested review from hubeadmin hubeadmin is a code owner

@pb82 pb82 Awaiting requested review from pb82 pb82 is a code owner

Assignees

No one assigned

Labels

feature this PR introduces a new feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@theSuess @weisdd @Baarsgaard