Add finalizer to clean up OAuthClients on notebook deletion

Author: daniellutz

components/odh-notebook-controller/config/crd/external/oauthclients.oauth.openshift.io.yaml

@@ -0,0 +1,65 @@
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  annotations:
+    crd/fake: "true"
+  name: oauthclients.oauth.openshift.io
+spec:
+  group: oauth.openshift.io
+  names:
+    kind: OAuthClient
+    listKind: OAuthClientList
+    plural: oauthclients
+    singular: oauthclient
+  scope: Cluster
+  versions:
+  - name: v1
+    schema:
+      openAPIV3Schema:
+        description: OAuthClient represents an OAuth client
+        type: object
+        properties:
+          apiVersion:
+            description: 'APIVersion defines the versioned schema of this representation
+              of an object.'
+            type: string
+          kind:
+            description: 'Kind is a string value representing the REST resource this
+              object represents.'
+            type: string
+          metadata:
+            type: object
+          grantMethod:
+            description: 'Method used to authorize client users (auto, prompt, or deny)'
+            type: string
+          secret:
+            description: 'Client secret'
+            type: string
+          redirectURIs:
+            description: 'List of valid redirect URIs for the client'
+            type: array
+            items:
+              type: string
+          respondWithChallenges:
+            description: 'If true, respond to 401 errors with a WWW-Authenticate challenge'
+            type: boolean
+          accessTokenMaxAgeSeconds:
+            description: 'Maximum token lifetime in seconds'
+            type: integer
+            format: int32
+          accessTokenInactivityTimeoutSeconds:
+            description: 'Timeout for inactive tokens in seconds'
+            type: integer
+            format: int32
+          additionalSecrets:
+            description: 'Additional client secrets'
+            type: array
+            items:
+              type: string
+          scopeRestrictions:
+            description: 'Restrictions on scopes the client can request'
+            type: array
+            items:
+              type: object
+    served: true
+    storage: true

components/odh-notebook-controller/config/rbac/role.yaml

@@ -58,6 +58,7 @@ rules:
   - get
   - list
   - patch
+  - update
   - watch
 - apiGroups:
   - kubeflow.org
@@ -88,6 +89,7 @@ rules:
   - oauthclients
   verbs:
   - create
+  - delete
   - get
   - list
   - patch

components/odh-notebook-controller/controllers/notebook_controller.go

@@ -67,14 +67,14 @@ type OpenshiftNotebookReconciler struct {
 
 // ClusterRole permissions
 
-// +kubebuilder:rbac:groups=kubeflow.org,resources=notebooks,verbs=get;list;watch;patch
+// +kubebuilder:rbac:groups=kubeflow.org,resources=notebooks,verbs=get;list;watch;update;patch
 // +kubebuilder:rbac:groups=kubeflow.org,resources=notebooks/status,verbs=get
 // +kubebuilder:rbac:groups=kubeflow.org,resources=notebooks/finalizers,verbs=update
 // +kubebuilder:rbac:groups=route.openshift.io,resources=routes,verbs=get;list;watch;create;update;patch
 // +kubebuilder:rbac:groups="",resources=services;serviceaccounts;secrets;configmaps,verbs=get;list;watch;create;update;patch
 // +kubebuilder:rbac:groups=config.openshift.io,resources=proxies,verbs=get;list;watch
 // +kubebuilder:rbac:groups=networking.k8s.io,resources=networkpolicies,verbs=get;list;watch;create;update;patch
-// +kubebuilder:rbac:groups=oauth.openshift.io,resources=oauthclients,verbs=get;list;watch;create;update;patch
+// +kubebuilder:rbac:groups=oauth.openshift.io,resources=oauthclients,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=get;list;watch;create;update;patch
 // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups="image.openshift.io",resources=imagestreams,verbs=list;get;watch

components/odh-notebook-controller/controllers/notebook_controller_test.go

@@ -29,6 +29,7 @@ import (
 	. "github.com/onsi/gomega"
 
 	dspav1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1"
+	oauthv1 "github.com/openshift/api/oauth/v1"
 	routev1 "github.com/openshift/api/route/v1"
 	corev1 "k8s.io/api/core/v1"
 	netv1 "k8s.io/api/networking/v1"
@@ -1014,6 +1015,154 @@ var _ = Describe("The Openshift Notebook controller", func() {
 		})
 	})
 
