Correctly catch and notify user about NodeShell pod failures (#4244)

* Correctly catch and notify user about NodeShell pod failures

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* Fix error throwing sometimes during Terminal.detach

Signed-off-by: Sebastian Malton <sebastian@malton.name>

src/main/lens-proxy.ts

@@ -60,12 +60,10 @@ export class LensProxy extends Singleton {
 
   public port: number;
 
-  constructor(protected router: Router, functions: LensProxyFunctions) {
+  constructor(protected router: Router, { shellApiRequest, kubeApiRequest, getClusterForRequest }: LensProxyFunctions) {
     super();
 
-    const { shellApiRequest, kubeApiRequest } = functions;
-
-    this.getClusterForRequest = functions.getClusterForRequest;
+    this.getClusterForRequest = getClusterForRequest;
 
     this.proxyServer = spdy.createServer({
       spdy: {
@@ -79,9 +77,15 @@ export class LensProxy extends Singleton {
     this.proxyServer
       .on("upgrade", (req: http.IncomingMessage, socket: net.Socket, head: Buffer) => {
         const isInternal = req.url.startsWith(`${apiPrefix}?`);
+        const cluster = getClusterForRequest(req);
+
+        if (!cluster) {
+          return void logger.error(`[LENS-PROXY]: Could not find cluster for upgrade request from url=${req.url}`);
+        }
+
         const reqHandler = isInternal ? shellApiRequest : kubeApiRequest;
 
-        (async () => reqHandler({ req, socket, head }))()
+        (async () => reqHandler({ req, socket, head, cluster }))()
           .catch(error => logger.error("[LENS-PROXY]: failed to handle proxy upgrade", error));
       });
   }

src/main/proxy-functions/kube-api-request.ts

@@ -23,18 +23,11 @@ import { chunk } from "lodash";
 import net from "net";
 import url from "url";
 import { apiKubePrefix } from "../../common/vars";
-import { ClusterManager } from "../cluster-manager";
 import type { ProxyApiRequestArgs } from "./types";
 
 const skipRawHeaders = new Set(["Host", "Authorization"]);
 
-export async function kubeApiRequest({ req, socket, head }: ProxyApiRequestArgs) {
-  const cluster = ClusterManager.getInstance().getClusterForRequest(req);
-
-  if (!cluster) {
-    return;
-  }
-
+export async function kubeApiRequest({ req, socket, head, cluster }: ProxyApiRequestArgs) {
   const proxyUrl = await cluster.contextHandler.resolveAuthProxyUrl() + req.url.replace(apiKubePrefix, "");
   const apiUrl = url.parse(cluster.apiUrl);
   const pUrl = url.parse(proxyUrl);

src/main/proxy-functions/types.ts

@@ -21,9 +21,11 @@
 
 import type http from "http";
 import type net from "net";
+import type { Cluster } from "../cluster";
 
 export interface ProxyApiRequestArgs {
   req: http.IncomingMessage,
   socket: net.Socket,
   head: Buffer,
+  cluster: Cluster,
 }

src/main/shell-session/node-shell-session.ts

@@ -33,12 +33,10 @@ import logger from "../logger";
 export class NodeShellSession extends ShellSession {
   ShellType = "node-shell";
 
-  protected podId = `node-shell-${uuid()}`;
+  protected readonly podName = `node-shell-${uuid()}`;
   protected kc: KubeConfig;
 
-  protected get cwd(): string | undefined {
-    return undefined;
-  }
+  protected readonly cwd: string | undefined = undefined;
 
   constructor(socket: WebSocket, cluster: Cluster, protected nodeName: string, terminalId: string) {
     super(socket, cluster, terminalId);
@@ -59,7 +57,7 @@ export class NodeShellSession extends ShellSession {
     }
 
     const env = await this.getCachedShellEnv();
-    const args = ["exec", "-i", "-t", "-n", "kube-system", this.podId, "--"];
+    const args = ["exec", "-i", "-t", "-n", "kube-system", this.podName, "--"];
     const nodeApi = new NodesApi({
       objectConstructor: Node,
       request: KubeJsonApi.forCluster(this.cluster),
@@ -93,7 +91,7 @@ export class NodeShellSession extends ShellSession {
       .makeApiClient(k8s.CoreV1Api)
       .createNamespacedPod("kube-system", {
         metadata: {
-          name: this.podId,
+          name: this.podName,
           namespace: "kube-system",
         },
         spec: {
@@ -121,33 +119,39 @@ export class NodeShellSession extends ShellSession {
   }
 
   protected waitForRunningPod(): Promise<void> {
-    return new Promise((resolve, reject) => {
-      const watch = new k8s.Watch(this.kc);
+    logger.debug(`[NODE-SHELL]: waiting for ${this.podName} to be running`);
 
-      watch
+    return new Promise((resolve, reject) => {
+      new k8s.Watch(this.kc)
         .watch(`/api/v1/namespaces/kube-system/pods`,
           {},
           // callback is called for each received object.
-          (type, obj) => {
-            if (obj.metadata.name == this.podId && obj.status.phase === "Running") {
-              resolve();
+          (type, { metadata: { name }, status }) => {
+            if (name === this.podName) {
+              switch (status.phase) {
+                case "Running":
+                  return resolve();
+                case "Failed":
+                  return reject(`Failed to be created: ${status.message || "unknown error"}`);
+              }
             }
           },
           // done callback is called if the watch terminates normally
           (err) => {
-            console.log(err);
+            logger.error(`[NODE-SHELL]: ${this.podName} was not created in time`);
             reject(err);
           },
         )
         .then(req => {
           setTimeout(() => {
-            console.log("aborting");
+            logger.error(`[NODE-SHELL]: aborting wait for ${this.podName}, timing out`);
             req.abort();
-          }, 2 * 60 * 1000);
+            reject("Pod creation timed out");
+          }, 2 * 60 * 1000); // 2 * 60 * 1000
         })
-        .catch(err => {
-          console.log("watch failed");
-          reject(err);
+        .catch(error => {
+          logger.error(`[NODE-SHELL]: waiting for ${this.podName} failed: ${error}`);
+          reject(error);
         });
     });
   }
@@ -161,6 +165,7 @@ export class NodeShellSession extends ShellSession {
     this
       .kc
       .makeApiClient(k8s.CoreV1Api)
-      .deleteNamespacedPod(this.podId, "kube-system");
+      .deleteNamespacedPod(this.podName, "kube-system")
+      .catch(error => logger.warn(`[NODE-SHELL]: failed to remove pod shell`, error));
   }
 }

src/renderer/api/websocket-api.ts

@@ -37,11 +37,11 @@ interface IMessage {
 }
 
 export enum WebSocketApiState {
-  PENDING = -1,
-  OPEN,
-  CONNECTING,
-  RECONNECTING,
-  CLOSED,
+  PENDING = "pending",
+  OPEN = "open",
+  CONNECTING = "connecting",
+  RECONNECTING = "reconnecting",
+  CLOSED = "closed",
 }
 
 export class WebSocketApi {

src/renderer/components/dock/terminal-window.tsx

@@ -53,7 +53,7 @@ export class TerminalWindow extends React.Component<Props> {
   }
 
   activate(tabId: TabId) {
-    if (this.terminal) this.terminal.detach(); // detach previous
+    this.terminal?.detach(); // detach previous
     this.terminal = TerminalStore.getInstance().getTerminal(tabId);
     this.terminal.attachTo(this.elem);
   }

src/renderer/components/dock/terminal.ts

@@ -26,16 +26,15 @@ import { FitAddon } from "xterm-addon-fit";
 import { dockStore, TabId } from "./dock.store";
 import type { TerminalApi } from "../../api/terminal-api";
 import { ThemeStore } from "../../theme.store";
-import { boundMethod } from "../../utils";
+import { boundMethod, disposer } from "../../utils";
 import { isMac } from "../../../common/vars";
 import { camelCase } from "lodash";
 import { UserStore } from "../../../common/user-store";
 import { clipboard } from "electron";
+import logger from "../../../common/logger";
 
 export class Terminal {
-  static spawningPool: HTMLElement;
-
-  static init() {
+  public static readonly spawningPool = (() => {
     // terminal element must be in DOM before attaching via xterm.open(elem)
     // https://xtermjs.org/docs/api/terminal/classes/terminal/#open
     const pool = document.createElement("div");
@@ -43,8 +42,9 @@ export class Terminal {
     pool.className = "terminal-init";
     pool.style.cssText = "position: absolute; top: 0; left: 0; height: 0; visibility: hidden; overflow: hidden";
     document.body.appendChild(pool);
-    Terminal.spawningPool = pool;
-  }
+
+    return pool;
+  })();
 
   static async preloadFonts() {
     const fontPath = require("../fonts/roboto-mono-nerd.ttf").default; // eslint-disable-line @typescript-eslint/no-var-requires
@@ -54,13 +54,22 @@ export class Terminal {
     document.fonts.add(fontFace);
   }
 
-  public xterm: XTerm;
-  public fitAddon: FitAddon;
-  public scrollPos = 0;
-  public disposers: Function[] = [];
+  private xterm: XTerm | null = new XTerm({
+    cursorBlink: true,
+    cursorStyle: "bar",
+    fontSize: 13,
+    fontFamily: "RobotoMono",
+  });
+  private readonly fitAddon = new FitAddon();
+  private scrollPos = 0;
+  private disposer = disposer();
 
   @boundMethod
   protected setTheme(colors: Record<string, string>) {
+    if (!this.xterm) {
+      return;
+    }
+
     // Replacing keys stored in styles to format accepted by terminal
     // E.g. terminalBrightBlack -> brightBlack
     const colorPrefix = "terminal";
@@ -73,17 +82,13 @@ export class Terminal {
   }
 
   get elem() {
-    return this.xterm.element;
+    return this.xterm?.element;
   }
 
   get viewport() {
     return this.xterm.element.querySelector(".xterm-viewport");
   }
 
-  constructor(public tabId: TabId, protected api: TerminalApi) {
-    this.init();
-  }
-
   get isActive() {
     const { isOpen, selectedTabId } = dockStore;
 
@@ -96,22 +101,15 @@ export class Terminal {
   }
 
   detach() {
-    Terminal.spawningPool.appendChild(this.elem);
+    const { elem } = this;
+
+    if (elem) {
+      Terminal.spawningPool.appendChild(elem);
+    }
   }
 
-  async init() {
-    if (this.xterm) {
-      return;
-    }
-    this.xterm = new XTerm({
-      cursorBlink: true,
-      cursorStyle: "bar",
-      fontSize: 13,
-      fontFamily: "RobotoMono",
-    });
-
+  constructor(public tabId: TabId, protected api: TerminalApi) {
     // enable terminal addons
-    this.fitAddon = new FitAddon();
     this.xterm.loadAddon(this.fitAddon);
 
     this.xterm.open(Terminal.spawningPool);
@@ -128,7 +126,7 @@ export class Terminal {
     this.api.onData.addListener(this.onApiData);
     window.addEventListener("resize", this.onResize);
 
-    this.disposers.push(
+    this.disposer.push(
       reaction(() => ThemeStore.getInstance().activeTheme.colors, this.setTheme, {
         fireImmediately: true,
       }),
@@ -142,16 +140,18 @@ export class Terminal {
   }
 
   destroy() {
-    if (!this.xterm) return;
-    this.disposers.forEach(dispose => dispose());
-    this.disposers = [];
-    this.xterm.dispose();
-    this.xterm = null;
+    if (this.xterm) {
+      this.disposer();
+      this.xterm.dispose();
+      this.xterm = null;
+    }
   }
 
   fit = () => {
     // Since this function is debounced we need to read this value as late as possible
-    if (!this.isActive) return;
+    if (!this.isActive || !this.xterm) {
+      return;
+    }
 
     try {
       this.fitAddon.fit();
@@ -159,9 +159,8 @@ export class Terminal {
 
       this.api.sendTerminalSize(cols, rows);
     } catch (error) {
-      console.error(error);
-
-      return; // see https://github.com/lensapp/lens/issues/1891
+      // see https://github.com/lensapp/lens/issues/1891
+      logger.error(`[TERMINAL]: failed to resize terminal to fit`, error);
     }
   };
 
@@ -204,19 +203,21 @@ export class Terminal {
   };
 
   onContextMenu = () => {
-    const { terminalCopyOnSelect } = UserStore.getInstance();
-    const textFromClipboard = clipboard.readText();
+    if (
+      // don't paste if user hasn't turned on the feature
+      UserStore.getInstance().terminalCopyOnSelect
 
-    if (terminalCopyOnSelect) {
-      this.xterm.paste(textFromClipboard);
+      // don't paste if the clipboard doesn't have text
+      && clipboard.availableFormats().includes("text/plain")
+    ) {
+      this.xterm.paste(clipboard.readText());
     }
   };
 
   onSelectionChange = () => {
-    const { terminalCopyOnSelect } = UserStore.getInstance();
     const selection = this.xterm.getSelection().trim();
 
-    if (terminalCopyOnSelect && selection !== "") {
+    if (UserStore.getInstance().terminalCopyOnSelect && selection) {
       clipboard.writeText(selection);
     }
   };
@@ -251,5 +252,3 @@ export class Terminal {
     return true;
   };
 }
-
-Terminal.init();