feat(ws): add spec.podTemplate.ports[] to WorkspaceKind #507
Conversation
google-oss-prow
bot
added
area/controller
google-oss-prow
bot
requested review from
andyatmiami,
kimwnasptd and
thesuperzapper
|
/ok-to-test |
workspaces/controller/config/crd/bases/kubeflow.org_workspacekinds.yaml
Outdated
Show resolved
Hide resolved
| // ports that the container listens on | ||
| // +kubebuilder:validation:Optional | ||
| HTTPProxy *HTTPProxy `json:"httpProxy,omitempty"` | ||
| Ports []WorkspaceKindPort `json:"ports,omitempty"` |
There was a problem hiding this comment.
Out of curiosity - what is the practical purpose of defining a WorkspaceKind with no Ports ? Why would someone want to do that ?
There was a problem hiding this comment.
This is a great question , we should rethink this 🤔
There was a problem hiding this comment.
We should require at least one port, just so that the frontend does not have to deal with the possibility of a workspace with no ports.
+kubebuilder:validation:MinItems:=1
workspaces/controller/internal/webhook/workspacekind_webhook.go
Outdated
Show resolved
Hide resolved
harshad16
force-pushed
the
port-in-wsk-podtemplate
branch
from
5170f6e to
438e485
Compare
harshad16
force-pushed
the
port-in-wsk-podtemplate
branch
from
438e485 to
3239dde
Compare
|
/ok-to-test |
|
/lgtm testing these changes on a cluster and was able to:
|
harshad16
force-pushed
the
port-in-wsk-podtemplate
branch
from
3239dde to
9532f2f
Compare
harshad16
force-pushed
the
port-in-wsk-podtemplate
branch
from
9532f2f to
c8382d8
Compare
|
/lgtm Testing Methodology
|
| return nil | ||
| } | ||
|
|
||
| services := make([]Service, len(imageConfigValue.Spec.Ports)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Made the changes accordingly
thanks
| // ports that the container listens on | ||
| // +kubebuilder:validation:Optional | ||
| HTTPProxy *HTTPProxy `json:"httpProxy,omitempty"` | ||
| Ports []WorkspaceKindPort `json:"ports,omitempty"` |
There was a problem hiding this comment.
We should require at least one port, just so that the frontend does not have to deal with the possibility of a workspace with no ports.
+kubebuilder:validation:MinItems:=1
workspaces/controller/internal/controller/workspace_controller.go
Outdated
Show resolved
Hide resolved
workspaces/controller/internal/webhook/workspacekind_webhook.go
Outdated
Show resolved
Hide resolved
workspaces/controller/internal/webhook/workspacekind_webhook.go
Outdated
Show resolved
Hide resolved
workspaces/controller/internal/webhook/workspacekind_webhook.go
Outdated
Show resolved
Hide resolved
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
harshad16
force-pushed
the
port-in-wsk-podtemplate
branch
3 times, most recently
from
5d88a27 to
30d1fac
Compare
There was a problem hiding this comment.
A couple (minor) last tweaks being suggested here. However, none of them are really functionality impacting - so I will note that I also tested this code against a kind cluster (making sure I updated both the controller AND backend deployments - and I can happily confirm:
- creating workspace kinds + workspaces through
kubectlworks - creating workspace kinds + workspaces through
frontendUI also work
Additionally confirmed common negative scenarios as well as ensuring "falling back" to the DefaultDisplayName is handled appropriately.
harshad16
force-pushed
the
port-in-wsk-podtemplate
branch
from
30d1fac to
689e7bc
Compare
- 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 <[email protected]>
harshad16
force-pushed
the
port-in-wsk-podtemplate
branch
from
689e7bc to
dffa3a7
Compare
|
/lgtm |
| var wskPodTemplatePorts []kubefloworgv1beta1.WorkspaceKindPort | ||
| if wskExists(wsk) { | ||
| wskPodTemplatePorts = wsk.Spec.PodTemplate.Ports | ||
| } |
There was a problem hiding this comment.
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.
| // http proxy configs (MUTABLE) | ||
| // +kubebuilder:validation:Optional | ||
| HTTPProxy *HTTPProxy `json:"httpProxy,omitempty"` | ||
| // ports that the container listens on |
There was a problem hiding this comment.
This is very important to document for users as it is used in kubectl explain to explain what this field is:
| // 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 |
| // PortId the id of the port | ||
| // |
There was a problem hiding this comment.
| // PortId the id of the port | |
| // | |
| // an id of a port |
| // +kubebuilder:example:="HTTP" | ||
| Protocol ImagePortProtocol `json:"protocol"` | ||
|
|
||
| // the display name of the port |
There was a problem hiding this comment.
| // 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"` |
There was a problem hiding this comment.
Let's include "default" so it's clear that this is override-able per port:
| DefaultDisplayName string `json:"displayName"` | |
| DefaultDisplayName string `json:"defaultDisplayName"` |
| ## - this only works if the application serves RELATIVE URLs for its assets | ||
| ## | ||
| removePathPrefix: false | ||
| ## port configs (MUTABLE) |
There was a problem hiding this comment.
| ## port configs (MUTABLE) | |
| ## port definitions which can be referenced in image config values (MUTABLE) | |
| ## - 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 | |
| ## |
|
|
||
| ## 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 |
There was a problem hiding this comment.
I think this part is explained by the above ports doc string:
| ## 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 |
| ## http proxy configs (MUTABLE) | ||
| ## only "HTTP" protocol ports are supported |
There was a problem hiding this comment.
| ## http proxy configs (MUTABLE) | |
| ## only "HTTP" protocol ports are supported | |
| ## http proxy configs (MUTABLE) | |
| ## only "HTTP" protocol ports are supported | |
| ## |
| #set: { "X-RStudio-Root-Path": "{{ .PathPrefix }}" } # for RStudio | ||
| #add: {} | ||
| #remove: [] |
There was a problem hiding this comment.
| #set: { "X-RStudio-Root-Path": "{{ .PathPrefix }}" } # for RStudio | |
| #add: {} | |
| #remove: [] | |
| #set: { "X-RStudio-Root-Path": "{{ .PathPrefix }}" } # for RStudio | |
| #add: {} | |
| #remove: [] |
| // 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")) | ||
| } |
There was a problem hiding this comment.
Let's try to make this so that a lint error will throw here once we add new port protocol enum values (e.g. ssh).
My thinking is to structure it as a switch over protocol, which under the HTTP case that is actually a no-op in this case, so it should just say something like // noop - when adding new protocols, disallow setting unrelated fields
related: #37
This PR adds
spec.podTemplate.ports[]to workspaceKind CRD, which lets users include ports httpproxy setting for their workspaces.WorkspaceKind CRD changes
Following changes are included:
These changes would be consider while setting the routing for proper traffic controller/routing to the pods.