From 99d29310dcb5504c38ca5cea2243d033309319e5 Mon Sep 17 00:00:00 2001 From: pnaik1 Date: Tue, 22 Oct 2024 17:26:29 +0530 Subject: [PATCH] Deleting workbench resource makes workbench editable --- .../src/__mocks__/mockNotebookK8sResource.ts | 20 +- .../cypress/cypress/pages/workbench.ts | 4 + .../mocked/projects/dataConnection.cy.ts | 6 +- .../tests/mocked/projects/workbench.cy.ts | 226 +++++++++++++++++- .../src/api/k8s/__tests__/notebooks.spec.ts | 4 +- .../projects/screens/spawner/SpawnerPage.tsx | 8 +- .../environmentVariables/AlertWarningText.tsx | 96 ++++++++ .../useNotebookEnvVariables.tsx | 47 +++- .../spawner/environmentVariables/utils.ts | 25 +- .../pages/projects/screens/spawner/service.ts | 8 +- 10 files changed, 414 insertions(+), 30 deletions(-) create mode 100644 frontend/src/pages/projects/screens/spawner/environmentVariables/AlertWarningText.tsx diff --git a/frontend/src/__mocks__/mockNotebookK8sResource.ts b/frontend/src/__mocks__/mockNotebookK8sResource.ts index ef8e631075..3235e20141 100644 --- a/frontend/src/__mocks__/mockNotebookK8sResource.ts +++ b/frontend/src/__mocks__/mockNotebookK8sResource.ts @@ -4,6 +4,7 @@ import { DEFAULT_NOTEBOOK_SIZES } from '~/pages/projects/screens/spawner/const'; import { ContainerResources, TolerationEffect, TolerationOperator, VolumeMount } from '~/types'; import { genUID } from '~/__mocks__/mockUtils'; import { RecursivePartial } from '~/typeHelpers'; +import { EnvironmentFromVariable } from '~/pages/projects/types'; type MockResourceConfigType = { name?: string; @@ -11,7 +12,7 @@ type MockResourceConfigType = { namespace?: string; user?: string; description?: string; - envFromName?: string; + envFrom?: EnvironmentFromVariable[]; resources?: ContainerResources; image?: string; lastImageSelection?: string; @@ -23,7 +24,14 @@ type MockResourceConfigType = { export const mockNotebookK8sResource = ({ name = 'test-notebook', displayName = 'Test Notebook', - envFromName = 'aws-connection-db-1', + envFrom = [ + { + secretRef: { + name: 'secret', + }, + }, + ], + namespace = 'test-project', user = 'test-user', description = '', @@ -99,13 +107,7 @@ export const mockNotebookK8sResource = ({ 'image-registry.openshift-image-registry.svc:5000/opendatahub/code-server-notebook:2023.2', }, ], - envFrom: [ - { - secretRef: { - name: envFromName, - }, - }, - ], + envFrom, image, imagePullPolicy: 'Always', livenessProbe: { diff --git a/frontend/src/__tests__/cypress/cypress/pages/workbench.ts b/frontend/src/__tests__/cypress/cypress/pages/workbench.ts index b4b771b97c..71dc84e4ef 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/workbench.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/workbench.ts @@ -374,6 +374,10 @@ class EditSpawnerPage extends CreateSpawnerPage { return this; } + findAlertMessage() { + return cy.findByTestId('env-variable-alert-message'); + } + findCancelButton() { return cy.findByTestId('workbench-cancel-button'); } diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/dataConnection.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/dataConnection.cy.ts index af09be021f..c9ee99d261 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/dataConnection.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/dataConnection.cy.ts @@ -54,7 +54,7 @@ const initIntercepts = ({ isEmpty = false }: HandlersProps) => { cy.interceptK8sList(PodModel, mockK8sResourceList([mockPodK8sResource({})])); cy.interceptK8sList( NotebookModel, - mockK8sResourceList([mockNotebookK8sResource({ envFromName: '' })]), + mockK8sResourceList([mockNotebookK8sResource({ envFrom: [] })]), ); cy.interceptK8sList( ProjectModel, @@ -173,7 +173,9 @@ describe('Data connections', () => { editDataConnectionModal.findNotebookRestartAlert().should('exist'); cy.interceptK8sList( NotebookModel, - mockK8sResourceList([mockNotebookK8sResource({ envFromName: 'test-secret' })]), + mockK8sResourceList([ + mockNotebookK8sResource({ envFrom: [{ secretRef: { name: 'test-secret' } }] }), + ]), ).as('addConnectedWorkbench'); cy.interceptK8s('PUT', SecretModel, mockSecretK8sResource({})).as('editDataConnection'); diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts index ee06548f5f..2b02a1a194 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts @@ -1,4 +1,5 @@ import { + mockCustomSecretK8sResource, mockDashboardConfig, mockDscStatus, mockK8sResourceList, @@ -35,20 +36,23 @@ import { StorageClassModel, AcceleratorProfileModel, } from '~/__tests__/cypress/cypress/utils/models'; -import { mock200Status } from '~/__mocks__/mockK8sStatus'; +import { mock200Status, mock404Error } from '~/__mocks__/mockK8sStatus'; import type { NotebookSize } from '~/types'; import { mockAcceleratorProfile } from '~/__mocks__/mockAcceleratorProfile'; import { mockConnectionTypeConfigMap } from '~/__mocks__/mockConnectionType'; +import type { EnvironmentFromVariable } from '~/pages/projects/types'; const configYamlPath = '../../__mocks__/mock-upload-configmap.yaml'; type HandlersProps = { isEmpty?: boolean; notebookSizes?: NotebookSize[]; + envFrom?: EnvironmentFromVariable[]; }; const initIntercepts = ({ isEmpty = false, + envFrom, notebookSizes = [ { name: 'Medium', @@ -136,6 +140,23 @@ const initIntercepts = ({ }), ]), ); + cy.interceptK8s( + { model: SecretModel, ns: 'test-project', name: 'secret' }, + mockCustomSecretK8sResource({ + name: 'secret', + namespace: 'test-project', + data: { test: 'c2RzZA==' }, + }), + ); + cy.interceptK8s( + 'PUT', + { model: SecretModel, ns: 'test-project', name: 'secret' }, + mockCustomSecretK8sResource({ + name: 'secret', + namespace: 'test-project', + data: { test: 'c2RzZA==' }, + }), + ); cy.interceptK8s(SecretModel, mockSecretK8sResource({ name: 'aws-connection-db-1' })); cy.interceptK8s('PATCH', NotebookModel, mockNotebookK8sResource({})).as('stopWorkbench'); cy.interceptK8s(RouteModel, mockRouteK8sResource({ notebookName: 'test-notebook' })); @@ -149,6 +170,7 @@ const initIntercepts = ({ ? [] : [ mockNotebookK8sResource({ + envFrom, opts: { metadata: { name: 'test-notebook', @@ -632,6 +654,7 @@ describe('Workbench page', () => { mockK8sResourceList([mockPVCK8sResource({ name: 'test-notebook' })]), ); editSpawnerPage.visit('test-notebook'); + editSpawnerPage.findAlertMessage().should('not.exist'); editSpawnerPage.k8sNameDescription.findDisplayNameInput().should('have.value', 'Test Notebook'); editSpawnerPage.shouldHaveNotebookImageSelectInput('Test Image'); editSpawnerPage.shouldHaveContainerSizeInput('Small'); @@ -661,6 +684,19 @@ describe('Workbench page', () => { spec: { template: { spec: { + containers: [ + { + envFrom: [ + { + secretRef: { + name: 'secret', + }, + }, + ], + + name: 'test-notebook', + }, + ], volumes: [ { name: 'test-notebook', persistentVolumeClaim: { claimName: 'test-notebook' } }, ], @@ -675,6 +711,157 @@ describe('Workbench page', () => { }); }); + it('Edit workbench when either configMap or secret variables not present', () => { + initIntercepts({ + envFrom: [ + { + secretRef: { + name: 'secret-1', + }, + }, + { + secretRef: { + name: 'secret-2', + }, + }, + ], + }); + cy.interceptK8s( + { + model: SecretModel, + ns: 'test-project', + name: 'secret-1', + }, + { + statusCode: 404, + body: mock404Error({}), + }, + ); + cy.interceptK8s( + { + model: SecretModel, + ns: 'test-project', + name: 'secret-2', + }, + { + statusCode: 404, + body: mock404Error({}), + }, + ); + editSpawnerPage.visit('test-notebook'); + editSpawnerPage.findAlertMessage().should('exist'); + editSpawnerPage.findAlertMessage().contains('secret-1 and secret-2'); + cy.interceptK8s('PUT', NotebookModel, mockNotebookK8sResource({})).as('editWorkbenchDryRun'); + cy.interceptK8s('PATCH', NotebookModel, mockNotebookK8sResource({})).as('editWorkbench'); + editSpawnerPage.findSubmitButton().click(); + cy.wait('@editWorkbenchDryRun').then((interception) => { + expect(interception.request.url).to.include('?dryRun=All'); + expect(interception.request.body).to.containSubset({ + metadata: { + annotations: { + 'openshift.io/description': '', + 'openshift.io/display-name': 'Test Notebook', + 'opendatahub.io/image-display-name': 'Test Image', + 'opendatahub.io/accelerator-name': '', + }, + }, + spec: { + template: { + spec: { + containers: [ + { + envFrom: [], + + name: 'test-notebook', + }, + ], + }, + }, + }, + }); + }); + // Actual request + cy.wait('@editWorkbench').then((interception) => { + expect(interception.request.url).not.to.include('?dryRun=All'); + }); + }); + + it('Edit workbench when both configMap and secret are deleted', () => { + initIntercepts({ + envFrom: [ + { + secretRef: { + name: 'secret-1', + }, + }, + { + configMapRef: { + name: 'secret-2', + }, + }, + ], + }); + cy.interceptK8s( + { + model: SecretModel, + ns: 'test-project', + name: 'secret-1', + }, + { + statusCode: 404, + body: mock404Error({}), + }, + ); + cy.interceptK8s( + { + model: ConfigMapModel, + ns: 'test-project', + name: 'secret-2', + }, + { + statusCode: 404, + body: mock404Error({}), + }, + ); + editSpawnerPage.visit('test-notebook'); + editSpawnerPage.findAlertMessage().should('exist'); + editSpawnerPage.findAlertMessage().contains('secret-1 secret'); + editSpawnerPage.findAlertMessage().contains('secret-2 config map'); + cy.interceptK8s('PUT', NotebookModel, mockNotebookK8sResource({})).as('editWorkbenchDryRun'); + cy.interceptK8s('PATCH', NotebookModel, mockNotebookK8sResource({})).as('editWorkbench'); + editSpawnerPage.findSubmitButton().click(); + cy.wait('@editWorkbenchDryRun').then((interception) => { + expect(interception.request.url).to.include('?dryRun=All'); + expect(interception.request.body).to.containSubset({ + metadata: { + annotations: { + 'openshift.io/description': '', + 'openshift.io/display-name': 'Test Notebook', + 'opendatahub.io/image-display-name': 'Test Image', + 'opendatahub.io/accelerator-name': '', + }, + }, + spec: { + template: { + spec: { + containers: [ + { + envFrom: [], + + name: 'test-notebook', + }, + ], + }, + }, + }, + }); + }); + // Actual request + cy.wait('@editWorkbench').then((interception) => { + expect(interception.request.url).not.to.include('?dryRun=All'); + }); + }); + it('Handle deleted notebook sizes in workbenches table', () => { initIntercepts({ notebookSizes: [ @@ -726,7 +913,42 @@ describe('Workbench page', () => { }); it('Delete Workbench', () => { - initIntercepts({}); + initIntercepts({ + envFrom: [ + { + secretRef: { + name: 'secret-1', + }, + }, + { + configMapRef: { + name: 'secret-2', + }, + }, + ], + }); + cy.interceptK8s( + { + model: SecretModel, + ns: 'test-project', + name: 'secret-1', + }, + { + statusCode: 404, + body: mock404Error({}), + }, + ); + cy.interceptK8s( + { + model: ConfigMapModel, + ns: 'test-project', + name: 'secret-2', + }, + { + statusCode: 404, + body: mock404Error({}), + }, + ); workbenchPage.visit('test-project'); const notebookRow = workbenchPage.getNotebookRow('Test Notebook'); notebookRow.findKebabAction('Delete workbench').click(); diff --git a/frontend/src/api/k8s/__tests__/notebooks.spec.ts b/frontend/src/api/k8s/__tests__/notebooks.spec.ts index bbdc9476e1..cb51f1a3c6 100644 --- a/frontend/src/api/k8s/__tests__/notebooks.spec.ts +++ b/frontend/src/api/k8s/__tests__/notebooks.spec.ts @@ -908,7 +908,7 @@ describe('removeNotebookSecret', () => { { op: 'replace', path: '/spec/template/spec/containers/0/envFrom', - value: [{ secretRef: { name: 'aws-connection-db-1' } }], + value: [{ secretRef: { name: 'secret' } }], }, ], queryOptions: { name, ns: namespace }, @@ -982,7 +982,7 @@ describe('removeNotebookSecret', () => { { op: 'replace', path: '/spec/template/spec/containers/0/envFrom', - value: [{ secretRef: { name: 'aws-connection-db-1' } }], + value: [{ secretRef: { name: 'secret' } }], }, ], queryOptions: { name, ns: namespace }, diff --git a/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx b/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx index f78c79282a..571565ed05 100644 --- a/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx +++ b/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx @@ -50,6 +50,7 @@ import useDefaultStorageClass from './storage/useDefaultStorageClass'; import usePreferredStorageClass from './storage/usePreferredStorageClass'; import { ConnectionsFormSection } from './connections/ConnectionsFormSection'; import { getConnectionsFromNotebook } from './connections/utils'; +import AlertWarningText from './environmentVariables/AlertWarningText'; type SpawnerPageProps = { existingNotebook?: NotebookKind; @@ -90,7 +91,9 @@ const SpawnerPage: React.FC = ({ existingNotebook }) => { defaultStorageClassName, ); - const [envVariables, setEnvVariables] = useNotebookEnvVariables(existingNotebook); + const [envVariables, setEnvVariables, envVariableloaded] = + useNotebookEnvVariables(existingNotebook); + const [dataConnectionData, setDataConnectionData] = useNotebookDataConnection( dataConnections.data, existingNotebook, @@ -216,6 +219,9 @@ const SpawnerPage: React.FC = ({ existingNotebook }) => { id={SpawnerPageSectionID.ENVIRONMENT_VARIABLES} aria-label={SpawnerPageSectionTitles[SpawnerPageSectionID.ENVIRONMENT_VARIABLES]} > + {envVariableloaded && ( + + )} = ({ notebook, envVariables }) => { + const { deletedConfigMaps, deletedSecrets } = getDeletedConfigMapOrSecretVariables( + notebook, + envVariables, + ); + + if (deletedConfigMaps.length === 0 && deletedSecrets.length === 0) { + return null; + } + + const formattedText = (items: string[], type: 'config map' | 'secret') => { + if (items.length === 1) { + return ( + <> + {items[0]} {type} + + ); + } + const lastItem = items[items.length - 1]; + const displayItems = items.slice(0, -1); + return ( + <> + {displayItems.map((item, index) => ( + <> + {item} + {index < displayItems.length - 1 ? ', ' : ' and '} + + ))} + {lastItem} {type}s + + ); + }; + + let description; + const commonDescriptionText = `To remove the orphaned connection, save the workbench.`; + const formattedConfigMapsText = formattedText(deletedConfigMaps, 'config map'); + const formattedSecretsText = formattedText(deletedSecrets, 'secret'); + + if (deletedConfigMaps.length > 0 && deletedSecrets.length > 0) { + description = ( + + + The following environment variables in this workbench cannot be found: + + + + {[formattedConfigMapsText, formattedSecretsText].map((text, i) => ( + {text} + ))} + + + {commonDescriptionText} + + ); + } else if (deletedConfigMaps.length > 0) { + description = ( + <> + The {formattedConfigMapsText} + {deletedConfigMaps.length === 1 ? ' cannot be found. ' : ' are not found. '} + {commonDescriptionText} + + ); + } else { + description = ( + <> + The {formattedSecretsText} + {deletedSecrets.length === 1 ? ' cannot be found. ' : ' are not found. '} + {commonDescriptionText} + + ); + } + + return ( + + {description} + + ); +}; + +export default AlertWarningText; diff --git a/frontend/src/pages/projects/screens/spawner/environmentVariables/useNotebookEnvVariables.tsx b/frontend/src/pages/projects/screens/spawner/environmentVariables/useNotebookEnvVariables.tsx index 35326a5e52..a4854683ce 100644 --- a/frontend/src/pages/projects/screens/spawner/environmentVariables/useNotebookEnvVariables.tsx +++ b/frontend/src/pages/projects/screens/spawner/environmentVariables/useNotebookEnvVariables.tsx @@ -8,6 +8,7 @@ import { EnvVariable, SecretCategory, } from '~/pages/projects/types'; +import useFetchState, { NotReadyError } from '~/utilities/useFetchState'; import { isConnection } from '~/concepts/connectionTypes/utils'; import { isSecretKind } from './utils'; @@ -17,20 +18,33 @@ export const fetchNotebookEnvVariables = (notebook: NotebookKind): Promise { if (envFrom.configMapRef) { - return getConfigMap(notebook.metadata.namespace, envFrom.configMapRef.name); + return getConfigMap(notebook.metadata.namespace, envFrom.configMapRef.name).catch((e) => { + if (e.statusObject?.code === 404) { + return null; + } + throw e; + }); } if (envFrom.secretRef) { - return getSecret(notebook.metadata.namespace, envFrom.secretRef.name); + return getSecret(notebook.metadata.namespace, envFrom.secretRef.name).catch((e) => { + if (e.statusObject?.code === 404) { + return null; + } + throw e; + }); } return Promise.resolve(undefined); }) .filter( ( - v: Promise | Promise | undefined, - ): v is Promise => !!v, + v: Promise | Promise | undefined, + ): v is Promise => !!v, ), ).then((results) => results.reduce((acc, resource) => { + if (!resource) { + return acc; + } const { data } = resource; let envVar: EnvVariable; if (resource.kind === EnvVarResourceType.ConfigMap) { @@ -61,17 +75,26 @@ export const fetchNotebookEnvVariables = (notebook: NotebookKind): Promise void] => { +): [ + envVariables: EnvVariable[], + setEnvVariables: (envVars: EnvVariable[]) => void, + envVariableLoaded: boolean, +] => { const [envVariables, setEnvVariables] = React.useState([]); - React.useEffect(() => { - if (notebook) { - fetchNotebookEnvVariables(notebook) - .then((envVars) => setEnvVariables(envVars)) - /* eslint-disable-next-line no-console */ - .catch((e) => console.error('Reading environment variables failed: ', e)); + const callback = React.useCallback(() => { + if (!notebook) { + return Promise.reject(new NotReadyError('No notebook')); } + + return fetchNotebookEnvVariables(notebook); }, [notebook]); - return [envVariables, setEnvVariables]; + const [fetchedVariable, envVariableloaded] = useFetchState(callback, []); + + React.useEffect(() => { + setEnvVariables(fetchedVariable); + }, [fetchedVariable]); + + return [envVariables, setEnvVariables, envVariableloaded]; }; diff --git a/frontend/src/pages/projects/screens/spawner/environmentVariables/utils.ts b/frontend/src/pages/projects/screens/spawner/environmentVariables/utils.ts index e73f077feb..6f404ce4f9 100644 --- a/frontend/src/pages/projects/screens/spawner/environmentVariables/utils.ts +++ b/frontend/src/pages/projects/screens/spawner/environmentVariables/utils.ts @@ -1,4 +1,5 @@ -import { ConfigMapKind, SecretKind } from '~/k8sTypes'; +import { ConfigMapKind, NotebookKind, SecretKind } from '~/k8sTypes'; +import { EnvVariable } from '~/pages/projects/types'; export const updateArrayValue = (values: T[], index: number, partialValue: Partial): T[] => values.map((v, i) => (i === index ? { ...v, ...partialValue } : v)); @@ -18,3 +19,25 @@ export const isStringKeyValuePairObject = (object: unknown): object is Record typeof key === 'string' && typeof value === 'string', ); + +export const getDeletedConfigMapOrSecretVariables = ( + notebook: NotebookKind | undefined, + envVariables: EnvVariable[], +): { + deletedSecrets: string[]; + deletedConfigMaps: string[]; +} => { + const existingEnvVariables = new Set(envVariables.map((env) => env.existingName)); + const deletedConfigMaps: string[] = []; + const deletedSecrets: string[] = []; + + (notebook?.spec.template.spec.containers[0].envFrom || []).forEach((env) => { + if (env.configMapRef && !existingEnvVariables.has(env.configMapRef.name)) { + deletedConfigMaps.push(env.configMapRef.name); + } + if (env.secretRef && !existingEnvVariables.has(env.secretRef.name)) { + deletedSecrets.push(env.secretRef.name); + } + }); + return { deletedSecrets, deletedConfigMaps }; +}; diff --git a/frontend/src/pages/projects/screens/spawner/service.ts b/frontend/src/pages/projects/screens/spawner/service.ts index cd6dbb1d0c..8bcf97fa86 100644 --- a/frontend/src/pages/projects/screens/spawner/service.ts +++ b/frontend/src/pages/projects/screens/spawner/service.ts @@ -26,6 +26,7 @@ import { ROOT_MOUNT_PATH } from '~/pages/projects/pvc/const'; import { ConfigMapKind, NotebookKind, SecretKind } from '~/k8sTypes'; import { getVolumesByStorageData } from './spawnerUtils'; import { fetchNotebookEnvVariables } from './environmentVariables/useNotebookEnvVariables'; +import { getDeletedConfigMapOrSecretVariables } from './environmentVariables/utils'; export const createPvcDataForNotebook = async ( projectName: string, @@ -196,6 +197,10 @@ export const updateConfigMapsAndSecretsForNotebook = async ( dryRun = false, ): Promise => { const existingEnvVars = await fetchNotebookEnvVariables(notebook); + const { deletedConfigMaps, deletedSecrets } = getDeletedConfigMapOrSecretVariables( + notebook, + existingEnvVars, + ); const newDataConnection = dataConnection?.enabled && dataConnection.type === 'creating' && dataConnection.creating ? dataConnection.creating @@ -217,6 +222,7 @@ export const updateConfigMapsAndSecretsForNotebook = async ( const currentNames = oldResources .map((envVar) => envVar.existingName) .filter((v): v is string => !!v); + const removeResources = existingEnvVars.filter( (envVar) => envVar.existingName && !currentNames.includes(envVar.existingName), ); @@ -267,7 +273,7 @@ export const updateConfigMapsAndSecretsForNotebook = async ( ]); const deletingNames = deleteResources.map((resource) => resource.existingName || ''); - deletingNames.push(...removeDataConnections); + deletingNames.push(...removeDataConnections, ...deletedSecrets, ...deletedConfigMaps); const envFromList = notebook.spec.template.spec.containers[0].envFrom || [];