Skip to content

feat(ws): add spec.podTemplate.ports[] to WorkspaceKind #507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: notebooks-v2
Choose a base branch
from
Open

feat(ws): add spec.podTemplate.ports[] to WorkspaceKind #507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions workspaces/backend/api/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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",
},
},
},
Expand All @@ -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,
},
},
},
Expand Down
5 changes: 4 additions & 1 deletion workspaces/backend/api/workspacekinds_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,10 @@ spec:
name: "default-editor"
volumeMounts:
home: "/home/jovyan"
ports:
- id: "jupyterlab"
displayName: "JupyterLab"
protocol: "HTTP"
options:
imageConfig:
spawner:
Expand All @@ -299,7 +303,6 @@ spec:
- id: "jupyterlab"
displayName: "JupyterLab"
port: 8888
protocol: "HTTP"
podConfig:
spawner:
default: "tiny_cpu"
Expand Down
14 changes: 10 additions & 4 deletions workspaces/backend/internal/models/workspaces/funcs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines +94 to +97
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should b a map from portId to port config so we can look it up by that in the buildServices section.

Same as comment https://github.com/kubeflow/notebooks/pull/507/files#r2342214467 from the last review.


workspaceModel := Workspace{
Name: ws.Name,
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -332,18 +336,20 @@ 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
}

services := make([]Service, len(imageConfigValue.Spec.Ports))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to first build a map from portId -> port config (from the outer workspace kind ports), so that we can look it up when processing each of the ports.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the changes accordingly
thanks

for i := range imageConfigValue.Spec.Ports {
port := imageConfigValue.Spec.Ports[i]
switch port.Protocol { //nolint:gocritic
protocol := wskPodTemplatePorts[i].Protocol
// golint complains about the single case in switch statement
switch protocol { //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),
}
}
Expand Down
49 changes: 36 additions & 13 deletions workspaces/controller/api/v1beta1/workspacekind_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ import (
===============================================================================
*/

// PortId the id of the port
//
Comment on lines +32 to +33
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// PortId the id of the port
//
// an id of a 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 {

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very important to document for users as it is used in kubectl explain to explain what this field is:

Suggested change
// ports that the container listens on
// port definitions which can be referenced in image config values
// - think of port definitions as the "types" of services which could be provided by a specific image
// - a port definition has a common id (URL path) for consistency if the listening TCP port changes
// - ports are referenced in image config values by their `id` and their definition here establishes
// their protocol type, and default display name in the ui

// +kubebuilder:validation:MinItems:=1
// +listType:="map"
// +listMapKey:="id"
Ports []WorkspaceKindPort `json:"ports"`

// environment variables for Workspace Pods (MUTABLE)
// - the following go template functions are available:
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// the display name of the port
// the default display name of the port
// - note, this can be overridden on a per image config value basis

// +kubebuilder:validation:MinLength:=2
// +kubebuilder:validation:MaxLength:=64
// +kubebuilder:example:="JupyterLab"
DefaultDisplayName string `json:"displayName"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's include "default" so it's clear that this is override-able per port:

Suggested change
DefaultDisplayName string `json:"displayName"`
DefaultDisplayName string `json:"defaultDisplayName"`


// 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
Expand Down Expand Up @@ -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
Expand All @@ -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"}
Expand Down
39 changes: 34 additions & 5 deletions workspaces/controller/api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading