diff --git a/backend/src/__tests__/objUtils.spec.ts b/backend/src/__tests__/objUtils.spec.ts new file mode 100644 index 0000000000..6915693a3b --- /dev/null +++ b/backend/src/__tests__/objUtils.spec.ts @@ -0,0 +1,69 @@ +import { smartMergeArraysWithNameObjects } from '../utils/objUtils'; + +describe('objUtils', () => { + describe('smartMergeArraysWithNameObjects', () => { + /** we only use the first two params of the customization utility, so ignore the last 3 */ + const smartMergeArraysWithNameObjectsWithUsedParams = (v1: any, v2: any) => + smartMergeArraysWithNameObjects(v1, v2, undefined, undefined, undefined); + + it('should do nothing with nulls', () => { + expect(smartMergeArraysWithNameObjectsWithUsedParams(null, null)).toBe(undefined); + }); + + it('should do nothing with objects', () => { + expect( + smartMergeArraysWithNameObjectsWithUsedParams({ a: true }, { a: false, b: true }), + ).toBe(undefined); + }); + + it('should do nothing with string[]', () => { + expect(smartMergeArraysWithNameObjectsWithUsedParams(['test'], ['test2'])).toBe(undefined); + }); + + it('should do nothing with invalid object arrays', () => { + expect( + smartMergeArraysWithNameObjectsWithUsedParams([{ id: 'test' }], [{ id: 'test2' }]), + ).toBe(undefined); + }); + + it('should replace 2nd object if given two same correct objects arrays', () => { + expect( + smartMergeArraysWithNameObjectsWithUsedParams( + [{ name: 'test', value: '1' }], + [{ name: 'test', value: '2' }], + ), + ).toEqual([{ name: 'test', value: '2' }]); + }); + + it('should add 2nd object if given two different correct object arrays', () => { + expect( + smartMergeArraysWithNameObjectsWithUsedParams( + [{ name: 'test', value: '1' }], + [{ name: 'test2', value: '2' }], + ), + ).toEqual([ + { name: 'test', value: '1' }, + { name: 'test2', value: '2' }, + ]); + }); + + it('should replace and add as appropriate if given two correct object arrays', () => { + expect( + smartMergeArraysWithNameObjectsWithUsedParams( + [ + { name: 'test', value: '1' }, + { name: 'test3', value: '3' }, + ], + [ + { name: 'test', value: '1b' }, + { name: 'test2', value: '2' }, + ], + ), + ).toEqual([ + { name: 'test', value: '1b' }, + { name: 'test3', value: '3' }, + { name: 'test2', value: '2' }, + ]); + }); + }); +}); diff --git a/backend/src/routes/api/notebooks/utils.ts b/backend/src/routes/api/notebooks/utils.ts index c4de264e86..b3efc2a3fe 100644 --- a/backend/src/routes/api/notebooks/utils.ts +++ b/backend/src/routes/api/notebooks/utils.ts @@ -120,8 +120,8 @@ export const enableNotebook = async ( const url = request.headers.origin; try { - await getNotebook(fastify, notebookNamespace, name); - return await updateNotebook(fastify, username, url, notebookData); + const notebook = await getNotebook(fastify, notebookNamespace, name); + return await updateNotebook(fastify, username, url, notebookData, notebook); } catch (e) { if (e.response?.statusCode === 404) { return await createNotebook(fastify, username, url, notebookData); diff --git a/backend/src/types.ts b/backend/src/types.ts index bde7b3a527..a65214c6d4 100644 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -766,7 +766,12 @@ export type DetectedAccelerators = { export type EnvironmentVariable = EitherNotBoth< { value: string | number }, - { valueFrom: Record } + { + valueFrom: Record & { + configMapKeyRef?: { key: string; name: string }; + secretKeyRef?: { key: string; name: string }; + }; + } > & { name: string; }; diff --git a/backend/src/utils/notebookUtils.ts b/backend/src/utils/notebookUtils.ts index ba93cba7aa..319d4851d9 100644 --- a/backend/src/utils/notebookUtils.ts +++ b/backend/src/utils/notebookUtils.ts @@ -1,4 +1,5 @@ import { getDashboardConfig } from './resourceUtils'; +import { mergeWith } from 'lodash'; import { ContainerResourceAttributes, EnvironmentVariable, @@ -30,6 +31,7 @@ import { import { DEFAULT_NOTEBOOK_SIZES, DEFAULT_PVC_SIZE, MOUNT_PATH } from './constants'; import { FastifyRequest } from 'fastify'; import { verifyEnvVars } from './envUtils'; +import { smartMergeArraysWithNameObjects } from './objUtils'; import { getImageInfo } from '../routes/api/images/imageUtils'; export const generateNotebookNameFromUsername = (username: string): string => @@ -510,6 +512,7 @@ export const updateNotebook = async ( username: string, url: string, notebookData: NotebookData, + oldNotebook: Notebook, ): Promise => { if (!notebookData) { const error = createCustomError( @@ -521,7 +524,42 @@ export const updateNotebook = async ( throw error; } try { - const notebookAssembled = await generateNotebookResources(fastify, username, url, notebookData); + const serverNotebook = await generateNotebookResources(fastify, username, url, notebookData); + + // Fix for Workbench Certs that get overridden + // We are intentionally applying on some details as to avoid implementing logic to properly + // manage the notebook the same way as workbench + const importantOldNotebookDetails: RecursivePartial = { + spec: { + template: { + spec: { + containers: [ + { + // Drop all env vars we added in the past, because we will just add them back if they are still there + env: oldNotebook.spec.template.spec.containers[0].env.filter(({ valueFrom }) => { + if (!valueFrom) { + return true; + } else { + const value = valueFrom.secretKeyRef ?? valueFrom.configMapKeyRef; + return !value?.name?.startsWith('jupyterhub-singleuser-profile'); + } + }), + volumeMounts: oldNotebook.spec.template.spec.containers[0].volumeMounts, + }, + ], + volumes: oldNotebook.spec.template.spec.volumes, + }, + }, + }, + }; + + const notebookAssembled = mergeWith( + {}, + importantOldNotebookDetails, + serverNotebook, + smartMergeArraysWithNameObjects, + ); + const response = await fastify.kube.customObjectsApi.patchNamespacedCustomObject( 'kubeflow.org', 'v1', diff --git a/backend/src/utils/objUtils.ts b/backend/src/utils/objUtils.ts new file mode 100644 index 0000000000..db78e8381b --- /dev/null +++ b/backend/src/utils/objUtils.ts @@ -0,0 +1,30 @@ +import { MergeWithCustomizer } from 'lodash'; + +/** + * When given two object arrays that have "name" keys, replace when keys are the same, or add to + * the end when new key. + * + * Note: returns `undefined` if invalid data is provided. + * + * @see mergeWith -- lodash mergeWith customizer + */ +export const smartMergeArraysWithNameObjects: MergeWithCustomizer = (objValue, srcValue) => { + type GoodArray = { name: string }[]; + const isGoodArray = (v: any): v is GoodArray => Array.isArray(v) && v.length > 0 && v[0].name; + if (isGoodArray(objValue) && isGoodArray(srcValue)) { + // Arrays with objects that have a .name property, sync on merge for the name + return srcValue.reduce((newArray, elm) => { + const key = elm.name; + const index = newArray.findIndex(({ name }) => name === key); + if (index >= 0) { + // existing value, replace + newArray[index] = elm; + } else { + // didn't find existing name, add to end + newArray.push(elm); + } + + return newArray; + }, objValue); + } +};