feat(ws): add validation webhooks for Workspace and WorkspaceKind (#34)

* add validating webhook Signed-off-by: Adem Baccara <[email protected]> * add tests for workspacekind and workspace webhooks Signed-off-by: Adem Baccara <[email protected]> * re-add cert-manager Signed-off-by: Adem Baccara <[email protected]> * refactor ValidateCreate and ValidateUpdate functions in wokrspaceKind webhook Signed-off-by: Adem Baccara <[email protected]> * add e2e test for webhooks Signed-off-by: Adem Baccara <[email protected]> * mathew refactor 1 Signed-off-by: Mathew Wicks <[email protected]> * mathew refactor 2 Signed-off-by: Mathew Wicks <[email protected]> * mathew refactor 3 Signed-off-by: Mathew Wicks <[email protected]> --------- Signed-off-by: Adem Baccara <[email protected]> Signed-off-by: Mathew Wicks <[email protected]> Co-authored-by: Mathew Wicks <[email protected]>
kubeflow · Aug 29, 2024 · bc4e445 · bc4e445
1 parent 38b8955
commit bc4e445
Show file tree

Hide file tree

Showing 37 changed files with 3,106 additions and 221 deletions.
diff --git a/workspaces/controller/Makefile b/workspaces/controller/Makefile
@@ -199,7 +199,7 @@ endef
 
 define prompt_for_e2e_test_execution
     if [ "$$(echo "$(KUBEFLOW_TEST_PROMPT)" | tr '[:upper:]' '[:lower:]')" = "false" ]; then \
-        echo "Skipping E2E test confirmation prompt (KUBEFLOW_TEST_PROMPT is set to true)"; \
+        echo "Skipping E2E test confirmation prompt (KUBEFLOW_TEST_PROMPT is set to false)"; \
     else \
         current_k8s_context=$$(kubectl config current-context); \
         echo "================================ WARNING ================================"; \

diff --git a/workspaces/controller/PROJECT b/workspaces/controller/PROJECT
@@ -16,11 +16,17 @@ resources:
   kind: Workspace
   path: github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1
   version: v1beta1
+  webhooks:
+    validation: true
+    webhookVersion: v1
 - api:
     crdVersion: v1
   controller: true
   domain: kubeflow.org
   kind: WorkspaceKind
   path: github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1
   version: v1beta1
+  webhooks:
+    validation: true
+    webhookVersion: v1
 version: "3"
diff --git a/workspaces/controller/cmd/main.go b/workspaces/controller/cmd/main.go
@@ -35,7 +35,9 @@ 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"
+	"github.com/kubeflow/notebooks/workspaces/controller/internal/helper"
+	webhookInternal "github.com/kubeflow/notebooks/workspaces/controller/internal/webhook"
 	//+kubebuilder:scaffold:imports
 )
 
