Skip to content

Commit 27c4300

Browse files
committed
Add finalizer to clean up OAuthClients on notebook deletion
1 parent 72ac948 commit 27c4300

File tree

6 files changed

+293
-3
lines changed

6 files changed

+293
-3
lines changed
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
apiVersion: apiextensions.k8s.io/v1
2+
kind: CustomResourceDefinition
3+
metadata:
4+
annotations:
5+
crd/fake: "true"
6+
name: oauthclients.oauth.openshift.io
7+
spec:
8+
group: oauth.openshift.io
9+
names:
10+
kind: OAuthClient
11+
listKind: OAuthClientList
12+
plural: oauthclients
13+
singular: oauthclient
14+
scope: Cluster
15+
versions:
16+
- name: v1
17+
schema:
18+
openAPIV3Schema:
19+
description: OAuthClient represents an OAuth client
20+
type: object
21+
properties:
22+
apiVersion:
23+
description: 'APIVersion defines the versioned schema of this representation
24+
of an object.'
25+
type: string
26+
kind:
27+
description: 'Kind is a string value representing the REST resource this
28+
object represents.'
29+
type: string
30+
metadata:
31+
type: object
32+
grantMethod:
33+
description: 'Method used to authorize client users (auto, prompt, or deny)'
34+
type: string
35+
secret:
36+
description: 'Client secret'
37+
type: string
38+
redirectURIs:
39+
description: 'List of valid redirect URIs for the client'
40+
type: array
41+
items:
42+
type: string
43+
respondWithChallenges:
44+
description: 'If true, respond to 401 errors with a WWW-Authenticate challenge'
45+
type: boolean
46+
accessTokenMaxAgeSeconds:
47+
description: 'Maximum token lifetime in seconds'
48+
type: integer
49+
format: int32
50+
accessTokenInactivityTimeoutSeconds:
51+
description: 'Timeout for inactive tokens in seconds'
52+
type: integer
53+
format: int32
54+
additionalSecrets:
55+
description: 'Additional client secrets'
56+
type: array
57+
items:
58+
type: string
59+
scopeRestrictions:
60+
description: 'Restrictions on scopes the client can request'
61+
type: array
62+
items:
63+
type: object
64+
served: true
65+
storage: true

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ rules:
4141
- get
4242
- list
4343
- patch
44+
- update
4445
- watch
4546
- apiGroups:
4647
- kubeflow.org
@@ -71,6 +72,7 @@ rules:
7172
- oauthclients
7273
verbs:
7374
- create
75+
- delete
7476
- get
7577
- list
7678
- patch

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,14 +66,14 @@ type OpenshiftNotebookReconciler struct {
6666

6767
// ClusterRole permissions
6868

69-
// +kubebuilder:rbac:groups=kubeflow.org,resources=notebooks,verbs=get;list;watch;patch
69+
// +kubebuilder:rbac:groups=kubeflow.org,resources=notebooks,verbs=get;list;watch;update;patch
7070
// +kubebuilder:rbac:groups=kubeflow.org,resources=notebooks/status,verbs=get
7171
// +kubebuilder:rbac:groups=kubeflow.org,resources=notebooks/finalizers,verbs=update
7272
// +kubebuilder:rbac:groups=route.openshift.io,resources=routes,verbs=get;list;watch;create;update;patch
7373
// +kubebuilder:rbac:groups="",resources=services;serviceaccounts;secrets;configmaps,verbs=get;list;watch;create;update;patch
7474
// +kubebuilder:rbac:groups=config.openshift.io,resources=proxies,verbs=get;list;watch
7575
// +kubebuilder:rbac:groups=networking.k8s.io,resources=networkpolicies,verbs=get;list;watch;create;update;patch
76-
// +kubebuilder:rbac:groups=oauth.openshift.io,resources=oauthclients,verbs=get;list;watch;create;update;patch
76+
// +kubebuilder:rbac:groups=oauth.openshift.io,resources=oauthclients,verbs=get;list;watch;create;update;patch;delete
7777
// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=get;list;watch;create;update;patch
7878
// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=get;list;watch;create;update;patch;delete
7979
// +kubebuilder:rbac:groups="image.openshift.io",resources=imagestreams,verbs=list;get

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

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,12 @@ import (
2727
. "github.com/onsi/ginkgo"
2828
. "github.com/onsi/gomega"
2929

30+
oauthv1 "github.com/openshift/api/oauth/v1"
3031
routev1 "github.com/openshift/api/route/v1"
3132
corev1 "k8s.io/api/core/v1"
3233
netv1 "k8s.io/api/networking/v1"
3334
rbacv1 "k8s.io/api/rbac/v1"
35+
"k8s.io/apimachinery/pkg/api/errors"
3436
"k8s.io/apimachinery/pkg/api/resource"
3537
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3638
"k8s.io/apimachinery/pkg/types"
@@ -884,6 +886,154 @@ var _ = Describe("The Openshift Notebook controller", func() {
884886
})
885887
})
886888

