Merge pull request #1770 from DaoDaoNoCode/upstream-issue-1721

Add owner references to Elyra role binding when creating notebooks
opendatahub-io · Oct 3, 2023 · 94a421f · 94a421f
2 parents cc34fdd + a582575
commit 94a421f
Show file tree

Hide file tree

Showing 4 changed files with 93 additions and 18 deletions.
diff --git a/frontend/src/api/k8s/notebooks.ts b/frontend/src/api/k8s/notebooks.ts
@@ -17,14 +17,13 @@ import { translateDisplayNameForK8s } from '~/pages/projects/utils';
 import { getTolerationPatch, TolerationChanges } from '~/utilities/tolerations';
 import { applyK8sAPIOptions } from '~/api/apiMergeUtils';
 import {
+  createElyraServiceAccountRoleBinding,
   ELYRA_VOLUME_NAME,
-  generateElyraServiceAccountRoleBinding,
   getElyraVolume,
   getElyraVolumeMount,
   getPipelineVolumeMountPatch,
   getPipelineVolumePatch,
 } from '~/concepts/pipelines/elyra/utils';
-import { createRoleBinding } from '~/api';
 import { Volume, VolumeMount } from '~/types';
 import { assemblePodSpecOptions, getshmVolume, getshmVolumeMount } from './utils';
 
@@ -213,8 +212,7 @@ export const stopNotebook = (name: string, namespace: string): Promise<NotebookK
   });
 
 export const startNotebook = async (
-  name: string,
-  namespace: string,
+  notebook: NotebookKind,
   tolerationChanges: TolerationChanges,
   enablePipelines?: boolean,
 ): Promise<NotebookKind> => {
@@ -229,18 +227,12 @@ export const startNotebook = async (
   if (enablePipelines) {
     patches.push(getPipelineVolumePatch());
     patches.push(getPipelineVolumeMountPatch());
-    await createRoleBinding(generateElyraServiceAccountRoleBinding(name, namespace)).catch((e) => {
-      // This is not ideal, but it shouldn't impact the starting of the notebook. Let us log it, and mute the error
-      // eslint-disable-next-line no-console
-      console.error(
-        `Could not patch rolebinding to service account for notebook, ${name}; Reason ${e.message}`,
-      );
-    });
+    await createElyraServiceAccountRoleBinding(notebook);
   }
 
   return k8sPatchResource<NotebookKind>({
     model: NotebookModel,
-    queryOptions: { name, ns: namespace },
+    queryOptions: { name: notebook.metadata.name, ns: notebook.metadata.namespace },
     patches,
   });
 };
@@ -258,9 +250,9 @@ export const createNotebook = (
   });
 
   if (canEnablePipelines) {
-    return createRoleBinding(
-      generateElyraServiceAccountRoleBinding(notebook.metadata.name, notebook.metadata.namespace),
-    ).then(() => notebookPromise);
+    return notebookPromise.then((notebook) =>
+      createElyraServiceAccountRoleBinding(notebook).then(() => notebook),
+    );
   }
 
   return notebookPromise;

diff --git a/frontend/src/api/k8s/roleBindings.ts b/frontend/src/api/k8s/roleBindings.ts
@@ -1,4 +1,5 @@
 import {
+  OwnerReference,
   k8sCreateResource,
   k8sDeleteResource,
   k8sGetResource,
@@ -162,3 +163,20 @@ export const patchRoleBindingName = (
       },
     ],
   });
