-
Notifications
You must be signed in to change notification settings - Fork 445
feat(ingress): add HTTPRoute support for Grafana instances #2283
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: master
Are you sure you want to change the base?
Conversation
weisdd
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.
Thanks for the PR. I've added a few questions / comments.
As a side note, I'd say 30.4K of changes in a single commit is too much. Independent changes and any code generation should better be spread across multiple commits, especially due to the fact that you tried not only to implement new feature, but also to change the behaviour of things that have existed for years (more details in my review comments).
Also, please, note that there's another PR covering the same functionality #2273, which was opened a few days earlier. I think we haven't had conflicting PRs before, let's see which implementation will be better.
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.
crd folder is not intended to contain third-party CRDs, it should only contain resources generated by kubebuilder. Files required only for tests should be hosted elsewhere.
Also, not sure why you added the same HTTPRoute CRD twice (httproutes.yaml + standard-install.yaml)
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.
Moved Gateway API CRDs to tests/fixtures/gateway-api/ as recommended.
Question for maintainers: Should we rely on Gateway API being pre-installed in test environment instead? Similar to approach in PR #2273.
api/v1beta1/grafana_types.go
Outdated
| PreferIngress *bool `json:"preferIngress,omitempty"` | ||
| // +nullable | ||
| // If the operator should send it's request through the grafana instances HTTPRoute object instead of through the service. | ||
| PreferHTTPRoute *bool `json:"preferHTTPRoute,omitempty"` |
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.
It's up for a discussion amongst maintainers, but I feel like it's not worth implementing as it would require too much complexity and wider cluster permissions for properly detecting protocol (HTTP/HTTPS).
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.
Understood. Keeping PreferHTTPRoute for now but ready to remove if maintainers decide it's too complex.
The implementation does Gateway lookup to properly detect protocol, but we can simplify to basic hostname-only approach if preferred.
|
|
||
| // On openshift, Fallback to Ingress when spec.route is undefined | ||
| // Track overall status - if any reconciliation is in progress, return in progress | ||
| overallStatus := v1beta1.OperatorStageResultSuccess |
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.
I would prefer not to mix adding support for HTTPRoutes and changes in reconciliation behaviour under a single PR. - So far, we've required users to pick a single option, and I'm yet to see a convincing explanation why we should change that.
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.
This addresses the migration use case: teams want to test HTTPRoute alongside stable Ingress without downtime.
Use case:
- Production: Ingress (proven, stable)
- Add HTTPRoute in parallel for validation
- Switch via preferHTTPRoute when ready
- Remove Ingress after full validation
No breaking changes - if only one specified, only one created. Can split into separate PR if preferred.
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.
Well, it would be breaking for someone who, for some reasons, has both Routes and Ingresses defined in their manifests. Any change in existing behaviour in a project with such a large user base as ours needs to be carefully considered and properly articulated in release descriptions.
In my view, in this particular PR, it's better to keep only implementation for HTTPRoutes, so we could reason here only about the feature. The best approach would be to preserve the existing precedence for Routes.
As for the behaviour change, please, open a new issue describing in details your reasoning, and you'll then get some feedback in the coming weeks (most of the time, issues are discussed at a weekly cadence). Once consensus is reached, a related PR would be welcomed.
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.
Pardon, missed the breaking change. Kept the Route > Ingress logic, moved HTTPRoute to parallel. Nothing breaks now. Is this OK?
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.
Let's not have any parallel execution until it is discussed in a separate issue. And even then, any change in behaviour needs to be handled separately.
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.
Got it. Changed to Route > Ingress > HTTPRoute precedence (mutually exclusive). No parallel execution.
Add Kubernetes Gateway API v1.3.0 as dependency for HTTPRoute support. Note: Using v1.3.0 instead of v1.4.0 due to invalid kubebuilder validation markers in v1.4.0 that cause controller-gen failures. Issue tracked at: kubernetes-sigs/gateway-api#4172 Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add HTTPRoute field to GrafanaSpec for Gateway API integration. Add PreferHTTPRoute field to GrafanaClient for routing preference. Implement HTTPRouteGatewayV1 type with Strategic Merge support. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add model function to generate HTTPRoute resource with controller reference and common labels. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add kubebuilder RBAC markers and regenerate role manifests for gateway.networking.k8s.io/httproutes resources. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add HTTPRouteReconciler with Gateway API integration: - Create/update HTTPRoute resources with Strategic Merge - AdminURL extraction via Gateway lookup - Listener matching for protocol detection (HTTP/HTTPS) - TLS detection from Gateway listener configuration - Fallback logic for missing Gateway information Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add HTTPRouteReconciler invocation from IngressReconciler to support parallel creation of Ingress and HTTPRoute resources when both are specified in Grafana CR. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add Gateway API v1.3.0 standard CRDs to tests/fixtures/ for envtest. Update suite_test.go to load CRDs from fixtures directory. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add unit tests for HTTPRoute functionality: - HTTPRoute creation when spec.httpRoute is defined - Parallel Ingress and HTTPRoute creation Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add three HTTPRoute examples demonstrating different configurations: - Basic: simple HTTPRoute with Gateway parentRef - TLS: HTTPRoute with TLS via Gateway sectionName - Filters: request/response header modification Examples include README with setup instructions and reference to Gateway API documentation. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Regenerate CustomResourceDefinitions, RBAC, and API documentation after adding HTTPRoute support to Grafana CRD. Generated with: - make manifests - make generate Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
ff67b1d to
4ecc281
Compare
|
@weisdd Thank you for the thorough review! I've addressed all the feedback and restructured the PR into 10 granular commits. Changes Made✅ go.mod: Reverted unrelated dependency changes (only gateway-api v1.3.0) Implementation DetailsGateway API v1.3.0: Using v1.3.0 instead of v1.4.0 due to invalid kubebuilder validation markers in v1.4.0 that cause controller-gen failures. Issue tracked at: kubernetes-sigs/gateway-api#4172 HTTPRouteReconciler: Extracted into separate file with proper Gateway status inspection for protocol detection (HTTP/HTTPS via listener TLS configuration). Parallel Ingress+HTTPRoute: Addressed migration use case - details in inline comments. Ready for re-review. |
|
@lexfrei Please, reply in the respective threads, otherwise conversations would be hard to follow. |
Restore original behavior where Route takes priority over Ingress on OpenShift for backwards compatibility. HTTPRoute works independently in parallel with Route/Ingress as it's part of Gateway API. This prevents breaking changes for users who have both Route and Ingress defined in their manifests. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
|
|
||
| // getHTTPRouteAdminURL returns the admin URL for accessing Grafana via HTTPRoute. | ||
| // It performs Gateway lookup to determine the correct protocol and hostname. | ||
| func (r *HTTPRouteReconciler) getHTTPRouteAdminURL(ctx context.Context, httpRoute *gatewayv1.HTTPRoute) (string, error) { |
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.
If the functionality gets accepted, the function will need to be refactored and covered by tests. - The current complexity is too high.
Related functions need to also be covered by tests.
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.
Removed PreferHTTPRoute and Gateway lookup complexity to simplify initial implementation. Created #2284 to discuss this functionality separately.
Change from parallel execution to strict precedence order. Users must pick a single ingress option (no parallel execution). This preserves existing behavior and prevents breaking changes until parallel execution is discussed in a separate issue. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add title/linkTitle frontmatter for documentation rendering. Replace manual sections with readfile shortcode for manifest inclusion. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Remove advanced filters example as it's out of scope. Examples should focus on basic manifests only. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
- Set ErrorIfCRDPathMissing to true to catch missing CRD paths - Update test to verify Ingress > HTTPRoute precedence - Test now expects HTTPRoute NOT to be created when both are defined Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Remove PreferHTTPRoute field and complex Gateway lookup logic to simplify initial HTTPRoute implementation. This functionality can be discussed and added in a future PR if needed. Changes: - Remove PreferHTTPRoute from ClientConfig - Remove getHTTPRouteAdminURL and findMatchingListener functions - Remove Gateway protocol detection logic - Remove unused imports and constants Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Update CRDs and documentation after removing PreferHTTPRoute field. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
| @@ -0,0 +1,30 @@ | |||
| # HTTPRoute with TLS Example | |||
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.
README here was not updated to have title and readfile instructions, but let's remove it instead. - "Basic HTTPRoute Example" example is enough, the rest can easily be found in Gateway API docs.
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.
Removed httproute_tls example. Only httproute_basic remains.
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.
Things that also need to be covered in the PR:
- the operator must be able to function when the Gateway API CRD(s) is not present;
- e2e-tests need to be extended to cover cases where the CRD IS NOT present and when it IS present;
- controller-runtime needs to be configured to cache the HTTPRoute objects (check this for reference).
Remove TLS example as basic HTTPRoute example is sufficient. Advanced configurations can be found in Gateway API documentation. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Allow operator to function without Gateway API CRDs installed: - Add optional Gateway API scheme registration in main.go - Add graceful error handling in HTTPRouteReconciler - Log info message if Gateway API CRDs not found - Continue with other reconciliation if HTTPRoute unavailable This ensures the operator works in clusters without Gateway API while still supporting HTTPRoute when CRDs are installed. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Configure controller-runtime to cache HTTPRoute objects with managed-by label, same as other managed resources (Ingress, Route). This ensures efficient caching and label-based filtering for HTTPRoute resources when Gateway API is available. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
|
|
||
| // Gateway API is optional - operator should function without it | ||
| // nolint:errcheck // Intentionally ignoring error - operator should work without Gateway API CRDs | ||
| _ = gatewayv1.Install(scheme) |
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.
Could you explain why it's implemented this way? Routes are also optional, but we don't suppress their registration errors (line 104).
| } | ||
|
|
||
| // Cache HTTPRoute objects (Gateway API is optional) | ||
| mgrOptions.Cache.ByObject[&gatewayv1.HTTPRoute{}] = cacheLabelConfig |
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.
Not sure if that would work. Have you tried to run this code? (I see that all commits are signed by Claude, so not sure if any of this goes through human validation) - E.g. we've seen with Routes that caching configuration (mgrOptions.Cache.ByObject[&routev1.Route{}] = cacheLabelConfig) leads to panic when not hidden behind the isOpenShift check. - If you want to check whether HTTPRoutes are present, I think you have to implement something similar to what we did for OpenShift. It would also make sense to add a log message stating whether the CRD was successfully discovered.
Summary
Adds full support for Kubernetes Gateway API HTTPRoute resources to Grafana instances, enabling modern ingress capabilities alongside traditional Ingress and OpenShift Route objects.
Changes
API Changes
HTTPRoute *HTTPRouteGatewayV1field toGrafanaSpecPreferHTTPRoute *boolfield toGrafanaClientfor routing preferenceHTTPRouteGatewayV1type with Strategic Merge supportController Changes
reconcileHTTPRoute()method with merge logicgetHTTPRouteSpec()helper for default HTTPRoute configurationgetHTTPRouteAdminURL()for AdminURL extraction from HTTPRoutegateway.networking.k8s.io/httproutesTesting
Examples
Added 4 comprehensive examples:
httproute_basic/- Basic HTTPRoute with Gateway parentRefhttproute_tls/- HTTPRoute with TLS via Gateway sectionNamehttproute_weighted/- Weighted routing for A/B testing (90/10 split)httproute_filters/- Request/response header modificationGateway API Version Note⚠️
This PR uses Gateway API v1.3.0 instead of v1.4.0.
Reason: Gateway API v1.4.0 contains invalid kubebuilder validation markers (
+kubebuilder:validation:MaxLengthapplied to arrays instead of strings) that causecontroller-gento fail during manifest generation.Status: The issue is fixed in Gateway API main branch (PR #4178), but v1.4.1 patch release has not been published yet.
Upstream Issue: kubernetes-sigs/gateway-api#4172
Recommendation: Do not upgrade to Gateway API v1.4.0 until v1.4.1 is released or the validation marker issue is confirmed resolved.
Test Plan
Breaking Changes
None. This is a purely additive feature.
Related Issues
Addresses the need for Gateway API support in grafana-operator.