Feat/gateway api support #1000
Feat/gateway api support #1000
Conversation
✅ Deploy Preview for kamaji-documentation canceled.
|
|
Congratulations for opening the 1000th Kamaji's PR! Besides that, we usually prefer receiving discussions to plan feature enhancements given that PR not getting merged could decrease perception the Open Source project score. Besides that, there are several remarks on the changes you put in place (especially regarding CRDs) but I would try to address that once we got something stable in the CI. |
|
#1001 discussion opened, thanks for pointing that out. Even if the PR is in draft I think we could keep this open cause overall we see a lot of benefits in the addition of this functionality, but let me know sorry about that @prometherion |
| $(HELM) upgrade --install cert-manager jetstack/cert-manager --namespace certmanager-system --create-namespace --set "installCRDs=true" | ||
|
|
||
| gateway-api: | ||
| kubectl apply --server-side -f https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.4.0/standard-install.yaml |
There was a problem hiding this comment.
We do already have several dependencies, such as Cert Manager: if we could manage those via Helm it would be better (not happy with Metal LB which requires a lot of mangling).
There was a problem hiding this comment.
As far as I my research went, there are no official helm charts. Could try to add an unofficial one?
I do see your point with the mangling.
There was a problem hiding this comment.
Then, no worries: it's fine, we're already doing the same for Metal LB which is used mostly for testing Load Balancer integrations, even tho several adopters are using it in production.
Ignore my request, it's fine!
There was a problem hiding this comment.
@alfredosa the version should match though. For now I think 1.2.0 will be easier to test, we can revisit later.
| kind: CustomResourceDefinition | ||
| metadata: | ||
| annotations: | ||
| cert-manager.io/inject-ca-from: {{ include "kamaji-crds.certManagerAnnotation" . }} |
There was a problem hiding this comment.
This templating must be kept since it provides idempotence.
There was a problem hiding this comment.
Do you know if this is a tool version issue as well? This wasn't intended.
charts/kamaji-crds/templates/kamaji.clastix.io_kubeconfiggenerators.yaml
Show resolved
Hide resolved
controllers/resources.go
Outdated
| resources = append(resources, getDataStoreMigratingCleanup(config.client, config.KamajiNamespace)...) | ||
| resources = append(resources, getKubernetesIngressResources(config.client)...) | ||
|
|
||
| resources = append(resources, getKubernetesGatewayResources(config.client)...) |
There was a problem hiding this comment.
These resources should be added only if the management cluster serves those APIs.
We could manage that maybe in the webhook rather than on the factory of resources.
There was a problem hiding this comment.
Maybe a discovery client + webhook for early feedback + controller logic for runtime?
There was a problem hiding this comment.
I see you suggested this below! I think this would be a nice check to not have extra flags! No gateway APIs = no change in behavior
The runtime would be for picking up if the crds are added after kamaji by some sort of hiccup
There was a problem hiding this comment.
If CRDs are installed at execution time, is good enough to share in the docs to restart the controller: it's a legit expected behaviour.
| Owns(&appsv1.Deployment{}). | ||
| Owns(&corev1.Service{}). | ||
| Owns(&networkingv1.Ingress{}). | ||
| Owns(&gatewayv1.HTTPRoute{}). |
There was a problem hiding this comment.
This will block the start of Controllers when the management cluster has no types installed.
We don't want to force Kamaji admin to install the Gateway API even tho it's not used for their clusters: we need a discovery mechanism which adds the watch only if that type in available in the API server.
There was a problem hiding this comment.
Is this done somewhere in the code for another API?
There was a problem hiding this comment.
Not in the same way, but similarly: https://github.com/projectcapsule/capsule/blob/866c69ffc322f0cff604601f314ef9b79673eae8/pkg/indexer/indexer.go#L48-L52
We could detect if the API is server by using the Kubernetes' client discovery.DiscoveryClient, as I did with Capsule: https://github.com/projectcapsule/capsule-proxy/blob/842b0c4fcfb1e43e4571275de6ae37e3daf4b44e/internal/modules/utils/gvk.go#L16-L19
This is a dynamic approach trying to assist users that doesn't know what's installed or not: what we could do is to provide a feature gate, such as: --feature-gates=EnableGatewayAPISupport=true and skip all the dynamic check if the API is installed.
3 participants
At the moment Ingres is being used optionally. We believe optionally the Gateway API should be an option. Extending the controller to be able to create both with a GatewayRouteSpec:
(TCP tbd)