+
+export const patchRoleBindingOwnerRef = (
+  rbName: string,
+  namespace: string,
+  ownerReferences: OwnerReference[],
+): Promise<RoleBindingKind> =>
+  k8sPatchResource<RoleBindingKind>({
+    model: RoleBindingModel,
+    queryOptions: { name: rbName, ns: namespace },
+    patches: [
+      {
+        op: 'replace',
+        path: '/metadata/ownerReferences',
+        value: ownerReferences,
+      },
+    ],
+  });
diff --git a/frontend/src/concepts/pipelines/elyra/utils.ts b/frontend/src/concepts/pipelines/elyra/utils.ts
@@ -10,9 +10,13 @@ import {
 import { AWS_KEYS } from '~/pages/projects/dataConnections/const';
 import { Volume, VolumeMount } from '~/types';
 import { RUNTIME_MOUNT_PATH } from '~/pages/projects/pvc/const';
+import { createRoleBinding, getRoleBinding, patchRoleBindingOwnerRef } from '~/api';
 
 export const ELYRA_VOLUME_NAME = 'elyra-dsp-details';
 
+export const getElyraServiceAccountRoleBindingName = (notebookName: string) =>
+  `elyra-pipelines-${notebookName}`;
+
 export const getElyraVolumeMount = (): VolumeMount => ({
   name: ELYRA_VOLUME_NAME,
   mountPath: RUNTIME_MOUNT_PATH,
@@ -25,6 +29,13 @@ export const getElyraVolume = (): Volume => ({
   },
 });
 
+export const getElyraRoleBindingOwnerRef = (notebookName: string, ownerUid: string) => ({
+  apiVersion: 'kubeflow.org/v1beta1',
+  kind: 'Notebook',
+  name: notebookName,
+  uid: ownerUid,
+});
+
 export const getPipelineVolumePatch = (): Patch => ({
   path: '/spec/template/spec/volumes/-',
   op: 'add',
@@ -83,15 +94,17 @@ export const generateElyraSecret = (
 export const generateElyraServiceAccountRoleBinding = (
   notebookName: string,
   namespace: string,
+  ownerUid: string,
 ): RoleBindingKind => ({
   apiVersion: 'rbac.authorization.k8s.io/v1',
   kind: 'RoleBinding',
   metadata: {
-    name: `elyra-pipelines-${notebookName}`,
+    name: getElyraServiceAccountRoleBindingName(notebookName),
     namespace,
     labels: {
       [KnownLabels.DASHBOARD_RESOURCE]: 'true',
     },
+    ownerReferences: [getElyraRoleBindingOwnerRef(notebookName, ownerUid)],
   },
   roleRef: {
     apiGroup: 'rbac.authorization.k8s.io',
@@ -105,3 +118,56 @@ export const generateElyraServiceAccountRoleBinding = (
     },
   ],
 });
+
+export const createElyraServiceAccountRoleBinding = async (
+  notebook: NotebookKind,
+): Promise<RoleBindingKind | void> => {
+  const notebookName = notebook.metadata.name;
+  const namespace = notebook.metadata.namespace;
+  const notebookUid = notebook.metadata.uid;
+
+  // Check if rolebinding is already exists for backward compatibility
+  const roleBinding = await getRoleBinding(
+    namespace,
+    getElyraServiceAccountRoleBindingName(notebookName),
+  ).catch((e) => {
+    // 404 is not an error
+    if (e.statusObject?.code !== 404) {
+      // eslint-disable-next-line no-console
+      console.error(
+        `Could not get rolebinding to service account for notebook, ${notebookName}; Reason ${e.message}`,
+      );
+    }
+    return undefined;
+  });
+
+  if (notebookUid) {
+    if (roleBinding) {
+      const ownerReferences = roleBinding.metadata.ownerReferences || [];
+      if (!ownerReferences.find((ownerReference) => ownerReference.uid === notebookUid)) {
+        ownerReferences.push(getElyraRoleBindingOwnerRef(notebookName, notebookUid));
+      }
+      return patchRoleBindingOwnerRef(
+        roleBinding.metadata.name,
+        roleBinding.metadata.namespace,
+        ownerReferences,
+      ).catch((e) => {
+        // This is not ideal, but it shouldn't impact the starting of the notebook. Let us log it, and mute the error
+        // eslint-disable-next-line no-console
+        console.error(
+          `Could not patch rolebinding to service account for notebook, ${notebookName}; Reason ${e.message}`,
+        );
+      });
+    }
+    return createRoleBinding(
+      generateElyraServiceAccountRoleBinding(notebookName, namespace, notebookUid),
+    ).catch((e) => {
+      // eslint-disable-next-line no-console
+      console.error(
+        `Could not create rolebinding to service account for notebook, ${notebookName}; Reason ${e.message}`,
+      );
+    });
+  }
+
+  return undefined;
+};
diff --git a/frontend/src/pages/projects/notebook/NotebookStatusToggle.tsx b/frontend/src/pages/projects/notebook/NotebookStatusToggle.tsx
@@ -104,8 +104,7 @@ const NotebookStatusToggle: React.FC<NotebookStatusToggleProps> = ({
                   notebookState.notebook,
                 );
                 startNotebook(
-                  notebookName,
-                  notebookNamespace,
+                  notebook,
                   tolerationSettings,
                   enablePipelines && !currentlyHasPipelines(notebook),
                 ).then(() => {