feat: add Gateway API support, instructions on how to integrate with new Istio charms #1
Conversation
This commit adds basic support for Gateway APIs in Knative Serving. Knative provides a net-gateway-api controller to support the Gateway APIs, but it is not fully supported by Knative's Operator. This commit thus: * adds charm config for the `ingress-class`, which can optionally be set to `gateway-api.ingress.networking.knative.dev` to enable the Gateway API mode * if `ingress-class=gateway-api...`, this commit deploys the net-gateway-api controller and configures it based on charm config While we deploy and configure net-gateway-api when ingress-class is set to gateway-api, we do not actually remove the net-istio controller from the cluster. Instead, net-istio is still deployed (but sits idle). Not sure how to avoid installing it.
|
I wasn't sure if I should change the integration tests or not. Right now they use the original istio-pilot/istio-gateway, but I could refactor them to instead use our new istio. I appreciate feedback from reviewers on this. If we want these updates to be pushed back to the kubeflow team's charm, we probably don't want to modify the integration tests anytime soon since they'd be using their own istio. What we could do is include integration tests for both istio configurations. that way its easy to see which failed, and optionally release anyway (maybe tests with the new istio wouldn't block CI releases?) |
|
To make this cherry-pickable to the upstream charm, maybe we should pull the CI disabling commit out into a separate PR, that way when things get squashed down those are separate |
I've found pytest markers to be very helpful in scenarios like this, we put our itests in a new file and mark them with smth like |
| --trust \ | ||
| --config namespace=knative-serving-1 \ | ||
| --config istio.gateway.namespace=istio-system \ | ||
| --config domain.name=10.64.140.43.nip.io \ # <-- may be different if you have a different load balancer configuration |
There was a problem hiding this comment.
any idea why we need this, shouldn't the gateway name and namespace be enough since it already holds the LB data as it owns it?, what would happen if there's a discrepancy between the domain name used here and the External IP of the loadbalancer owned by the gateway?
There was a problem hiding this comment.
This is used to populate the links that knative tells you about. It goes into the config-domain configmap. So if you deploy a knative service called my-knative-thing, you can then do kubectl get route my-knative-thing --output jsonpath="{.status.url}" which would show http://my-knative-thing.my-domain-name.com. the "my-domain-name.com" part comes from this domain.name config. I think this is because we cannot really introspect the hostname based solely on the Gateway due to wildcard hosts
Which makes me wonder if we need to do something similar when using wildcards ourselves. Maybe its just not feasible to introspect those, and instead a human needs to give guidance
| protocol: HTTP | ||
| hosts: | ||
| # wildcard so we can use it for knative, which adds to the front of the domain | ||
| - "*.10.64.140.43.nip.io" |
There was a problem hiding this comment.
Is the wildcard a prereq here, meaning does the operator by default insert subdomains for workloads it deploys or is it just to add support when needed?, if it's a prereq we will need to do something about the ingress charm to support wildcards as well.
There was a problem hiding this comment.
Yeah it is a prereq. All knative urls are my-knative-service.my-domain. So we'll have to support them somehow
| # This is analogous to the config-domain configmap | ||
| # TODO: defining this gateway config did nothing. Maybe the operator doesn't support it? | ||
| {% if ingress_class == 'gateway-api.ingress.networking.knative.dev' %} |
There was a problem hiding this comment.
tbh I have no idea what I'm looking at here, what exactly is this API group and what is it used for?
There was a problem hiding this comment.
😄
this doc might help. the gateway part of this configures the external and internal gateways that knative will use when creating VirtualService or HTTPRoute objects.
by default, knative will use class Istio APIs to create everything. But if we set ingress-class: gateway-api.ingress.networking.knative.dev, that turns on the new gateway APIs. So what the jinja is doing here is:
- above here, in the
if not ingress_classblock, that is saying "if we have no ingress_class defined, apply settings for istio-classic" - then here, if ingress_class is set to gateway-api, define these gateway settings which are specific to gateway-api
- a little further down, if
ingress_classis set to anything, then add the ingress_class entry in the configmap
There was a problem hiding this comment.
its hard to read and I'm happy to edit if you have suggestions. what gave me trouble is that omitting the ingress_class means "use classic istio api", so then there's those odd looking if not ingress_class calls
| network: | ||
| # Setting this tells net-istio to not reconcile things, I think? If I set this, I don't see any VirtualServices | ||
| # created for knative services | ||
| ingress-class: gateway-api.ingress.networking.knative.dev |
There was a problem hiding this comment.
same as the previous comment, no idea what's this or what it's used for 🥲
|
This is functionally complete. Leaving it as a branch for now, but we'll pick it back up again later |
This PR:
README.mddescribing how you can connect knative to the istio and ingress fromistio-k8sandistio-ingress-k8singress_classconfigurations and features needed to use knative with the Gateway APIWhat doesn't work:
Todo:
Testing instructions
There are integration tests here, but they're from the original charm and do not currently use the new istio. For now, manual testing can be done with the instructions in
README.md.