From 536fc9b2676f15bb204c604cc51e1d74d5209587 Mon Sep 17 00:00:00 2001 From: Sivanantham <90966311+sivanantha321@users.noreply.github.com> Date: Thu, 19 Sep 2024 08:47:44 +0530 Subject: [PATCH] Fix explainer endpoint not working with path based routing (#3257) * Fix explainer not working with path based routing Signed-off-by: Sivanantham Chinnaiyan * Add test Signed-off-by: Sivanantham Chinnaiyan * Add explainer e2e test for path based routing Signed-off-by: Sivanantham Chinnaiyan * Rebase master Signed-off-by: Sivanantham Chinnaiyan --------- Signed-off-by: Sivanantham Chinnaiyan --- .github/workflows/e2e-test.yml | 6 + pkg/constants/constants.go | 4 + .../reconcilers/ingress/ingress_reconciler.go | 35 +++ .../ingress/ingress_reconciler_test.go | 214 +++++++++++++++++- test/e2e/common/utils.py | 4 +- test/e2e/explainer/test_art_explainer.py | 1 + 6 files changed, 260 insertions(+), 4 deletions(-) diff --git a/.github/workflows/e2e-test.yml b/.github/workflows/e2e-test.yml index ebec0c8b949..20ad80a1ef3 100644 --- a/.github/workflows/e2e-test.yml +++ b/.github/workflows/e2e-test.yml @@ -552,6 +552,12 @@ jobs: name: ${{ env.TRANSFORMER_ARTIFACT_PREFIX }}-${{ env.IMAGE_TRANSFORMER_IMG }}-${{ github.sha }} path: ./tmp + - name: Download Art Explainer image + uses: actions/download-artifact@v4 + with: + name: ${{ env.EXPLAINER_ARTIFACT_PREFIX }}-${{ env.ART_IMG }}-${{ github.sha }} + path: ./tmp + - name: Load docker images uses: ./.github/actions/load-docker-images with: diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index d5cdd068c53..14d09a733c7 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -560,6 +560,10 @@ func ExplainPrefix() string { return "^/v1/models/[\\w-]+:explain$" } +func PathBasedExplainPrefix() string { + return "(/v1/models/[\\w-]+:explain)$" +} + func VirtualServiceHostname(name string, predictorHostName string) string { index := strings.Index(predictorHostName, ".") return name + predictorHostName[index:] diff --git a/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go b/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go index 2cd81959da7..18e80025fa3 100644 --- a/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go +++ b/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go @@ -486,6 +486,41 @@ func createIngress(isvc *v1beta1.InferenceService, useDefault bool, config *v1be url.Path = strings.TrimSuffix(path, "/") // remove trailing "/" if present url.Host = config.IngressDomain // In this case, we have a path-based URL so we add a path-based rule + if isvc.Spec.Explainer != nil { + httpRoutes = append(httpRoutes, &istiov1beta1.HTTPRoute{ + Match: []*istiov1beta1.HTTPMatchRequest{ + { + Uri: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: url.Path + constants.PathBasedExplainPrefix(), + }, + }, + Authority: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: constants.HostRegExp(url.Host), + }, + }, + Gateways: []string{config.IngressGateway}, + }, + }, + Rewrite: &istiov1beta1.HTTPRewrite{ + UriRegexRewrite: &istiov1beta1.RegexRewrite{ + Match: url.Path + constants.PathBasedExplainPrefix(), + Rewrite: `\1`, + }, + }, + Route: []*istiov1beta1.HTTPRouteDestination{ + createHTTPRouteDestination(config.LocalGatewayServiceName), + }, + Headers: &istiov1beta1.Headers{ + Request: &istiov1beta1.Headers_HeaderOperations{ + Set: map[string]string{ + "Host": network.GetServiceHostname(expBackend, isvc.Namespace), + }, + }, + }, + }) + } httpRoutes = append(httpRoutes, &istiov1beta1.HTTPRoute{ Match: []*istiov1beta1.HTTPMatchRequest{ { diff --git a/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler_test.go b/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler_test.go index 6afd76f0961..c53e7589030 100644 --- a/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler_test.go +++ b/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler_test.go @@ -22,8 +22,6 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "github.com/kserve/kserve/pkg/apis/serving/v1beta1" - "github.com/kserve/kserve/pkg/constants" "github.com/onsi/gomega" gomegaTypes "github.com/onsi/gomega/types" "google.golang.org/protobuf/testing/protocmp" @@ -34,6 +32,9 @@ import ( "knative.dev/pkg/apis" duckv1 "knative.dev/pkg/apis/duck/v1" "knative.dev/pkg/network" + + "github.com/kserve/kserve/pkg/apis/serving/v1beta1" + "github.com/kserve/kserve/pkg/constants" ) func TestCreateVirtualService(t *testing.T) { @@ -948,6 +949,215 @@ func TestCreateVirtualService(t *testing.T) { }, }, }, + }, { + name: "found predictor and explainer status with path template", + ingressConfig: &v1beta1.IngressConfig{ + IngressGateway: constants.KnativeIngressGateway, + IngressServiceName: "someIngressServiceName", + LocalGateway: constants.KnativeLocalGateway, + LocalGatewayServiceName: "knative-local-gateway.istio-system.svc.cluster.local", + UrlScheme: "http", + IngressDomain: "my-domain.com", + PathTemplate: "/serving/{{ .Namespace }}/{{ .Name }}", + DisableIstioVirtualHost: false, + }, + useDefault: false, + componentStatus: &v1beta1.InferenceServiceStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: v1beta1.PredictorReady, + Status: corev1.ConditionTrue, + }, + { + Type: v1beta1.ExplainerReady, + Status: corev1.ConditionTrue, + }, + }, + }, + Components: map[v1beta1.ComponentType]v1beta1.ComponentStatusSpec{ + v1beta1.ExplainerComponent: { + URL: &apis.URL{ + Scheme: "http", + Host: explainerHostname, + }, + Address: &duckv1.Addressable{ + URL: &apis.URL{ + Scheme: "http", + Host: network.GetServiceHostname(constants.ExplainerServiceName(serviceName), namespace), + }, + }, + }, + v1beta1.PredictorComponent: { + URL: &apis.URL{ + Scheme: "http", + Host: predictorHostname, + }, + Address: &duckv1.Addressable{ + URL: &apis.URL{ + Scheme: "http", + Host: network.GetServiceHostname(constants.PredictorServiceName(serviceName), namespace), + }, + }, + }, + }, + }, + expectedService: &istioclientv1beta1.VirtualService{ + ObjectMeta: metav1.ObjectMeta{Name: serviceName, Namespace: namespace, Annotations: annotations, Labels: labels}, + Spec: istiov1beta1.VirtualService{ + Hosts: []string{serviceInternalHostName, serviceHostName, "my-domain.com"}, + Gateways: []string{constants.KnativeLocalGateway, constants.IstioMeshGateway, constants.KnativeIngressGateway}, + Http: []*istiov1beta1.HTTPRoute{ + { + Match: []*istiov1beta1.HTTPMatchRequest{ + { + Uri: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: constants.ExplainPrefix(), + }, + }, + Authority: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: constants.HostRegExp(network.GetServiceHostname(serviceName, namespace)), + }, + }, + Gateways: []string{constants.KnativeLocalGateway, constants.IstioMeshGateway}, + }, + { + Uri: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: constants.ExplainPrefix(), + }, + }, + Authority: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: constants.HostRegExp(constants.InferenceServiceHostName(serviceName, namespace, domain)), + }, + }, + Gateways: []string{constants.KnativeIngressGateway}, + }, + }, + Route: []*istiov1beta1.HTTPRouteDestination{ + { + Destination: &istiov1beta1.Destination{Host: constants.LocalGatewayHost, Port: &istiov1beta1.PortSelector{Number: constants.CommonDefaultHttpPort}}, + Weight: 100, + }, + }, + Headers: &istiov1beta1.Headers{ + Request: &istiov1beta1.Headers_HeaderOperations{Set: map[string]string{ + "Host": network.GetServiceHostname(constants.ExplainerServiceName(serviceName), namespace)}, + }, + }, + }, + { + Match: []*istiov1beta1.HTTPMatchRequest{ + { + Authority: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: constants.HostRegExp(network.GetServiceHostname(serviceName, namespace)), + }, + }, + Gateways: []string{constants.KnativeLocalGateway, constants.IstioMeshGateway}, + }, + { + Authority: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: constants.HostRegExp(constants.InferenceServiceHostName(serviceName, namespace, domain)), + }, + }, + Gateways: []string{constants.KnativeIngressGateway}, + }, + }, + Route: []*istiov1beta1.HTTPRouteDestination{ + { + Destination: &istiov1beta1.Destination{Host: constants.LocalGatewayHost, Port: &istiov1beta1.PortSelector{Number: constants.CommonDefaultHttpPort}}, + Weight: 100, + }, + }, + Headers: &istiov1beta1.Headers{ + Request: &istiov1beta1.Headers_HeaderOperations{Set: map[string]string{ + "Host": network.GetServiceHostname(constants.PredictorServiceName(serviceName), namespace)}}, + }, + }, + { + Match: []*istiov1beta1.HTTPMatchRequest{ + { + Uri: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: fmt.Sprintf("/serving/%s/%s%s", namespace, serviceName, constants.PathBasedExplainPrefix()), + }, + }, + Authority: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: constants.HostRegExp("my-domain.com"), + }, + }, + Gateways: []string{constants.KnativeIngressGateway}, + }, + }, + Rewrite: &istiov1beta1.HTTPRewrite{ + UriRegexRewrite: &istiov1beta1.RegexRewrite{ + Match: fmt.Sprintf("/serving/%s/%s%s", namespace, serviceName, constants.PathBasedExplainPrefix()), + Rewrite: `\1`, + }, + }, + Route: []*istiov1beta1.HTTPRouteDestination{ + createHTTPRouteDestination("knative-local-gateway.istio-system.svc.cluster.local"), + }, + Headers: &istiov1beta1.Headers{ + Request: &istiov1beta1.Headers_HeaderOperations{ + Set: map[string]string{ + "Host": network.GetServiceHostname(constants.ExplainerServiceName(serviceName), namespace), + }, + }, + }, + }, + { + Match: []*istiov1beta1.HTTPMatchRequest{ + { + Uri: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Prefix{ + Prefix: fmt.Sprintf("/serving/%s/%s/", namespace, serviceName), + }, + }, + Authority: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: constants.HostRegExp("my-domain.com"), + }, + }, + Gateways: []string{constants.KnativeIngressGateway}, + }, + { + Uri: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Exact{ + Exact: fmt.Sprintf("/serving/%s/%s", namespace, serviceName), + }, + }, + Authority: &istiov1beta1.StringMatch{ + MatchType: &istiov1beta1.StringMatch_Regex{ + Regex: constants.HostRegExp("my-domain.com"), + }, + }, + Gateways: []string{constants.KnativeIngressGateway}, + }, + }, + Rewrite: &istiov1beta1.HTTPRewrite{ + Uri: "/", + }, + Route: []*istiov1beta1.HTTPRouteDestination{ + createHTTPRouteDestination("knative-local-gateway.istio-system.svc.cluster.local"), + }, + Headers: &istiov1beta1.Headers{ + Request: &istiov1beta1.Headers_HeaderOperations{ + Set: map[string]string{ + "Host": network.GetServiceHostname(constants.PredictorServiceName(serviceName), namespace), + }, + }, + }, + }, + }, + }, + }, }, { name: "found predictor status with default suffix", ingressConfig: &v1beta1.IngressConfig{ diff --git a/test/e2e/common/utils.py b/test/e2e/common/utils.py index 30e72a2567d..d72490d7e2c 100644 --- a/test/e2e/common/utils.py +++ b/test/e2e/common/utils.py @@ -191,8 +191,8 @@ async def explain_response(client, service_name, input_path) -> Dict: namespace=KSERVE_TEST_NAMESPACE, version=constants.KSERVE_V1BETA1_VERSION, ) - cluster_ip, host, _ = get_isvc_endpoint(isvc) - url = f"http://{cluster_ip}" + cluster_ip, host, path = get_isvc_endpoint(isvc) + url = f"http://{cluster_ip}{path}" headers = {"Host": host} with open(input_path) as json_file: data = json.load(json_file) diff --git a/test/e2e/explainer/test_art_explainer.py b/test/e2e/explainer/test_art_explainer.py index c9ed1bae6db..f83b1ac905c 100644 --- a/test/e2e/explainer/test_art_explainer.py +++ b/test/e2e/explainer/test_art_explainer.py @@ -35,6 +35,7 @@ kserve_client = KServeClient(config_file=os.environ.get("KUBECONFIG", "~/.kube/config")) +@pytest.mark.path_based_routing @pytest.mark.explainer @pytest.mark.asyncio(scope="session") async def test_tabular_explainer(rest_v1_client):