From ce5ea50940ef455ba8be5b5aa37707cf0a81c3c2 Mon Sep 17 00:00:00 2001 From: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com> Date: Mon, 26 Aug 2024 13:07:13 -0700 Subject: [PATCH] mathew refactor 1 Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com> --- .../api/v1beta1/workspace_webhook.go | 132 ---- .../api/v1beta1/workspacekind_webhook.go | 409 ------------ .../api/v1beta1/zz_generated.deepcopy.go | 21 +- workspaces/controller/cmd/main.go | 37 +- .../controller/config/webhook/manifests.yaml | 1 + .../internal/controller/suite_test.go | 13 +- .../controller/workspace_controller.go | 107 +--- .../controller/workspacekind_controller.go | 15 +- .../workspacekind_controller_test.go | 4 +- .../controller/internal/helper/graph.go | 55 ++ .../controller/internal/helper/graph_test.go | 110 ++++ .../controller/internal/helper/index.go | 77 +++ .../controller/internal/helper/suite_test.go | 39 ++ .../controller/internal/helper/template.go | 30 + .../webhook/suite_test.go} | 219 +++---- .../internal/webhook/workspace_webhook.go | 231 +++++++ .../webhook}/workspace_webhook_test.go | 20 +- .../internal/webhook/workspacekind_webhook.go | 595 ++++++++++++++++++ .../webhook}/workspacekind_webhook_test.go | 87 ++- 19 files changed, 1395 insertions(+), 807 deletions(-) delete mode 100644 workspaces/controller/api/v1beta1/workspace_webhook.go delete mode 100644 workspaces/controller/api/v1beta1/workspacekind_webhook.go create mode 100644 workspaces/controller/internal/helper/graph.go create mode 100644 workspaces/controller/internal/helper/graph_test.go create mode 100644 workspaces/controller/internal/helper/index.go create mode 100644 workspaces/controller/internal/helper/suite_test.go create mode 100644 workspaces/controller/internal/helper/template.go rename workspaces/controller/{api/v1beta1/webhook_suite_test.go => internal/webhook/suite_test.go} (69%) create mode 100644 workspaces/controller/internal/webhook/workspace_webhook.go rename workspaces/controller/{api/v1beta1 => internal/webhook}/workspace_webhook_test.go (86%) create mode 100644 workspaces/controller/internal/webhook/workspacekind_webhook.go rename workspaces/controller/{api/v1beta1 => internal/webhook}/workspacekind_webhook_test.go (76%) diff --git a/workspaces/controller/api/v1beta1/workspace_webhook.go b/workspaces/controller/api/v1beta1/workspace_webhook.go deleted file mode 100644 index 979a1d1e..00000000 --- a/workspaces/controller/api/v1beta1/workspace_webhook.go +++ /dev/null @@ -1,132 +0,0 @@ -/* -Copyright 2024. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package v1beta1 - -import ( - "context" - "fmt" - "k8s.io/apimachinery/pkg/runtime" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/webhook" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" -) - -// log is for logging in this package. -var ( - workspaceLog = logf.Log.WithName("workspace-resource") - k8sClient client.Client -) - -// SetupWebhookWithManager will setup the manager to manage the webhooks -func (r *Workspace) SetupWebhookWithManager(mgr ctrl.Manager) error { - k8sClient = mgr.GetClient() - return ctrl.NewWebhookManagedBy(mgr). - For(r). - Complete() -} - -// TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation. -//+kubebuilder:webhook:path=/validate-kubeflow-org-v1beta1-workspace,mutating=false,failurePolicy=fail,sideEffects=None,groups=kubeflow.org,resources=workspaces,verbs=create;update,versions=v1beta1,name=vworkspace.kb.io,admissionReviewVersions=v1 - -var _ webhook.Validator = &Workspace{} - -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type -func (r *Workspace) ValidateCreate() (admission.Warnings, error) { - workspaceLog.Info("validate create", "name", r.Name) - - workspaceKindName := r.Spec.Kind - workspaceKind := &WorkspaceKind{} - if err := k8sClient.Get(context.Background(), client.ObjectKey{Name: workspaceKindName}, workspaceKind); err != nil { - return nil, fmt.Errorf("workspace kind %s not found", workspaceKindName) - } - - var errorList ErrorList - if err := validateImageConfig(workspaceKind, r.Spec.PodTemplate.Options.ImageConfig); err != nil { - errorList = append(errorList, err.Error()) - } - if err := validatePodConfig(workspaceKind, r.Spec.PodTemplate.Options.PodConfig); err != nil { - errorList = append(errorList, err.Error()) - } - - if len(errorList) > 0 { - return nil, errorList - } - - return nil, nil -} - -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type -func (r *Workspace) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - workspaceLog.Info("validate update", "name", r.Name) - - oldWorkspace, ok := old.(*Workspace) - if !ok { - return nil, fmt.Errorf("old object is not a workspace") - } - workspaceKindName := r.Spec.Kind - workspaceKind := &WorkspaceKind{} - if err := k8sClient.Get(context.Background(), client.ObjectKey{Name: workspaceKindName}, workspaceKind); err != nil { - return nil, fmt.Errorf("workspace kind %s not found", workspaceKindName) - } - var errorList ErrorList - - if r.Spec.PodTemplate.Options.ImageConfig != oldWorkspace.Spec.PodTemplate.Options.ImageConfig { - if err := validateImageConfig(workspaceKind, r.Spec.PodTemplate.Options.ImageConfig); err != nil { - errorList = append(errorList, err.Error()) - } - } - if r.Spec.PodTemplate.Options.PodConfig != oldWorkspace.Spec.PodTemplate.Options.PodConfig { - if err := validatePodConfig(workspaceKind, r.Spec.PodTemplate.Options.PodConfig); err != nil { - errorList = append(errorList, err.Error()) - } - } - - if len(errorList) > 1 { - return nil, errorList - } - return nil, nil -} - -// validateImageConfig checks if the selected imageConfig is valid -func validateImageConfig(workspaceKind *WorkspaceKind, imageConfigID string) error { - for _, imageConfig := range workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values { - if imageConfig.Id == imageConfigID { - return nil - } - } - return fmt.Errorf("imageConfig %s not found in workspace kind %s", imageConfigID, workspaceKind.Name) -} - -// validatePodConfig checks if the selected podConfig is valid -func validatePodConfig(workspaceKind *WorkspaceKind, podConfigID string) error { - for _, podConfig := range workspaceKind.Spec.PodTemplate.Options.PodConfig.Values { - if podConfig.Id == podConfigID { - return nil - } - } - return fmt.Errorf("podConfig %s not found in workspace kind %s", podConfigID, workspaceKind.Name) -} - -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type -func (r *Workspace) ValidateDelete() (admission.Warnings, error) { - workspaceLog.Info("validate delete", "name", r.Name) - - // TODO(user): fill in your validation logic upon object deletion. - return nil, nil -} diff --git a/workspaces/controller/api/v1beta1/workspacekind_webhook.go b/workspaces/controller/api/v1beta1/workspacekind_webhook.go deleted file mode 100644 index ab44c264..00000000 --- a/workspaces/controller/api/v1beta1/workspacekind_webhook.go +++ /dev/null @@ -1,409 +0,0 @@ -/* -Copyright 2024. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package v1beta1 - -import ( - "bytes" - "context" - "errors" - "fmt" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" - "k8s.io/apimachinery/pkg/fields" - "k8s.io/apimachinery/pkg/runtime" - "reflect" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/webhook" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "strings" - "text/template" -) - -type ErrorList []string - -func (e ErrorList) Error() string { - return strings.Join(e, " , ") -} - -const kbCacheWorkspaceKindField = ".spec.kind" - -// log is for logging in this package. -var workspaceKindLog = logf.Log.WithName("workspacekind-resource") - -// SetupWebhookWithManager will setup the manager to manage the webhooks -func (r *WorkspaceKind) SetupWebhookWithManager(mgr ctrl.Manager) error { - return ctrl.NewWebhookManagedBy(mgr). - For(r). - Complete() -} - -// TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation. -//+kubebuilder:webhook:path=/validate-kubeflow-org-v1beta1-workspacekind,mutating=false,failurePolicy=fail,sideEffects=None,groups=kubeflow.org,resources=workspacekinds,verbs=create;update,versions=v1beta1,name=vworkspacekind.kb.io,admissionReviewVersions=v1 - -var _ webhook.Validator = &WorkspaceKind{} - -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type -func (r *WorkspaceKind) ValidateCreate() (admission.Warnings, error) { - workspaceKindLog.Info("validate create", "name", r.Name) - - imageConfigValueMap, err := generateImageConfigAndValidatePorts(r.Spec.PodTemplate.Options.ImageConfig) - if err != nil { - return nil, err - } - podConfigValueMap := make(map[string]PodConfigValue) - for _, podConfigValue := range r.Spec.PodTemplate.Options.PodConfig.Values { - podConfigValueMap[podConfigValue.Id] = podConfigValue - } - - if err := validateImageConfigCycles(imageConfigValueMap); err != nil { - return nil, err - } - if err := validatePodConfigCycle(podConfigValueMap); err != nil { - return nil, err - } - - if err := ensureDefaultOptions(imageConfigValueMap, podConfigValueMap, r.Spec.PodTemplate.Options); err != nil { - return nil, err - } - - if _, err := RenderAndValidateExtraEnv(r.Spec.PodTemplate.ExtraEnv, func(string) string { return "" }, false); err != nil { - return nil, err - } - - return nil, nil -} - -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type -func (r *WorkspaceKind) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - var ( - imageConfigUsageCount map[string]int - podConfigUsageCount map[string]int - isConfigUsageCountCalculated bool - err error - ) - - generateAndValidateImageConfig := func(imageConfig ImageConfig, oldImageConfig ImageConfig) (map[string]ImageConfigValue, map[string]ImageConfigValue, error) { - oldImageConfigValueMap := make(map[string]ImageConfigValue) - imageConfigValueMap := make(map[string]ImageConfigValue) - - for _, imageConfigValue := range oldImageConfig.Values { - oldImageConfigValueMap[imageConfigValue.Id] = imageConfigValue - } - - for _, imageConfigValue := range imageConfig.Values { - if oldImageConfigValue, exists := oldImageConfigValueMap[imageConfigValue.Id]; exists { - if !isConfigUsageCountCalculated { - imageConfigUsageCount, podConfigUsageCount, err = getConfigUsageCount(r.Name) - if err != nil { - return nil, nil, err - } - isConfigUsageCountCalculated = true - } - if imageConfigUsageCount[imageConfigValue.Id] > 0 && !reflect.DeepEqual(oldImageConfigValue.Spec, imageConfigValue.Spec) { - return nil, nil, fmt.Errorf("spec.podTemplate.options.imageConfig.values with id '%s' is immutable because it is used by %d workspace(s)", imageConfigValue.Id, imageConfigUsageCount[imageConfigValue.Id]) - } - } - imageConfigValueMap[imageConfigValue.Id] = imageConfigValue - } - - for id, _ := range oldImageConfigValueMap { - if _, exists := imageConfigValueMap[id]; !exists && imageConfigUsageCount[id] > 0 { - return nil, nil, fmt.Errorf("spec.podTemplate.options.imageConfig.values with id '%s' is used by %d workspace(s)", id, imageConfigUsageCount[id]) - } - } - return imageConfigValueMap, oldImageConfigValueMap, nil - } - generateAndValidatePodConfig := func(podConfig PodConfig, oldPodConfig PodConfig) (map[string]PodConfigValue, map[string]PodConfigValue, error) { - oldPodConfigValueMap := make(map[string]PodConfigValue) - podConfigValueMap := make(map[string]PodConfigValue) - - for _, podConfigValue := range oldPodConfig.Values { - oldPodConfigValueMap[podConfigValue.Id] = podConfigValue - } - - for _, podConfigValue := range podConfig.Values { - if oldPodConfigValue, exists := oldPodConfigValueMap[podConfigValue.Id]; exists { - err := normalizePodConfigSpec(&oldPodConfigValue.Spec) - if err != nil { - return nil, nil, err - } - err = normalizePodConfigSpec(&podConfigValue.Spec) - if err != nil { - return nil, nil, err - } - if !isConfigUsageCountCalculated { - _, podConfigUsageCount, err = getConfigUsageCount(r.Name) - if err != nil { - return nil, nil, err - } - isConfigUsageCountCalculated = true - } - if podConfigUsageCount[podConfigValue.Id] > 0 && !reflect.DeepEqual(oldPodConfigValue.Spec, podConfigValue.Spec) { - return nil, nil, fmt.Errorf("spec.podTemplate.options.podConfig.values with id '%s' is immutable because it is used by %d workspace(s)", podConfigValue.Id, podConfigUsageCount[podConfigValue.Id]) - } - } - podConfigValueMap[podConfigValue.Id] = podConfigValue - } - - for id, _ := range oldPodConfigValueMap { - if _, exists := podConfigValueMap[id]; !exists { - if podConfigUsageCount[id] > 0 { - return nil, nil, fmt.Errorf("spec.podTemplate.options.podConfig.values with id '%s' is used by %d workspace(s)", id, podConfigUsageCount[id]) - } - } - } - - return podConfigValueMap, oldPodConfigValueMap, nil - } - - workspaceKindLog.Info("validate update", "name", r.Name) - - // Type assertion to convert the old runtime.Object to WorkspaceKind - oldWorkspaceKind, ok := old.(*WorkspaceKind) - if !ok { - return nil, errors.New("old object is not a WorkspaceKind") - } - - imageConfigValueMap, oldImageConfigValueMap, err := generateAndValidateImageConfig( - r.Spec.PodTemplate.Options.ImageConfig, - oldWorkspaceKind.Spec.PodTemplate.Options.ImageConfig, - ) - if err != nil { - return nil, err - } - if !reflect.DeepEqual(imageConfigValueMap, oldImageConfigValueMap) { - if err := validateImageConfigCycles(imageConfigValueMap); err != nil { - return nil, err - } - } - - podConfigValueMap, oldPodConfigValueMap, err := generateAndValidatePodConfig( - r.Spec.PodTemplate.Options.PodConfig, - oldWorkspaceKind.Spec.PodTemplate.Options.PodConfig, - ) - if err != nil { - return nil, err - } - - if !reflect.DeepEqual(podConfigValueMap, oldPodConfigValueMap) { - if err := validatePodConfigCycle(podConfigValueMap); err != nil { - return nil, err - } - } - - if !reflect.DeepEqual(imageConfigValueMap, oldImageConfigValueMap) || - !reflect.DeepEqual(podConfigValueMap, oldPodConfigValueMap) || - r.Spec.PodTemplate.Options.ImageConfig.Spawner.Default != oldWorkspaceKind.Spec.PodTemplate.Options.ImageConfig.Spawner.Default || - r.Spec.PodTemplate.Options.PodConfig.Spawner.Default != oldWorkspaceKind.Spec.PodTemplate.Options.PodConfig.Spawner.Default { - if err := ensureDefaultOptions(imageConfigValueMap, podConfigValueMap, r.Spec.PodTemplate.Options); err != nil { - return nil, err - } - } - - if !reflect.DeepEqual(r.Spec.PodTemplate.ExtraEnv, oldWorkspaceKind.Spec.PodTemplate.ExtraEnv) { - if _, err := RenderAndValidateExtraEnv(r.Spec.PodTemplate.ExtraEnv, func(string) string { return "" }, false); err != nil { - return nil, err - } - } - - return nil, nil -} - -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type -func (r *WorkspaceKind) ValidateDelete() (admission.Warnings, error) { - workspaceKindLog.Info("validate delete", "name", r.Name) - if r.Status.Workspaces > 0 { - return nil, fmt.Errorf("can not delete workspaceKind %s becuase it is used by %d workspace(s)", r.Name, r.Status.Workspaces) - } - return nil, nil -} - -func generateImageConfigAndValidatePorts(imageConfig ImageConfig) (map[string]ImageConfigValue, error) { - var errorList ErrorList - imageConfigValueMap := make(map[string]ImageConfigValue) - for _, imageConfigValue := range imageConfig.Values { - - ports := make(map[int32]bool) - for _, port := range imageConfigValue.Spec.Ports { - if _, exists := ports[port.Port]; exists { - errorList = append(errorList, fmt.Sprintf("duplicate port %d in imageConfig with id '%s'", port.Port, imageConfigValue.Id)) - } - ports[port.Port] = true - } - - imageConfigValueMap[imageConfigValue.Id] = imageConfigValue - } - if len(errorList) > 0 { - return imageConfigValueMap, errorList - } - return imageConfigValueMap, nil -} - -func ensureDefaultOptions(imageConfigValueMap map[string]ImageConfigValue, podConfigValueMap map[string]PodConfigValue, workspaceOptions WorkspaceKindPodOptions) error { - var errorList ErrorList - if _, ok := imageConfigValueMap[workspaceOptions.ImageConfig.Spawner.Default]; !ok { - errorList = append(errorList, fmt.Sprintf("default image config with id '%s' is not found in spec.podTemplate.options.imageConfig.values", workspaceOptions.ImageConfig.Spawner.Default)) - } - - if _, ok := podConfigValueMap[workspaceOptions.PodConfig.Spawner.Default]; !ok { - errorList = append(errorList, fmt.Sprintf("default pod config with id '%s' is not found in spec.podTemplate.options.podConfig.values", workspaceOptions.PodConfig.Spawner.Default)) - } - if len(errorList) > 0 { - return errorList - } - return nil -} - -func validateImageConfigCycles(imageConfigValueMap map[string]ImageConfigValue) error { - for _, currentImageConfig := range imageConfigValueMap { - // follow any redirects to get the desired imageConfig - desiredImageConfig := currentImageConfig - visitedNodes := map[string]bool{currentImageConfig.Id: true} - for { - if desiredImageConfig.Redirect == nil { - break - } - if visitedNodes[desiredImageConfig.Redirect.To] { - return fmt.Errorf("imageConfig with id '%s' has a circular redirect", desiredImageConfig.Id) - } - nextNode, ok := imageConfigValueMap[desiredImageConfig.Redirect.To] - if !ok { - return fmt.Errorf("imageConfig with id '%s' not found, was redirected from '%s'", desiredImageConfig.Redirect.To, desiredImageConfig.Id) - } - desiredImageConfig = nextNode - visitedNodes[desiredImageConfig.Id] = true - } - } - return nil -} - -func validatePodConfigCycle(podConfigValueMap map[string]PodConfigValue) error { - for _, currentPodConfig := range podConfigValueMap { - // follow any redirects to get the desired podConfig - desiredPodConfig := currentPodConfig - visitedNodes := map[string]bool{currentPodConfig.Id: true} - for { - if desiredPodConfig.Redirect == nil { - break - } - if visitedNodes[desiredPodConfig.Redirect.To] { - return fmt.Errorf("podConfig with id '%s' has a circular redirect", desiredPodConfig.Id) - } - nextNode, ok := podConfigValueMap[desiredPodConfig.Redirect.To] - if !ok { - return fmt.Errorf("podConfig with id '%s' not found, was redirected from '%s'", desiredPodConfig.Redirect.To, desiredPodConfig.Id) - } - desiredPodConfig = nextNode - visitedNodes[desiredPodConfig.Id] = true - } - } - return nil -} - -func RenderAndValidateExtraEnv(extraEnv []corev1.EnvVar, templateFunc func(string) string, shouldExecTemplate bool) ([]corev1.EnvVar, error) { - var errorList ErrorList - containerEnv := make([]corev1.EnvVar, 0) - - for _, env := range extraEnv { - if env.Value != "" { - rawValue := env.Value - tmpl, err := template.New("value").Funcs(template.FuncMap{"httpPathPrefix": templateFunc}).Parse(rawValue) - if err != nil { - errorList = append(errorList, fmt.Sprintf("failed to parse value %q: %v", rawValue, err)) - continue - } - if shouldExecTemplate { - var buf bytes.Buffer - err = tmpl.Execute(&buf, nil) - if err != nil { - errorList = append(errorList, fmt.Sprintf("failed to execute template for extraEnv '%s': %v", env.Name, err)) - continue - } - - env.Value = buf.String() - } - } - containerEnv = append(containerEnv, env) - } - if len(errorList) > 0 { - return nil, errorList - } - return containerEnv, nil - -} - -func getConfigUsageCount(workspaceKindName string) (map[string]int, map[string]int, error) { - workspaces := &WorkspaceList{} - listOpts := &client.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(kbCacheWorkspaceKindField, workspaceKindName), - Namespace: corev1.NamespaceAll, - } - if err := k8sClient.List(context.Background(), workspaces, listOpts); err != nil { - return nil, nil, err - } - - imageConfigUsageCount := make(map[string]int) - podConfigUsageCount := make(map[string]int) - for _, ws := range workspaces.Items { - imageConfigUsageCount[ws.Spec.PodTemplate.Options.ImageConfig]++ - podConfigUsageCount[ws.Spec.PodTemplate.Options.PodConfig]++ - } - return imageConfigUsageCount, podConfigUsageCount, nil -} - -func normalizePodConfigSpec(spec *PodConfigSpec) (err error) { - // 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 ResourceRequests - 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 ResourceLimits - 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/api/v1beta1/zz_generated.deepcopy.go b/workspaces/controller/api/v1beta1/zz_generated.deepcopy.go index c1d8785a..1beab4fd 100644 --- a/workspaces/controller/api/v1beta1/zz_generated.deepcopy.go +++ b/workspaces/controller/api/v1beta1/zz_generated.deepcopy.go @@ -22,7 +22,7 @@ package v1beta1 import ( "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime" + runtime "k8s.io/apimachinery/pkg/runtime" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. @@ -85,25 +85,6 @@ func (in *ActivityProbeJupyter) DeepCopy() *ActivityProbeJupyter { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in ErrorList) DeepCopyInto(out *ErrorList) { - { - in := &in - *out = make(ErrorList, len(*in)) - copy(*out, *in) - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ErrorList. -func (in ErrorList) DeepCopy() ErrorList { - if in == nil { - return nil - } - out := new(ErrorList) - in.DeepCopyInto(out) - return *out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HTTPProxy) DeepCopyInto(out *HTTPProxy) { *out = *in diff --git a/workspaces/controller/cmd/main.go b/workspaces/controller/cmd/main.go index e9f85fd2..82659c2f 100644 --- a/workspaces/controller/cmd/main.go +++ b/workspaces/controller/cmd/main.go @@ -21,6 +21,9 @@ 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" @@ -35,7 +38,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" - "github.com/kubeflow/notebooks/workspaces/controller/internal/controller" + controllerInternal "github.com/kubeflow/notebooks/workspaces/controller/internal/controller" //+kubebuilder:scaffold:imports ) @@ -115,40 +118,56 @@ func main() { // the manager stops, so would be fine to enable this option. However, // if you are doing or is intended to do any operation such as perform cleanups // after the manager stops then its usage might be unsafe. - // LeaderElectionReleaseOnCancel: true, + // + // TODO: check if we are doing anything which would prevent us from using this option. + //LeaderElectionReleaseOnCancel: true, }) if err != nil { setupLog.Error(err, "unable to start manager") os.Exit(1) } - if err = (&controller.WorkspaceReconciler{ + // setup field indexers on the manager cache. we use these indexes to efficiently + // query the cache for things like which Workspaces are using a particular WorkspaceKind + if err := helper.SetupManagerFieldIndexers(mgr); err != nil { + setupLog.Error(err, "unable to setup field indexers") + os.Exit(1) + } + + if err = (&controllerInternal.WorkspaceReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Workspace") os.Exit(1) } - if err = (&controller.WorkspaceKindReconciler{ + if err = (&controllerInternal.WorkspaceKindReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "WorkspaceKind") os.Exit(1) } + //+kubebuilder:scaffold:builder + if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err = (&kubefloworgv1beta1.WorkspaceKind{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "WorkspaceKind") + if err = (&webhookInternal.WorkspaceValidator{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + }).SetupWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "Workspace") os.Exit(1) } } if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err = (&kubefloworgv1beta1.Workspace{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "Workspace") + if err = (&webhookInternal.WorkspaceKindValidator{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + }).SetupWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "WorkspaceKind") os.Exit(1) } } - //+kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { setupLog.Error(err, "unable to set up health check") diff --git a/workspaces/controller/config/webhook/manifests.yaml b/workspaces/controller/config/webhook/manifests.yaml index 04256eb3..d757a597 100644 --- a/workspaces/controller/config/webhook/manifests.yaml +++ b/workspaces/controller/config/webhook/manifests.yaml @@ -41,6 +41,7 @@ webhooks: operations: - CREATE - UPDATE + - DELETE resources: - workspacekinds sideEffects: None diff --git a/workspaces/controller/internal/controller/suite_test.go b/workspaces/controller/internal/controller/suite_test.go index f467c820..a4444fd6 100644 --- a/workspaces/controller/internal/controller/suite_test.go +++ b/workspaces/controller/internal/controller/suite_test.go @@ -41,6 +41,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" //+kubebuilder:scaffold:imports ) @@ -101,26 +102,30 @@ var _ = BeforeSuite(func() { BindAddress: "0", // disable metrics serving }, }) - Expect(err).ToNot(HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) + + By("setting up the field indexers for the controller manager") + err = helper.SetupManagerFieldIndexers(k8sManager) + Expect(err).NotTo(HaveOccurred()) By("setting up the Workspace controller") err = (&WorkspaceReconciler{ Client: k8sManager.GetClient(), Scheme: k8sManager.GetScheme(), }).SetupWithManager(k8sManager) - Expect(err).ToNot(HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) By("setting up the WorkspaceKind controller") err = (&WorkspaceKindReconciler{ Client: k8sManager.GetClient(), Scheme: k8sManager.GetScheme(), }).SetupWithManager(k8sManager) - Expect(err).ToNot(HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) go func() { defer GinkgoRecover() err = k8sManager.Start(ctx) - Expect(err).ToNot(HaveOccurred(), "failed to run manager") + Expect(err).NotTo(HaveOccurred(), "failed to run manager") }() }) diff --git a/workspaces/controller/internal/controller/workspace_controller.go b/workspaces/controller/internal/controller/workspace_controller.go index 27aa5e41..8a7b0fcb 100644 --- a/workspaces/controller/internal/controller/workspace_controller.go +++ b/workspaces/controller/internal/controller/workspace_controller.go @@ -19,15 +19,13 @@ package controller import ( "context" "fmt" - "k8s.io/apimachinery/pkg/util/intstr" "reflect" "strings" - "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" + "k8s.io/apimachinery/pkg/util/intstr" "github.com/go-logr/logr" - kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -44,6 +42,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" + + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" ) const ( @@ -51,11 +52,6 @@ const ( workspaceNameLabel = "notebooks.kubeflow.org/workspace-name" workspaceSelectorLabel = "statefulset" - // KubeBuilder cache fields - kfCacheEventInvolvedObjectUidKey = ".involvedObject.uid" - kbCacheWorkspaceOwnerKey = ".metadata.controller" - kbCacheWorkspaceKindField = ".spec.kind" - // lengths for resource names generateNameSuffixLength = 6 maxServiceNameLength = 63 @@ -82,10 +78,6 @@ const ( stateMsgUnknown = "Workspace is in an unknown state" ) -var ( - apiGroupVersionStr = kubefloworgv1beta1.GroupVersion.String() -) - // WorkspaceReconciler reconciles a Workspace object type WorkspaceReconciler struct { client.Client @@ -145,8 +137,8 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // add finalizer to WorkspaceKind // NOTE: finalizers can only be added to non-deleted objects if workspaceKind.GetDeletionTimestamp().IsZero() { - if !controllerutil.ContainsFinalizer(workspaceKind, workspaceKindFinalizer) { - controllerutil.AddFinalizer(workspaceKind, workspaceKindFinalizer) + if !controllerutil.ContainsFinalizer(workspaceKind, WorkspaceKindFinalizer) { + controllerutil.AddFinalizer(workspaceKind, WorkspaceKindFinalizer) if err := r.Update(ctx, workspaceKind); err != nil { if apierrors.IsConflict(err) { log.V(2).Info("update conflict while adding finalizer to WorkspaceKind, will requeue") @@ -243,7 +235,7 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( var statefulSetName string ownedStatefulSets := &appsv1.StatefulSetList{} listOpts := &client.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(kbCacheWorkspaceOwnerKey, workspace.Name), + FieldSelector: fields.OneTermEqualSelector(helper.IndexWorkspaceOwnerField, workspace.Name), Namespace: req.Namespace, } if err := r.List(ctx, ownedStatefulSets, listOpts); err != nil { @@ -307,7 +299,7 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( var serviceName string ownedServices := &corev1.ServiceList{} listOpts = &client.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(kbCacheWorkspaceOwnerKey, workspace.Name), + FieldSelector: fields.OneTermEqualSelector(helper.IndexWorkspaceOwnerField, workspace.Name), Namespace: req.Namespace, } if err := r.List(ctx, ownedServices, listOpts); err != nil { @@ -390,57 +382,9 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // SetupWithManager sets up the controller with the Manager. func (r *WorkspaceReconciler) SetupWithManager(mgr ctrl.Manager) error { - // Index Event by `involvedObject.uid` - if err := mgr.GetFieldIndexer().IndexField(context.Background(), &corev1.Event{}, kfCacheEventInvolvedObjectUidKey, func(rawObj client.Object) []string { - event := rawObj.(*corev1.Event) - if event.InvolvedObject.UID == "" { - return nil - } - return []string{string(event.InvolvedObject.UID)} - }); err != nil { - return err - } - // Index StatefulSet by owner - if err := mgr.GetFieldIndexer().IndexField(context.Background(), &appsv1.StatefulSet{}, kbCacheWorkspaceOwnerKey, func(rawObj client.Object) []string { - statefulSet := rawObj.(*appsv1.StatefulSet) - owner := metav1.GetControllerOf(statefulSet) - if owner == nil { - return nil - } - if owner.APIVersion != apiGroupVersionStr || owner.Kind != "Workspace" { - return nil - } - return []string{owner.Name} - }); err != nil { - return err - } - - // Index Service by owner - if err := mgr.GetFieldIndexer().IndexField(context.Background(), &corev1.Service{}, kbCacheWorkspaceOwnerKey, func(rawObj client.Object) []string { - service := rawObj.(*corev1.Service) - owner := metav1.GetControllerOf(service) - if owner == nil { - return nil - } - if owner.APIVersion != apiGroupVersionStr || owner.Kind != "Workspace" { - return nil - } - return []string{owner.Name} - }); err != nil { - return err - } - - // Index Workspace by WorkspaceKind - if err := mgr.GetFieldIndexer().IndexField(context.Background(), &kubefloworgv1beta1.Workspace{}, kbCacheWorkspaceKindField, func(rawObj client.Object) []string { - ws := rawObj.(*kubefloworgv1beta1.Workspace) - if ws.Spec.Kind == "" { - return nil - } - return []string{ws.Spec.Kind} - }); err != nil { - return err - } + // NOTE: the SetupManagerFieldIndexers() helper in `helper/index.go` should have already been + // called on `mgr` by the time this function is called, so the indexes are already set up // function to convert pod events to reconcile requests for workspaces mapPodToRequest := func(ctx context.Context, object client.Object) []reconcile.Request { @@ -501,7 +445,7 @@ func (r *WorkspaceReconciler) updateWorkspaceState(ctx context.Context, log logr func (r *WorkspaceReconciler) mapWorkspaceKindToRequest(ctx context.Context, workspaceKind client.Object) []reconcile.Request { attachedWorkspaces := &kubefloworgv1beta1.WorkspaceList{} listOps := &client.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(kbCacheWorkspaceKindField, workspaceKind.GetName()), + FieldSelector: fields.OneTermEqualSelector(helper.IndexWorkspaceKindField, workspaceKind.GetName()), Namespace: "", // fetch Workspaces in all namespaces } err := r.List(ctx, attachedWorkspaces, listOps) @@ -532,7 +476,7 @@ func getImageConfig(workspace *kubefloworgv1beta1.Workspace, workspaceKind *kube currentImageConfigKey := workspace.Spec.PodTemplate.Options.ImageConfig currentImageConfig, ok := imageConfigIdMap[currentImageConfigKey] if !ok { - return nil, nil, nil, fmt.Errorf("imageConfig with id '%s' not found", currentImageConfigKey) + return nil, nil, nil, fmt.Errorf("imageConfig with id %q not found", currentImageConfigKey) } // follow any redirects to get the desired imageConfig @@ -544,11 +488,11 @@ func getImageConfig(workspace *kubefloworgv1beta1.Workspace, workspaceKind *kube break } if visitedNodes[desiredImageConfig.Redirect.To] { - return nil, nil, nil, fmt.Errorf("imageConfig with id '%s' has a circular redirect", desiredImageConfig.Id) + return nil, nil, nil, fmt.Errorf("imageConfig with id %q has a circular redirect", desiredImageConfig.Id) } nextNode, ok := imageConfigIdMap[desiredImageConfig.Redirect.To] if !ok { - return nil, nil, nil, fmt.Errorf("imageConfig with id '%s' not found, was redirected from '%s'", desiredImageConfig.Redirect.To, desiredImageConfig.Id) + return nil, nil, nil, fmt.Errorf("imageConfig with id %q not found, was redirected from %q", desiredImageConfig.Redirect.To, desiredImageConfig.Id) } redirectChain = append(redirectChain, kubefloworgv1beta1.WorkspacePodOptionRedirectStep{ Source: desiredImageConfig.Id, @@ -577,7 +521,7 @@ func getPodConfig(workspace *kubefloworgv1beta1.Workspace, workspaceKind *kubefl currentPodConfigKey := workspace.Spec.PodTemplate.Options.PodConfig currentPodConfig, ok := podConfigIdMap[currentPodConfigKey] if !ok { - return nil, nil, nil, fmt.Errorf("podConfig with id '%s' not found", currentPodConfigKey) + return nil, nil, nil, fmt.Errorf("podConfig with id %q not found", currentPodConfigKey) } // follow any redirects to get the desired podConfig @@ -589,11 +533,11 @@ func getPodConfig(workspace *kubefloworgv1beta1.Workspace, workspaceKind *kubefl break } if visitedNodes[desiredPodConfig.Redirect.To] { - return nil, nil, nil, fmt.Errorf("podConfig with id '%s' has a circular redirect", desiredPodConfig.Id) + return nil, nil, nil, fmt.Errorf("podConfig with id %q has a circular redirect", desiredPodConfig.Id) } nextNode, ok := podConfigIdMap[desiredPodConfig.Redirect.To] if !ok { - return nil, nil, nil, fmt.Errorf("podConfig with id '%s' not found, was redirected from '%s'", desiredPodConfig.Redirect.To, desiredPodConfig.Id) + return nil, nil, nil, fmt.Errorf("podConfig with id %q not found, was redirected from %q", desiredPodConfig.Redirect.To, desiredPodConfig.Id) } redirectChain = append(redirectChain, kubefloworgv1beta1.WorkspacePodOptionRedirectStep{ Source: desiredPodConfig.Id, @@ -678,9 +622,18 @@ func generateStatefulSet(workspace *kubefloworgv1beta1.Workspace, workspaceKind } // generate container env - containerEnv, err := kubefloworgv1beta1.RenderAndValidateExtraEnv(workspaceKind.Spec.PodTemplate.ExtraEnv, httpPathPrefixFunc, true) - if err != nil { - return nil, err + containerEnv := make([]corev1.EnvVar, len(workspaceKind.Spec.PodTemplate.ExtraEnv)) + for i, env := range workspaceKind.Spec.PodTemplate.ExtraEnv { + env := env.DeepCopy() // copy to avoid modifying the original + if env.Value != "" { + rawValue := env.Value + outValue, err := helper.RenderExtraEnvValueTemplate(rawValue, httpPathPrefixFunc) + if err != nil { + return nil, fmt.Errorf("failed to render extraEnv %q: %w", env.Name, err) + } + env.Value = outValue + } + containerEnv[i] = *env } // generate container resources @@ -924,7 +877,7 @@ func (r *WorkspaceReconciler) generateWorkspaceStatus(ctx context.Context, log l // there might be StatefulSet events statefulSetEvents := &corev1.EventList{} listOpts := &client.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(kfCacheEventInvolvedObjectUidKey, string(statefulSet.UID)), + FieldSelector: fields.OneTermEqualSelector(helper.IndexEventInvolvedObjectUidField, string(statefulSet.UID)), Namespace: statefulSet.Namespace, } if err := r.List(ctx, statefulSetEvents, listOpts); err != nil { diff --git a/workspaces/controller/internal/controller/workspacekind_controller.go b/workspaces/controller/internal/controller/workspacekind_controller.go index 996010b1..e6cd5c3b 100644 --- a/workspaces/controller/internal/controller/workspacekind_controller.go +++ b/workspaces/controller/internal/controller/workspacekind_controller.go @@ -35,10 +35,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" ) const ( - workspaceKindFinalizer = "notebooks.kubeflow.org/workspacekind-protection" + WorkspaceKindFinalizer = "notebooks.kubeflow.org/workspacekind-protection" ) // WorkspaceKindReconciler reconciles a WorkspaceKind object @@ -73,7 +74,7 @@ func (r *WorkspaceKindReconciler) Reconcile(ctx context.Context, req ctrl.Reques // fetch all Workspaces that are using this WorkspaceKind workspaces := &kubefloworgv1beta1.WorkspaceList{} listOpts := &client.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(kbCacheWorkspaceKindField, workspaceKind.Name), + FieldSelector: fields.OneTermEqualSelector(helper.IndexWorkspaceKindField, workspaceKind.Name), Namespace: "", // fetch Workspaces in all namespaces } if err := r.List(ctx, workspaces, listOpts); err != nil { @@ -84,8 +85,8 @@ func (r *WorkspaceKindReconciler) Reconcile(ctx context.Context, req ctrl.Reques // if no Workspaces are using this WorkspaceKind, remove the finalizer numWorkspace := len(workspaces.Items) if numWorkspace == 0 { - if controllerutil.ContainsFinalizer(workspaceKind, workspaceKindFinalizer) { - controllerutil.RemoveFinalizer(workspaceKind, workspaceKindFinalizer) + if controllerutil.ContainsFinalizer(workspaceKind, WorkspaceKindFinalizer) { + controllerutil.RemoveFinalizer(workspaceKind, WorkspaceKindFinalizer) if err := r.Update(ctx, workspaceKind); err != nil { if apierrors.IsConflict(err) { log.V(2).Info("update conflict while removing finalizer from WorkspaceKind, will requeue") @@ -146,10 +147,8 @@ func (r *WorkspaceKindReconciler) Reconcile(ctx context.Context, req ctrl.Reques // SetupWithManager sets up the controller with the Manager. func (r *WorkspaceKindReconciler) SetupWithManager(mgr ctrl.Manager) error { - // Index Workspace by WorkspaceKind - // NOTE: the Workspace index is defined in the SetupWithManager function of the WorkspaceReconciler. - // these controllers always share a manager (in both `main.go` and `suite_test.go`), - // so initializing the same index twice would result in a conflict. + // NOTE: the SetupManagerFieldIndexers() helper in `helper/index.go` should have already been + // called on `mgr` by the time this function is called, so the indexes are already set up // function to convert Workspace events to reconcile requests for WorkspaceKinds mapWorkspaceToRequest := func(ctx context.Context, object client.Object) []reconcile.Request { diff --git a/workspaces/controller/internal/controller/workspacekind_controller_test.go b/workspaces/controller/internal/controller/workspacekind_controller_test.go index dc062fd9..59e7dd15 100644 --- a/workspaces/controller/internal/controller/workspacekind_controller_test.go +++ b/workspaces/controller/internal/controller/workspacekind_controller_test.go @@ -203,7 +203,7 @@ var _ = Describe("WorkspaceKind Controller", func() { }, timeout, interval).Should(Equal(expectedStatus)) By("having a finalizer set on the WorkspaceKind") - Expect(workspaceKind.GetFinalizers()).To(ContainElement(workspaceKindFinalizer)) + Expect(workspaceKind.GetFinalizers()).To(ContainElement(WorkspaceKindFinalizer)) By("deleting the Workspace") Expect(k8sClient.Delete(ctx, workspace)).To(Succeed()) @@ -250,7 +250,7 @@ var _ = Describe("WorkspaceKind Controller", func() { By("deleting the WorkspaceKind") Expect(k8sClient.Delete(ctx, workspaceKind)).To(Succeed()) - Expect(k8sClient.Get(ctx, workspaceKindKey, workspaceKind)).ToNot(Succeed()) + Expect(k8sClient.Get(ctx, workspaceKindKey, workspaceKind)).NotTo(Succeed()) }) }) }) diff --git a/workspaces/controller/internal/helper/graph.go b/workspaces/controller/internal/helper/graph.go new file mode 100644 index 00000000..2cea2a5f --- /dev/null +++ b/workspaces/controller/internal/helper/graph.go @@ -0,0 +1,55 @@ +package helper + +// DetectGraphCycle checks if there is a cycle involving a given node in directed graph +// +// Assumptions: +// - all nodes have AT MOST one OUTGOING edge +// +// Parameters: +// - startNode: the node to start the cycle detection from +// - checkedNodes: a map of nodes which have already been checked for cycles (updated in-place if no cycle is detected) +// - edgeMap: a map representing the edges in the graph +// +// Returns: +// - returns nil, if no cycle is detected +// - returns a slice of nodes representing the cycle, if a cycle is detected +func DetectGraphCycle(startNode string, checkedNodes map[string]bool, edgeMap map[string]string) []string { + currentPath := make([]string, 0) + currentPathNodes := make(map[string]bool) + var currentNode = startNode + for { + // if the current node has already been checked, no cycle is detected + if checkedNodes[currentNode] { + break + } + + // if the current node is already in the current path, a cycle is detected + if currentPathNodes[currentNode] { + for i, node := range currentPath { + // the cycle starts from the first occurrence of the current node + if node == currentNode { + return currentPath[i:] + } + } + } + + // add the current node to the current path + currentPath = append(currentPath, currentNode) + currentPathNodes[currentNode] = true + + // get the next node + nextNode, exists := edgeMap[currentNode] + if !exists { + // if there is no outgoing edge, no cycle is detected + break + } + currentNode = nextNode + } + + // mark all nodes in the current path as checked + for node := range currentPathNodes { + checkedNodes[node] = true + } + + return nil +} diff --git a/workspaces/controller/internal/helper/graph_test.go b/workspaces/controller/internal/helper/graph_test.go new file mode 100644 index 00000000..2bfed2f2 --- /dev/null +++ b/workspaces/controller/internal/helper/graph_test.go @@ -0,0 +1,110 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package helper + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("DetectGraphCycle", func() { + + It("should detect a simple cycle", func() { + startNode := "A" + checkedNodes := map[string]bool{} + edgeMap := map[string]string{"A": "B", "B": "C", "C": "A"} + + result := DetectGraphCycle(startNode, checkedNodes, edgeMap) + Expect(result).To(Equal([]string{"A", "B", "C"})) + }) + + It("should return nil for no cycle", func() { + startNode := "A" + checkedNodes := map[string]bool{} + edgeMap := map[string]string{"A": "B", "B": "C", "C": "D"} + + result := DetectGraphCycle(startNode, checkedNodes, edgeMap) + Expect(result).To(BeNil()) + Expect(checkedNodes).To(Equal(map[string]bool{"A": true, "B": true, "C": true, "D": true})) + }) + + It("should detect a self-loop cycle", func() { + startNode := "A" + checkedNodes := map[string]bool{} + edgeMap := map[string]string{"A": "A"} + + result := DetectGraphCycle(startNode, checkedNodes, edgeMap) + Expect(result).To(Equal([]string{"A"})) + }) + + It("should detect a cycle and ignore unconnected nodes", func() { + startNode := "A" + checkedNodes := map[string]bool{} + edgeMap := map[string]string{"A": "B", "B": "C", "C": "A", "D": "E"} + + result := DetectGraphCycle(startNode, checkedNodes, edgeMap) + Expect(result).To(Equal([]string{"A", "B", "C"})) + }) + + It("should detect cycles starting from different nodes in a complex graph", func() { + startNode := "A" + checkedNodes := map[string]bool{} + edgeMap := map[string]string{"A": "B", "B": "C", "C": "D", "D": "B", "E": "F"} + + result := DetectGraphCycle(startNode, checkedNodes, edgeMap) + Expect(result).To(Equal([]string{"B", "C", "D"})) + + startNode = "E" + result = DetectGraphCycle(startNode, checkedNodes, edgeMap) + Expect(result).To(BeNil()) + Expect(checkedNodes).To(Equal(map[string]bool{"E": true, "F": true})) + }) + + It("should detect cycles in a graph with multiple components", func() { + startNode := "X" + checkedNodes := map[string]bool{} + edgeMap := map[string]string{"A": "B", "B": "C", "C": "D", "D": "B", "X": "Y", "Y": "Z"} + + result := DetectGraphCycle(startNode, checkedNodes, edgeMap) + Expect(result).To(BeNil()) + Expect(checkedNodes).To(Equal(map[string]bool{"X": true, "Y": true, "Z": true})) + + startNode = "A" + result = DetectGraphCycle(startNode, checkedNodes, edgeMap) + Expect(result).To(Equal([]string{"B", "C", "D"})) + }) + + It("should return nil when starting from a node with no outgoing edge", func() { + startNode := "Z" + checkedNodes := map[string]bool{} + edgeMap := map[string]string{"A": "B", "B": "C", "C": "D", "D": "B", "X": "Y"} + + result := DetectGraphCycle(startNode, checkedNodes, edgeMap) + Expect(result).To(BeNil()) + Expect(checkedNodes).To(Equal(map[string]bool{"Z": true})) + }) + + It("should return nil when the start node has already been checked", func() { + startNode := "A" + checkedNodes := map[string]bool{"A": true, "B": true} + edgeMap := map[string]string{"A": "B", "B": "C", "C": "D", "D": "B"} + + result := DetectGraphCycle(startNode, checkedNodes, edgeMap) + Expect(result).To(BeNil()) + Expect(checkedNodes).To(Equal(map[string]bool{"A": true, "B": true})) + }) +}) diff --git a/workspaces/controller/internal/helper/index.go b/workspaces/controller/internal/helper/index.go new file mode 100644 index 00000000..22c24c00 --- /dev/null +++ b/workspaces/controller/internal/helper/index.go @@ -0,0 +1,77 @@ +package helper + +import ( + "context" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" +) + +const ( + IndexEventInvolvedObjectUidField = ".involvedObject.uid" + IndexWorkspaceOwnerField = ".metadata.controller" + IndexWorkspaceKindField = ".spec.kind" +) + +// SetupManagerFieldIndexers sets up field indexes on a controller-runtime manager +func SetupManagerFieldIndexers(mgr ctrl.Manager) error { + + // Index Event by `involvedObject.uid` + if err := mgr.GetFieldIndexer().IndexField(context.Background(), &corev1.Event{}, IndexEventInvolvedObjectUidField, func(rawObj client.Object) []string { + event := rawObj.(*corev1.Event) + if event.InvolvedObject.UID == "" { + return nil + } + return []string{string(event.InvolvedObject.UID)} + }); err != nil { + return err + } + + // Index StatefulSet by its owner Workspace + if err := mgr.GetFieldIndexer().IndexField(context.Background(), &appsv1.StatefulSet{}, IndexWorkspaceOwnerField, func(rawObj client.Object) []string { + statefulSet := rawObj.(*appsv1.StatefulSet) + owner := metav1.GetControllerOf(statefulSet) + if owner == nil { + return nil + } + if owner.APIVersion != kubefloworgv1beta1.GroupVersion.String() || owner.Kind != "Workspace" { + return nil + } + return []string{owner.Name} + }); err != nil { + return err + } + + // Index Service by its owner Workspace + if err := mgr.GetFieldIndexer().IndexField(context.Background(), &corev1.Service{}, IndexWorkspaceOwnerField, func(rawObj client.Object) []string { + service := rawObj.(*corev1.Service) + owner := metav1.GetControllerOf(service) + if owner == nil { + return nil + } + if owner.APIVersion != kubefloworgv1beta1.GroupVersion.String() || owner.Kind != "Workspace" { + return nil + } + return []string{owner.Name} + }); err != nil { + return err + } + + // Index Workspace by WorkspaceKind + if err := mgr.GetFieldIndexer().IndexField(context.Background(), &kubefloworgv1beta1.Workspace{}, IndexWorkspaceKindField, func(rawObj client.Object) []string { + ws := rawObj.(*kubefloworgv1beta1.Workspace) + if ws.Spec.Kind == "" { + return nil + } + return []string{ws.Spec.Kind} + }); err != nil { + return err + } + + return nil +} diff --git a/workspaces/controller/internal/helper/suite_test.go b/workspaces/controller/internal/helper/suite_test.go new file mode 100644 index 00000000..0aaabeee --- /dev/null +++ b/workspaces/controller/internal/helper/suite_test.go @@ -0,0 +1,39 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package helper + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +// These tests use Ginkgo (BDD-style Go testing framework). Refer to +// http://onsi.github.io/ginkgo/ to learn more about Ginkgo. + +func TestHelpers(t *testing.T) { + RegisterFailHandler(Fail) + + RunSpecs(t, "Helpers Suite") +} + +var _ = BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) +}) diff --git a/workspaces/controller/internal/helper/template.go b/workspaces/controller/internal/helper/template.go new file mode 100644 index 00000000..57d829a6 --- /dev/null +++ b/workspaces/controller/internal/helper/template.go @@ -0,0 +1,30 @@ +package helper + +import ( + "bytes" + "fmt" + "text/template" +) + +// RenderExtraEnvValueTemplate renders a single WorkspaceKind `spec.podTemplate.extraEnv[].value` string template +func RenderExtraEnvValueTemplate(rawValue string, httpPathPrefixFunc func(string) string) (string, error) { + + // Parse the raw value as a template + tmpl, err := template.New("value"). + Funcs(template.FuncMap{"httpPathPrefix": httpPathPrefixFunc}). + Parse(rawValue) + if err != nil { + err = fmt.Errorf("failed to parse template %q: %w", rawValue, err) + return "", err + } + + // Execute the template + var buf bytes.Buffer + err = tmpl.Execute(&buf, nil) + if err != nil { + err = fmt.Errorf("failed to execute template %q: %w", rawValue, err) + return "", err + } + + return buf.String(), nil +} diff --git a/workspaces/controller/api/v1beta1/webhook_suite_test.go b/workspaces/controller/internal/webhook/suite_test.go similarity index 69% rename from workspaces/controller/api/v1beta1/webhook_suite_test.go rename to workspaces/controller/internal/webhook/suite_test.go index 4885c0d5..1c480b13 100644 --- a/workspaces/controller/api/v1beta1/webhook_suite_test.go +++ b/workspaces/controller/internal/webhook/suite_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package webhook import ( "context" @@ -34,9 +34,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - admissionv1 "k8s.io/api/admission/v1" - //+kubebuilder:scaffold:imports - apimachineryruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -45,17 +43,23 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" "sigs.k8s.io/controller-runtime/pkg/webhook" + + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "github.com/kubeflow/notebooks/workspaces/controller/internal/helper" + //+kubebuilder:scaffold:imports ) // These tests use Ginkgo (BDD-style Go testing framework). Refer to // http://onsi.github.io/ginkgo/ to learn more about Ginkgo. var ( - cfg *rest.Config - k8sTestClient client.Client - testEnv *envtest.Environment - ctx context.Context - cancel context.CancelFunc + testEnv *envtest.Environment + cfg *rest.Config + + k8sClient client.Client + + ctx context.Context + cancel context.CancelFunc ) func TestAPIs(t *testing.T) { @@ -66,85 +70,82 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) - - ctx, cancel = context.WithCancel(context.TODO()) + ctx, cancel = context.WithCancel(context.Background()) By("bootstrapping test environment") testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, - ErrorIfCRDPathMissing: false, + ErrorIfCRDPathMissing: true, - // The BinaryAssetsDirectory is only required if you want to run the tests directly - // without call the makefile target test. If not informed it will look for the - // default path defined in controller-runtime which is /usr/local/kubebuilder/. - // Note that you must have the required binaries setup under the bin directory to perform - // the tests directly. When we run make test it will be setup and used automatically. - BinaryAssetsDirectory: filepath.Join("..", "..", "bin", "k8s", - fmt.Sprintf("1.29.0-%s-%s", runtime.GOOS, runtime.GOARCH)), + // The BinaryAssetsDirectory is only required if you want to run the tests directly without call the makefile target test. + // If not informed it will look for the default path defined in controller-runtime which is /usr/local/kubebuilder/. + // Note that you must have the required binaries setup under the bin directory to perform the tests directly. + // When we run make test it will be setup and used automatically. + BinaryAssetsDirectory: filepath.Join("..", "..", "bin", "k8s", fmt.Sprintf("1.29.0-%s-%s", runtime.GOOS, runtime.GOARCH)), WebhookInstallOptions: envtest.WebhookInstallOptions{ Paths: []string{filepath.Join("..", "..", "config", "webhook")}, }, } - var err error - // cfg is defined in this file globally. cfg, err = testEnv.Start() Expect(err).NotTo(HaveOccurred()) Expect(cfg).NotTo(BeNil()) - scheme := apimachineryruntime.NewScheme() - err = AddToScheme(scheme) - Expect(err).NotTo(HaveOccurred()) - - err = admissionv1.AddToScheme(scheme) + By("setting up the scheme") + err = kubefloworgv1beta1.AddToScheme(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) //+kubebuilder:scaffold:scheme - k8sTestClient, err = client.New(cfg, client.Options{Scheme: scheme}) + By("creating the k8s client") + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) Expect(err).NotTo(HaveOccurred()) - Expect(k8sTestClient).NotTo(BeNil()) + Expect(k8sClient).NotTo(BeNil()) - // start webhook server using Manager + By("setting up the controller manager") webhookInstallOptions := &testEnv.WebhookInstallOptions - mgr, err := ctrl.NewManager(cfg, ctrl.Options{ - Scheme: scheme, + k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: scheme.Scheme, + Metrics: metricsserver.Options{ + BindAddress: "0", // disable metrics serving + }, WebhookServer: webhook.NewServer(webhook.Options{ Host: webhookInstallOptions.LocalServingHost, Port: webhookInstallOptions.LocalServingPort, CertDir: webhookInstallOptions.LocalServingCertDir, }), LeaderElection: false, - Metrics: metricsserver.Options{BindAddress: "0"}, }) Expect(err).NotTo(HaveOccurred()) - err = (&WorkspaceKind{}).SetupWebhookWithManager(mgr) + By("setting up the field indexers for the controller manager") + err = helper.SetupManagerFieldIndexers(k8sManager) Expect(err).NotTo(HaveOccurred()) - // Indexing `.spec.kind` here, not in SetupWebhookWithManager, to avoid conflicts with existing indexing. - // This indexing is specifically for testing purposes to index `Workspace` by `WorkspaceKind`. - err = mgr.GetFieldIndexer().IndexField(context.Background(), &Workspace{}, kbCacheWorkspaceKindField, func(rawObj client.Object) []string { - ws := rawObj.(*Workspace) - if ws.Spec.Kind == "" { - return nil - } - return []string{ws.Spec.Kind} - }) + By("setting up the Workspace webhook") + err = (&WorkspaceValidator{ + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + }).SetupWebhookWithManager(k8sManager) Expect(err).NotTo(HaveOccurred()) - err = (&Workspace{}).SetupWebhookWithManager(mgr) + + By("setting up the WorkspaceKind webhook") + err = (&WorkspaceKindValidator{ + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + }).SetupWebhookWithManager(k8sManager) Expect(err).NotTo(HaveOccurred()) //+kubebuilder:scaffold:webhook go func() { defer GinkgoRecover() - err = mgr.Start(ctx) - Expect(err).NotTo(HaveOccurred()) + err = k8sManager.Start(ctx) + Expect(err).NotTo(HaveOccurred(), "failed to run manager") }() - // wait for the webhook server to get ready + // wait for the webhook server to become ready dialer := &net.Dialer{Timeout: time.Second} addrPort := fmt.Sprintf("%s:%d", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort) Eventually(func() error { @@ -158,56 +159,58 @@ var _ = BeforeSuite(func() { }) var _ = AfterSuite(func() { + By("stopping the manager") cancel() + By("tearing down the test environment") err := testEnv.Stop() Expect(err).NotTo(HaveOccurred()) }) // NewExampleWorkspaceKind returns the common "WorkspaceKind" object used in tests. -func NewExampleWorkspaceKind(name string) *WorkspaceKind { - return &WorkspaceKind{ +func NewExampleWorkspaceKind(name string) *kubefloworgv1beta1.WorkspaceKind { + return &kubefloworgv1beta1.WorkspaceKind{ ObjectMeta: metav1.ObjectMeta{ Name: name, }, - Spec: WorkspaceKindSpec{ - Spawner: WorkspaceKindSpawner{ + Spec: kubefloworgv1beta1.WorkspaceKindSpec{ + Spawner: kubefloworgv1beta1.WorkspaceKindSpawner{ DisplayName: "JupyterLab Notebook", Description: "A Workspace which runs JupyterLab in a Pod", Hidden: ptr.To(false), Deprecated: ptr.To(false), DeprecationMessage: ptr.To("This WorkspaceKind will be removed on 20XX-XX-XX, please use another WorkspaceKind."), - Icon: WorkspaceKindIcon{ + Icon: kubefloworgv1beta1.WorkspaceKindIcon{ Url: ptr.To("https://jupyter.org/assets/favicons/apple-touch-icon-152x152.png"), }, - Logo: WorkspaceKindIcon{ - ConfigMap: &WorkspaceKindConfigMap{ + Logo: kubefloworgv1beta1.WorkspaceKindIcon{ + ConfigMap: &kubefloworgv1beta1.WorkspaceKindConfigMap{ Name: "my-logos", Key: "apple-touch-icon-152x152.png", }, }, }, - PodTemplate: WorkspaceKindPodTemplate{ - PodMetadata: &WorkspaceKindPodMetadata{}, - ServiceAccount: WorkspaceKindServiceAccount{ + PodTemplate: kubefloworgv1beta1.WorkspaceKindPodTemplate{ + PodMetadata: &kubefloworgv1beta1.WorkspaceKindPodMetadata{}, + ServiceAccount: kubefloworgv1beta1.WorkspaceKindServiceAccount{ Name: "default-editor", }, - Culling: &WorkspaceKindCullingConfig{ + Culling: &kubefloworgv1beta1.WorkspaceKindCullingConfig{ Enabled: ptr.To(true), MaxInactiveSeconds: ptr.To(int32(86400)), - ActivityProbe: ActivityProbe{ - Jupyter: &ActivityProbeJupyter{ + ActivityProbe: kubefloworgv1beta1.ActivityProbe{ + Jupyter: &kubefloworgv1beta1.ActivityProbeJupyter{ LastActivity: true, }, }, }, - Probes: &WorkspaceKindProbes{}, - VolumeMounts: WorkspaceKindVolumeMounts{ + Probes: &kubefloworgv1beta1.WorkspaceKindProbes{}, + VolumeMounts: kubefloworgv1beta1.WorkspaceKindVolumeMounts{ Home: "/home/jovyan", }, - HTTPProxy: &HTTPProxy{ + HTTPProxy: &kubefloworgv1beta1.HTTPProxy{ RemovePathPrefix: ptr.To(false), - RequestHeaders: &IstioHeaderOperations{ + RequestHeaders: &kubefloworgv1beta1.IstioHeaderOperations{ Set: map[string]string{"X-RStudio-Root-Path": "{{ .PathPrefix }}"}, Add: map[string]string{}, Remove: []string{}, @@ -245,18 +248,18 @@ func NewExampleWorkspaceKind(name string) *WorkspaceKind { }, RunAsNonRoot: ptr.To(true), }, - Options: WorkspaceKindPodOptions{ - ImageConfig: ImageConfig{ - Spawner: OptionsSpawnerConfig{ + Options: kubefloworgv1beta1.WorkspaceKindPodOptions{ + ImageConfig: kubefloworgv1beta1.ImageConfig{ + Spawner: kubefloworgv1beta1.OptionsSpawnerConfig{ Default: "jupyterlab_scipy_190", }, - Values: []ImageConfigValue{ + Values: []kubefloworgv1beta1.ImageConfigValue{ { Id: "jupyterlab_scipy_180", - Spawner: OptionSpawnerInfo{ + Spawner: kubefloworgv1beta1.OptionSpawnerInfo{ DisplayName: "jupyter-scipy:v1.8.0", Description: ptr.To("JupyterLab, with SciPy Packages"), - Labels: []OptionSpawnerLabel{ + Labels: []kubefloworgv1beta1.OptionSpawnerLabel{ { Key: "python_version", Value: "3.11", @@ -264,16 +267,16 @@ func NewExampleWorkspaceKind(name string) *WorkspaceKind { }, Hidden: ptr.To(true), }, - Redirect: &OptionRedirect{ + Redirect: &kubefloworgv1beta1.OptionRedirect{ To: "jupyterlab_scipy_190", - Message: &RedirectMessage{ + Message: &kubefloworgv1beta1.RedirectMessage{ Level: "Info", Text: "This update will change...", }, }, - Spec: ImageConfigSpec{ + Spec: kubefloworgv1beta1.ImageConfigSpec{ Image: "docker.io/kubeflownotebookswg/jupyter-scipy:v1.8.0", - Ports: []ImagePort{ + Ports: []kubefloworgv1beta1.ImagePort{ { Id: "jupyterlab", DisplayName: "JupyterLab", @@ -285,19 +288,19 @@ func NewExampleWorkspaceKind(name string) *WorkspaceKind { }, { Id: "jupyterlab_scipy_190", - Spawner: OptionSpawnerInfo{ + Spawner: kubefloworgv1beta1.OptionSpawnerInfo{ DisplayName: "jupyter-scipy:v1.9.0", Description: ptr.To("JupyterLab, with SciPy Packages"), - Labels: []OptionSpawnerLabel{ + Labels: []kubefloworgv1beta1.OptionSpawnerLabel{ { Key: "python_version", Value: "3.11", }, }, }, - Spec: ImageConfigSpec{ + Spec: kubefloworgv1beta1.ImageConfigSpec{ Image: "docker.io/kubeflownotebookswg/jupyter-scipy:v1.9.0", - Ports: []ImagePort{ + Ports: []kubefloworgv1beta1.ImagePort{ { Id: "jupyterlab", DisplayName: "JupyterLab", @@ -309,17 +312,17 @@ func NewExampleWorkspaceKind(name string) *WorkspaceKind { }, }, }, - PodConfig: PodConfig{ - Spawner: OptionsSpawnerConfig{ + PodConfig: kubefloworgv1beta1.PodConfig{ + Spawner: kubefloworgv1beta1.OptionsSpawnerConfig{ Default: "tiny_cpu", }, - Values: []PodConfigValue{ + Values: []kubefloworgv1beta1.PodConfigValue{ { Id: "tiny_cpu", - Spawner: OptionSpawnerInfo{ + Spawner: kubefloworgv1beta1.OptionSpawnerInfo{ DisplayName: "Tiny CPU", Description: ptr.To("Pod with 0.1 CPU, 128 MB RAM"), - Labels: []OptionSpawnerLabel{ + Labels: []kubefloworgv1beta1.OptionSpawnerLabel{ { Key: "cpu", Value: "100m", @@ -330,7 +333,7 @@ func NewExampleWorkspaceKind(name string) *WorkspaceKind { }, }, }, - Spec: PodConfigSpec{ + Spec: kubefloworgv1beta1.PodConfigSpec{ Resources: &v1.ResourceRequirements{ Requests: map[v1.ResourceName]resource.Quantity{ v1.ResourceCPU: resource.MustParse("100m"), @@ -341,10 +344,10 @@ func NewExampleWorkspaceKind(name string) *WorkspaceKind { }, { Id: "small_cpu", - Spawner: OptionSpawnerInfo{ + Spawner: kubefloworgv1beta1.OptionSpawnerInfo{ DisplayName: "Small CPU", Description: ptr.To("Pod with 1 CPU, 2 GB RAM"), - Labels: []OptionSpawnerLabel{ + Labels: []kubefloworgv1beta1.OptionSpawnerLabel{ { Key: "cpu", Value: "1000m", @@ -355,7 +358,7 @@ func NewExampleWorkspaceKind(name string) *WorkspaceKind { }, }, }, - Spec: PodConfigSpec{ + Spec: kubefloworgv1beta1.PodConfigSpec{ Resources: &v1.ResourceRequirements{ Requests: map[v1.ResourceName]resource.Quantity{ v1.ResourceCPU: resource.MustParse("1000m"), @@ -366,10 +369,10 @@ func NewExampleWorkspaceKind(name string) *WorkspaceKind { }, { Id: "big_gpu", - Spawner: OptionSpawnerInfo{ + Spawner: kubefloworgv1beta1.OptionSpawnerInfo{ DisplayName: "Big GPU", Description: ptr.To("Pod with 4 CPU, 16 GB RAM, and 1 GPU"), - Labels: []OptionSpawnerLabel{ + Labels: []kubefloworgv1beta1.OptionSpawnerLabel{ { Key: "cpu", Value: "4000m", @@ -384,7 +387,7 @@ func NewExampleWorkspaceKind(name string) *WorkspaceKind { }, }, }, - Spec: PodConfigSpec{ + Spec: kubefloworgv1beta1.PodConfigSpec{ Affinity: nil, NodeSelector: nil, Tolerations: []v1.Toleration{ @@ -414,25 +417,25 @@ func NewExampleWorkspaceKind(name string) *WorkspaceKind { } // NewExampleWorkspaceKindWithImageConfigCycle returns a WorkspaceKind with a cycle in the ImageConfig options. -func NewExampleWorkspaceKindWithImageConfigCycle(name string) *WorkspaceKind { +func NewExampleWorkspaceKindWithImageConfigCycle(name string) *kubefloworgv1beta1.WorkspaceKind { workspaceKind := NewExampleWorkspaceKind(name) - workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &OptionRedirect{ + workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &kubefloworgv1beta1.OptionRedirect{ To: "jupyterlab_scipy_180", } return workspaceKind } // NewExampleWorkspaceKindWithPodConfigCycle returns a WorkspaceKind with a cycle in the PodConfig options. -func NewExampleWorkspaceKindWithPodConfigCycle(name string) *WorkspaceKind { +func NewExampleWorkspaceKindWithPodConfigCycle(name string) *kubefloworgv1beta1.WorkspaceKind { workspaceKind := NewExampleWorkspaceKind(name) - workspaceKind.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &OptionRedirect{ + workspaceKind.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &kubefloworgv1beta1.OptionRedirect{ To: "small_cpu", - Message: &RedirectMessage{ + Message: &kubefloworgv1beta1.RedirectMessage{ Level: "Info", Text: "This update will change...", }, } - workspaceKind.Spec.PodTemplate.Options.PodConfig.Values[1].Redirect = &OptionRedirect{ + workspaceKind.Spec.PodTemplate.Options.PodConfig.Values[1].Redirect = &kubefloworgv1beta1.OptionRedirect{ To: "tiny_cpu", } @@ -440,9 +443,9 @@ func NewExampleWorkspaceKindWithPodConfigCycle(name string) *WorkspaceKind { } // NewExampleWorkspaceKindWithInvalidImageConfig returns a WorkspaceKind with an invalid redirect in the ImageConfig options. -func NewExampleWorkspaceKindWithInvalidImageConfig(name string) *WorkspaceKind { +func NewExampleWorkspaceKindWithInvalidImageConfig(name string) *kubefloworgv1beta1.WorkspaceKind { workspaceKind := NewExampleWorkspaceKind(name) - workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &OptionRedirect{ + workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &kubefloworgv1beta1.OptionRedirect{ To: "invalid_image_config", } @@ -450,9 +453,9 @@ func NewExampleWorkspaceKindWithInvalidImageConfig(name string) *WorkspaceKind { } // NewExampleWorkspaceKindWithInvalidPodConfig returns a WorkspaceKind with an invalid redirect in the PodConfig options. -func NewExampleWorkspaceKindWithInvalidPodConfig(name string) *WorkspaceKind { +func NewExampleWorkspaceKindWithInvalidPodConfig(name string) *kubefloworgv1beta1.WorkspaceKind { workspaceKind := NewExampleWorkspaceKind(name) - workspaceKind.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &OptionRedirect{ + workspaceKind.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &kubefloworgv1beta1.OptionRedirect{ To: "invalid_pod_config", } @@ -460,23 +463,23 @@ func NewExampleWorkspaceKindWithInvalidPodConfig(name string) *WorkspaceKind { } // NewExampleWorkspaceKindWithMissingDefaultImageConfig returns a WorkspaceKind with missing default image config. -func NewExampleWorkspaceKindWithInvalidDefaultImageConfig(name string) *WorkspaceKind { +func NewExampleWorkspaceKindWithInvalidDefaultImageConfig(name string) *kubefloworgv1beta1.WorkspaceKind { workspaceKind := NewExampleWorkspaceKind(name) workspaceKind.Spec.PodTemplate.Options.ImageConfig.Spawner.Default = "invalid_image_config" return workspaceKind } // NewExampleWorkspaceKindWithMissingDefaultPodConfig returns a WorkspaceKind with missing default pod config. -func NewExampleWorkspaceKindWithInvalidDefaultPodConfig(name string) *WorkspaceKind { +func NewExampleWorkspaceKindWithInvalidDefaultPodConfig(name string) *kubefloworgv1beta1.WorkspaceKind { workspaceKind := NewExampleWorkspaceKind(name) workspaceKind.Spec.PodTemplate.Options.PodConfig.Spawner.Default = "invalid_pod_config" return workspaceKind } // NewExampleWorkspaceKindWithInvalidExtraEnvValue returns a WorkspaceKind with an invalid extraEnv value. -func NewExampleWorkspaceKindWithDuplicatePorts(name string) *WorkspaceKind { +func NewExampleWorkspaceKindWithDuplicatePorts(name string) *kubefloworgv1beta1.WorkspaceKind { workspaceKind := NewExampleWorkspaceKind(name) - workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values[0].Spec.Ports = []ImagePort{ + workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values[0].Spec.Ports = []kubefloworgv1beta1.ImagePort{ { Id: "jupyterlab", DisplayName: "JupyterLab", @@ -494,7 +497,7 @@ func NewExampleWorkspaceKindWithDuplicatePorts(name string) *WorkspaceKind { } // NewExampleWorkspaceKindWithInvalidExtraEnvValue returns a WorkspaceKind with an invalid extraEnv value. -func NewExampleWorkspaceKindWithInvalidExtraEnvValue(name string) *WorkspaceKind { +func NewExampleWorkspaceKindWithInvalidExtraEnvValue(name string) *kubefloworgv1beta1.WorkspaceKind { workspaceKind := NewExampleWorkspaceKind(name) workspaceKind.Spec.PodTemplate.ExtraEnv = []v1.EnvVar{ { @@ -506,15 +509,15 @@ func NewExampleWorkspaceKindWithInvalidExtraEnvValue(name string) *WorkspaceKind } // NewExampleWorkspace returns the common "Workspace" object used in tests. -func NewExampleWorkspace(name, namespace, workspaceKindName string) *Workspace { - return &Workspace{ +func NewExampleWorkspace(name, namespace, workspaceKindName string) *kubefloworgv1beta1.Workspace { + return &kubefloworgv1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, }, - Spec: WorkspaceSpec{ + Spec: kubefloworgv1beta1.WorkspaceSpec{ Kind: workspaceKindName, - PodTemplate: WorkspacePodTemplate{Options: WorkspacePodOptions{ + PodTemplate: kubefloworgv1beta1.WorkspacePodTemplate{Options: kubefloworgv1beta1.WorkspacePodOptions{ ImageConfig: "jupyterlab_scipy_180", PodConfig: "tiny_cpu", }, diff --git a/workspaces/controller/internal/webhook/workspace_webhook.go b/workspaces/controller/internal/webhook/workspace_webhook.go new file mode 100644 index 00000000..7e8cfe16 --- /dev/null +++ b/workspaces/controller/internal/webhook/workspace_webhook.go @@ -0,0 +1,231 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package webhook + +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" + "k8s.io/apimachinery/pkg/util/validation/field" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// WorkspaceValidator validates a Workspace object +type WorkspaceValidator struct { + client.Client + Scheme *runtime.Scheme +} + +//+kubebuilder:webhook:path=/validate-kubeflow-org-v1beta1-workspace,mutating=false,failurePolicy=fail,sideEffects=None,groups=kubeflow.org,resources=workspaces,verbs=create;update,versions=v1beta1,name=vworkspace.kb.io,admissionReviewVersions=v1 + +// SetupWebhookWithManager sets up the webhook with the manager +func (v *WorkspaceValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr). + For(&kubefloworgv1beta1.Workspace{}). + WithValidator(v). + Complete() +} + +// ValidateCreate validates the Workspace on creation. +// The optional warnings will be added to the response as warning messages. +// Return an error if the object is invalid. +func (v *WorkspaceValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + 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)) + } + + // fetch the WorkspaceKind + workspaceKind, err := v.validateWorkspaceKind(ctx, workspace) + if err != nil { + allErrs = append(allErrs, err) + + // if the WorkspaceKind is not found, we cannot validate the Workspace further + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: kubefloworgv1beta1.GroupVersion.Group, Kind: "Workspace"}, + workspace.Name, + allErrs, + ) + } + + // validate the Workspace + if err := v.validateImageConfig(workspace, workspaceKind); err != nil { + allErrs = append(allErrs, err) + } + if err := v.validatePodConfig(workspace, workspaceKind); err != nil { + allErrs = append(allErrs, err) + } + if len(allErrs) == 0 { + return nil, nil + } + + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: kubefloworgv1beta1.GroupVersion.Group, Kind: "Workspace"}, + workspace.Name, + allErrs, + ) +} + +// ValidateUpdate validates the Workspace on update. +// The optional warnings will be added to the response as warning messages. +// Return an error if the object is invalid. +func (v *WorkspaceValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + 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)) + } + oldWorkspace, ok := oldObj.(*kubefloworgv1beta1.Workspace) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected old object to be a Workspace but got %T", oldObj)) + } + + // check if workspace kind related fields have changed + var workspaceKindChange = false + var imageConfigChange = false + var podConfigChange = false + if newWorkspace.Spec.Kind != oldWorkspace.Spec.Kind { + workspaceKindChange = true + } + if newWorkspace.Spec.PodTemplate.Options.ImageConfig != oldWorkspace.Spec.PodTemplate.Options.ImageConfig { + imageConfigChange = true + } + if newWorkspace.Spec.PodTemplate.Options.PodConfig != oldWorkspace.Spec.PodTemplate.Options.PodConfig { + podConfigChange = true + } + + // if any of the workspace kind related fields have changed, revalidate the workspace + if workspaceKindChange || imageConfigChange || podConfigChange { + // fetch the WorkspaceKind + workspaceKind, err := v.validateWorkspaceKind(ctx, newWorkspace) + if err != nil { + allErrs = append(allErrs, err) + + // if the WorkspaceKind is not found, we cannot validate the Workspace further + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: kubefloworgv1beta1.GroupVersion.Group, Kind: "Workspace"}, + newWorkspace.Name, + allErrs, + ) + } + + // validate the new imageConfig + if imageConfigChange { + if err := v.validateImageConfig(newWorkspace, workspaceKind); err != nil { + allErrs = append(allErrs, err) + } + } + + // validate the new podConfig + if podConfigChange { + if err := v.validatePodConfig(newWorkspace, workspaceKind); err != nil { + allErrs = append(allErrs, err) + } + } + } + + if len(allErrs) == 0 { + return nil, nil + } + + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: kubefloworgv1beta1.GroupVersion.Group, Kind: "Workspace"}, + newWorkspace.Name, + allErrs, + ) +} + +// ValidateDelete validates the Workspace on deletion. +// The optional warnings will be added to the response as warning messages. +// Return an error if the object is invalid. +func (v *WorkspaceValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + // no validation needed for deletion + // NOTE: add "delete" to the webhook configuration (+kubebuilder:webhook) if you want to enable deletion validation + return nil, nil +} + +// validateWorkspaceKind fetches the WorkspaceKind for a Workspace and returns an error if it does not exist +func (v *WorkspaceValidator) validateWorkspaceKind(ctx context.Context, workspace *kubefloworgv1beta1.Workspace) (*kubefloworgv1beta1.WorkspaceKind, *field.Error) { + workspaceKindName := workspace.Spec.Kind + workspaceKind := &kubefloworgv1beta1.WorkspaceKind{} + if err := v.Get(ctx, client.ObjectKey{Name: workspaceKindName}, workspaceKind); err != nil { + workspaceKindNamePath := field.NewPath("spec", "kind") + if apierrors.IsNotFound(err) { + return nil, field.Invalid( + workspaceKindNamePath, + workspaceKindName, + fmt.Sprintf("workspace kind %q not found", workspaceKindName), + ) + } else { + return nil, field.InternalError( + workspaceKindNamePath, + err, + ) + } + } + return workspaceKind, nil +} + +// validateImageConfig checks if the imageConfig selected by a Workspace exists a WorkspaceKind +func (v *WorkspaceValidator) validateImageConfig(workspace *kubefloworgv1beta1.Workspace, workspaceKind *kubefloworgv1beta1.WorkspaceKind) *field.Error { + imageConfig := workspace.Spec.PodTemplate.Options.ImageConfig + for _, value := range workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values { + if imageConfig == value.Id { + // imageConfig found + return nil + } + } + imageConfigPath := field.NewPath("spec", "podTemplate", "options", "imageConfig") + return field.Invalid( + imageConfigPath, + imageConfig, + fmt.Sprintf("imageConfig with id %q not found in workspace kind %q", imageConfig, workspaceKind.Name), + ) +} + +// validatePodConfig checks if the podConfig selected by a Workspace exists a WorkspaceKind +func (v *WorkspaceValidator) validatePodConfig(workspace *kubefloworgv1beta1.Workspace, workspaceKind *kubefloworgv1beta1.WorkspaceKind) *field.Error { + podConfig := workspace.Spec.PodTemplate.Options.PodConfig + for _, value := range workspaceKind.Spec.PodTemplate.Options.PodConfig.Values { + if podConfig == value.Id { + // podConfig found + return nil + } + } + podConfigPath := field.NewPath("spec", "podTemplate", "options", "podConfig") + return field.Invalid( + podConfigPath, + podConfig, + fmt.Sprintf("podConfig with id %q not found in workspace kind %q", podConfig, workspaceKind.Name), + ) +} diff --git a/workspaces/controller/api/v1beta1/workspace_webhook_test.go b/workspaces/controller/internal/webhook/workspace_webhook_test.go similarity index 86% rename from workspaces/controller/api/v1beta1/workspace_webhook_test.go rename to workspaces/controller/internal/webhook/workspace_webhook_test.go index 729bc421..9136c3ea 100644 --- a/workspaces/controller/api/v1beta1/workspace_webhook_test.go +++ b/workspaces/controller/internal/webhook/workspace_webhook_test.go @@ -14,10 +14,12 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package webhook import ( "fmt" + + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -41,41 +43,35 @@ var _ = Describe("Workspace Webhook", func() { By("creating the WorkspaceKind") workspaceKind := NewExampleWorkspaceKind(workspaceKindName) Expect(k8sClient.Create(ctx, workspaceKind)).To(Succeed()) - }) AfterAll(func() { - By("deleting the WorkspaceKind") - workspaceKind := &WorkspaceKind{ + workspaceKind := &kubefloworgv1beta1.WorkspaceKind{ ObjectMeta: metav1.ObjectMeta{ Name: workspaceKindName, }, } Expect(k8sClient.Delete(ctx, workspaceKind)).To(Succeed()) - }) It("should reject workspace creation with an invalid WorkspaceKind", func() { - workspaceKindName := "invalid-workspace-kind" + invalidWorkspaceKindName := "invalid-workspace-kind" By("creating the Workspace") - workspace := NewExampleWorkspace(workspaceName, namespaceName, workspaceKindName) + workspace := NewExampleWorkspace(workspaceName, namespaceName, invalidWorkspaceKindName) err := k8sClient.Create(ctx, workspace) - Expect(err).ToNot(Succeed()) - Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("workspace kind %s not found", workspaceKindName))) - + Expect(err).NotTo(Succeed()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("workspace kind %q not found", invalidWorkspaceKindName))) }) It("should successfully create workspace with a valid WorkspaceKind", func() { - By("creating the Workspace") workspace := NewExampleWorkspace(workspaceName, namespaceName, workspaceKindName) Expect(k8sClient.Create(ctx, workspace)).To(Succeed()) By("deleting the Workspace") Expect(k8sClient.Delete(ctx, workspace)).To(Succeed()) - }) }) diff --git a/workspaces/controller/internal/webhook/workspacekind_webhook.go b/workspaces/controller/internal/webhook/workspacekind_webhook.go new file mode 100644 index 00000000..bee30a61 --- /dev/null +++ b/workspaces/controller/internal/webhook/workspacekind_webhook.go @@ -0,0 +1,595 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package webhook + +import ( + "context" + "errors" + "fmt" + "reflect" + + 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" + "k8s.io/apimachinery/pkg/util/validation/field" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// WorkspaceKindValidator validates a Workspace object +type WorkspaceKindValidator struct { + client.Client + Scheme *runtime.Scheme +} + +//+kubebuilder:webhook:path=/validate-kubeflow-org-v1beta1-workspacekind,mutating=false,failurePolicy=fail,sideEffects=None,groups=kubeflow.org,resources=workspacekinds,verbs=create;update;delete,versions=v1beta1,name=vworkspacekind.kb.io,admissionReviewVersions=v1 + +// SetupWebhookWithManager sets up the webhook with the manager +func (v *WorkspaceKindValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr). + For(&kubefloworgv1beta1.WorkspaceKind{}). + WithValidator(v). + Complete() +} + +// ValidateCreate validates the WorkspaceKind on creation. +// The optional warnings will be added to the response as warning messages. +// Return an error if the object is invalid. +func (v *WorkspaceKindValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + 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)) + } + + // validate the extra environment variables + allErrs = append(allErrs, validateExtraEnv(workspaceKind)...) + + // generate helper maps for imageConfig values + imageConfigIdMap := make(map[string]kubefloworgv1beta1.ImageConfigValue) + imageConfigRedirectMap := make(map[string]string) + for _, imageConfigValue := range workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values { + imageConfigIdMap[imageConfigValue.Id] = imageConfigValue + if imageConfigValue.Redirect != nil { + imageConfigRedirectMap[imageConfigValue.Id] = imageConfigValue.Redirect.To + } + } + + // generate helper maps for podConfig values + podConfigIdMap := make(map[string]kubefloworgv1beta1.PodConfigValue) + podConfigRedirectMap := make(map[string]string) + for _, podConfigValue := range workspaceKind.Spec.PodTemplate.Options.PodConfig.Values { + podConfigIdMap[podConfigValue.Id] = podConfigValue + if podConfigValue.Redirect != nil { + podConfigRedirectMap[podConfigValue.Id] = podConfigValue.Redirect.To + } + } + + // validate default options + allErrs = append(allErrs, validateDefaultImageConfig(workspaceKind, imageConfigIdMap)...) + allErrs = append(allErrs, validateDefaultPodConfig(workspaceKind, podConfigIdMap)...) + + // validate imageConfig values + for _, imageConfigValue := range imageConfigIdMap { + imageConfigValueId := imageConfigValue.Id + imageConfigValuePath := field.NewPath("spec", "podTemplate", "options", "imageConfig", "values").Key(imageConfigValueId) + allErrs = append(allErrs, validateImageConfigValue(&imageConfigValue, imageConfigValuePath)...) + } + + // validate redirects + allErrs = append(allErrs, validateImageConfigRedirects(imageConfigIdMap, imageConfigRedirectMap)...) + allErrs = append(allErrs, validatePodConfigRedirects(podConfigIdMap, podConfigRedirectMap)...) + + if len(allErrs) == 0 { + return nil, nil + } + + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: kubefloworgv1beta1.GroupVersion.Group, Kind: "WorkspaceKind"}, + workspaceKind.Name, + allErrs, + ) +} + +// ValidateUpdate validates the WorkspaceKind on update. +// The optional warnings will be added to the response as warning messages. +// Return an error if the object is invalid. +func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { // nolint:gocyclo + 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)) + } + + // get usage count for imageConfig and podConfig values + imageConfigUsageCount, podConfigUsageCount, err := v.getOptionsUsageCounts(ctx, oldWorkspaceKind) + if err != nil { + return nil, err + } + + // validate the extra environment variables + if !reflect.DeepEqual(newWorkspaceKind.Spec.PodTemplate.ExtraEnv, oldWorkspaceKind.Spec.PodTemplate.ExtraEnv) { + allErrs = append(allErrs, validateExtraEnv(newWorkspaceKind)...) + } + + // calculate changes to imageConfig values + var shouldValidateImageConfigRedirects = false + toValidateImageConfigIds := make(map[string]bool) + badChangedImageConfigIds := make(map[string]bool) + badRemovedImageConfigIds := make(map[string]bool) + oldImageConfigIdMap := make(map[string]kubefloworgv1beta1.ImageConfigValue) + newImageConfigIdMap := make(map[string]kubefloworgv1beta1.ImageConfigValue) + newImageConfigRedirectMap := make(map[string]string) + for _, imageConfigValue := range oldWorkspaceKind.Spec.PodTemplate.Options.ImageConfig.Values { + oldImageConfigIdMap[imageConfigValue.Id] = imageConfigValue + } + for _, imageConfigValue := range newWorkspaceKind.Spec.PodTemplate.Options.ImageConfig.Values { + newImageConfigIdMap[imageConfigValue.Id] = imageConfigValue + if imageConfigValue.Redirect != nil { + newImageConfigRedirectMap[imageConfigValue.Id] = imageConfigValue.Redirect.To + } + + // check if the imageConfig value is new + if _, exists := oldImageConfigIdMap[imageConfigValue.Id]; !exists { + // we need to validate this imageConfig value since it is new + toValidateImageConfigIds[imageConfigValue.Id] = true + + // we always need to validate the imageConfig redirects if an imageConfig value was added + 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)) + } + + // check if the spec has changed + if !reflect.DeepEqual(oldImageConfigIdMap[imageConfigValue.Id].Spec, imageConfigValue.Spec) { + // we need to validate this imageConfig value since it has changed + toValidateImageConfigIds[imageConfigValue.Id] = true + + // if this imageConfig is used by any workspaces, mark this imageConfig as bad, + // (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)) + } + + // if this imageConfig is used by any workspaces, mark this imageConfig as bad, + // it is not safe to remove an imageConfig value that is in use + if usageCount > 0 { + badRemovedImageConfigIds[id] = true + } + + // we always need to validate the imageConfig redirects if an imageConfig was removed + shouldValidateImageConfigRedirects = true + } + } + + // calculate changes to podConfig values + var shouldValidatePodConfigRedirects = false + badChangedPodConfigIds := make(map[string]bool) + badRemovedPodConfigIds := make(map[string]bool) + newPodConfigIdMap := make(map[string]kubefloworgv1beta1.PodConfigValue) + newPodConfigRedirectMap := make(map[string]string) + oldPodConfigIdMap := make(map[string]kubefloworgv1beta1.PodConfigValue) + for _, podConfigValue := range oldWorkspaceKind.Spec.PodTemplate.Options.PodConfig.Values { + oldPodConfigIdMap[podConfigValue.Id] = podConfigValue + } + for _, podConfigValue := range newWorkspaceKind.Spec.PodTemplate.Options.PodConfig.Values { + newPodConfigIdMap[podConfigValue.Id] = podConfigValue + if podConfigValue.Redirect != nil { + newPodConfigRedirectMap[podConfigValue.Id] = podConfigValue.Redirect.To + } + + // 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 + 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)) + } + + // normalize the podConfig specs + oldPodConfigSpec := oldPodConfigIdMap[podConfigValue.Id].Spec + err := normalizePodConfigSpec(oldPodConfigSpec) + if err != nil { + return nil, apierrors.NewInternalError(fmt.Errorf("failed to normalize podConfig spec: %w", err)) + } + newPodConfigSpec := podConfigValue.Spec + err = normalizePodConfigSpec(newPodConfigSpec) + if err != nil { + return nil, apierrors.NewInternalError(fmt.Errorf("failed to normalize podConfig spec: %w", 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 + } + + // 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 + } + } + } + for id := range oldPodConfigIdMap { + + // 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)) + } + + // if this podConfig is used by any workspaces, mark this podConfig as bad, + // it is not safe to remove a podConfig value that is in use + if usageCount > 0 { + badRemovedPodConfigIds[id] = true + } + + // we always need to validate the podConfig redirects if a podConfig was removed + 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)...) + } + + // validate imageConfig values + // NOTE: we only need to validate new or changed imageConfig values + for imageConfigValueId := range toValidateImageConfigIds { + imageConfigValue := newImageConfigIdMap[imageConfigValueId] + imageConfigValuePath := field.NewPath("spec", "podTemplate", "options", "imageConfig", "values").Key(imageConfigValueId) + allErrs = append(allErrs, validateImageConfigValue(&imageConfigValue, imageConfigValuePath)...) + } + + // validate 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))) + } + for id := range badRemovedImageConfigIds { + 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 removed", id))) + } + + // validate 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))) + } + for id := range badRemovedPodConfigIds { + 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 removed", id))) + } + + // validate redirects + if shouldValidateImageConfigRedirects { + allErrs = append(allErrs, validateImageConfigRedirects(newImageConfigIdMap, newImageConfigRedirectMap)...) + } + if shouldValidatePodConfigRedirects { + allErrs = append(allErrs, validatePodConfigRedirects(newPodConfigIdMap, newPodConfigRedirectMap)...) + } + + if len(allErrs) == 0 { + return nil, nil + } + + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: kubefloworgv1beta1.GroupVersion.Group, Kind: "WorkspaceKind"}, + newWorkspaceKind.Name, + allErrs, + ) +} + +// ValidateDelete validates the WorkspaceKind on deletion. +// The optional warnings will be added to the response as warning messages. +// Return an error if the object is invalid. +func (v *WorkspaceKindValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + log := log.FromContext(ctx) + log.V(1).Info("validating WorkspaceKind delete") + + workspaceKind, ok := obj.(*kubefloworgv1beta1.WorkspaceKind) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a WorkspaceKind object but got %T", obj)) + } + + // don't allow deletion of WorkspaceKind if it is used by any workspaces + if workspaceKind.Status.Workspaces > 0 { + return nil, apierrors.NewConflict( + schema.GroupResource{Group: kubefloworgv1beta1.GroupVersion.Group, Resource: "WorkspaceKind"}, + workspaceKind.Name, + fmt.Errorf("WorkspaceKind is used by %d workspace(s)", workspaceKind.Status.Workspaces), + ) + } + + // don't allow deletion of WorkspaceKind if it has the protection finalizer + // NOTE: while the finalizer also protects the WorkspaceKind from deletion, + // it is impossible to "un-delete" a resource once it has started terminating + // and this is a bad user experience, so we prevent deletion in the first place + if controllerutil.ContainsFinalizer(workspaceKind, controller.WorkspaceKindFinalizer) { + return nil, apierrors.NewConflict( + schema.GroupResource{Group: kubefloworgv1beta1.GroupVersion.Group, Resource: "WorkspaceKind"}, + workspaceKind.Name, + errors.New("WorkspaceKind has protection finalizer, indicating one or more workspaces are still using it"), + ) + } + + 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) + + // 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 + } + for _, podConfigMetrics := range workspaceKind.Status.PodTemplateOptions.PodConfig { + podConfigUsageCount[podConfigMetrics.Id] = podConfigMetrics.Workspaces + } + } + + // fetch all Workspaces that are using this WorkspaceKind + workspaces := &kubefloworgv1beta1.WorkspaceList{} + listOpts := &client.ListOptions{ + FieldSelector: fields.OneTermEqualSelector(helper.IndexWorkspaceKindField, workspaceKind.Name), + Namespace: "", // fetch Workspaces in all namespaces + } + if err := v.List(ctx, workspaces, listOpts); err != nil { + return nil, nil, apierrors.NewInternalError(err) + } + + // count the number of Workspaces using each option + for _, imageConfig := range workspaceKind.Spec.PodTemplate.Options.ImageConfig.Values { + imageConfigUsageCount[imageConfig.Id] = 0 + } + for _, podConfig := range workspaceKind.Spec.PodTemplate.Options.PodConfig.Values { + podConfigUsageCount[podConfig.Id] = 0 + } + for _, ws := range workspaces.Items { + imageConfigUsageCount[ws.Spec.PodTemplate.Options.ImageConfig]++ + podConfigUsageCount[ws.Spec.PodTemplate.Options.PodConfig]++ + } + + return imageConfigUsageCount, podConfigUsageCount, nil +} + +// validateExtraEnv validates the extra environment variables in a WorkspaceKind +func validateExtraEnv(workspaceKind *kubefloworgv1beta1.WorkspaceKind) []*field.Error { + var errs []*field.Error + + // the real httpPathPrefix can't fail, so we return a dummy value + httpPathPrefixFunc := func(portId string) string { + return "DUMMY_HTTP_PATH_PREFIX" + } + + // validate that each value template can be rendered successfully + for _, env := range workspaceKind.Spec.PodTemplate.ExtraEnv { + if env.Value != "" { + rawValue := env.Value + _, err := helper.RenderExtraEnvValueTemplate(rawValue, httpPathPrefixFunc) + if err != nil { + extraEnvPath := field.NewPath("spec", "podTemplate", "extraEnv").Key(env.Name).Child("value") + errs = append(errs, field.Invalid(extraEnvPath, rawValue, err.Error())) + } + } + } + + return errs +} + +// validateDefaultImageConfig validates the default imageConfig in a WorkspaceKind +func validateDefaultImageConfig(workspaceKind *kubefloworgv1beta1.WorkspaceKind, imageConfigValueMap map[string]kubefloworgv1beta1.ImageConfigValue) []*field.Error { + var errs []*field.Error + + // validate the default imageConfig + defaultImageConfig := workspaceKind.Spec.PodTemplate.Options.ImageConfig.Spawner.Default + if _, exists := imageConfigValueMap[defaultImageConfig]; !exists { + defaultImageConfigPath := field.NewPath("spec", "podTemplate", "options", "imageConfig", "spawner", "default") + errs = append(errs, field.Invalid(defaultImageConfigPath, defaultImageConfig, fmt.Sprintf("default imageConfig %q not found", defaultImageConfig))) + } + + return errs +} + +// validateDefaultPodConfig validates the default podConfig in a WorkspaceKind +func validateDefaultPodConfig(workspaceKind *kubefloworgv1beta1.WorkspaceKind, podConfigValueMap map[string]kubefloworgv1beta1.PodConfigValue) []*field.Error { + var errs []*field.Error + + // validate the default podConfig + defaultPodConfig := workspaceKind.Spec.PodTemplate.Options.PodConfig.Spawner.Default + if _, exists := podConfigValueMap[defaultPodConfig]; !exists { + defaultPodConfigPath := field.NewPath("spec", "podTemplate", "options", "podConfig", "spawner", "default") + errs = append(errs, field.Invalid(defaultPodConfigPath, defaultPodConfig, fmt.Sprintf("default podConfig %q not found", defaultPodConfig))) + } + + return errs +} + +// validateImageConfigValue validates an imageConfig value +func validateImageConfigValue(imageConfigValue *kubefloworgv1beta1.ImageConfigValue, imageConfigValuePath *field.Path) []*field.Error { + var errs []*field.Error + + // validate the ports + seenPorts := make(map[int32]bool) + for _, port := range imageConfigValue.Spec.Ports { + portId := port.Id + portNumber := port.Port + if _, exists := seenPorts[portNumber]; exists { + portPath := imageConfigValuePath.Child("spec", "ports").Key(portId).Child("port") + errs = append(errs, field.Invalid(portPath, portNumber, fmt.Sprintf("port %d is defined more than once", portNumber))) + } + seenPorts[portNumber] = true + } + + return errs +} + +// validateImageConfigRedirects validates redirects in the imageConfig values +func validateImageConfigRedirects(imageConfigIdMap map[string]kubefloworgv1beta1.ImageConfigValue, imageConfigRedirectMap map[string]string) []*field.Error { + var errs []*field.Error + + // validate imageConfig redirects + checkedNodes := make(map[string]bool) + for id, redirectTo := range imageConfigRedirectMap { + // 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))) + 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))) + } + } + + return errs +} + +// validatePodConfigRedirects validates redirects in the podConfig values +func validatePodConfigRedirects(podConfigIdMap map[string]kubefloworgv1beta1.PodConfigValue, podConfigRedirectMap map[string]string) []*field.Error { + var errs []*field.Error + + // validate podConfig redirects + checkedNodes := make(map[string]bool) + for id, redirectTo := range podConfigRedirectMap { + // 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))) + 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))) + } + } + + 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/api/v1beta1/workspacekind_webhook_test.go b/workspaces/controller/internal/webhook/workspacekind_webhook_test.go similarity index 76% rename from workspaces/controller/api/v1beta1/workspacekind_webhook_test.go rename to workspaces/controller/internal/webhook/workspacekind_webhook_test.go index 6a120916..9ef7f968 100644 --- a/workspaces/controller/api/v1beta1/workspacekind_webhook_test.go +++ b/workspaces/controller/internal/webhook/workspacekind_webhook_test.go @@ -14,10 +14,13 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +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" @@ -25,7 +28,6 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" - "time" ) var _ = Describe("WorkspaceKind Webhook", func() { @@ -47,39 +49,48 @@ var _ = Describe("WorkspaceKind Webhook", func() { testCases := []struct { description string - workspaceKind *WorkspaceKind + workspaceKind *v1beta1.WorkspaceKind + shouldSucceed bool }{ { description: "should reject WorkspaceKind creation with cycles in ImageConfig options", workspaceKind: NewExampleWorkspaceKindWithImageConfigCycle("wsk-webhook-image-config-cycle-test"), + shouldSucceed: false, }, { description: "should reject WorkspaceKind creation with cycles in PodConfig options", 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"), + shouldSucceed: false, }, { description: "should reject WorkspaceKind creation with invalid redirects in PodConfig options", workspaceKind: NewExampleWorkspaceKindWithInvalidPodConfig("wsk-webhook-pod-config-invalid-test"), + shouldSucceed: false, }, { description: "should reject WorkspaceKind creation if the default ImageConfig option is missing", workspaceKind: NewExampleWorkspaceKindWithInvalidDefaultImageConfig("wsk-webhook-image-config-default-test"), + shouldSucceed: false, }, { description: "should reject WorkspaceKind creation if the default PodConfig option is missing", 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"), + 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"), + shouldSucceed: false, }, } @@ -87,7 +98,11 @@ var _ = Describe("WorkspaceKind Webhook", func() { tc := tc // Create a new instance of tc to avoid capturing the loop variable. It(tc.description, func() { By("creating the WorkspaceKind") - Expect(k8sClient.Create(ctx, tc.workspaceKind)).ToNot(Succeed()) + if tc.shouldSucceed { + Expect(k8sClient.Create(ctx, tc.workspaceKind)).To(Succeed()) + } else { + Expect(k8sClient.Create(ctx, tc.workspaceKind)).NotTo(Succeed()) + } }) } @@ -97,7 +112,7 @@ var _ = Describe("WorkspaceKind Webhook", func() { var ( workspaceKindName string workspaceKindKey types.NamespacedName - workspaceKind *WorkspaceKind + workspaceKind *v1beta1.WorkspaceKind ) BeforeAll(func() { @@ -110,7 +125,7 @@ var _ = Describe("WorkspaceKind Webhook", func() { Expect(k8sClient.Create(ctx, createdWorkspaceKind)).To(Succeed()) By("getting the created WorkspaceKind") - workspaceKind = &WorkspaceKind{} + workspaceKind = &v1beta1.WorkspaceKind{} Eventually(func() error { return k8sClient.Get(ctx, workspaceKindKey, workspaceKind) }, timeout, interval).Should(Succeed()) @@ -123,19 +138,21 @@ var _ = Describe("WorkspaceKind Webhook", func() { testCases := []struct { description string - modifyKindFn func(*WorkspaceKind) + modifyKindFn func(*v1beta1.WorkspaceKind) workspaceName *string + shouldSucceed bool }{ { description: "should reject updates to used imageConfig spec", - modifyKindFn: func(wsk *WorkspaceKind) { + modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { wsk.Spec.PodTemplate.Options.ImageConfig.Values[0].Spec.Image = "new-image:latest" }, workspaceName: ptr.To("ws-webhook-update-image-config-spec-test"), + shouldSucceed: false, }, { description: "should reject updates to used podConfig spec", - modifyKindFn: func(wsk *WorkspaceKind) { + modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { wsk.Spec.PodTemplate.Options.PodConfig.Values[0].Spec.Resources = &corev1.ResourceRequirements{ Limits: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("1.5"), @@ -143,64 +160,79 @@ var _ = Describe("WorkspaceKind Webhook", func() { } }, workspaceName: ptr.To("ws-webhook-update-pod-config-spec-test"), + shouldSucceed: false, }, { description: "should reject WorkspaceKind update with cycles in imageConfig options", - modifyKindFn: func(wsk *WorkspaceKind) { - wsk.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &OptionRedirect{To: "jupyterlab_scipy_190"} + modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { + wsk.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &v1beta1.OptionRedirect{To: "jupyterlab_scipy_190"} }, + shouldSucceed: false, }, { description: "should reject WorkspaceKind update with invalid redirects in ImageConfig options", - modifyKindFn: func(wsk *WorkspaceKind) { - wsk.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &OptionRedirect{To: "invalid-image-config"} + modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { + wsk.Spec.PodTemplate.Options.ImageConfig.Values[1].Redirect = &v1beta1.OptionRedirect{To: "invalid-image-config"} }, + shouldSucceed: false, }, { description: "should reject WorkspaceKind update with cycles in PodConfig options", - modifyKindFn: func(wsk *WorkspaceKind) { - wsk.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &OptionRedirect{To: "small_cpu"} - wsk.Spec.PodTemplate.Options.PodConfig.Values[1].Redirect = &OptionRedirect{To: "tiny_cpu"} - + 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"} }, + shouldSucceed: false, }, { description: "should reject WorkspaceKind creation with invalid redirects in PodConfig options", - modifyKindFn: func(wsk *WorkspaceKind) { - wsk.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &OptionRedirect{To: "invalid-pod-config"} + modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { + wsk.Spec.PodTemplate.Options.PodConfig.Values[0].Redirect = &v1beta1.OptionRedirect{To: "invalid-pod-config"} }, + shouldSucceed: false, }, { description: "should reject updates to WorkspaceKind with missing default imageConfig", - modifyKindFn: func(wsk *WorkspaceKind) { + modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { wsk.Spec.PodTemplate.Options.ImageConfig.Spawner.Default = "invalid-image-config" }, + shouldSucceed: false, }, { description: "should reject updates to WorkspaceKind with missing default podConfig", - modifyKindFn: func(wsk *WorkspaceKind) { + modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { wsk.Spec.PodTemplate.Options.PodConfig.Spawner.Default = "invalid-pod-config" }, }, { description: "should reject updates to WorkspaceKind if extraEnv[].value is not a valid Go template", - modifyKindFn: func(wsk *WorkspaceKind) { + modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { wsk.Spec.PodTemplate.ExtraEnv[0].Value = `{{ httpPathPrefix "jupyterlab" }` }, + 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" }}` + }, + shouldSucceed: true, }, { description: "should reject updates that remove ImageConfig in use", - modifyKindFn: func(wsk *WorkspaceKind) { + modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { wsk.Spec.PodTemplate.Options.ImageConfig.Values = wsk.Spec.PodTemplate.Options.ImageConfig.Values[1:] }, workspaceName: ptr.To("ws-webhook-update-image-config-test"), + shouldSucceed: false, }, { description: "should reject updates that remove podConfig in use", - modifyKindFn: func(wsk *WorkspaceKind) { + modifyKindFn: func(wsk *v1beta1.WorkspaceKind) { wsk.Spec.PodTemplate.Options.PodConfig.Values = wsk.Spec.PodTemplate.Options.PodConfig.Values[1:] }, workspaceName: ptr.To("ws-webhook-update-pod-config-test"), + shouldSucceed: false, }, } @@ -208,7 +240,6 @@ var _ = Describe("WorkspaceKind Webhook", func() { 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) Expect(k8sClient.Create(ctx, workspace)).To(Succeed()) @@ -218,7 +249,11 @@ var _ = Describe("WorkspaceKind Webhook", func() { modifiedWorkspaceKind := workspaceKind.DeepCopy() tc.modifyKindFn(modifiedWorkspaceKind) - Expect(k8sClient.Patch(ctx, modifiedWorkspaceKind, patch)).NotTo(Succeed()) + if tc.shouldSucceed { + Expect(k8sClient.Patch(ctx, modifiedWorkspaceKind, patch)).To(Succeed()) + } else { + Expect(k8sClient.Patch(ctx, modifiedWorkspaceKind, patch)).NotTo(Succeed()) + } }) } })