From b2571d9fb370e56a3f53be89f5d81f4bd6b89105 Mon Sep 17 00:00:00 2001 From: Harshad Reddy Nalla Date: Thu, 2 Oct 2025 14:39:07 -0400 Subject: [PATCH 1/4] setup virtualservice to route traffic for workspaces - Update the helper func with CopyDeepVirtualService Signed-off-by: Harshad Reddy Nalla --- .../internal/models/workspaces/funcs.go | 2 +- .../api/v1beta1/workspacekind_types.go | 4 +- workspaces/controller/cmd/main.go | 3 + .../bases/kubeflow.org_workspacekinds.yaml | 4 +- .../config/manager/kustomization.yaml | 6 + .../controller/config/manager/manager.yaml | 16 ++ .../controller/config/manager/params.env | 4 + .../jupyterlab_v1beta1_workspacekind.yaml | 4 +- workspaces/controller/go.mod | 3 + workspaces/controller/go.sum | 10 +- .../internal/controller/suite_test.go | 5 + .../controller/workspace_controller.go | 177 +++++++++++++++++- .../controller/internal/helper/helper.go | 39 ++++ .../controller/internal/helper/index.go | 23 ++- .../controller/internal/webhook/suite_test.go | 4 + workspaces/controller/test/e2e/e2e_test.go | 2 +- .../cypress/tests/mocked/workspace.mock.ts | 2 +- 17 files changed, 292 insertions(+), 16 deletions(-) create mode 100644 workspaces/controller/config/manager/params.env diff --git a/workspaces/backend/internal/models/workspaces/funcs.go b/workspaces/backend/internal/models/workspaces/funcs.go index 7329057b3..c4cf3caa6 100644 --- a/workspaces/backend/internal/models/workspaces/funcs.go +++ b/workspaces/backend/internal/models/workspaces/funcs.go @@ -359,7 +359,7 @@ func buildServices(ws *kubefloworgv1beta1.Workspace, wskPodTemplatePorts map[kub case kubefloworgv1beta1.ImagePortProtocolHTTP: services[i].HttpService = &HttpService{ DisplayName: ptr.Deref(port.DisplayName, wskPort.DefaultDisplayName), - HttpPath: fmt.Sprintf("/workspace/%s/%s/%s/", ws.Namespace, ws.Name, port.Id), + HttpPath: fmt.Sprintf("/workspace/connect/%s/%s/%s/", ws.Namespace, ws.Name, port.Id), } } } diff --git a/workspaces/controller/api/v1beta1/workspacekind_types.go b/workspaces/controller/api/v1beta1/workspacekind_types.go index 88fccee93..9bdd44da4 100644 --- a/workspaces/controller/api/v1beta1/workspacekind_types.go +++ b/workspaces/controller/api/v1beta1/workspacekind_types.go @@ -279,7 +279,7 @@ type WorkspaceKindVolumeMounts struct { type HTTPProxy struct { // if the path prefix is stripped from incoming HTTP requests - // - if true, the '/workspace/{profile_name}/{workspace_name}/' path prefix + // - if true, the '/workspace/connect/{profile_name}/{workspace_name}/' path prefix // is stripped from incoming requests, the application sees the request // as if it was made to '/...' // - this only works if the application serves RELATIVE URLs for its assets @@ -291,7 +291,7 @@ type HTTPProxy struct { // - sets the `spec.http[].headers.request` of the Istio VirtualService // https://istio.io/latest/docs/reference/config/networking/virtual-service/#Headers-HeaderOperations // - the following string templates are available: - // - `.PathPrefix`: the path prefix of the Workspace (e.g. '/workspace/{profile_name}/{workspace_name}/') + // - `.PathPrefix`: the path prefix of the Workspace (e.g. '/workspace/connect/{profile_name}/{workspace_name}/') // +kubebuilder:validation:Optional RequestHeaders *IstioHeaderOperations `json:"requestHeaders,omitempty"` } diff --git a/workspaces/controller/cmd/main.go b/workspaces/controller/cmd/main.go index 296580456..a3f8659b5 100644 --- a/workspaces/controller/cmd/main.go +++ b/workspaces/controller/cmd/main.go @@ -25,6 +25,7 @@ import ( // to ensure that exec-entrypoint and run can make use of them. _ "k8s.io/client-go/plugin/pkg/client/auth" + istiov1 "istio.io/client-go/pkg/apis/networking/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" @@ -50,6 +51,8 @@ var ( func init() { utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + utilruntime.Must(istiov1.AddToScheme(scheme)) + utilruntime.Must(kubefloworgv1beta1.AddToScheme(scheme)) // +kubebuilder:scaffold:scheme } diff --git a/workspaces/controller/config/crd/bases/kubeflow.org_workspacekinds.yaml b/workspaces/controller/config/crd/bases/kubeflow.org_workspacekinds.yaml index 80c174a2e..1f0848945 100644 --- a/workspaces/controller/config/crd/bases/kubeflow.org_workspacekinds.yaml +++ b/workspaces/controller/config/crd/bases/kubeflow.org_workspacekinds.yaml @@ -3699,7 +3699,7 @@ spec: default: false description: |- if the path prefix is stripped from incoming HTTP requests - - if true, the '/workspace/{profile_name}/{workspace_name}/' path prefix + - if true, the '/workspace/connect/{profile_name}/{workspace_name}/' path prefix is stripped from incoming requests, the application sees the request as if it was made to '/...' - this only works if the application serves RELATIVE URLs for its assets @@ -3710,7 +3710,7 @@ spec: - sets the `spec.http[].headers.request` of the Istio VirtualService https://istio.io/latest/docs/reference/config/networking/virtual-service/#Headers-HeaderOperations - the following string templates are available: - - `.PathPrefix`: the path prefix of the Workspace (e.g. '/workspace/{profile_name}/{workspace_name}/') + - `.PathPrefix`: the path prefix of the Workspace (e.g. '/workspace/connect/{profile_name}/{workspace_name}/') properties: add: additionalProperties: diff --git a/workspaces/controller/config/manager/kustomization.yaml b/workspaces/controller/config/manager/kustomization.yaml index ea39fc1f5..f4578cee6 100644 --- a/workspaces/controller/config/manager/kustomization.yaml +++ b/workspaces/controller/config/manager/kustomization.yaml @@ -8,6 +8,12 @@ labels: pairs: app.kubernetes.io/component: controller-manager +configMapGenerator: +- envs: + - params.env + name: config +generatorOptions: + disableNameSuffixHash: true images: - name: workspaces-controller newName: ghcr.io/kubeflow/notebooks/workspaces-controller diff --git a/workspaces/controller/config/manager/manager.yaml b/workspaces/controller/config/manager/manager.yaml index 1595fea2e..67295c964 100644 --- a/workspaces/controller/config/manager/manager.yaml +++ b/workspaces/controller/config/manager/manager.yaml @@ -54,6 +54,22 @@ spec: - --health-probe-bind-address=:8081 - --metrics-bind-address=0 image: workspaces-controller:latest + env: + - name: USE_ISTIO + valueFrom: + configMapKeyRef: + name: config + key: USE_ISTIO + - name: ISTIO_GATEWAY + valueFrom: + configMapKeyRef: + name: config + key: ISTIO_GATEWAY + - name: ISTIO_HOST + valueFrom: + configMapKeyRef: + name: config + key: ISTIO_HOST imagePullPolicy: IfNotPresent name: manager securityContext: diff --git a/workspaces/controller/config/manager/params.env b/workspaces/controller/config/manager/params.env new file mode 100644 index 000000000..cf6f8d1e3 --- /dev/null +++ b/workspaces/controller/config/manager/params.env @@ -0,0 +1,4 @@ +USE_ISTIO=true +ISTIO_GATEWAY=kubeflow/kubeflow-gateway +ISTIO_HOST=* +CLUSTER_DOMAIN=cluster.local diff --git a/workspaces/controller/config/samples/jupyterlab_v1beta1_workspacekind.yaml b/workspaces/controller/config/samples/jupyterlab_v1beta1_workspacekind.yaml index 6dfa28649..55fa5701b 100644 --- a/workspaces/controller/config/samples/jupyterlab_v1beta1_workspacekind.yaml +++ b/workspaces/controller/config/samples/jupyterlab_v1beta1_workspacekind.yaml @@ -153,7 +153,7 @@ spec: httpProxy: ## if the path prefix is stripped from incoming HTTP requests - ## - if true, the '/workspace/{profile_name}/{workspace_name}/' path prefix + ## - if true, the '/workspace/connect/{profile_name}/{workspace_name}/' path prefix ## is stripped from incoming requests, the application sees the request ## as if it was made to '/...' ## - this only works if the application serves RELATIVE URLs for its assets @@ -164,7 +164,7 @@ spec: ## - sets the `spec.http[].headers.request` of the Istio VirtualService ## https://istio.io/latest/docs/reference/config/networking/virtual-service/#Headers-HeaderOperations ## - the following string templates are available: - ## - `.PathPrefix`: the path prefix of the Workspace (e.g. '/workspace/{profile_name}/{workspace_name}/') + ## - `.PathPrefix`: the path prefix of the Workspace (e.g. '/workspace/connect/{profile_name}/{workspace_name}/') ## requestHeaders: {} #set: { "X-RStudio-Root-Path": "{{ .PathPrefix }}" } # for RStudio diff --git a/workspaces/controller/go.mod b/workspaces/controller/go.mod index b5c848044..f4bd2b7d9 100644 --- a/workspaces/controller/go.mod +++ b/workspaces/controller/go.mod @@ -7,6 +7,8 @@ require ( github.com/onsi/ginkgo/v2 v2.19.0 github.com/onsi/gomega v1.33.1 golang.org/x/time v0.3.0 + istio.io/api v1.22.8 + istio.io/client-go v1.22.8 k8s.io/api v0.31.0 k8s.io/apimachinery v0.31.0 k8s.io/client-go v0.31.0 @@ -59,6 +61,7 @@ require ( golang.org/x/text v0.16.0 // indirect golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d // indirect gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect + google.golang.org/genproto/googleapis/api v0.0.0-20240528184218-531527333157 // indirect google.golang.org/protobuf v1.34.2 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect diff --git a/workspaces/controller/go.sum b/workspaces/controller/go.sum index 8496f957c..03e28f87b 100644 --- a/workspaces/controller/go.sum +++ b/workspaces/controller/go.sum @@ -9,8 +9,8 @@ github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/emicklei/go-restful/v3 v3.11.0 h1:rAQeMHw1c7zTmncogyy8VvRZwtkmkZ4FxERmMY4rD+g= github.com/emicklei/go-restful/v3 v3.11.0/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc= -github.com/evanphx/json-patch v0.5.2 h1:xVCHIVMUu1wtM/VkR9jVZ45N3FhZfYMMYGorLCR8P3k= -github.com/evanphx/json-patch v0.5.2/go.mod h1:ZWS5hhDbVDyob71nXKNL0+PWn6ToqBHMikGIFbs31qQ= +github.com/evanphx/json-patch v5.6.0+incompatible h1:jBYDEEiFBPxA0v50tFdvOzQQTCvpL6mnFh5mB2/l16U= +github.com/evanphx/json-patch v5.6.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= github.com/evanphx/json-patch/v5 v5.9.0 h1:kcBlZQbplgElYIlo/n1hJbls2z/1awpXxpRi0/FOJfg= github.com/evanphx/json-patch/v5 v5.9.0/go.mod h1:VNkHZ/282BpEyt/tObQO8s5CMPmYYq14uClGH4abBuQ= github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA= @@ -155,6 +155,8 @@ golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8T golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gomodules.xyz/jsonpatch/v2 v2.4.0 h1:Ci3iUJyx9UeRx7CeFN8ARgGbkESwJK+KB9lLcWxY/Zw= gomodules.xyz/jsonpatch/v2 v2.4.0/go.mod h1:AH3dM2RI6uoBZxn3LVrfvJ3E0/9dG4cSrbuBJT4moAY= +google.golang.org/genproto/googleapis/api v0.0.0-20240528184218-531527333157 h1:7whR9kGa5LUwFtpLm2ArCEejtnxlGeLbAyjFY8sGNFw= +google.golang.org/genproto/googleapis/api v0.0.0-20240528184218-531527333157/go.mod h1:99sLkeliLXfdj2J75X3Ho+rrVCaJze0uwN7zDDkjPVU= google.golang.org/protobuf v1.34.2 h1:6xV6lTsCfpGD21XK49h7MhtcApnLqkfYgPcdHftf6hg= google.golang.org/protobuf v1.34.2/go.mod h1:qYOHts0dSfpeUzUFpOMr/WGzszTmLH+DiWniOlNbLDw= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= @@ -170,6 +172,10 @@ gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +istio.io/api v1.22.8 h1:mhkaeFJ13WZ2d6pvL9+exNeQ9UB6HX7e6m+XwO9XoYY= +istio.io/api v1.22.8/go.mod h1:S3l8LWqNYS9yT+d4bH+jqzH2lMencPkW7SKM1Cu9EyM= +istio.io/client-go v1.22.8 h1:wojmt220jSbfhpRDsPiflj2nSFTBuYtZNiW9hqKeaWE= +istio.io/client-go v1.22.8/go.mod h1:noO8SoyMxLwni3w+yGK67aydi2klExjmiqnXyeRS/00= k8s.io/api v0.31.0 h1:b9LiSjR2ym/SzTOlfMHm1tr7/21aD7fSkqgD/CVJBCo= k8s.io/api v0.31.0/go.mod h1:0YiFF+JfFxMM6+1hQei8FY8M7s1Mth+z/q7eF1aJkTE= k8s.io/apiextensions-apiserver v0.31.0 h1:fZgCVhGwsclj3qCw1buVXCV6khjRzKC5eCFt24kyLSk= diff --git a/workspaces/controller/internal/controller/suite_test.go b/workspaces/controller/internal/controller/suite_test.go index c606cf8b7..80f361c95 100644 --- a/workspaces/controller/internal/controller/suite_test.go +++ b/workspaces/controller/internal/controller/suite_test.go @@ -42,6 +42,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + + istiov1 "istio.io/client-go/pkg/apis/networking/v1" + "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" // +kubebuilder:scaffold:imports ) @@ -88,6 +91,8 @@ var _ = BeforeSuite(func() { By("setting up the scheme") err = kubefloworgv1beta1.AddToScheme(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) + err = istiov1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) // +kubebuilder:scaffold:scheme diff --git a/workspaces/controller/internal/controller/workspace_controller.go b/workspaces/controller/internal/controller/workspace_controller.go index b88e27290..ad96e4deb 100644 --- a/workspaces/controller/internal/controller/workspace_controller.go +++ b/workspaces/controller/internal/controller/workspace_controller.go @@ -19,9 +19,12 @@ package controller import ( "context" "fmt" + "os" "strings" "github.com/go-logr/logr" + networkingv1 "istio.io/api/networking/v1" + istiov1 "istio.io/client-go/pkg/apis/networking/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -68,6 +71,7 @@ const ( stateMsgErrorGenFailureService = "Workspace failed to generate Service with error: %s" stateMsgErrorMultipleStatefulSets = "Workspace owns multiple StatefulSets: %s" stateMsgErrorMultipleServices = "Workspace owns multiple Services: %s" + stateMsgErrorMultipleVirtualServices = "Workspace owns multiple VirtualServices: %s" stateMsgErrorStatefulSetWarningEvent = "Workspace StatefulSet has warning event: %s" stateMsgErrorPodUnschedulable = "Workspace Pod is unschedulable: %s" stateMsgErrorPodSchedulingGate = "Workspace Pod is waiting for scheduling gate: %s" @@ -359,6 +363,76 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // and implement the `spec.podTemplate.httpProxy` options // + log.V(2).Info("reconciling VirtualService for Workspace") + if os.Getenv("USE_ISTIO") == "true" { + // generateVirtualService + currentPodTemplatePortsMap := make(map[kubefloworgv1beta1.PortId]kubefloworgv1beta1.WorkspaceKindPort) + for _, port := range workspaceKind.Spec.PodTemplate.Ports { + currentPodTemplatePortsMap[port.Id] = port + } + + virtualsvc, err := generateVirtualService(workspace, currentPodTemplatePortsMap, serviceName, currentImageConfig.Spec) + if err != nil { + log.V(0).Info("failed to generate VirtualService for Workspace", "error", err.Error()) + return r.updateWorkspaceState(ctx, log, workspace, + kubefloworgv1beta1.WorkspaceStateError, + fmt.Sprintf("failed to generate VirtualService for Workspace: %s", err.Error()), + ) + } + if err := ctrl.SetControllerReference(workspace, virtualsvc, r.Scheme); err != nil { + log.Error(err, "unable to set controller reference on VirtualService") + return ctrl.Result{}, err + } + + // fetch VirtualServices + // NOTE: we filter by VirtualServices that are owned by the Workspace, not by name + // this allows us to generate a random name for the VirtualService with `metadata.generateName` + var VirtualServiceName string + ownedVirtualServices := &istiov1.VirtualServiceList{} + listOpts = &client.ListOptions{ + FieldSelector: fields.OneTermEqualSelector(helper.IndexWorkspaceOwnerField, workspace.Name), + Namespace: req.Namespace, + } + if err := r.List(ctx, ownedVirtualServices, listOpts); err != nil { + log.Error(err, "unable to list VirtualServices") + return ctrl.Result{}, err + } + switch numVirtualServices := len(ownedVirtualServices.Items); { + case numVirtualServices > 1: + virtualServiceList := make([]string, len(ownedVirtualServices.Items)) + for i, vs := range ownedVirtualServices.Items { + virtualServiceList[i] = vs.Name + } + virtualServiceListString := strings.Join(virtualServiceList, ", ") + log.Error(nil, "Workspace owns multiple VirtualServices", "virtualServices", virtualServiceListString) + return r.updateWorkspaceState(ctx, log, workspace, + kubefloworgv1beta1.WorkspaceStateError, + fmt.Sprintf(stateMsgErrorMultipleVirtualServices, virtualServiceListString), + ) + case numVirtualServices == 0: + if err := r.Create(ctx, virtualsvc); err != nil { + log.Error(err, "unable to create VirtualService") + return ctrl.Result{}, err + } + VirtualServiceName = virtualsvc.ObjectMeta.Name + log.V(2).Info("VirtualService created", "virtualService", VirtualServiceName) + default: + foundVirtualService := ownedVirtualServices.Items[0] + VirtualServiceName = foundVirtualService.ObjectMeta.Name + if helper.CopyVirtualServiceFields(virtualsvc, foundVirtualService) { + if err := r.Update(ctx, foundVirtualService); err != nil { + if apierrors.IsConflict(err) { + log.V(2).Info("update conflict while updating VirtualService, will requeue") + return ctrl.Result{Requeue: true}, nil + } + log.Error(err, "unable to update VirtualService") + return ctrl.Result{}, err + } + log.V(2).Info("VirtualService updated", "virtualService", VirtualServiceName) + } + } + } + // fetch Pod // NOTE: the first StatefulSet Pod is always called "{statefulSetName}-0" podName := fmt.Sprintf("%s-0", statefulSetName) @@ -418,11 +492,20 @@ func (r *WorkspaceReconciler) SetupWithManager(mgr ctrl.Manager, opts controller return labelExists }) - return ctrl.NewControllerManagedBy(mgr). + // Build the controller with core resources + controllerBuilder := ctrl.NewControllerManagedBy(mgr). WithOptions(opts). For(&kubefloworgv1beta1.Workspace{}). Owns(&appsv1.StatefulSet{}). - Owns(&corev1.Service{}). + Owns(&corev1.Service{}) + + // Conditionally add VirtualService ownership if the CRD is available + // This prevents test failures when Istio CRDs are not installed + if _, err := mgr.GetRESTMapper().RESTMapping(istiov1.SchemeGroupVersion.WithKind("VirtualService").GroupKind()); err == nil { + controllerBuilder = controllerBuilder.Owns(&istiov1.VirtualService{}) + } + + return controllerBuilder. Watches( &kubefloworgv1beta1.WorkspaceKind{}, handler.EnqueueRequestsFromMapFunc(r.mapWorkspaceKindToRequest), @@ -629,7 +712,7 @@ func generateStatefulSet(workspace *kubefloworgv1beta1.Workspace, workspaceKind httpPathPrefixFunc := func(portId kubefloworgv1beta1.PortId) string { port, ok := containerPortsIdMap[portId] if ok { - return fmt.Sprintf("/workspace/%s/%s/%s/", workspace.Namespace, workspace.Name, port.Id) + return fmt.Sprintf("/workspace/connect/%s/%s/%s/", workspace.Namespace, workspace.Name, port.Id) } else { return "" } @@ -881,6 +964,94 @@ func generateService(workspace *kubefloworgv1beta1.Workspace, imageConfigSpec ku return service, nil } +// generateVirtualService generates a VirtualService for a Workspace +func generateVirtualService(workspace *kubefloworgv1beta1.Workspace, currentPodTemplatePortsMap map[kubefloworgv1beta1.PortId]kubefloworgv1beta1.WorkspaceKindPort, serviceName string, imageConfigSpec kubefloworgv1beta1.ImageConfigSpec) (*istiov1.VirtualService, error) { + // NOTE: the name prefix is used to generate a unique name for the VirtualService + namePrefix := generateNamePrefix(workspace.Name, maxServiceNameLength) + + // TODO: Add a possible default for istioGateway + istioGateway := os.Getenv("ISTIO_GATEWAY") + if istioGateway == "" { + return nil, fmt.Errorf("ISTIO_GATEWAY environment variable is not set") + } + + istioHosts := "*" + if istioHostsEnv, ok := os.LookupEnv("ISTIO_HOSTS"); ok { + istioHosts = istioHostsEnv + } + + clusterDomain := "cluster.local" + if clusterDomainEnv, ok := os.LookupEnv("CLUSTER_DOMAIN"); ok { + clusterDomain = clusterDomainEnv + } + serviceHost := fmt.Sprintf("%s.%s.svc.%s", serviceName, workspace.Namespace, clusterDomain) + + virtualService := &istiov1.VirtualService{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: namePrefix, + Namespace: workspace.Namespace, + Labels: map[string]string{ + workspaceNameLabel: workspace.Name, + }, + }, + Spec: networkingv1.VirtualService{ + Gateways: []string{istioGateway}, + Hosts: []string{istioHosts}, + }, + } + + for _, port := range imageConfigSpec.Ports { + if _, exists := currentPodTemplatePortsMap[port.Id]; exists { + podTemplatePort := currentPodTemplatePortsMap[port.Id] + matchUriPrefix := fmt.Sprintf("/workspace/connect/%s/%s/%s/", workspace.Namespace, workspace.Name, port.Id) + + // Additional Cases would be added for SSH, etc. + switch podTemplatePort.Protocol { //nolint:gocritic + case kubefloworgv1beta1.ImagePortProtocolHTTP: + virtualServiceSpecHttp := &networkingv1.HTTPRoute{ + Headers: &networkingv1.Headers{ + Request: &networkingv1.Headers_HeaderOperations{}, + }, + Rewrite: &networkingv1.HTTPRewrite{}, + Match: []*networkingv1.HTTPMatchRequest{ + { + Uri: &networkingv1.StringMatch{ + MatchType: &networkingv1.StringMatch_Prefix{ + Prefix: matchUriPrefix, + }, + }, + }, + }, + Route: []*networkingv1.HTTPRouteDestination{ + { + Destination: &networkingv1.Destination{ + Host: serviceHost, + Port: &networkingv1.PortSelector{ + Number: uint32(port.Port), //nolint:gosec + }, + }, + }, + }, + } + if !ptr.Deref(podTemplatePort.HTTPProxy.RemovePathPrefix, false) { + virtualServiceSpecHttp.Rewrite.Uri = fmt.Sprintf("/workspace/connect/%s/%s/%s/", workspace.Namespace, workspace.Name, port.Id) + } + if podTemplatePort.HTTPProxy.RequestHeaders.Set != nil { + virtualServiceSpecHttp.Headers.Request.Set = podTemplatePort.HTTPProxy.RequestHeaders.Set + } + if podTemplatePort.HTTPProxy.RequestHeaders.Add != nil { + virtualServiceSpecHttp.Headers.Request.Add = podTemplatePort.HTTPProxy.RequestHeaders.Add + } + if podTemplatePort.HTTPProxy.RequestHeaders.Remove != nil { + virtualServiceSpecHttp.Headers.Request.Remove = podTemplatePort.HTTPProxy.RequestHeaders.Remove + } + virtualService.Spec.Http = append(virtualService.Spec.Http, virtualServiceSpecHttp) + } + } + } + return virtualService, nil +} + // generateWorkspaceStatus generates a WorkspaceStatus for a Workspace func (r *WorkspaceReconciler) generateWorkspaceStatus(ctx context.Context, log logr.Logger, workspace *kubefloworgv1beta1.Workspace, pod *corev1.Pod, statefulSet *appsv1.StatefulSet) (kubefloworgv1beta1.WorkspaceStatus, error) { // NOTE: some fields are populated before this function is called, diff --git a/workspaces/controller/internal/helper/helper.go b/workspaces/controller/internal/helper/helper.go index d4d6bdef2..3a5403200 100644 --- a/workspaces/controller/internal/helper/helper.go +++ b/workspaces/controller/internal/helper/helper.go @@ -19,6 +19,7 @@ package helper import ( "reflect" + istiov1 "istio.io/client-go/pkg/apis/networking/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -118,6 +119,44 @@ func CopyServiceFields(desired *corev1.Service, target *corev1.Service) bool { return requireUpdate } +// CopyVirtualServiceFields updates a target VirtualService with the fields from a desired VirtualService, returning true if an update is required. +func CopyVirtualServiceFields(desired *istiov1.VirtualService, target *istiov1.VirtualService) bool { + requireUpdate := false + + // Using the Spec definition https://pkg.go.dev/istio.io/api/networking/v1alpha3alpha3#VirtualService + if !reflect.DeepEqual(target.Spec.Gateways, desired.Spec.Gateways) { + target.Spec.Gateways = desired.Spec.Gateways + requireUpdate = true + } + + if !reflect.DeepEqual(target.Spec.Hosts, desired.Spec.Hosts) { + target.Spec.Hosts = desired.Spec.Hosts + requireUpdate = true + } + + if !reflect.DeepEqual(target.Spec.Http, desired.Spec.Http) { + target.Spec.Http = desired.Spec.Http + requireUpdate = true + } + + if !reflect.DeepEqual(target.Spec.Tls, desired.Spec.Tls) { + target.Spec.Tls = desired.Spec.Tls + requireUpdate = true + } + + if !reflect.DeepEqual(target.Spec.Tcp, desired.Spec.Tcp) { + target.Spec.Tcp = desired.Spec.Tcp + requireUpdate = true + } + + if !reflect.DeepEqual(target.Spec.ExportTo, desired.Spec.ExportTo) { + target.Spec.ExportTo = desired.Spec.ExportTo + requireUpdate = true + } + + return requireUpdate +} + // NormalizePodConfigSpec normalizes a PodConfigSpec so that it can be compared with reflect.DeepEqual func NormalizePodConfigSpec(spec kubefloworgv1beta1.PodConfigSpec) error { diff --git a/workspaces/controller/internal/helper/index.go b/workspaces/controller/internal/helper/index.go index 7c3d06163..88310ad1e 100644 --- a/workspaces/controller/internal/helper/index.go +++ b/workspaces/controller/internal/helper/index.go @@ -19,6 +19,7 @@ package helper import ( "context" + istiov1 "istio.io/client-go/pkg/apis/networking/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -32,6 +33,7 @@ const ( IndexEventInvolvedObjectUidField = ".involvedObject.uid" IndexWorkspaceOwnerField = ".metadata.controller" IndexWorkspaceKindField = ".spec.kind" + OwnerKindWorkspace = "Workspace" ) // SetupManagerFieldIndexers sets up field indexes on a controller-runtime manager @@ -55,7 +57,7 @@ func SetupManagerFieldIndexers(mgr ctrl.Manager) error { if owner == nil { return nil } - if owner.APIVersion != kubefloworgv1beta1.GroupVersion.String() || owner.Kind != "Workspace" { + if owner.APIVersion != kubefloworgv1beta1.GroupVersion.String() || owner.Kind != OwnerKindWorkspace { return nil } return []string{owner.Name} @@ -70,7 +72,7 @@ func SetupManagerFieldIndexers(mgr ctrl.Manager) error { if owner == nil { return nil } - if owner.APIVersion != kubefloworgv1beta1.GroupVersion.String() || owner.Kind != "Workspace" { + if owner.APIVersion != kubefloworgv1beta1.GroupVersion.String() || owner.Kind != OwnerKindWorkspace { return nil } return []string{owner.Name} @@ -78,6 +80,23 @@ func SetupManagerFieldIndexers(mgr ctrl.Manager) error { return err } + // Index VirtualService by its owner Workspace + // This is conditional and will not fail if VirtualService CRD is not available (e.g., in test environments) + if err := mgr.GetFieldIndexer().IndexField(context.Background(), &istiov1.VirtualService{}, IndexWorkspaceOwnerField, func(rawObj client.Object) []string { + virtualService := rawObj.(*istiov1.VirtualService) + owner := metav1.GetControllerOf(virtualService) + if owner == nil { + return nil + } + if owner.APIVersion != kubefloworgv1beta1.GroupVersion.String() || owner.Kind != OwnerKindWorkspace { + return nil + } + return []string{owner.Name} + }); err != nil { + // Log the error but don't fail if VirtualService CRD is not available (e.g., in test environments) + ctrl.Log.Info("Failed to setup VirtualService field indexer, VirtualService CRD may not be available", "error", err) + } + // Index Workspace by WorkspaceKind if err := mgr.GetFieldIndexer().IndexField(context.Background(), &kubefloworgv1beta1.Workspace{}, IndexWorkspaceKindField, func(rawObj client.Object) []string { ws := rawObj.(*kubefloworgv1beta1.Workspace) diff --git a/workspaces/controller/internal/webhook/suite_test.go b/workspaces/controller/internal/webhook/suite_test.go index f187c350d..102b0133e 100644 --- a/workspaces/controller/internal/webhook/suite_test.go +++ b/workspaces/controller/internal/webhook/suite_test.go @@ -46,6 +46,8 @@ import ( kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" + + istiov1 "istio.io/client-go/pkg/apis/networking/v1" // +kubebuilder:scaffold:imports ) @@ -95,6 +97,8 @@ var _ = BeforeSuite(func() { By("setting up the scheme") err = kubefloworgv1beta1.AddToScheme(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) + err = istiov1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) // +kubebuilder:scaffold:scheme diff --git a/workspaces/controller/test/e2e/e2e_test.go b/workspaces/controller/test/e2e/e2e_test.go index 207621fd1..674267097 100644 --- a/workspaces/controller/test/e2e/e2e_test.go +++ b/workspaces/controller/test/e2e/e2e_test.go @@ -274,7 +274,7 @@ var _ = Describe("controller", Ordered, func() { Eventually(getServiceName, timeout, interval).Should(Succeed()) By("validating that the workspace service endpoint is reachable") - serviceEndpoint := fmt.Sprintf("http://%s:%d/workspace/%s/%s/%s/lab", + serviceEndpoint := fmt.Sprintf("http://%s:%d/workspace/connect/%s/%s/%s/lab", workspaceSvcName, workspacePortInt, workspaceNamespace, workspaceName, workspacePortId, ) curlService := func() error { diff --git a/workspaces/frontend/src/__tests__/cypress/cypress/tests/mocked/workspace.mock.ts b/workspaces/frontend/src/__tests__/cypress/cypress/tests/mocked/workspace.mock.ts index a5e33aec2..16ab31854 100644 --- a/workspaces/frontend/src/__tests__/cypress/cypress/tests/mocked/workspace.mock.ts +++ b/workspaces/frontend/src/__tests__/cypress/cypress/tests/mocked/workspace.mock.ts @@ -80,7 +80,7 @@ const generateMockWorkspace = ( { httpService: { displayName: 'Jupyter-lab', - httpPath: `/workspace/${namespace}/${name}/Jupyter-lab/`, + httpPath: `/workspace/connect/${namespace}/${name}/Jupyter-lab/`, }, }, ], From 2968f61239087e6cea95699700048bf42895cd5a Mon Sep 17 00:00:00 2001 From: Harshad Reddy Nalla Date: Mon, 6 Oct 2025 15:06:39 -0400 Subject: [PATCH 2/4] set istio env var as config across controller Signed-off-by: Harshad Reddy Nalla --- workspaces/controller/cmd/main.go | 34 +++++- .../controller/internal/config/environment.go | 24 ++++ .../internal/controller/suite_test.go | 6 +- .../controller/workspace_controller.go | 114 +++++++++--------- .../controller/internal/helper/index.go | 31 ++--- .../controller/internal/webhook/suite_test.go | 5 +- 6 files changed, 140 insertions(+), 74 deletions(-) create mode 100644 workspaces/controller/internal/config/environment.go diff --git a/workspaces/controller/cmd/main.go b/workspaces/controller/cmd/main.go index a3f8659b5..c65346f4b 100644 --- a/workspaces/controller/cmd/main.go +++ b/workspaces/controller/cmd/main.go @@ -20,6 +20,7 @@ import ( "crypto/tls" "flag" "os" + "strconv" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. @@ -37,6 +38,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "github.com/kubeflow/notebooks/workspaces/controller/internal/config" controllerInternal "github.com/kubeflow/notebooks/workspaces/controller/internal/controller" "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" webhookInternal "github.com/kubeflow/notebooks/workspaces/controller/internal/webhook" @@ -63,6 +65,10 @@ func main() { var probeAddr string var secureMetrics bool var enableHTTP2 bool + + // Define command line flags + cfg := &config.EnvConfig{} + flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") flag.BoolVar(&enableLeaderElection, "leader-elect", false, @@ -72,6 +78,15 @@ func main() { "If set the metrics endpoint is served securely") flag.BoolVar(&enableHTTP2, "enable-http2", false, "If set, HTTP/2 will be enabled for the metrics and webhook servers") + flag.StringVar(&cfg.IstioGateway, "istio-gateway", getEnvAsStr("ISTIO_GATEWAY", ""), + "The name of the Istio gateway to use") + flag.StringVar(&cfg.IstioHosts, "istio-hosts", getEnvAsStr("ISTIO_HOSTS", ""), + "The hosts to use for the Istio VirtualService") + flag.StringVar(&cfg.ClusterDomain, "cluster-domain", getEnvAsStr("CLUSTER_DOMAIN", ""), + "The domain to use for the Istio VirtualService") + flag.BoolVar(&cfg.UseIstio, "use-istio", getEnvAsBool("USE_ISTIO", false), + "If set, Istio will be used") + opts := zap.Options{ Development: true, } @@ -132,7 +147,7 @@ func main() { // setup field indexers on the manager cache. we use these indexes to efficiently // query the cache for things like which Workspaces are using a particular WorkspaceKind - if err := helper.SetupManagerFieldIndexers(mgr); err != nil { + if err := helper.SetupManagerFieldIndexers(mgr, cfg); err != nil { setupLog.Error(err, "unable to setup field indexers") os.Exit(1) } @@ -140,6 +155,7 @@ func main() { if err = (&controllerInternal.WorkspaceReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), + Config: cfg, }).SetupWithManager(mgr, controller.Options{ RateLimiter: helper.BuildRateLimiter(), }); err != nil { @@ -191,3 +207,19 @@ func main() { os.Exit(1) } } + +func getEnvAsStr(name string, defaultVal string) string { + if value, exists := os.LookupEnv(name); exists { + return value + } + return defaultVal +} + +func getEnvAsBool(name string, defaultVal bool) bool { + if value, exists := os.LookupEnv(name); exists { + if boolValue, err := strconv.ParseBool(value); err == nil { + return boolValue + } + } + return defaultVal +} diff --git a/workspaces/controller/internal/config/environment.go b/workspaces/controller/internal/config/environment.go new file mode 100644 index 000000000..5250697f1 --- /dev/null +++ b/workspaces/controller/internal/config/environment.go @@ -0,0 +1,24 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +type EnvConfig struct { + IstioGateway string + IstioHosts string + ClusterDomain string + UseIstio bool +} diff --git a/workspaces/controller/internal/controller/suite_test.go b/workspaces/controller/internal/controller/suite_test.go index 80f361c95..60a6d3ba5 100644 --- a/workspaces/controller/internal/controller/suite_test.go +++ b/workspaces/controller/internal/controller/suite_test.go @@ -45,6 +45,7 @@ import ( istiov1 "istio.io/client-go/pkg/apis/networking/v1" + "github.com/kubeflow/notebooks/workspaces/controller/internal/config" "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" // +kubebuilder:scaffold:imports ) @@ -111,13 +112,16 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) By("setting up the field indexers for the controller manager") - err = helper.SetupManagerFieldIndexers(k8sManager) + // Use test config with UseIstio disabled to avoid VirtualService CRD requirements in tests + testCfg := &config.EnvConfig{UseIstio: false} + err = helper.SetupManagerFieldIndexers(k8sManager, testCfg) Expect(err).NotTo(HaveOccurred()) By("setting up the Workspace controller") err = (&WorkspaceReconciler{ Client: k8sManager.GetClient(), Scheme: k8sManager.GetScheme(), + Config: testCfg, }).SetupWithManager(k8sManager, controller.Options{ RateLimiter: helper.BuildRateLimiter(), }) diff --git a/workspaces/controller/internal/controller/workspace_controller.go b/workspaces/controller/internal/controller/workspace_controller.go index ad96e4deb..e2b6c1f8b 100644 --- a/workspaces/controller/internal/controller/workspace_controller.go +++ b/workspaces/controller/internal/controller/workspace_controller.go @@ -19,7 +19,6 @@ package controller import ( "context" "fmt" - "os" "strings" "github.com/go-logr/logr" @@ -47,6 +46,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "github.com/kubeflow/notebooks/workspaces/controller/internal/config" "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" ) @@ -59,9 +59,13 @@ const ( workspacePodTemplateContainerName = "main" // lengths for resource names - generateNameSuffixLength = 6 - maxServiceNameLength = 63 - maxStatefulSetNameLength = 52 // https://github.com/kubernetes/kubernetes/issues/64023 + generateNameSuffixLength = 6 + maxServiceNameLength = 63 + maxVirtualServiceNameLength = 63 + maxStatefulSetNameLength = 52 // https://github.com/kubernetes/kubernetes/issues/64023 + + // workspace connection path template + workspaceConnectPathTemplate = "/workspace/connect/%s/%s/%s/" // state message formats for Workspace status stateMsgErrorUnknownWorkspaceKind = "Workspace references unknown WorkspaceKind: %s" @@ -89,6 +93,7 @@ const ( type WorkspaceReconciler struct { client.Client Scheme *runtime.Scheme + Config *config.EnvConfig } // +kubebuilder:rbac:groups=kubeflow.org,resources=workspaces,verbs=create;delete;get;list;patch;update;watch @@ -358,20 +363,10 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } } - // - // TODO: reconcile the Istio VirtualService to expose the Workspace - // and implement the `spec.podTemplate.httpProxy` options - // - - log.V(2).Info("reconciling VirtualService for Workspace") - if os.Getenv("USE_ISTIO") == "true" { + if r.Config.UseIstio { + log.V(2).Info("reconciling VirtualService for Workspace") // generateVirtualService - currentPodTemplatePortsMap := make(map[kubefloworgv1beta1.PortId]kubefloworgv1beta1.WorkspaceKindPort) - for _, port := range workspaceKind.Spec.PodTemplate.Ports { - currentPodTemplatePortsMap[port.Id] = port - } - - virtualsvc, err := generateVirtualService(workspace, currentPodTemplatePortsMap, serviceName, currentImageConfig.Spec) + virtualsvc, err := generateVirtualService(workspace, workspaceKind, serviceName, currentImageConfig.Spec, r.Config) if err != nil { log.V(0).Info("failed to generate VirtualService for Workspace", "error", err.Error()) return r.updateWorkspaceState(ctx, log, workspace, @@ -397,6 +392,7 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( log.Error(err, "unable to list VirtualServices") return ctrl.Result{}, err } + switch numVirtualServices := len(ownedVirtualServices.Items); { case numVirtualServices > 1: virtualServiceList := make([]string, len(ownedVirtualServices.Items)) @@ -499,9 +495,8 @@ func (r *WorkspaceReconciler) SetupWithManager(mgr ctrl.Manager, opts controller Owns(&appsv1.StatefulSet{}). Owns(&corev1.Service{}) - // Conditionally add VirtualService ownership if the CRD is available - // This prevents test failures when Istio CRDs are not installed - if _, err := mgr.GetRESTMapper().RESTMapping(istiov1.SchemeGroupVersion.WithKind("VirtualService").GroupKind()); err == nil { + if r.Config.UseIstio { + controllerBuilder = controllerBuilder.Owns(&istiov1.VirtualService{}) } @@ -653,6 +648,11 @@ func getPodConfig(workspace *kubefloworgv1beta1.Workspace, workspaceKind *kubefl } } +// getWorkspaceConnectPath generates the HTTP path for connecting to a workspace port +func getWorkspaceConnectPath(namespace, workspaceName string, portId kubefloworgv1beta1.PortId) string { + return fmt.Sprintf(workspaceConnectPathTemplate, namespace, workspaceName, portId) +} + // generateNamePrefix generates a name prefix for a Workspace // the format is "ws-{WORKSPACE_NAME}-" the workspace name is truncated to fit within the max length func generateNamePrefix(workspaceName string, maxLength int) string { @@ -712,7 +712,7 @@ func generateStatefulSet(workspace *kubefloworgv1beta1.Workspace, workspaceKind httpPathPrefixFunc := func(portId kubefloworgv1beta1.PortId) string { port, ok := containerPortsIdMap[portId] if ok { - return fmt.Sprintf("/workspace/connect/%s/%s/%s/", workspace.Namespace, workspace.Name, port.Id) + return getWorkspaceConnectPath(workspace.Namespace, workspace.Name, port.Id) } else { return "" } @@ -965,45 +965,26 @@ func generateService(workspace *kubefloworgv1beta1.Workspace, imageConfigSpec ku } // generateVirtualService generates a VirtualService for a Workspace -func generateVirtualService(workspace *kubefloworgv1beta1.Workspace, currentPodTemplatePortsMap map[kubefloworgv1beta1.PortId]kubefloworgv1beta1.WorkspaceKindPort, serviceName string, imageConfigSpec kubefloworgv1beta1.ImageConfigSpec) (*istiov1.VirtualService, error) { +func generateVirtualService(workspace *kubefloworgv1beta1.Workspace, workspaceKind *kubefloworgv1beta1.WorkspaceKind, serviceName string, imageConfigSpec kubefloworgv1beta1.ImageConfigSpec, cfg *config.EnvConfig) (*istiov1.VirtualService, error) { // NOTE: the name prefix is used to generate a unique name for the VirtualService - namePrefix := generateNamePrefix(workspace.Name, maxServiceNameLength) + namePrefix := generateNamePrefix(workspace.Name, maxVirtualServiceNameLength) - // TODO: Add a possible default for istioGateway - istioGateway := os.Getenv("ISTIO_GATEWAY") - if istioGateway == "" { - return nil, fmt.Errorf("ISTIO_GATEWAY environment variable is not set") - } - - istioHosts := "*" - if istioHostsEnv, ok := os.LookupEnv("ISTIO_HOSTS"); ok { - istioHosts = istioHostsEnv + currentPodTemplatePortsMap := make(map[kubefloworgv1beta1.PortId]kubefloworgv1beta1.WorkspaceKindPort) + for _, port := range workspaceKind.Spec.PodTemplate.Ports { + currentPodTemplatePortsMap[port.Id] = port } clusterDomain := "cluster.local" - if clusterDomainEnv, ok := os.LookupEnv("CLUSTER_DOMAIN"); ok { - clusterDomain = clusterDomainEnv + if cfg.ClusterDomain != "" { + clusterDomain = cfg.ClusterDomain } serviceHost := fmt.Sprintf("%s.%s.svc.%s", serviceName, workspace.Namespace, clusterDomain) - virtualService := &istiov1.VirtualService{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: namePrefix, - Namespace: workspace.Namespace, - Labels: map[string]string{ - workspaceNameLabel: workspace.Name, - }, - }, - Spec: networkingv1.VirtualService{ - Gateways: []string{istioGateway}, - Hosts: []string{istioHosts}, - }, - } - + httpRoutes := []*networkingv1.HTTPRoute{} for _, port := range imageConfigSpec.Ports { if _, exists := currentPodTemplatePortsMap[port.Id]; exists { podTemplatePort := currentPodTemplatePortsMap[port.Id] - matchUriPrefix := fmt.Sprintf("/workspace/connect/%s/%s/%s/", workspace.Namespace, workspace.Name, port.Id) + matchUriPrefix := getWorkspaceConnectPath(workspace.Namespace, workspace.Name, port.Id) // Additional Cases would be added for SSH, etc. switch podTemplatePort.Protocol { //nolint:gocritic @@ -1034,21 +1015,42 @@ func generateVirtualService(workspace *kubefloworgv1beta1.Workspace, currentPodT }, } if !ptr.Deref(podTemplatePort.HTTPProxy.RemovePathPrefix, false) { - virtualServiceSpecHttp.Rewrite.Uri = fmt.Sprintf("/workspace/connect/%s/%s/%s/", workspace.Namespace, workspace.Name, port.Id) + virtualServiceSpecHttp.Rewrite.Uri = matchUriPrefix } - if podTemplatePort.HTTPProxy.RequestHeaders.Set != nil { + if podTemplatePort.HTTPProxy.RequestHeaders != nil { virtualServiceSpecHttp.Headers.Request.Set = podTemplatePort.HTTPProxy.RequestHeaders.Set - } - if podTemplatePort.HTTPProxy.RequestHeaders.Add != nil { virtualServiceSpecHttp.Headers.Request.Add = podTemplatePort.HTTPProxy.RequestHeaders.Add - } - if podTemplatePort.HTTPProxy.RequestHeaders.Remove != nil { virtualServiceSpecHttp.Headers.Request.Remove = podTemplatePort.HTTPProxy.RequestHeaders.Remove } - virtualService.Spec.Http = append(virtualService.Spec.Http, virtualServiceSpecHttp) + httpRoutes = append(httpRoutes, virtualServiceSpecHttp) } } } + + if cfg.IstioGateway == "" { + return nil, fmt.Errorf("ISTIO_GATEWAY environment variable is not set") + } + + istioHosts := "*" + if cfg.IstioHosts != "" { + istioHosts = cfg.IstioHosts + } + + virtualService := &istiov1.VirtualService{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: namePrefix, + Namespace: workspace.Namespace, + Labels: map[string]string{ + workspaceNameLabel: workspace.Name, + }, + }, + Spec: networkingv1.VirtualService{ + Gateways: []string{cfg.IstioGateway}, + Hosts: []string{istioHosts}, + Http: httpRoutes, + }, + } + return virtualService, nil } diff --git a/workspaces/controller/internal/helper/index.go b/workspaces/controller/internal/helper/index.go index 88310ad1e..26bd1060a 100644 --- a/workspaces/controller/internal/helper/index.go +++ b/workspaces/controller/internal/helper/index.go @@ -27,6 +27,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "github.com/kubeflow/notebooks/workspaces/controller/internal/config" ) const ( @@ -37,7 +38,7 @@ const ( ) // SetupManagerFieldIndexers sets up field indexes on a controller-runtime manager -func SetupManagerFieldIndexers(mgr ctrl.Manager) error { +func SetupManagerFieldIndexers(mgr ctrl.Manager, cfg *config.EnvConfig) error { // Index Event by `involvedObject.uid` if err := mgr.GetFieldIndexer().IndexField(context.Background(), &corev1.Event{}, IndexEventInvolvedObjectUidField, func(rawObj client.Object) []string { @@ -80,21 +81,21 @@ func SetupManagerFieldIndexers(mgr ctrl.Manager) error { return err } - // Index VirtualService by its owner Workspace - // This is conditional and will not fail if VirtualService CRD is not available (e.g., in test environments) - if err := mgr.GetFieldIndexer().IndexField(context.Background(), &istiov1.VirtualService{}, IndexWorkspaceOwnerField, func(rawObj client.Object) []string { - virtualService := rawObj.(*istiov1.VirtualService) - owner := metav1.GetControllerOf(virtualService) - if owner == nil { - return nil + // Index VirtualService by its owner Workspace (only when Istio is enabled) + if cfg.UseIstio { + if err := mgr.GetFieldIndexer().IndexField(context.Background(), &istiov1.VirtualService{}, IndexWorkspaceOwnerField, func(rawObj client.Object) []string { + virtualService := rawObj.(*istiov1.VirtualService) + owner := metav1.GetControllerOf(virtualService) + if owner == nil { + return nil + } + if owner.APIVersion != kubefloworgv1beta1.GroupVersion.String() || owner.Kind != OwnerKindWorkspace { + return nil + } + return []string{owner.Name} + }); err != nil { + return err } - if owner.APIVersion != kubefloworgv1beta1.GroupVersion.String() || owner.Kind != OwnerKindWorkspace { - return nil - } - return []string{owner.Name} - }); err != nil { - // Log the error but don't fail if VirtualService CRD is not available (e.g., in test environments) - ctrl.Log.Info("Failed to setup VirtualService field indexer, VirtualService CRD may not be available", "error", err) } // Index Workspace by WorkspaceKind diff --git a/workspaces/controller/internal/webhook/suite_test.go b/workspaces/controller/internal/webhook/suite_test.go index 102b0133e..f5f74635a 100644 --- a/workspaces/controller/internal/webhook/suite_test.go +++ b/workspaces/controller/internal/webhook/suite_test.go @@ -45,6 +45,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "github.com/kubeflow/notebooks/workspaces/controller/internal/config" "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" istiov1 "istio.io/client-go/pkg/apis/networking/v1" @@ -124,7 +125,9 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) By("setting up the field indexers for the controller manager") - err = helper.SetupManagerFieldIndexers(k8sManager) + // Use test config with UseIstio disabled to avoid VirtualService CRD requirements in tests + testCfg := &config.EnvConfig{UseIstio: false} + err = helper.SetupManagerFieldIndexers(k8sManager, testCfg) Expect(err).NotTo(HaveOccurred()) By("setting up the Workspace webhook") From 3900875fc453ae539facf1a40856e569838be3f8 Mon Sep 17 00:00:00 2001 From: Harshad Reddy Nalla Date: Mon, 6 Oct 2025 15:07:27 -0400 Subject: [PATCH 3/4] make controller manifest consistent with frontend/backend Signed-off-by: Harshad Reddy Nalla --- workspaces/controller/Makefile | 6 ++-- .../components/common/kustomization.yaml | 9 ++++++ .../components/istio/kustomization.yaml | 16 ++++++++++ .../components/istio/manager_istio_patch.yaml | 30 +++++++++++++++++++ .../{manager => components/istio}/params.env | 0 .../config/manager/kustomization.yaml | 6 ---- .../controller/config/manager/manager.yaml | 16 ---------- .../config/overlays/istio/kustomization.yaml | 11 +++++++ 8 files changed, 69 insertions(+), 25 deletions(-) create mode 100644 workspaces/controller/config/components/common/kustomization.yaml create mode 100644 workspaces/controller/config/components/istio/kustomization.yaml create mode 100644 workspaces/controller/config/components/istio/manager_istio_patch.yaml rename workspaces/controller/config/{manager => components/istio}/params.env (100%) create mode 100644 workspaces/controller/config/overlays/istio/kustomization.yaml diff --git a/workspaces/controller/Makefile b/workspaces/controller/Makefile index 47985aedc..10c2478c3 100644 --- a/workspaces/controller/Makefile +++ b/workspaces/controller/Makefile @@ -138,12 +138,12 @@ uninstall: manifests kustomize ## Uninstall CRDs from the K8s cluster specified .PHONY: deploy deploy: manifests kustomize ## Deploy controller to the K8s cluster specified in ~/.kube/config. - cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG} - $(KUSTOMIZE) build config/default | $(KUBECTL) apply -f - + cd config/manager && $(KUSTOMIZE) edit set image workspaces-controller=${IMG} + $(KUSTOMIZE) build config/overlays/istio | $(KUBECTL) apply -f - .PHONY: undeploy undeploy: kustomize ## Undeploy controller from the K8s cluster specified in ~/.kube/config. Call with ignore-not-found=true to ignore resource not found errors during deletion. - $(KUSTOMIZE) build config/default | $(KUBECTL) delete --ignore-not-found=$(ignore-not-found) -f - + $(KUSTOMIZE) build config/overlays/istio | $(KUBECTL) delete --ignore-not-found=$(ignore-not-found) -f - ##@ Dependencies diff --git a/workspaces/controller/config/components/common/kustomization.yaml b/workspaces/controller/config/components/common/kustomization.yaml new file mode 100644 index 000000000..c49e81cab --- /dev/null +++ b/workspaces/controller/config/components/common/kustomization.yaml @@ -0,0 +1,9 @@ +apiVersion: kustomize.config.k8s.io/v1alpha1 +kind: Component + +labels: +- includeSelectors: true + pairs: + app.kubernetes.io/managed-by: kustomize + app.kubernetes.io/name: workspaces-controller + app.kubernetes.io/part-of: kubeflow-workspaces \ No newline at end of file diff --git a/workspaces/controller/config/components/istio/kustomization.yaml b/workspaces/controller/config/components/istio/kustomization.yaml new file mode 100644 index 000000000..0a3433b1b --- /dev/null +++ b/workspaces/controller/config/components/istio/kustomization.yaml @@ -0,0 +1,16 @@ +apiVersion: kustomize.config.k8s.io/v1alpha1 +kind: Component + +configMapGenerator: +- envs: + - params.env + name: kubeflow-workspaces-istio-config +generatorOptions: + disableNameSuffixHash: true + +labels: +- pairs: + app.kubernetes.io/component: controller-manager + +patches: +- path: manager_istio_patch.yaml diff --git a/workspaces/controller/config/components/istio/manager_istio_patch.yaml b/workspaces/controller/config/components/istio/manager_istio_patch.yaml new file mode 100644 index 000000000..711dd0c97 --- /dev/null +++ b/workspaces/controller/config/components/istio/manager_istio_patch.yaml @@ -0,0 +1,30 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: workspaces-controller +spec: + template: + spec: + containers: + - name: manager + env: + - name: USE_ISTIO + valueFrom: + configMapKeyRef: + name: config + key: USE_ISTIO + - name: ISTIO_GATEWAY + valueFrom: + configMapKeyRef: + name: config + key: ISTIO_GATEWAY + - name: ISTIO_HOST + valueFrom: + configMapKeyRef: + name: config + key: ISTIO_HOST + - name: CLUSTER_DOMAIN + valueFrom: + configMapKeyRef: + name: config + key: CLUSTER_DOMAIN \ No newline at end of file diff --git a/workspaces/controller/config/manager/params.env b/workspaces/controller/config/components/istio/params.env similarity index 100% rename from workspaces/controller/config/manager/params.env rename to workspaces/controller/config/components/istio/params.env diff --git a/workspaces/controller/config/manager/kustomization.yaml b/workspaces/controller/config/manager/kustomization.yaml index f4578cee6..ea39fc1f5 100644 --- a/workspaces/controller/config/manager/kustomization.yaml +++ b/workspaces/controller/config/manager/kustomization.yaml @@ -8,12 +8,6 @@ labels: pairs: app.kubernetes.io/component: controller-manager -configMapGenerator: -- envs: - - params.env - name: config -generatorOptions: - disableNameSuffixHash: true images: - name: workspaces-controller newName: ghcr.io/kubeflow/notebooks/workspaces-controller diff --git a/workspaces/controller/config/manager/manager.yaml b/workspaces/controller/config/manager/manager.yaml index 67295c964..1595fea2e 100644 --- a/workspaces/controller/config/manager/manager.yaml +++ b/workspaces/controller/config/manager/manager.yaml @@ -54,22 +54,6 @@ spec: - --health-probe-bind-address=:8081 - --metrics-bind-address=0 image: workspaces-controller:latest - env: - - name: USE_ISTIO - valueFrom: - configMapKeyRef: - name: config - key: USE_ISTIO - - name: ISTIO_GATEWAY - valueFrom: - configMapKeyRef: - name: config - key: ISTIO_GATEWAY - - name: ISTIO_HOST - valueFrom: - configMapKeyRef: - name: config - key: ISTIO_HOST imagePullPolicy: IfNotPresent name: manager securityContext: diff --git a/workspaces/controller/config/overlays/istio/kustomization.yaml b/workspaces/controller/config/overlays/istio/kustomization.yaml new file mode 100644 index 000000000..e751551b6 --- /dev/null +++ b/workspaces/controller/config/overlays/istio/kustomization.yaml @@ -0,0 +1,11 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +namespace: kubeflow-workspaces + +resources: +- ../../default + +components: +- ../../components/istio +- ../../components/common From d1c391e2e679d8d6f4a5a548701d42ba243b76e5 Mon Sep 17 00:00:00 2001 From: Harshad Reddy Nalla Date: Mon, 6 Oct 2025 16:19:41 -0400 Subject: [PATCH 4/4] fix the configmap name in manager_istio_patch.yaml Signed-off-by: Harshad Reddy Nalla --- .../config/components/istio/manager_istio_patch.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/workspaces/controller/config/components/istio/manager_istio_patch.yaml b/workspaces/controller/config/components/istio/manager_istio_patch.yaml index 711dd0c97..38fe201a8 100644 --- a/workspaces/controller/config/components/istio/manager_istio_patch.yaml +++ b/workspaces/controller/config/components/istio/manager_istio_patch.yaml @@ -11,20 +11,20 @@ spec: - name: USE_ISTIO valueFrom: configMapKeyRef: - name: config + name: kubeflow-workspaces-istio-config key: USE_ISTIO - name: ISTIO_GATEWAY valueFrom: configMapKeyRef: - name: config + name: kubeflow-workspaces-istio-config key: ISTIO_GATEWAY - name: ISTIO_HOST valueFrom: configMapKeyRef: - name: config + name: kubeflow-workspaces-istio-config key: ISTIO_HOST - name: CLUSTER_DOMAIN valueFrom: configMapKeyRef: - name: config + name: kubeflow-workspaces-istio-config key: CLUSTER_DOMAIN \ No newline at end of file