feat(ws): add WorkspaceCreate model to backend #205

thesuperzapper · 2025-02-12T03:56:10Z

This PR does the following:

Creates a new WorkspaceCreate model which we use in the body of "create workspace" POST (and its return body)
Introduces a causes.validation_errors field for Error responses, which is used when an understood but invalid body is passed (HTTP 422)
Adds validation for the Content-Type header on POST (for now only affects the "create workspace", as this is the only POST).
Correctly returns the 409 status when trying to create a workspace that already exists
Correctly sets the Location header when successfully creating a new Workspace (to the GET path for that workspace)
Renames confusingly named envelopes like WorkspaceKindsEnvelope -> WorkspaceKindListEnvelope
Factors out a common variable for ResourceNamePathParam (for HTTP path parameter), rather than having "name" be defined as both WorkspaceNamePathParam and WorkspaceKindNamePathParam
Adds validation to namespace and name HTTP path parameters
- mostly fixes Notebooks 2.0 // Backend // Reject invalid requests #129
Unpacks validation errors from Kubernetes when we fail to create a Workspace because the webhook rejects it (see helper.StatusCausesFromAPIStatus and a.failedValidationResponse)
Logs the body in tests when we get an unexpected status code (see descUnexpectedHTTPStatus)

For future PRs:

Take the tests from feat(ws): reject invalid HTTP path params in backend #139 (after rebasing), and apply them now that this PR also verifies that namespace/name parameters are valid RCF 1123 subdomains.

Example POST

Here is an example post to create a new workspace, and its response:

curl -X POST http://localhost:4000/api/v1/workspaces/default \
    -H "Content-Type: application/json" \
    -d '{
    "data": {
        "name": "dora",
        "kind": "jupyterlab",
        "paused": false,
        "defer_updates": false,
        "pod_template": {
            "pod_metadata": {
                "labels": {
                    "app": "dora"
                },
                "annotations": {
                    "app": "dora"
                }
            },
            "volumes": {
                "home": "workspace-home-bella",
                "data": [
                    {
                        "pvc_name": "workspace-data-bella",
                        "mount_path": "/data/my-data",
                        "read_only": false
                    }
                ]
            },
            "options": {
                "image_config": "jupyterlab_scipy_190",
                "pod_config": "tiny_cpu"
            }
        }
    }
}'

RESPONSE:

{
        "data": {
                "name": "dora",
                "kind": "jupyterlab",
                "paused": false,
                "defer_updates": false,
                "pod_template": {
                        "pod_metadata": {
                                "labels": {
                                        "app": "dora"
                                },
                                "annotations": {
                                        "app": "dora"
                                }
                        },
                        "volumes": {
                                "home": "workspace-home-bella",
                                "data": [
                                        {
                                                "pvc_name": "workspace-data-bella",
                                                "mount_path": "/data/my-data"
                                        }
                                ]
                        },
                        "options": {
                                "image_config": "jupyterlab_scipy_190",
                                "pod_config": "tiny_cpu"
                        }
                }
        }
}

Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>

thesuperzapper · 2025-02-13T06:36:30Z

@ederign this should be ready to do a quick review of, it solves a lot of the outstanding tasks on the backend.

It also sets the example of how to do the other "create" and "update" APIs, especially around payload validation and unpacking the errors returned by our validating webhooks.

However, we will still need a follow-up to add more tests for the validation (both the invalid create body payloads, and namespace/name HTTP path parameters), perhaps @Yael-F can take this up after we merge, as they already did something similar in #139

ederign · 2025-02-14T16:18:57Z

workspaces/backend/api/logging.go

