DSC, DSCI: add validating webhook (opendatahub-io#711)

This patch reverts 5288015 ("Revert "DSC, DSCI: add validating webhook (opendatahub-io#711)"") * webhook: add initial skeleton Originally it was generated with ```operator-sdk create webhook --group datasciencecluster --version v1 --kind DataScienceCluster --programmatic-validation``` but webhook.Validator interface (like described in the kubebuilder book[1]) does not work well for the purpose of the webhook due to needs to access openshift cluster (client.Client) to check existing instances of DSC. So, direct implementation of Handler was done inspired by [2] and odh-notebooks implementation [3]. Move it from api package closer to controllers as in [3] as well since it's not DataScienceCluster or DSCInitialization extention anymore. Amend webhook_suite_test.go's path to configs accordingly. Fix linter issues in webhook_suite_test.go: - disable ssl check; - move to package webhook_test certmanager files removed too due to usage of OpenShift service serving certificates[4] (see also service.beta.openshift.io/inject-cabundle annotation in config/webhook/kustomization.yaml). Add webhook generation to `make manifests` target so webhook/manifests.yaml is generated with it. Since DSCI creation now requires webhook it should be delayed after manager started. Move it to a closure and add it to the manager for run with Add() API. It requires explicit declaration of the interface variable otherwise complains about type mismatch for the function literal. [1] https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation [2] https://book-v1.book.kubebuilder.io/beyond_basics/sample_webhook.html [3] https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/controllers/notebook_webhook.go [4] https://docs.openshift.com/container-platform/4.9/security/certificates/service-serving-certificate.html Signed-off-by: Yauheni Kaliuta <[email protected]> * webhook: implement one instance enforcing The webhook is written with the idea to handle both Create and Update requests (configured in config/webhook/manifests.yaml), but at the moment only duplication check on Create is implemented. Implements the logic which is done now on reconcile time [1] (same for DSCI). It checks for 0 instances, not 1, since when the webhook is running the object has not been created yet. Means if it's 1 then it handles request to create a second one. It could be probably possible to use generics but does not make a lot of sense for such a simple case. Closes: opendatahub-io#693 [1] https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/controllers/datasciencecluster/datasciencecluster_controller.go#L98 Signed-off-by: Yauheni Kaliuta <[email protected]> * tests: add tests to check duplication blocking Add both envtest and e2e tests of a second DataScienceCluster instance creation blocking. envtest's one is a part of webhook test suite. e2e: Add `name` parameter to setupDSCInstance() function to reuse it. Use require.Error() as the assertion, shorter and more straight logic than implementing it in the test itself. Add e2e test to check DSCInitialization similar way. Signed-off-by: Yauheni Kaliuta <[email protected]> * tests: e2e: refactor duplication tests in more abstract way Factor out common code using Unstructured/List objects. Change structure to remind more prepare/action/assert. Use "require" features when appropriate. Signed-off-by: Yauheni Kaliuta <[email protected]> --------- Signed-off-by: Yauheni Kaliuta <[email protected]> chore(webhook): (opendatahub-io#870) - add testcase on DSCI - remove kubebuilder marker not needed - remove checks on instance number in existing controllers - re-generate bundle - we do not act on update but we keep it on webhook for now Signed-off-by: Wen Zhou <[email protected]>
red-hat-data-services · May 2, 2024 · 34031a2 · 34031a2
1 parent 4f3ad07
commit 34031a2
Show file tree

Hide file tree

Showing 20 changed files with 626 additions and 87 deletions.
diff --git a/Makefile b/Makefile
@@ -142,9 +142,7 @@ endef
 
 .PHONY: manifests
 manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
-# TODO: enable below when we do webhook
-# $(CONTROLLER_GEN) rbac:roleName=controller-manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
-	$(CONTROLLER_GEN) rbac:roleName=controller-manager-role crd:ignoreUnexportedFields=true paths="./..." output:crd:artifacts:config=config/crd/bases
+	$(CONTROLLER_GEN) rbac:roleName=controller-manager-role crd:ignoreUnexportedFields=true webhook paths="./..." output:crd:artifacts:config=config/crd/bases
 	$(call fetch-external-crds,github.com/openshift/api,route/v1)
 	$(call fetch-external-crds,github.com/openshift/api,user/v1)
 

diff --git a/PROJECT b/PROJECT
@@ -21,6 +21,9 @@ resources:
   kind: DSCInitialization
   path: github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1
   version: v1
+  webhooks:
+    validation: true
+    webhookVersion: v1
 - api:
     crdVersion: v1
     namespaced: false
@@ -30,4 +33,7 @@ resources:
   kind: DataScienceCluster
   path: github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1
   version: v1
+  webhooks:
+    validation: true
+    webhookVersion: v1
 version: "3"
diff --git a/bundle/manifests/redhat-ods-operator-webhook-service_v1_service.yaml b/bundle/manifests/redhat-ods-operator-webhook-service_v1_service.yaml
@@ -0,0 +1,24 @@
+apiVersion: v1
+kind: Service
+metadata:
+  annotations:
+    service.beta.openshift.io/inject-cabundle: "true"
+    service.beta.openshift.io/serving-cert-secret-name: redhat-ods-operator-controller-webhook-cert
+  creationTimestamp: null
+  labels:
+    app.kubernetes.io/component: webhook
+    app.kubernetes.io/created-by: rhods-operator
+    app.kubernetes.io/instance: webhook-service
+    app.kubernetes.io/managed-by: kustomize
+    app.kubernetes.io/name: service
+    app.kubernetes.io/part-of: rhods-operator
+  name: redhat-ods-operator-webhook-service
+spec:
+  ports:
+  - port: 443
+    protocol: TCP
+    targetPort: 9443
+  selector:
+    control-plane: controller-manager
+status:
+  loadBalancer: {}
diff --git a/bundle/manifests/rhods-operator.clusterserviceversion.yaml b/bundle/manifests/rhods-operator.clusterserviceversion.yaml
@@ -1684,6 +1684,10 @@ spec:
                   initialDelaySeconds: 15
                   periodSeconds: 20
                 name: manager
+                ports:
+                - containerPort: 9443
+                  name: webhook-server
+                  protocol: TCP
                 readinessProbe:
                   httpGet:
                     path: /readyz
@@ -1702,10 +1706,19 @@ spec:
                   capabilities:
                     drop:
                     - ALL
+                volumeMounts:
+                - mountPath: /tmp/k8s-webhook-server/serving-certs
+                  name: cert
+                  readOnly: true
               securityContext:
                 runAsNonRoot: true
               serviceAccountName: redhat-ods-operator-controller-manager
               terminationGracePeriodSeconds: 10
+              volumes:
+              - name: cert
+                secret:
+                  defaultMode: 420
+                  secretName: redhat-ods-operator-controller-webhook-cert
     strategy: deployment
   installModes:
   - supported: false
@@ -1733,3 +1746,26 @@ spec:
   provider:
     name: Red Hat
   version: 2.9.0
+  webhookdefinitions:
+  - admissionReviewVersions:
+    - v1
+    containerPort: 443
+    deploymentName: redhat-ods-operator-controller-manager
+    failurePolicy: Fail
+    generateName: operator.opendatahub.io
+    rules:
+    - apiGroups:
+      - datasciencecluster.opendatahub.io
+      - dscinitialization.opendatahub.io
+      apiVersions:
+      - v1
+      operations:
+      - CREATE
+      - UPDATE
+      resources:
+      - datascienceclusters
+      - dscinitializations
+    sideEffects: None
+    targetPort: 9443
+    type: ValidatingAdmissionWebhook
+    webhookPath: /validate-opendatahub-io-v1
diff --git a/config/default/kustomization.yaml b/config/default/kustomization.yaml
@@ -21,7 +21,7 @@ resources:
 - ../manager
 # [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in
 # crd/kustomization.yaml
-#- ../webhook
+- ../webhook
 # [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER'. 'WEBHOOK' components are required.
 #- ../certmanager
 # [PROMETHEUS] To enable prometheus monitor, uncomment all sections with 'PROMETHEUS'.
@@ -37,6 +37,7 @@ resources:
 
 # [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in
 # crd/kustomization.yaml
+# Moved below to patches
 #- manager_webhook_patch.yaml
 
 # [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER'.
@@ -76,3 +77,5 @@ resources:
 
 patches:
 - path: manager_auth_proxy_patch.yaml
+# [WEBHOOK]
+- path: manager_webhook_patch.yaml
diff --git a/config/default/manager_webhook_patch.yaml b/config/default/manager_webhook_patch.yaml
@@ -0,0 +1,23 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: controller-manager
+  namespace: system
+spec:
+  template:
+    spec:
+      containers:
+      - name: manager
+        ports:
+        - containerPort: 9443
+          name: webhook-server
+          protocol: TCP
+        volumeMounts:
+        - mountPath: /tmp/k8s-webhook-server/serving-certs
+          name: cert
+          readOnly: true
+      volumes:
+      - name: cert
+        secret:
+          defaultMode: 420
+          secretName: redhat-ods-operator-controller-webhook-cert
diff --git a/config/webhook/kustomization.yaml b/config/webhook/kustomization.yaml
@@ -0,0 +1,9 @@
+resources:
+- manifests.yaml
+- service.yaml
+
+commonAnnotations:
+  service.beta.openshift.io/inject-cabundle: "true"
+
+configurations:
+- kustomizeconfig.yaml
diff --git a/config/webhook/kustomizeconfig.yaml b/config/webhook/kustomizeconfig.yaml
@@ -0,0 +1,25 @@
+# the following config is for teaching kustomize where to look at when substituting vars.
+# It requires kustomize v2.1.0 or newer to work properly.
+nameReference:
+- kind: Service
+  version: v1
+  fieldSpecs:
+  - kind: MutatingWebhookConfiguration
+    group: admissionregistration.k8s.io
+    path: webhooks/clientConfig/service/name
+  - kind: ValidatingWebhookConfiguration
+    group: admissionregistration.k8s.io
+    path: webhooks/clientConfig/service/name
+
+namespace:
+- kind: MutatingWebhookConfiguration
+  group: admissionregistration.k8s.io
+  path: webhooks/clientConfig/service/namespace
+  create: true
+- kind: ValidatingWebhookConfiguration
+  group: admissionregistration.k8s.io
+  path: webhooks/clientConfig/service/namespace
+  create: true
+
+varReference:
+- path: metadata/annotations
diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml
@@ -0,0 +1,29 @@
+---
+apiVersion: admissionregistration.k8s.io/v1
+kind: ValidatingWebhookConfiguration
+metadata:
+  creationTimestamp: null
+  name: validating-webhook-configuration
+webhooks:
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-opendatahub-io-v1
+  failurePolicy: Fail
+  name: operator.opendatahub.io
+  rules:
+  - apiGroups:
+    - datasciencecluster.opendatahub.io
+    - dscinitialization.opendatahub.io
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - datascienceclusters
+    - dscinitializations
+  sideEffects: None
diff --git a/config/webhook/service.yaml b/config/webhook/service.yaml
@@ -0,0 +1,22 @@
+
+apiVersion: v1
+kind: Service
+metadata:
+  labels:
+    app.kubernetes.io/name: service
+    app.kubernetes.io/instance: webhook-service
+    app.kubernetes.io/component: webhook
+    app.kubernetes.io/created-by: rhods-operator
+    app.kubernetes.io/part-of: rhods-operator
+    app.kubernetes.io/managed-by: kustomize
+  name: webhook-service
+  namespace: system
+  annotations:
+    service.beta.openshift.io/serving-cert-secret-name: redhat-ods-operator-controller-webhook-cert
+spec:
+  ports:
+    - port: 443
+      protocol: TCP
+      targetPort: 9443
+  selector:
+    control-plane: controller-manager
diff --git a/controllers/certconfigmapgenerator/certconfigmapgenerator_controller.go b/controllers/certconfigmapgenerator/certconfigmapgenerator_controller.go
@@ -67,9 +67,6 @@ func (r *CertConfigmapGeneratorReconciler) Reconcile(ctx context.Context, req ct
 		return ctrl.Result{}, nil
 	case 1:
 		dsciInstance = &dsciInstances.Items[0]
-	default:
-		message := "only one instance of DSCInitialization object is allowed"
-		return ctrl.Result{}, errors.New(message)
 	}
 
 	if dsciInstance.Spec.TrustedCABundle.ManagementState != operatorv1.Managed {

diff --git a/controllers/datasciencecluster/datasciencecluster_controller.go b/controllers/datasciencecluster/datasciencecluster_controller.go
@@ -19,7 +19,6 @@ package datasciencecluster
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -138,19 +137,6 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
 		return reconcile.Result{Requeue: true}, nil
 	}
 
-	if len(instances.Items) > 1 {
-		message := fmt.Sprintf("only one instance of DataScienceCluster object is allowed. Update existing instance %s", req.Name)
-		err := errors.New(message)
-		_ = r.reportError(err, instance, message)
-
-		_, _ = status.UpdateWithRetry(ctx, r.Client, instance, func(saved *dsc.DataScienceCluster) {
-			status.SetErrorCondition(&saved.Status.Conditions, status.DuplicateDataScienceCluster, message)
-			saved.Status.Phase = status.PhaseError
-		})
-
-		return ctrl.Result{}, err
-	}
-
 	// Verify a valid DSCInitialization instance is created
 	dsciInstances := &dsci.DSCInitializationList{}
 	err = r.Client.List(ctx, dsciInstances)
@@ -161,7 +147,7 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
 	}
 
 	// Update phase to error state if DataScienceCluster is created without valid DSCInitialization
-	switch len(dsciInstances.Items) {
+	switch len(dsciInstances.Items) { // only handle number as 0 or 1, others won't be existed since webhook block creation
 	case 0:
 		reason := status.ReconcileFailed
 		message := "Failed to get a valid DSCInitialization instance, please create a DSCI instance"
@@ -178,13 +164,6 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
 	case 1:
 		dscInitializationSpec := dsciInstances.Items[0].Spec
 		dscInitializationSpec.DeepCopyInto(r.DataScienceCluster.DSCISpec)
-	default:
-		message := "only one instance of DSCInitialization object is allowed"
-		_, _ = status.UpdateWithRetry(ctx, r.Client, instance, func(saved *dsc.DataScienceCluster) {
-			status.SetErrorCondition(&saved.Status.Conditions, status.DuplicateDSCInitialization, message)
-			saved.Status.Phase = status.PhaseError
-		})
-		return ctrl.Result{}, errors.New(message)
 	}
 
 	if instance.ObjectMeta.DeletionTimestamp.IsZero() {

diff --git a/controllers/dscinitialization/dscinitialization_controller.go b/controllers/dscinitialization/dscinitialization_controller.go
@@ -19,8 +19,6 @@ package dscinitialization
 
 import (
 	"context"
-	"errors"
-	"fmt"
 	"path/filepath"
 	"reflect"
 
@@ -89,33 +87,11 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 	}
 
 	var instance *dsciv1.DSCInitialization
-	switch {
+	switch { // only handle number as 0 or 1, others won't be existed since webhook block creation
 	case len(instances.Items) == 0:
 		return ctrl.Result{}, nil
 	case len(instances.Items) == 1:
 		instance = &instances.Items[0]
-	case len(instances.Items) > 1:
-		// find out the one by created timestamp and use it as the default one
-		earliestDSCI := &instances.Items[0]
-		for _, instance := range instances.Items {
-			currentDSCI := instance
-			if currentDSCI.CreationTimestamp.Before(&earliestDSCI.CreationTimestamp) {
-				earliestDSCI = &currentDSCI
-			}
-		}
-		message := fmt.Sprintf("only one instance of DSCInitialization object is allowed. Please delete other instances than %s", earliestDSCI.Name)
-		// update all instances Message and Status
-		for _, deletionInstance := range instances.Items {
-			deletionInstance := deletionInstance
-			if deletionInstance.Name != earliestDSCI.Name {
-				_, _ = status.UpdateWithRetry(ctx, r.Client, &deletionInstance, func(saved *dsciv1.DSCInitialization) {
-					status.SetErrorCondition(&saved.Status.Conditions, status.DuplicateDSCInitialization, message)
-					saved.Status.Phase = status.PhaseError
-				})
-			}
-		}
-
-		return ctrl.Result{}, errors.New(message)
 	}
 
 	if instance.ObjectMeta.DeletionTimestamp.IsZero() {