Merge pull request #1866 from DaoDaoNoCode/upstream-issue-463

Log group config errors to the pod and refine logic

backend/src/routes/api/groups-config/groupsConfigUtil.ts

@@ -16,19 +16,12 @@ const SYSTEM_AUTHENTICATED = 'system:authenticated';
 export const getGroupsConfig = async (fastify: KubeFastifyInstance): Promise<GroupsConfig> => {
   const customObjectsApi = fastify.kube.customObjectsApi;
 
-  try {
-    const groupsCluster = await getAllGroups(customObjectsApi);
-    const groupsData = getGroupsCR();
-    const groupsProcessed = processGroupData(groupsData);
-    const groupsConfigProcessed = processGroupConfig(groupsProcessed, groupsCluster);
-    await removeDeletedGroups(fastify, groupsData, groupsConfigProcessed.groupsCRData);
-
-    return groupsConfigProcessed.groupsConfig;
-  } catch (e) {
-    fastify.log.error(e, 'Error retrieving group configuration.');
-    const error = createError(500, 'Error retrieving group configuration');
-    throw error;
-  }
+  const groupsCluster = await getAllGroups(customObjectsApi);
+  const groupsData = getGroupsCR();
+  const groupsProcessed = processGroupData(groupsData);
+  const groupsConfigProcessed = processGroupConfig(fastify, groupsProcessed, groupsCluster);
+  await removeDeletedGroups(fastify, groupsData, groupsConfigProcessed.groupsCRData);
+  return groupsConfigProcessed.groupsConfig;
 };
 
 const transformGroupsConfig = (groupStatus: GroupStatus[]): string[] => {
@@ -38,7 +31,7 @@ const transformGroupsConfig = (groupStatus: GroupStatus[]): string[] => {
 export const updateGroupsConfig = async (
   fastify: KubeFastifyInstance,
   request: FastifyRequest<{ Body: GroupsConfig }>,
-): Promise<{ success: GroupsConfig | null; error: string | null }> => {
+): Promise<GroupsConfig> => {
   const customObjectsApi = fastify.kube.customObjectsApi;
   const { namespace } = fastify.kube;
 
@@ -58,26 +51,17 @@ export const updateGroupsConfig = async (
     const error = createError(403, 'Error, groups cannot be empty');
     throw error;
   }
-  try {
-    const dataUpdated: GroupsConfigBody = {
-      adminGroups: adminConfig.join(','),
-      allowedGroups: allowedConfig.join(','),
-    };
-
-    const groupsData = await updateGroupsCR(fastify, dataUpdated);
-    const groupsProcessed = processGroupData(groupsData);
-    const groupsCluster = await getAllGroups(customObjectsApi);
-    const updatedConfig = processGroupConfig(groupsProcessed, groupsCluster);
-    await removeDeletedGroups(fastify, groupsData, updatedConfig.groupsCRData);
-    return {
-      success: updatedConfig.groupsConfig,
-      error: null,
-    };
-  } catch (e) {
-    fastify.log.error(e, 'Error updating group configuration.');
-    const error = createError(500, 'Error updating group configuration');
-    throw error;
-  }
+  const dataUpdated: GroupsConfigBody = {
+    adminGroups: adminConfig.join(','),
+    allowedGroups: allowedConfig.join(','),
+  };
+
+  const groupsData = await updateGroupsCR(fastify, dataUpdated);
+  const groupsProcessed = processGroupData(groupsData);
+  const groupsCluster = await getAllGroups(customObjectsApi);
+  const updatedConfig = processGroupConfig(fastify, groupsProcessed, groupsCluster);
+  await removeDeletedGroups(fastify, groupsData, updatedConfig.groupsCRData);
+  return updatedConfig.groupsConfig;
 };
 
 const processGroupData = (groupsData: GroupsConfigBody): GroupsConfigBodyList => {
@@ -105,6 +89,7 @@ const mapListToGroupStatus =
  * @returns Processed object with the groups, removing missing groups that might be selected
  */
 const processGroupConfig = (
+  fastify: KubeFastifyInstance,
   groupsDataList: GroupsConfigBodyList,
   groups: string[],
 ): { groupsConfig: GroupsConfig; groupsCRData: GroupsConfigBody } => {
@@ -120,9 +105,14 @@ const processGroupConfig = (
   const groupsConfig: GroupsConfig = {
     adminGroups: adminGroupsConfig,
     allowedGroups: allowedGroupsConfig,
-    errorAdmin: getError(groupsDataList.adminGroups, (group) => !groups.includes(group)),
+    errorAdmin: getError(
+      fastify,
+      groupsDataList.adminGroups.filter((group) => group),
+      (group) => !groups.includes(group),
+    ),
     errorUser: getError(
-      groupsDataList.allowedGroups,
+      fastify,
+      groupsDataList.allowedGroups.filter((group) => group),
       (group) => !groups.includes(group) && group !== SYSTEM_AUTHENTICATED,
     ),
   };
@@ -137,13 +127,26 @@ const processGroupConfig = (
   return { groupsConfig, groupsCRData: updatedBody };
 };
 
-const getError = (array: string[], predicate: (group: string) => boolean): string | undefined => {
+const getError = (
+  fastify: KubeFastifyInstance,
+  array: string[],
+  predicate: (group: string) => boolean,
+): string | undefined => {
+  let error;
+  if (array.length === 0) {
+    error = 'No group is set in the group config, please set one or more group.';
+    fastify.log.error(error);
+    return error;
+  }
+
   const missingItems = array.filter(predicate);
   if (missingItems.length === 0) return undefined;
 
-  return `The group${missingItems.length === 1 ? '' : 's'} ${missingItems.join(
+  error = `The group${missingItems.length === 1 ? '' : 's'} ${missingItems.join(
     ', ',
   )} no longer exists in OpenShift and has been removed from the selected group list.`;
+  fastify.log.error(error);
+  return error;
 };
 
 /**

backend/src/routes/api/groups-config/index.ts

@@ -6,8 +6,13 @@ import { secureAdminRoute } from '../../../utils/route-security';
 export default async (fastify: FastifyInstance): Promise<void> => {
   fastify.get(
     '/',
-    secureAdminRoute(fastify)(async () => {
-      return getGroupsConfig(fastify);
+    secureAdminRoute(fastify)(async (request, reply) => {
+      return getGroupsConfig(fastify).catch((e) => {
+        fastify.log.error(
+          `Error retrieving group configuration, ${e.response?.body?.message || e.message}`,
+        );
+        reply.status(500).send({ message: e.response?.body?.message || e.message });
+      });
     }),
   );
 

backend/src/utils/groupsUtils.ts

@@ -16,7 +16,7 @@ export class MissingGroupError extends Error {
 }
 
 export const getGroupsCR = (): GroupsConfigBody => {
-  if (typeof getDashboardConfig().spec.groupsConfig !== 'undefined') {
+  if (getDashboardConfig().spec.groupsConfig) {
     return getDashboardConfig().spec.groupsConfig;
   }
   throw new Error(`Failed to retrieve Dashboard CR groups configuration`);

frontend/src/services/groupSettingsService.ts

@@ -11,9 +11,7 @@ export const fetchGroupsSettings = (): Promise<GroupsConfig> => {
     });
 };
 
-export const updateGroupsSettings = (
-  settings: GroupsConfig,
-): Promise<{ success: GroupsConfig | null; error: string | null }> => {
+export const updateGroupsSettings = (settings: GroupsConfig): Promise<GroupsConfig> => {
   const url = '/api/groups-config';
   return axios
     .put(url, settings)

frontend/src/utilities/__tests__/useWatchGroups.spec.ts

@@ -0,0 +1,34 @@
+import { testHook } from '~/__tests__/unit/testUtils/hooks';
+import { fetchGroupsSettings } from '~/services/groupSettingsService';
+import { useWatchGroups } from '~/utilities/useWatchGroups';
+
+jest.mock('~/services/groupSettingsService', () => ({
+  fetchGroupsSettings: jest.fn(),
+}));
+
+jest.mock('react-redux', () => ({
+  useDispatch: jest.fn(),
+}));
+
+const fetchGroupSettingsMock = fetchGroupsSettings as jest.Mock;
+
+describe('useWatchGroups', () => {
+  it('should fetch groups successfully', async () => {
+    const mockEmptyGroupSettings = {
+      adminGroups: [],
+      allowedGroups: [],
+    };
+    const mockGroupSettings = {
+      adminGroups: ['odh-admins'],
+      allowedGroups: [],
+    };
+    fetchGroupSettingsMock.mockReturnValue(Promise.resolve(mockGroupSettings));
+
+    const renderResult = testHook(useWatchGroups)();
+    expect(fetchGroupSettingsMock).toHaveBeenCalledTimes(1);
+    expect(renderResult.result.current.groupSettings).toStrictEqual(mockEmptyGroupSettings);
+
+    await renderResult.waitForNextUpdate();
+    expect(renderResult.result.current.groupSettings).toStrictEqual(mockGroupSettings);
+  });
+});

frontend/src/utilities/useWatchGroups.tsx

@@ -48,24 +48,22 @@ export const useWatchGroups = (): {
 
   React.useEffect(() => {
     if (errorAdmin) {
-      notification.error(`Group no longer exists`, errorAdmin);
+      notification.error(`Group error`, errorAdmin);
     }
     if (errorUser) {
-      notification.error(`Group no longer exists`, errorUser);
+      notification.error(`Group error`, errorUser);
     }
   }, [errorAdmin, errorUser, notification]);
 
   const updateGroups = (group: GroupsConfig) => {
     setIsLoading(true);
     updateGroupsSettings(group)
       .then((response) => {
-        if (response.success) {
-          setGroupSettings(response.success);
-          notification.success(
-            'Group settings changes saved',
-            'It may take up to 2 minutes for configuration changes to be applied.',
-          );
-        }
+        setGroupSettings(response);
+        notification.success(
+          'Group settings changes saved',
+          'It may take up to 2 minutes for configuration changes to be applied.',
+        );
       })
       .catch((error) => {
         setLoadError(error);