From 48f45ebf83978338993acc28c8b851fbb3c66179 Mon Sep 17 00:00:00 2001 From: Suresh Nakkeran <30799624+Suresh-Nakkeran@users.noreply.github.com> Date: Fri, 9 Sep 2022 10:07:33 +0530 Subject: [PATCH] added a flag to make istio optional (#2380) Signed-off-by: Suresh Nakkeran Signed-off-by: Suresh Nakkeran --- cmd/manager/main.go | 18 ++- config/configmap/inferenceservice.yaml | 3 +- pkg/apis/serving/v1beta1/configmap.go | 1 + .../v1beta1/inferenceservice/controller.go | 12 +- .../inferenceservice/controller_test.go | 109 ++++++++++++++++++ .../reconcilers/ingress/ingress_reconciler.go | 90 +++++++++------ .../v1beta1/inferenceservice/suite_test.go | 2 +- 7 files changed, 190 insertions(+), 45 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index e65453320a6..da24a4a2a71 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -117,17 +117,23 @@ func main() { log.Error(err, "unable to get deploy config.") os.Exit(1) } + ingressConfig, err := v1beta1.NewIngressConfig(client) + if err != nil { + log.Error(err, "unable to get ingress config.") + os.Exit(1) + } if deployConfig.DefaultDeploymentMode == string(constants.Serverless) { log.Info("Setting up Knative scheme") if err := knservingv1.AddToScheme(mgr.GetScheme()); err != nil { log.Error(err, "unable to add Knative APIs to scheme") os.Exit(1) } - - log.Info("Setting up Istio schemes") - if err := v1alpha3.AddToScheme(mgr.GetScheme()); err != nil { - log.Error(err, "unable to add Istio v1alpha3 APIs to scheme") - os.Exit(1) + if ingressConfig.DisableIstioVirtualHost == false { + log.Info("Setting up Istio schemes") + if err := v1alpha3.AddToScheme(mgr.GetScheme()); err != nil { + log.Error(err, "unable to add Istio v1alpha3 APIs to scheme") + os.Exit(1) + } } } @@ -152,7 +158,7 @@ func main() { Scheme: mgr.GetScheme(), Recorder: eventBroadcaster.NewRecorder( mgr.GetScheme(), v1.EventSource{Component: "v1beta1Controllers"}), - }).SetupWithManager(mgr, deployConfig); err != nil { + }).SetupWithManager(mgr, deployConfig, ingressConfig.DisableIstioVirtualHost); err != nil { setupLog.Error(err, "unable to create controller", "v1beta1Controller", "InferenceService") os.Exit(1) } diff --git a/config/configmap/inferenceservice.yaml b/config/configmap/inferenceservice.yaml index 9e4d8d52005..ba3fc6085c7 100644 --- a/config/configmap/inferenceservice.yaml +++ b/config/configmap/inferenceservice.yaml @@ -177,7 +177,8 @@ data: "ingressDomain" : "example.com", "ingressClassName" : "istio", "domainTemplate": "{{ .Name }}-{{ .Namespace }}.{{ .IngressDomain }}", - "urlScheme": "http" + "urlScheme": "http", + "disableIstioVirtualHost": false } logger: |- { diff --git a/pkg/apis/serving/v1beta1/configmap.go b/pkg/apis/serving/v1beta1/configmap.go index 22b4b3a27aa..f4aab0dc773 100644 --- a/pkg/apis/serving/v1beta1/configmap.go +++ b/pkg/apis/serving/v1beta1/configmap.go @@ -127,6 +127,7 @@ type IngressConfig struct { IngressClassName *string `json:"ingressClassName,omitempty"` DomainTemplate string `json:"domainTemplate,omitempty"` UrlScheme string `json:"urlScheme,omitempty"` + DisableIstioVirtualHost bool `json:"disableIstioVirtualHost,omitempty"` } // +kubebuilder:object:generate=false diff --git a/pkg/controller/v1beta1/inferenceservice/controller.go b/pkg/controller/v1beta1/inferenceservice/controller.go index 7d75230dcec..85c9917cd1e 100644 --- a/pkg/controller/v1beta1/inferenceservice/controller.go +++ b/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); err != nil { + if err := reconciler.Reconcile(isvc, ingressConfig.DisableIstioVirtualHost); err != nil { return reconcile.Result{}, errors.Wrapf(err, "fails to reconcile ingress") } } @@ -274,19 +274,25 @@ func inferenceServiceStatusEqual(s1, s2 v1beta1api.InferenceServiceStatus, deplo return equality.Semantic.DeepEqual(s1, s2) } -func (r *InferenceServiceReconciler) SetupWithManager(mgr ctrl.Manager, deployConfig *v1beta1api.DeployConfig) error { +func (r *InferenceServiceReconciler) SetupWithManager(mgr ctrl.Manager, deployConfig *v1beta1api.DeployConfig, disableIstioVirtualHost bool) error { if deployConfig.DefaultDeploymentMode == string(constants.RawDeployment) { return ctrl.NewControllerManagedBy(mgr). For(&v1beta1api.InferenceService{}). Owns(&appsv1.Deployment{}). Complete(r) - } else { + } else if disableIstioVirtualHost == false { return ctrl.NewControllerManagedBy(mgr). For(&v1beta1api.InferenceService{}). Owns(&knservingv1.Service{}). Owns(&v1alpha3.VirtualService{}). Owns(&appsv1.Deployment{}). Complete(r) + } else { + return ctrl.NewControllerManagedBy(mgr). + For(&v1beta1api.InferenceService{}). + Owns(&knservingv1.Service{}). + Owns(&appsv1.Deployment{}). + Complete(r) } } diff --git a/pkg/controller/v1beta1/inferenceservice/controller_test.go b/pkg/controller/v1beta1/inferenceservice/controller_test.go index 8fc96cbdca3..c7877d88202 100644 --- a/pkg/controller/v1beta1/inferenceservice/controller_test.go +++ b/pkg/controller/v1beta1/inferenceservice/controller_test.go @@ -1653,4 +1653,113 @@ var _ = Describe("v1beta1 inference service controller", func() { }) }) + Context("When creating inference service with predictor and without top level istio virtual service", func() { + It("Should have knative service created", func() { + By("By creating a new InferenceService") + // Create configmap + copiedConfigs := make(map[string]string) + for key, value := range configs { + if key == "ingress" { + copiedConfigs[key] = `{ + "disableIstioVirtualHost": true, + "ingressGateway": "knative-serving/knative-ingress-gateway", + "ingressService": "test-destination", + "localGateway": "knative-serving/knative-local-gateway", + "localGatewayService": "knative-local-gateway.istio-system.svc.cluster.local" + }` + } else { + copiedConfigs[key] = value + } + } + var configMap = &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: constants.InferenceServiceConfigMapName, + Namespace: constants.KServeNamespace, + }, + Data: copiedConfigs, + } + Expect(k8sClient.Create(context.TODO(), configMap)).NotTo(HaveOccurred()) + defer k8sClient.Delete(context.TODO(), configMap) + serviceName := "foo-disable-istio" + var expectedRequest = reconcile.Request{NamespacedName: types.NamespacedName{Name: serviceName, Namespace: "default"}} + var serviceKey = expectedRequest.NamespacedName + var storageUri = "s3://test/mnist/export" + ctx := context.Background() + isvc := &v1beta1.InferenceService{ + ObjectMeta: metav1.ObjectMeta{ + Name: serviceKey.Name, + Namespace: serviceKey.Namespace, + }, + Spec: v1beta1.InferenceServiceSpec{ + Predictor: v1beta1.PredictorSpec{ + ComponentExtensionSpec: v1beta1.ComponentExtensionSpec{ + MinReplicas: v1beta1.GetIntReference(1), + MaxReplicas: 3, + }, + Tensorflow: &v1beta1.TFServingSpec{ + PredictorExtensionSpec: v1beta1.PredictorExtensionSpec{ + StorageURI: &storageUri, + RuntimeVersion: proto.String("1.14.0"), + Container: v1.Container{ + Name: "kfs", + Resources: defaultResource, + }, + }, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, isvc)).Should(Succeed()) + defer k8sClient.Delete(ctx, isvc) + inferenceService := &v1beta1.InferenceService{} + + Eventually(func() bool { + err := k8sClient.Get(ctx, serviceKey, inferenceService) + if err != nil { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + + actualService := &knservingv1.Service{} + predictorServiceKey := types.NamespacedName{Name: constants.DefaultPredictorServiceName(serviceKey.Name), + Namespace: serviceKey.Namespace} + Eventually(func() error { + return k8sClient.Get(context.TODO(), predictorServiceKey, actualService) + }, timeout).Should(Succeed()) + predictorUrl, _ := apis.ParseURL("http://" + constants.InferenceServiceHostName(constants.DefaultPredictorServiceName(serviceKey.Name), serviceKey.Namespace, domain)) + // update predictor + updatedService := actualService.DeepCopy() + updatedService.Status.LatestCreatedRevisionName = "revision-v1" + updatedService.Status.LatestReadyRevisionName = "revision-v1" + updatedService.Status.URL = predictorUrl + updatedService.Status.Conditions = duckv1.Conditions{ + { + Type: knservingv1.ServiceConditionReady, + Status: "True", + }, + } + Expect(k8sClient.Status().Update(context.TODO(), updatedService)).NotTo(gomega.HaveOccurred()) + // get inference service + time.Sleep(10 * time.Second) + actualIsvc := &v1beta1.InferenceService{} + Eventually(func() bool { + err := k8sClient.Get(ctx, expectedRequest.NamespacedName, actualIsvc) + if err != nil { + return false + } + return true + }, timeout, interval).Should(BeTrue()) + + Expect(actualIsvc.Status.URL).To(gomega.Equal(&apis.URL{ + Scheme: "http", + Host: constants.InferenceServiceHostName(constants.DefaultPredictorServiceName(serviceKey.Name), serviceKey.Namespace, domain), + })) + Expect(actualIsvc.Status.Address.URL).To(gomega.Equal(&apis.URL{ + Scheme: "http", + Host: network.GetServiceHostname(fmt.Sprintf("%s-%s-default", serviceKey.Name, string(constants.Predictor)), serviceKey.Namespace), + Path: constants.PredictPath(serviceKey.Name, constants.ProtocolV1), + })) + }) + }) }) diff --git a/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go b/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go index 51ee583c33a..a27ef7fffdb 100644 --- a/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go +++ b/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go @@ -89,7 +89,7 @@ func getServiceHost(isvc *v1beta1.InferenceService) string { } } -func getServiceUrl(isvc *v1beta1.InferenceService, urlScheme string) string { +func getServiceUrl(isvc *v1beta1.InferenceService, urlScheme string, disableIstioVirtualHost bool) string { if isvc.Status.Components == nil { return "" } @@ -102,7 +102,11 @@ func getServiceUrl(isvc *v1beta1.InferenceService, urlScheme string) string { } else { url := transformerStatus.URL url.Scheme = urlScheme - return strings.Replace(url.String(), fmt.Sprintf("-%s-default", string(constants.Transformer)), "", 1) + if disableIstioVirtualHost == false { + return strings.Replace(url.String(), fmt.Sprintf("-%s-default", string(constants.Transformer)), "", 1) + } else { + return url.String() + } } } @@ -113,7 +117,11 @@ func getServiceUrl(isvc *v1beta1.InferenceService, urlScheme string) string { } else { url := predictorStatus.URL url.Scheme = urlScheme - return strings.Replace(url.String(), fmt.Sprintf("-%s-default", string(constants.Predictor)), "", 1) + if disableIstioVirtualHost == false { + return strings.Replace(url.String(), fmt.Sprintf("-%s-default", string(constants.Predictor)), "", 1) + } else { + return url.String() + } } } @@ -323,46 +331,49 @@ func createIngress(isvc *v1beta1.InferenceService, config *v1beta1.IngressConfig return desiredIngress } -func (ir *IngressReconciler) Reconcile(isvc *v1beta1.InferenceService) error { +func (ir *IngressReconciler) Reconcile(isvc *v1beta1.InferenceService, disableIstioVirtualHost bool) error { serviceHost := getServiceHost(isvc) - serviceUrl := getServiceUrl(isvc, ir.ingressConfig.UrlScheme) + serviceUrl := getServiceUrl(isvc, ir.ingressConfig.UrlScheme, disableIstioVirtualHost) if serviceHost == "" || serviceUrl == "" { return nil } - //Create ingress - desiredIngress := createIngress(isvc, ir.ingressConfig) - if desiredIngress == nil { - return nil - } + // When Istio virtual host is disabled, we return the underlying component url. + // When Istio virtual host is enabled. we return the url using inference service virtual host name and redirect to the corresponding transformer, predictor or explainer url. + if disableIstioVirtualHost == false { + desiredIngress := createIngress(isvc, ir.ingressConfig) + if desiredIngress == nil { + return nil + } - //Create external service which points to local gateway - if err := ir.reconcileExternalService(isvc, ir.ingressConfig); err != nil { - return errors.Wrapf(err, "fails to reconcile external name service") - } + //Create external service which points to local gateway + if err := ir.reconcileExternalService(isvc, ir.ingressConfig); 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") - } + if err := controllerutil.SetControllerReference(isvc, desiredIngress, ir.scheme); err != nil { + return errors.Wrapf(err, "fails to set owner reference for ingress") + } - existing := &v1alpha3.VirtualService{} - err := ir.client.Get(context.TODO(), types.NamespacedName{Name: desiredIngress.Name, Namespace: desiredIngress.Namespace}, existing) - if err != nil { - if apierr.IsNotFound(err) { - log.Info("Creating Ingress for isvc", "namespace", desiredIngress.Namespace, "name", desiredIngress.Name) - err = ir.client.Create(context.TODO(), desiredIngress) + existing := &v1alpha3.VirtualService{} + err := ir.client.Get(context.TODO(), types.NamespacedName{Name: desiredIngress.Name, Namespace: desiredIngress.Namespace}, existing) + if err != nil { + if apierr.IsNotFound(err) { + log.Info("Creating Ingress for isvc", "namespace", desiredIngress.Namespace, "name", desiredIngress.Name) + err = ir.client.Create(context.TODO(), desiredIngress) + } + } else { + if !routeSemanticEquals(desiredIngress, existing) { + existing.Spec = desiredIngress.Spec + existing.Annotations = desiredIngress.Annotations + existing.Labels = desiredIngress.Labels + log.Info("Update Ingress for isvc", "namespace", desiredIngress.Namespace, "name", desiredIngress.Name) + err = ir.client.Update(context.TODO(), existing) + } } - } else { - if !routeSemanticEquals(desiredIngress, existing) { - existing.Spec = desiredIngress.Spec - existing.Annotations = desiredIngress.Annotations - existing.Labels = desiredIngress.Labels - log.Info("Update Ingress for isvc", "namespace", desiredIngress.Namespace, "name", desiredIngress.Name) - err = ir.client.Update(context.TODO(), existing) + if err != nil { + return errors.Wrapf(err, "fails to create or update ingress") } } - if err != nil { - return errors.Wrapf(err, "fails to create or update ingress") - } if url, err := apis.ParseURL(serviceUrl); err == nil { isvc.Status.URL = url @@ -379,9 +390,10 @@ func (ir *IngressReconciler) Reconcile(isvc *v1beta1.InferenceService) error { path = constants.PredictPath(isvc.Name, constants.ProtocolV2) } } + hostPrefix := getHostPrefix(isvc, disableIstioVirtualHost) isvc.Status.Address = &duckv1.Addressable{ URL: &apis.URL{ - Host: network.GetServiceHostname(isvc.Name, isvc.Namespace), + Host: network.GetServiceHostname(hostPrefix, isvc.Namespace), Scheme: "http", Path: path, }, @@ -401,3 +413,13 @@ func routeSemanticEquals(desired, existing *v1alpha3.VirtualService) bool { equality.Semantic.DeepEqual(desired.ObjectMeta.Labels, existing.ObjectMeta.Labels) && equality.Semantic.DeepEqual(desired.ObjectMeta.Annotations, existing.ObjectMeta.Annotations) } + +func getHostPrefix(isvc *v1beta1.InferenceService, disableIstioVirtualHost bool) string { + if disableIstioVirtualHost == true { + if isvc.Spec.Transformer != nil { + return constants.DefaultTransformerServiceName(isvc.Name) + } + return constants.DefaultPredictorServiceName(isvc.Name) + } + return isvc.Name +} diff --git a/pkg/controller/v1beta1/inferenceservice/suite_test.go b/pkg/controller/v1beta1/inferenceservice/suite_test.go index c4d6bc49e84..2944adde13d 100644 --- a/pkg/controller/v1beta1/inferenceservice/suite_test.go +++ b/pkg/controller/v1beta1/inferenceservice/suite_test.go @@ -100,7 +100,7 @@ var _ = BeforeSuite(func() { Scheme: k8sClient.Scheme(), Log: ctrl.Log.WithName("V1beta1InferenceServiceController"), Recorder: k8sManager.GetEventRecorderFor("V1beta1InferenceServiceController"), - }).SetupWithManager(k8sManager, deployConfig) + }).SetupWithManager(k8sManager, deployConfig, false) Expect(err).ToNot(HaveOccurred()) go func() {