889+
When("Creating a Notebook with OAuth and then deleting it", func() {
890+
const (
891+
Name = "test-notebook-oauth-delete"
892+
Namespace = "default"
893+
Finalizer = "notebooks.kubeflow.org/oauthclient"
894+
)
895+
896+
var (
897+
notebook *nbv1.Notebook
898+
oauthClient *oauthv1.OAuthClient
899+
oauthClientName string
900+
ctx context.Context
901+
)
902+
903+
// Setup for all tests in this context
904+
BeforeEach(func() {
905+
ctx = context.Background()
906+
notebook = createNotebook(Name, Namespace)
907+
oauthClientName = fmt.Sprintf("%s-%s-%s", Name, Namespace, "oauth-client")
908+
909+
oauthClient = &oauthv1.OAuthClient{
910+
ObjectMeta: metav1.ObjectMeta{
911+
Name: oauthClientName,
912+
},
913+
RedirectURIs: []string{"https://www.kubeflow.org/"},
914+
GrantMethod: oauthv1.GrantHandlerAuto,
915+
}
916+
})
917+
918+
// Clean up resources after each test
919+
AfterEach(func() {
920+
nbToCleanup := &nbv1.Notebook{}
921+
err := cli.Get(ctx, types.NamespacedName{Name: Name, Namespace: Namespace}, nbToCleanup)
922+
if err == nil {
923+
// Remove finalizers to allow deletion
924+
nbToCleanup.Finalizers = nil
925+
_ = cli.Update(ctx, nbToCleanup)
926+
_ = cli.Delete(ctx, nbToCleanup)
927+
}
928+
929+
oauthClientToCleanup := &oauthv1.OAuthClient{}
930+
err = cli.Get(ctx, types.NamespacedName{Name: oauthClientName}, oauthClientToCleanup)
931+
if err == nil {
932+
_ = cli.Delete(ctx, oauthClientToCleanup)
933+
}
934+
})
935+
936+
It("Should add a finalizer when a Notebook has been created", func() {
937+
By("Creating a new Notebook")
938+
Expect(cli.Create(ctx, notebook)).Should(Succeed())
939+
940+
By("Creating an OAuthClient")
941+
err := cli.Create(ctx, oauthClient)
942+
if err != nil {
943+
GinkgoT().Logf("OAuthClient creation result: %v", err)
944+
}
945+
946+
By("Adding the finalizer to the Notebook")
947+
key := types.NamespacedName{Name: Name, Namespace: Namespace}
948+
Expect(cli.Get(ctx, key, notebook)).Should(Succeed())
949+
950+
notebook.Finalizers = append(notebook.Finalizers, Finalizer)
951+
Expect(cli.Update(ctx, notebook)).Should(Succeed())
952+
953+
By("Verifying the finalizer is added to the Notebook")
954+
Expect(cli.Get(ctx, key, notebook)).Should(Succeed())
955+
Expect(notebook.Finalizers).To(ContainElement(Finalizer))
956+
})
957+
958+
It("Should prevent deletion when a finalizer is present", func() {
959+
By("Creating a Notebook with finalizer")
960+
notebook.Finalizers = []string{Finalizer}
961+
Expect(cli.Create(ctx, notebook)).Should(Succeed())
962+
963+
By("Creating an OAuthClient")
964+
Expect(cli.Create(ctx, oauthClient)).Should(Succeed())
965+
966+
By("Requesting deletion of the Notebook")
967+
Expect(cli.Delete(ctx, notebook)).Should(Succeed())
968+
969+
By("Verifying Notebook has deletion timestamp but still exists")
970+
key := types.NamespacedName{Name: Name, Namespace: Namespace}
971+
Expect(cli.Get(ctx, key, notebook)).Should(Succeed())
972+
Expect(notebook.DeletionTimestamp).NotTo(BeNil(), "Notebook should have deletion timestamp")
973+
Expect(notebook.Finalizers).To(ContainElement(Finalizer), "Finalizer should still be present")
974+
})
975+
976+
It("Should delete the OAuthClient when the Notebook is deleted", func() {
977+
By("Creating a Notebook with finalizer")
978+
notebook.Finalizers = []string{Finalizer}
979+
Expect(cli.Create(ctx, notebook)).Should(Succeed())
980+
981+
By("Creating an OAuthClient")
982+
Expect(cli.Create(ctx, oauthClient)).Should(Succeed())
983+
984+
By("Requesting deletion of the Notebook")
985+
Expect(cli.Delete(ctx, notebook)).Should(Succeed())
986+
987+
By("Verifying OAuthClient still exists before finalizer processing")
988+
oauthClientKey := types.NamespacedName{Name: oauthClientName}
989+
Expect(cli.Get(ctx, oauthClientKey, oauthClient)).Should(Succeed())
990+
991+
By("Manually deleting OAuthClient to simulate controller behavior")
992+
Expect(cli.Delete(ctx, oauthClient)).Should(Succeed())
993+
994+
By("Verifying OAuthClient is deleted")
995+
Eventually(func() bool {
996+
err := cli.Get(ctx, oauthClientKey, oauthClient)
997+
return errors.IsNotFound(err)
998+
}, duration, interval).Should(BeTrue(), "OAuthClient should be deleted")
999+
})
1000+
1001+
It("Should remove the finalizer from the Notebook when OAuthClient has been deleted", func() {
1002+
By("Creating a Notebook with finalizer")
1003+
notebook.Finalizers = []string{Finalizer}
1004+
Expect(cli.Create(ctx, notebook)).Should(Succeed())
1005+
1006+
By("Creating an OAuthClient")
1007+
Expect(cli.Create(ctx, oauthClient)).Should(Succeed())
1008+
1009+
By("Requesting deletion of the Notebook")
1010+
Expect(cli.Delete(ctx, notebook)).Should(Succeed())
1011+
1012+
By("Deleting the OAuthClient")
1013+
Expect(cli.Delete(ctx, oauthClient)).Should(Succeed())
1014+
1015+
By("Removing the finalizer to simulate controller behavior")
1016+
key := types.NamespacedName{Name: Name, Namespace: Namespace}
1017+
Expect(cli.Get(ctx, key, notebook)).Should(Succeed())
1018+
1019+
// Remove finalizer
1020+
var updatedFinalizers []string
1021+
for _, f := range notebook.Finalizers {
1022+
if f != Finalizer {
1023+
updatedFinalizers = append(updatedFinalizers, f)
1024+
}
1025+
}
1026+
notebook.Finalizers = updatedFinalizers
1027+
Expect(cli.Update(ctx, notebook)).Should(Succeed())
1028+
1029+
By("Verifying Notebook is fully deleted")
1030+
Eventually(func() bool {
1031+
err := cli.Get(ctx, key, notebook)
1032+
return errors.IsNotFound(err)
1033+
}, duration, interval).Should(BeTrue(), "Notebook should be deleted after finalizer removal")
1034+
})
1035+
})
1036+
8871037
When("Creating notebook as part of Service Mesh", func() {
8881038

8891039
const (

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

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"k8s.io/apimachinery/pkg/types"
3636
ctrl "sigs.k8s.io/controller-runtime"
3737
"sigs.k8s.io/controller-runtime/pkg/client"
38+
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
3839
logf "sigs.k8s.io/controller-runtime/pkg/log"
3940
)
4041

@@ -275,8 +276,20 @@ func (r *OpenshiftNotebookReconciler) ReconcileOAuthSecret(notebook *nbv1.Notebo
275276
func (r *OpenshiftNotebookReconciler) ReconcileOAuthClient(notebook *nbv1.Notebook, ctx context.Context) error {
276277
log := logf.FromContext(ctx)
277278

279+
// Before handling the OAuth creation method, we should check if the Notebook parent object has
280+
// been marked for deletion.
281+
finalizationInProgress, err := r.HandleOAuthClientFinalizer(notebook, ctx)
282+
if err != nil {
283+
return err
284+
}
285+
286+
// If finalization is in progress, then stop the reconciler here, as OAuthClient do not need to be created
287+
if finalizationInProgress {
288+
return nil
289+
}
290+
278291
route := &routev1.Route{}
279-
err := r.Get(ctx, types.NamespacedName{
292+
err = r.Get(ctx, types.NamespacedName{
280293
Name: notebook.Name,
281294
Namespace: notebook.Namespace,
282295
}, route)
@@ -298,6 +311,64 @@ func (r *OpenshiftNotebookReconciler) ReconcileOAuthClient(notebook *nbv1.Notebo
298311
return nil
299312
}
300313

314+
func (r *OpenshiftNotebookReconciler) HandleOAuthClientFinalizer(notebook *nbv1.Notebook, ctx context.Context) (bool, error) {
315+
finalizerName := "notebooks.kubeflow.org/oauthclient"
316+
317+
// Check if object is being deleted
318+
if notebook.GetDeletionTimestamp() != nil {
319+
if controllerutil.ContainsFinalizer(notebook, finalizerName) {
320+
if err := r.cleanupOAuthClient(notebook, ctx); err != nil {
321+
return true, err
322+
}
323+
324+
controllerutil.RemoveFinalizer(notebook, finalizerName)
325+
if err := r.Update(ctx, notebook); err != nil {
326+
return true, err
327+
}
328+
}
329+
// Return true to indicate finalization is in progress
330+
return true, nil
331+
}
332+
333+
// Add finalizer if it doesn't exist yet
334+
if !controllerutil.ContainsFinalizer(notebook, finalizerName) {
335+
controllerutil.AddFinalizer(notebook, finalizerName)
336+
if err := r.Update(ctx, notebook); err != nil {
337+
return true, err
338+
}
339+
// Return true to indicate we're setting up finalization
340+
return true, nil
341+
}
342+
343+
// Return false to indicate normal reconciliation should proceed
344+
return false, nil
345+
}
346+
347+
// cleanupOAuthClient will delete an OAuthClient object in OpenShift based on the Notebook name. An important note is that
348+
// as OAuthClient objects are not namespaced objects - they are cluster objects - we need to search properly for them to do
349+
// a clean up, without overloading OpenShift resources.
350+
func (r *OpenshiftNotebookReconciler) cleanupOAuthClient(notebook *nbv1.Notebook, ctx context.Context) error {
351+
// Name of the OAuthClient based on the Notebook name
352+
oauthClientName := fmt.Sprintf("%s-%s-%s", notebook.Name, notebook.Namespace, "oauth-client")
353+
354+
oauthClient := &oauthv1.OAuthClient{}
355+
err := r.Get(ctx, types.NamespacedName{
356+
Name: oauthClientName,
357+
}, oauthClient)
358+
if err != nil {
359+
if apierrs.IsNotFound(err) {
360+
return nil
361+
}
362+
return err
363+
}
364+
365+
err = r.Delete(ctx, oauthClient)
366+
if err != nil {
367+
return err
368+
}
369+
return nil
370+
}
371+
301372
func (r *OpenshiftNotebookReconciler) createSecret(notebook *nbv1.Notebook, ctx context.Context, desiredSecret *corev1.Secret) error {
302373
log := logf.FromContext(ctx)
303374

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import (
3636
ctrl "sigs.k8s.io/controller-runtime"
3737

3838
nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1"
39+
oauthv1 "github.com/openshift/api/oauth/v1"
3940
routev1 "github.com/openshift/api/route/v1"
4041
v1 "k8s.io/api/core/v1"
4142
netv1 "k8s.io/api/networking/v1"
@@ -154,6 +155,7 @@ var _ = BeforeSuite(func() {
154155
utilruntime.Must(nbv1.AddToScheme(scheme))
155156
utilruntime.Must(routev1.AddToScheme(scheme))
156157
utilruntime.Must(netv1.AddToScheme(scheme))
158+
utilruntime.Must(oauthv1.AddToScheme(scheme))
157159

158160
// +kubebuilder:scaffold:scheme
159161

0 commit comments

Comments
 (0)