@@ -115,21 +117,30 @@ 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 {
@@ -138,6 +149,25 @@ func main() {
 	}
 	//+kubebuilder:scaffold:builder
 
+	if os.Getenv("ENABLE_WEBHOOKS") != "false" {
+		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 = (&webhookInternal.WorkspaceKindValidator{
+			Client: mgr.GetClient(),
+			Scheme: mgr.GetScheme(),
+		}).SetupWebhookWithManager(mgr); err != nil {
+			setupLog.Error(err, "unable to create webhook", "webhook", "WorkspaceKind")
+			os.Exit(1)
+		}
+	}
+
 	if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil {
 		setupLog.Error(err, "unable to set up health check")
 		os.Exit(1)

diff --git a/workspaces/controller/config/certmanager/certificate.yaml b/workspaces/controller/config/certmanager/certificate.yaml
@@ -0,0 +1,39 @@
+# The following manifests contain a self-signed issuer CR and a certificate CR.
+# More document can be found at https://docs.cert-manager.io
+# WARNING: Targets CertManager v1.0. Check https://cert-manager.io/docs/installation/upgrading/ for breaking changes.
+apiVersion: cert-manager.io/v1
+kind: Issuer
+metadata:
+  labels:
+    app.kubernetes.io/name: certificate
+    app.kubernetes.io/instance: serving-cert
+    app.kubernetes.io/component: certificate
+    app.kubernetes.io/created-by: workspace-controller
+    app.kubernetes.io/part-of: workspace-controller
+    app.kubernetes.io/managed-by: kustomize
+  name: selfsigned-issuer
+  namespace: system
+spec:
+  selfSigned: {}
+---
+apiVersion: cert-manager.io/v1
+kind: Certificate
+metadata:
+  labels:
+    app.kubernetes.io/name: certificate
+    app.kubernetes.io/instance: serving-cert
+    app.kubernetes.io/component: certificate
+    app.kubernetes.io/created-by: workspace-controller
+    app.kubernetes.io/part-of: workspace-controller
+    app.kubernetes.io/managed-by: kustomize
+  name: serving-cert  # this name should match the one appeared in kustomizeconfig.yaml
+  namespace: system
+spec:
+  # SERVICE_NAME and SERVICE_NAMESPACE will be substituted by kustomize
+  dnsNames:
+  - SERVICE_NAME.SERVICE_NAMESPACE.svc
+  - SERVICE_NAME.SERVICE_NAMESPACE.svc.cluster.local
+  issuerRef:
+    kind: Issuer
+    name: selfsigned-issuer
+  secretName: webhook-server-cert # this secret will not be prefixed, since it's not managed by kustomize
diff --git a/workspaces/controller/config/certmanager/kustomization.yaml b/workspaces/controller/config/certmanager/kustomization.yaml
@@ -0,0 +1,5 @@
+resources:
+- certificate.yaml
+
+configurations:
+- kustomizeconfig.yaml
diff --git a/workspaces/controller/config/certmanager/kustomizeconfig.yaml b/workspaces/controller/config/certmanager/kustomizeconfig.yaml
@@ -0,0 +1,8 @@
+# This configuration is for teaching kustomize how to update name ref substitution
+nameReference:
+- kind: Issuer
+  group: cert-manager.io
+  fieldSpecs:
+  - kind: Certificate
+    group: cert-manager.io
+    path: spec/issuerRef/name
diff --git a/workspaces/controller/config/crd/kustomization.yaml b/workspaces/controller/config/crd/kustomization.yaml
@@ -13,12 +13,12 @@ patches:
 
 # [CERTMANAGER] To enable cert-manager, uncomment all the sections with [CERTMANAGER] prefix.
 # patches here are for enabling the CA injection for each CRD
-#- path: patches/cainjection_in_workspaces.yaml
-#- path: patches/cainjection_in_workspacekinds.yaml
+- path: patches/cainjection_in_workspaces.yaml
+- path: patches/cainjection_in_workspacekinds.yaml
 #+kubebuilder:scaffold:crdkustomizecainjectionpatch
 
 # [WEBHOOK] To enable webhook, uncomment the following section
 # the following config is for teaching kustomize how to do kustomization for CRDs.
 
-#configurations:
-#- kustomizeconfig.yaml
+configurations:
+- kustomizeconfig.yaml
diff --git a/workspaces/controller/config/crd/patches/cainjection_in_workspacekinds.yaml b/workspaces/controller/config/crd/patches/cainjection_in_workspacekinds.yaml
@@ -0,0 +1,7 @@
+# The following patch adds a directive for certmanager to inject CA into the CRD
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  annotations:
+    cert-manager.io/inject-ca-from: CERTIFICATE_NAMESPACE/CERTIFICATE_NAME
+  name: workspacekinds.kubeflow.org
diff --git a/workspaces/controller/config/crd/patches/cainjection_in_workspaces.yaml b/workspaces/controller/config/crd/patches/cainjection_in_workspaces.yaml
@@ -0,0 +1,7 @@
+# The following patch adds a directive for certmanager to inject CA into the CRD
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  annotations:
+    cert-manager.io/inject-ca-from: CERTIFICATE_NAMESPACE/CERTIFICATE_NAME
+  name: workspaces.kubeflow.org
diff --git a/workspaces/controller/config/crd/patches/webhook_in_workspacekinds.yaml b/workspaces/controller/config/crd/patches/webhook_in_workspacekinds.yaml
@@ -0,0 +1,16 @@
+# The following patch enables a conversion webhook for the CRD
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  name: workspacekinds.kubeflow.org
+spec:
+  conversion:
+    strategy: Webhook
+    webhook:
+      clientConfig:
+        service:
+          namespace: system
+          name: webhook-service
+          path: /convert
+      conversionReviewVersions:
+      - v1
diff --git a/workspaces/controller/config/crd/patches/webhook_in_workspaces.yaml b/workspaces/controller/config/crd/patches/webhook_in_workspaces.yaml
@@ -0,0 +1,16 @@
+# The following patch enables a conversion webhook for the CRD
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  name: workspaces.kubeflow.org
+spec:
+  conversion:
+    strategy: Webhook
+    webhook:
+      clientConfig:
+        service:
+          namespace: system
+          name: webhook-service
+          path: /convert
+      conversionReviewVersions:
+      - v1