fix istio VirtualService manifest

[tarilabs]

Seems to me the definition proposed in this changed manifest makes it working, and it's aligned with other Kubeflow components' approach.

Description

Before, I was not able to reach by e2e test (from outside the cluster) the Model Registry REST API endpoint --unless of manual p.fwd.

This solution aligns for the REST API endpoint (the one on 8080) following similar strategy of other Kubeflow components: see here for some examples.

This way, request on the gateway for /model-registry/... will be routed to the correct service accordingly.

How Has This Been Tested?

Install vanilla KF, used 1.9.0-rc.0 without networkpolicies.
(I'm skipping networkpolicies as this PR I considered propaedeutic work to later work on networkpolicy for model registry)

Execute required step of p.fwd the istio-ingressgateway per normal vanilla KF installation:

INGRESS_GATEWAY_SERVICE=$(kubectl get svc --namespace istio-system --selector="app=istio-ingressgateway" --output jsonpath='{.items[0].metadata.name}')
kubectl port-forward --namespace istio-system svc/${INGRESS_GATEWAY_SERVICE} 8080:80

In another terminal:

export KF_TOKEN="$(kubectl -n default create token default)"

curl -H "Authorization: Bearer "$KF_TOKEN http://localhost:8080/model-registry/api/model_registry/v1alpha3/registered_models

Alternative solution (explored, but not adopted here)

I've explored with alternative solution of:

spec:
  gateways:
  - kubeflow-gateway
  hosts:
  - '*'
  http:
  - match:
      - headers:
          Host:
            exact: model-registry-service.kubeflow.svc.cluster.local
    route:
    - destination:
        host: model-registry-service.kubeflow.svc.cluster.local
        port:
          number: 8080
...

which works too, using:

export KF_TOKEN="$(kubectl -n default create token default)"
curl -H "Host: model-registry-service.kubeflow.svc.cluster.local" -H "Authorization: Bearer "$KF_TOKEN http://localhost:8080/model-registry/api/model_registry/v1alpha3/registered_models

The reasons I'm not proposing this solution:

• does not align with other Kubeflow components VirtualService definitions
• would require Host: model-registry-service.kubeflow.svc.cluster.local in the header of the REST api request, which is not allowed by front-end (JS) code.

Merge criteria:

• The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
• Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has manually tested the changes and verified that the changes work

[rareddy]

this matches with my understanding of virtualservice from examples I worked before. I have not tested but I am good with it.

[dhirajsb]

This sort of solves the problem for model registry REST API. But I think we can solve it for both REST and gRPC by re-using some bits from our model registry midstream operator and maybe other virtualservice filtering capabilities.
Since this config is for a single model registry, we don't have the same dynamic hostname requirement as mids-stream. So, as long as we stick with that assumption, we should be able to create a virtualservice that should work with different gateway configs.
I need to experiment a bit and debug the envoy proxy logs and update this PR tomorrow.

[dhirajsb]

BTW, url rewrite will break any clients that are using full paths declared in the REST API.

[dhirajsb]

@tarilabs I created a PR on your repo branch tarilabs#2
Can you test it in your cluster and verify that it resolves both REST and gRPC routing without conflicting with dsp mlmd service? I'll test it more tomorrow in my cluster as well.

[tarilabs]

@tarilabs I created a PR on your repo branch tarilabs#2 Can you test it in your cluster and verify that it resolves both REST and gRPC routing without conflicting with dsp mlmd service? I'll test it more tomorrow in my cluster as well.

thank you @dhirajsb

just to cross-check and for my understanding:

• your proposed changes assumes /api/model_registry/ to be valid "cluster scope" for the single instance Model Registry (in kubeflow ns), so effectively leveraging a "common denominator" from our API paths so to avoid url rewrite. Correct?
• not entirely sure whether this in your proposed change would conflict with this one. Isn't this colliding on the same base prefix? 🤔

[dhirajsb]

Yes to cluster scope single registry for uri prefix check question.
As discussed previously, we have additional port and authority checks to avoid collisions with mlmd virtual service.

I also update my PR tarilabs#2 to simplify istio config further and tested the REST service endpoint in a local cluster and it works through the kubeflow-gateway.

[tarilabs]

Thank you @dhirajsb for the amendment in linked PR: tarilabs#2 (review)

Tested with:

export KF_TOKEN="$(kubectl -n default create token default)"
curl -H "Authorization: Bearer "$KF_TOKEN http://localhost:8080/api/model_registry/v1alpha3/registered_models

(no path prefix from CLI)

and from Notebook:

Both working as expected.

[rareddy]

/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rareddy

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:

• manifests/kustomize/OWNERS [rareddy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment