Fix cluster local inference service (kserve#1224)

* Fix cluster local label val

* Add ingress tests

* Move ingress creation tests

* Fix local cluster predictor test

* Update knative version to 0.18

docs/samples/v1beta1/advanced/cluster_local.yaml

@@ -0,0 +1,10 @@
+apiVersion: "serving.kubeflow.org/v1beta1"
+kind: "InferenceService"
+metadata:
+  name: "sklearn-iris-local"
+  labels:
+    serving.knative.dev/visibility: cluster-local
+spec:
+  predictor:
+    sklearn:
+      storageUri: "gs://kfserving-samples/models/sklearn/iris"

hack/quick_install.sh

@@ -1,7 +1,7 @@
 set -e
 
 export ISTIO_VERSION=1.6.2
-export KNATIVE_VERSION=v0.15.0
+export KNATIVE_VERSION=v0.18.0
 export KFSERVING_VERSION=v0.4.1
 curl -L https://git.io/getLatestIstio | sh -
 cd istio-${ISTIO_VERSION}

pkg/constants/constants.go

@@ -204,6 +204,10 @@ var (
 		StorageInitializerSourceUriInternalAnnotationKey,
 		"kubectl.kubernetes.io/last-applied-configuration",
 	}
+
+	RevisionTemplateLabelDisallowedList = []string{
+		VisibilityLabel,
+	}
 )
 
 func (e InferenceServiceComponent) String() string {

pkg/controller/v1beta1/inferenceservice/controller_test.go

@@ -217,12 +217,12 @@ var _ = Describe("v1beta1 inference service controller", func() {
 			expectedVirtualService := &v1alpha3.VirtualService{
 				Spec: istiov1alpha3.VirtualService{
 					Gateways: []string{
-						constants.KnativeIngressGateway,
 						constants.KnativeLocalGateway,
+						constants.KnativeIngressGateway,
 					},
 					Hosts: []string{
-						constants.InferenceServiceHostName(serviceKey.Name, serviceKey.Namespace, domain),
 						network.GetServiceHostname(serviceKey.Name, serviceKey.Namespace),
+						constants.InferenceServiceHostName(serviceKey.Name, serviceKey.Namespace, domain),
 					},
 					Http: []*istiov1alpha3.HTTPRoute{
 						{
@@ -257,6 +257,7 @@ var _ = Describe("v1beta1 inference service controller", func() {
 										Host: network.GetServiceHostname("cluster-local-gateway", "istio-system"),
 										Port: &istiov1alpha3.PortSelector{Number: constants.CommonDefaultHttpPort},
 									},
+									Weight: 100,
 								},
 							},
 						},

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

@@ -159,7 +159,7 @@ func (r *IngressReconciler) reconcileExternalService(isvc *v1beta1.InferenceServ
 	return nil
 }
 
-func (ir *IngressReconciler) createHTTPRouteDestination(targetHost, namespace string, gatewayService string) *istiov1alpha3.HTTPRouteDestination {
+func createHTTPRouteDestination(targetHost, namespace string, gatewayService string) *istiov1alpha3.HTTPRouteDestination {
 	httpRouteDestination := &istiov1alpha3.HTTPRouteDestination{
 		Headers: &istiov1alpha3.Headers{
 			Request: &istiov1alpha3.Headers_HeaderOperations{
@@ -174,11 +174,12 @@ func (ir *IngressReconciler) createHTTPRouteDestination(targetHost, namespace st
 				Number: constants.CommonDefaultHttpPort,
 			},
 		},
+		Weight: 100,
 	}
 	return httpRouteDestination
 }
 
-func (ir *IngressReconciler) createHTTPMatchRequest(prefix, targetHost, internalHost string, isInternal bool) []*istiov1alpha3.HTTPMatchRequest {
+func createHTTPMatchRequest(prefix, targetHost, internalHost string, isInternal bool, config *v1beta1.IngressConfig) []*istiov1alpha3.HTTPMatchRequest {
 	var uri *istiov1alpha3.StringMatch
 	if prefix != "" {
 		uri = &istiov1alpha3.StringMatch{
@@ -207,13 +208,18 @@ func (ir *IngressReconciler) createHTTPMatchRequest(prefix, targetHost, internal
 						Regex: constants.HostRegExp(targetHost),
 					},
 				},
-				Gateways: []string{ir.ingressConfig.IngressGateway},
+				Gateways: []string{config.IngressGateway},
 			})
 	}
 	return matchRequests
 }
 
-func (ir *IngressReconciler) Reconcile(isvc *v1beta1.InferenceService) error {
+func createIngress(isvc *v1beta1.InferenceService, config *v1beta1.IngressConfig) *v1alpha3.VirtualService {
+	serviceHost := getServiceHost(isvc)
+	if serviceHost == "" {
+		return nil
+	}
+
 	if !isvc.Status.IsConditionReady(v1beta1.PredictorReady) {
 		isvc.Status.SetCondition(v1beta1.IngressReady, &apis.Condition{
 			Type:   v1beta1.IngressReady,
@@ -222,11 +228,6 @@ func (ir *IngressReconciler) Reconcile(isvc *v1beta1.InferenceService) error {
 		})
 		return nil
 	}
-	serviceHost := getServiceHost(isvc)
-	serviceUrl := getServiceUrl(isvc)
-	if serviceHost == "" || serviceUrl == "" {
-		return nil
-	}
 	backend := constants.DefaultPredictorServiceName(isvc.Name)
 
 	if isvc.Spec.Transformer != nil {
@@ -242,7 +243,7 @@ func (ir *IngressReconciler) Reconcile(isvc *v1beta1.InferenceService) error {
 	}
 	isInternal := false
 	//if service is labelled with cluster local or knative domain is configured as internal
-	if val, ok := isvc.Labels[constants.VisibilityLabel]; ok && val == "ClusterLocal" {
+	if val, ok := isvc.Labels[constants.VisibilityLabel]; ok && val == "cluster-local" {
 		isInternal = true
 	}
 	serviceInternalHostName := network.GetServiceHostname(isvc.Name, isvc.Namespace)
@@ -261,45 +262,63 @@ func (ir *IngressReconciler) Reconcile(isvc *v1beta1.InferenceService) error {
 			return nil
 		}
 		explainerRouter := istiov1alpha3.HTTPRoute{
-			Match: ir.createHTTPMatchRequest(constants.ExplainPrefix(), serviceHost,
-				network.GetServiceHostname(isvc.Name, isvc.Namespace), isInternal),
+			Match: createHTTPMatchRequest(constants.ExplainPrefix(), serviceHost,
+				network.GetServiceHostname(isvc.Name, isvc.Namespace), isInternal, config),
 			Route: []*istiov1alpha3.HTTPRouteDestination{
-				ir.createHTTPRouteDestination(constants.DefaultExplainerServiceName(isvc.Name), isvc.Namespace, constants.LocalGatewayHost),
+				createHTTPRouteDestination(constants.DefaultExplainerServiceName(isvc.Name), isvc.Namespace, constants.LocalGatewayHost),
 			},
 		}
 		httpRoutes = append(httpRoutes, &explainerRouter)
 	}
 	// Add predict route
 	httpRoutes = append(httpRoutes, &istiov1alpha3.HTTPRoute{
-		Match: ir.createHTTPMatchRequest("", serviceHost,
-			network.GetServiceHostname(isvc.Name, isvc.Namespace), isInternal),
+		Match: createHTTPMatchRequest("", serviceHost,
+			network.GetServiceHostname(isvc.Name, isvc.Namespace), isInternal, config),
 		Route: []*istiov1alpha3.HTTPRouteDestination{
-			ir.createHTTPRouteDestination(backend, isvc.Namespace, constants.LocalGatewayHost),
+			createHTTPRouteDestination(backend, isvc.Namespace, constants.LocalGatewayHost),
 		},
 	})
-
-	//Create external service which points to local gateway
-	if err := ir.reconcileExternalService(isvc); err != nil {
-		return errors.Wrapf(err, "fails to reconcile external name service")
+	hosts := []string{
+		network.GetServiceHostname(isvc.Name, isvc.Namespace),
+	}
+	gateways := []string{
+		constants.KnativeLocalGateway,
+	}
+	if !isInternal {
+		hosts = append(hosts, serviceHost)
+		gateways = append(gateways, config.IngressGateway)
 	}
-	//Create ingress
 	desiredIngress := &v1alpha3.VirtualService{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      isvc.Name,
 			Namespace: isvc.Namespace,
 		},
 		Spec: istiov1alpha3.VirtualService{
-			Hosts: []string{
-				serviceHost,
-				network.GetServiceHostname(isvc.Name, isvc.Namespace),
-			},
-			Gateways: []string{
-				ir.ingressConfig.IngressGateway,
-				constants.KnativeLocalGateway,
-			},
-			Http: httpRoutes,
+			Hosts:    hosts,
+			Gateways: gateways,
+			Http:     httpRoutes,
 		},
 	}
+	return desiredIngress
+}
+
+func (ir *IngressReconciler) Reconcile(isvc *v1beta1.InferenceService) error {
+	serviceHost := getServiceHost(isvc)
+	serviceUrl := getServiceUrl(isvc)
+	if serviceHost == "" || serviceUrl == "" {
+		return nil
+	}
+	//Create ingress
+	desiredIngress := createIngress(isvc, ir.ingressConfig)
+	if desiredIngress == nil {
+		return nil
+	}
+
+	//Create external service which points to local gateway
+	if err := ir.reconcileExternalService(isvc); err != nil {
+		return errors.Wrapf(err, "fails to reconcile external name service")
+	}
+
 	if err := controllerutil.SetControllerReference(isvc, desiredIngress, ir.scheme); err != nil {
 		return errors.Wrapf(err, "fails to set owner reference for ingress")
 	}