Conversation
|
|
|
|
191e471 to
c61bb86
Compare
|
c61bb86 to
91af5d2
Compare
|
cd7489c to
ac82d2b
Compare
|
|
ac82d2b to
0984d68
Compare
|
| return nil | ||
| } | ||
|
|
||
| func addNextcloudHTTPRoute(comp *vshnv1.VSHNNextcloud, svc *runtime.ServiceRuntime) *xfnproto.Result { |
There was a problem hiding this comment.
This function is basically redefined 4 times in the PR. And not even consistently:
- Some pass
AllowDeletionand some don't. - Some return fatals and some only warnings.
I'd suggest to move it to common and have it accept a common.HTTPRouteConfig.
There was a problem hiding this comment.
Refactored this.
| } | ||
|
|
||
| func addNextcloudHTTPRoute(comp *vshnv1.VSHNNextcloud, svc *runtime.ServiceRuntime) *xfnproto.Result { | ||
| gatewayName := svc.Config.Data["httpGatewayName"] |
There was a problem hiding this comment.
Checking if it's set would be nice. If not set we should probably do a fatal.
|
| ServiceNameSuffix: svcNameSuffix, | ||
| ServicePortNumber: 3000, | ||
| }, | ||
| }, runtime.KubeOptionAllowDeletion); err != nil { |
There was a problem hiding this comment.
I'm not sure about the AllowDeletion TBH.
On one hand we'd need it to seamlessly switch from gateway to ingress. The thing is, the other way around would currently probably not work, because we don't have the allow deletion on the ingress objects as far as I know.
There was a problem hiding this comment.
Valid point. I'll remove AllowDeletion for the gateway related resources.
This allows HTTPRoutes to be used instead of ingress on servala clusters.
f1e7ac9 to
867917c
Compare
|
mdnix
left a comment
There was a problem hiding this comment.
Overall looking good.
Also we will have to add a redirect http -> https.
But I will take care of that on infra side.
| svc.Log.Info("Adding HTTPRoute for Collabora") | ||
|
|
||
| return common.ApplyHTTPRoute(comp, svc, common.HTTPRouteConfig{ | ||
| FQDNs: []string{comp.Spec.Parameters.Service.Collabora.FQDN}, |
There was a problem hiding this comment.
We should check if this FQDN is even set before doing ApplyHTTPRoute
There was a problem hiding this comment.
good catch, thanks!
|
|
||
| err = AddCollaboraIngress(comp, svc) | ||
| if errors.Is(err, common.ErrHTTPGatewayNotConfigured) { | ||
| return runtime.NewFatalResult(fmt.Errorf("Failed to add Collabora Ingress: %w", err)) |
There was a problem hiding this comment.
| return runtime.NewFatalResult(fmt.Errorf("Failed to add Collabora Ingress: %w", err)) | |
| return runtime.NewFatalResult(fmt.Errorf("failed to add Collabora Ingress: %w", err)) |
There was a problem hiding this comment.
fair but actually have multiple occurrences of this being capitalized already xD
| } | ||
| func AddCollaboraIngress(comp *vshnv1.VSHNNextcloud, svc *runtime.ServiceRuntime) error { | ||
|
|
||
| if svc.Config.Data["routeType"] == "HTTPRoute" { |
There was a problem hiding this comment.
This should be a const in common/httproute.go
const RouteTypeHTTPRoute = "HTTPRoute"`and hide the check behind
func IsHTTPRouteMode(svc *runtime.ServiceRuntime) bool {
return svc.Config.Data["routeType"] == RouteTypeHTTPRoute
}| }) | ||
| } | ||
|
|
||
| annotations := getIngressAnnotations(svc, nil) |
There was a problem hiding this comment.
These are not necessary on the XLS since we don't use the certmanager shim?
There was a problem hiding this comment.
ahh darn this was a leftover from my annotation shenanigans
| // HTTP upstream, so attach a BackendConfigPolicy that originates TLS to | ||
| // the keycloakx-http Service, validating against the CA secret created | ||
| // by common.CreateTLSCerts ("tls-ca-certificate" in the instance ns). | ||
| svcName := comp.GetName() + "-keycloakx-http" |
There was a problem hiding this comment.
I noticed that we don't react to the relativePath field in the VSHNKeycloak claim
k explain vshnkeycloak.spec.parameters.service.relativePath
GROUP: vshn.appcat.vshn.io
KIND: VSHNKeycloak
VERSION: v1
FIELD: relativePath <string>
DESCRIPTION:
RelativePath on which Keycloak will listen.If a user sets that it completely gets ignored and we always set path to /
There was a problem hiding this comment.
true I didnt consider that. should be fixed now.
867917c to
0e90424
Compare
|
Summary
Adds
HTTPRoute(Gateway API) as an alternative toIngressfor Forgejo, Keycloak, and Nextcloud (incl. Collabora). Shared helper lives inpkg/comp-functions/functions/common/httproute.go. Enables Servala clusters to route via Gateway API.Component PR: vshn/component-appcat#1147
Checklist
/mergecomment.