Feature kandrio path based (kubeflow#2357)

* Take disableIstioVirtualHost and urlScheme from ingressConfig

Commit 48f45eb introduced disableIstioVirtualHost in ingressConfig
and updated various functions to take this as an option. Update codebase
to use ingressConfig directly. This way we can use the urlScheme and
later on the pathTemplate without changing the prototype of the methods.

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>

* Introduce pathTemplate in ingressConfig

Extend the IngressConfig struct with a pathTemplate field to enable
path-based routing. If this is set the user must also provide
ingressDomain and urlScheme.

Validate the given template just by trying to parse it. The user is
responsible for providing a valid template that is unique for a given
inference service, for example, by using both namespace and name in the
template.

Introduce a path module (similar to the existing domain) that has
GenerateUrlPath() helper. This will will try to render the pathTemplate
based on the name and namespace of an isvc.

Add unittests for this new helper.

Refs kserve/kserve#2257

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
Signed-off-by: Kostis Andriopoulos <kandrio@arrikto.com>

* Add path-based rule in top-level VirtualService

Extend the Ingress reconciler to add a matching rule in the top-level
VirtualService of each InferenceService if the 'pathTemplate' field is
not empty.

Specifically adds a VirtualService rule that redirects path-based requests from
the public to the local gateway. Also extend the list of hosts to include the
ingressDomain.

Modify existing getServiceUrl() to take into account pathTemplate. Update
unittests accordingly.

Refs kserve/kserve#2257

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
Signed-off-by: Kostis Andriopoulos <kandrio@arrikto.com>

* Add e2e tests for path-based routing

Extend e2e tests to test path-based routing. Specifically, extend
the test-fast job, to patch the inferenceservice-config to set
pathTemplate, ingressDomain, and urlScheme and re-run the fast tests.

Update predict() to take into account service urls with paths.

Closes kserve/kserve#2257

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>

---------

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
Signed-off-by: Kostis Andriopoulos <kandrio@arrikto.com>
Co-authored-by: Dimitris Aragiorgis <dimara@arrikto.com>

.github/workflows/e2e-test.yml

@@ -159,6 +159,16 @@ jobs:
         run: |
           ./test/scripts/gh-actions/run-e2e-tests.sh "fast or pmml or graph"
 
+          kubectl get pods -n kserve
+      - name: Patch inferenceservice config
+        run : |
+          kubectl patch configmaps -n kserve inferenceservice-config --patch-file config/overlays/test/configmap/inferenceservice-ingress.yaml
+          kubectl describe configmaps -n kserve inferenceservice-config
+      - name: Run E2E tests with path-based routing
+        timeout-minutes: 40
+        run: |
+          ./test/scripts/gh-actions/run-e2e-tests.sh fast
+
           kubectl get pods -n kserve
       - name: Check system status
         if: always()

cmd/manager/main.go

@@ -180,7 +180,7 @@ func main() {
 		Scheme: mgr.GetScheme(),
 		Recorder: eventBroadcaster.NewRecorder(
 			mgr.GetScheme(), v1.EventSource{Component: "v1beta1Controllers"}),
-	}).SetupWithManager(mgr, deployConfig, ingressConfig.DisableIstioVirtualHost); err != nil {
+	}).SetupWithManager(mgr, deployConfig, ingressConfig); err != nil {
 		setupLog.Error(err, "unable to create controller", "v1beta1Controller", "InferenceService")
 		os.Exit(1)
 	}

config/overlays/test/configmap/inferenceservice-ingress.yaml

@@ -0,0 +1,18 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: inferenceservice-config
+  namespace: kserve
+data:
+  ingress: |-
+    {
+        "ingressGateway" : "knative-serving/knative-ingress-gateway",
+        "ingressService" : "istio-ingressgateway.istio-system.svc.cluster.local",
+        "localGateway": "knative-serving/knative-local-gateway",
+        "localGatewayService" : "knative-local-gateway.istio-system.svc.cluster.local",
+        "ingressClassName" : "istio",
+        "domainTemplate": "{{ .Name }}-{{ .Namespace }}.{{ .IngressDomain }}",
+        "urlScheme": "http",
+        "pathTemplate": "/serving/{{ .Namespace }}/{{ .Name }}",
+        "ingressDomain": "mydomain.com"
+    }

