From 30d1facfbc54419ed05770062bdfb7669ec4d056 Mon Sep 17 00:00:00 2001 From: Harshad Reddy Nalla Date: Mon, 28 Jul 2025 17:50:41 -0400 Subject: [PATCH] include ports array in podtemplate for httpProxy setting - moved protocal from imageconfig.spec.ports to podtemplates.ports - included the podtemplates.ports with defaultdisplayname - add validation webhook for podtemplate.ports - update the sample workspacekind with ports reference - referencing same id for portid in imageconfig and podtemplate.ports Signed-off-by: Harshad Reddy Nalla --- workspaces/backend/api/suite_test.go | 28 ++-- .../api/workspacekinds_handler_test.go | 1 - .../internal/models/workspaces/funcs.go | 14 +- .../api/v1beta1/workspacekind_types.go | 49 +++++-- .../api/v1beta1/zz_generated.deepcopy.go | 39 ++++- .../bases/kubeflow.org_workspacekinds.yaml | 135 +++++++++++------- .../jupyterlab_v1beta1_workspacekind.yaml | 58 ++++---- .../internal/controller/suite_test.go | 28 ++-- .../controller/workspace_controller.go | 4 +- .../controller/internal/helper/template.go | 4 +- .../controller/internal/webhook/suite_test.go | 89 ++++++++---- .../internal/webhook/workspacekind_webhook.go | 44 +++++- .../webhook/workspacekind_webhook_test.go | 43 +++++- 13 files changed, 372 insertions(+), 164 deletions(-) diff --git a/workspaces/backend/api/suite_test.go b/workspaces/backend/api/suite_test.go index 9c0c77fef..7ad4e5462 100644 --- a/workspaces/backend/api/suite_test.go +++ b/workspaces/backend/api/suite_test.go @@ -245,12 +245,19 @@ func NewExampleWorkspaceKind(name string) *kubefloworgv1beta1.WorkspaceKind { VolumeMounts: kubefloworgv1beta1.WorkspaceKindVolumeMounts{ Home: "/home/jovyan", }, - HTTPProxy: &kubefloworgv1beta1.HTTPProxy{ - RemovePathPrefix: ptr.To(false), - RequestHeaders: &kubefloworgv1beta1.IstioHeaderOperations{ - Set: map[string]string{"X-RStudio-Root-Path": "{{ .PathPrefix }}"}, - Add: map[string]string{}, - Remove: []string{}, + Ports: []kubefloworgv1beta1.WorkspaceKindPort{ + { + Id: "jupyterlab", + DefaultDisplayName: "JupyterLab", + Protocol: "HTTP", + HTTPProxy: &kubefloworgv1beta1.HTTPProxy{ + RemovePathPrefix: ptr.To(false), + RequestHeaders: &kubefloworgv1beta1.IstioHeaderOperations{ + Set: map[string]string{}, + Add: map[string]string{}, + Remove: []string{}, + }, + }, }, }, ExtraEnv: []v1.EnvVar{ @@ -317,9 +324,8 @@ func NewExampleWorkspaceKind(name string) *kubefloworgv1beta1.WorkspaceKind { Ports: []kubefloworgv1beta1.ImagePort{ { Id: "jupyterlab", - DisplayName: "JupyterLab", + DisplayName: ptr.To("JupyterLab"), Port: 8888, - Protocol: "HTTP", }, }, }, @@ -341,10 +347,8 @@ func NewExampleWorkspaceKind(name string) *kubefloworgv1beta1.WorkspaceKind { Image: "ghcr.io/kubeflow/kubeflow/notebook-servers/jupyter-scipy:v1.9.0", Ports: []kubefloworgv1beta1.ImagePort{ { - Id: "jupyterlab", - DisplayName: "JupyterLab", - Port: 8888, - Protocol: "HTTP", + Id: "jupyterlab", + Port: 8888, }, }, }, diff --git a/workspaces/backend/api/workspacekinds_handler_test.go b/workspaces/backend/api/workspacekinds_handler_test.go index 464659272..1d90055bb 100644 --- a/workspaces/backend/api/workspacekinds_handler_test.go +++ b/workspaces/backend/api/workspacekinds_handler_test.go @@ -299,7 +299,6 @@ spec: - id: "jupyterlab" displayName: "JupyterLab" port: 8888 - protocol: "HTTP" podConfig: spawner: default: "tiny_cpu" diff --git a/workspaces/backend/internal/models/workspaces/funcs.go b/workspaces/backend/internal/models/workspaces/funcs.go index 610c79ea2..7da40bee7 100644 --- a/workspaces/backend/internal/models/workspaces/funcs.go +++ b/workspaces/backend/internal/models/workspaces/funcs.go @@ -91,6 +91,10 @@ func NewWorkspaceModelFromWorkspace(ws *kubefloworgv1beta1.Workspace, wsk *kubef imageConfigModel, imageConfigValue := buildImageConfig(ws, wsk) podConfigModel, _ := buildPodConfig(ws, wsk) + var wskPodTemplatePorts []kubefloworgv1beta1.WorkspaceKindPort + if wskExists(wsk) { + wskPodTemplatePorts = wsk.Spec.PodTemplate.Ports + } workspaceModel := Workspace{ Name: ws.Name, @@ -128,7 +132,7 @@ func NewWorkspaceModelFromWorkspace(ws *kubefloworgv1beta1.Workspace, wsk *kubef // https://github.com/kubeflow/notebooks/issues/38 LastProbe: nil, }, - Services: buildServices(ws, imageConfigValue), + Services: buildServices(ws, wskPodTemplatePorts, imageConfigValue), } return workspaceModel } @@ -332,7 +336,7 @@ func buildRedirectMessage(msg *kubefloworgv1beta1.RedirectMessage) *RedirectMess } } -func buildServices(ws *kubefloworgv1beta1.Workspace, imageConfigValue *kubefloworgv1beta1.ImageConfigValue) []Service { +func buildServices(ws *kubefloworgv1beta1.Workspace, wskPodTemplatePorts []kubefloworgv1beta1.WorkspaceKindPort, imageConfigValue *kubefloworgv1beta1.ImageConfigValue) []Service { if imageConfigValue == nil { return nil } @@ -340,10 +344,12 @@ func buildServices(ws *kubefloworgv1beta1.Workspace, imageConfigValue *kubeflowo services := make([]Service, len(imageConfigValue.Spec.Ports)) for i := range imageConfigValue.Spec.Ports { port := imageConfigValue.Spec.Ports[i] - switch port.Protocol { //nolint:gocritic + portocol := wskPodTemplatePorts[i].Protocol + // golint complains about the single case in switch statement + switch portocol { //nolint:gocritic case kubefloworgv1beta1.ImagePortProtocolHTTP: services[i].HttpService = &HttpService{ - DisplayName: port.DisplayName, + DisplayName: ptr.Deref(port.DisplayName, wskPodTemplatePorts[i].DefaultDisplayName), HttpPath: fmt.Sprintf("/workspace/%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 7e7952a02..f40282620 100644 --- a/workspaces/controller/api/v1beta1/workspacekind_types.go +++ b/workspaces/controller/api/v1beta1/workspacekind_types.go @@ -29,6 +29,13 @@ import ( =============================================================================== */ +// PortId the id of the port +// +// +kubebuilder:validation:MinLength:=1 +// +kubebuilder:validation:MaxLength:=32 +// +kubebuilder:validation:Pattern:=^[a-z0-9][a-z0-9_-]*[a-z0-9]$ +type PortId string + // WorkspaceKindSpec defines the desired state of WorkspaceKind type WorkspaceKindSpec struct { @@ -115,9 +122,11 @@ type WorkspaceKindPodTemplate struct { // volume mount paths VolumeMounts WorkspaceKindVolumeMounts `json:"volumeMounts"` - // http proxy configs (MUTABLE) - // +kubebuilder:validation:Optional - HTTPProxy *HTTPProxy `json:"httpProxy,omitempty"` + // ports that the container listens on + // +kubebuilder:validation:MinItems:=1 + // +listType:="map" + // +listMapKey:="id" + Ports []WorkspaceKindPort `json:"ports,omitempty"` // environment variables for Workspace Pods (MUTABLE) // - the following go template functions are available: @@ -151,6 +160,27 @@ type WorkspaceKindPodTemplate struct { Options WorkspaceKindPodOptions `json:"options"` } +type WorkspaceKindPort struct { + // the id of the port + // - identifier for the port in `imageconfig` ports.[].id + // +kubebuilder:example="jupyterlab" + Id PortId `json:"id"` + + // the protocol of the port + // +kubebuilder:example:="HTTP" + Protocol ImagePortProtocol `json:"protocol"` + + // the display name of the port + // +kubebuilder:validation:MinLength:=2 + // +kubebuilder:validation:MaxLength:=64 + // +kubebuilder:example:="JupyterLab" + DefaultDisplayName string `json:"displayName"` + + // the http proxy config for the port (MUTABLE) + // +kubebuilder:validation:Optional + HTTPProxy *HTTPProxy `json:"httpProxy,omitempty"` +} + type WorkspaceKindPodMetadata struct { // labels to be applied to the Pod resource // +kubebuilder:validation:Optional @@ -339,11 +369,8 @@ type ImageConfigSpec struct { type ImagePort struct { // the id of the port // - this is NOT used as the Container or Service port name, but as part of the HTTP path - // +kubebuilder:validation:MinLength:=1 - // +kubebuilder:validation:MaxLength:=32 - // +kubebuilder:validation:Pattern:=^[a-z0-9][a-z0-9_-]*[a-z0-9]$ // +kubebuilder:example="jupyterlab" - Id string `json:"id"` + Id PortId `json:"id"` // the port number // +kubebuilder:validation:Minimum:=1 @@ -354,12 +381,8 @@ type ImagePort struct { // the display name of the port // +kubebuilder:validation:MinLength:=2 // +kubebuilder:validation:MaxLength:=64 - // +kubebuilder:example:="JupyterLab" - DisplayName string `json:"displayName"` - - // the protocol of the port - // +kubebuilder:example:="HTTP" - Protocol ImagePortProtocol `json:"protocol"` + // +kubebuilder:validation:Optional + DisplayName *string `json:"displayName,omitempty"` } // +kubebuilder:validation:Enum:={"HTTP"} diff --git a/workspaces/controller/api/v1beta1/zz_generated.deepcopy.go b/workspaces/controller/api/v1beta1/zz_generated.deepcopy.go index 568c3f809..5667c71f6 100644 --- a/workspaces/controller/api/v1beta1/zz_generated.deepcopy.go +++ b/workspaces/controller/api/v1beta1/zz_generated.deepcopy.go @@ -144,7 +144,9 @@ func (in *ImageConfigSpec) DeepCopyInto(out *ImageConfigSpec) { if in.Ports != nil { in, out := &in.Ports, &out.Ports *out = make([]ImagePort, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } } @@ -183,6 +185,11 @@ func (in *ImageConfigValue) DeepCopy() *ImageConfigValue { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ImagePort) DeepCopyInto(out *ImagePort) { *out = *in + if in.DisplayName != nil { + in, out := &in.DisplayName, &out.DisplayName + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ImagePort. @@ -701,10 +708,12 @@ func (in *WorkspaceKindPodTemplate) DeepCopyInto(out *WorkspaceKindPodTemplate) (*in).DeepCopyInto(*out) } out.VolumeMounts = in.VolumeMounts - if in.HTTPProxy != nil { - in, out := &in.HTTPProxy, &out.HTTPProxy - *out = new(HTTPProxy) - (*in).DeepCopyInto(*out) + if in.Ports != nil { + in, out := &in.Ports, &out.Ports + *out = make([]WorkspaceKindPort, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } if in.ExtraEnv != nil { in, out := &in.ExtraEnv, &out.ExtraEnv @@ -750,6 +759,26 @@ func (in *WorkspaceKindPodTemplate) DeepCopy() *WorkspaceKindPodTemplate { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *WorkspaceKindPort) DeepCopyInto(out *WorkspaceKindPort) { + *out = *in + if in.HTTPProxy != nil { + in, out := &in.HTTPProxy, &out.HTTPProxy + *out = new(HTTPProxy) + (*in).DeepCopyInto(*out) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WorkspaceKindPort. +func (in *WorkspaceKindPort) DeepCopy() *WorkspaceKindPort { + if in == nil { + return nil + } + out := new(WorkspaceKindPort) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *WorkspaceKindProbes) DeepCopyInto(out *WorkspaceKindProbes) { *out = *in diff --git a/workspaces/controller/config/crd/bases/kubeflow.org_workspacekinds.yaml b/workspaces/controller/config/crd/bases/kubeflow.org_workspacekinds.yaml index 9fea44636..a7dec7f4b 100644 --- a/workspaces/controller/config/crd/bases/kubeflow.org_workspacekinds.yaml +++ b/workspaces/controller/config/crd/bases/kubeflow.org_workspacekinds.yaml @@ -2275,51 +2275,6 @@ spec: x-kubernetes-list-map-keys: - name x-kubernetes-list-type: map - httpProxy: - description: http proxy configs (MUTABLE) - properties: - removePathPrefix: - default: false - description: |- - if the path prefix is stripped from incoming HTTP requests - - if true, the '/workspace/{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 - type: boolean - requestHeaders: - description: |- - header manipulation rules for incoming HTTP requests - - 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}/') - properties: - add: - additionalProperties: - type: string - description: append the given values to the headers specified - by keys (will create a comma-separated list of values) - example: - My-Header: value-to-append - type: object - remove: - description: remove the specified headers - example: - - Header-To-Remove - items: - type: string - type: array - set: - additionalProperties: - type: string - description: overwrite the headers specified by key with - the given values - example: - X-RStudio-Root-Path: '{{ .PathPrefix }}' - type: object - type: object - type: object options: description: options are the user-selectable fields, they determine the PodSpec of the Workspace @@ -2457,7 +2412,6 @@ spec: properties: displayName: description: the display name of the port - example: JupyterLab maxLength: 64 minLength: 2 type: string @@ -2477,17 +2431,9 @@ spec: maximum: 65535 minimum: 1 type: integer - protocol: - description: the protocol of the port - enum: - - HTTP - example: HTTP - type: string required: - - displayName - id - port - - protocol type: object minItems: 1 type: array @@ -3729,6 +3675,87 @@ spec: description: labels to be applied to the Pod resource type: object type: object + ports: + description: ports that the container listens on + items: + properties: + displayName: + description: the display name of the port + example: JupyterLab + maxLength: 64 + minLength: 2 + type: string + httpProxy: + description: the http proxy config for the port (MUTABLE) + properties: + removePathPrefix: + default: false + description: |- + if the path prefix is stripped from incoming HTTP requests + - if true, the '/workspace/{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 + type: boolean + requestHeaders: + description: |- + header manipulation rules for incoming HTTP requests + - 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}/') + properties: + add: + additionalProperties: + type: string + description: append the given values to the headers + specified by keys (will create a comma-separated + list of values) + example: + My-Header: value-to-append + type: object + remove: + description: remove the specified headers + example: + - Header-To-Remove + items: + type: string + type: array + set: + additionalProperties: + type: string + description: overwrite the headers specified by + key with the given values + example: + X-RStudio-Root-Path: '{{ .PathPrefix }}' + type: object + type: object + type: object + id: + description: |- + the id of the port + - identifier for the port in `imageconfig` ports.[].id + example: jupyterlab + maxLength: 32 + minLength: 1 + pattern: ^[a-z0-9][a-z0-9_-]*[a-z0-9]$ + type: string + protocol: + description: the protocol of the port + enum: + - HTTP + example: HTTP + type: string + required: + - displayName + - id + - protocol + type: object + minItems: 1 + type: array + x-kubernetes-list-map-keys: + - id + x-kubernetes-list-type: map probes: description: standard probes to determine Container health (MUTABLE) properties: diff --git a/workspaces/controller/config/samples/jupyterlab_v1beta1_workspacekind.yaml b/workspaces/controller/config/samples/jupyterlab_v1beta1_workspacekind.yaml index d7bb858b2..0476e1878 100644 --- a/workspaces/controller/config/samples/jupyterlab_v1beta1_workspacekind.yaml +++ b/workspaces/controller/config/samples/jupyterlab_v1beta1_workspacekind.yaml @@ -135,28 +135,38 @@ spec: ## home: "/home/jovyan" - ## http proxy configs (MUTABLE) - ## - httpProxy: - - ## if the path prefix is stripped from incoming HTTP requests - ## - if true, the '/workspace/{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 - ## - removePathPrefix: false + ## port configs (MUTABLE) + ports: + + ## the list of ports that the Workspace exposes + ## configs apply to a single port + ## portId is the identifier for the port in `imageconfig` ports.[].id + - id: "jupyterlab" + displayName: "JupyterLab" + protocol: "HTTP" + + ## http proxy configs (MUTABLE) + ## only "HTTP" protocol ports are supported + httpProxy: + + ## if the path prefix is stripped from incoming HTTP requests + ## - if true, the '/workspace/{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 + ## + removePathPrefix: false - ## header manipulation rules for incoming HTTP requests - ## - 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}/') - ## - requestHeaders: {} - #set: { "X-RStudio-Root-Path": "{{ .PathPrefix }}" } # for RStudio - #add: {} - #remove: [] + ## header manipulation rules for incoming HTTP requests + ## - 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}/') + ## + requestHeaders: {} + #set: { "X-RStudio-Root-Path": "{{ .PathPrefix }}" } # for RStudio + #add: {} + #remove: [] ## environment variables for Workspace Pods (MUTABLE) ## - spec for EnvVar: @@ -286,9 +296,8 @@ spec: ## ports: - id: "jupyterlab" - displayName: "JupyterLab" port: 8888 - protocol: "HTTP" + ## ================================ ## EXAMPLE 2: a visible option @@ -305,9 +314,8 @@ spec: imagePullPolicy: "IfNotPresent" ports: - id: "jupyterlab" - displayName: "JupyterLab" port: 8888 - protocol: "HTTP" + ## ============================================================ ## POD CONFIG OPTIONS diff --git a/workspaces/controller/internal/controller/suite_test.go b/workspaces/controller/internal/controller/suite_test.go index fbc5a5918..c606cf8b7 100644 --- a/workspaces/controller/internal/controller/suite_test.go +++ b/workspaces/controller/internal/controller/suite_test.go @@ -220,12 +220,19 @@ func NewExampleWorkspaceKind1(name string) *kubefloworgv1beta1.WorkspaceKind { VolumeMounts: kubefloworgv1beta1.WorkspaceKindVolumeMounts{ Home: "/home/jovyan", }, - HTTPProxy: &kubefloworgv1beta1.HTTPProxy{ - RemovePathPrefix: ptr.To(false), - RequestHeaders: &kubefloworgv1beta1.IstioHeaderOperations{ - Set: map[string]string{"X-RStudio-Root-Path": "{{ .PathPrefix }}"}, - Add: map[string]string{}, - Remove: []string{}, + Ports: []kubefloworgv1beta1.WorkspaceKindPort{ + { + Id: "jupyterlab", + DefaultDisplayName: "JupyterLab", + Protocol: "HTTP", + HTTPProxy: &kubefloworgv1beta1.HTTPProxy{ + RemovePathPrefix: ptr.To(false), + RequestHeaders: &kubefloworgv1beta1.IstioHeaderOperations{ + Set: map[string]string{}, + Add: map[string]string{}, + Remove: []string{}, + }, + }, }, }, ExtraEnv: []v1.EnvVar{ @@ -292,9 +299,8 @@ func NewExampleWorkspaceKind1(name string) *kubefloworgv1beta1.WorkspaceKind { Ports: []kubefloworgv1beta1.ImagePort{ { Id: "jupyterlab", - DisplayName: "JupyterLab", + DisplayName: ptr.To("JupyterLab"), Port: 8888, - Protocol: "HTTP", }, }, }, @@ -316,10 +322,8 @@ func NewExampleWorkspaceKind1(name string) *kubefloworgv1beta1.WorkspaceKind { Image: "ghcr.io/kubeflow/kubeflow/notebook-servers/jupyter-scipy:v1.9.0", Ports: []kubefloworgv1beta1.ImagePort{ { - Id: "jupyterlab", - DisplayName: "JupyterLab", - Port: 8888, - Protocol: "HTTP", + Id: "jupyterlab", + Port: 8888, }, }, }, diff --git a/workspaces/controller/internal/controller/workspace_controller.go b/workspaces/controller/internal/controller/workspace_controller.go index 66c13b174..b88e27290 100644 --- a/workspaces/controller/internal/controller/workspace_controller.go +++ b/workspaces/controller/internal/controller/workspace_controller.go @@ -625,8 +625,8 @@ func generateStatefulSet(workspace *kubefloworgv1beta1.Workspace, workspaceKind // define go string template functions // NOTE: these are used in places like the `extraEnv` values - containerPortsIdMap := make(map[string]kubefloworgv1beta1.ImagePort) - httpPathPrefixFunc := func(portId string) string { + containerPortsIdMap := make(map[kubefloworgv1beta1.PortId]kubefloworgv1beta1.ImagePort) + 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) diff --git a/workspaces/controller/internal/helper/template.go b/workspaces/controller/internal/helper/template.go index 601c38c6b..524a832d4 100644 --- a/workspaces/controller/internal/helper/template.go +++ b/workspaces/controller/internal/helper/template.go @@ -20,10 +20,12 @@ import ( "bytes" "fmt" "text/template" + + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" ) // RenderExtraEnvValueTemplate renders a single WorkspaceKind `spec.podTemplate.extraEnv[].value` string template -func RenderExtraEnvValueTemplate(rawValue string, httpPathPrefixFunc func(string) string) (string, error) { +func RenderExtraEnvValueTemplate(rawValue string, httpPathPrefixFunc func(kubefloworgv1beta1.PortId) string) (string, error) { // Parse the raw value as a template tmpl, err := template.New("value"). diff --git a/workspaces/controller/internal/webhook/suite_test.go b/workspaces/controller/internal/webhook/suite_test.go index 2eee78390..f187c350d 100644 --- a/workspaces/controller/internal/webhook/suite_test.go +++ b/workspaces/controller/internal/webhook/suite_test.go @@ -208,12 +208,24 @@ func NewExampleWorkspaceKind(name string) *kubefloworgv1beta1.WorkspaceKind { VolumeMounts: kubefloworgv1beta1.WorkspaceKindVolumeMounts{ Home: "/home/jovyan", }, - HTTPProxy: &kubefloworgv1beta1.HTTPProxy{ - RemovePathPrefix: ptr.To(false), - RequestHeaders: &kubefloworgv1beta1.IstioHeaderOperations{ - Set: map[string]string{"X-RStudio-Root-Path": "{{ .PathPrefix }}"}, - Add: map[string]string{}, - Remove: []string{}, + Ports: []kubefloworgv1beta1.WorkspaceKindPort{ + { + Id: "jupyterlab", + DefaultDisplayName: "JupyterLab", + Protocol: "HTTP", + HTTPProxy: &kubefloworgv1beta1.HTTPProxy{ + RemovePathPrefix: ptr.To(false), + RequestHeaders: &kubefloworgv1beta1.IstioHeaderOperations{ + Set: map[string]string{}, + Add: map[string]string{}, + Remove: []string{}, + }, + }, + }, + { + Id: "my_port", + DefaultDisplayName: "My Port", + Protocol: "HTTP", }, }, ExtraEnv: []v1.EnvVar{ @@ -280,9 +292,8 @@ func NewExampleWorkspaceKind(name string) *kubefloworgv1beta1.WorkspaceKind { Ports: []kubefloworgv1beta1.ImagePort{ { Id: "jupyterlab", - DisplayName: "JupyterLab", + DisplayName: ptr.To("JupyterLab"), Port: 8888, - Protocol: "HTTP", }, }, }, @@ -304,10 +315,8 @@ func NewExampleWorkspaceKind(name string) *kubefloworgv1beta1.WorkspaceKind { Image: "ghcr.io/kubeflow/kubeflow/notebook-servers/jupyter-scipy:v1.9.0", Ports: []kubefloworgv1beta1.ImagePort{ { - Id: "jupyterlab", - DisplayName: "JupyterLab", - Port: 8888, - Protocol: "HTTP", + Id: "jupyterlab", + Port: 8888, }, }, }, @@ -326,9 +335,8 @@ func NewExampleWorkspaceKind(name string) *kubefloworgv1beta1.WorkspaceKind { Ports: []kubefloworgv1beta1.ImagePort{ { Id: "my_port", - DisplayName: "something", + DisplayName: ptr.To("something"), Port: 1234, - Protocol: "HTTP", }, }, }, @@ -346,10 +354,8 @@ func NewExampleWorkspaceKind(name string) *kubefloworgv1beta1.WorkspaceKind { Image: "redirect-test:step-2", Ports: []kubefloworgv1beta1.ImagePort{ { - Id: "my_port", - DisplayName: "something", - Port: 1234, - Protocol: "HTTP", + Id: "my_port", + Port: 1234, }, }, }, @@ -364,10 +370,8 @@ func NewExampleWorkspaceKind(name string) *kubefloworgv1beta1.WorkspaceKind { Image: "redirect-test:step-3", Ports: []kubefloworgv1beta1.ImagePort{ { - Id: "my_port", - DisplayName: "something", - Port: 1234, - Protocol: "HTTP", + Id: "my_port", + Port: 1234, }, }, }, @@ -599,15 +603,48 @@ func NewExampleWorkspaceKindWithDuplicatePorts(name string) *kubefloworgv1beta1. workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values[0].Spec.Ports = []kubefloworgv1beta1.ImagePort{ { Id: "jupyterlab", - DisplayName: "JupyterLab", + DisplayName: ptr.To("JupyterLab"), Port: 8888, - Protocol: "HTTP", }, { Id: "jupyterlab2", - DisplayName: "JupyterLab2", + DisplayName: ptr.To("JupyterLab2"), Port: 8888, - Protocol: "HTTP", + }, + } + return workspaceKind +} + +// NewExampleWorkspaceKindWithEmptyPortsArrayInPodTemplate returns a WorkspaceKind with an empty ports array in podTemplate.ports. +func NewExampleWorkspaceKindWithEmptyPortsArrayInPodTemplate(name string) *kubefloworgv1beta1.WorkspaceKind { + workspaceKind := NewExampleWorkspaceKind(name) + workspaceKind.Spec.PodTemplate.Ports = []kubefloworgv1beta1.WorkspaceKindPort{} + return workspaceKind +} + +// NewExampleWorkspaceKindWithDuplicatePortsInPodTemplate returns a WorkspaceKind with duplicate ports in podTemplate.ports. +func NewExampleWorkspaceKindWithDuplicatePortsInPodTemplate(name string) *kubefloworgv1beta1.WorkspaceKind { + workspaceKind := NewExampleWorkspaceKind(name) + workspaceKind.Spec.PodTemplate.Ports = []kubefloworgv1beta1.WorkspaceKindPort{ + { + Id: "jupyterlab", + DefaultDisplayName: "JupyterLab", + }, + { + Id: "jupyterlab", + DefaultDisplayName: "JupyterLab", + }, + } + return workspaceKind +} + +// NewExampleWorkspaceKindWithNonExistentPortIdInImageConfig returns a WorkspaceKind with a non-existent portId in imageConfig.ports. +func NewExampleWorkspaceKindWithNonExistentPortIdInImageConfig(name string) *kubefloworgv1beta1.WorkspaceKind { + workspaceKind := NewExampleWorkspaceKind(name) + workspaceKind.Spec.PodTemplate.Ports = []kubefloworgv1beta1.WorkspaceKindPort{ + { + Id: "non-existent-port-id", + DefaultDisplayName: "Non Existent Port", }, } return workspaceKind diff --git a/workspaces/controller/internal/webhook/workspacekind_webhook.go b/workspaces/controller/internal/webhook/workspacekind_webhook.go index e7fd49a38..9163cd97e 100644 --- a/workspaces/controller/internal/webhook/workspacekind_webhook.go +++ b/workspaces/controller/internal/webhook/workspacekind_webhook.go @@ -98,6 +98,12 @@ func (v *WorkspaceKindValidator) ValidateCreate(ctx context.Context, obj runtime } } + // generate helper maps for podtemplate ports + podTemplatePortsIdMap := make(map[kubefloworgv1beta1.PortId]kubefloworgv1beta1.WorkspaceKindPort) + for _, port := range workspaceKind.Spec.PodTemplate.Ports { + podTemplatePortsIdMap[port.Id] = port + } + // validate default options allErrs = append(allErrs, validateDefaultImageConfig(workspaceKind, imageConfigIdMap)...) allErrs = append(allErrs, validateDefaultPodConfig(workspaceKind, podConfigIdMap)...) @@ -106,7 +112,7 @@ func (v *WorkspaceKindValidator) ValidateCreate(ctx context.Context, obj runtime for _, imageConfigValue := range imageConfigIdMap { imageConfigValueId := imageConfigValue.Id imageConfigValuePath := field.NewPath("spec", "podTemplate", "options", "imageConfig", "values").Key(imageConfigValueId) - allErrs = append(allErrs, validateImageConfigValue(&imageConfigValue, imageConfigValuePath)...) + allErrs = append(allErrs, validateImageConfigValue(&imageConfigValue, imageConfigValuePath, podTemplatePortsIdMap)...) } // validate redirects @@ -156,6 +162,15 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new allErrs = append(allErrs, validateExtraEnv(newWorkspaceKind)...) } + // if the ports config changed, we need to validate all image config values again + shouldValidateAllImageConfigValues := !equality.Semantic.DeepEqual(newWorkspaceKind.Spec.PodTemplate.Ports, oldWorkspaceKind.Spec.PodTemplate.Ports) + + // generate helper maps for podtemplate ports + podTemplatePortsIdMap := make(map[kubefloworgv1beta1.PortId]kubefloworgv1beta1.WorkspaceKindPort) + for _, port := range newWorkspaceKind.Spec.PodTemplate.Ports { + podTemplatePortsIdMap[port.Id] = port + } + // calculate changes to imageConfig values var shouldValidateImageConfigRedirects = false toValidateImageConfigIds := make(map[string]bool) @@ -173,6 +188,11 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new newImageConfigRedirectMap[imageConfigValue.Id] = imageConfigValue.Redirect.To } + // if ports changed, we need to validate all image config values + if shouldValidateAllImageConfigValues { + toValidateImageConfigIds[imageConfigValue.Id] = true + } + // check if the imageConfig value is new if _, exists := oldImageConfigIdMap[imageConfigValue.Id]; !exists { // we need to validate this imageConfig value since it is new @@ -181,6 +201,7 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new // we always need to validate the imageConfig redirects if an imageConfig value was added // because the new imageConfig value could be used by a redirect or cause a cycle shouldValidateImageConfigRedirects = true + } else { // if we haven't already decided to validate the imageConfig redirects, // check if the redirect has changed @@ -331,7 +352,7 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new for imageConfigValueId := range toValidateImageConfigIds { imageConfigValue := newImageConfigIdMap[imageConfigValueId] imageConfigValuePath := field.NewPath("spec", "podTemplate", "options", "imageConfig", "values").Key(imageConfigValueId) - allErrs = append(allErrs, validateImageConfigValue(&imageConfigValue, imageConfigValuePath)...) + allErrs = append(allErrs, validateImageConfigValue(&imageConfigValue, imageConfigValuePath, podTemplatePortsIdMap)...) } // process bad imageConfig values @@ -521,7 +542,7 @@ func validateExtraEnv(workspaceKind *kubefloworgv1beta1.WorkspaceKind) []*field. var errs []*field.Error // the real httpPathPrefix can't fail, so we return a dummy value - httpPathPrefixFunc := func(portId string) string { + httpPathPrefixFunc := func(portId kubefloworgv1beta1.PortId) string { return "DUMMY_HTTP_PATH_PREFIX" } @@ -569,19 +590,32 @@ func validateDefaultPodConfig(workspaceKind *kubefloworgv1beta1.WorkspaceKind, p } // validateImageConfigValue validates an imageConfig value -func validateImageConfigValue(imageConfigValue *kubefloworgv1beta1.ImageConfigValue, imageConfigValuePath *field.Path) []*field.Error { +func validateImageConfigValue(imageConfigValue *kubefloworgv1beta1.ImageConfigValue, imageConfigValuePath *field.Path, podTemplatePortsIdMap map[kubefloworgv1beta1.PortId]kubefloworgv1beta1.WorkspaceKindPort) []*field.Error { var errs []*field.Error // validate the ports seenPorts := make(map[int32]bool) for _, port := range imageConfigValue.Spec.Ports { - portId := port.Id + portId := string(port.Id) portNumber := port.Port if _, exists := seenPorts[portNumber]; exists { portPath := imageConfigValuePath.Child("spec", "ports").Key(portId).Child("port") errs = append(errs, field.Invalid(portPath, portNumber, fmt.Sprintf("port %d is defined more than once", portNumber))) } seenPorts[portNumber] = true + + // validate that the port ID exists in podTemplate.ports + if _, exists := podTemplatePortsIdMap[port.Id]; !exists { + portIdPath := imageConfigValuePath.Child("spec", "ports").Key(portId).Child("id") + errs = append(errs, field.Invalid(portIdPath, port.Id, "missing from spec.podTemplate.ports")) + } else { + // validate HTTPProxy is only set if protocol is HTTP + podTemplatePort := podTemplatePortsIdMap[port.Id] + if podTemplatePort.HTTPProxy != nil && podTemplatePort.Protocol != kubefloworgv1beta1.ImagePortProtocolHTTP { + httpProxyPath := imageConfigValuePath.Child("spec", "ports").Key(portId).Child("httpProxy") + errs = append(errs, field.Invalid(httpProxyPath, podTemplatePort.HTTPProxy, "httpProxy can only be set when protocol is HTTP")) + } + } } return errs diff --git a/workspaces/controller/internal/webhook/workspacekind_webhook_test.go b/workspaces/controller/internal/webhook/workspacekind_webhook_test.go index dd2c321ae..afab5c021 100644 --- a/workspaces/controller/internal/webhook/workspacekind_webhook_test.go +++ b/workspaces/controller/internal/webhook/workspacekind_webhook_test.go @@ -25,6 +25,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" @@ -98,6 +99,21 @@ var _ = Describe("WorkspaceKind Webhook", func() { workspaceKind: NewExampleWorkspaceKindWithDuplicatePorts("wsk-webhook-create--image-config-duplicate-ports"), shouldSucceed: false, }, + { + description: "should reject creation with empty ports array in podTemplate", + workspaceKind: NewExampleWorkspaceKindWithEmptyPortsArrayInPodTemplate("wsk-webhook-create--pod-template-empty-ports-array"), + shouldSucceed: false, + }, + { + description: "should reject creation with duplicate ports in podTemplate.ports", + workspaceKind: NewExampleWorkspaceKindWithDuplicatePortsInPodTemplate("wsk-webhook-create--pod-template-duplicate-portids"), + shouldSucceed: false, + }, + { + description: "should reject creation with non-existent portId in imageConfig.ports", + workspaceKind: NewExampleWorkspaceKindWithNonExistentPortIdInImageConfig("wsk-webhook-create--image-config-non-existent-portid"), + shouldSucceed: false, + }, { description: "should reject creation if extraEnv[].value is not a valid Go template", workspaceKind: NewExampleWorkspaceKindWithInvalidExtraEnvValue("wsk-webhook-create--extra-invalid-env-value"), @@ -494,20 +510,39 @@ var _ = Describe("WorkspaceKind Webhook", func() { wsk.Spec.PodTemplate.Options.ImageConfig.Values[1].Spec.Ports = []kubefloworgv1beta1.ImagePort{ { Id: "jupyterlab", - DisplayName: "JupyterLab", + DisplayName: ptr.To("JupyterLab"), Port: duplicatePortNumber, - Protocol: "HTTP", }, { Id: "jupyterlab2", - DisplayName: "JupyterLab2", + DisplayName: ptr.To("JupyterLab2"), Port: duplicatePortNumber, - Protocol: "HTTP", }, } return ContainSubstring("port %d is defined more than once", duplicatePortNumber) }, }, + { + description: "should reject updating a portId in podTemplate.ports to a duplicate portId", + shouldSucceed: false, + + workspaceKind: NewExampleWorkspaceKind(workspaceKindName), + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) gomegaTypes.GomegaMatcher { + wsk.Spec.PodTemplate.Ports[1].Id = "jupyterlab" + return And(ContainSubstring("Duplicate value:"), ContainSubstring("jupyterlab")) + }, + }, + { + description: "should reject updating a portId in podTemplate.ports to a non-existent portId in imageConfig.ports", + shouldSucceed: false, + + workspaceKind: NewExampleWorkspaceKind(workspaceKindName), + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) gomegaTypes.GomegaMatcher { + existingPortId := wsk.Spec.PodTemplate.Ports[0].Id + wsk.Spec.PodTemplate.Ports[0].Id = "non-existent-port-id" + return ContainSubstring("%q: missing from spec.podTemplate.ports", existingPortId) + }, + }, { description: "should reject updating a podMetadata.labels key to an invalid value", shouldSucceed: false,