feat(rocketchat): add Gateway API HTTPRoute support#234
Conversation
|
Love this. With ingress api freezed, definitely something we needed to add eventually. Do you think some tests could be added for this here - https://github.com/RocketChat/helm-charts/blob/master/rocketchat/tests/rocketchat.bats ? |
|
Thanks for the quick look, @debdutdeb! Agreed — with the Ingress API frozen this is overdue. I've added HTTPRoute rendering tests in Coverage:
All pass locally in both modes (8/8 microservices, 8/8 monolith). Happy to adjust the style or fold them into the cluster-based detik assertions instead if you'd prefer — just let me know. |
|
@somaz94 would you mind if i asked you to record just a small video of this working with an actual loadbalancer running and rc working through gapi? I might not be able to test this until 2-3 weeks in. But unless this breaks existing code, I'll merge as-is as long as it works. |
| federation: | ||
| # serveWellKnown adds /.well-known/matrix/{server,client} routes to the wellknown service | ||
| # (only rendered when federation.enabled is also true) | ||
| serveWellKnown: false |
There was a problem hiding this comment.
Let's remove this whole thing. We have deprecated the last federation architecture that needed deployment level proxy configuration. Not only in the new design even in the old architecture's latest iteration the app serves the endpoints itself. These are relics at this point, and no need to accumulate the debt here.
There was a problem hiding this comment.
Done in 7d7974a — removed the entire federation / well-known block (and the separate Synapse HTTPRoute). The chart now renders only the main Rocket.Chat route (plus the microservices /sockjs + /websocket rules). Thanks for the context on the deprecated arch.
| # HTTPRoute apiVersion (defaults to "gateway.networking.k8s.io/v1" when empty) | ||
| apiVersion: "" |
There was a problem hiding this comment.
Any need to do it this way? apiversion change generally means we need the spec to also change. changing this but not the underlying generations are not making sense to me.
There was a problem hiding this comment.
Done — dropped the apiVersion override entirely; the template now pins gateway.networking.k8s.io/v1. Agreed that exposing it without changing the generated spec didn't make sense.
| | `ingress.tls` | A list of [IngressTLS](https://kubernetes.io/docs/reference/kubernetes-api/service-resources/ingress-v1/#IngressSpec) items | `[]` | | ||
| | `httproute.enabled` | If `true`, a Gateway API [HTTPRoute](https://gateway-api.sigs.k8s.io/api-types/httproute/) is created (requires the Gateway API CRDs) | `false` | | ||
| | `httproute.apiVersion` | HTTPRoute apiVersion override. Defaults to `gateway.networking.k8s.io/v1` when empty | `""` | | ||
| | `httproute.parentRefs` | List of [parentRefs](https://gateway-api.sigs.k8s.io/api-types/httproute/#attaching-to-gateways) the HTTPRoute attaches to. At least one is required when enabled | `[]` | |
There was a problem hiding this comment.
| | `httproute.parentRefs` | List of [parentRefs](https://gateway-api.sigs.k8s.io/api-types/httproute/#attaching-to-gateways) the HTTPRoute attaches to. At least one is required when enabled | `[]` | | |
| | `httproute.parentRefs` | List of [parentRefs](https://gateway-api.sigs.k8s.io/reference/api-spec/1.5/spec/#parentreference) the HTTPRoute attaches to. At least one is required when enabled | `[]` | |
There was a problem hiding this comment.
Applied your suggestion in 7d7974a — the row now links to the versioned .../reference/api-spec/1.5/spec/#parentreference.
| # hostnames matched against the HTTP Host header. Defaults to [host] when empty. | ||
| # hostnames: | ||
| # - chat.example.com | ||
| hostnames: [] |
There was a problem hiding this comment.
Why multiple hostnames? the chart is deploying rocket.chat, so only the hostnames we need should be in the generated route. This is unnecessary flexibility at this stage.
There was a problem hiding this comment.
Done — removed the hostnames value; the route now derives its hostname from the chart's .Values.host only. Agreed it was unnecessary flexibility here.
| # filters applied to the main Rocket.Chat rule | ||
| filters: [] | ||
| # additionalRules (templated) prepends custom HTTPRoute rules to the main route | ||
| additionalRules: [] |
There was a problem hiding this comment.
| additionalRules: [] | |
| extraRules: [] |
just the wording we are already using.
There was a problem hiding this comment.
Applied — renamed additionalRules to extraRules to match the wording you already use.
|
No rush at all, @debdutdeb — let's not block on the video. The bats tests added here already cover the HTTPRoute rendering path ( |
|
Addressed all of the inline feedback in 7d7974a — net result is a much smaller surface (8 insertions / 99 deletions):
Tests/docs updated to match (dropped the now-obsolete hostnames/apiVersion/serveWellKnown render tests). |
Adds an optional Gateway API
HTTPRouteto therocketchatchart as an alternative to the existing Ingress, gated behindhttproute.enabled(defaultfalse). It mirrors the Ingress routing so existing deployments are unaffected.What
hostto the Rocket.Chat service via aPathPrefix/match (mirroring the Ingress).microservices.enabled,/sockjsand/websocketare routed to theddp-streamerservice (required for microservices mode, mirroring the Ingress).httproute.enabledrequires at least onehttproute.parentRefsentry; the template fails fast with a clear message otherwise.apiVersionis pinned togateway.networking.k8s.io/v1.httproute.matches,filters, andextraRulesfor customization.New values block (
httproute.*), README parameter rows, aNOTES.txthint, and bats render tests are included. Existing Ingress behavior and labels are unchanged.Validation (helm 3.18.1)
helm lintpasses with default values.httproute.enabled=truewithoutparentRefsfails with the guard message./sockjs+/websocketrules; monolith renders the single main rule.No chart version bump (handled by maintainers at release time, matching recent feature PRs).