diff --git a/pkg/inferenceservice-controller/controller.go b/pkg/inferenceservice-controller/controller.go index fc4ac445..780e2815 100644 --- a/pkg/inferenceservice-controller/controller.go +++ b/pkg/inferenceservice-controller/controller.go @@ -29,6 +29,7 @@ type InferenceServiceController struct { modelRegistryNameLabel string modelRegistryURLAnnotation string modelRegistryFinalizer string + serviceURLAnnotation string defaultModelRegistryNamespace string } @@ -44,6 +45,7 @@ func NewInferenceServiceController( mrNameLabel, mrURLAnnotation, mrFinalizer, + serviceURLAnnotation, defaultMRNamespace string, ) *InferenceServiceController { httpClient := http.DefaultClient @@ -66,6 +68,7 @@ func NewInferenceServiceController( modelRegistryNameLabel: mrNameLabel, modelRegistryURLAnnotation: mrURLAnnotation, modelRegistryFinalizer: mrFinalizer, + serviceURLAnnotation: serviceURLAnnotation, defaultModelRegistryNamespace: defaultMRNamespace, } } @@ -112,9 +115,8 @@ func (r *InferenceServiceController) Reconcile(ctx context.Context, req ctrl.Req } if !okMrName && !okMrUrl { - // Early check: it's required to have the model registry name or url set in the ISVC - log.Error(fmt.Errorf("missing model registry name or url, unable to link ISVC to Model Registry, skipping InferenceService"), "Stop ModelRegistry InferenceService reconciliation") - return ctrl.Result{}, nil + // Early check: it's optional to have the model registry name or url set in the ISVC, but if not set, it will fail if there's more than one model registry in the namespace + log.Info(fmt.Sprintf("missing %s or %s, will try to connect to the Model Registry service in the namespace %s", r.modelRegistryNameLabel, r.modelRegistryURLAnnotation, mrNamespace)) } if mrNSFromISVC, ok := isvc.Labels[r.modelRegistryNamespaceLabel]; ok { @@ -124,7 +126,7 @@ func (r *InferenceServiceController) Reconcile(ctx context.Context, req ctrl.Req log.Info("Creating model registry service..") mrApi, err := r.initModelRegistryService(ctx, log, mrName, mrNamespace, mrUrl) if err != nil { - log.Error(err, "Unable to initialize Model Registry Service") + log.Error(err, "Unable to initialize Model Registry service") return ctrl.Result{}, err } @@ -229,11 +231,11 @@ func (r *InferenceServiceController) initModelRegistryService(ctx context.Contex log1 := log.WithValues("mr-namespace", namespace, "mr-name", name) if url == "" { - log1.Info("Retrieving api http port from deployed model registry service") + log1.Info("Retrieving url from deployed model registry service") url, err = r.getMRUrlFromService(ctx, name, namespace) if err != nil { - log1.Error(err, "Unable to fetch the Model Registry Service") + log1.Error(err, "Unable to fetch the Model Registry service") return nil, err } } @@ -253,15 +255,50 @@ func (r *InferenceServiceController) initModelRegistryService(ctx context.Contex } func (r *InferenceServiceController) getMRUrlFromService(ctx context.Context, name, namespace string) (string, error) { + svc, err := r.getMRService(ctx, name, namespace) + if err != nil { + return "", fmt.Errorf("unable to find the Model Registry service: %w", err) + } + + return r.buildURLFromService(svc) +} + +func (r *InferenceServiceController) getMRService(ctx context.Context, name, namespace string) (*corev1.Service, error) { + if name == "" { + svcList := &corev1.ServiceList{} + + if err := r.client.List(ctx, svcList, client.InNamespace(namespace), client.MatchingLabels{"component": "model-registry"}); err != nil { + return nil, fmt.Errorf("unable to list services in the namespace %s: %w", namespace, err) + } + + if len(svcList.Items) == 0 { + return nil, fmt.Errorf("no model registry services found in the namespace %s", namespace) + } + + if len(svcList.Items) > 1 { + return nil, fmt.Errorf("more than one model registry service found in the namespace %s, consider to specify the name in the label %s", namespace, r.modelRegistryNameLabel) + } + + return &svcList.Items[0], nil + } + svc := &corev1.Service{} err := r.client.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, svc) if err != nil { - return "", err + return nil, err } + return svc, nil +} + +func (r *InferenceServiceController) buildURLFromService(svc *corev1.Service) (string, error) { var restApiPort *int32 + if url, ok := svc.Annotations[r.modelRegistryURLAnnotation]; ok { + return fmt.Sprintf("https://%s", url), nil + } + for _, port := range svc.Spec.Ports { if port.Name == "http-api" { restApiPort = &port.Port @@ -270,10 +307,10 @@ func (r *InferenceServiceController) getMRUrlFromService(ctx context.Context, na } if restApiPort == nil { - return "", fmt.Errorf("unable to find the http port in the Model Registry Service") + return "", fmt.Errorf("unable to find the http port in the Model Registry service") } - return fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", name, namespace, *restApiPort), nil + return fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", svc.Name, svc.Namespace, *restApiPort), nil } func (r *InferenceServiceController) createMRInferenceService( diff --git a/pkg/inferenceservice-controller/controller_test.go b/pkg/inferenceservice-controller/controller_test.go index 8d3f6335..7945971e 100644 --- a/pkg/inferenceservice-controller/controller_test.go +++ b/pkg/inferenceservice-controller/controller_test.go @@ -8,6 +8,7 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" ) @@ -16,10 +17,204 @@ var _ = Describe("InferenceService Controller", func() { When("Creating a new InferenceService with Model Registry labels", func() { It("If a label with inference service id is missing, it should add it after creating the required resources on model registry", func() { const CorrectInferenceServicePath = "./testdata/inferenceservices/inference-service-correct.yaml" + const ModelRegistrySVCPath = "./testdata/deploy/model-registry-svc.yaml" + const namespace = "correct" + + ns := &corev1.Namespace{} + + ns.SetName(namespace) + + if err := cli.Create(ctx, ns); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } + + mrSvc := &corev1.Service{} + Expect(ConvertFileToStructuredResource(ModelRegistrySVCPath, mrSvc)).To(Succeed()) + + mrSvc.SetNamespace(namespace) + + if err := cli.Create(ctx, mrSvc); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } + + inferenceService := &kservev1beta1.InferenceService{} + Expect(ConvertFileToStructuredResource(CorrectInferenceServicePath, inferenceService)).To(Succeed()) + + inferenceService.SetNamespace(namespace) + + inferenceService.Labels[namespaceLabel] = namespace + + if err := cli.Create(ctx, inferenceService); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } + + Eventually(func() error { + isvc := &kservev1beta1.InferenceService{} + err := cli.Get(ctx, types.NamespacedName{ + Name: inferenceService.Name, + Namespace: inferenceService.Namespace, + }, isvc) + if err != nil { + return err + } + + if isvc.Labels[inferenceServiceIDLabel] != "1" { + return fmt.Errorf("Label for InferenceServiceID is not set, got %s", isvc.Labels[inferenceServiceIDLabel]) + } + + return nil + }, 10*time.Second, 1*time.Second).Should(Succeed()) + }) + }) + + When("Creating a new InferenceService without a Model Registry name", func() { + It("Should successfully create the InferenceService if there's just one model registry in the namespace", func() { + const InferenceServiceMissingNamePath = "./testdata/inferenceservices/inference-service-missing-name.yaml" + const ModelRegistrySVCPath = "./testdata/deploy/model-registry-svc.yaml" + const namespace = "correct-no-name" + + ns := &corev1.Namespace{} + + ns.SetName(namespace) + + if err := cli.Create(ctx, ns); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } + + mrSvc := &corev1.Service{} + Expect(ConvertFileToStructuredResource(ModelRegistrySVCPath, mrSvc)).To(Succeed()) + + mrSvc.SetNamespace(namespace) + + if err := cli.Create(ctx, mrSvc); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } + + inferenceService := &kservev1beta1.InferenceService{} + Expect(ConvertFileToStructuredResource(InferenceServiceMissingNamePath, inferenceService)).To(Succeed()) + + inferenceService.SetNamespace(namespace) + + inferenceService.Labels[namespaceLabel] = namespace + + if err := cli.Create(ctx, inferenceService); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } + + Eventually(func() error { + isvc := &kservev1beta1.InferenceService{} + err := cli.Get(ctx, types.NamespacedName{ + Name: inferenceService.Name, + Namespace: inferenceService.Namespace, + }, isvc) + if err != nil { + return err + } + + if isvc.Labels[inferenceServiceIDLabel] != "1" { + return fmt.Errorf("Label for InferenceServiceID is not set, got %s", isvc.Labels[inferenceServiceIDLabel]) + } + + return nil + }, 10*time.Second, 1*time.Second).Should(Succeed()) + }) + + It("Should fail to create the InferenceService if there are multiple model registries in the namespace", func() { + const InferenceServiceMissingNamePath = "./testdata/inferenceservices/inference-service-missing-name.yaml" + const ModelRegistrySVCPath = "./testdata/deploy/model-registry-svc.yaml" + const namespace = "fail-no-name" + + ns := &corev1.Namespace{} + + ns.SetName(namespace) + + if err := cli.Create(ctx, ns); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } + + mrSvc := &corev1.Service{} + Expect(ConvertFileToStructuredResource(ModelRegistrySVCPath, mrSvc)).To(Succeed()) + + mrSvc.SetNamespace(namespace) + + if err := cli.Create(ctx, mrSvc); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } + + mrSvc2 := &corev1.Service{} + Expect(ConvertFileToStructuredResource(ModelRegistrySVCPath, mrSvc2)).To(Succeed()) + + mrSvc2.SetNamespace(namespace) + mrSvc2.SetName("model-registry-2") + + if err := cli.Create(ctx, mrSvc2); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } + + inferenceService := &kservev1beta1.InferenceService{} + Expect(ConvertFileToStructuredResource(InferenceServiceMissingNamePath, inferenceService)).To(Succeed()) + + inferenceService.SetNamespace(namespace) + + inferenceService.Labels[namespaceLabel] = namespace + + if err := cli.Create(ctx, inferenceService); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } + + Consistently(func() error { + isvc := &kservev1beta1.InferenceService{} + err := cli.Get(ctx, types.NamespacedName{ + Name: inferenceService.Name, + Namespace: inferenceService.Namespace, + }, isvc) + if err != nil { + return err + } + + if isvc.Labels[inferenceServiceIDLabel] != "1" { + return fmt.Errorf("Label for InferenceServiceID is not set, got %s", isvc.Labels[inferenceServiceIDLabel]) + } + + return nil + }, 5*time.Second, 1*time.Second).Should(Not(Succeed())) + }) + }) + + When("Creating a new InferenceService with a Model Registry service specifies an annotation URL", func() { + It("Should successfully create the InferenceService with the correct URL", func() { + const CorrectInferenceServicePath = "./testdata/inferenceservices/inference-service-correct.yaml" + const ModelRegistrySVCPath = "./testdata/deploy/model-registry-svc.yaml" + const namespace = "correct-annotation-url" + + ns := &corev1.Namespace{} + + ns.SetName(namespace) + + if err := cli.Create(ctx, ns); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } + + mrSvc := &corev1.Service{} + Expect(ConvertFileToStructuredResource(ModelRegistrySVCPath, mrSvc)).To(Succeed()) + + mrSvc.SetNamespace(namespace) + + mrSvc.Annotations = map[string]string{ + urlAnnotation: "model-registry.svc.cluster.local:8080", + } + + if err := cli.Create(ctx, mrSvc); err != nil && !errors.IsAlreadyExists(err) { + Fail(err.Error()) + } inferenceService := &kservev1beta1.InferenceService{} Expect(ConvertFileToStructuredResource(CorrectInferenceServicePath, inferenceService)).To(Succeed()) + inferenceService.SetNamespace(namespace) + + inferenceService.Labels[namespaceLabel] = namespace + if err := cli.Create(ctx, inferenceService); err != nil && !errors.IsAlreadyExists(err) { Fail(err.Error()) } diff --git a/pkg/inferenceservice-controller/suite_test.go b/pkg/inferenceservice-controller/suite_test.go index ab951b06..bda43ce2 100644 --- a/pkg/inferenceservice-controller/suite_test.go +++ b/pkg/inferenceservice-controller/suite_test.go @@ -8,8 +8,8 @@ import ( "net/http/httptest" "net/url" "os" + "path" "path/filepath" - "strings" "testing" "time" @@ -18,7 +18,6 @@ import ( "go.uber.org/zap/zapcore" corev1 "k8s.io/api/core/v1" authv1 "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -44,6 +43,7 @@ const ( skipTLSVerify = true urlAnnotation = "modelregistry.kubeflow.org/url" finalizerLabel = "modelregistry.kubeflow.org/finalizer" + serviceURLAnnotation = "routing.kubeflow.org/external-address-rest" defaultNamespace = "default" accessToken = "" kserveVersion = "v0.12.1" @@ -122,17 +122,6 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) - const ModelRegistrySVCPath = "./testdata/deploy/model-registry-svc.yaml" - - mrSvc := &corev1.Service{} - Expect(ConvertFileToStructuredResource(ModelRegistrySVCPath, mrSvc)).To(Succeed()) - - mrSvc.SetNamespace(defaultNamespace) - - if err := cli.Create(ctx, mrSvc); err != nil && !errors.IsAlreadyExists(err) { - Fail(err.Error()) - } - inferenceServiceController := inferenceservicecontroller.NewInferenceServiceController( cli, ctrl.Log.WithName("controllers").WithName("ModelRegistry-InferenceService-Controller"), @@ -145,33 +134,20 @@ var _ = BeforeSuite(func() { nameLabel, urlAnnotation, finalizerLabel, + serviceURLAnnotation, defaultNamespace, ) mrMockServer = ModelRegistryDefaultMockServer() - inferenceServiceController.OverrideHTTPClient(&http.Client{ - Transport: &http.Transport{ - Proxy: func(req *http.Request) (*url.URL, error) { - if strings.Contains(req.URL.String(), "svc.cluster.local") { - url, err := url.Parse(mrMockServer.URL) - if err != nil { - return nil, err - } - - logf.Log.Info("Proxying request", "request", req.URL) - - proxyUrl, err := http.ProxyURL(url)(req) - - logf.Log.Info("Proxying request", "proxyUrl", proxyUrl) + mockUrl, _ := url.Parse(mrMockServer.URL) - return proxyUrl, err - } + mrMockServer.Client().Transport = RewriteTransport{ + Transport: mrMockServer.Client().Transport, + URL: mockUrl, + } - return req.URL, nil - }, - }, - }) + inferenceServiceController.OverrideHTTPClient(mrMockServer.Client()) err = inferenceServiceController.SetupWithManager(mgr) Expect(err).ToNot(HaveOccurred()) @@ -269,7 +245,7 @@ func ModelRegistryDefaultMockServer() *httptest.Server { w.WriteHeader(http.StatusOK) }) - return httptest.NewServer(handler) + return httptest.NewTLSServer(handler) } func RegisterSchemes(s *runtime.Scheme) { @@ -321,3 +297,19 @@ func DownloadFile(url string, path string) error { return nil } + +type RewriteTransport struct { + Transport http.RoundTripper + URL *url.URL +} + +func (t RewriteTransport) RoundTrip(req *http.Request) (*http.Response, error) { + req.URL.Scheme = t.URL.Scheme + req.URL.Host = t.URL.Host + req.URL.Path = path.Join(t.URL.Path, req.URL.Path) + rt := t.Transport + if rt == nil { + rt = http.DefaultTransport + } + return rt.RoundTrip(req) +} diff --git a/pkg/inferenceservice-controller/testdata/deploy/model-registry-svc.yaml b/pkg/inferenceservice-controller/testdata/deploy/model-registry-svc.yaml index 08af702d..0d6e6ffc 100644 --- a/pkg/inferenceservice-controller/testdata/deploy/model-registry-svc.yaml +++ b/pkg/inferenceservice-controller/testdata/deploy/model-registry-svc.yaml @@ -3,8 +3,8 @@ kind: Service metadata: labels: app: metadata + component: model-registry name: model-registry - namespace: default spec: ports: - appProtocol: http diff --git a/pkg/inferenceservice-controller/testdata/inferenceservices/inference-service-correct.yaml b/pkg/inferenceservice-controller/testdata/inferenceservices/inference-service-correct.yaml index edb6c9f3..f0cb68b3 100644 --- a/pkg/inferenceservice-controller/testdata/inferenceservices/inference-service-correct.yaml +++ b/pkg/inferenceservice-controller/testdata/inferenceservices/inference-service-correct.yaml @@ -2,11 +2,9 @@ apiVersion: serving.kserve.io/v1beta1 kind: InferenceService metadata: name: example-onnx-mnist - namespace: default labels: modelregistry.kubeflow.org/registered-model-id: "1" modelregistry.kubeflow.org/name: "model-registry" - modelregistry.kubeflow.org/namespace: "default" spec: predictor: model: diff --git a/pkg/inferenceservice-controller/testdata/inferenceservices/inference-service-missing-name.yaml b/pkg/inferenceservice-controller/testdata/inferenceservices/inference-service-missing-name.yaml new file mode 100644 index 00000000..fd714db4 --- /dev/null +++ b/pkg/inferenceservice-controller/testdata/inferenceservices/inference-service-missing-name.yaml @@ -0,0 +1,15 @@ +apiVersion: serving.kserve.io/v1beta1 +kind: InferenceService +metadata: + name: example-onnx-mnist-missing-name + labels: + modelregistry.kubeflow.org/registered-model-id: "1" +spec: + predictor: + model: + modelFormat: + name: onnx + runtime: kserve-ovms + storage: + key: testkey + path: /testpath/test