feat: add annotations for external service host and port in MR service, fixes RHOAIENG-11092 #125
Conversation
…e, fixes RHOAIENG-11092
|
@mturley I added a default annotation |
| routing.opendatahub.io/external-address: {{.Name}}-rest.{{.Spec.Istio.Gateway.Domain}}:{{.Spec.Istio.Gateway.Rest.Port}} | ||
| routing.opendatahub.io/external-address-rest: {{.Name}}-rest.{{.Spec.Istio.Gateway.Domain}}:{{.Spec.Istio.Gateway.Rest.Port}} | ||
| routing.opendatahub.io/external-address-grpc: {{.Name}}-grpc.{{.Spec.Istio.Gateway.Domain}}:{{.Spec.Istio.Gateway.Grpc.Port}} |
There was a problem hiding this comment.
why not just making the default one to be the rest, so to avoid the routing.opendatahub.io/external-address-rest annotation which seems redundant?
There was a problem hiding this comment.
For uniformity across protocols. And it also makes it clear that the rest endpoint is the default endpoint as well. Does that make sense?
There was a problem hiding this comment.
The dashboard could just use the -rest endpoint if we want to, since it specifically expects the REST API. Not sure what you mean by uniformity across protocols being a reason for the default annotation.
Also, are these "external" in the sense described by this doc or should we be using "public"? (i.e. are they accessible outside the cluster?) https://docs.google.com/document/d/1UA9c8DWVQg1nzkiw3b34xofL8KCw_MM2hD5MOf-P2VU/edit
There was a problem hiding this comment.
I was thinking that dashboard doesn't care about the grpc protocol, so it would be easier to have a default annotation routing.opendatahub.io/external-address for clients like dashboard that always use a default/common protocol for the service.
For other clients, they can look for the endpoint for a specific protocol by adding the protocol suffix. Hence the redundancy.
I am hesitant about using public, since that has RBAC connotations, i.e. whether they are publicly accessible i.e. not secured. External is external to the mesh right now.
There was a problem hiding this comment.
The dashboard does use gRPC for DSP, just not ModelRegistry currently. If we extend this pattern to other services we'll want to have each usage be explicit about what protocol/API it's using.
There was a problem hiding this comment.
I'll drop the annotation without the protocol suffix in that case.
There was a problem hiding this comment.
Thanks! Yeah I think that makes sense for our use case
…e, fixes RHOAIENG-11092 (opendatahub-io#125) * feat: add annotations for external service host and port in MR service, fixes RHOAIENG-11092 * fix: remove unprefixed non protocol annotation
Description
Add annotations for external service host and port in k8s MR service
Fixes RHOAIENG-11092
How Has This Been Tested?
Tested istio and non-istio samples to look for annotations being set correctly in istio sample only.
Merge criteria: