From ad02d75f25dead97e4cfd49135ee79799cdfe6d0 Mon Sep 17 00:00:00 2001 From: Mike Turley Date: Mon, 9 Sep 2024 12:31:56 -0400 Subject: [PATCH 1/5] Conditionally use specific role for viewing inferenceservices only for KServe Signed-off-by: Mike Turley --- backend/src/plugins/kube.ts | 2 + backend/src/utils/resourceUtils.ts | 11 +++ frontend/src/__mocks__/mockRoleK8sResource.ts | 32 +++++++ frontend/src/api/index.ts | 1 + .../api/k8s/__tests__/roleBindings.spec.ts | 9 +- frontend/src/api/k8s/__tests__/roles.spec.ts | 92 +++++++++++++++++++ frontend/src/api/k8s/roleBindings.ts | 6 +- frontend/src/api/k8s/roles.ts | 40 ++++++++ frontend/src/api/models/k8s.ts | 7 ++ frontend/src/k8sTypes.ts | 22 +++-- .../modelServing/screens/projects/utils.ts | 3 + frontend/src/pages/modelServing/utils.ts | 54 ++++++++++- 12 files changed, 264 insertions(+), 15 deletions(-) create mode 100644 frontend/src/__mocks__/mockRoleK8sResource.ts create mode 100644 frontend/src/api/k8s/__tests__/roles.spec.ts create mode 100644 frontend/src/api/k8s/roles.ts diff --git a/backend/src/plugins/kube.ts b/backend/src/plugins/kube.ts index 86b2429b75..cff1d54cce 100644 --- a/backend/src/plugins/kube.ts +++ b/backend/src/plugins/kube.ts @@ -88,6 +88,8 @@ export default fp(async (fastify: FastifyInstance) => { }`, ), ); + + // TODO call new cleanup fn } }); diff --git a/backend/src/utils/resourceUtils.ts b/backend/src/utils/resourceUtils.ts index 97c0095097..9b23011039 100644 --- a/backend/src/utils/resourceUtils.ts +++ b/backend/src/utils/resourceUtils.ts @@ -682,9 +682,20 @@ export const getConsoleLinks = (): ConsoleLinkKind[] => { return consoleLinksWatcher.getResources(); }; +// TODO +// * List all RoleBindings in all namespaces filtered with labelselector for opendatahub.io/dashboard: 'true' +// * Filter by RoleRef kind = ClusterRole and name = view +// * Filter that by ones that have ownerReferences with kind InferenceService, Subject kind ServiceAccount +// * Delete and replace with new RoleBinding to new role with the same subject and ownerReferences +// Note: bring ownerReference util into backend +// export const cleanupKserveRoleBindings = ...; + +// TODO test with RB llama-gem-view in kserve namespace + /** * Converts GPU usage to use accelerator by adding an accelerator profile CRD to the cluster if GPU usage is detected */ +// TODO copy this approach for migrating for https://issues.redhat.com/browse/RHOAIENG-11010 export const cleanupGPU = async (fastify: KubeFastifyInstance): Promise => { // When we startup — in kube.ts we can handle a migration (catch ALL promise errors — exit gracefully and use fastify logging) // Check for migration-gpu-status configmap in dashboard namespace — if found, exit early diff --git a/frontend/src/__mocks__/mockRoleK8sResource.ts b/frontend/src/__mocks__/mockRoleK8sResource.ts new file mode 100644 index 0000000000..b69b4c3a6e --- /dev/null +++ b/frontend/src/__mocks__/mockRoleK8sResource.ts @@ -0,0 +1,32 @@ +import { genUID } from '~/__mocks__/mockUtils'; +import { KnownLabels, ResourceRule, RoleKind } from '~/k8sTypes'; + +type MockResourceConfigType = { + name?: string; + namespace?: string; + rules?: ResourceRule[]; + roleRefName?: string; + uid?: string; + modelRegistryName?: string; + isProjectSubject?: boolean; +}; + +export const mockRoleK8sResource = ({ + name = 'test-name-view', + namespace = 'test-project', + rules = [], + uid = genUID('role'), +}: MockResourceConfigType): RoleKind => ({ + kind: 'Role', + apiVersion: 'rbac.authorization.k8s.io/v1', + metadata: { + name, + namespace, + uid, + creationTimestamp: '2023-02-14T21:43:59Z', + labels: { + [KnownLabels.DASHBOARD_RESOURCE]: 'true', + }, + }, + rules, +}); diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index 1e1e791e57..c9f464c809 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -8,6 +8,7 @@ export * from './k8s/notebooks'; export * from './k8s/pods'; export * from './k8s/projects'; export * from './k8s/pvcs'; +export * from './k8s/roles'; export * from './k8s/roleBindings'; export * from './k8s/routes'; export * from './k8s/secrets'; diff --git a/frontend/src/api/k8s/__tests__/roleBindings.spec.ts b/frontend/src/api/k8s/__tests__/roleBindings.spec.ts index b9846938e5..bec6ded342 100644 --- a/frontend/src/api/k8s/__tests__/roleBindings.spec.ts +++ b/frontend/src/api/k8s/__tests__/roleBindings.spec.ts @@ -14,7 +14,7 @@ import { createRoleBinding, deleteRoleBinding, generateRoleBindingPermissions, - generateRoleBindingServingRuntime, + generateRoleBindingServiceAccount, getRoleBinding, listRoleBindings, patchRoleBindingOwnerRef, @@ -73,7 +73,12 @@ const createRoleBindingObject = (roleRefName: string, subjects: RoleBindingSubje describe('generateRoleBindingServingRuntime', () => { it('should generate serving runtime role binding ', () => { - const result = generateRoleBindingServingRuntime('rbName', 'serviceAccountName', namespace); + const result = generateRoleBindingServiceAccount( + 'rbName', + 'serviceAccountName', + { kind: 'ClusterRole', name: 'view' }, + namespace, + ); const subjects = [ { kind: 'ServiceAccount', diff --git a/frontend/src/api/k8s/__tests__/roles.spec.ts b/frontend/src/api/k8s/__tests__/roles.spec.ts new file mode 100644 index 0000000000..260b76b034 --- /dev/null +++ b/frontend/src/api/k8s/__tests__/roles.spec.ts @@ -0,0 +1,92 @@ +import { k8sCreateResource, k8sGetResource } from '@openshift/dynamic-plugin-sdk-utils'; +import { mockRoleK8sResource } from '~/__mocks__/mockRoleK8sResource'; +import { KnownLabels, RoleKind } from '~/k8sTypes'; +import { createRole, generateRoleInferenceService, getRole } from '~/api/k8s/roles'; +import { RoleModel } from '~/api/models/k8s'; + +jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ + k8sGetResource: jest.fn(), + k8sCreateResource: jest.fn(), + k8sDeleteResource: jest.fn(), +})); + +const k8sGetResourceMock = jest.mocked(k8sGetResource); +const k8sCreateResourceMock = jest.mocked(k8sCreateResource); + +const namespace = 'namespace'; +const roleMock = mockRoleK8sResource({ name: 'roleName', namespace }); + +describe('generateRoleInferenceService', () => { + it('should generate role for inference service', () => { + const result = generateRoleInferenceService('roleName', 'inferenceServiceName', namespace); + expect(result).toStrictEqual({ + apiVersion: 'rbac.authorization.k8s.io/v1', + kind: 'Role', + metadata: { + name: 'roleName', + namespace, + labels: { + [KnownLabels.DASHBOARD_RESOURCE]: 'true', + }, + }, + rules: [ + { + verbs: ['get'], + apiGroups: ['serving.kserve.io'], + resources: ['inferenceservices'], + resourceNames: ['inferenceServiceName'], + }, + ], + } satisfies RoleKind); + }); +}); + +describe('getRole', () => { + it('should fetch role', async () => { + k8sGetResourceMock.mockResolvedValue(roleMock); + const result = await getRole('projectName', 'roleName'); + expect(k8sGetResourceMock).toHaveBeenCalledWith({ + model: RoleModel, + queryOptions: { name: 'roleName', ns: 'projectName' }, + }); + expect(k8sGetResourceMock).toHaveBeenCalledTimes(1); + expect(result).toStrictEqual(roleMock); + }); + + it('should handle errors and rethrow', async () => { + k8sGetResourceMock.mockRejectedValue(new Error('error1')); + await expect(getRole('projectName', 'roleName')).rejects.toThrow('error1'); + expect(k8sGetResourceMock).toHaveBeenCalledTimes(1); + expect(k8sGetResourceMock).toHaveBeenCalledWith({ + model: RoleModel, + queryOptions: { name: 'roleName', ns: 'projectName' }, + }); + }); +}); + +describe('createRole', () => { + it('should create role', async () => { + k8sCreateResourceMock.mockResolvedValue(roleMock); + const result = await createRole(roleMock); + expect(k8sCreateResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, + model: RoleModel, + queryOptions: { queryParams: {} }, + resource: roleMock, + }); + expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1); + expect(result).toStrictEqual(roleMock); + }); + + it('should handle errors and rethrow', async () => { + k8sCreateResourceMock.mockRejectedValue(new Error('error1')); + await expect(createRole(roleMock)).rejects.toThrow('error1'); + expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1); + expect(k8sCreateResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, + model: RoleModel, + queryOptions: { queryParams: {} }, + resource: roleMock, + }); + }); +}); diff --git a/frontend/src/api/k8s/roleBindings.ts b/frontend/src/api/k8s/roleBindings.ts index 7805523b5b..e373571818 100644 --- a/frontend/src/api/k8s/roleBindings.ts +++ b/frontend/src/api/k8s/roleBindings.ts @@ -21,9 +21,10 @@ import { applyK8sAPIOptions } from '~/api/apiMergeUtils'; import { RoleBindingPermissionsRoleType } from '~/concepts/roleBinding/types'; import { addOwnerReference } from '~/api/k8sUtils'; -export const generateRoleBindingServingRuntime = ( +export const generateRoleBindingServiceAccount = ( name: string, serviceAccountName: string, + roleRef: Omit, namespace: string, ): RoleBindingKind => { const roleBindingObject: RoleBindingKind = { @@ -38,8 +39,7 @@ export const generateRoleBindingServingRuntime = ( }, roleRef: { apiGroup: 'rbac.authorization.k8s.io', - kind: 'ClusterRole', - name: 'view', + ...roleRef, }, subjects: [ { diff --git a/frontend/src/api/k8s/roles.ts b/frontend/src/api/k8s/roles.ts new file mode 100644 index 0000000000..519d8a8983 --- /dev/null +++ b/frontend/src/api/k8s/roles.ts @@ -0,0 +1,40 @@ +import { k8sCreateResource, k8sGetResource } from '@openshift/dynamic-plugin-sdk-utils'; +import { K8sAPIOptions, KnownLabels, RoleKind } from '~/k8sTypes'; +import { RoleModel } from '~/api/models'; +import { applyK8sAPIOptions } from '~/api/apiMergeUtils'; + +export const generateRoleInferenceService = ( + roleName: string, + inferenceServiceName: string, + namespace: string, +): RoleKind => { + const role: RoleKind = { + apiVersion: 'rbac.authorization.k8s.io/v1', + kind: 'Role', + metadata: { + name: roleName, + namespace, + labels: { + [KnownLabels.DASHBOARD_RESOURCE]: 'true', + }, + }, + rules: [ + { + verbs: ['get'], + apiGroups: ['serving.kserve.io'], + resources: ['inferenceservices'], + resourceNames: [inferenceServiceName], + }, + ], + }; + return role; +}; + +export const getRole = (namespace: string, roleName: string): Promise => + k8sGetResource({ + model: RoleModel, + queryOptions: { name: roleName, ns: namespace }, + }); + +export const createRole = (data: RoleKind, opts?: K8sAPIOptions): Promise => + k8sCreateResource(applyK8sAPIOptions({ model: RoleModel, resource: data }, opts)); diff --git a/frontend/src/api/models/k8s.ts b/frontend/src/api/models/k8s.ts index ba47097926..254fe5a720 100644 --- a/frontend/src/api/models/k8s.ts +++ b/frontend/src/api/models/k8s.ts @@ -44,6 +44,13 @@ export const NamespaceModel: K8sModelCommon = { plural: 'namespaces', }; +export const RoleModel: K8sModelCommon = { + apiVersion: 'v1', + apiGroup: 'rbac.authorization.k8s.io', + kind: 'Role', + plural: 'roles', +}; + export const RoleBindingModel: K8sModelCommon = { apiVersion: 'v1', apiGroup: 'rbac.authorization.k8s.io', diff --git a/frontend/src/k8sTypes.ts b/frontend/src/k8sTypes.ts index 3dd3b2b58b..1f9435cb7c 100644 --- a/frontend/src/k8sTypes.ts +++ b/frontend/src/k8sTypes.ts @@ -539,6 +539,14 @@ export type RoleBindingRoleRef = { name: string; }; +export type RoleKind = K8sResourceCommon & { + metadata: { + name: string; + namespace: string; + }; + rules: ResourceRule[]; +}; + export type RoleBindingKind = K8sResourceCommon & { metadata: { name: string; @@ -1047,6 +1055,13 @@ export type SelfSubjectAccessReviewKind = K8sResourceCommon & { }; }; +export type ResourceRule = { + verbs: string[]; + apiGroups?: string[]; + resourceNames?: string[]; + resources?: string[]; +}; + export type SelfSubjectRulesReviewKind = K8sResourceCommon & { spec: { namespace: string; @@ -1057,12 +1072,7 @@ export type SelfSubjectRulesReviewKind = K8sResourceCommon & { verbs: string[]; nonResourceURLs?: string[]; }[]; - resourceRules: { - verbs: string[]; - apiGroups?: string[]; - resourceNames?: string[]; - resources?: string[]; - }[]; + resourceRules: ResourceRule[]; evaluationError?: string; }; }; diff --git a/frontend/src/pages/modelServing/screens/projects/utils.ts b/frontend/src/pages/modelServing/screens/projects/utils.ts index bfb8b0aac3..c6ffa63a31 100644 --- a/frontend/src/pages/modelServing/screens/projects/utils.ts +++ b/frontend/src/pages/modelServing/screens/projects/utils.ts @@ -419,6 +419,7 @@ export const getSubmitInferenceServiceResourceFn = ( createData.project, createTokenAuth, inferenceService, + isModelMesh, secrets || [], { dryRun, @@ -504,6 +505,7 @@ export const getSubmitServingRuntimeResourcesFn = ( namespace, createTokenAuth, editInfo.servingRuntime, + isModelMesh, editInfo.secrets, { dryRun, @@ -529,6 +531,7 @@ export const getSubmitServingRuntimeResourcesFn = ( namespace, createTokenAuth, servingRuntime, + isModelMesh, editInfo?.secrets, { dryRun, diff --git a/frontend/src/pages/modelServing/utils.ts b/frontend/src/pages/modelServing/utils.ts index d17b91bb88..170ae13a7b 100644 --- a/frontend/src/pages/modelServing/utils.ts +++ b/frontend/src/pages/modelServing/utils.ts @@ -11,13 +11,16 @@ import { createRoleBinding, createSecret, deleteSecret, - generateRoleBindingServingRuntime, + generateRoleInferenceService, + generateRoleBindingServiceAccount, replaceSecret, assembleServiceAccount, createServiceAccount, getRoleBinding, addOwnerReference, getServiceAccount, + getRole, + createRole, } from '~/api'; import { SecretKind, @@ -26,6 +29,7 @@ import { ServingRuntimeKind, InferenceServiceKind, ServiceAccountKind, + RoleKind, } from '~/k8sTypes'; import { ContainerResources } from '~/types'; import { getDisplayNameFromK8sResource, translateDisplayNameForK8s } from '~/concepts/k8s/utils'; @@ -41,6 +45,7 @@ import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/s type TokenNames = { serviceAccountName: string; + roleName: string; roleBindingName: string; }; @@ -49,6 +54,7 @@ export const getModelServingRuntimeName = (namespace: string): string => export const getModelServiceAccountName = (name: string): string => `${name}-sa`; +export const getModelRole = (name: string): string => `${name}-view-role`; export const getModelRoleBinding = (name: string): string => `${name}-view`; const isValidCpuOrMemoryValue = (value?: string | number) => @@ -70,23 +76,50 @@ export const setUpTokenAuth = async ( namespace: string, createTokenAuth: boolean, owner: ServingRuntimeKind | InferenceServiceKind, + isModelMesh?: boolean, existingSecrets?: SecretKind[], opts?: K8sAPIOptions, ): Promise => { - const { serviceAccountName, roleBindingName } = getTokenNames(deployedModelName, namespace); + const { serviceAccountName, roleName, roleBindingName } = getTokenNames( + deployedModelName, + namespace, + ); const serviceAccount = addOwnerReference( assembleServiceAccount(serviceAccountName, namespace), owner, ); + + // We only need the inferenceservice view role for KServe, not ModelMesh + const role = !isModelMesh + ? addOwnerReference(generateRoleInferenceService(roleName, deployedModelName, namespace), owner) + : null; + const roleBinding = addOwnerReference( - generateRoleBindingServingRuntime(roleBindingName, serviceAccountName, namespace), + generateRoleBindingServiceAccount( + roleBindingName, + serviceAccountName, + role + ? { + kind: 'Role', + name: roleName, + } + : { + // Fallback to insecure ClusterRole for ModelMesh + // This is a security issue we will not fix for now, this may change in the future. + // See https://issues.redhat.com/browse/RHOAIENG-12314 + kind: 'ClusterRole', + name: 'view', + }, + namespace, + ), owner, ); return ( createTokenAuth ? Promise.all([ createServiceAccountIfMissing(serviceAccount, namespace, opts), + ...(role ? [createRoleIfMissing(role, namespace, opts)] : []), createRoleBindingIfMissing(roleBinding, namespace, opts), ]) : Promise.resolve() @@ -107,6 +140,18 @@ export const createServiceAccountIfMissing = async ( return Promise.reject(e); }); +export const createRoleIfMissing = async ( + role: RoleKind, + namespace: string, + opts?: K8sAPIOptions, +): Promise => + getRole(role.metadata.name, namespace).catch((e) => { + if (e.statusObject?.code === 404) { + return createRole(role, opts); + } + return Promise.reject(e); + }); + export const createRoleBindingIfMissing = async ( rolebinding: RoleBindingKind, namespace: string, @@ -158,9 +203,10 @@ export const getTokenNames = (servingRuntimeName: string, namespace: string): To servingRuntimeName !== '' ? servingRuntimeName : getModelServingRuntimeName(namespace); const serviceAccountName = getModelServiceAccountName(name); + const roleName = getModelRole(name); const roleBindingName = getModelRoleBinding(name); - return { serviceAccountName, roleBindingName }; + return { serviceAccountName, roleName, roleBindingName }; }; export const getResourceSize = ( From 6b039391263b6846c26bbc019346eee0701b8ae9 Mon Sep 17 00:00:00 2001 From: Mike Turley Date: Mon, 9 Sep 2024 15:59:39 -0400 Subject: [PATCH 2/5] Handle migration of old user data Signed-off-by: Mike Turley --- backend/src/plugins/kube.ts | 14 +- backend/src/utils/resourceUtils.ts | 221 +++++++++++++++++++++++------ 2 files changed, 188 insertions(+), 47 deletions(-) diff --git a/backend/src/plugins/kube.ts b/backend/src/plugins/kube.ts index cff1d54cce..0c91794c59 100644 --- a/backend/src/plugins/kube.ts +++ b/backend/src/plugins/kube.ts @@ -5,7 +5,11 @@ import * as jsYaml from 'js-yaml'; import * as k8s from '@kubernetes/client-node'; import { errorHandler, isKubeFastifyInstance } from '../utils'; import { DEV_MODE } from '../utils/constants'; -import { cleanupGPU, initializeWatchedResources } from '../utils/resourceUtils'; +import { + cleanupGPU, + cleanupKserveRoleBindings, + initializeWatchedResources, +} from '../utils/resourceUtils'; const CONSOLE_CONFIG_YAML_FIELD = 'console-config.yaml'; @@ -89,7 +93,13 @@ export default fp(async (fastify: FastifyInstance) => { ), ); - // TODO call new cleanup fn + cleanupKserveRoleBindings(fastify).catch((e) => + fastify.log.error( + `Unable to fully convert kserve rolebindings to use secure role. ${ + e.response?.body?.message || e.message || e + }`, + ), + ); } }); diff --git a/backend/src/utils/resourceUtils.ts b/backend/src/utils/resourceUtils.ts index 9b23011039..2c2e5a20ce 100644 --- a/backend/src/utils/resourceUtils.ts +++ b/backend/src/utils/resourceUtils.ts @@ -1,6 +1,14 @@ import * as _ from 'lodash'; import createError from 'http-errors'; -import { PatchUtils, V1ConfigMap, V1Namespace, V1NamespaceList } from '@kubernetes/client-node'; +import { + PatchUtils, + V1ConfigMap, + V1Namespace, + V1NamespaceList, + V1Role, + V1RoleBinding, + V1RoleBindingList, +} from '@kubernetes/client-node'; import { AcceleratorProfileKind, BuildPhase, @@ -21,6 +29,7 @@ import { TolerationEffect, TolerationOperator, DataScienceClusterKindStatus, + KnownLabels, } from '../types'; import { DEFAULT_ACTIVE_TIMEOUT, @@ -682,15 +691,170 @@ export const getConsoleLinks = (): ConsoleLinkKind[] => { return consoleLinksWatcher.getResources(); }; -// TODO -// * List all RoleBindings in all namespaces filtered with labelselector for opendatahub.io/dashboard: 'true' -// * Filter by RoleRef kind = ClusterRole and name = view -// * Filter that by ones that have ownerReferences with kind InferenceService, Subject kind ServiceAccount -// * Delete and replace with new RoleBinding to new role with the same subject and ownerReferences -// Note: bring ownerReference util into backend -// export const cleanupKserveRoleBindings = ...; +const shouldMigrationContinue = async ( + fastify: KubeFastifyInstance, + configMapName: string, + description: string, +): Promise => + fastify.kube.coreV1Api + .readNamespacedConfigMap(configMapName, fastify.kube.namespace) + .then(() => { + // Found configmap, not continuing + fastify.log.info(`${description} migration already completed, skipping`); + return false; + }) + .catch((e) => { + if (e.statusCode === 404) { + // No config saying we have already migrated, continue + return true; + } + throw `fetching ${description} migration configmap had a ${e.statusCode} error: ${ + e.response?.body?.message || e?.response?.statusMessage + }`; + }); + +const createSuccessfulMigrationConfigMap = async ( + fastify: KubeFastifyInstance, + configMapName: string, + description: string, +): Promise => { + // Create configmap to flag operation as successful + const configMap: V1ConfigMap = { + metadata: { + name: configMapName, + namespace: fastify.kube.namespace, + }, + data: { + migratedCompleted: 'true', + }, + }; + return await fastify.kube.coreV1Api + .createNamespacedConfigMap(fastify.kube.namespace, configMap) + .then(() => fastify.log.info(`Successfully migrated ${description}`)) + .catch((e) => { + throw `A ${ + e.statusCode + } error occurred when trying to create configmap for ${description} migration: ${ + e.response?.body?.message || e?.response?.statusMessage + }`; + }); +}; + +export const cleanupKserveRoleBindings = async (fastify: KubeFastifyInstance): Promise => { + // When we startup — in kube.ts we can handle a migration (catch ALL promise errors — exit gracefully and use fastify logging) + // Check for migration-kserve-inferenceservices-role configmap in dashboard namespace — if found, exit early + const CONFIG_MAP_NAME = 'migration-kserve-inferenceservices-role'; + const DESCRIPTION = 'KServe secure rolebindings'; + + const continueProcessing = await shouldMigrationContinue(fastify, CONFIG_MAP_NAME, DESCRIPTION); + + if (continueProcessing) { + const roleBindings = await fastify.kube.customObjectsApi.listClusterCustomObject( + 'rbac.authorization.k8s.io', + 'v1', + 'rolebindings', + undefined, + undefined, + undefined, + `${KnownLabels.DASHBOARD_RESOURCE} = true`, + ); + const kserveSARoleBindings = + (roleBindings?.body as V1RoleBindingList).items?.filter( + ({ roleRef, subjects, metadata }) => + roleRef.kind === 'ClusterRole' && + roleRef.name === 'view' && + subjects.length === 1 && + subjects[0].kind === 'ServiceAccount' && + metadata?.ownerReferences.length === 1 && + metadata?.ownerReferences[0].kind === 'InferenceService', + ) || []; + + const replaceRoleBinding = async (existingRoleBinding: V1RoleBinding) => { + const inferenceServiceName = existingRoleBinding.metadata?.ownerReferences[0].name; + const namespace = existingRoleBinding.metadata?.namespace; + const newRoleBindingName = `${inferenceServiceName}-view`; + const newRoleName = `${inferenceServiceName}-view-role`; + + const newRole: V1Role = { + apiVersion: 'rbac.authorization.k8s.io/v1', + kind: 'Role', + metadata: { + name: newRoleName, + namespace, + labels: { + [KnownLabels.DASHBOARD_RESOURCE]: 'true', + }, + }, + rules: [ + { + verbs: ['get'], + apiGroups: ['serving.kserve.io'], + resources: ['inferenceservices'], + resourceNames: [inferenceServiceName], + }, + ], + }; + + const newRoleBinding: V1RoleBinding = { + kind: 'RoleBinding', + apiVersion: 'rbac.authorization.k8s.io/v1', + metadata: { + name: newRoleBindingName, + namespace, + labels: existingRoleBinding.metadata?.labels, + ownerReferences: existingRoleBinding.metadata?.ownerReferences, + }, + subjects: existingRoleBinding.subjects, + roleRef: { + apiGroup: 'rbac.authorization.k8s.io', + kind: 'Role', + name: newRoleName, + }, + }; + + // Create new role if it doesn't already exist + await fastify.kube.customObjectsApi + .getNamespacedCustomObject( + 'rbac.authorization.k8s.io', + 'v1', + namespace, + 'roles', + newRoleName, + ) + .catch((e) => { + if (e.statusCode === 404) { + return fastify.kube.customObjectsApi.createNamespacedCustomObject( + 'rbac.authorization.k8s.io', + 'v1', + namespace, + 'roles', + newRole, + ); + } + }); + + // Delete and replace old RB because we can't patch rolebindings + await fastify.kube.customObjectsApi.deleteNamespacedCustomObject( + 'rbac.authorization.k8s.io', + 'v1', + namespace, + 'rolebindings', + existingRoleBinding?.metadata.name, + ); + await fastify.kube.customObjectsApi.createNamespacedCustomObject( + 'rbac.authorization.k8s.io', + 'v1', + namespace, + 'rolebindings', + newRoleBinding, + ); + }; -// TODO test with RB llama-gem-view in kserve namespace + await Promise.all(kserveSARoleBindings.map(replaceRoleBinding)); + + await createSuccessfulMigrationConfigMap(fastify, CONFIG_MAP_NAME, DESCRIPTION); + } +}; /** * Converts GPU usage to use accelerator by adding an accelerator profile CRD to the cluster if GPU usage is detected @@ -700,24 +864,9 @@ export const cleanupGPU = async (fastify: KubeFastifyInstance): Promise => // When we startup — in kube.ts we can handle a migration (catch ALL promise errors — exit gracefully and use fastify logging) // Check for migration-gpu-status configmap in dashboard namespace — if found, exit early const CONFIG_MAP_NAME = 'migration-gpu-status'; + const DESCRIPTION = 'GPU'; - const continueProcessing = await fastify.kube.coreV1Api - .readNamespacedConfigMap(CONFIG_MAP_NAME, fastify.kube.namespace) - .then(() => { - // Found configmap, not continuing - fastify.log.info(`GPU migration already completed, skipping`); - return false; - }) - .catch((e) => { - if (e.statusCode === 404) { - // No config saying we have already migrated gpus, continue - return true; - } else { - throw `fetching gpu migration configmap had a ${e.statusCode} error: ${ - e.response?.body?.message || e?.response?.statusMessage - }`; - } - }); + const continueProcessing = await shouldMigrationContinue(fastify, CONFIG_MAP_NAME, DESCRIPTION); if (continueProcessing) { // Read existing AcceleratorProfiles @@ -795,25 +944,7 @@ export const cleanupGPU = async (fastify: KubeFastifyInstance): Promise => } } - // Create configmap to flag operation as successful - const configMap = { - metadata: { - name: CONFIG_MAP_NAME, - namespace: fastify.kube.namespace, - }, - data: { - migratedCompleted: 'true', - }, - }; - - await fastify.kube.coreV1Api - .createNamespacedConfigMap(fastify.kube.namespace, configMap) - .then(() => fastify.log.info('Successfully migrated GPUs to accelerator profiles')) - .catch((e) => { - throw `A ${e.statusCode} error occurred when trying to create gpu migration configmap: ${ - e.response?.body?.message || e?.response?.statusMessage - }`; - }); + await createSuccessfulMigrationConfigMap(fastify, CONFIG_MAP_NAME, DESCRIPTION); } }; /** From 357be4d16fe6938e4e67bef2878de4cd6a95cd4e Mon Sep 17 00:00:00 2001 From: Mike Turley Date: Wed, 11 Sep 2024 17:38:06 -0400 Subject: [PATCH 3/5] Update tests Signed-off-by: Mike Turley --- .../modelServing/__tests__/utils.spec.ts | 142 +++++++++++++----- 1 file changed, 105 insertions(+), 37 deletions(-) diff --git a/frontend/src/pages/modelServing/__tests__/utils.spec.ts b/frontend/src/pages/modelServing/__tests__/utils.spec.ts index 1192ba15df..22c62173c6 100644 --- a/frontend/src/pages/modelServing/__tests__/utils.spec.ts +++ b/frontend/src/pages/modelServing/__tests__/utils.spec.ts @@ -9,14 +9,17 @@ import { ContainerResources } from '~/types'; import { mockServiceAccountK8sResource } from '~/__mocks__/mockServiceAccountK8sResource'; import { mockRoleBindingK8sResource } from '~/__mocks__/mockRoleBindingK8sResource'; import { + createRole, createRoleBinding, createSecret, createServiceAccount, + getRole, getRoleBinding, getServiceAccount, } from '~/api'; import { mock404Error } from '~/__mocks__/mockK8sStatus'; import { mockInferenceServiceK8sResource } from '~/__mocks__/mockInferenceServiceK8sResource'; +import { mockRoleK8sResource } from '~/__mocks__/mockRoleK8sResource'; jest.mock('~/api', () => ({ ...jest.requireActual('~/api'), @@ -24,6 +27,8 @@ jest.mock('~/api', () => ({ createServiceAccount: jest.fn(), getRoleBinding: jest.fn(), createRoleBinding: jest.fn(), + getRole: jest.fn(), + createRole: jest.fn(), createSecret: jest.fn(), })); @@ -85,6 +90,11 @@ describe('setUpTokenAuth', () => { .mockImplementation((name: string, namespace: string) => Promise.resolve(mockServiceAccountK8sResource({ name, namespace })), ); + jest + .mocked(getRole) + .mockImplementation((name: string, namespace: string) => + Promise.resolve(mockRoleK8sResource({ name, namespace })), + ); jest .mocked(getRoleBinding) .mockImplementation((name: string, namespace: string) => @@ -94,6 +104,9 @@ describe('setUpTokenAuth', () => { jest .mocked(getServiceAccount) .mockImplementation(() => Promise.reject({ statusObject: mock404Error({}) })); + jest + .mocked(getRole) + .mockImplementation(() => Promise.reject({ statusObject: mock404Error({}) })); jest .mocked(getRoleBinding) .mockImplementation(() => Promise.reject({ statusObject: mock404Error({}) })); @@ -116,46 +129,101 @@ describe('setUpTokenAuth', () => { tokens: [{ uuid: '', name: 'default-name', error: '' }], }; - it('should create service account, role binding and secrets if createTokenAuth is true', async () => { - setMockImplementations(false); - await setUpTokenAuth( - { ...fillData, tokenAuth: true }, - 'test-name', - 'test-project', - true, - mockServingRuntimeK8sResource({ name: 'test-name-sa', namespace: 'test-project' }), - ); - expect(createServiceAccount).toHaveBeenCalled(); - expect(createRoleBinding).toHaveBeenCalled(); - expect(createSecret).toHaveBeenCalled(); - }); + describe('for kserve', () => { + it('should create service account, role, role binding and secrets if createTokenAuth is true', async () => { + setMockImplementations(false); + await setUpTokenAuth( + { ...fillData, tokenAuth: true }, + 'test-name', + 'test-project', + true, + mockServingRuntimeK8sResource({ name: 'test-name-sa', namespace: 'test-project' }), + false, + ); + expect(createServiceAccount).toHaveBeenCalled(); + expect(createRole).toHaveBeenCalled(); + expect(createRoleBinding).toHaveBeenCalled(); + expect(createSecret).toHaveBeenCalled(); + }); + + it('should not create service account and role binding if they already exist', async () => { + setMockImplementations(true); + await setUpTokenAuth( + { ...fillData, tokenAuth: true }, + 'test-name', + 'test-project', + true, + mockServingRuntimeK8sResource({ name: 'test-name-sa', namespace: 'test-project' }), + false, + ); + expect(createServiceAccount).not.toHaveBeenCalled(); + expect(createRole).not.toHaveBeenCalled(); + expect(createRoleBinding).not.toHaveBeenCalled(); + expect(createSecret).toHaveBeenCalled(); + }); - it('should not create service account and role binding if they already exist', async () => { - setMockImplementations(true); - await setUpTokenAuth( - { ...fillData, tokenAuth: true }, - 'test-name', - 'test-project', - true, - mockServingRuntimeK8sResource({ name: 'test-name-sa', namespace: 'test-project' }), - ); - expect(createServiceAccount).not.toHaveBeenCalled(); - expect(createRoleBinding).not.toHaveBeenCalled(); - expect(createSecret).toHaveBeenCalled(); + it('should not create service account and role binding if createTokenAuth is false', async () => { + setMockImplementations(false); + await setUpTokenAuth( + { ...fillData, tokenAuth: false }, + 'test-name', + 'test-project', + false, + mockServingRuntimeK8sResource({ name: 'test-name-sa', namespace: 'test-project' }), + false, + ); + expect(createServiceAccount).not.toHaveBeenCalled(); + expect(createRole).not.toHaveBeenCalled(); + expect(createRoleBinding).not.toHaveBeenCalled(); + expect(createSecret).toHaveBeenCalled(); + }); }); - it('should not create service account and role binding if createTokenAuth is false', async () => { - setMockImplementations(false); - await setUpTokenAuth( - { ...fillData, tokenAuth: false }, - 'test-name', - 'test-project', - false, - mockServingRuntimeK8sResource({ name: 'test-name-sa', namespace: 'test-project' }), - ); - expect(createServiceAccount).not.toHaveBeenCalled(); - expect(createRoleBinding).not.toHaveBeenCalled(); - expect(createSecret).toHaveBeenCalled(); + describe('for modelmesh', () => { + it('should create service account, role binding and secrets if createTokenAuth is true', async () => { + setMockImplementations(false); + await setUpTokenAuth( + { ...fillData, tokenAuth: true }, + 'test-name', + 'test-project', + true, + mockServingRuntimeK8sResource({ name: 'test-name-sa', namespace: 'test-project' }), + true, + ); + expect(createServiceAccount).toHaveBeenCalled(); + expect(createRoleBinding).toHaveBeenCalled(); + expect(createSecret).toHaveBeenCalled(); + }); + + it('should not create service account and role binding if they already exist', async () => { + setMockImplementations(true); + await setUpTokenAuth( + { ...fillData, tokenAuth: true }, + 'test-name', + 'test-project', + true, + mockServingRuntimeK8sResource({ name: 'test-name-sa', namespace: 'test-project' }), + true, + ); + expect(createServiceAccount).not.toHaveBeenCalled(); + expect(createRoleBinding).not.toHaveBeenCalled(); + expect(createSecret).toHaveBeenCalled(); + }); + + it('should not create service account and role binding if createTokenAuth is false', async () => { + setMockImplementations(false); + await setUpTokenAuth( + { ...fillData, tokenAuth: false }, + 'test-name', + 'test-project', + false, + mockServingRuntimeK8sResource({ name: 'test-name-sa', namespace: 'test-project' }), + true, + ); + expect(createServiceAccount).not.toHaveBeenCalled(); + expect(createRoleBinding).not.toHaveBeenCalled(); + expect(createSecret).toHaveBeenCalled(); + }); }); }); From 8f57c60e41c232f3ab69133ea06437abc84e804a Mon Sep 17 00:00:00 2001 From: gitdallas <5322142+gitdallas@users.noreply.github.com> Date: Fri, 13 Sep 2024 15:58:06 -0500 Subject: [PATCH 4/5] fix incorrect order of params, update tests Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> --- .../modelServing/servingRuntimeList.cy.ts | 72 +++++++++++++++++++ .../modelServing/__tests__/utils.spec.ts | 4 +- frontend/src/pages/modelServing/utils.ts | 2 +- 3 files changed, 75 insertions(+), 3 deletions(-) diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts index 7977d4ba5f..a7691f7ef4 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts @@ -47,12 +47,14 @@ import { PodModel, ProjectModel, RoleBindingModel, + RoleModel, RouteModel, SecretModel, ServiceAccountModel, ServingRuntimeModel, TemplateModel, } from '~/__tests__/cypress/cypress/utils/models'; +import { mockRoleK8sResource } from '~/__mocks__/mockRoleK8sResource'; type HandlersProps = { disableKServeConfig?: boolean; @@ -65,6 +67,7 @@ type HandlersProps = { rejectAddSupportServingPlatformProject?: boolean; serviceAccountAlreadyExists?: boolean; roleBindingAlreadyExists?: boolean; + roleAlreadyExists?: boolean; rejectInferenceService?: boolean; rejectServingRuntime?: boolean; rejectDataConnection?: boolean; @@ -103,6 +106,7 @@ const initIntercepts = ({ rejectAddSupportServingPlatformProject = false, serviceAccountAlreadyExists = false, roleBindingAlreadyExists = false, + roleAlreadyExists = false, rejectInferenceService = false, rejectServingRuntime = false, rejectDataConnection = false, @@ -230,6 +234,38 @@ const initIntercepts = ({ }), }, ).as('createRoleBinding'); + cy.interceptK8s( + { + model: RoleModel, + ns: 'test-project', + name: 'test-name-view-role', + }, + roleAlreadyExists + ? { + statusCode: 200, + body: mockRoleK8sResource({ + name: 'test-name-view-role', + namespace: 'test-project', + }), + } + : { statusCode: 404, body: mock404Error({}) }, + ); + cy.interceptK8s( + 'POST', + { + model: RoleModel, + ns: 'test-project', + }, + roleAlreadyExists + ? { statusCode: 409, body: mock409Error({}) } + : { + statusCode: 200, + body: mockRoleK8sResource({ + name: 'test-name-view', + namespace: 'test-project', + }), + }, + ).as('createRole'); cy.interceptK8sList(ServingRuntimeModel, mockK8sResourceList(servingRuntimes)); cy.interceptK8s( 'POST', @@ -593,6 +629,35 @@ describe('Serving Runtime List', () => { cy.get('@createServingRuntime.all').then((interceptions) => { expect(interceptions).to.have.length(2); // 1 dry-run request and 1 actual request }); + + //dry run request + cy.wait('@createRole').then((interception) => { + expect(interception.request.url).to.include('?dryRun=All'); + expect(interception.request.body).to.containSubset({ + metadata: { + name: 'test-name-view-role', + namespace: 'test-project', + ownerReferences: [], + }, + rules: [ + { + verbs: ['get'], + apiGroups: ['serving.kserve.io'], + resources: ['inferenceservices'], + resourceNames: ['test-name'], + }, + ], + }); + }); + + //Actual request + cy.wait('@createRole').then((interception) => { + expect(interception.request.url).not.to.include('?dryRun=All'); + }); + + cy.get('@createRole.all').then((interceptions) => { + expect(interceptions).to.have.length(2); //1 dry run request and 1 actual request + }); }); it('Kserve auth should be hidden when auth is disabled', () => { @@ -1309,6 +1374,9 @@ describe('Serving Runtime List', () => { cy.get('@createRoleBinding.all').then((interceptions) => { expect(interceptions).to.have.length(0); }); + cy.get('@createRole.all').then((interceptions) => { + expect(interceptions).to.have.length(0); + }); }); it('Add model server - create ServiceAccount and RoleBinding if token auth is selected', () => { @@ -1392,6 +1460,7 @@ describe('Serving Runtime List', () => { disableModelMeshConfig: false, serviceAccountAlreadyExists: true, roleBindingAlreadyExists: true, + roleAlreadyExists: true, }); projectDetails.visitSection('test-project', 'model-server'); @@ -1453,6 +1522,9 @@ describe('Serving Runtime List', () => { cy.get('@createRoleBinding.all').then((interceptions) => { expect(interceptions).to.have.length(0); }); + cy.get('@createRole.all').then((interceptions) => { + expect(interceptions).to.have.length(0); + }); }); }); diff --git a/frontend/src/pages/modelServing/__tests__/utils.spec.ts b/frontend/src/pages/modelServing/__tests__/utils.spec.ts index 22c62173c6..957adbff05 100644 --- a/frontend/src/pages/modelServing/__tests__/utils.spec.ts +++ b/frontend/src/pages/modelServing/__tests__/utils.spec.ts @@ -92,12 +92,12 @@ describe('setUpTokenAuth', () => { ); jest .mocked(getRole) - .mockImplementation((name: string, namespace: string) => + .mockImplementation((namespace: string, name: string) => Promise.resolve(mockRoleK8sResource({ name, namespace })), ); jest .mocked(getRoleBinding) - .mockImplementation((name: string, namespace: string) => + .mockImplementation((namespace: string, name: string) => Promise.resolve(mockRoleBindingK8sResource({ name, namespace })), ); } else { diff --git a/frontend/src/pages/modelServing/utils.ts b/frontend/src/pages/modelServing/utils.ts index 170ae13a7b..8ad1547d6e 100644 --- a/frontend/src/pages/modelServing/utils.ts +++ b/frontend/src/pages/modelServing/utils.ts @@ -145,7 +145,7 @@ export const createRoleIfMissing = async ( namespace: string, opts?: K8sAPIOptions, ): Promise => - getRole(role.metadata.name, namespace).catch((e) => { + getRole(namespace, role.metadata.name).catch((e) => { if (e.statusObject?.code === 404) { return createRole(role, opts); } From a122ad06a297f4e8b2a065eece910aa882cc3fa6 Mon Sep 17 00:00:00 2001 From: Mike Turley Date: Wed, 25 Sep 2024 19:04:51 -0400 Subject: [PATCH 5/5] Add missing rules to service account role Signed-off-by: Mike Turley --- manifests/core-bases/base/cluster-role.yaml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/manifests/core-bases/base/cluster-role.yaml b/manifests/core-bases/base/cluster-role.yaml index 97f82cfdad..9bbbb2b080 100644 --- a/manifests/core-bases/base/cluster-role.yaml +++ b/manifests/core-bases/base/cluster-role.yaml @@ -187,6 +187,14 @@ rules: - get resources: - dscinitializations + - apiGroups: + - serving.kserve.io + resources: + - inferenceservices + verbs: + - get + - list + - watch - apiGroups: - modelregistry.opendatahub.io verbs: