MR fixes identified during architecture review (#3263)

* Remove stale TODO - we do need the code in question Signed-off-by: Mike Turley <[email protected]> * Properly return promise when deleting a registry Signed-off-by: Mike Turley <[email protected]> * Exclude namespace when sending MR rolebindings to the backend service due to requestSecurityGuard Signed-off-by: Mike Turley <[email protected]> * Fix tests Signed-off-by: Mike Turley <[email protected]> --------- Signed-off-by: Mike Turley <[email protected]>
opendatahub-io · Sep 27, 2024 · 3783c3f · 3783c3f
1 parent 73b0c4c
commit 3783c3f
Show file tree

Hide file tree

Showing 5 changed files with 21 additions and 13 deletions.
diff --git a/backend/src/routes/api/modelRegistries/index.ts b/backend/src/routes/api/modelRegistries/index.ts
@@ -162,7 +162,7 @@ export default async (fastify: KubeFastifyInstance): Promise<void> => {
         const { modelRegistryName } = request.params;
         try {
           const modelRegistryNamespace = getModelRegistryNamespace(fastify);
-          deleteModelRegistryAndSecret(
+          return deleteModelRegistryAndSecret(
             fastify,
             modelRegistryName,
             modelRegistryNamespace,

diff --git a/backend/src/routes/api/modelRegistryRoleBindings/modelRegistryRolebindingsUtils.ts b/backend/src/routes/api/modelRegistryRoleBindings/modelRegistryRolebindingsUtils.ts
@@ -27,12 +27,19 @@ export const createModelRegistryRoleBinding = async (
   rbRequest: V1RoleBinding,
   mrNamespace: string,
 ): Promise<V1RoleBinding> => {
+  // Re-inject the namespace value that was omitted by the client
+  //   (see createModelRegistryRoleBinding in frontend/src/services/modelRegistrySettingsService.ts)
+  // This will be unnecessary when we remove the backend service as part of https://issues.redhat.com/browse/RHOAIENG-12077
+  const roleBindingWithNamespace = {
+    ...rbRequest,
+    metadata: { ...rbRequest.metadata, namespace: mrNamespace },
+  };
   const response = await (fastify.kube.customObjectsApi.createNamespacedCustomObject(
     MODEL_REGISTRY_ROLE_BINDING_API_GROUP,
     MODEL_REGISTRY_ROLE_BINDING_API_VERSION,
     mrNamespace,
     MODEL_REGISTRY_ROLE_BINDING_PLURAL,
-    rbRequest,
+    roleBindingWithNamespace,
   ) as Promise<{ body: V1RoleBinding }>);
   return response.body;
 };

diff --git a/...tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistryPermissions.cy.ts b/...tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistryPermissions.cy.ts
@@ -430,7 +430,6 @@ describe('MR Permissions', () => {
       cy.interceptOdh(
         'POST /api/modelRegistryRoleBindings',
         mockRoleBindingK8sResource({
-          namespace: MODEL_REGISTRY_DEFAULT_NAMESPACE,
           subjects: projectSubjects,
           roleRefName: 'registry-user-example-mr',
           modelRegistryName: 'example-mr',
@@ -443,9 +442,6 @@ describe('MR Permissions', () => {
 
       cy.wait('@addProject').then((interception) => {
         expect(interception.request.body).to.containSubset({
-          metadata: {
-            namespace: 'odh-model-registries',
-          },
           roleRef: {
             apiGroup: 'rbac.authorization.k8s.io',
             kind: 'Role',
@@ -484,9 +480,6 @@ describe('MR Permissions', () => {
 
       cy.wait('@editProject').then((interception) => {
         expect(interception.request.body).to.containSubset({
-          metadata: {
-            namespace: 'odh-model-registries',
-          },
           roleRef: {
             apiGroup: 'rbac.authorization.k8s.io',
             kind: 'Role',

diff --git a/frontend/src/api/modelRegistry/errorUtils.ts b/frontend/src/api/modelRegistry/errorUtils.ts
@@ -18,7 +18,6 @@ export const handleModelRegistryFailures = <T>(promise: Promise<T>): Promise<T>
       }
       if (isCommonStateError(e)) {
         // Common state errors are handled by useFetchState at storage level, let them deal with it
-        // TODO: check whether we need this or not
         throw e;
       }
       // eslint-disable-next-line no-console

diff --git a/frontend/src/services/modelRegistrySettingsService.ts b/frontend/src/services/modelRegistrySettingsService.ts
@@ -1,3 +1,4 @@
+import * as _ from 'lodash-es';
 import { K8sStatus } from '@openshift/dynamic-plugin-sdk-utils';
 import axios from '~/utilities/axios';
 import { ModelRegistryKind, RoleBindingKind } from '~/k8sTypes';
@@ -66,13 +67,21 @@ export const listModelRegistryRoleBindings = (): Promise<RoleBindingKind[]> =>
       throw new Error(e.response.data.message);
     });
 
-export const createModelRegistryRoleBinding = (data: RoleBindingKind): Promise<RoleBindingKind> =>
-  axios
-    .post(mrRoleBindingsUrl, data)
+export const createModelRegistryRoleBinding = (data: RoleBindingKind): Promise<RoleBindingKind> => {
+  // Don't include the namespace in the object we pass because it would get rejected by requestSecurityGuard.
+  //   (see backend/src/utils/route-security.ts)
+  // Instead the namespace will be reinjected by the backend
+  //   (see backend/src/routes/api/modelRegistries/modelRegistryUtils.ts)
+  // This will be unnecessary when we remove this service as part of https://issues.redhat.com/browse/RHOAIENG-12077
+  const roleBindingWithoutNamespace: RoleBindingKind & { metadata: { namespace?: string } } =
+    _.omit(data, 'metadata.namespace');
+  return axios
+    .post(mrRoleBindingsUrl, roleBindingWithoutNamespace)
     .then((response) => response.data)
     .catch((e) => {
       throw new Error(e.response.data.message);
     });
+};
 
 export const deleteModelRegistryRoleBinding = (roleBindingName: string): Promise<K8sStatus> =>
   axios