fix: istio integration and add an example #20
Conversation
Removes the deprecated service-mesh interface and instead drives istio integration based on whether an ingress is provided. This may not be sufficient as Istio integration may be useful even when an ingress is not provided. If so, we can add a second config option to enable Istio separately from the ingress.
|
Sorry I should have clarified above that the example here is written assuming the Once those charms get updated and fixes released, the example will work as is. |
Adds citation for example in README.md
| @@ -53,6 +53,12 @@ def set_pod_spec(self, event): | |||
| env = Environment( | |||
| loader=FileSystemLoader("src/templates/"), | |||
| ) | |||
|
|
|||
| if config["istio-gateway"] != "": | |||
There was a problem hiding this comment.
By default, this is a non-empty string, and I don't see where config["istio-gateway"] is emptied (unless I'm missing something).
I would like to understand your decision of not using a relation here. I probably want to see a relation updating ISTIO_ENABLED rather than a config definition doing so. I see some scenarios where a relation could handle this better: istio could not even be deployed, default-gateway can point to something else, and there's nothing ensuring the gateway exists and is used by istio.
There was a problem hiding this comment.
Sorry for the slow response, I didn't notice this comment!
You're right, it is non-empty by default (but someone could do juju config istio-gateway="") so I think it still needs handling. In hindsight, I think I should also change the default value to "" as I doubt many people by default will happen to have a gateway called seldon-gateway in the istio-system namespace. Assuming we keep the design this way (instead of change it as discussed next), what do you think about switching default to ""? Also missing in the charm currently is any validation that this Gateway exists - that's probably something that should be added (eg: lightkube.get() for Gateway object of correct name, Block if missing). Thoughts?
You're totally right about how istio might not be available, etc. - I don't like that either... At first I leaned toward a relation, but then went this way because I couldn't think of a relation that models this well. Some ideas were:
- relation with istio-gateway: that isn't enough. Istio-gateway deploys the workload that behaves as a gateway, but does not govern the
networking.istio.io/Gatewayobject (that's created by istio-pilot). - relation with istio-pilot: that could be enough as it generates
Gatewayobjects, but we'd need to add something to istio-pilot. Theingressrelation is not a good fit (it createsVirtualServices, which we explicitly do not want as Seldon will do that for itself, and does not return theGatewayname). We could create a newprovidedrelation inistio-pilotthat provides the default-gateway name, and then relateseldon:default-gateway->istio-pilot:default-gatewayas a replacement for the config. That works so long as everything goes through the single default gateway, but we've talked about supporting multiple gateways. If we supported multiple gateways, we could have a similargatewaysrelation that returns the list of available gateways, but then the seldon charm would still need some config to choose which gateway to actually use from that list (and we're back to the current implementation, except that we've got an additional relation).
What do you think about this, did you have something specific in mind? I'd love some help :)
There was a problem hiding this comment.
I get your point. Right now there is not a suitable interface that either of the istio charms provide, meaning we'll have to add it if we want to use a relation. I do not dislike the idea, we just have to be very careful where we add it and why. I didn't have something in mind when reviewing this PR, btw.
That said and based on the options you gave, I think a relation with istio-pilot could be our best option. As you said, pilot is the one actually handling Gateway and VirtualService, meaning it can create/destroy them as we relate or change the config, and is aware of them. We could do something like what you said:
If we supported multiple gateways, we could have a similar gateways relation that returns the list of available gateways, but then the seldon charm would still need some config to choose which gateway to actually use from that list
What we get from that is that we are actually ensuring the gateway we need is there and is managed by the istio charm, in this case pilot, and not just assuming is there as we are doing right now.
What do you think?
There was a problem hiding this comment.
Would this relation actually ensure the gateway is there? I don't see how it would. If we implement a relation that lets the Seldon relate to istio-pilot to get the available Gateways then the Seldon charm knows what is available, but the user still sets the desired gateway via juju config seldon gateway-to-use=xyz without that relation information. With this new relation, the config is still a "try to use gateway xyz" (just like it is now).
Having the additional relation means that Seldon could raise an error saying "Configured gateway xyz not in gateways provided by istio-pilot relation". But if that's what we're after, it might be easier to just get Seldon to look for the gateway we request directly (lightkube.client.get(Gateway, name="xyz")).
| "# NOTE:\n", | ||
| "\n", | ||
| "\n", | ||
| "This guide requires the seldon-core charm revision >55 and istio-pilot charm revision >62. As these are rolled out, you may need to deploy from the edge channel (`--channel latest/edge`) or build locally (`charmcraft pack`) for those charms. See [seldon-core](https://charmhub.io/seldon-core) and [istio-pilot](https://charmhub.io/istio-pilot) in Charmhub to check the available versions." |
There was a problem hiding this comment.
I wouldn't add the revision number here, but a supported channel instead.
There was a problem hiding this comment.
I agree in principle, but the revision we depend on here is newer than edge (the versions we need are still in branches). I was relying on some luck though, as >55/>62 might not be enough if we push something else to one of those charms without fixing the things this demo needs :) I'd love suggestions on a better way to phrase it, I just couldn't think of any
There was a problem hiding this comment.
This is even worse than that, if we push a new revision for istio 1.5 the revision would be >62 but an entirely different charm. We can never rely on a range of revision numbers. So we either hard pin it to 55 and 62 or make the effort of pushing it to a channel as soon as possible.
There was a problem hiding this comment.
Very true. What do you propose as improved language? It seems wrong to have no warning at all, but until istio is patched we can't actually say what version is needed. Maybe something like "requires istio-pilot from a latest track that has a charm revision >62"? More confusing, but more explicit
| } | ||
| ], | ||
| "source": [ | ||
| "gateway_ip=!kubectl -n $juju_model_name get svc istio-ingressgateway -o yaml -o jsonpath='{.status.loadBalancer.ingress[0].ip}'\n", |
There was a problem hiding this comment.
s/istio-ingressgateway/istio-ingressgateway-workload due to recent changes on istio-gateway
Closes #18
This fixes the integration with Istio, which previously needed a deprecated relation with an Istio charm. Also added are several examples of using the SeldonDeployments with and without authentication
Testing Instructions
Use the recipe prescribed in
/examples/ingress_canary_and_auth.ipynb, which should demo all new features