+	When("Creating a Notebook with OAuth and then deleting it", func() {
+		const (
+			Name      = "test-notebook-oauth-delete"
+			Namespace = "default"
+			Finalizer = "notebooks.kubeflow.org/oauthclient"
+		)
+
+		var (
+			notebook        *nbv1.Notebook
+			oauthClient     *oauthv1.OAuthClient
+			oauthClientName string
+			ctx             context.Context
+		)
+
+		// Setup for all tests in this context
+		BeforeEach(func() {
+			ctx = context.Background()
+			notebook = createNotebook(Name, Namespace)
+			oauthClientName = fmt.Sprintf("%s-%s-%s", Name, Namespace, "oauth-client")
+
+			oauthClient = &oauthv1.OAuthClient{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: oauthClientName,
+				},
+				RedirectURIs: []string{"https://www.kubeflow.org/"},
+				GrantMethod:  oauthv1.GrantHandlerAuto,
+			}
+		})
+
+		// Clean up resources after each test
+		AfterEach(func() {
+			nbToCleanup := &nbv1.Notebook{}
+			err := cli.Get(ctx, types.NamespacedName{Name: Name, Namespace: Namespace}, nbToCleanup)
+			if err == nil {
+				// Remove finalizers to allow deletion
+				nbToCleanup.Finalizers = nil
+				_ = cli.Update(ctx, nbToCleanup)
+				_ = cli.Delete(ctx, nbToCleanup)
+			}
+
+			oauthClientToCleanup := &oauthv1.OAuthClient{}
+			err = cli.Get(ctx, types.NamespacedName{Name: oauthClientName}, oauthClientToCleanup)
+			if err == nil {
+				_ = cli.Delete(ctx, oauthClientToCleanup)
+			}
+		})
+
+		It("Should add a finalizer when a Notebook has been created", func() {
+			By("Creating a new Notebook")
+			Expect(cli.Create(ctx, notebook)).Should(Succeed())
+
+			By("Creating an OAuthClient")
+			err := cli.Create(ctx, oauthClient)
+			if err != nil {
+				GinkgoT().Logf("OAuthClient creation result: %v", err)
+			}
+
+			By("Adding the finalizer to the Notebook")
+			key := types.NamespacedName{Name: Name, Namespace: Namespace}
+			Expect(cli.Get(ctx, key, notebook)).Should(Succeed())
+
+			notebook.Finalizers = append(notebook.Finalizers, Finalizer)
+			Expect(cli.Update(ctx, notebook)).Should(Succeed())
+
+			By("Verifying the finalizer is added to the Notebook")
+			Expect(cli.Get(ctx, key, notebook)).Should(Succeed())
+			Expect(notebook.Finalizers).To(ContainElement(Finalizer))
+		})
+
+		It("Should prevent deletion when a finalizer is present", func() {
+			By("Creating a Notebook with finalizer")
+			notebook.Finalizers = []string{Finalizer}
+			Expect(cli.Create(ctx, notebook)).Should(Succeed())
+
+			By("Creating an OAuthClient")
+			Expect(cli.Create(ctx, oauthClient)).Should(Succeed())
+
+			By("Requesting deletion of the Notebook")
+			Expect(cli.Delete(ctx, notebook)).Should(Succeed())
+
+			By("Verifying Notebook has deletion timestamp but still exists")
+			key := types.NamespacedName{Name: Name, Namespace: Namespace}
+			Expect(cli.Get(ctx, key, notebook)).Should(Succeed())
+			Expect(notebook.DeletionTimestamp).NotTo(BeNil(), "Notebook should have deletion timestamp")
+			Expect(notebook.Finalizers).To(ContainElement(Finalizer), "Finalizer should still be present")
+		})
+
+		It("Should delete the OAuthClient when the Notebook is deleted", func() {
+			By("Creating a Notebook with finalizer")
+			notebook.Finalizers = []string{Finalizer}
+			Expect(cli.Create(ctx, notebook)).Should(Succeed())
+
+			By("Creating an OAuthClient")
+			Expect(cli.Create(ctx, oauthClient)).Should(Succeed())
+
+			By("Requesting deletion of the Notebook")
+			Expect(cli.Delete(ctx, notebook)).Should(Succeed())
+
+			By("Verifying OAuthClient still exists before finalizer processing")
+			oauthClientKey := types.NamespacedName{Name: oauthClientName}
+			Expect(cli.Get(ctx, oauthClientKey, oauthClient)).Should(Succeed())
+
+			By("Manually deleting OAuthClient to simulate controller behavior")
+			Expect(cli.Delete(ctx, oauthClient)).Should(Succeed())
+
+			By("Verifying OAuthClient is deleted")
+			Eventually(func() bool {
+				err := cli.Get(ctx, oauthClientKey, oauthClient)
+				return apierrors.IsNotFound(err)
+			}, duration, interval).Should(BeTrue(), "OAuthClient should be deleted")
+		})
+
+		It("Should remove the finalizer from the Notebook when OAuthClient has been deleted", func() {
+			By("Creating a Notebook with finalizer")
+			notebook.Finalizers = []string{Finalizer}
+			Expect(cli.Create(ctx, notebook)).Should(Succeed())
+
+			By("Creating an OAuthClient")
+			Expect(cli.Create(ctx, oauthClient)).Should(Succeed())
+
+			By("Requesting deletion of the Notebook")
+			Expect(cli.Delete(ctx, notebook)).Should(Succeed())
+
+			By("Deleting the OAuthClient")
+			Expect(cli.Delete(ctx, oauthClient)).Should(Succeed())
+
+			By("Removing the finalizer to simulate controller behavior")
+			key := types.NamespacedName{Name: Name, Namespace: Namespace}
+			Expect(cli.Get(ctx, key, notebook)).Should(Succeed())
+
+			// Remove finalizer
+			var updatedFinalizers []string
+			for _, f := range notebook.Finalizers {
+				if f != Finalizer {
+					updatedFinalizers = append(updatedFinalizers, f)
+				}
+			}
+			notebook.Finalizers = updatedFinalizers
+			Expect(cli.Update(ctx, notebook)).Should(Succeed())
+
+			By("Verifying Notebook is fully deleted")
+			Eventually(func() bool {
+				err := cli.Get(ctx, key, notebook)
+				return apierrors.IsNotFound(err)
+			}, duration, interval).Should(BeTrue(), "Notebook should be deleted after finalizer removal")
+		})
+	})
+
 	When("Creating notebook as part of Service Mesh", func() {
 
 		const (

components/odh-notebook-controller/controllers/notebook_oauth.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -303,6 +304,18 @@ func (r *OpenshiftNotebookReconciler) ReconcileOAuthClient(notebook *nbv1.Notebo
 		return nil
 	}
 
+	// Before handling the OAuth creation method, we should check if the Notebook parent object has
+	// been marked for deletion.
+	finalizationInProgress, err := r.HandleOAuthClientFinalizer(notebook, ctx)
+	if err != nil {
+		return err
+	}
+
+	// If finalization is in progress, then stop the reconciler here, as OAuthClient do not need to be created
+	if finalizationInProgress {
+		return nil
+	}
+
 	err = r.createOAuthClient(notebook, ctx)
 	if err != nil {
 		log.Error(err, "Unable to handle OAuthClient creation / update")
@@ -312,6 +325,64 @@ func (r *OpenshiftNotebookReconciler) ReconcileOAuthClient(notebook *nbv1.Notebo
 	return nil
 }
 
+func (r *OpenshiftNotebookReconciler) HandleOAuthClientFinalizer(notebook *nbv1.Notebook, ctx context.Context) (bool, error) {
+	finalizerName := "notebooks.kubeflow.org/oauthclient"
+
+	// Check if object is being deleted
+	if notebook.GetDeletionTimestamp() != nil {
+		if controllerutil.ContainsFinalizer(notebook, finalizerName) {
+			if err := r.cleanupOAuthClient(notebook, ctx); err != nil {
+				return true, err
+			}
+
+			controllerutil.RemoveFinalizer(notebook, finalizerName)
+			if err := r.Update(ctx, notebook); err != nil {
+				return true, err
+			}
+		}
+		// Return true to indicate finalization is in progress
+		return true, nil
+	}
+
+	// Add finalizer if it doesn't exist yet
+	if !controllerutil.ContainsFinalizer(notebook, finalizerName) {
+		controllerutil.AddFinalizer(notebook, finalizerName)
+		if err := r.Update(ctx, notebook); err != nil {
+			return true, err
+		}
+		// Return true to indicate we're setting up finalization
+		return true, nil
+	}
+
+	// Return false to indicate normal reconciliation should proceed
+	return false, nil
+}
+
+// cleanupOAuthClient will delete an OAuthClient object in OpenShift based on the Notebook name. An important note is that
+// as OAuthClient objects are not namespaced objects - they are cluster objects - we need to search properly for them to do
+// a clean up, without overloading OpenShift resources.
+func (r *OpenshiftNotebookReconciler) cleanupOAuthClient(notebook *nbv1.Notebook, ctx context.Context) error {
+	// Name of the OAuthClient based on the Notebook name
+	oauthClientName := fmt.Sprintf("%s-%s-%s", notebook.Name, notebook.Namespace, "oauth-client")
+
+	oauthClient := &oauthv1.OAuthClient{}
+	err := r.Get(ctx, types.NamespacedName{
+		Name: oauthClientName,
+	}, oauthClient)
+	if err != nil {
+		if apierrs.IsNotFound(err) {
+			return nil
+		}
+		return err
+	}
+
+	err = r.Delete(ctx, oauthClient)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
 func (r *OpenshiftNotebookReconciler) createSecret(notebook *nbv1.Notebook, ctx context.Context, desiredSecret *corev1.Secret) error {
 	log := logf.FromContext(ctx)
 

components/odh-notebook-controller/controllers/suite_test.go

@@ -38,6 +38,7 @@ import (
 	nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1"
 	dspav1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1"
 	imagev1 "github.com/openshift/api/image/v1"
+	oauthv1 "github.com/openshift/api/oauth/v1"
 	routev1 "github.com/openshift/api/route/v1"
 	v1 "k8s.io/api/core/v1"
 	netv1 "k8s.io/api/networking/v1"
@@ -158,6 +159,7 @@ var _ = BeforeSuite(func() {
 	utilruntime.Must(netv1.AddToScheme(scheme))
 	utilruntime.Must(imagev1.AddToScheme(scheme))
 	utilruntime.Must(dspav1.AddToScheme(scheme))
+	utilruntime.Must(oauthv1.AddToScheme(scheme))
 
 	// +kubebuilder:scaffold:scheme