Skip to content

Commit

Permalink
Merge pull request #2319 from mturley/548-deploy-model-403-error
Browse files Browse the repository at this point in the history 
Assert edit permissions on appropriate mutations in namespaceUtils, fix promise logic when deploying a model or adding a model server, show inner error messages from the backend when present
  • Loading branch information
@openshift-merge-bot
openshift-merge-bot[bot] authored Jan 17, 2024
2 parents 6219e72 + 739adc8 commit e328be0
Show file tree
Hide file tree
Showing 7 changed files with 328 additions and 81 deletions.
108 changes: 73 additions & 35 deletions backend/src/routes/api/namespaces/namespaceUtils.ts
View file
Original file line number Diff line number Diff line change
@@ -1,29 +1,24 @@
import { PatchUtils, V1SelfSubjectAccessReview } from '@kubernetes/client-node';
import {
PatchUtils,
V1ResourceAttributes,
V1SelfSubjectAccessReview,
} from '@kubernetes/client-node';
import { NamespaceApplicationCase } from './const';
import { K8sStatus, KubeFastifyInstance, OauthFastifyRequest } from '../../../types';
import { createCustomError } from '../../../utils/requestUtils';
import { isK8sStatus, passThrough } from '../k8s/pass-through';

const checkNamespacePermission = (
const createSelfSubjectAccessReview = (
fastify: KubeFastifyInstance,
request: OauthFastifyRequest,
name: string,
resourceAttributes: V1ResourceAttributes,
): Promise<V1SelfSubjectAccessReview | K8sStatus> => {
const kc = fastify.kube.config;
const cluster = kc.getCurrentCluster();
const selfSubjectAccessReviewObject: V1SelfSubjectAccessReview = {
apiVersion: 'authorization.k8s.io/v1',
kind: 'SelfSubjectAccessReview',
spec: {
resourceAttributes: {
group: 'project.openshift.io',
resource: 'projects',
subresource: '',
verb: 'update',
name,
namespace: name,
},
},
spec: { resourceAttributes },
};
return passThrough<V1SelfSubjectAccessReview>(fastify, request, {
url: `${cluster.server}/apis/authorization.k8s.io/v1/selfsubjectaccessreviews`,
Expand All @@ -32,6 +27,34 @@ const checkNamespacePermission = (
});
};

const checkAdminNamespacePermission = (
fastify: KubeFastifyInstance,
request: OauthFastifyRequest,
name: string,
): Promise<V1SelfSubjectAccessReview | K8sStatus> =>
createSelfSubjectAccessReview(fastify, request, {
group: 'project.openshift.io',
resource: 'projects',
subresource: '',
verb: 'update',
name,
namespace: name,
});

const checkEditNamespacePermission = (
fastify: KubeFastifyInstance,
request: OauthFastifyRequest,
name: string,
): Promise<V1SelfSubjectAccessReview | K8sStatus> =>
createSelfSubjectAccessReview(fastify, request, {
group: 'serving.kserve.io',
resource: 'ServingRuntimes',
subresource: '',
verb: 'create',
name,
namespace: name,
});

export const applyNamespaceChange = async (
fastify: KubeFastifyInstance,
request: OauthFastifyRequest,
Expand All @@ -47,40 +70,55 @@ export const applyNamespaceChange = async (
);
}

const selfSubjectAccessReview = await checkNamespacePermission(fastify, request, name);
if (isK8sStatus(selfSubjectAccessReview)) {
throw createCustomError(
selfSubjectAccessReview.reason,
selfSubjectAccessReview.message,
selfSubjectAccessReview.code,
);
}
if (!selfSubjectAccessReview.status.allowed) {
fastify.log.error(`Unable to access the namespace, ${selfSubjectAccessReview.status.reason}`);
throw createCustomError('Forbidden', "You don't have the access to update the namespace", 403);
}

let labels = {};
let checkPermissionsFn = null;
switch (context) {
case NamespaceApplicationCase.DSG_CREATION:
labels = {
'opendatahub.io/dashboard': 'true',
};
{
labels = { 'opendatahub.io/dashboard': 'true' };
checkPermissionsFn = checkAdminNamespacePermission;
}
break;
case NamespaceApplicationCase.MODEL_MESH_PROMOTION:
labels = {
'modelmesh-enabled': 'true',
};
{
labels = { 'modelmesh-enabled': 'true' };
checkPermissionsFn = checkEditNamespacePermission;
}
break;
case NamespaceApplicationCase.KSERVE_PROMOTION:
labels = {
'modelmesh-enabled': 'false',
};
{
labels = { 'modelmesh-enabled': 'false' };
checkPermissionsFn = checkEditNamespacePermission;
}
break;
default:
throw createCustomError('Unknown configuration', 'Cannot apply namespace change', 400);
}

if (checkPermissionsFn === null) {
throw createCustomError(
'Invalid backend state -- dev broken workflow',
'checkPermissionsFn is null -- appropriate permissions must be checked for all actions',
500,
);
}
const selfSubjectAccessReview = await checkPermissionsFn(fastify, request, name);
if (isK8sStatus(selfSubjectAccessReview)) {
throw createCustomError(
selfSubjectAccessReview.reason,
selfSubjectAccessReview.message,
selfSubjectAccessReview.code,
);
}
if (!selfSubjectAccessReview.status.allowed) {
fastify.log.error(`Unable to access the namespace, ${selfSubjectAccessReview.status.reason}`);
throw createCustomError(
'Forbidden',
"You don't have permission to update serving platform labels on the current project.",
403,
);
}

return fastify.kube.coreV1Api
.patchNamespace(name, { metadata: { labels } }, undefined, undefined, undefined, undefined, {
headers: { 'Content-type': PatchUtils.PATCH_FORMAT_JSON_MERGE_PATCH },
Expand Down
21 changes: 21 additions & 0 deletions frontend/src/__mocks__/mockServiceAccountK8sResource.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { genUID } from '~/__mocks__/mockUtils';
import { ServiceAccountKind } from '~/k8sTypes';

type MockResourceConfigType = {
name?: string;
namespace?: string;
};

export const mockServiceAccountK8sResource = ({
name = 'test-model-sa',
namespace = 'test-project',
}: MockResourceConfigType): ServiceAccountKind => ({
kind: 'ServiceAccount',
apiVersion: 'v1',
metadata: {
name,
namespace,
uid: genUID('serviceaccount'),
creationTimestamp: '2023-02-14T21:43:59Z',
},
});
164 changes: 164 additions & 0 deletions frontend/src/__tests__/cypress/cypress/e2e/modelServing/ServingRuntimeList.cy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
mockRouteK8sResourceModelServing,
} from '~/__mocks__/mockRouteK8sResource';
import { mockSecretK8sResource } from '~/__mocks__/mockSecretK8sResource';
import { mockServiceAccountK8sResource } from '~/__mocks__/mockServiceAccountK8sResource';
import {
mockServingRuntimeK8sResource,
mockServingRuntimeK8sResourceLegacy,
Expand Down Expand Up @@ -38,6 +39,7 @@ type HandlersProps = {
projectEnableModelMesh?: boolean;
servingRuntimes?: ServingRuntimeKind[];
inferenceServices?: InferenceServiceKind[];
rejectAddSupportServingPlatformProject?: boolean;
};

const initIntercepts = ({
Expand Down Expand Up @@ -67,6 +69,7 @@ const initIntercepts = ({
activeModelState: 'Loaded',
}),
],
rejectAddSupportServingPlatformProject = false,
}: HandlersProps) => {
cy.intercept(
'/api/dsc/status',
Expand Down Expand Up @@ -110,20 +113,68 @@ const initIntercepts = ({
);
cy.intercept(
{
method: 'GET',
pathname: '/api/k8s/apis/serving.kserve.io/v1beta1/namespaces/test-project/inferenceservices',
},
mockK8sResourceList(inferenceServices),
);
cy.intercept(
{
method: 'POST',
pathname: '/api/k8s/apis/serving.kserve.io/v1beta1/namespaces/test-project/inferenceservices',
},
mockInferenceServiceK8sResource({ name: 'test-inference' }),
).as('createInferenceService');
cy.intercept(
{ pathname: '/api/k8s/api/v1/namespaces/test-project/secrets' },
mockK8sResourceList([mockSecretK8sResource({})]),
);
// used by addSupportServingPlatformProject
cy.intercept(
{
pathname: '/api/namespaces/test-project/*',
},
rejectAddSupportServingPlatformProject
? { statusCode: 401 }
: { statusCode: 200, body: { applied: true } },
);
cy.intercept(
{
method: 'POST',
pathname: '/api/k8s/api/v1/namespaces/test-project/serviceaccounts',
},
mockServiceAccountK8sResource({
name: 'test-model-sa',
namespace: 'test-project',
}),
);
cy.intercept(
{
method: 'GET',
pathname: '/api/k8s/apis/serving.kserve.io/v1alpha1/namespaces/test-project/servingruntimes',
},
mockK8sResourceList(servingRuntimes),
);
cy.intercept(
{
method: 'POST',
pathname: '/api/k8s/apis/serving.kserve.io/v1alpha1/namespaces/test-project/servingruntimes',
},
mockServingRuntimeK8sResource({
name: 'test-model',
namespace: 'test-project',
auth: true,
route: true,
}),
).as('createServingRuntime');
cy.intercept(
{
method: 'GET',
pathname:
'/api/k8s/apis/opendatahub.io/v1alpha/namespaces/opendatahub/odhdashboardconfigs/odh-dashboard-config',
},
mockDashboardConfig({}),
);
cy.intercept(
{
pathname: '/api/k8s/apis/route.openshift.io/v1/namespaces/test-project/routes/test-inference',
Expand Down Expand Up @@ -261,6 +312,58 @@ describe('Serving Runtime List', () => {
kserveModal.findLocationBucketInput().type('test-bucket');
kserveModal.findLocationPathInput().type('test-model/');
kserveModal.findSubmitButton().should('be.enabled');

// test submitting form, the modal should close to indicate success.
kserveModal.findSubmitButton().click();
kserveModal.shouldBeOpen(false);

// the serving runtime should have been created
cy.get('@createServingRuntime.all').then((interceptions) => {
expect(interceptions).to.have.length(2); // 1 dry-run request and 1 actual request
});
});

it('Do not deploy KServe model when user cannot edit namespace', () => {
initIntercepts({
disableModelMeshConfig: false,
disableKServeConfig: false,
servingRuntimes: [],
rejectAddSupportServingPlatformProject: true,
});

projectDetails.visit('test-project');

modelServingSection.findDeployModelButton().click();

kserveModal.shouldBeOpen();

// test filling in minimum required fields
kserveModal.findModelNameInput().type('Test Name');
kserveModal.findServingRuntimeTemplateDropdown().findDropdownItem('Caikit').click();
kserveModal.findModelFrameworkSelect().findSelectOption('onnx - 1').click();
kserveModal.findExistingConnectionSelect().findSelectOption('Test Secret').click();
kserveModal.findNewDataConnectionOption().click();
kserveModal.findLocationNameInput().type('Test Name');
kserveModal.findLocationAccessKeyInput().type('test-key');
kserveModal.findLocationSecretKeyInput().type('test-secret-key');
kserveModal.findLocationEndpointInput().type('test-endpoint');
kserveModal.findLocationBucketInput().type('test-bucket');
kserveModal.findLocationPathInput().type('test-model/');
kserveModal.findSubmitButton().should('be.enabled');

// test submitting form, an error should appear
kserveModal.findSubmitButton().click();
cy.findByText('Error creating model server');

// the serving runtime should NOT have been created
cy.get('@createServingRuntime.all').then((interceptions) => {
expect(interceptions).to.have.length(1); // 1 dry-run request only
});

// the inference service should NOT have been created
cy.get('@createInferenceService.all').then((interceptions) => {
expect(interceptions).to.have.length(0);
});
});

it('No model serving platform available', () => {
Expand Down Expand Up @@ -451,4 +554,65 @@ describe('Serving Runtime List', () => {
editServingRuntimeModal.findAuthenticationCheckbox().uncheck();
editServingRuntimeModal.findSubmitButton().should('be.disabled');
});

it('Successfully add model server when user can edit namespace', () => {
initIntercepts({
projectEnableModelMesh: undefined,
disableKServeConfig: false,
disableModelMeshConfig: false,
});
projectDetails.visit('test-project');

modelServingSection.findAddModelServerButton().click();

createServingRuntimeModal.shouldBeOpen();

// fill in minimum required fields
createServingRuntimeModal.findModelServerNameInput().type('Test Name');
createServingRuntimeModal
.findServingRuntimeTemplateDropdown()
.findDropdownItem('New OVMS Server')
.click();
createServingRuntimeModal.findSubmitButton().should('be.enabled');

// test submitting form, the modal should close to indicate success.
createServingRuntimeModal.findSubmitButton().click();
createServingRuntimeModal.shouldBeOpen(false);

// the serving runtime should have been created
cy.get('@createServingRuntime.all').then((interceptions) => {
expect(interceptions).to.have.length(2); // 1 dry-run request and 1 actual request
});
});

it('Do not add model server when user cannot edit namespace', () => {
initIntercepts({
projectEnableModelMesh: undefined,
disableKServeConfig: false,
disableModelMeshConfig: false,
rejectAddSupportServingPlatformProject: true,
});
projectDetails.visit('test-project');

modelServingSection.findAddModelServerButton().click();

createServingRuntimeModal.shouldBeOpen();

// fill in minimum required fields
createServingRuntimeModal.findModelServerNameInput().type('Test Name');
createServingRuntimeModal
.findServingRuntimeTemplateDropdown()
.findDropdownItem('New OVMS Server')
.click();
createServingRuntimeModal.findSubmitButton().should('be.enabled');

// test submitting form, an error should appear
createServingRuntimeModal.findSubmitButton().click();
cy.findByText('Error creating model server');

// the serving runtime should NOT have been created
cy.get('@createServingRuntime.all').then((interceptions) => {
expect(interceptions).to.have.length(1); // 1 dry-run request only
});
});
});
Loading

0 comments on commit e328be0

Please sign in to comment.