From 30cc710bb7e549856717513a1833b4922c40369c Mon Sep 17 00:00:00 2001 From: Mike Turley Date: Wed, 15 May 2024 14:30:38 -0400 Subject: [PATCH] Model Registry Settings: Add model registries table with db config viewer, delete action and stub for permission management modal Signed-off-by: Mike Turley --- .../src/routes/api/modelRegistries/index.ts | 96 +++--- .../api/modelRegistries/modelRegistryUtils.ts | 300 ++++++++++++++++++ backend/src/types.ts | 45 ++- frontend/src/__mocks__/mockModelRegistry.ts | 3 +- .../cypress/pages/modelRegistrySettings.ts | 85 +++++ .../cypress/cypress/support/commands/odh.ts | 12 + .../modelRegistrySettings.cy.ts | 201 +++++++++--- .../dashboard/DashboardModalFooter.tsx | 5 +- .../useModelRegistriesBackend.ts | 11 + frontend/src/k8sTypes.ts | 48 +-- .../modelRegistrySettings/CreateModal.tsx | 8 +- .../DeleteModelRegistryModal.tsx | 103 ++++++ .../ManagePermissionsModal.tsx | 43 +++ .../ModelRegistriesTable.tsx | 45 +++ .../ModelRegistriesTableRow.tsx | 70 ++++ .../ModelRegistryDatabasePassword.tsx | 34 ++ .../ModelRegistrySettings.tsx | 71 +++-- .../ViewDatabaseConfigModal.tsx | 75 +++++ .../pages/modelRegistrySettings/columns.ts | 17 + frontend/src/services/modelRegistryService.ts | 60 ---- .../services/modelRegistrySettingsService.ts | 57 ++++ 21 files changed, 1155 insertions(+), 234 deletions(-) create mode 100644 backend/src/routes/api/modelRegistries/modelRegistryUtils.ts create mode 100644 frontend/src/concepts/modelRegistrySettings/useModelRegistriesBackend.ts create mode 100644 frontend/src/pages/modelRegistrySettings/DeleteModelRegistryModal.tsx create mode 100644 frontend/src/pages/modelRegistrySettings/ManagePermissionsModal.tsx create mode 100644 frontend/src/pages/modelRegistrySettings/ModelRegistriesTable.tsx create mode 100644 frontend/src/pages/modelRegistrySettings/ModelRegistriesTableRow.tsx create mode 100644 frontend/src/pages/modelRegistrySettings/ModelRegistryDatabasePassword.tsx create mode 100644 frontend/src/pages/modelRegistrySettings/ViewDatabaseConfigModal.tsx create mode 100644 frontend/src/pages/modelRegistrySettings/columns.ts delete mode 100644 frontend/src/services/modelRegistryService.ts create mode 100644 frontend/src/services/modelRegistrySettingsService.ts diff --git a/backend/src/routes/api/modelRegistries/index.ts b/backend/src/routes/api/modelRegistries/index.ts index 4f7891e16c..50ed8bd732 100644 --- a/backend/src/routes/api/modelRegistries/index.ts +++ b/backend/src/routes/api/modelRegistries/index.ts @@ -1,9 +1,21 @@ import { FastifyReply, FastifyRequest } from 'fastify'; -import { PatchUtils } from '@kubernetes/client-node'; import { secureAdminRoute } from '../../../utils/route-security'; import { KubeFastifyInstance, ModelRegistryKind, RecursivePartial } from '../../../types'; -import { MODEL_REGISTRY_NAMESPACE } from '../../../utils/constants'; +import { + createModelRegistryAndSecret, + deleteModelRegistryAndSecret, + getDatabasePassword, + getModelRegistry, + listModelRegistries, + patchModelRegistryAndUpdatePassword, +} from './modelRegistryUtils'; +type ModelRegistryAndDBPassword = { + modelRegistry: ModelRegistryKind; + databasePassword?: string; +}; + +// Lists ModelRegistries directly (does not look up passwords from associated Secrets, you must make a direct request to '/:modelRegistryName' for that) export default async (fastify: KubeFastifyInstance): Promise => { fastify.get( '/', @@ -14,17 +26,7 @@ export default async (fastify: KubeFastifyInstance): Promise => { ) => { const { labelSelector } = request.query; try { - const response = await fastify.kube.customObjectsApi.listNamespacedCustomObject( - 'modelregistry.opendatahub.io', - 'v1alpha1', - MODEL_REGISTRY_NAMESPACE, - 'modelregistries', - undefined, - undefined, - undefined, - labelSelector, - ); - return response.body; + return listModelRegistries(fastify, labelSelector); } catch (e) { fastify.log.error( `ModelRegistries could not be listed, ${e.response?.body?.message || e.message}`, @@ -35,29 +37,22 @@ export default async (fastify: KubeFastifyInstance): Promise => { ), ); + // Accepts a ModelRegistry and a password, creates the MR and an associated Secret + // Returns the ModelRegistry only fastify.post( '/', secureAdminRoute(fastify)( async ( request: FastifyRequest<{ Querystring: { dryRun?: string }; - Body: ModelRegistryKind; + Body: ModelRegistryAndDBPassword; }>, reply: FastifyReply, ) => { const { dryRun } = request.query; - const modelRegistry = request.body; + const { modelRegistry, databasePassword } = request.body; try { - const response = await fastify.kube.customObjectsApi.createNamespacedCustomObject( - 'modelregistry.opendatahub.io', - 'v1alpha1', - MODEL_REGISTRY_NAMESPACE, - 'modelregistries', - request.body, - undefined, - dryRun, - ); - return response.body; + return createModelRegistryAndSecret(fastify, modelRegistry, databasePassword, !!dryRun); } catch (e) { fastify.log.error( `ModelRegistry ${modelRegistry.metadata.name} could not be created, ${ @@ -70,6 +65,7 @@ export default async (fastify: KubeFastifyInstance): Promise => { ), ); + // Returns both the ModelRegistry and the password decoded from its associated Secret fastify.get( '/:modelRegistryName', secureAdminRoute(fastify)( @@ -79,14 +75,9 @@ export default async (fastify: KubeFastifyInstance): Promise => { ) => { const { modelRegistryName } = request.params; try { - const response = await fastify.kube.customObjectsApi.getNamespacedCustomObject( - 'modelregistry.opendatahub.io', - 'v1alpha1', - MODEL_REGISTRY_NAMESPACE, - 'modelregistries', - modelRegistryName, - ); - return response.body; + const modelRegistry = await getModelRegistry(fastify, modelRegistryName); + const databasePassword = await getDatabasePassword(fastify, modelRegistry); + return { modelRegistry, databasePassword } satisfies ModelRegistryAndDBPassword; } catch (e) { fastify.log.error( `ModelRegistry ${modelRegistryName} could not be read, ${ @@ -99,6 +90,8 @@ export default async (fastify: KubeFastifyInstance): Promise => { ), ); + // Accepts both a patch for the ModelRegistry and (optionally) a password to replace on the associated Secret + // Returns the patched ModelRegistry only fastify.patch( '/:modelRegistryName', secureAdminRoute(fastify)( @@ -106,29 +99,22 @@ export default async (fastify: KubeFastifyInstance): Promise => { request: FastifyRequest<{ Querystring: { dryRun?: string }; Params: { modelRegistryName: string }; - Body: RecursivePartial; + Body: RecursivePartial; }>, reply: FastifyReply, ) => { - const options = { - headers: { 'Content-type': PatchUtils.PATCH_FORMAT_JSON_PATCH }, - }; const { dryRun } = request.query; const { modelRegistryName } = request.params; + const { modelRegistry: patchBody, databasePassword } = request.body; try { - const response = await fastify.kube.customObjectsApi.patchNamespacedCustomObject( - 'modelregistry.opendatahub.io', - 'v1alpha1', - MODEL_REGISTRY_NAMESPACE, - 'modelregistries', + const modelRegistry = await patchModelRegistryAndUpdatePassword( + fastify, modelRegistryName, - request.body, - dryRun, - undefined, - undefined, - options, + patchBody, + databasePassword, + !!dryRun, ); - return response.body; + return { modelRegistry, databasePassword }; } catch (e) { fastify.log.error( `ModelRegistry ${modelRegistryName} could not be modified, ${ @@ -141,6 +127,7 @@ export default async (fastify: KubeFastifyInstance): Promise => { ), ); + // Deletes both the ModelRegistry and its associated Secret fastify.delete( '/:modelRegistryName', secureAdminRoute(fastify)( @@ -154,18 +141,7 @@ export default async (fastify: KubeFastifyInstance): Promise => { const { dryRun } = request.query; const { modelRegistryName } = request.params; try { - const response = await fastify.kube.customObjectsApi.deleteNamespacedCustomObject( - 'modelregistry.opendatahub.io', - 'v1alpha1', - MODEL_REGISTRY_NAMESPACE, - 'modelregistries', - modelRegistryName, - undefined, - undefined, - undefined, - dryRun, - ); - return response.body; + deleteModelRegistryAndSecret(fastify, modelRegistryName, !!dryRun); } catch (e) { fastify.log.error( `ModelRegistry ${modelRegistryName} could not be deleted, ${ diff --git a/backend/src/routes/api/modelRegistries/modelRegistryUtils.ts b/backend/src/routes/api/modelRegistries/modelRegistryUtils.ts new file mode 100644 index 0000000000..967aa5c4f4 --- /dev/null +++ b/backend/src/routes/api/modelRegistries/modelRegistryUtils.ts @@ -0,0 +1,300 @@ +import { MODEL_REGISTRY_NAMESPACE } from '../../../utils/constants'; +import { KubeFastifyInstance, ModelRegistryKind, RecursivePartial } from '../../../types'; +import { PatchUtils, V1Secret, V1Status } from '@kubernetes/client-node'; + +const MODEL_REGISTRY_API_GROUP = 'modelregistry.opendatahub.io'; +const MODEL_REGISTRY_API_VERSION = 'v1alpha1'; +const MODEL_REGISTRY_PLURAL = 'modelregistries'; + +const base64encode = (value?: string): string => { + // This usage of toString is fine for encoding + // eslint-disable-next-line no-restricted-properties + return Buffer.from(value || '').toString('base64'); +}; +const base64decode = (encodedValue?: string): string => + String(Buffer.from(encodedValue || '', 'base64')); + +const MR_DATABASE_TYPES = ['mysql', 'postgres'] satisfies (keyof ModelRegistryKind['spec'])[]; +type MRDatabaseType = (typeof MR_DATABASE_TYPES)[number]; +const getDatabaseType = (modelRegistry: ModelRegistryKind): MRDatabaseType | undefined => + MR_DATABASE_TYPES.find((type) => !!modelRegistry.spec[type]); + +const getDatabaseSpec = ( + modelRegistry: ModelRegistryKind, +): ModelRegistryKind['spec'][MRDatabaseType] | undefined => { + const dbType = getDatabaseType(modelRegistry); + return dbType && modelRegistry.spec[dbType]; +}; + +export const listModelRegistries = async ( + fastify: KubeFastifyInstance, + labelSelector?: string, +): Promise<{ items: ModelRegistryKind[] }> => { + const response = await (fastify.kube.customObjectsApi.listNamespacedCustomObject( + MODEL_REGISTRY_API_GROUP, + MODEL_REGISTRY_API_VERSION, + MODEL_REGISTRY_NAMESPACE, + MODEL_REGISTRY_PLURAL, + undefined, + undefined, + undefined, + labelSelector, + // listNamespacedCustomObject doesn't support TS generics and returns body as `object`, so we assert its real type + ) as Promise<{ body: { items: ModelRegistryKind[] } }>); + return response.body; +}; + +const createDatabasePasswordSecret = async ( + fastify: KubeFastifyInstance, + modelRegistry: ModelRegistryKind, + databasePassword?: string, + dryRun = false, +): Promise => { + const dbSpec = getDatabaseSpec(modelRegistry); + if (!dbSpec) { + return null; + } + const secret: V1Secret = { + kind: 'Secret', + apiVersion: 'v1', + metadata: { + generateName: `${modelRegistry.metadata.name}-db-`, + namespace: MODEL_REGISTRY_NAMESPACE, + annotations: { + 'template.openshift.io/expose-database_name': "{.data['database-name']}", + 'template.openshift.io/expose-username': "{.data['database-user']}", + 'template.openshift.io/expose-password': "{.data['database-password']}", + }, + }, + data: { + 'database-name': base64encode(dbSpec.database), + 'database-user': base64encode(dbSpec.username), + 'database-password': base64encode(databasePassword), + }, + type: 'Opaque', + }; + const response = await fastify.kube.coreV1Api.createNamespacedSecret( + MODEL_REGISTRY_NAMESPACE, + secret, + undefined, + dryRun ? 'All' : undefined, + ); + return response.body; +}; + +const createModelRegistry = async ( + fastify: KubeFastifyInstance, + modelRegistry: ModelRegistryKind, + secret?: V1Secret, + dryRun = false, +): Promise => { + const dbType = getDatabaseType(modelRegistry); + const modelRegistryWithSecretRef: ModelRegistryKind = + dbType && secret + ? { + ...modelRegistry, + spec: { + ...modelRegistry.spec, + [dbType]: { + ...modelRegistry.spec[dbType], + passwordSecret: { + key: 'database-password', + name: secret.metadata.name, + }, + }, + }, + } + : modelRegistry; + const response = await (fastify.kube.customObjectsApi.createNamespacedCustomObject( + MODEL_REGISTRY_API_GROUP, + MODEL_REGISTRY_API_VERSION, + MODEL_REGISTRY_NAMESPACE, + MODEL_REGISTRY_PLURAL, + modelRegistryWithSecretRef, + undefined, + dryRun ? 'All' : undefined, + // createNamespacedCustomObject doesn't support TS generics and returns body as `object`, so we assert its real type + ) as Promise<{ body: ModelRegistryKind }>); + return response.body; +}; + +export const createModelRegistryAndSecret = async ( + fastify: KubeFastifyInstance, + modelRegistry: ModelRegistryKind, + databasePassword?: string, + dryRunOnly = false, +): Promise => { + const createBoth = async (dryRun = false) => { + const dbSpec = getDatabaseSpec(modelRegistry); + const newSecret = + databasePassword && dbSpec + ? await createDatabasePasswordSecret(fastify, modelRegistry, databasePassword, dryRun) + : undefined; + return createModelRegistry(fastify, modelRegistry, newSecret, dryRun); + }; + // Dry run both MR and Secret creation first so there are no changes if either would fail + const dryRunResult = await createBoth(true); + if (dryRunOnly) { + return dryRunResult; + } + return createBoth(); +}; + +export const getModelRegistry = async ( + fastify: KubeFastifyInstance, + modelRegistryName: string, +): Promise => { + const response = await (fastify.kube.customObjectsApi.getNamespacedCustomObject( + MODEL_REGISTRY_API_GROUP, + MODEL_REGISTRY_API_VERSION, + MODEL_REGISTRY_NAMESPACE, + MODEL_REGISTRY_PLURAL, + modelRegistryName, + // getNamespacedCustomObject doesn't support TS generics and returns body as `object`, so we assert its real type + ) as Promise<{ body: ModelRegistryKind }>); + return response.body; +}; + +const getDatabasePasswordSecret = async ( + fastify: KubeFastifyInstance, + modelRegistry: ModelRegistryKind, +): Promise<{ secret?: V1Secret; passwordDataKey?: string }> => { + const secretRef = getDatabaseSpec(modelRegistry)?.passwordSecret; + if (!secretRef) { + return {}; + } + const response = await fastify.kube.coreV1Api.readNamespacedSecret( + secretRef.name, + MODEL_REGISTRY_NAMESPACE, + ); + return { secret: response.body, passwordDataKey: secretRef.key }; +}; + +export const getDatabasePassword = async ( + fastify: KubeFastifyInstance, + modelRegistry: ModelRegistryKind, +): Promise => { + const { secret, passwordDataKey } = await getDatabasePasswordSecret(fastify, modelRegistry); + return base64decode(secret.data[passwordDataKey]); +}; + +const deleteDatabasePasswordSecret = async ( + fastify: KubeFastifyInstance, + modelRegistry: ModelRegistryKind, + dryRun = false, +): Promise => { + let existingSecret: V1Secret | undefined; + try { + existingSecret = (await getDatabasePasswordSecret(fastify, modelRegistry)).secret; + } catch (e) { + // If the secret doesn't exist, don't try to delete and cause a 404 error, just do nothing. + // The user may have deleted their own secret and we don't want to block deleting the model registry. + return; + } + const response = await fastify.kube.coreV1Api.deleteNamespacedSecret( + existingSecret.metadata.name, + MODEL_REGISTRY_NAMESPACE, + undefined, + dryRun ? 'All' : undefined, + ); + return response.body; +}; + +const patchModelRegistry = async ( + fastify: KubeFastifyInstance, + modelRegistryName: string, + patchBody: RecursivePartial, + dryRun = false, +): Promise => { + const response = await (fastify.kube.customObjectsApi.patchNamespacedCustomObject( + MODEL_REGISTRY_API_GROUP, + MODEL_REGISTRY_API_VERSION, + MODEL_REGISTRY_NAMESPACE, + MODEL_REGISTRY_PLURAL, + modelRegistryName, + patchBody, + dryRun ? 'All' : undefined, + undefined, + undefined, + { headers: { 'Content-type': PatchUtils.PATCH_FORMAT_JSON_PATCH } }, + // patchNamespacedCustomObject doesn't support TS generics and returns body as `object`, so we assert its real type + ) as Promise<{ body: ModelRegistryKind }>); + return response.body; +}; + +const updateDatabasePassword = async ( + fastify: KubeFastifyInstance, + modelRegistry: ModelRegistryKind, + databasePassword?: string, + dryRun = false, +): Promise => { + const { secret, passwordDataKey } = await getDatabasePasswordSecret(fastify, modelRegistry); + if (!secret) { + return; + } + if (databasePassword) { + await fastify.kube.coreV1Api.patchNamespacedSecret( + secret.metadata.name, + MODEL_REGISTRY_NAMESPACE, + { + data: { + ...secret.data, + [passwordDataKey]: base64encode(databasePassword), + }, + }, + undefined, + dryRun ? 'All' : undefined, + ); + } else { + await deleteDatabasePasswordSecret(fastify, modelRegistry); + } +}; + +export const patchModelRegistryAndUpdatePassword = async ( + fastify: KubeFastifyInstance, + modelRegistryName: string, + patchBody: RecursivePartial, + databasePassword?: string, + dryRunOnly = false, +): Promise => { + const patchBoth = async (dryRun = false) => { + const modelRegistry = await patchModelRegistry(fastify, modelRegistryName, patchBody, dryRun); + await updateDatabasePassword(fastify, modelRegistry, databasePassword, dryRun); + return modelRegistry; + }; + // Dry run both patches first so there are no changes if either would fail + const dryRunResult = await patchBoth(true); + if (dryRunOnly) { + return dryRunResult; + } + return patchBoth(); +}; + +export const deleteModelRegistryAndSecret = async ( + fastify: KubeFastifyInstance, + modelRegistryName: string, + dryRunOnly = false, +): Promise => { + const modelRegistry = await getModelRegistry(fastify, modelRegistryName); + const deleteBoth = async (dryRun = false) => { + const response = await fastify.kube.customObjectsApi.deleteNamespacedCustomObject( + MODEL_REGISTRY_API_GROUP, + MODEL_REGISTRY_API_VERSION, + MODEL_REGISTRY_NAMESPACE, + MODEL_REGISTRY_PLURAL, + modelRegistryName, + undefined, + undefined, + undefined, + dryRun ? 'All' : undefined, + ); + await deleteDatabasePasswordSecret(fastify, modelRegistry); + return response.body; + }; + // Dry run both deletes first so there are no changes if either would fail + const dryRunResult = await deleteBoth(true); + if (dryRunOnly) { + return dryRunResult; + } + return deleteBoth(); +}; diff --git a/backend/src/types.ts b/backend/src/types.ts index 6f702af60a..692cc9cf4d 100644 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -1042,24 +1042,35 @@ export type ModelRegistryKind = K8sResourceCommon & { port: number; serviceRoute: string; }; - mysql?: { - database: string; - host: string; - port?: number; - }; - postgres: { - database: string; - host?: string; - passwordSecret?: { - key: string; - name: string; + } & EitherNotBoth< + { + mysql?: { + database: string; + host: string; + passwordSecret?: { + key: string; + name: string; + }; + port?: number; + skipDBCreation?: boolean; + username?: string; }; - port: number; - skipDBCreation?: boolean; - sslMode?: string; - username?: string; - }; - }; + }, + { + postgres?: { + database: string; + host?: string; + passwordSecret?: { + key: string; + name: string; + }; + port: number; + skipDBCreation?: boolean; + sslMode?: string; + username?: string; + }; + } + >; status?: { conditions?: K8sCondition[]; }; diff --git a/frontend/src/__mocks__/mockModelRegistry.ts b/frontend/src/__mocks__/mockModelRegistry.ts index b90f594b8c..b692426be9 100644 --- a/frontend/src/__mocks__/mockModelRegistry.ts +++ b/frontend/src/__mocks__/mockModelRegistry.ts @@ -1,3 +1,4 @@ +import { MODEL_REGISTRY_DEFAULT_NAMESPACE } from '~/concepts/modelRegistry/const'; import { ModelRegistryKind } from '~/k8sTypes'; type MockModelRegistryType = { @@ -7,7 +8,7 @@ type MockModelRegistryType = { export const mockModelRegistry = ({ name = 'modelregistry-sample', - namespace = 'odh-model-registries', + namespace = MODEL_REGISTRY_DEFAULT_NAMESPACE, }: MockModelRegistryType): ModelRegistryKind => ({ apiVersion: 'modelregistry.opendatahub.io/v1alpha1', kind: 'ModelRegistry', diff --git a/frontend/src/__tests__/cypress/cypress/pages/modelRegistrySettings.ts b/frontend/src/__tests__/cypress/cypress/pages/modelRegistrySettings.ts index 77e2e2b0ff..52eb1626f2 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/modelRegistrySettings.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/modelRegistrySettings.ts @@ -1,5 +1,31 @@ import { appChrome } from './appChrome'; +export enum FormFieldSelector { + NAME = '#mr-name', + HOST = '#mr-host', + PORT = '#mr-port', + USERNAME = '#mr-username', + PASSWORD = '#mr-password', + DATABASE = '#mr-database', +} + +export enum FormErrorTestId { + NAME = 'mr-name-error', + HOST = 'mr-host-error', + PORT = 'mr-port-error', + USERNAME = 'mr-username-error', + PASSWORD = 'mr-password-error', + DATABASE = 'mr-database-error', +} + +export enum DatabaseDetailsTestId { + HOST = 'mr-db-host', + PORT = 'mr-db-port', + USERNAME = 'mr-db-username', + PASSWORD = 'mr-db-password', + DATABASE = 'mr-db-database', +} + class ModelRegistrySettings { visit(wait = true) { cy.visitWithLogin('/modelRegistrySettings'); @@ -26,6 +52,65 @@ class ModelRegistrySettings { findNavItem() { return appChrome.findNavItem('Model registry settings', 'Settings'); } + + findEmptyState() { + return cy.findByTestId('mr-settings-empty-state'); + } + + findCreateButton() { + return cy.findByText('Create model registry'); + } + + findFormField(selector: FormFieldSelector) { + return cy.get(selector); + } + + clearFormFields() { + Object.values(FormFieldSelector).forEach((selector) => { + this.findFormField(selector).clear(); + this.findFormField(selector).blur(); + }); + } + + findFormError(testId: FormErrorTestId) { + return cy.findByTestId(testId); + } + + shouldHaveAllErrors() { + Object.values(FormErrorTestId).forEach((testId) => this.findFormError(testId).should('exist')); + } + + shouldHaveNoErrors() { + Object.values(FormErrorTestId).forEach((testId) => + this.findFormError(testId).should('not.exist'), + ); + } + + findSubmitButton() { + return cy.findByTestId('modal-submit-button'); + } + + findTable() { + return cy.findByTestId('model-registries-table'); + } + + findModelRegistryRow(registryName: string) { + return this.findTable().findByText(registryName).closest('tr'); + } + + findDatabaseDetail(testId: DatabaseDetailsTestId) { + return cy.findByTestId(testId); + } + + findDatabasePasswordHiddenButton() { + return this.findDatabaseDetail(DatabaseDetailsTestId.PASSWORD).findByTestId( + 'password-hidden-button', + ); + } + + findConfirmDeleteNameInput() { + return cy.findByTestId('confirm-delete-input'); + } } export const modelRegistrySettings = new ModelRegistrySettings(); diff --git a/frontend/src/__tests__/cypress/cypress/support/commands/odh.ts b/frontend/src/__tests__/cypress/cypress/support/commands/odh.ts index 57e836e84e..88a50e62d2 100644 --- a/frontend/src/__tests__/cypress/cypress/support/commands/odh.ts +++ b/frontend/src/__tests__/cypress/cypress/support/commands/odh.ts @@ -17,6 +17,7 @@ import type { ServingRuntimeKind, TemplateKind, NotebookKind, + ModelRegistryKind, } from '~/k8sTypes'; import { StartNotebookData } from '~/pages/projects/types'; @@ -313,6 +314,17 @@ declare global { options: { path: { serviceName: string; apiVersion: string; modelVersionId: number } }, response: OdhResponse, ) => Cypress.Chainable) & + (( + type: 'GET /api/modelRegistries', + response: OdhResponse>, + ) => Cypress.Chainable) & + (( + type: 'GET /api/modelRegistries/:modelRegistryName', + options: { + path: { modelRegistryName: string }; + }, + response: OdhResponse<{ modelRegistry: ModelRegistryKind; databasePassword?: string }>, + ) => Cypress.Chainable) & (( type: `GET /api/service/pipelines/:namespace/:serviceName/apis/v2beta1/pipelines/:pipelineId/versions/:pipelineVersionId`, options: { diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts index 2680fcfb03..ff49d58210 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts @@ -1,11 +1,21 @@ -import { mockDashboardConfig, mockDscStatus } from '~/__mocks__'; +import { mockDashboardConfig, mockDscStatus, mockK8sResourceList } from '~/__mocks__'; import { mockDsciStatus } from '~/__mocks__/mockDsciStatus'; import { StackCapability, StackComponent } from '~/concepts/areas/types'; -import { modelRegistrySettings } from '~/__tests__/cypress/cypress/pages/modelRegistrySettings'; +import { + FormFieldSelector, + FormErrorTestId, + modelRegistrySettings, + DatabaseDetailsTestId, +} from '~/__tests__/cypress/cypress/pages/modelRegistrySettings'; import { pageNotfound } from '~/__tests__/cypress/cypress/pages/pageNotFound'; import { asProductAdminUser, asProjectAdminUser } from '~/__tests__/cypress/cypress/utils/users'; +import { mockModelRegistry } from '~/__mocks__/mockModelRegistry'; -const setupMocksForMRSettingAccess = () => { +const setupMocksForMRSettingAccess = ({ + hasModelRegistries = true, +}: { + hasModelRegistries?: boolean; +}) => { asProductAdminUser(); cy.interceptOdh( 'GET /api/config', @@ -28,6 +38,37 @@ const setupMocksForMRSettingAccess = () => { requiredCapabilities: [StackCapability.SERVICE_MESH, StackCapability.SERVICE_MESH_AUTHZ], }), ); + cy.interceptOdh( + 'GET /api/modelRegistries', + mockK8sResourceList( + hasModelRegistries + ? [ + mockModelRegistry({ name: 'test-registry-1' }), + mockModelRegistry({ name: 'test-registry-2' }), + mockModelRegistry({ name: 'test-registry-3' }), + ] + : [], + ), + ); + cy.interceptOdh( + 'GET /api/modelRegistries/:modelRegistryName', + { + path: { modelRegistryName: 'test-registry-1' }, + }, + { + modelRegistry: mockModelRegistry({ name: 'test-registry-1' }), + databasePassword: 'test-password', + }, + ); + cy.interceptOdh( + 'GET /api/modelRegistries/:modelRegistryName', + { + path: { modelRegistryName: 'test-registry-2' }, + }, + (req) => { + req.reply(500); // Something went wrong on the backend when decoding the secret + }, + ); }; it('Model registry settings should not be available for non product admins', () => { @@ -38,64 +79,134 @@ it('Model registry settings should not be available for non product admins', () }); it('Model registry settings should be available for product admins with capabilities', () => { - setupMocksForMRSettingAccess(); + setupMocksForMRSettingAccess({}); // check page is accessible modelRegistrySettings.visit(true); // check nav item exists modelRegistrySettings.findNavItem().should('exist'); }); +it('Shows empty state when there are no registries', () => { + setupMocksForMRSettingAccess({ hasModelRegistries: false }); + modelRegistrySettings.visit(true); + modelRegistrySettings.findEmptyState().should('exist'); +}); + describe('CreateModal', () => { beforeEach(() => { - setupMocksForMRSettingAccess(); + setupMocksForMRSettingAccess({}); }); it('should display error messages when form fields are invalid and not allow submit', () => { modelRegistrySettings.visit(true); - cy.findByText('Create model registry').click(); - // Fill in the form with invalid data - cy.get('#mr-name').clear(); - cy.get('#mr-host').clear(); - cy.get('#mr-port').clear(); - cy.get('#mr-username').clear(); - cy.get('#mr-password').clear(); - cy.get('#mr-database').clear(); - cy.get('#mr-database').blur(); - // Assert the submit button is disabled - cy.findByTestId('modal-submit-button').should('be.disabled'); - // Assert that error messages are displayed - cy.findByTestId('mr-name-error') - .should('be.visible') - .contains( - "Must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character", - ); - cy.findByTestId('mr-host-error').should('be.visible').contains('Host cannot be empty'); - cy.findByTestId('mr-port-error').should('be.visible').contains('Port cannot be empty'); - cy.findByTestId('mr-username-error').should('be.visible').contains('Username cannot be empty'); - cy.findByTestId('mr-password-error').should('be.visible').contains('Password cannot be empty'); - cy.findByTestId('mr-database-error').scrollIntoView(); - cy.findByTestId('mr-database-error').should('be.visible').contains('Database cannot be empty'); + modelRegistrySettings.findCreateButton().click(); + modelRegistrySettings.findSubmitButton().should('be.disabled'); + modelRegistrySettings.clearFormFields(); + modelRegistrySettings.findSubmitButton().should('be.disabled'); + modelRegistrySettings.shouldHaveAllErrors(); + }); + + it('should display error when name is not empty but is invalid', () => { + modelRegistrySettings.visit(true); + modelRegistrySettings.findCreateButton().click(); + modelRegistrySettings.findFormField(FormFieldSelector.NAME).type('Invalid Name'); + modelRegistrySettings.findFormField(FormFieldSelector.NAME).blur(); + modelRegistrySettings.findFormError(FormErrorTestId.NAME).should('exist'); }); it('should enable submit button if fields are valid', () => { modelRegistrySettings.visit(true); cy.findByText('Create model registry').click(); - // Fill in the form with invalid data - cy.get('#mr-name').type('valid-mr-name'); - cy.get('#mr-host').type('host'); - cy.get('#mr-port').type('1234'); - cy.get('#mr-username').type('validUser'); - cy.get('#mr-password').type('strongPassword'); - cy.get('#mr-database').type('myDatabase'); - cy.get('#mr-database').blur(); - // Assert the submit button is disabled - cy.findByTestId('modal-submit-button').should('be.enabled'); - // Assert that error messages are not displayed - cy.findByTestId('mr-name-error').should('not.exist'); - cy.findByTestId('mr-host-error').should('not.exist'); - cy.findByTestId('mr-port-error').should('not.exist'); - cy.findByTestId('mr-username-error').should('not.exist'); - cy.findByTestId('mr-password-error').should('not.exist'); - cy.findByTestId('mr-database-error').should('not.exist'); + modelRegistrySettings.findFormField(FormFieldSelector.NAME).type('valid-mr-name'); + modelRegistrySettings.findFormField(FormFieldSelector.HOST).type('host'); + modelRegistrySettings.findFormField(FormFieldSelector.PORT).type('1234'); + modelRegistrySettings.findFormField(FormFieldSelector.USERNAME).type('validUser'); + modelRegistrySettings.findFormField(FormFieldSelector.PASSWORD).type('strongPassword'); + modelRegistrySettings.findFormField(FormFieldSelector.DATABASE).type('myDatabase'); + modelRegistrySettings.findFormField(FormFieldSelector.DATABASE).blur(); + modelRegistrySettings.findSubmitButton().should('be.enabled'); + modelRegistrySettings.shouldHaveNoErrors(); + }); +}); + +describe('ModelRegistriesTable', () => { + it('Shows table when there are registries', () => { + setupMocksForMRSettingAccess({}); + modelRegistrySettings.visit(true); + modelRegistrySettings.findEmptyState().should('not.exist'); + modelRegistrySettings.findTable().should('exist'); + modelRegistrySettings.findModelRegistryRow('test-registry-1').should('exist'); + }); +}); + +describe('ViewDatabaseConfigModal', () => { + it('Shows database details for a registry', () => { + setupMocksForMRSettingAccess({}); + modelRegistrySettings.visit(true); + modelRegistrySettings + .findModelRegistryRow('test-registry-1') + .findKebabAction('View database configuration') + .click(); + modelRegistrySettings + .findDatabaseDetail(DatabaseDetailsTestId.HOST) + .should('contain.text', 'model-registry-db'); + modelRegistrySettings + .findDatabaseDetail(DatabaseDetailsTestId.PORT) + .should('contain.text', '5432'); + modelRegistrySettings + .findDatabaseDetail(DatabaseDetailsTestId.USERNAME) + .should('contain.text', 'mlmduser'); + modelRegistrySettings.findDatabasePasswordHiddenButton().click(); + modelRegistrySettings + .findDatabaseDetail(DatabaseDetailsTestId.PASSWORD) + .should('contain.text', 'test-password'); + modelRegistrySettings + .findDatabaseDetail(DatabaseDetailsTestId.DATABASE) + .should('contain.text', 'model-registry'); + }); + + it('Shows error loading password when secret fails to decode', () => { + setupMocksForMRSettingAccess({}); + modelRegistrySettings.visit(true); + modelRegistrySettings + .findModelRegistryRow('test-registry-2') + .findKebabAction('View database configuration') + .click(); + cy.findByText('Error loading password').should('exist'); + }); +}); + +describe('ManagePermissionsModal', () => { + beforeEach(() => { + setupMocksForMRSettingAccess({}); + modelRegistrySettings.visit(true); + modelRegistrySettings + .findModelRegistryRow('test-registry-1') + .findByText('Manage permissions') + .click(); + }); + + it('Shows modal for registry', () => { + cy.findByText('Manage permissions of test-registry-1').should('exist'); + }); +}); + +describe('DeleteModelRegistryModal', () => { + beforeEach(() => { + setupMocksForMRSettingAccess({}); + modelRegistrySettings.visit(true); + modelRegistrySettings + .findModelRegistryRow('test-registry-1') + .findKebabAction('Delete model registry') + .click(); + }); + + it('disables confirm button before name is typed', () => { + modelRegistrySettings.findSubmitButton().should('be.disabled'); + }); + + it('enables confirm button after name is typed', () => { + modelRegistrySettings.findConfirmDeleteNameInput().type('test-registry-1'); + modelRegistrySettings.findSubmitButton().should('be.enabled'); }); }); diff --git a/frontend/src/concepts/dashboard/DashboardModalFooter.tsx b/frontend/src/concepts/dashboard/DashboardModalFooter.tsx index d85c99e870..f11b2f1265 100644 --- a/frontend/src/concepts/dashboard/DashboardModalFooter.tsx +++ b/frontend/src/concepts/dashboard/DashboardModalFooter.tsx @@ -4,12 +4,14 @@ import { ActionListItem, Alert, Button, + ButtonProps, Stack, StackItem, } from '@patternfly/react-core'; type DashboardModalFooterProps = { submitLabel: string; + submitButtonVariant?: ButtonProps['variant']; onSubmit: () => void; onCancel: () => void; isSubmitDisabled: boolean; @@ -21,6 +23,7 @@ type DashboardModalFooterProps = { const DashboardModalFooter: React.FC = ({ submitLabel, + submitButtonVariant = 'primary', onSubmit, onCancel, isSubmitDisabled, @@ -43,7 +46,7 @@ const DashboardModalFooter: React.FC = ({