+import "net/http"
+
+// LogError logs an error message with the request details.
+func (a *App) LogError(r *http.Request, err error) {


It's a style choice, but maybe unify them under a single Log(level, message, error, r *http.Request)

It's a lot more common to have separate functions, especially because some of them don't have errors.

ederign

@thesuperzapper I just leave some optional comments here. I just would like to double check with you something.

When I've tested the API to Create, on the backend perspective worked well and I've received the right payload. (and error if I duplicate it)

I can see the workspace in the admissions logs of the controller:

2025-02-14T16:01:13Z    DEBUG    admission    validating Workspace create    {"webhookGroup": "kubeflow.org", "webhookKind": "Workspace", "Workspace": {"name":"dora","namespace":"default"}, "namespace": " │
│ 2025-02-14T16:01:31Z    DEBUG    admission    validating Workspace create    {"webhookGroup": "kubeflow.org", "webhookKind": "Workspace", "Workspace": {"name":"dora","namespace":"default"}, "namespace": " │
│ 2025-02-14T16:02:02Z    DEBUG    admission    validating Workspace create    {"webhookGroup": "kubeflow.org", "webhookKind": "Workspace", "Workspace": {"name":"dora1","namespace":"default"}, "namespace":  │
│ 2025-02-14T16:10:15Z    DEBUG    admission    validating Workspace create    {"webhookGroup": "kubeflow.org", "webhookKind": "Workspace", "Workspace": {"name":"dora2","namespace":"default"}, "namespace":  │
│
```

but I don't work see the workspace pod being created. Is this expected at this stage?

ederign · 2025-02-14T16:20:41Z

workspaces/backend/api/logging.go

+		method = r.Method
+		uri    = r.URL.RequestURI()
+	)
+	a.logger.Error(err.Error(), "method", method, "uri", uri)


One thing that we did on MR is to have tracing or correlation IDs on requests. When we do that, we should remember to log here also the r.context().

@alexcreasy I wonder if this would be something that you can do eventually for us? (include the debug/logs that you did on MR on Notebooks 2.0)

ederign · 2025-02-14T16:28:48Z

workspaces/backend/api/workspacekinds_handler.go

+	name := ps.ByName(ResourceNamePathParam)
+
+	// validate path parameters
+	var valErrs field.ErrorList


We are duplicating this validation in multiple places. I would suggest moving this to a middleware. @thesuperzapper I can do this refactoring in a FUP if you agree.

The code in my POV is becoming a little complicated with the pattern:

validate (I believe we can move this to middleware)

auth (I believe we can move this to middleware)

actual handler logic

Related to what Adem proposed #207

Similar to what I do here: https://github.com/kubeflow/model-registry/blob/main/clients/ui/bff/internal/api/app.go#L102-L113

Just let me know and I do this.

Regarding perimeter validation, I guess we could do it with a middleware, but let's do that in a follow-up PR to see what it might look like.

I worry that it might get a bit messy, because each of the apis has a different set of parameters.

I can send a small proposal in a PR as a FUP for this one.

Then we can discuss what is the best approach! Better to compare with code examples

ederign · 2025-02-14T16:30:48Z

workspaces/backend/api/workspaces_handler.go

 }

 func (a *App) GetWorkspacesHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
 	namespace := ps.ByName(NamespacePathParam)

+	// validate path parameters


same case as above

thesuperzapper · 2025-02-14T17:52:39Z

@thesuperzapper I just leave some optional comments here. I just would like to double check with you something.

When I've tested the API to Create, on the backend perspective worked well and I've received the right payload. (and error if I duplicate it)

I can see the workspace in the admissions logs of the controller:

2025-02-14T16:01:13Z    DEBUG    admission    validating Workspace create    {"webhookGroup": "kubeflow.org", "webhookKind": "Workspace", "Workspace": {"name":"dora","namespace":"default"}, "namespace": " │
│ 2025-02-14T16:01:31Z    DEBUG    admission    validating Workspace create    {"webhookGroup": "kubeflow.org", "webhookKind": "Workspace", "Workspace": {"name":"dora","namespace":"default"}, "namespace": " │
│ 2025-02-14T16:02:02Z    DEBUG    admission    validating Workspace create    {"webhookGroup": "kubeflow.org", "webhookKind": "Workspace", "Workspace": {"name":"dora1","namespace":"default"}, "namespace":  │
│ 2025-02-14T16:10:15Z    DEBUG    admission    validating Workspace create    {"webhookGroup": "kubeflow.org", "webhookKind": "Workspace", "Workspace": {"name":"dora2","namespace":"default"}, "namespace":  │
│
```

but I don't work see the workspace pod being created. Is this expected at this stage?

@ederign Were you actually running the controller, because it won't create the pod unless you are.

And if you were running the controller, what was the status of the workspace.

ederign · 2025-02-14T21:25:19Z

@thesuperzapper I was running the controller the error is not related with this PR:

State Message: Workspace StatefulSet has warning event: create Pod ws-dora1-w6kxv-0 in StatefulSet ws-dora1-w6kxv failed error: pods "ws-dora1-w6kxv-0" is forbidden: error looking up service account default/default-editor: serviceaccount "default-editor" not found

I'll setup everything correctly later and probably will work (because e2e tests works, to controller works).

ederign · 2025-02-14T21:25:45Z

/lgtm

thesuperzapper · 2025-02-14T22:24:21Z

/ok-to-test
/approve

google-oss-prow · 2025-02-14T22:24:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ederign, thesuperzapper

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [thesuperzapper]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

google-oss-prow bot requested a review from kimwnasptd February 12, 2025 03:56

google-oss-prow bot added the size/XL label Feb 12, 2025

thesuperzapper requested review from ederign and removed request for kimwnasptd February 12, 2025 03:56

feat(ws): add WorkspaceCreate model to backend

f39d6e7

Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>

thesuperzapper force-pushed the workspace-create-model branch from da20e77 to f39d6e7 Compare February 13, 2025 06:21

google-oss-prow bot added size/XXL and removed size/XL labels Feb 13, 2025

ederign reviewed Feb 14, 2025

View reviewed changes

google-oss-prow bot assigned ederign Feb 14, 2025

google-oss-prow bot added the lgtm label Feb 14, 2025

ederign approved these changes Feb 14, 2025

View reviewed changes

google-oss-prow bot added the ok-to-test label Feb 14, 2025

google-oss-prow bot added the approved label Feb 14, 2025

google-oss-prow bot merged commit e5d4e41 into kubeflow:notebooks-v2 Feb 14, 2025
20 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ws): add WorkspaceCreate model to backend #205

feat(ws): add WorkspaceCreate model to backend #205

thesuperzapper commented Feb 12, 2025 •

edited

Loading

thesuperzapper commented Feb 13, 2025

ederign Feb 14, 2025

thesuperzapper Feb 14, 2025

ederign left a comment

ederign Feb 14, 2025

ederign Feb 14, 2025

thesuperzapper Feb 14, 2025 •

edited

Loading

ederign Feb 14, 2025

ederign Feb 14, 2025

ederign Feb 14, 2025

thesuperzapper commented Feb 14, 2025

ederign commented Feb 14, 2025

ederign commented Feb 14, 2025

thesuperzapper commented Feb 14, 2025

google-oss-prow bot commented Feb 14, 2025

feat(ws): add WorkspaceCreate model to backend #205

feat(ws): add WorkspaceCreate model to backend #205

Conversation

thesuperzapper commented Feb 12, 2025 • edited Loading

For future PRs:

Example POST

thesuperzapper commented Feb 13, 2025

ederign Feb 14, 2025

Choose a reason for hiding this comment

thesuperzapper Feb 14, 2025

Choose a reason for hiding this comment

ederign left a comment

Choose a reason for hiding this comment

ederign Feb 14, 2025

Choose a reason for hiding this comment

ederign Feb 14, 2025

Choose a reason for hiding this comment

thesuperzapper Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

ederign Feb 14, 2025

Choose a reason for hiding this comment

ederign Feb 14, 2025

Choose a reason for hiding this comment

ederign Feb 14, 2025

Choose a reason for hiding this comment

thesuperzapper commented Feb 14, 2025

ederign commented Feb 14, 2025

ederign commented Feb 14, 2025

thesuperzapper commented Feb 14, 2025

google-oss-prow bot commented Feb 14, 2025

thesuperzapper commented Feb 12, 2025 •

edited

Loading

thesuperzapper Feb 14, 2025 •

edited

Loading