Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 16 additions & 0 deletions workspaces/backend/api/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,19 @@ func (a *App) LocationGetWorkspaceKind(name string) string {
path := strings.Replace(WorkspaceKindsByNamePath, ":"+ResourceNamePathParam, name, 1)
return path
}

// GetBooleanQueryParameter reads a query parameter that only accepts "true"/"false".
// If the parameter is absent, returns (false, nil).
// On invalid value, writes a 400 and returns an error.
func (a *App) GetBooleanQueryParameter(w http.ResponseWriter, r *http.Request, paramName string) (bool, error) {
val := r.URL.Query().Get(paramName)
if val == "" {
return false, nil // default if not provided
}
if val != "true" && val != "false" {
err := fmt.Errorf("invalid query parameter '%s' value '%s'. Must be 'true' or 'false'", paramName, val)
a.badRequestResponse(w, r, err)
return false, err
}
return val == "true", nil
}
27 changes: 25 additions & 2 deletions workspaces/backend/api/workspacekinds_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,24 @@ func (a *App) GetWorkspaceKindsHandler(w http.ResponseWriter, r *http.Request, _
// @Router /workspacekinds [post]
func (a *App) CreateWorkspaceKindHandler(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {

// Parse dry_run query parameter using helper
isDryRun, err := a.GetBooleanQueryParameter(w, r, "dry_run")
if err != nil {
return
}
hasDryRun := r.URL.Query().Get("dry_run") != ""

// validate the Content-Type header
if success := a.ValidateContentType(w, r, MediaTypeYaml); !success {
if !a.ValidateContentType(w, r, MediaTypeYaml) {
if hasDryRun {
// Special case: dry_run provided with wrong media type → 400
a.badRequestResponse(w, r,
fmt.Errorf("dry_run is only supported when Content-Type: %s", MediaTypeYaml))
} else {
// Normal case: wrong media type → 415
a.unsupportedMediaTypeResponse(w, r,
fmt.Errorf("only %s is supported", MediaTypeYaml))
}
Comment on lines +165 to +174
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this implementation is specifically following the GH issue guidance... but now that I am re-reviewing things (and note: i wrote that issue!) - I can't for the life of me think why this would be desirable!

We shouldn't carve out a specific error message here for dry_run parameter... if the ValidateContentType check fails... we should just be doing

Suggested change
if !a.ValidateContentType(w, r, MediaTypeYaml) {
if hasDryRun {
// Special case: dry_run provided with wrong media type → 400
a.badRequestResponse(w, r,
fmt.Errorf("dry_run is only supported when Content-Type: %s", MediaTypeYaml))
} else {
// Normal case: wrong media type → 415
a.unsupportedMediaTypeResponse(w, r,
fmt.Errorf("only %s is supported", MediaTypeYaml))
}
if !a.ValidateContentType(w, r, MediaTypeYaml) {
a.unsupportedMediaTypeResponse(w, r,
fmt.Errorf("only %s is supported", MediaTypeYaml))

This also means we don't need this additional "look up":

hasDryRun := r.URL.Query().Get("dry_run") != ""

which is a nice added benefit - as the URL.Query.Get(...) code now can purely be contained within the helper function.

sorry for that poor initial guidance!

return
}

Expand Down Expand Up @@ -204,7 +220,7 @@ func (a *App) CreateWorkspaceKindHandler(w http.ResponseWriter, r *http.Request,
}
// ============================================================

createdWorkspaceKind, err := a.repositories.WorkspaceKind.Create(r.Context(), workspaceKind)
createdWorkspaceKind, err := a.repositories.WorkspaceKind.Create(r.Context(), workspaceKind, isDryRun)
if err != nil {
if errors.Is(err, repository.ErrWorkspaceKindAlreadyExists) {
a.conflictResponse(w, r, err)
Expand All @@ -219,6 +235,13 @@ func (a *App) CreateWorkspaceKindHandler(w http.ResponseWriter, r *http.Request,
return
}

// Return appropriate response based on dry-run
if isDryRun {
responseEnvelope := &WorkspaceKindEnvelope{Data: *createdWorkspaceKind}
a.dataResponse(w, r, responseEnvelope)
return
}

Comment on lines +238 to +244
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can do a little better job here combining L239-243 with L246-249

in both cases - the following invocation is the same:

responseEnvelope := &WorkspaceKindCreateEnvelope{Data: createdWorkspaceKind}
  • I notice you are passing a pointer in your newly added code (which is preferred style now!)

But i think refactoring this into an if/else (pullin gout the responseEnvelope declaration) might be a little clearer expressing intent

// calculate the GET location for the created workspace kind (for the Location header)
location := a.LocationGetWorkspaceKind(createdWorkspaceKind.Name)

Expand Down
215 changes: 215 additions & 0 deletions workspaces/backend/api/workspacekinds_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/validation/field"
Expand Down Expand Up @@ -510,4 +511,218 @@ metadata:
))
})
})

Context("when creating a WorkspaceKind (POST)", Serial, func() {
var newWorkspaceKindName string
var validYAML []byte

BeforeEach(func() {
newWorkspaceKindName = "wsk-create-test"

validYAML = []byte(fmt.Sprintf(`
apiVersion: kubeflow.org/v1beta1
kind: WorkspaceKind
metadata:
name: %s
spec:
spawner:
displayName: "JupyterLab Notebook"
description: "A Workspace which runs JupyterLab in a Pod"
icon:
url: "https://jupyter.org/assets/favicons/apple-touch-icon.png"
logo:
url: "https://jupyter.org/assets/logos/jupyter/jupyter.png"
podTemplate:
serviceAccount:
name: default-editor
volumeMounts:
home: "/home/jovyan"
options:
imageConfig:
default: "jupyterlab_scipy_180"
values:
- id: "jupyterlab_scipy_180"
displayName: "JupyterLab SciPy 1.8.0"
description: "JupyterLab with SciPy 1.8.0"
spec:
image: "jupyter/scipy-notebook:2024.1.0"
podConfig:
default: "tiny_cpu"
values:
- id: "tiny_cpu"
displayName: "Tiny CPU"
description: "1 CPU core, 2GB RAM"
spec:
resources:
requests:
cpu: "100m"
memory: "512Mi"
limits:
cpu: "1"
memory: "2Gi"
`, newWorkspaceKindName))
})

AfterEach(func() {
By("deleting the WorkspaceKind if it exists")
workspaceKind := &kubefloworgv1beta1.WorkspaceKind{
ObjectMeta: metav1.ObjectMeta{
Name: newWorkspaceKindName,
},
}
_ = k8sClient.Delete(ctx, workspaceKind)
})

It("should create a WorkspaceKind successfully without dry-run", func() {
req, err := http.NewRequest(http.MethodPost, AllWorkspaceKindsPath, bytes.NewReader(validYAML))
Expect(err).NotTo(HaveOccurred())
req.Header.Set("Content-Type", MediaTypeYaml)
req.Header.Set(userIdHeader, adminUser)

rr := httptest.NewRecorder()
a.CreateWorkspaceKindHandler(rr, req, httprouter.Params{})
rs := rr.Result()
defer rs.Body.Close()

Expect(rs.StatusCode).To(Equal(http.StatusCreated), descUnexpectedHTTPStatus, rr.Body.String())

location := rs.Header.Get("Location")
Expect(location).To(Equal(fmt.Sprintf("/api/v1/workspacekinds/%s", newWorkspaceKindName)))

body, err := io.ReadAll(rs.Body)
Expect(err).NotTo(HaveOccurred())

var response WorkspaceKindCreateEnvelope
err = json.Unmarshal(body, &response)
Expect(err).NotTo(HaveOccurred())

created := &kubefloworgv1beta1.WorkspaceKind{}
err = k8sClient.Get(ctx, types.NamespacedName{Name: newWorkspaceKindName}, created)
Expect(err).NotTo(HaveOccurred())

expected := models.NewWorkspaceKindModelFromWorkspaceKind(created)
Expect(response.Data).To(BeComparableTo(expected))
})

It("should validate WorkspaceKind with dry-run=true without persisting it", func() {
req, err := http.NewRequest(http.MethodPost, AllWorkspaceKindsPath+"?dry_run=true", bytes.NewReader(validYAML))
Expect(err).NotTo(HaveOccurred())
req.Header.Set("Content-Type", MediaTypeYaml)
req.Header.Set(userIdHeader, adminUser)

rr := httptest.NewRecorder()
a.CreateWorkspaceKindHandler(rr, req, httprouter.Params{})
rs := rr.Result()
defer rs.Body.Close()

Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String())

// Ensure NOT persisted
notCreated := &kubefloworgv1beta1.WorkspaceKind{}
err = k8sClient.Get(ctx, types.NamespacedName{Name: newWorkspaceKindName}, notCreated)
Expect(err).To(HaveOccurred())
Expect(apierrors.IsNotFound(err)).To(BeTrue())
})

It("should return 400 for invalid YAML", func() {
invalidYAML := []byte("invalid: yaml: :")

req, err := http.NewRequest(http.MethodPost, AllWorkspaceKindsPath, bytes.NewReader(invalidYAML))
Expect(err).NotTo(HaveOccurred())
req.Header.Set("Content-Type", MediaTypeYaml)
req.Header.Set(userIdHeader, adminUser)

rr := httptest.NewRecorder()
a.CreateWorkspaceKindHandler(rr, req, httprouter.Params{})
rs := rr.Result()
defer rs.Body.Close()

Expect(rs.StatusCode).To(Equal(http.StatusBadRequest), descUnexpectedHTTPStatus, rr.Body.String())
})

It("should return 415 for wrong content-type when dry-run not set", func() {
req, err := http.NewRequest(http.MethodPost, AllWorkspaceKindsPath, bytes.NewReader(validYAML))
Expect(err).NotTo(HaveOccurred())
req.Header.Set("Content-Type", MediaTypeJson)
req.Header.Set(userIdHeader, adminUser)

rr := httptest.NewRecorder()
a.CreateWorkspaceKindHandler(rr, req, httprouter.Params{})
rs := rr.Result()
defer rs.Body.Close()

Expect(rs.StatusCode).To(Equal(http.StatusUnsupportedMediaType), descUnexpectedHTTPStatus, rr.Body.String())
})

It("should return 400 for wrong content-type when dry-run is set", func() {
req, err := http.NewRequest(http.MethodPost, AllWorkspaceKindsPath+"?dry_run=true", bytes.NewReader(validYAML))
Expect(err).NotTo(HaveOccurred())
req.Header.Set("Content-Type", MediaTypeJson)
req.Header.Set(userIdHeader, adminUser)

rr := httptest.NewRecorder()
a.CreateWorkspaceKindHandler(rr, req, httprouter.Params{})
rs := rr.Result()
defer rs.Body.Close()

Expect(rs.StatusCode).To(Equal(http.StatusBadRequest), descUnexpectedHTTPStatus, rr.Body.String())
})

It("should return 400 for invalid dry-run value", func() {
req, err := http.NewRequest(http.MethodPost, AllWorkspaceKindsPath+"?dry_run=invalid", bytes.NewReader(validYAML))
Expect(err).NotTo(HaveOccurred())
req.Header.Set("Content-Type", MediaTypeYaml)
req.Header.Set(userIdHeader, adminUser)

rr := httptest.NewRecorder()
a.CreateWorkspaceKindHandler(rr, req, httprouter.Params{})
rs := rr.Result()
defer rs.Body.Close()

Expect(rs.StatusCode).To(Equal(http.StatusBadRequest), descUnexpectedHTTPStatus, rr.Body.String())
})

It("should surface Kubernetes validation errors when dry-run=true with invalid resource", func() {
invalidDryRunYAML := []byte(`
apiVersion: kubeflow.org/v1beta1
kind: WorkspaceKind
metadata:
name: INVALID_NAME # invalid DNS-1123 (uppercase not allowed)
spec:
spawner:
displayName: "Bad Test"
description: "Invalid name test"
`)

req, _ := http.NewRequest(http.MethodPost, AllWorkspaceKindsPath+"?dry_run=true", bytes.NewReader(invalidDryRunYAML))
req.Header.Set("Content-Type", MediaTypeYaml)
req.Header.Set(userIdHeader, adminUser)

rr := httptest.NewRecorder()
a.CreateWorkspaceKindHandler(rr, req, httprouter.Params{})
rs := rr.Result()
defer rs.Body.Close()

// Your handler maps apierrors.IsInvalid to 422
Expect(rs.StatusCode).To(Equal(http.StatusUnprocessableEntity), descUnexpectedHTTPStatus, rr.Body.String())

body, _ := io.ReadAll(rs.Body)

// Unmarshal to the envelope shape your backend uses
var env ErrorEnvelope
_ = json.Unmarshal(body, &env)

// Ensure we received an error envelope
Expect(env.Error).NotTo(BeNil(), "expected error envelope in response")

// Be lenient about exact schema of causes: assert the raw payload mentions metadata.name
Expect(strings.Contains(string(body), "metadata.name")).
To(BeTrue(), "expected validation details about metadata.name in error payload")

// Ensure object not persisted
notCreated := &kubefloworgv1beta1.WorkspaceKind{}
getErr := k8sClient.Get(ctx, types.NamespacedName{Name: "INVALID_NAME"}, notCreated)
Expect(apierrors.IsNotFound(getErr)).To(BeTrue())
})
})
})
10 changes: 8 additions & 2 deletions workspaces/backend/internal/repositories/workspacekinds/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"

models "github.com/kubeflow/notebooks/workspaces/backend/internal/models/workspacekinds"
Expand Down Expand Up @@ -70,9 +71,14 @@ func (r *WorkspaceKindRepository) GetWorkspaceKinds(ctx context.Context) ([]mode
return workspaceKindsModels, nil
}

func (r *WorkspaceKindRepository) Create(ctx context.Context, workspaceKind *kubefloworgv1beta1.WorkspaceKind) (*models.WorkspaceKind, error) {
func (r *WorkspaceKindRepository) Create(ctx context.Context, workspaceKind *kubefloworgv1beta1.WorkspaceKind, dryRun bool) (*models.WorkspaceKind, error) {
opts := &client.CreateOptions{}
if dryRun {
opts.DryRun = []string{metav1.DryRunAll} // server-side dry-run
}

// create workspace kind
if err := r.client.Create(ctx, workspaceKind); err != nil {
if err := r.client.Create(ctx, workspaceKind, opts); err != nil {
if apierrors.IsAlreadyExists(err) {
return nil, ErrWorkspaceKindAlreadyExists
}
Expand Down
Loading