Add support for external routes to proxyService, use for model registry #3203
Conversation
mturley
changed the title
mturley
force-pushed
the
RHOAIENG-10133-mr-external-route
branch
4 times, most recently
from
39f3233
to
e51dd17
Compare
backend/src/utils/proxy.ts
Outdated
| const k8sService = ( | ||
| await fastify.kube.coreV1Api.readNamespacedService(serviceName, namespace) | ||
| ).body; | ||
| const address = k8sService.metadata.annotations[annotation]; |
There was a problem hiding this comment.
annotations are optional.
How come typescript isn't complaining about this in our back end :(
There was a problem hiding this comment.
Oof, good question...
There was a problem hiding this comment.
I think our backend tsconfig doesn't have strict or strictNullChecks. Maybe we want a separate tech debt issue for that.
mturley
force-pushed
the
RHOAIENG-10133-mr-external-route
branch
2 times, most recently
from
71550c0
to
2c74818
Compare
|
@christianvogt addressed your feedback |
mturley
force-pushed
the
RHOAIENG-10133-mr-external-route
branch
2 times, most recently
from
bfa6cd6
to
31e4653
Compare
|
/retest |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3203 +/- ##
==========================================
+ Coverage 85.41% 85.43% +0.02%
==========================================
Files 1275 1277 +2
Lines 28004 28082 +78
Branches 7451 7487 +36
==========================================
+ Hits 23920 23993 +73
- Misses 4084 4089 +5
... and 15 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
mturley
force-pushed
the
RHOAIENG-10133-mr-external-route
branch
2 times, most recently
from
30344e9
to
dca3fb2
Compare
Signed-off-by: Mike Turley <[email protected]>
mturley
force-pushed
the
RHOAIENG-10133-mr-external-route
branch
from
dca3fb2
to
97e0d6b
Compare
mturley
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
|
||
| export enum ServiceAddressAnnotation { | ||
| EXTERNAL_REST = 'routing.opendatahub.io/external-address-rest', | ||
| EXTERNAL_GRPC = 'routing.opendatahub.io/external-address-grpc', |
There was a problem hiding this comment.
Are we expecting connecting the proxy server through grpc? I know between servers can be done, but since it's a proxified call browsers don't support grpc natively
There was a problem hiding this comment.
We aren't currently using that annotation, I just added it in the constant here because it was added at the same time on the service in case we may need it later. Maybe I should just remove it?
There was a problem hiding this comment.
I think you can keep it because it's in the generic types folder -- but I agree with Lucas, there is not a likelihood of this working through the http-proxy -- I think Pipelines MLMD uses envoy proxy which is a http endpoint that converts it... @Gkrumbach07 would have to confirm that though.
|
the pipelines construct url stuff looks to be in working order |
|
Tested on cluster and looks to be working and connecting to the annotation route. |
|
/retest |
1 similar comment
|
/retest |
|
Self-approving with the LGTM from Gage and no more unaddressed feedback. Thanks guys |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
Signed-off-by: Mike Turley <[email protected]>
mturley
force-pushed
the
RHOAIENG-10133-mr-external-route
branch
from
2498217
to
bc1eb37
Compare
|
/lgtm |
openshift-merge-bot
bot
merged commit da0aa67
into
opendatahub-io:main
Closes https://issues.redhat.com/browse/RHOAIENG-10133 and https://issues.redhat.com/browse/RHOAIENG-9037
Description
Updates
proxyServiceto support configuring services with an optionaladdressAnnotation. If theaddressAnnotationis omitted, the existing behavior will apply, so this PR should not affect the behavior of proxied services other than model registry. If theaddressAnnotationisEXTERNAL_RESTorEXTERNAL_GRPC, the proxy will fetch the k8s Service named in the request URL and look for a corresponding annotation. If present, the proxy will use that annotation's value as the upstream URL. If it's missing, the proxy will fall back to the existing behavior (constructing an internal.svc.cluster.localURL), which will provide compatibility with non-Istio model registry services that don't require the external URL.How Has This Been Tested?
On ods-qe-psi-08, we deployed a model registry from the istio TLS samples and observed that the Service has the
routing.opendatahub.io/external-address-restannotation on it. We then deployed the image for this PR using a devFlag and saw that the model registry page works when proxying requests through this new route.We then also created a new registry using the settings page and reused the database details from the sample registry, and saw that the new istio spec provided on creation works correctly. That registry also became available and worked with the new proxy logic.
Test Impact
No tests for backend code, unfortunately.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main