mathew refactor 2

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

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
 )
 

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

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

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
+}

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",

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

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())
+		})
+	})
 })