From f6ca24bdd2b0e8595fc143d0b8928fb4b834ba92 Mon Sep 17 00:00:00 2001 From: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com> Date: Tue, 27 Aug 2024 17:18:35 -0700 Subject: [PATCH] mathew refactor 2 Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com> --- workspaces/controller/cmd/main.go | 5 +- .../controller/workspace_controller_test.go | 2 +- .../workspacekind_controller_test.go | 2 +- .../controller/internal/helper/helper.go | 51 ++++ .../controller/internal/webhook/suite_test.go | 5 +- .../internal/webhook/workspace_webhook.go | 11 +- .../webhook/workspace_webhook_test.go | 137 ++++++++- .../internal/webhook/workspacekind_webhook.go | 265 +++++++++--------- .../webhook/workspacekind_webhook_test.go | 239 ++++++++++------ workspaces/controller/test/e2e/e2e_test.go | 55 +++- 10 files changed, 536 insertions(+), 236 deletions(-) diff --git a/workspaces/controller/cmd/main.go b/workspaces/controller/cmd/main.go index 82659c2f..eb2a26c7 100644 --- a/workspaces/controller/cmd/main.go +++ b/workspaces/controller/cmd/main.go @@ -21,9 +21,6 @@ import ( "flag" "os" - "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" - webhookInternal "github.com/kubeflow/notebooks/workspaces/controller/internal/webhook" - // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. _ "k8s.io/client-go/plugin/pkg/client/auth" @@ -39,6 +36,8 @@ import ( kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" controllerInternal "github.com/kubeflow/notebooks/workspaces/controller/internal/controller" + "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" + webhookInternal "github.com/kubeflow/notebooks/workspaces/controller/internal/webhook" //+kubebuilder:scaffold:imports ) diff --git a/workspaces/controller/internal/controller/workspace_controller_test.go b/workspaces/controller/internal/controller/workspace_controller_test.go index e28749b0..868c645e 100644 --- a/workspaces/controller/internal/controller/workspace_controller_test.go +++ b/workspaces/controller/internal/controller/workspace_controller_test.go @@ -44,7 +44,7 @@ var _ = Describe("Workspace Controller", func() { timeout = time.Second * 10 // how long to wait in "Consistently" blocks - duration = time.Second * 10 + duration = time.Second * 10 // nolint:unused // how frequently to poll for conditions interval = time.Millisecond * 250 diff --git a/workspaces/controller/internal/controller/workspacekind_controller_test.go b/workspaces/controller/internal/controller/workspacekind_controller_test.go index 59e7dd15..505a9dc8 100644 --- a/workspaces/controller/internal/controller/workspacekind_controller_test.go +++ b/workspaces/controller/internal/controller/workspacekind_controller_test.go @@ -40,7 +40,7 @@ var _ = Describe("WorkspaceKind Controller", func() { timeout = time.Second * 10 // how long to wait in "Consistently" blocks - duration = time.Second * 10 + duration = time.Second * 10 // nolint:unused // how frequently to poll for conditions interval = time.Millisecond * 250 diff --git a/workspaces/controller/internal/helper/helper.go b/workspaces/controller/internal/helper/helper.go index 486b1424..ca68176f 100644 --- a/workspaces/controller/internal/helper/helper.go +++ b/workspaces/controller/internal/helper/helper.go @@ -3,6 +3,9 @@ package helper import ( "reflect" + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "k8s.io/apimachinery/pkg/api/resource" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" ) @@ -98,3 +101,51 @@ func CopyServiceFields(desired *corev1.Service, target *corev1.Service) bool { return requireUpdate } + +// NormalizePodConfigSpec normalizes a PodConfigSpec so that it can be compared with reflect.DeepEqual +func NormalizePodConfigSpec(spec kubefloworgv1beta1.PodConfigSpec) error { + // Normalize Affinity + if spec.Affinity != nil && reflect.DeepEqual(spec.Affinity, corev1.Affinity{}) { + spec.Affinity = nil + } + + // Normalize NodeSelector + if spec.NodeSelector != nil && len(spec.NodeSelector) == 0 { + spec.NodeSelector = nil + } + + // Normalize Tolerations + if spec.Tolerations != nil && len(spec.Tolerations) == 0 { + spec.Tolerations = nil + } + + // Normalize Resources.Requests + if reflect.DeepEqual(spec.Resources.Requests, corev1.ResourceList{}) { + spec.Resources.Requests = nil + } + if spec.Resources.Requests != nil { + for key, value := range spec.Resources.Requests { + q, err := resource.ParseQuantity(value.String()) + if err != nil { + return err + } + spec.Resources.Requests[key] = q + } + } + + // Normalize Resources.Limits + if reflect.DeepEqual(spec.Resources.Limits, corev1.ResourceList{}) { + spec.Resources.Limits = nil + } + if spec.Resources.Limits != nil { + for key, value := range spec.Resources.Limits { + q, err := resource.ParseQuantity(value.String()) + if err != nil { + return err + } + spec.Resources.Limits[key] = q + } + } + + return nil +} diff --git a/workspaces/controller/internal/webhook/suite_test.go b/workspaces/controller/internal/webhook/suite_test.go index 1c480b13..579123c7 100644 --- a/workspaces/controller/internal/webhook/suite_test.go +++ b/workspaces/controller/internal/webhook/suite_test.go @@ -14,6 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ +// nolint:goconst package webhook import ( @@ -443,7 +444,7 @@ func NewExampleWorkspaceKindWithPodConfigCycle(name string) *kubefloworgv1beta1. } // NewExampleWorkspaceKindWithInvalidImageConfig returns a WorkspaceKind with an invalid redirect in the ImageConfig options. -func NewExampleWorkspaceKindWithInvalidImageConfig(name string) *kubefloworgv1beta1.WorkspaceKind { +func NewExampleWorkspaceKindWithInvalidImageConfigRedirect(name string) *kubefloworgv1beta1.WorkspaceKind { workspaceKind := NewExampleWorkspaceKind(name) workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &kubefloworgv1beta1.OptionRedirect{ To: "invalid_image_config", @@ -453,7 +454,7 @@ func NewExampleWorkspaceKindWithInvalidImageConfig(name string) *kubefloworgv1be } // NewExampleWorkspaceKindWithInvalidPodConfig returns a WorkspaceKind with an invalid redirect in the PodConfig options. -func NewExampleWorkspaceKindWithInvalidPodConfig(name string) *kubefloworgv1beta1.WorkspaceKind { +func NewExampleWorkspaceKindWithInvalidPodConfigRedirect(name string) *kubefloworgv1beta1.WorkspaceKind { workspaceKind := NewExampleWorkspaceKind(name) workspaceKind.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &kubefloworgv1beta1.OptionRedirect{ To: "invalid_pod_config", diff --git a/workspaces/controller/internal/webhook/workspace_webhook.go b/workspaces/controller/internal/webhook/workspace_webhook.go index 7e8cfe16..412d6ebd 100644 --- a/workspaces/controller/internal/webhook/workspace_webhook.go +++ b/workspaces/controller/internal/webhook/workspace_webhook.go @@ -20,7 +20,6 @@ import ( "context" "fmt" - kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -29,6 +28,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" ) // WorkspaceValidator validates a Workspace object @@ -54,13 +55,13 @@ func (v *WorkspaceValidator) ValidateCreate(ctx context.Context, obj runtime.Obj log := log.FromContext(ctx) log.V(1).Info("validating Workspace create") - var allErrs field.ErrorList - workspace, ok := obj.(*kubefloworgv1beta1.Workspace) if !ok { return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Workspace object but got %T", obj)) } + var allErrs field.ErrorList + // fetch the WorkspaceKind workspaceKind, err := v.validateWorkspaceKind(ctx, workspace) if err != nil { @@ -99,8 +100,6 @@ func (v *WorkspaceValidator) ValidateUpdate(ctx context.Context, oldObj, newObj log := log.FromContext(ctx) log.V(1).Info("validating Workspace update") - var allErrs field.ErrorList - newWorkspace, ok := newObj.(*kubefloworgv1beta1.Workspace) if !ok { return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Workspace object but got %T", newObj)) @@ -110,6 +109,8 @@ func (v *WorkspaceValidator) ValidateUpdate(ctx context.Context, oldObj, newObj return nil, apierrors.NewBadRequest(fmt.Sprintf("expected old object to be a Workspace but got %T", oldObj)) } + var allErrs field.ErrorList + // check if workspace kind related fields have changed var workspaceKindChange = false var imageConfigChange = false diff --git a/workspaces/controller/internal/webhook/workspace_webhook_test.go b/workspaces/controller/internal/webhook/workspace_webhook_test.go index 9136c3ea..e6efc213 100644 --- a/workspaces/controller/internal/webhook/workspace_webhook_test.go +++ b/workspaces/controller/internal/webhook/workspace_webhook_test.go @@ -19,25 +19,31 @@ package webhook import ( "fmt" - kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "k8s.io/apimachinery/pkg/types" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" ) var _ = Describe("Workspace Webhook", func() { - Context("When creating Workspace under Validating Webhook", Ordered, func() { + const ( + namespaceName = "default" + ) + + Context("When creating a Workspace", Ordered, func() { var ( workspaceName string workspaceKindName string - namespaceName string ) BeforeAll(func() { - uniqueName := "ws-create-test" + uniqueName := "ws-webhook-create-test" workspaceName = fmt.Sprintf("workspace-%s", uniqueName) - namespaceName = "default" workspaceKindName = fmt.Sprintf("workspacekind-%s", uniqueName) By("creating the WorkspaceKind") @@ -55,7 +61,7 @@ var _ = Describe("Workspace Webhook", func() { Expect(k8sClient.Delete(ctx, workspaceKind)).To(Succeed()) }) - It("should reject workspace creation with an invalid WorkspaceKind", func() { + It("should reject an invalid workspace kind", func() { invalidWorkspaceKindName := "invalid-workspace-kind" By("creating the Workspace") @@ -65,7 +71,29 @@ var _ = Describe("Workspace Webhook", func() { Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("workspace kind %q not found", invalidWorkspaceKindName))) }) - It("should successfully create workspace with a valid WorkspaceKind", func() { + It("should reject an invalid imageConfig", func() { + invalidImageConfig := "invalid_image_config" + + By("creating the Workspace") + workspace := NewExampleWorkspace(workspaceName, namespaceName, workspaceKindName) + workspace.Spec.PodTemplate.Options.ImageConfig = invalidImageConfig + err := k8sClient.Create(ctx, workspace) + Expect(err).NotTo(Succeed()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("imageConfig with id %q not found in workspace kind %q", invalidImageConfig, workspaceKindName))) + }) + + It("should reject an invalid podConfig", func() { + invalidPodConfig := "invalid_pod_config" + + By("creating the Workspace") + workspace := NewExampleWorkspace(workspaceName, namespaceName, workspaceKindName) + workspace.Spec.PodTemplate.Options.PodConfig = invalidPodConfig + err := k8sClient.Create(ctx, workspace) + Expect(err).NotTo(Succeed()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("podConfig with id %q not found in workspace kind %q", invalidPodConfig, workspaceKindName))) + }) + + It("should accept a valid workspace", func() { By("creating the Workspace") workspace := NewExampleWorkspace(workspaceName, namespaceName, workspaceKindName) Expect(k8sClient.Create(ctx, workspace)).To(Succeed()) @@ -75,4 +103,99 @@ var _ = Describe("Workspace Webhook", func() { }) }) + Context("When updating a Workspace", Ordered, func() { + var ( + workspaceName string + workspaceKindName string + workspaceKey types.NamespacedName + ) + + BeforeAll(func() { + uniqueName := "ws-webhook-update-test" + workspaceName = fmt.Sprintf("workspace-%s", uniqueName) + workspaceKindName = fmt.Sprintf("workspacekind-%s", uniqueName) + workspaceKey = types.NamespacedName{Name: workspaceName, Namespace: namespaceName} + + By("creating the WorkspaceKind") + workspaceKind := NewExampleWorkspaceKind(workspaceKindName) + Expect(k8sClient.Create(ctx, workspaceKind)).To(Succeed()) + + By("creating the Workspace") + workspace := NewExampleWorkspace(workspaceName, namespaceName, workspaceKindName) + Expect(k8sClient.Create(ctx, workspace)).To(Succeed()) + }) + + AfterAll(func() { + By("deleting the WorkspaceKind") + workspaceKind := &kubefloworgv1beta1.WorkspaceKind{ + ObjectMeta: metav1.ObjectMeta{ + Name: workspaceKindName, + }, + } + Expect(k8sClient.Delete(ctx, workspaceKind)).To(Succeed()) + + By("deleting the Workspace") + workspace := &kubefloworgv1beta1.Workspace{ + ObjectMeta: metav1.ObjectMeta{ + Name: workspaceName, + Namespace: namespaceName, + }, + } + Expect(k8sClient.Delete(ctx, workspace)).To(Succeed()) + }) + + It("should not allow updating immutable fields", func() { + By("getting the Workspace") + workspace := &kubefloworgv1beta1.Workspace{} + Expect(k8sClient.Get(ctx, workspaceKey, workspace)).To(Succeed()) + patch := client.MergeFrom(workspace.DeepCopy()) + + By("failing to update the `spec.kind` field") + newWorkspace := workspace.DeepCopy() + newWorkspace.Spec.Kind = "new_kind" + Expect(k8sClient.Patch(ctx, newWorkspace, patch)).NotTo(Succeed()) + }) + + It("should handle imageConfig updates", func() { + By("getting the Workspace") + workspace := &kubefloworgv1beta1.Workspace{} + Expect(k8sClient.Get(ctx, workspaceKey, workspace)).To(Succeed()) + patch := client.MergeFrom(workspace.DeepCopy()) + + By("failing to update the `spec.podTemplate.options.imageConfig` field to an invalid value") + invalidPodConfig := "invalid_image_config" + newWorkspace := workspace.DeepCopy() + newWorkspace.Spec.PodTemplate.Options.ImageConfig = invalidPodConfig + err := k8sClient.Patch(ctx, newWorkspace, patch) + Expect(err).NotTo(Succeed()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("imageConfig with id %q not found in workspace kind %q", invalidPodConfig, workspace.Spec.Kind))) + + By("updating the `spec.podTemplate.options.imageConfig` field to a valid value") + validImageConfig := "jupyterlab_scipy_190" + newWorkspace = workspace.DeepCopy() + newWorkspace.Spec.PodTemplate.Options.ImageConfig = validImageConfig + Expect(k8sClient.Patch(ctx, newWorkspace, patch)).To(Succeed()) + }) + + It("should handle podConfig updates", func() { + By("getting the Workspace") + workspace := &kubefloworgv1beta1.Workspace{} + Expect(k8sClient.Get(ctx, workspaceKey, workspace)).To(Succeed()) + patch := client.MergeFrom(workspace.DeepCopy()) + + By("failing to update the `spec.podTemplate.options.podConfig` field to an invalid value") + invalidPodConfig := "invalid_pod_config" + newWorkspace := workspace.DeepCopy() + newWorkspace.Spec.PodTemplate.Options.PodConfig = invalidPodConfig + err := k8sClient.Patch(ctx, newWorkspace, patch) + Expect(err).NotTo(Succeed()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("podConfig with id %q not found in workspace kind %q", invalidPodConfig, workspace.Spec.Kind))) + + By("updating the `spec.podTemplate.options.podConfig` field to a valid value") + validPodConfig := "small_cpu" + newWorkspace = workspace.DeepCopy() + newWorkspace.Spec.PodTemplate.Options.PodConfig = validPodConfig + Expect(k8sClient.Patch(ctx, newWorkspace, patch)).To(Succeed()) + }) + }) }) diff --git a/workspaces/controller/internal/webhook/workspacekind_webhook.go b/workspaces/controller/internal/webhook/workspacekind_webhook.go index bee30a61..e84fc1b6 100644 --- a/workspaces/controller/internal/webhook/workspacekind_webhook.go +++ b/workspaces/controller/internal/webhook/workspacekind_webhook.go @@ -21,13 +21,9 @@ import ( "errors" "fmt" "reflect" + "sync" - kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" - "github.com/kubeflow/notebooks/workspaces/controller/internal/controller" - "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" - corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -37,6 +33,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "github.com/kubeflow/notebooks/workspaces/controller/internal/controller" + "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" ) // WorkspaceKindValidator validates a Workspace object @@ -62,13 +62,13 @@ func (v *WorkspaceKindValidator) ValidateCreate(ctx context.Context, obj runtime log := log.FromContext(ctx) log.V(1).Info("validating WorkspaceKind create") - var allErrs field.ErrorList - workspaceKind, ok := obj.(*kubefloworgv1beta1.WorkspaceKind) if !ok { return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a WorkspaceKind object but got %T", obj)) } + var allErrs field.ErrorList + // validate the extra environment variables allErrs = append(allErrs, validateExtraEnv(workspaceKind)...) @@ -125,22 +125,20 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new log := log.FromContext(ctx) log.V(1).Info("validating WorkspaceKind update") - var allErrs field.ErrorList - newWorkspaceKind, ok := newObj.(*kubefloworgv1beta1.WorkspaceKind) if !ok { return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a WorkspaceKind object but got %T", newObj)) } oldWorkspaceKind, ok := oldObj.(*kubefloworgv1beta1.WorkspaceKind) if !ok { - return nil, apierrors.NewBadRequest(fmt.Sprintf("expected old object to be a WorkspaceKind but got %T", oldObj)) + return nil, apierrors.NewInternalError(fmt.Errorf("old object is not a WorkspaceKind, but a %T", oldObj)) } - // get usage count for imageConfig and podConfig values - imageConfigUsageCount, podConfigUsageCount, err := v.getOptionsUsageCounts(ctx, oldWorkspaceKind) - if err != nil { - return nil, err - } + var allErrs field.ErrorList + + // get functions to lazily fetch usage counts for imageConfig and podConfig values + // NOTE: the cluster is only queried when either function is called for the first time + getImageConfigUsageCount, getPodConfigUsageCount := v.getLazyOptionUsageCountFuncs(ctx, oldWorkspaceKind) // validate the extra environment variables if !reflect.DeepEqual(newWorkspaceKind.Spec.PodTemplate.ExtraEnv, oldWorkspaceKind.Spec.PodTemplate.ExtraEnv) { @@ -170,12 +168,13 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new toValidateImageConfigIds[imageConfigValue.Id] = true // 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 { - // check if this imageConfig value is used by any workspaces - var usageCount int32 - if usageCount, exists = imageConfigUsageCount[imageConfigValue.Id]; !exists { - return nil, apierrors.NewInternalError(fmt.Errorf("usage count not found for imageConfig value %q", imageConfigValue.Id)) + // if we haven't already decided to validate the imageConfig redirects, + // check if the redirect has changed + if !shouldValidateImageConfigRedirects && !reflect.DeepEqual(oldImageConfigIdMap[imageConfigValue.Id].Redirect, imageConfigValue.Redirect) { + shouldValidateImageConfigRedirects = true } // check if the spec has changed @@ -183,28 +182,30 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new // we need to validate this imageConfig value since it has changed toValidateImageConfigIds[imageConfigValue.Id] = true + // check how many workspaces are using this imageConfig value + usageCount, err := getImageConfigUsageCount(imageConfigValue.Id) + if err != nil { + // if the usage count is not found, we cannot validate the WorkspaceKind further + return nil, apierrors.NewInternalError(fmt.Errorf("failed to get usage count for imageConfig with id %q: %w", imageConfigValue.Id, err)) + } + // if this imageConfig is used by any workspaces, mark this imageConfig as bad, - // (the spec is immutable while in use) + // the spec is immutable while in use if usageCount > 0 { badChangedImageConfigIds[imageConfigValue.Id] = true } } - - // if we haven't already decided to validate the imageConfig redirects, - // check if the redirect has changed - if !shouldValidateImageConfigRedirects && !reflect.DeepEqual(oldImageConfigIdMap[imageConfigValue.Id].Redirect, imageConfigValue.Redirect) { - shouldValidateImageConfigRedirects = true - } } } for id := range oldImageConfigIdMap { // check if this imageConfig value was removed if _, exists := newImageConfigIdMap[id]; !exists { - // check if this imageConfig value is used by any workspaces - var usageCount int32 - if usageCount, exists = imageConfigUsageCount[id]; !exists { - return nil, apierrors.NewInternalError(fmt.Errorf("usage count not found for imageConfig value %q", id)) + // check how many workspaces are using this imageConfig value + usageCount, err := getImageConfigUsageCount(id) + if err != nil { + // if the usage count is not found, we cannot validate the WorkspaceKind further + return nil, apierrors.NewInternalError(fmt.Errorf("failed to get usage count for imageConfig with id %q: %w", id, err)) } // if this imageConfig is used by any workspaces, mark this imageConfig as bad, @@ -214,6 +215,7 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new } // we always need to validate the imageConfig redirects if an imageConfig was removed + // because an existing redirect could be pointing to the removed imageConfig value shouldValidateImageConfigRedirects = true } } @@ -237,36 +239,50 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new // check if the podConfig value is new if _, exists := oldPodConfigIdMap[podConfigValue.Id]; !exists { // we always need to validate the podConfig redirects if a podConfig was added + // because the new podConfig value could be used by a redirect or cause a cycle shouldValidatePodConfigRedirects = true } else { - // check if this podConfig value is used by any workspaces - var usageCount int32 - if usageCount, exists = podConfigUsageCount[podConfigValue.Id]; !exists { - return nil, apierrors.NewInternalError(fmt.Errorf("usage count not found for podConfig value %q", podConfigValue.Id)) + // if we haven't already decided to validate the podConfig redirects, + // check if the redirect has changed + if !shouldValidatePodConfigRedirects && !reflect.DeepEqual(oldPodConfigIdMap[podConfigValue.Id].Redirect, podConfigValue.Redirect) { + shouldValidatePodConfigRedirects = true } - // normalize the podConfig specs - oldPodConfigSpec := oldPodConfigIdMap[podConfigValue.Id].Spec - err := normalizePodConfigSpec(oldPodConfigSpec) + // we must normalize the podConfig specs so that we can compare them + newPodConfigSpec := podConfigValue.Spec + err := helper.NormalizePodConfigSpec(newPodConfigSpec) if err != nil { - return nil, apierrors.NewInternalError(fmt.Errorf("failed to normalize podConfig spec: %w", err)) + podConfigValueSpecPath := field.NewPath("spec", "podTemplate", "options", "podConfig", "values").Key(podConfigValue.Id).Child("spec") + allErrs = append(allErrs, field.InternalError(podConfigValueSpecPath, fmt.Errorf("failed to normalize podConfig spec: %w", err))) + + // if the spec could not be normalized, we cannot validate the WorkspaceKind further + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: kubefloworgv1beta1.GroupVersion.Group, Kind: "WorkspaceKind"}, + newWorkspaceKind.Name, + allErrs, + ) } - newPodConfigSpec := podConfigValue.Spec - err = normalizePodConfigSpec(newPodConfigSpec) + oldPodConfigSpec := oldPodConfigIdMap[podConfigValue.Id].Spec + err = helper.NormalizePodConfigSpec(oldPodConfigSpec) if err != nil { - return nil, apierrors.NewInternalError(fmt.Errorf("failed to normalize podConfig spec: %w", err)) + // this should never happen, as it would indicate that the old podConfig spec is invalid + return nil, apierrors.NewInternalError(fmt.Errorf("old podConfig spec of %q could not be normalized: %w", podConfigValue.Id, err)) } - // if this podConfig is used by any workspaces, check if the spec has changed - // if the spec has changed, mark this podConfig as bad (the spec is immutable while in use) - if usageCount > 0 && !reflect.DeepEqual(oldPodConfigSpec, newPodConfigSpec) { - badChangedPodConfigIds[podConfigValue.Id] = true - } + // check if the spec has changed + if !reflect.DeepEqual(oldPodConfigSpec, newPodConfigSpec) { + // check how many workspaces are using this podConfig value + usageCount, err := getPodConfigUsageCount(podConfigValue.Id) + if err != nil { + // if the usage count is not found, we cannot validate the WorkspaceKind further + return nil, apierrors.NewInternalError(fmt.Errorf("failed to get usage count for podConfig with id %q: %w", podConfigValue.Id, err)) + } - // if we haven't already decided to validate the podConfig redirects, - // check if the redirect has changed - if !shouldValidatePodConfigRedirects && !reflect.DeepEqual(oldPodConfigIdMap[podConfigValue.Id].Redirect, podConfigValue.Redirect) { - shouldValidatePodConfigRedirects = true + // if this podConfig is used by any workspaces, mark this podConfig as bad, + // the spec is immutable while in use + if usageCount > 0 { + badChangedPodConfigIds[podConfigValue.Id] = true + } } } } @@ -274,10 +290,11 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new // check if this podConfig value was removed if _, exists := newPodConfigIdMap[id]; !exists { - // check if this podConfig value is used by any workspaces - var usageCount int32 - if usageCount, exists = podConfigUsageCount[id]; !exists { - return nil, apierrors.NewInternalError(fmt.Errorf("usage count not found for podConfig value %q", id)) + // check how many workspaces are using this podConfig value + usageCount, err := getPodConfigUsageCount(id) + if err != nil { + // if the usage count is not found, we cannot validate the WorkspaceKind further + return nil, apierrors.NewInternalError(fmt.Errorf("failed to get usage count for podConfig with id %q: %w", id, err)) } // if this podConfig is used by any workspaces, mark this podConfig as bad, @@ -287,17 +304,16 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new } // we always need to validate the podConfig redirects if a podConfig was removed + // because an existing redirect could be pointing to the removed imageConfig value shouldValidatePodConfigRedirects = true } } // validate default options - if oldWorkspaceKind.Spec.PodTemplate.Options.ImageConfig.Spawner.Default != newWorkspaceKind.Spec.PodTemplate.Options.ImageConfig.Spawner.Default { - allErrs = append(allErrs, validateDefaultImageConfig(newWorkspaceKind, newImageConfigIdMap)...) - } - if oldWorkspaceKind.Spec.PodTemplate.Options.PodConfig.Spawner.Default != newWorkspaceKind.Spec.PodTemplate.Options.PodConfig.Spawner.Default { - allErrs = append(allErrs, validateDefaultPodConfig(newWorkspaceKind, newPodConfigIdMap)...) - } + // NOTE: we always check this because it's cheap, and otherwise we would need to keep track of if + // any options were changed or removed + allErrs = append(allErrs, validateDefaultImageConfig(newWorkspaceKind, newImageConfigIdMap)...) + allErrs = append(allErrs, validateDefaultPodConfig(newWorkspaceKind, newPodConfigIdMap)...) // validate imageConfig values // NOTE: we only need to validate new or changed imageConfig values @@ -307,7 +323,7 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new allErrs = append(allErrs, validateImageConfigValue(&imageConfigValue, imageConfigValuePath)...) } - // validate bad imageConfig values + // process bad imageConfig values for id := range badChangedImageConfigIds { imageConfigValuePath := field.NewPath("spec", "podTemplate", "options", "imageConfig", "values").Key(id) allErrs = append(allErrs, field.Invalid(imageConfigValuePath, id, fmt.Sprintf("imageConfig value %q is in use and cannot be changed", id))) @@ -317,7 +333,7 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new allErrs = append(allErrs, field.Invalid(imageConfigValuePath, id, fmt.Sprintf("imageConfig value %q is in use and cannot be removed", id))) } - // validate bad podConfig values + // process bad podConfig values for id := range badChangedPodConfigIds { podConfigValuePath := field.NewPath("spec", "podTemplate", "options", "podConfig", "values").Key(id) allErrs = append(allErrs, field.Invalid(podConfigValuePath, id, fmt.Sprintf("podConfig value %q is in use and cannot be changed", id))) @@ -382,26 +398,63 @@ func (v *WorkspaceKindValidator) ValidateDelete(ctx context.Context, obj runtime return nil, nil } -// getOptionsUsageCounts returns the usage counts for each imageConfig and podConfig value -func (v *WorkspaceKindValidator) getOptionsUsageCounts(ctx context.Context, workspaceKind *kubefloworgv1beta1.WorkspaceKind) (map[string]int32, map[string]int32, *apierrors.StatusError) { - podConfigUsageCount := make(map[string]int32) - imageConfigUsageCount := make(map[string]int32) +// getLazyOptionUsageCountFuncs returns functions that get usage counts for imageConfig and podConfig values in a WorkspaceKind +// the cluster is only queried when either function is called for the first time +func (v *WorkspaceKindValidator) getLazyOptionUsageCountFuncs(ctx context.Context, workspaceKind *kubefloworgv1beta1.WorkspaceKind) (func(string) (int32, error), func(string) (int32, error)) { + + // usageCountWrapper is a wrapper for the usage count maps + // NOTE: this is needed because sync.OnceValues can only return 2 values + type usageCountWrapper struct { + imageConfigUsageCounts map[string]int32 + podConfigUsageCounts map[string]int32 + } + + // this function will lazily fetch the usage counts for each option + lazyGetOptionsUsageCounts := sync.OnceValues(func() (usageCountWrapper, error) { + imageConfigUsageCounts, podConfigUsageCounts, err := v.getOptionsUsageCounts(ctx, workspaceKind) + if err != nil { + return usageCountWrapper{}, err + } + return usageCountWrapper{ + imageConfigUsageCounts: imageConfigUsageCounts, + podConfigUsageCounts: podConfigUsageCounts, + }, nil + }) + + // getImageConfigUsageCount returns the usage count for an imageConfig value + getImageConfigUsageCount := func(id string) (int32, error) { + wrapper, err := lazyGetOptionsUsageCounts() + if err != nil { + return 0, err + } + if count, exists := wrapper.imageConfigUsageCounts[id]; !exists { + return 0, errors.New("unknown imageConfig id") + } else { + return count, nil + } + } - // if possible, we get the counts from the status of the WorkspaceKind. these counts are updated by the - // controller so could be stale if the controller is not running or a workspace was very recently added or removed. - // however, since the controller gracefully handles cases of a Workspace referencing a non-existent imageConfig - // or podConfig value, we can safely use these counts to validate the WorkspaceKind, Workspaces will simply be - // put into an error state and the user can correct the issue. these counts will NOT be set in the WorkspaceKind - // unit tests, so we implement a fallback method to count the Workspaces that are using each option. - if len(workspaceKind.Status.PodTemplateOptions.ImageConfig) > 0 && len(workspaceKind.Status.PodTemplateOptions.PodConfig) > 0 { - for _, imageConfigMetrics := range workspaceKind.Status.PodTemplateOptions.ImageConfig { - imageConfigUsageCount[imageConfigMetrics.Id] = imageConfigMetrics.Workspaces + // getPodConfigUsageCount returns the usage count for a podConfig value + getPodConfigUsageCount := func(id string) (int32, error) { + wrapper, err := lazyGetOptionsUsageCounts() + if err != nil { + return 0, err } - for _, podConfigMetrics := range workspaceKind.Status.PodTemplateOptions.PodConfig { - podConfigUsageCount[podConfigMetrics.Id] = podConfigMetrics.Workspaces + if count, exists := wrapper.podConfigUsageCounts[id]; !exists { + return 0, errors.New("unknown podConfig id") + } else { + return count, nil } } + return getImageConfigUsageCount, getPodConfigUsageCount +} + +// getOptionsUsageCounts returns the usage counts for each imageConfig and podConfig value +func (v *WorkspaceKindValidator) getOptionsUsageCounts(ctx context.Context, workspaceKind *kubefloworgv1beta1.WorkspaceKind) (map[string]int32, map[string]int32, error) { + imageConfigUsageCount := make(map[string]int32) + podConfigUsageCount := make(map[string]int32) + // fetch all Workspaces that are using this WorkspaceKind workspaces := &kubefloworgv1beta1.WorkspaceList{} listOpts := &client.ListOptions{ @@ -409,7 +462,7 @@ func (v *WorkspaceKindValidator) getOptionsUsageCounts(ctx context.Context, work Namespace: "", // fetch Workspaces in all namespaces } if err := v.List(ctx, workspaces, listOpts); err != nil { - return nil, nil, apierrors.NewInternalError(err) + return nil, nil, err } // count the number of Workspaces using each option @@ -508,14 +561,14 @@ func validateImageConfigRedirects(imageConfigIdMap map[string]kubefloworgv1beta1 // check if there is a cycle involving the current node if cycle := helper.DetectGraphCycle(id, checkedNodes, imageConfigRedirectMap); cycle != nil { redirectToPath := field.NewPath("spec", "podTemplate", "options", "imageConfig", "values").Key(id).Child("redirect", "to") - errs = append(errs, field.Invalid(redirectToPath, redirectTo, fmt.Sprintf("cycle detected: %v", cycle))) + errs = append(errs, field.Invalid(redirectToPath, redirectTo, fmt.Sprintf("imageConfig redirect cycle detected: %v", cycle))) break // stop checking redirects if a cycle is detected } // ensure the target of the redirect exists if _, exists := imageConfigIdMap[redirectTo]; !exists { redirectToPath := field.NewPath("spec", "podTemplate", "options", "imageConfig", "values").Key(id).Child("redirect", "to") - errs = append(errs, field.Invalid(redirectToPath, redirectTo, fmt.Sprintf("invalid redirect target %q", redirectTo))) + errs = append(errs, field.Invalid(redirectToPath, redirectTo, fmt.Sprintf("target imageConfig %q does not exist", redirectTo))) } } @@ -532,64 +585,16 @@ func validatePodConfigRedirects(podConfigIdMap map[string]kubefloworgv1beta1.Pod // check if there is a cycle involving the current node if cycle := helper.DetectGraphCycle(id, checkedNodes, podConfigRedirectMap); cycle != nil { redirectToPath := field.NewPath("spec", "podTemplate", "options", "podConfig", "values").Key(id).Child("redirect", "to") - errs = append(errs, field.Invalid(redirectToPath, redirectTo, fmt.Sprintf("cycle detected: %v", cycle))) + errs = append(errs, field.Invalid(redirectToPath, redirectTo, fmt.Sprintf("podConfig redirect cycle detected: %v", cycle))) break // stop checking redirects if a cycle is detected } // ensure the target of the redirect exists if _, exists := podConfigIdMap[redirectTo]; !exists { redirectToPath := field.NewPath("spec", "podTemplate", "options", "podConfig", "values").Key(id).Child("redirect", "to") - errs = append(errs, field.Invalid(redirectToPath, redirectTo, fmt.Sprintf("invalid redirect target %q", redirectTo))) + errs = append(errs, field.Invalid(redirectToPath, redirectTo, fmt.Sprintf("target podConfig %q does not exist", redirectTo))) } } return errs } - -// normalizePodConfigSpec normalizes a PodConfigSpec so that it can be compared with reflect.DeepEqual -func normalizePodConfigSpec(spec kubefloworgv1beta1.PodConfigSpec) (err error) { - // Normalize Affinity - if spec.Affinity != nil && reflect.DeepEqual(spec.Affinity, corev1.Affinity{}) { - spec.Affinity = nil - } - - // Normalize NodeSelector - if spec.NodeSelector != nil && len(spec.NodeSelector) == 0 { - spec.NodeSelector = nil - } - - // Normalize Tolerations - if spec.Tolerations != nil && len(spec.Tolerations) == 0 { - spec.Tolerations = nil - } - - // Normalize Resources.Requests - if reflect.DeepEqual(spec.Resources.Requests, corev1.ResourceList{}) { - spec.Resources.Requests = nil - } - if spec.Resources.Requests != nil { - for key, value := range spec.Resources.Requests { - q, err := resource.ParseQuantity(value.String()) - if err != nil { - return err - } - spec.Resources.Requests[key] = q - } - } - - // Normalize Resources.Limits - if reflect.DeepEqual(spec.Resources.Limits, corev1.ResourceList{}) { - spec.Resources.Limits = nil - } - if spec.Resources.Limits != nil { - for key, value := range spec.Resources.Limits { - q, err := resource.ParseQuantity(value.String()) - if err != nil { - return err - } - spec.Resources.Limits[key] = q - } - } - - return nil -} diff --git a/workspaces/controller/internal/webhook/workspacekind_webhook_test.go b/workspaces/controller/internal/webhook/workspacekind_webhook_test.go index 9ef7f968..1a13a94f 100644 --- a/workspaces/controller/internal/webhook/workspacekind_webhook_test.go +++ b/workspaces/controller/internal/webhook/workspacekind_webhook_test.go @@ -18,78 +18,74 @@ package webhook import ( "fmt" - "time" - "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" + + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" ) var _ = Describe("WorkspaceKind Webhook", func() { const ( namespaceName = "default" - - // how long to wait in "Eventually" blocks - timeout = time.Second * 10 - - // how long to wait in "Consistently" blocks - duration = time.Second * 10 - - // how frequently to poll for conditions - interval = time.Millisecond * 250 ) - Context("When creating WorkspaceKind under Validating Webhook", Ordered, func() { + Context("When creating a WorkspaceKind", Ordered, func() { testCases := []struct { description string - workspaceKind *v1beta1.WorkspaceKind + workspaceKind *kubefloworgv1beta1.WorkspaceKind shouldSucceed bool }{ { - description: "should reject WorkspaceKind creation with cycles in ImageConfig options", + description: "should accept creation of a valid WorkspaceKind", + workspaceKind: NewExampleWorkspaceKind("wsk-webhook-create-test"), + shouldSucceed: true, + }, + { + description: "should reject creation with cycle in imageConfig redirects", workspaceKind: NewExampleWorkspaceKindWithImageConfigCycle("wsk-webhook-image-config-cycle-test"), shouldSucceed: false, }, { - description: "should reject WorkspaceKind creation with cycles in PodConfig options", + description: "should reject creation with cycle in podConfig redirects", workspaceKind: NewExampleWorkspaceKindWithPodConfigCycle("wsk-webhook-pod-config-cycle-test"), shouldSucceed: false, }, { - description: "should reject WorkspaceKind creation with invalid redirects in ImageConfig options", - workspaceKind: NewExampleWorkspaceKindWithInvalidImageConfig("wsk-webhook-image-config-invalid-test"), + description: "should reject creation with invalid redirect target in imageConfig options", + workspaceKind: NewExampleWorkspaceKindWithInvalidImageConfigRedirect("wsk-webhook-image-config-invalid-redirect-test"), shouldSucceed: false, }, { - description: "should reject WorkspaceKind creation with invalid redirects in PodConfig options", - workspaceKind: NewExampleWorkspaceKindWithInvalidPodConfig("wsk-webhook-pod-config-invalid-test"), + description: "should reject creation with invalid redirect target in podConfig options", + workspaceKind: NewExampleWorkspaceKindWithInvalidPodConfigRedirect("wsk-webhook-pod-config-invalid-redirect-test"), shouldSucceed: false, }, { - description: "should reject WorkspaceKind creation if the default ImageConfig option is missing", + description: "should reject creation with invalid default imageConfig", workspaceKind: NewExampleWorkspaceKindWithInvalidDefaultImageConfig("wsk-webhook-image-config-default-test"), shouldSucceed: false, }, { - description: "should reject WorkspaceKind creation if the default PodConfig option is missing", + description: "should reject creation with invalid default podConfig", workspaceKind: NewExampleWorkspaceKindWithInvalidDefaultPodConfig("wsk-webhook-pod-config-default-test"), shouldSucceed: false, }, { - description: "should reject WorkspaceKind creation with non-unique ports in PodConfig", - workspaceKind: NewExampleWorkspaceKindWithDuplicatePorts("wsk-webhook-ports-port-not-unique-test"), + description: "should reject creation with duplicate ports in imageConfig", + workspaceKind: NewExampleWorkspaceKindWithDuplicatePorts("wsk-webhook-image-config-duplicate-ports-test"), shouldSucceed: false, }, { - description: "should reject WorkspaceKind creation if extraEnv[].value is not a valid Go template", - workspaceKind: NewExampleWorkspaceKindWithInvalidExtraEnvValue("wsk-webhook-extra-env-value-invalid-test"), + description: "should reject creation if extraEnv[].value is not a valid Go template", + workspaceKind: NewExampleWorkspaceKindWithInvalidExtraEnvValue("wsk-webhook-invalid-extra-env-value-test"), shouldSucceed: false, }, } @@ -97,22 +93,25 @@ var _ = Describe("WorkspaceKind Webhook", func() { for _, tc := range testCases { tc := tc // Create a new instance of tc to avoid capturing the loop variable. It(tc.description, func() { - By("creating the WorkspaceKind") if tc.shouldSucceed { + By("creating the WorkspaceKind") Expect(k8sClient.Create(ctx, tc.workspaceKind)).To(Succeed()) + + By("deleting the WorkspaceKind") + Expect(k8sClient.Delete(ctx, tc.workspaceKind)).To(Succeed()) } else { + By("creating the WorkspaceKind") Expect(k8sClient.Create(ctx, tc.workspaceKind)).NotTo(Succeed()) } }) } - }) - Context("When updating WorkspaceKind under Validating Webhook", Ordered, func() { + Context("When updating a WorkspaceKind", Ordered, func() { var ( workspaceKindName string workspaceKindKey types.NamespacedName - workspaceKind *v1beta1.WorkspaceKind + workspaceKind *kubefloworgv1beta1.WorkspaceKind ) BeforeAll(func() { @@ -125,10 +124,8 @@ var _ = Describe("WorkspaceKind Webhook", func() { Expect(k8sClient.Create(ctx, createdWorkspaceKind)).To(Succeed()) By("getting the created WorkspaceKind") - workspaceKind = &v1beta1.WorkspaceKind{} - Eventually(func() error { - return k8sClient.Get(ctx, workspaceKindKey, workspaceKind) - }, timeout, interval).Should(Succeed()) + workspaceKind = &kubefloworgv1beta1.WorkspaceKind{} + Expect(k8sClient.Get(ctx, workspaceKindKey, workspaceKind)).To(Succeed()) }) AfterAll(func() { @@ -137,125 +134,201 @@ var _ = Describe("WorkspaceKind Webhook", func() { }) testCases := []struct { - description string - modifyKindFn func(*v1beta1.WorkspaceKind) - workspaceName *string + description string + // modifyKindFn is a function that modifies the WorkspaceKind in some way + // and returns a string matcher for the expected error message (if any) + modifyKindFn func(*kubefloworgv1beta1.WorkspaceKind) string + workspaceName string + + // if shouldSucceed is true, the test is expected to succeed shouldSucceed bool }{ { - description: "should reject updates to used imageConfig spec", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { + description: "should reject updates to in-use imageConfig spec", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { wsk.Spec.PodTemplate.Options.ImageConfig.Values[0].Spec.Image = "new-image:latest" + return fmt.Sprintf("imageConfig value %q is in use and cannot be changed", wsk.Spec.PodTemplate.Options.ImageConfig.Values[0].Id) }, - workspaceName: ptr.To("ws-webhook-update-image-config-spec-test"), + workspaceName: "wsk-webhook-update-image-config-test", shouldSucceed: false, }, { - description: "should reject updates to used podConfig spec", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { + description: "should reject updates to in-use podConfig spec", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { wsk.Spec.PodTemplate.Options.PodConfig.Values[0].Spec.Resources = &corev1.ResourceRequirements{ Limits: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("1.5"), }, } + return fmt.Sprintf("podConfig value %q is in use and cannot be changed", wsk.Spec.PodTemplate.Options.PodConfig.Values[0].Id) }, - workspaceName: ptr.To("ws-webhook-update-pod-config-spec-test"), + workspaceName: "ws-webhook-update-pod-config-test", shouldSucceed: false, }, { - description: "should reject WorkspaceKind update with cycles in imageConfig options", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { - wsk.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &v1beta1.OptionRedirect{To: "jupyterlab_scipy_190"} + description: "should reject removing in-use imageConfig values", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { + toBeRemoved := "jupyterlab_scipy_180" + newValues := make([]kubefloworgv1beta1.ImageConfigValue, 0) + for _, value := range wsk.Spec.PodTemplate.Options.ImageConfig.Values { + if value.Id != toBeRemoved { + newValues = append(newValues, value) + } + } + wsk.Spec.PodTemplate.Options.ImageConfig.Values = newValues + return fmt.Sprintf("imageConfig value %q is in use and cannot be removed", toBeRemoved) }, + workspaceName: "ws-webhook-update-image-config-test", shouldSucceed: false, }, { - description: "should reject WorkspaceKind update with invalid redirects in ImageConfig options", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { - wsk.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &v1beta1.OptionRedirect{To: "invalid-image-config"} + description: "should reject removing in-use podConfig values", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { + toBeRemoved := "tiny_cpu" + newValues := make([]kubefloworgv1beta1.PodConfigValue, 0) + for _, value := range wsk.Spec.PodTemplate.Options.PodConfig.Values { + if value.Id != toBeRemoved { + newValues = append(newValues, value) + } + } + wsk.Spec.PodTemplate.Options.PodConfig.Values = newValues + return fmt.Sprintf("podConfig value %q is in use and cannot be removed", toBeRemoved) }, + workspaceName: "ws-webhook-update-pod-config-test", shouldSucceed: false, }, { - description: "should reject WorkspaceKind update with cycles in PodConfig options", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { - wsk.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &v1beta1.OptionRedirect{To: "small_cpu"} - wsk.Spec.PodTemplate.Options.PodConfig.Values[1].Redirect = &v1beta1.OptionRedirect{To: "tiny_cpu"} + description: "should reject updating an imageConfig value to create a self-cycle", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { + valueId := wsk.Spec.PodTemplate.Options.ImageConfig.Values[1].Id + wsk.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &kubefloworgv1beta1.OptionRedirect{To: valueId} + return fmt.Sprintf("imageConfig redirect cycle detected: [%s]", valueId) }, shouldSucceed: false, }, { - description: "should reject WorkspaceKind creation with invalid redirects in PodConfig options", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { - wsk.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &v1beta1.OptionRedirect{To: "invalid-pod-config"} + description: "should reject updating a podConfig value to create a 2-step cycle", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { + step1 := wsk.Spec.PodTemplate.Options.PodConfig.Values[0].Id + step2 := wsk.Spec.PodTemplate.Options.PodConfig.Values[1].Id + wsk.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &kubefloworgv1beta1.OptionRedirect{To: step2} + wsk.Spec.PodTemplate.Options.PodConfig.Values[1].Redirect = &kubefloworgv1beta1.OptionRedirect{To: step1} + return "podConfig redirect cycle detected: [" // there is no guarantee on which element will be first }, shouldSucceed: false, }, { - description: "should reject updates to WorkspaceKind with missing default imageConfig", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { - wsk.Spec.PodTemplate.Options.ImageConfig.Spawner.Default = "invalid-image-config" + description: "should reject updating an imageConfig to redirect to a non-existent value", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { + invalidTarget := "invalid_image_config" + wsk.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &kubefloworgv1beta1.OptionRedirect{To: invalidTarget} + return fmt.Sprintf("target imageConfig %q does not exist", invalidTarget) }, shouldSucceed: false, }, { - description: "should reject updates to WorkspaceKind with missing default podConfig", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { - wsk.Spec.PodTemplate.Options.PodConfig.Spawner.Default = "invalid-pod-config" + description: "should reject updating a podConfig to redirect to a non-existent value", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { + invalidTarget := "invalid_pod_config" + wsk.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &kubefloworgv1beta1.OptionRedirect{To: invalidTarget} + return fmt.Sprintf("target podConfig %q does not exist", invalidTarget) }, + shouldSucceed: false, }, { - description: "should reject updates to WorkspaceKind if extraEnv[].value is not a valid Go template", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { - wsk.Spec.PodTemplate.ExtraEnv[0].Value = `{{ httpPathPrefix "jupyterlab" }` + description: "should reject updating the default imageConfig value to a non-existent value", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { + invalidDefault := "invalid_image_config" + wsk.Spec.PodTemplate.Options.ImageConfig.Spawner.Default = invalidDefault + return fmt.Sprintf("default imageConfig %q not found", invalidDefault) }, shouldSucceed: false, }, { - description: "should accept updates to WorkspaceKind with valid extraEnv[].value Go template", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { - wsk.Spec.PodTemplate.ExtraEnv[0].Value = `{{ httpPathPrefix "jupyterlab" }}` + description: "should reject updating the default podConfig value to a non-existent value", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { + invalidDefault := "invalid_pod_config" + wsk.Spec.PodTemplate.Options.PodConfig.Spawner.Default = invalidDefault + return fmt.Sprintf("default podConfig %q not found", invalidDefault) }, - shouldSucceed: true, + shouldSucceed: false, }, { - description: "should reject updates that remove ImageConfig in use", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { - wsk.Spec.PodTemplate.Options.ImageConfig.Values = wsk.Spec.PodTemplate.Options.ImageConfig.Values[1:] + description: "should reject updating an imageConfig to have duplicate ports", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { + duplicatePortNumber := int32(8888) + wsk.Spec.PodTemplate.Options.ImageConfig.Values[1].Spec.Ports = []kubefloworgv1beta1.ImagePort{ + { + Id: "jupyterlab", + DisplayName: "JupyterLab", + Port: duplicatePortNumber, + Protocol: "HTTP", + }, + { + Id: "jupyterlab2", + DisplayName: "JupyterLab2", + Port: duplicatePortNumber, + Protocol: "HTTP", + }, + } + return fmt.Sprintf("port %d is defined more than once", duplicatePortNumber) }, - workspaceName: ptr.To("ws-webhook-update-image-config-test"), shouldSucceed: false, }, { - description: "should reject updates that remove podConfig in use", - modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { - wsk.Spec.PodTemplate.Options.PodConfig.Values = wsk.Spec.PodTemplate.Options.PodConfig.Values[1:] + description: "should reject updating an extraEnv[].value to an invalid Go template", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { + invalidValue := `{{ httpPathPrefix "jupyterlab" }` + wsk.Spec.PodTemplate.ExtraEnv[0].Value = invalidValue + return fmt.Sprintf("failed to parse template %q", invalidValue) }, - workspaceName: ptr.To("ws-webhook-update-pod-config-test"), shouldSucceed: false, }, + { + description: "should accept updating an extraEnv[].value to a valid Go template", + modifyKindFn: func(wsk *kubefloworgv1beta1.WorkspaceKind) string { + wsk.Spec.PodTemplate.ExtraEnv[0].Value = `{{ httpPathPrefix "jupyterlab" }}` + return "" + }, + shouldSucceed: true, + }, } for _, tc := range testCases { tc := tc // Create a new instance of tc to avoid capturing the loop variable. It(tc.description, func() { - if tc.workspaceName != nil { - By("creating a Workspace with the WorkspaceKind") - workspace := NewExampleWorkspace(*tc.workspaceName, namespaceName, workspaceKind.Name) + if tc.workspaceName != "" { + By("creating a Workspace that uses the WorkspaceKind") + workspace := NewExampleWorkspace(tc.workspaceName, namespaceName, workspaceKindName) Expect(k8sClient.Create(ctx, workspace)).To(Succeed()) } patch := client.MergeFrom(workspaceKind.DeepCopy()) modifiedWorkspaceKind := workspaceKind.DeepCopy() + expectedErrorMessage := tc.modifyKindFn(modifiedWorkspaceKind) - tc.modifyKindFn(modifiedWorkspaceKind) + By("updating the WorkspaceKind") if tc.shouldSucceed { Expect(k8sClient.Patch(ctx, modifiedWorkspaceKind, patch)).To(Succeed()) } else { - Expect(k8sClient.Patch(ctx, modifiedWorkspaceKind, patch)).NotTo(Succeed()) + err := k8sClient.Patch(ctx, modifiedWorkspaceKind, patch) + Expect(err).NotTo(Succeed()) + if expectedErrorMessage != "" { + Expect(err.Error()).To(ContainSubstring(expectedErrorMessage)) + } + } + + if tc.workspaceName != "" { + By("deleting the Workspace that uses the WorkspaceKind") + workspace := &kubefloworgv1beta1.Workspace{ + ObjectMeta: metav1.ObjectMeta{ + Name: tc.workspaceName, + Namespace: namespaceName, + }, + } + Expect(k8sClient.Delete(ctx, workspace)).To(Succeed()) } }) } }) - }) diff --git a/workspaces/controller/test/e2e/e2e_test.go b/workspaces/controller/test/e2e/e2e_test.go index 0e2d2f2a..52c7493b 100644 --- a/workspaces/controller/test/e2e/e2e_test.go +++ b/workspaces/controller/test/e2e/e2e_test.go @@ -43,6 +43,9 @@ const ( workspacePortInt = 8888 workspacePortId = "jupyterlab" + // workspacekind configs + workspaceKindName = "jupyterlab" + // curl image curlImage = "curlimages/curl:8.9.1" @@ -50,7 +53,7 @@ const ( timeout = time.Second * 60 // how long to wait in "Consistently" blocks - duration = time.Second * 10 + duration = time.Second * 10 // nolint:unused // how frequently to poll for conditions interval = time.Second * 1 @@ -311,14 +314,58 @@ var _ = Describe("controller", Ordered, func() { } Eventually(curlService, timeout, interval).Should(Succeed()) - By("ensuring that an option in WorkspaceKind cannot be removed if it is currently in use") + By("ensuring in-use imageConfig values cannot be removed from WorkspaceKind") EventuallyWithOffset(1, func() error { - // Attempt to remove an option from the WorkspaceKind that is currently in use - cmd := exec.Command("kubectl", "patch", "workspacekind", "jupyterlab", + cmd := exec.Command("kubectl", "patch", "workspacekind", workspaceKindName, "--type=json", "-p", `[{"op": "remove", "path": "/spec/podTemplate/options/imageConfig/values/1"}]`) _, err := utils.Run(cmd) return err }, timeout, interval).ShouldNot(Succeed()) + + By("ensuring unused imageConfig values can be removed from WorkspaceKind") + EventuallyWithOffset(1, func() error { + cmd := exec.Command("kubectl", "patch", "workspacekind", workspaceKindName, + "--type=json", "-p", `[{"op": "remove", "path": "/spec/podTemplate/options/imageConfig/values/0"}]`) + _, err := utils.Run(cmd) + return err + }, timeout, interval).Should(Succeed()) + + By("ensuring in-use podConfig values cannot be removed from WorkspaceKind") + EventuallyWithOffset(1, func() error { + cmd := exec.Command("kubectl", "patch", "workspacekind", workspaceKindName, + "--type=json", "-p", `[{"op": "remove", "path": "/spec/podTemplate/options/podConfig/values/0"}]`) + _, err := utils.Run(cmd) + return err + }, timeout, interval).ShouldNot(Succeed()) + + By("ensuring unused podConfig values can be removed from WorkspaceKind") + EventuallyWithOffset(1, func() error { + cmd := exec.Command("kubectl", "patch", "workspacekind", workspaceKindName, + "--type=json", "-p", `[{"op": "remove", "path": "/spec/podTemplate/options/podConfig/values/1"}]`) + _, err := utils.Run(cmd) + return err + }, timeout, interval).Should(Succeed()) + + By("failing to delete an in-use WorkspaceKind") + EventuallyWithOffset(1, func() error { + cmd = exec.Command("kubectl", "delete", "workspacekind", workspaceKindName) + _, err := utils.Run(cmd) + return err + }, timeout, interval).ShouldNot(Succeed()) + + By("deleting the Workspace") + EventuallyWithOffset(1, func() error { + cmd := exec.Command("kubectl", "delete", "workspace", workspaceName, "-n", workspaceNamespace) + _, err := utils.Run(cmd) + return err + }, timeout, interval).Should(Succeed()) + + By("deleting an unused WorkspaceKind") + EventuallyWithOffset(1, func() error { + cmd = exec.Command("kubectl", "delete", "workspacekind", workspaceKindName) + _, err := utils.Run(cmd) + return err + }, timeout, interval).Should(Succeed()) }) }) })