pkg/apis/serving/v1beta1/configmap.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"text/template"
 
 	"github.com/kserve/kserve/pkg/constants"
 	v1 "k8s.io/api/core/v1"
@@ -74,6 +75,7 @@ type IngressConfig struct {
 	DomainTemplate          string  `json:"domainTemplate,omitempty"`
 	UrlScheme               string  `json:"urlScheme,omitempty"`
 	DisableIstioVirtualHost bool    `json:"disableIstioVirtualHost,omitempty"`
+	PathTemplate            string  `json:"pathTemplate,omitempty"`
 }
 
 // +kubebuilder:object:generate=false
@@ -114,6 +116,19 @@ func NewIngressConfig(cli client.Client) (*IngressConfig, error) {
 		if ingressConfig.IngressGateway == "" || ingressConfig.IngressServiceName == "" {
 			return nil, fmt.Errorf("invalid ingress config - ingressGateway and ingressService are required")
 		}
+		if ingressConfig.PathTemplate != "" {
+			// TODO: ensure that the generated path is valid, that is:
+			// * both Name and Namespace are used to avoid collisions
+			// * starts with a /
+			// For now simply check that this is a valid template.
+			_, err := template.New("path-template").Parse(ingressConfig.PathTemplate)
+			if err != nil {
+				return nil, fmt.Errorf("Invalid ingress config, unable to parse pathTemplate: %v", err)
+			}
+			if ingressConfig.IngressDomain == "" {
+				return nil, fmt.Errorf("Invalid ingress config - igressDomain is required if pathTemplate is given")
+			}
+		}
 	}
 
 	if ingressConfig.DomainTemplate == "" {

pkg/controller/v1beta1/inferenceservice/controller.go

@@ -205,7 +205,7 @@ func (r *InferenceServiceReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	} else {
 		reconciler := ingress.NewIngressReconciler(r.Client, r.Scheme, ingressConfig)
 		r.Log.Info("Reconciling ingress for inference service", "isvc", isvc.Name)
-		if err := reconciler.Reconcile(isvc, ingressConfig.DisableIstioVirtualHost); err != nil {
+		if err := reconciler.Reconcile(isvc); err != nil {
 			return reconcile.Result{}, errors.Wrapf(err, "fails to reconcile ingress")
 		}
 	}
@@ -274,13 +274,13 @@ func inferenceServiceStatusEqual(s1, s2 v1beta1api.InferenceServiceStatus, deplo
 	return equality.Semantic.DeepEqual(s1, s2)
 }
 
-func (r *InferenceServiceReconciler) SetupWithManager(mgr ctrl.Manager, deployConfig *v1beta1api.DeployConfig, disableIstioVirtualHost bool) error {
+func (r *InferenceServiceReconciler) SetupWithManager(mgr ctrl.Manager, deployConfig *v1beta1api.DeployConfig, ingressConfig *v1beta1api.IngressConfig) error {
 	if deployConfig.DefaultDeploymentMode == string(constants.RawDeployment) {
 		return ctrl.NewControllerManagedBy(mgr).
 			For(&v1beta1api.InferenceService{}).
 			Owns(&appsv1.Deployment{}).
 			Complete(r)
-	} else if disableIstioVirtualHost == false {
+	} else if ingressConfig.DisableIstioVirtualHost == false {
 		return ctrl.NewControllerManagedBy(mgr).
 			For(&v1beta1api.InferenceService{}).
 			Owns(&knservingv1.Service{}).

pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go

@@ -89,7 +89,37 @@ func getServiceHost(isvc *v1beta1.InferenceService) string {
 	}
 }
 
-func getServiceUrl(isvc *v1beta1.InferenceService, urlScheme string, disableIstioVirtualHost bool) string {
+func getServiceUrl(isvc *v1beta1.InferenceService, config *v1beta1.IngressConfig) string {
+
+	url := getHostBasedServiceUrl(isvc, config)
+	if url == "" {
+		return ""
+	}
+	if config.PathTemplate == "" {
+		return url
+	} else {
+		return getPathBasedServiceUrl(isvc, config)
+	}
+
+}
+
+func getPathBasedServiceUrl(isvc *v1beta1.InferenceService, config *v1beta1.IngressConfig) string {
+	path, err := GenerateUrlPath(isvc.Name, isvc.Namespace, config)
+	if err != nil {
+		log.Error(err, "Failed to generate URL path from pathTemplate")
+		return ""
+	}
+	url := &apis.URL{}
+	url.Scheme = config.UrlScheme
+	url.Host = config.IngressDomain
+	url.Path = path
+
+	return url.String()
+}
+
+func getHostBasedServiceUrl(isvc *v1beta1.InferenceService, config *v1beta1.IngressConfig) string {
+	urlScheme := config.UrlScheme
+	disableIstioVirtualHost := config.DisableIstioVirtualHost
 	if isvc.Status.Components == nil {
 		return ""
 	}
@@ -311,6 +341,62 @@ func createIngress(isvc *v1beta1.InferenceService, config *v1beta1.IngressConfig
 		hosts = append(hosts, serviceHost)
 		gateways = append(gateways, config.IngressGateway)
 	}
+	if config.PathTemplate != "" {
+		path, err := GenerateUrlPath(isvc.Name, isvc.Namespace, config)
+		if err != nil {
+			log.Error(err, "Failed to generate URL from pathTemplate")
+			return nil
+		}
+		url := &apis.URL{}
+		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
+		httpRoutes = append(httpRoutes, &istiov1alpha3.HTTPRoute{
+			Match: []*istiov1alpha3.HTTPMatchRequest{
+				{
+					Uri: &istiov1alpha3.StringMatch{
+						MatchType: &istiov1alpha3.StringMatch_Prefix{
+							Prefix: url.Path + "/",
+						},
+					},
+					Authority: &istiov1alpha3.StringMatch{
+						MatchType: &istiov1alpha3.StringMatch_Regex{
+							Regex: constants.HostRegExp(url.Host),
+						},
+					},
+					Gateways: []string{config.IngressGateway},
+				},
+				{
+					Uri: &istiov1alpha3.StringMatch{
+						MatchType: &istiov1alpha3.StringMatch_Exact{
+							Exact: url.Path,
+						},
+					},
+					Authority: &istiov1alpha3.StringMatch{
+						MatchType: &istiov1alpha3.StringMatch_Regex{
+							Regex: constants.HostRegExp(url.Host),
+						},
+					},
+					Gateways: []string{config.IngressGateway},
+				},
+			},
+			Rewrite: &istiov1alpha3.HTTPRewrite{
+				Uri: "/",
+			},
+			Route: []*istiov1alpha3.HTTPRouteDestination{
+				createHTTPRouteDestination(backend, isvc.Namespace, config.LocalGatewayServiceName),
+			},
+			Headers: &istiov1alpha3.Headers{
+				Request: &istiov1alpha3.Headers_HeaderOperations{
+					Set: map[string]string{
+						"Host": network.GetServiceHostname(backend, isvc.Namespace),
+					},
+				},
+			},
+		})
+		// Include ingressDomain to the domains (both internal and external) derived by KNative
+		hosts = append(hosts, url.Host)
+	}
 
 	annotations := utils.Filter(isvc.Annotations, func(key string) bool {
 		return !utils.Includes(constants.ServiceAnnotationDisallowedList, key)
@@ -331,9 +417,10 @@ func createIngress(isvc *v1beta1.InferenceService, config *v1beta1.IngressConfig
 	return desiredIngress
 }
 
-func (ir *IngressReconciler) Reconcile(isvc *v1beta1.InferenceService, disableIstioVirtualHost bool) error {
+func (ir *IngressReconciler) Reconcile(isvc *v1beta1.InferenceService) error {
 	serviceHost := getServiceHost(isvc)
-	serviceUrl := getServiceUrl(isvc, ir.ingressConfig.UrlScheme, disableIstioVirtualHost)
+	serviceUrl := getServiceUrl(isvc, ir.ingressConfig)
+	disableIstioVirtualHost := ir.ingressConfig.DisableIstioVirtualHost
 	if serviceHost == "" || serviceUrl == "" {
 		return nil
 	}

pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler_test.go

@@ -554,7 +554,10 @@ func TestGetServiceUrl(t *testing.T) {
 	labels := map[string]string{"test": "test"}
 	predictorUrl, _ := url.Parse("http://my-model-predictor-default.example.com")
 	transformerUrl, _ := url.Parse("http://my-model-transformer-default.example.com")
-	urlScheme := "http"
+	ingressConfig := &v1beta1.IngressConfig{
+		UrlScheme:               "http",
+		DisableIstioVirtualHost: false,
+	}
 
 	cases := map[string]struct {
 		isvc    *v1beta1.InferenceService
@@ -677,7 +680,100 @@ func TestGetServiceUrl(t *testing.T) {
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			url := getServiceUrl(tc.isvc, urlScheme, false)
+			url := getServiceUrl(tc.isvc, ingressConfig)
+			g.Expect(url).Should(tc.matcher)
+		})
+	}
+}
+
+func TestGetServiceUrlPathBased(t *testing.T) {
+	g := gomega.NewGomegaWithT(t)
+	serviceName := "my-model"
+	namespace := "test"
+	isvcAnnotations := map[string]string{"test": "test", "kubectl.kubernetes.io/last-applied-configuration": "test"}
+	labels := map[string]string{"test": "test"}
+	predictorUrl, _ := url.Parse("http://my-model-predictor-default.example.com")
+	ingressConfig := &v1beta1.IngressConfig{
+		UrlScheme:               "http",
+		IngressDomain:		 "my-domain.com",
+		PathTemplate:            "/serving/{{ .Namespace }}/{{ .Name }}",
+		DisableIstioVirtualHost: false,
+	}
+
+	cases := map[string]struct {
+		isvc    *v1beta1.InferenceService
+		matcher gomegaTypes.GomegaMatcher
+	}{
+		"component is empty": {
+			isvc: &v1beta1.InferenceService{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        serviceName,
+					Namespace:   namespace,
+					Annotations: isvcAnnotations,
+					Labels:      labels,
+				},
+				Spec: v1beta1.InferenceServiceSpec{
+					Predictor: v1beta1.PredictorSpec{},
+				},
+			},
+			matcher: gomega.Equal(""),
+		},
+		"predictor url is empty": {
+			isvc: &v1beta1.InferenceService{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        serviceName,
+					Namespace:   namespace,
+					Annotations: isvcAnnotations,
+					Labels:      labels,
+				},
+				Spec: v1beta1.InferenceServiceSpec{
+					Predictor: v1beta1.PredictorSpec{
+						SKLearn: &v1beta1.SKLearnSpec{},
+					},
+				},
+				Status: v1beta1.InferenceServiceStatus{
+					Status:  duckv1.Status{},
+					Address: nil,
+					URL:     nil,
+					Components: map[v1beta1.ComponentType]v1beta1.ComponentStatusSpec{
+						v1beta1.PredictorComponent: v1beta1.ComponentStatusSpec{},
+					},
+					ModelStatus: v1beta1.ModelStatus{},
+				},
+			},
+			matcher: gomega.Equal(""),
+		},
+		"predictor url is not empty": {
+			isvc: &v1beta1.InferenceService{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        serviceName,
+					Namespace:   namespace,
+					Annotations: isvcAnnotations,
+					Labels:      labels,
+				},
+				Spec: v1beta1.InferenceServiceSpec{
+					Predictor: v1beta1.PredictorSpec{
+						SKLearn: &v1beta1.SKLearnSpec{},
+					},
+				},
+				Status: v1beta1.InferenceServiceStatus{
+					Status:  duckv1.Status{},
+					Address: nil,
+					URL:     nil,
+					Components: map[v1beta1.ComponentType]v1beta1.ComponentStatusSpec{
+						v1beta1.PredictorComponent: v1beta1.ComponentStatusSpec{
+							URL: (*apis.URL)(predictorUrl),
+						},
+					},
+				},
+			},
+			matcher: gomega.Equal("http://my-domain.com/serving/test/my-model"),
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			url := getServiceUrl(tc.isvc, ingressConfig)
 			g.Expect(url).Should(tc.matcher)
 		})
 	}