Merge pull request #7340 from mook-as/path-management/diagnostics

Pick Diagnostics: path management onto release-1.15

Author: Nino-K

pkg/rancher-desktop/assets/specs/command-api.yaml

@@ -101,7 +101,7 @@ paths:
       summary: >-
         Return a list of the check IDs for the Diagnostics category,
         or 404 if there is no such `category`.
-        Specifying an exiting category with no checks
+        Specifying an existing category with no checks
         will return status code 200 and an empty array.
       parameters:
       - in: query

pkg/rancher-desktop/integrations/manageLinesInFile.ts

@@ -6,11 +6,60 @@ export const START_LINE = '### MANAGED BY RANCHER DESKTOP START (DO NOT EDIT)';
 export const END_LINE = '### MANAGED BY RANCHER DESKTOP END (DO NOT EDIT)';
 const DEFAULT_FILE_MODE = 0o644;
 
+/**
+ * `newErrorWithPath` returns a dynamically constructed subclass of `Error` that
+ * has a constructor that constructs a message using the `messageTemplate`
+ * function, and also sets any inputs to the function as properties on the
+ * resulting object.
+ * @param messageTemplate A function used to generate an error message, based on
+ *                        any arguments passed in as properties of an object.
+ * @returns A subclass of Error.
+ */
+function newErrorWithPath<T extends Record<string, any>>(messageTemplate: (input: T) => string) {
+  const result = class extends Error {
+    constructor(input: T, options?: ErrorOptions) {
+      super(messageTemplate(input), options);
+      Object.assign(this, input);
+    }
+  };
+
+  return result as unknown as new(...args: ConstructorParameters<typeof result>) => (InstanceType<typeof result> & T);
+}
+
+/**
+ * `ErrorDeterminingExtendedAttributes` signifies that we failed to determine if
+ * the given path contains extended attributes; to be safe, we are not managing
+ * this file.
+ */
+export const ErrorDeterminingExtendedAttributes =
+  newErrorWithPath(({ path }: {path: string}) => `Failed to determine if \`${ path }\` contains extended attributes`);
+/**
+ * `ErrorHasExtendedAttributes` signifies that we were unable to process a file
+ * because it has extended attributes that would not have been preserved had we
+ * tried to edit it.
+ */
+export const ErrorHasExtendedAttributes =
+  newErrorWithPath(({ path }: {path: string}) => `Refusing to manage \`${ path }\` which has extended attributes`);
+/**
+ * `ErrorNotRegularFile` signifies that we were unable to process a file because
+ * it is not a regular file (e.g. a named pipe or a device).
+ */
+export const ErrorNotRegularFile =
+  newErrorWithPath(({ path }: {path: string}) => `Refusing to manage \`${ path }\` which is neither a regular file nor a symbolic link`);
+/**
+ * `ErrorWritingFile` signifies that we attempted to process a file but writing
+ * to it resulted in unexpected contents.
+ */
+export const ErrorWritingFile =
+  newErrorWithPath(({ path, backupPath }: {path: string, backupPath: string}) => `Error writing to \`${ path }\`: written contents are unexpected; see backup in \`${ backupPath }\``);
+
 /**
  * Inserts/removes fenced lines into/from a file. Idempotent.
  * @param path The path to the file to work on.
  * @param desiredManagedLines The lines to insert into the file.
  * @param desiredPresent Whether the lines should be present.
+ * @throws If the file could not be managed; for example, if it has extended
+ *         attributes, is not a regular file, or a backup exists.
  */
 export default async function manageLinesInFile(path: string, desiredManagedLines: string[], desiredPresent: boolean): Promise<void> {
   const desired = getDesiredLines(desiredManagedLines, desiredPresent);
@@ -35,7 +84,7 @@ export default async function manageLinesInFile(path: string, desiredManagedLine
 
   if (fileStats.isFile()) {
     if (await fileHasExtendedAttributes(path)) {
-      throw new Error(`Refusing to manage ${ path } which has extended attributes`);
+      throw new ErrorHasExtendedAttributes({ path });
     }
 
     const tempName = `${ path }.rd-temp`;
@@ -88,13 +137,13 @@ export default async function manageLinesInFile(path: string, desiredManagedLine
     const actualContents = await fs.promises.readFile(path, 'utf-8');
 
     if (!isEqual(targetContents, actualContents)) {
-      throw new Error(`Error writing to ${ path }: written contents are unexpected; see backup in ${ backupPath }`);
+      throw new ErrorWritingFile({ path, backupPath });
     }
     await fs.promises.unlink(backupPath);
   } else {
     // Target exists, and is neither a normal file nor a symbolic link.
     // Return with an error.
-    throw new Error(`Refusing to manage ${ path } which is neither a regular file nor a symbolic link`);
+    throw new ErrorNotRegularFile({ path });
   }
 }
 
@@ -112,15 +161,15 @@ async function fileHasExtendedAttributes(filePath: string): Promise<boolean> {
     const { list } = await import('fs-xattr');
 
     return (await list(filePath)).length > 0;
-  } catch {
+  } catch (cause) {
     if (process.env.NODE_ENV === 'test' && process.env.RD_TEST !== 'e2e') {
       // When running unit tests, assume they do not have extended attributes.
       return false;
     }
 
-    console.error(`Failed to import fs-xattr, cannot check for extended attributes on ${ filePath }; assuming it exists.`);
+    console.error(`Failed to import fs-xattr, cannot check for extended attributes on ${ filePath }`);
 
-    return true;
+    throw new ErrorDeterminingExtendedAttributes({ path: filePath }, { cause });
   }
 }
 

pkg/rancher-desktop/integrations/pathManager.ts

@@ -7,9 +7,15 @@
 export interface PathManager {
   /** The PathManagementStrategy that corresponds to the implementation. */
   readonly strategy: PathManagementStrategy
-  /** Makes real any changes to the system. Should be idempotent. */
+  /**
+   * Applies changes to the system. Should be idempotent, and should not throw
+   * any exceptions.
+   */
   enforce(): Promise<void>
-  /** Removes any changes that the PathManager may have made. Should be idempotent. */
+  /**
+   * Removes any changes that the PathManager may have made. Should be
+   * idempotent, and should not throw any exceptions.
+   */
   remove(): Promise<void>
 }
 

pkg/rancher-desktop/integrations/pathManagerImpl.ts

@@ -7,8 +7,11 @@ import { Mutex } from 'async-mutex';
 import manageLinesInFile from '@pkg/integrations/manageLinesInFile';
 import { ManualPathManager, PathManagementStrategy, PathManager } from '@pkg/integrations/pathManager';
 import mainEvents from '@pkg/main/mainEvents';
+import Logging from '@pkg/utils/logging';
 import paths from '@pkg/utils/paths';
 
+const console = Logging['path-management'];
+
 /**
  * RcFilePathManager is for when the user wants Rancher Desktop to
  * make changes to their PATH by putting lines that change it in their
@@ -32,15 +35,36 @@ export class RcFilePathManager implements PathManager {
   }
 
   async enforce(): Promise<void> {
-    await this.managePosix(true);
-    await this.manageCsh(true);
-    await this.manageFish(true);
+    try {
+      await this.managePosix(true);
+      await this.manageCsh(true);
+      await this.manageFish(true);
+    } catch (error) {
+      console.error(error);
+    }
   }
 
   async remove(): Promise<void> {
-    await this.managePosix(false);
-    await this.manageCsh(false);
-    await this.manageFish(false);
+    try {
+      await this.managePosix(false);
+      await this.manageCsh(false);
+      await this.manageFish(false);
+    } catch (error) {
+      console.error(error);
+    }
+  }
+
+  /**
+   * Call manageFilesInLine, wrapped in calls to trigger diagnostics updates.
+   */
+  protected async manageLinesInFile(fileName: string, filePath: string, lines: string[], desiredPresent: boolean) {
+    try {
+      await manageLinesInFile(filePath, lines, desiredPresent);
+      mainEvents.emit('diagnostics-event', 'path-management', { fileName, error: undefined });
+    } catch (error: any) {
+      mainEvents.emit('diagnostics-event', 'path-management', { fileName, error });
+      throw error;
+    }
   }
 
   /**
@@ -51,7 +75,8 @@ export class RcFilePathManager implements PathManager {
   protected async managePosix(desiredPresent: boolean): Promise<void> {
     await this.posixMutex.runExclusive(async() => {
       const pathLine = `export PATH="${ paths.integration }:$PATH"`;
-      // Note: order is important here. Only the first one that is present is modified.
+      // Note: order is important here.  Only the first one has the PATH added;
+      // all others have it removed.
       const bashLoginShellFiles = [
         '.bash_profile',
         '.bash_login',
@@ -70,35 +95,38 @@ export class RcFilePathManager implements PathManager {
             await fs.promises.stat(filePath);
           } catch (error: any) {
             if (error.code === 'ENOENT') {
+              // If the file does not exist, it is not an error.
+              mainEvents.emit('diagnostics-event', 'path-management', { fileName, error: undefined });
               continue;
             }
+            mainEvents.emit('diagnostics-event', 'path-management', { fileName, error });
             throw error;
           }
-          await manageLinesInFile(filePath, [pathLine], desiredPresent);
+          await this.manageLinesInFile(fileName, filePath, [pathLine], !linesAdded);
           linesAdded = true;
-          break;
         }
 
         // If none of the files exist, write .bash_profile
         if (!linesAdded) {
-          const filePath = path.join(os.homedir(), bashLoginShellFiles[0]);
+          const fileName = bashLoginShellFiles[0];
+          const filePath = path.join(os.homedir(), fileName);
 
-          await manageLinesInFile(filePath, [pathLine], desiredPresent);
+          await this.manageLinesInFile(fileName, filePath, [pathLine], true);
         }
       } else {
         // Ensure lines are not present in any of the files
-        await Promise.all(bashLoginShellFiles.map((fileName) => {
+        await Promise.all(bashLoginShellFiles.map(async(fileName) => {
           const filePath = path.join(os.homedir(), fileName);
 
-          return manageLinesInFile(filePath, [], desiredPresent);
+          await this.manageLinesInFile(fileName, filePath, [], false);
         }));
       }
 
       // Handle other shells' rc files and .bashrc
-      await Promise.all(['.bashrc', '.zshrc'].map((rcName) => {
-        const rcPath = path.join(os.homedir(), rcName);
+      await Promise.all(['.bashrc', '.zshrc'].map((fileName) => {
+        const rcPath = path.join(os.homedir(), fileName);
 
-        return manageLinesInFile(rcPath, [pathLine], desiredPresent);
+        return this.manageLinesInFile(fileName, rcPath, [pathLine], desiredPresent);
       }));
 
       mainEvents.invoke('diagnostics-trigger', 'RD_BIN_IN_BASH_PATH');
@@ -110,10 +138,10 @@ export class RcFilePathManager implements PathManager {
     await this.cshMutex.runExclusive(async() => {
       const pathLine = `setenv PATH "${ paths.integration }"\\:"$PATH"`;
 
-      await Promise.all(['.cshrc', '.tcshrc'].map((rcName) => {
-        const rcPath = path.join(os.homedir(), rcName);
+      await Promise.all(['.cshrc', '.tcshrc'].map((fileName) => {
+        const rcPath = path.join(os.homedir(), fileName);
 
-        return manageLinesInFile(rcPath, [pathLine], desiredPresent);
+        return this.manageLinesInFile(fileName, rcPath, [pathLine], desiredPresent);
       }));
     });
   }
@@ -122,11 +150,12 @@ export class RcFilePathManager implements PathManager {
     await this.fishMutex.runExclusive(async() => {
       const pathLine = `set --export --prepend PATH "${ paths.integration }"`;
       const configHome = process.env['XDG_CONFIG_HOME'] || path.join(os.homedir(), '.config');
+      const fileName = 'config.fish';
       const fishConfigDir = path.join(configHome, 'fish');
-      const fishConfigPath = path.join(fishConfigDir, 'config.fish');
+      const fishConfigPath = path.join(fishConfigDir, fileName);
 
       await fs.promises.mkdir(fishConfigDir, { recursive: true, mode: 0o700 });
-      await manageLinesInFile(fishConfigPath, [pathLine], desiredPresent);
+      await this.manageLinesInFile(fileName, fishConfigPath, [pathLine], desiredPresent);
     });
   }
 }

pkg/rancher-desktop/main/diagnostics/diagnostics.ts

@@ -1,4 +1,4 @@
-import { DiagnosticsCategory, DiagnosticsChecker, DiagnosticsCheckerResult } from './types';
+import { DiagnosticsCategory, DiagnosticsChecker, DiagnosticsCheckerResult, DiagnosticsCheckerSingleResult } from './types';
 
 import mainEvents from '@pkg/main/mainEvents';
 import Logging from '@pkg/utils/logging';
@@ -38,7 +38,7 @@ export class DiagnosticsManager {
   lastUpdate = new Date(0);
 
   /** Last known check results, indexed by the checker id. */
-  results: Record<DiagnosticsChecker['id'], DiagnosticsCheckerResult> = {};
+  results: Record<DiagnosticsChecker['id'], DiagnosticsCheckerResult|DiagnosticsCheckerSingleResult[]> = {};
 
   /** Mapping of category name to diagnostic ids */
   readonly checkerIdByCategory: Partial<Record<DiagnosticsCategory, string[]>> = {};
@@ -52,6 +52,7 @@ export class DiagnosticsManager {
         import('./kubeContext'),
         import('./limaDarwin'),
         import('./mockForScreenshots'),
+        import('./pathManagement'),
         import('./rdBinInShell'),
         import('./testCheckers'),
         import('./wslFromStore'),
@@ -92,9 +93,10 @@ export class DiagnosticsManager {
   }
 
   protected async applicableCheckers(categoryName: string | null, id: string | null): Promise<DiagnosticsChecker[]> {
+    const checkerId = id?.split(':', 1)[0];
     const checkers = (await this.checkers)
       .filter(checker => categoryName ? checker.category === categoryName : true)
-      .filter(checker => id ? checker.id === id : true);
+      .filter(checker => checkerId ? checker.id === checkerId : true);
 
     return (await Promise.all(checkers.map(async(checker) => {
       try {
@@ -124,12 +126,25 @@ export class DiagnosticsManager {
     return {
       last_update: this.lastUpdate.toISOString(),
       checks:      checkers
-        .map(checker => ({
-          ...this.results[checker.id],
-          id:       checker.id,
-          category: checker.category,
-          mute:     false,
-        })),
+        .flatMap((checker) => {
+          const result = this.results[checker.id];
+
+          if (Array.isArray(result)) {
+            return result.map(result => ({
+              ...result,
+              id:       `${ checker.id }:${ result.id }`,
+              category: checker.category,
+              mute:     false,
+            }));
+          } else {
+            return {
+              ...result,
+              id:       checker.id,
+              category: checker.category,
+              mute:     false,
+            };
+          }
+        }),
     };
   }
 
@@ -139,8 +154,16 @@ export class DiagnosticsManager {
   protected async runChecker(checker: DiagnosticsChecker) {
     console.debug(`Running check ${ checker.id }`);
     try {
-      this.results[checker.id] = await checker.check();
-      console.debug(`Check ${ checker.id } result: ${ JSON.stringify(this.results[checker.id]) }`);
+      const result = await checker.check();
+
+      this.results[checker.id] = result;
+      if (Array.isArray(result)) {
+        for (const singleResult of result) {
+          console.debug(`Check ${ checker.id }:${ singleResult.id } result: ${ JSON.stringify(singleResult) }`);
+        }
+      } else {
+        console.debug(`Check ${ checker.id } result: ${ JSON.stringify(result) }`);
+      }
     } catch (e) {
       console.error(`ERROR checking ${ checker.id }`, { e });
     }

pkg/rancher-desktop/main/diagnostics/kubeConfigSymlink.ts

@@ -27,7 +27,7 @@ const CheckKubeConfigSymlink: DiagnosticsChecker = {
   id:       'VERIFY_WSL_INTEGRATION_KUBECONFIG',
   category: DiagnosticsCategory.Kubernetes,
   applicable() {
-    return Promise.resolve(true);
+    return Promise.resolve(process.platform === 'win32');
   },
   async check() {
     return Promise.resolve({