diff --git a/frontend/src/__mocks__/mockStartNotebookData.ts b/frontend/src/__mocks__/mockStartNotebookData.ts index fe26856b90..b01f705ea3 100644 --- a/frontend/src/__mocks__/mockStartNotebookData.ts +++ b/frontend/src/__mocks__/mockStartNotebookData.ts @@ -1,5 +1,14 @@ import { ImageStreamKind } from '~/k8sTypes'; -import { StartNotebookData } from '~/pages/projects/types'; +import { + ConfigMapCategory, + DataConnectionData, + EnvironmentVariableType, + EnvVariable, + SecretCategory, + StartNotebookData, + StorageData, + StorageType, +} from '~/pages/projects/types'; import { mockK8sNameDescriptionFieldData } from '~/__mocks__/mockK8sNameDescriptionFieldData'; type MockResourceConfigType = { @@ -84,3 +93,80 @@ export const mockStartNotebookData = ({ }, }, }); + +export const mockStorageData: StorageData = { + storageType: StorageType.NEW_PVC, + creating: { + nameDesc: { + name: 'test-pvc', + description: '', + }, + size: '20Gi', + storageClassName: 'gp2-csi', + }, + existing: { storage: '' }, +}; + +export const mockDataConnectionData: DataConnectionData = { + type: 'creating', + enabled: true, + creating: { + type: EnvironmentVariableType.SECRET, + values: { + category: SecretCategory.AWS, + data: [ + { + key: 'Name', + value: 'test-name', + }, + { + key: 'AWS_ACCESS_KEY_ID', + value: 'test-access-key', + }, + { + key: 'AWS_SECRET_ACCESS_KEY', + value: 'test-secret-key', + }, + { + key: 'AWS_S3_BUCKET', + value: '', + }, + { + key: 'AWS_S3_ENDPOINT', + value: 'test-endpoint', + }, + { + key: 'AWS_DEFAULT_REGION', + value: '', + }, + ], + }, + }, +}; + +export const mockEnvVariables: EnvVariable[] = [ + { + type: EnvironmentVariableType.CONFIG_MAP, + values: { + category: ConfigMapCategory.GENERIC, + data: [ + { + key: 'test-key', + value: 'test-value', + }, + ], + }, + }, + { + type: EnvironmentVariableType.SECRET, + values: { + category: SecretCategory.GENERIC, + data: [ + { + key: 'test-key', + value: 'test-value', + }, + ], + }, + }, +]; diff --git a/frontend/src/api/errorUtils.ts b/frontend/src/api/errorUtils.ts index 1ac6f1c5e9..a977172df1 100644 --- a/frontend/src/api/errorUtils.ts +++ b/frontend/src/api/errorUtils.ts @@ -5,7 +5,7 @@ export const isK8sStatus = (data: unknown): data is K8sStatus => typeof data === 'object' && data !== null && 'kind' in data && data.kind === 'Status'; export class K8sStatusError extends Error { - public statusObject: K8sStatus; + public statusObject: K8sStatus & { details?: { kind?: string } }; constructor(statusObject: K8sStatus) { super(statusObject.message); diff --git a/frontend/src/api/k8s/__tests__/notebooks.spec.ts b/frontend/src/api/k8s/__tests__/notebooks.spec.ts index b989ef17db..e641102339 100644 --- a/frontend/src/api/k8s/__tests__/notebooks.spec.ts +++ b/frontend/src/api/k8s/__tests__/notebooks.spec.ts @@ -161,7 +161,7 @@ describe('assembleNotebook', () => { startNotebookDataMock.volumeMounts = undefined; startNotebookDataMock.volumes = undefined; - const Notebook = assembleNotebook(startNotebookDataMock, username, true); + const notebook = assembleNotebook(startNotebookDataMock, username, true); k8sCreateResourceMock.mockResolvedValue(mockNotebookK8sResource({ uid: 'test' })); createElyraServiceAccountRoleBindingMock.mockResolvedValue( @@ -171,11 +171,16 @@ describe('assembleNotebook', () => { const renderResult = await createNotebook(startNotebookDataMock, username, true); expect(k8sCreateResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, model: NotebookModel, - resource: Notebook, + resource: notebook, + queryOptions: { + queryParams: {}, + }, }); expect(createElyraServiceAccountRoleBindingMock).toHaveBeenCalledWith( mockNotebookK8sResource({ uid: 'test' }), + undefined, ); expect(createElyraServiceAccountRoleBindingMock).toHaveBeenCalledTimes(1); expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1); @@ -185,7 +190,7 @@ describe('assembleNotebook', () => { const startNotebookMock = mockStartNotebookData({}); startNotebookMock.volumes = undefined; startNotebookMock.volumeMounts = undefined; - const Notebook = assembleNotebook(startNotebookMock, username, false); + const notebook = assembleNotebook(startNotebookMock, username, false); k8sCreateResourceMock.mockResolvedValue(mockNotebookK8sResource({ uid: 'test' })); createElyraServiceAccountRoleBindingMock.mockResolvedValue( @@ -195,8 +200,12 @@ describe('assembleNotebook', () => { const renderResult = await createNotebook(startNotebookMock, username, false); expect(k8sCreateResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, model: NotebookModel, - resource: Notebook, + resource: notebook, + queryOptions: { + queryParams: {}, + }, }); expect(createElyraServiceAccountRoleBindingMock).toHaveBeenCalledTimes(0); expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1); @@ -218,14 +227,18 @@ describe('createNotebook', () => { expect(createElyraServiceAccountRoleBinding).not.toHaveBeenCalled(); expect(k8sCreateResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, model: NotebookModel, resource: notebook, + queryOptions: { + queryParams: {}, + }, }); expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1); expect(renderResult).toStrictEqual(mockNotebookK8sResource({ uid })); }); it('should create a notebook with pipelines', async () => { - const Notebook = assembleNotebook(mockStartNotebookData({}), username, true); + const notebook = assembleNotebook(mockStartNotebookData({}), username, true); k8sCreateResourceMock.mockResolvedValue(mockNotebookK8sResource({ uid })); createElyraServiceAccountRoleBindingMock.mockResolvedValue(mockRoleBindingK8sResource({ uid })); @@ -233,23 +246,31 @@ describe('createNotebook', () => { const renderResult = await createNotebook(mockStartNotebookData({}), username, true); expect(k8sCreateResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, model: NotebookModel, - resource: Notebook, + resource: notebook, + queryOptions: { + queryParams: {}, + }, }); expect(createElyraServiceAccountRoleBindingMock).toHaveBeenCalledTimes(1); expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1); expect(renderResult).toStrictEqual(mockNotebookK8sResource({ uid })); }); it('should handle errors and rethrow', async () => { - const Notebook = assembleNotebook(mockStartNotebookData({}), username, true); + const notebook = assembleNotebook(mockStartNotebookData({}), username, true); k8sCreateResourceMock.mockRejectedValue(new Error('error1')); await expect(createNotebook(mockStartNotebookData({}), username, true)).rejects.toThrow( 'error1', ); expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1); expect(k8sCreateResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, model: NotebookModel, - resource: Notebook, + resource: notebook, + queryOptions: { + queryParams: {}, + }, }); }); }); diff --git a/frontend/src/api/k8s/__tests__/roleBindings.spec.ts b/frontend/src/api/k8s/__tests__/roleBindings.spec.ts index bec6ded342..9e2b2beaf5 100644 --- a/frontend/src/api/k8s/__tests__/roleBindings.spec.ts +++ b/frontend/src/api/k8s/__tests__/roleBindings.spec.ts @@ -347,9 +347,10 @@ describe('patchRoleBindingOwnerRef', () => { k8sPatchResourceMock.mockResolvedValue(roleBindingMock); const result = await patchRoleBindingOwnerRef('rbName', namespace, []); expect(k8sPatchResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, model: RoleBindingModel, patches: [{ op: 'replace', path: '/metadata/ownerReferences', value: [] }], - queryOptions: { name: 'rbName', ns: namespace }, + queryOptions: { name: 'rbName', ns: namespace, queryParams: {} }, }); expect(k8sPatchResourceMock).toHaveBeenCalledTimes(1); expect(result).toStrictEqual(roleBindingMock); diff --git a/frontend/src/api/k8s/notebooks.ts b/frontend/src/api/k8s/notebooks.ts index 0be437d25c..99039899e2 100644 --- a/frontend/src/api/k8s/notebooks.ts +++ b/frontend/src/api/k8s/notebooks.ts @@ -256,23 +256,32 @@ export const createNotebook = ( data: StartNotebookData, username: string, canEnablePipelines?: boolean, -): Promise => { - const notebook = assembleNotebook(data, username, canEnablePipelines); + opts?: K8sAPIOptions, +): Promise => + new Promise((resolve, reject) => { + const notebook = assembleNotebook(data, username, canEnablePipelines); - const notebookPromise = k8sCreateResource({ - model: NotebookModel, - resource: notebook, + k8sCreateResource( + applyK8sAPIOptions( + { + model: NotebookModel, + resource: notebook, + }, + opts, + ), + ) + .then((fetchedNotebook) => { + if (canEnablePipelines) { + createElyraServiceAccountRoleBinding(fetchedNotebook, opts) + .then(() => resolve(fetchedNotebook)) + .catch(reject); + } else { + resolve(fetchedNotebook); + } + }) + .catch(reject); }); - if (canEnablePipelines) { - return notebookPromise.then((fetchedNotebook) => - createElyraServiceAccountRoleBinding(fetchedNotebook).then(() => fetchedNotebook), - ); - } - - return notebookPromise; -}; - export const updateNotebook = ( existingNotebook: NotebookKind, assignableData: StartNotebookData, diff --git a/frontend/src/api/k8s/roleBindings.ts b/frontend/src/api/k8s/roleBindings.ts index e373571818..e9d1147c92 100644 --- a/frontend/src/api/k8s/roleBindings.ts +++ b/frontend/src/api/k8s/roleBindings.ts @@ -132,15 +132,21 @@ export const patchRoleBindingOwnerRef = ( rbName: string, namespace: string, ownerReferences: OwnerReference[], + opts?: K8sAPIOptions, ): Promise => - k8sPatchResource({ - model: RoleBindingModel, - queryOptions: { name: rbName, ns: namespace }, - patches: [ + k8sPatchResource( + applyK8sAPIOptions( { - op: 'replace', - path: '/metadata/ownerReferences', - value: ownerReferences, + model: RoleBindingModel, + queryOptions: { name: rbName, ns: namespace }, + patches: [ + { + op: 'replace', + path: '/metadata/ownerReferences', + value: ownerReferences, + }, + ], }, - ], - }); + opts, + ), + ); diff --git a/frontend/src/concepts/pipelines/elyra/utils.ts b/frontend/src/concepts/pipelines/elyra/utils.ts index 172e9a757b..935f560a16 100644 --- a/frontend/src/concepts/pipelines/elyra/utils.ts +++ b/frontend/src/concepts/pipelines/elyra/utils.ts @@ -2,6 +2,7 @@ import { Patch } from '@openshift/dynamic-plugin-sdk-utils'; import { DSPipelineExternalStorageKind, ImageStreamSpecTagType, + K8sAPIOptions, KnownLabels, NotebookKind, RoleBindingKind, @@ -151,6 +152,7 @@ export const generateElyraServiceAccountRoleBinding = ( export const createElyraServiceAccountRoleBinding = async ( notebook: NotebookKind, + opts?: K8sAPIOptions, ): Promise => { const notebookName = notebook.metadata.name; const { namespace } = notebook.metadata; @@ -181,6 +183,7 @@ export const createElyraServiceAccountRoleBinding = async ( roleBinding.metadata.name, roleBinding.metadata.namespace, ownerReferences, + opts, ).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 @@ -191,6 +194,7 @@ export const createElyraServiceAccountRoleBinding = async ( } return createRoleBinding( generateElyraServiceAccountRoleBinding(notebookName, namespace, notebookUid), + opts, ).catch((e) => { // eslint-disable-next-line no-console console.error( diff --git a/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx b/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx index 5748bd473b..c693e944d1 100644 --- a/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx +++ b/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx @@ -9,14 +9,7 @@ import { Stack, StackItem, } from '@patternfly/react-core'; -import { - assembleSecret, - createNotebook, - createSecret, - K8sStatusError, - mergePatchUpdateNotebook, - updateNotebook, -} from '~/api'; +import { createNotebook, K8sStatusError, mergePatchUpdateNotebook, updateNotebook } from '~/api'; import { DataConnectionData, EnvVariable, @@ -25,7 +18,7 @@ import { } from '~/pages/projects/types'; import { useUser } from '~/redux/selectors'; import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext'; -import { AppContext } from '~/app/AppContext'; +import { useAppContext } from '~/app/AppContext'; import { ProjectSectionID } from '~/pages/projects/screens/detail/types'; import { fireFormTrackingEvent } from '~/concepts/analyticsTracking/segmentIOUtils'; import { @@ -62,7 +55,7 @@ const SpawnerFooter: React.FC = ({ dashboardConfig: { spec: { notebookController }, }, - } = React.useContext(AppContext); + } = useAppContext(); const tolerationSettings = notebookController?.notebookTolerationSettings; const { notebooks: { data }, @@ -146,7 +139,7 @@ const SpawnerFooter: React.FC = ({ editNotebook, storageData, dryRun, - ).catch(handleError); + ); const envFrom = await updateConfigMapsAndSecretsForNotebook( projectName, @@ -155,11 +148,7 @@ const SpawnerFooter: React.FC = ({ dataConnection, existingNotebookDataConnection, dryRun, - ).catch(handleError); - - if (!pvcDetails || !envFrom) { - return; - } + ); const annotations = { ...editNotebook.metadata.annotations }; if (envFrom.length > 0) { @@ -181,26 +170,6 @@ const SpawnerFooter: React.FC = ({ }; const onUpdateNotebook = async () => { - if (dataConnection.type === 'creating') { - const dataAsRecord = dataConnection.creating?.values?.data.reduce>( - (acc, { key, value }) => ({ ...acc, [key]: value }), - {}, - ); - if (dataAsRecord) { - const isSuccess = await createSecret(assembleSecret(projectName, dataAsRecord, 'aws'), { - dryRun: true, - }) - .then(() => true) - .catch((e) => { - handleError(e); - return false; - }); - if (!isSuccess) { - return; - } - } - } - handleStart(); updateNotebookPromise(true) .then(() => @@ -215,29 +184,7 @@ const SpawnerFooter: React.FC = ({ .catch(handleError); }; - const onCreateNotebook = async () => { - if (dataConnection.type === 'creating') { - const dataAsRecord = dataConnection.creating?.values?.data.reduce>( - (acc, { key, value }) => ({ ...acc, [key]: value }), - {}, - ); - if (dataAsRecord) { - const isSuccess = await createSecret(assembleSecret(projectName, dataAsRecord, 'aws'), { - dryRun: true, - }) - .then(() => true) - .catch((e) => { - handleError(e); - return false; - }); - if (!isSuccess) { - return; - } - } - } - - handleStart(); - + const createNotebookPromise = async (dryRun: boolean) => { const newDataConnection = dataConnection.enabled && dataConnection.type === 'creating' && dataConnection.creating ? [dataConnection.creating] @@ -246,17 +193,12 @@ const SpawnerFooter: React.FC = ({ dataConnection.enabled && dataConnection.type === 'existing' && dataConnection.existing ? [dataConnection.existing] : []; - - const pvcDetails = await createPvcDataForNotebook(projectName, storageData).catch(handleError); - const envFrom = await createConfigMapsAndSecretsForNotebook(projectName, [ - ...envVariables, - ...newDataConnection, - ]).catch(handleError); - - if (!pvcDetails || !envFrom) { - // Error happened, let the error code handle it - return; - } + const pvcDetails = await createPvcDataForNotebook(projectName, storageData, dryRun); + const envFrom = await createConfigMapsAndSecretsForNotebook( + projectName, + [...envVariables, ...newDataConnection], + dryRun, + ); const { volumes, volumeMounts } = pvcDetails; const newStartData: StartNotebookData = { @@ -266,9 +208,19 @@ const SpawnerFooter: React.FC = ({ envFrom: [...envFrom, ...existingDataConnection], tolerationSettings, }; + return createNotebook(newStartData, username, canEnablePipelines, { dryRun }); + }; - createNotebook(newStartData, username, canEnablePipelines) - .then((notebook) => afterStart(notebook.metadata.name, 'created')) + const onCreateNotebook = async () => { + handleStart(); + createNotebookPromise(true) + .then(() => + createNotebookPromise(false) + .then((notebook) => { + afterStart(notebook.metadata.name, 'created'); + }) + .catch(handleError), + ) .catch(handleError); }; @@ -281,8 +233,9 @@ const SpawnerFooter: React.FC = ({ variant="danger" title="Error creating workbench" actionLinks={ - // If this is a 409 conflict error - error.statusObject.code === 409 ? ( + // If this is a 409 conflict error on the notebook (not PVC or Secret or ConfigMap) + error.statusObject.code === 409 && + error.statusObject.details?.kind === 'notebooks' ? ( <> diff --git a/frontend/src/pages/projects/screens/spawner/__tests__/SpawnerFooter.spec.tsx b/frontend/src/pages/projects/screens/spawner/__tests__/SpawnerFooter.spec.tsx new file mode 100644 index 0000000000..c7866f2db5 --- /dev/null +++ b/frontend/src/pages/projects/screens/spawner/__tests__/SpawnerFooter.spec.tsx @@ -0,0 +1,136 @@ +import React, { act } from 'react'; +import { fireEvent, render } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { k8sCreateResource, k8sGetResource } from '@openshift/dynamic-plugin-sdk-utils'; +import { useUser } from '~/redux/selectors'; +import SpawnerFooter from '~/pages/projects/screens/spawner/SpawnerFooter'; +import { + mockDataConnectionData, + mockEnvVariables, + mockStartNotebookData, + mockStorageData, +} from '~/__mocks__/mockStartNotebookData'; +import { useAppContext } from '~/app/AppContext'; +import { mockDashboardConfig, mockNotebookK8sResource } from '~/__mocks__'; +import { + ConfigMapKind, + NotebookKind, + PersistentVolumeClaimKind, + RoleBindingKind, + SecretKind, +} from '~/k8sTypes'; +import { ConfigMapModel, NotebookModel, PVCModel, RoleBindingModel, SecretModel } from '~/api'; +import { mockPVCK8sResource } from '~/__mocks__/mockPVCK8sResource'; + +jest.mock('~/app/AppContext', () => ({ + __esModule: true, + useAppContext: jest.fn(), +})); + +jest.mock('~/redux/selectors', () => ({ + ...jest.requireActual('~/redux/selectors'), + useUser: jest.fn(), + useClusterInfo: jest.fn(), +})); + +jest.mock('react-router-dom', () => ({ + ...jest.requireActual('react-router-dom'), + useNavigate: () => jest.fn(), +})); + +jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ + k8sCreateResource: jest.fn(), + k8sGetResource: jest.fn(), +})); + +const k8sCreateSecretMock = jest.mocked(k8sCreateResource); +const k8sCreatePVCMock = jest.mocked(k8sCreateResource); +const k8sCreateConfigMapMock = jest.mocked(k8sCreateResource); +const k8sCreateNotebookMock = jest.mocked(k8sCreateResource); +const k8sCreateRoleBindingMock = jest.mocked(k8sCreateResource); +const k8sGetRoleBindingMock = jest.mocked(k8sGetResource); + +k8sCreatePVCMock.mockResolvedValue(mockPVCK8sResource({})); +k8sCreateNotebookMock.mockResolvedValue(mockNotebookK8sResource({})); +k8sGetRoleBindingMock.mockRejectedValue(new Error()); + +const useAppContextMock = jest.mocked(useAppContext); +useAppContextMock.mockReturnValue({ + buildStatuses: [], + dashboardConfig: mockDashboardConfig({}), + storageClasses: [], + isRHOAI: false, +}); + +const useUserMock = jest.mocked(useUser); +useUserMock.mockReturnValue({ + username: 'test-user', + isAdmin: false, + isAllowed: true, + userLoading: false, + userError: null, +}); + +const startNotebookDataMock = mockStartNotebookData({ notebookId: 'test-notebook' }); +global.structuredClone = (val: unknown) => JSON.parse(JSON.stringify(val)); + +const dryRunOptions = { + fetchOptions: { requestInit: {} }, + queryOptions: { + queryParams: { dryRun: 'All' }, + }, + payload: { + dryRun: ['All'], + }, +}; + +describe('EmptyProjects', () => { + beforeEach(() => { + // make console happy + jest.spyOn(console, 'error').mockImplementation(jest.fn()); + }); + it('should dry run all the API calls', async () => { + const result = render( + , + ); + expect(result.getByTestId('submit-button')).toBeEnabled(); + await act(() => fireEvent.click(result.getByTestId('submit-button'))); + + expect(k8sCreateSecretMock).toHaveBeenCalledWith( + expect.objectContaining({ + model: SecretModel, + ...dryRunOptions, + }), + ); + expect(k8sCreatePVCMock).toHaveBeenCalledWith( + expect.objectContaining({ + model: PVCModel, + ...dryRunOptions, + }), + ); + expect(k8sCreateConfigMapMock).toHaveBeenCalledWith( + expect.objectContaining({ + model: ConfigMapModel, + ...dryRunOptions, + }), + ); + expect(k8sCreateNotebookMock).toHaveBeenCalledWith( + expect.objectContaining({ + model: NotebookModel, + ...dryRunOptions, + }), + ); + expect(k8sCreateRoleBindingMock).toHaveBeenCalledWith( + expect.objectContaining({ + model: RoleBindingModel, + ...dryRunOptions, + }), + ); + }); +}); diff --git a/frontend/src/pages/projects/screens/spawner/service.ts b/frontend/src/pages/projects/screens/spawner/service.ts index bb77d5400d..cd6dbb1d0c 100644 --- a/frontend/src/pages/projects/screens/spawner/service.ts +++ b/frontend/src/pages/projects/screens/spawner/service.ts @@ -30,13 +30,14 @@ import { fetchNotebookEnvVariables } from './environmentVariables/useNotebookEnv export const createPvcDataForNotebook = async ( projectName: string, storageData: StorageData, + dryRun?: boolean, ): Promise<{ volumes: Volume[]; volumeMounts: VolumeMount[] }> => { const { storageType } = storageData; const { volumes, volumeMounts } = getVolumesByStorageData(storageData); if (storageType === StorageType.NEW_PVC) { - const pvc = await createPvc(storageData.creating, projectName); + const pvc = await createPvc(storageData.creating, projectName, { dryRun }); const newPvcName = pvc.metadata.name; volumes.push({ name: newPvcName, persistentVolumeClaim: { claimName: newPvcName } }); volumeMounts.push({ mountPath: ROOT_MOUNT_PATH, name: newPvcName }); @@ -168,8 +169,14 @@ const getEnvFromList = ( export const createConfigMapsAndSecretsForNotebook = async ( projectName: string, envVariables: EnvVariable[], + dryRun?: boolean, ): Promise => { - const creatingPromises = getPromisesForConfigMapsAndSecrets(projectName, envVariables, 'create'); + const creatingPromises = getPromisesForConfigMapsAndSecrets( + projectName, + envVariables, + 'create', + dryRun, + ); return Promise.all(creatingPromises) .then((results: (ConfigMapKind | SecretKind)[]) => getEnvFromList(results, []))