Fix cluster frame display issue (#5518)

* Fix cluster frame display issue

- Add some defensive code to prevent this sort of infinite loop

- Add some unit tests

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

* fix unit tests

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

* Fix build

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

* Fix lint

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

* Factor out injectable for getting of cluster config data

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

src/main/__test__/cluster.test.ts

@@ -8,11 +8,10 @@ jest.mock("request");
 jest.mock("request-promise-native");
 
 import { Console } from "console";
-import mockFs from "mock-fs";
 import type { Cluster } from "../../common/cluster/cluster";
 import { Kubectl } from "../kubectl/kubectl";
 import { getDiForUnitTesting } from "../getDiForUnitTesting";
-import type { ClusterModel } from "../../common/cluster-types";
+import type { CreateCluster } from "../../common/cluster/create-cluster-injection-token";
 import { createClusterInjectionToken } from "../../common/cluster/create-cluster-injection-token";
 import authorizationReviewInjectable from "../../common/cluster/authorization-review.injectable";
 import listNamespacesInjectable from "../../common/cluster/list-namespaces.injectable";
@@ -29,36 +28,13 @@ console = new Console(process.stdout, process.stderr); // fix mockFS
 
 describe("create clusters", () => {
   let cluster: Cluster;
-  let createCluster: (model: ClusterModel) => Cluster;
+  let createCluster: CreateCluster;
 
   beforeEach(() => {
     jest.clearAllMocks();
 
     const di = getDiForUnitTesting({ doGeneralOverrides: true });
-
-    mockFs({
-      "minikube-config.yml": JSON.stringify({
-        apiVersion: "v1",
-        clusters: [{
-          name: "minikube",
-          cluster: {
-            server: "https://192.168.64.3:8443",
-          },
-        }],
-        contexts: [{
-          context: {
-            cluster: "minikube",
-            user: "minikube",
-          },
-          name: "minikube",
-        }],
-        users: [{
-          name: "minikube",
-        }],
-        kind: "Config",
-        preferences: {},
-      }),
-    });
+    const clusterServerUrl = "https://192.168.64.3:8443";
 
     di.override(directoryForUserDataInjectable, () => "some-directory-for-user-data");
     di.override(directoryForTempInjectable, () => "some-directory-for-temp");
@@ -87,12 +63,13 @@ describe("create clusters", () => {
       id: "foo",
       contextName: "minikube",
       kubeConfigPath: "minikube-config.yml",
+    }, {
+      clusterServerUrl,
     });
   });
 
   afterEach(() => {
     cluster.disconnect();
-    mockFs.restore();
   });
 
   it("should be able to create a cluster from a cluster model and apiURL should be decoded", () => {
@@ -108,12 +85,6 @@ describe("create clusters", () => {
   });
 
   it("activating cluster should try to connect to cluster and do a refresh", async () => {
-    const cluster = createCluster({
-      id: "foo",
-      contextName: "minikube",
-      kubeConfigPath: "minikube-config.yml",
-    });
-
     jest.spyOn(cluster, "reconnect");
     jest.spyOn(cluster, "refreshConnectionStatus");
 

src/main/__test__/kube-auth-proxy.test.ts

@@ -3,8 +3,6 @@
  * Licensed under MIT License. See LICENSE in root directory for more information.
  */
 
-import type { ClusterModel } from "../../common/cluster-types";
-
 jest.mock("winston", () => ({
   format: {
     colorize: jest.fn(),
@@ -51,6 +49,7 @@ import { stdout, stderr } from "process";
 import mockFs from "mock-fs";
 import { getDiForUnitTesting } from "../getDiForUnitTesting";
 import createKubeAuthProxyInjectable from "../kube-auth-proxy/create-kube-auth-proxy.injectable";
+import type { CreateCluster } from "../../common/cluster/create-cluster-injection-token";
 import { createClusterInjectionToken } from "../../common/cluster/create-cluster-injection-token";
 import path from "path";
 import spawnInjectable from "../child-process/spawn.injectable";
@@ -67,9 +66,10 @@ console = new Console(stdout, stderr);
 const mockBroadcastIpc = broadcastMessage as jest.MockedFunction<typeof broadcastMessage>;
 const mockSpawn = spawn as jest.MockedFunction<typeof spawn>;
 const mockWaitUntilUsed = waitUntilUsed as jest.MockedFunction<typeof waitUntilUsed>;
+const clusterServerUrl = "https://192.168.64.3:8443";
 
 describe("kube auth proxy tests", () => {
-  let createCluster: (model: ClusterModel) => Cluster;
+  let createCluster: CreateCluster;
   let createKubeAuthProxy: (cluster: Cluster, environmentVariables: NodeJS.ProcessEnv) => KubeAuthProxy;
 
   beforeEach(async () => {
@@ -81,7 +81,7 @@ describe("kube auth proxy tests", () => {
         clusters: [{
           name: "minikube",
           cluster: {
-            server: "https://192.168.64.3:8443",
+            server: clusterServerUrl,
           },
         }],
         "current-context": "minikube",
@@ -130,6 +130,8 @@ describe("kube auth proxy tests", () => {
       id: "foobar",
       kubeConfigPath: "minikube-config.yml",
       contextName: "minikube",
+    }, {
+      clusterServerUrl,
     });
 
     const kap = createKubeAuthProxy(cluster, {});
@@ -222,6 +224,8 @@ describe("kube auth proxy tests", () => {
         id: "foobar",
         kubeConfigPath: "minikube-config.yml",
         contextName: "minikube",
+      }, {
+        clusterServerUrl,
       });
 
       proxy = createKubeAuthProxy(cluster, {});

src/main/__test__/kubeconfig-manager.test.ts

@@ -26,6 +26,8 @@ import kubectlDownloadingNormalizedArchInjectable from "../kubectl/normalized-ar
 
 console = new Console(process.stdout, process.stderr); // fix mockFS
 
+const clusterServerUrl = "https://192.168.64.3:8443";
+
 describe("kubeconfig manager tests", () => {
   let clusterFake: Cluster;
   let createKubeconfigManager: (cluster: Cluster) => KubeconfigManager | undefined;
@@ -57,7 +59,7 @@ describe("kubeconfig manager tests", () => {
         clusters: [{
           name: "minikube",
           cluster: {
-            server: "https://192.168.64.3:8443",
+            server: clusterServerUrl,
           },
         }],
         contexts: [{
@@ -95,6 +97,8 @@ describe("kubeconfig manager tests", () => {
       id: "foo",
       contextName: "minikube",
       kubeConfigPath: "minikube-config.yml",
+    }, {
+      clusterServerUrl,
     });
 
     jest.spyOn(KubeconfigManager.prototype, "resolveProxyUrl", "get").mockReturnValue("http://127.0.0.1:9191/foo");

src/main/catalog-sources/__test__/kubeconfig-sync.test.ts

@@ -14,7 +14,6 @@ import clusterStoreInjectable from "../../../common/cluster-store/cluster-store.
 import { getDiForUnitTesting } from "../../getDiForUnitTesting";
 import { createClusterInjectionToken } from "../../../common/cluster/create-cluster-injection-token";
 import directoryForKubeConfigsInjectable from "../../../common/app-paths/directory-for-kube-configs/directory-for-kube-configs.injectable";
-import { ClusterStore } from "../../../common/cluster-store/cluster-store";
 import getConfigurationFileModelInjectable from "../../../common/get-configuration-file-model/get-configuration-file-model.injectable";
 import appVersionInjectable from "../../../common/get-configuration-file-model/app-version/app-version.injectable";
 import clusterManagerInjectable from "../../cluster-manager.injectable";
@@ -23,6 +22,8 @@ import directoryForTempInjectable from "../../../common/app-paths/directory-for-
 import kubectlBinaryNameInjectable from "../../kubectl/binary-name.injectable";
 import kubectlDownloadingNormalizedArchInjectable from "../../kubectl/normalized-arch.injectable";
 import normalizedPlatformInjectable from "../../../common/vars/normalized-platform.injectable";
+import { iter } from "../../../common/utils";
+import fsInjectable from "../../../common/fs/fs.injectable";
 
 jest.mock("electron", () => ({
   app: {
@@ -54,10 +55,9 @@ describe("kubeconfig-sync.source tests", () => {
     di.override(kubectlDownloadingNormalizedArchInjectable, () => "amd64");
     di.override(normalizedPlatformInjectable, () => "darwin");
 
-    di.override(clusterStoreInjectable, () =>
-      ClusterStore.createInstance({ createCluster: () => null as never }),
-    );
-
+    di.permitSideEffects(fsInjectable);
+    di.unoverride(clusterStoreInjectable);
+    di.permitSideEffects(clusterStoreInjectable);
     di.permitSideEffects(getConfigurationFileModelInjectable);
     di.permitSideEffects(appVersionInjectable);
 
@@ -66,13 +66,10 @@ describe("kubeconfig-sync.source tests", () => {
       createCluster: di.inject(createClusterInjectionToken),
       clusterManager: di.inject(clusterManagerInjectable),
     });
-
-    di.inject(clusterStoreInjectable);
   });
 
   afterEach(() => {
     mockFs.restore();
-    ClusterStore.resetInstance();
   });
 
   describe("configsToModels", () => {
@@ -108,8 +105,8 @@ describe("kubeconfig-sync.source tests", () => {
       const models = configToModels(config, "/bar");
 
       expect(models.length).toBe(1);
-      expect(models[0].contextName).toBe("context-name");
-      expect(models[0].kubeConfigPath).toBe("/bar");
+      expect(models[0][0].contextName).toBe("context-name");
+      expect(models[0][0].kubeConfigPath).toBe("/bar");
     });
   });
 
@@ -160,7 +157,8 @@ describe("kubeconfig-sync.source tests", () => {
 
       expect(rootSource.size).toBe(1);
 
-      const c = rootSource.values().next().value[0] as Cluster;
+      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+      const c = (iter.first(rootSource.values())!)[0];
 
       expect(c.kubeConfigPath).toBe("/bar");
       expect(c.contextName).toBe("context-name");

src/main/catalog-sources/kubeconfig-sync/manager.ts

@@ -25,9 +25,10 @@ import { createHash } from "crypto";
 import { homedir } from "os";
 import globToRegExp from "glob-to-regexp";
 import { inspect } from "util";
-import type { ClusterModel, UpdateClusterModel } from "../../../common/cluster-types";
+import type { ClusterConfigData, UpdateClusterModel } from "../../../common/cluster-types";
 import type { Cluster } from "../../../common/cluster/cluster";
 import type { CatalogEntityRegistry } from "../../catalog/entity-registry";
+import type { CreateCluster } from "../../../common/cluster/create-cluster-injection-token";
 
 const logPrefix = "[KUBECONFIG-SYNC]:";
 
@@ -56,7 +57,7 @@ interface KubeconfigSyncManagerDependencies {
   readonly directoryForKubeConfigs: string;
   readonly entityRegistry: CatalogEntityRegistry;
   readonly clusterManager: ClusterManager;
-  createCluster: (model: ClusterModel) => Cluster;
+  createCluster: CreateCluster;
 }
 
 const kubeConfigSyncName = "lens:kube-sync";
@@ -147,17 +148,22 @@ export class KubeconfigSyncManager {
 }
 
 // exported for testing
-export function configToModels(rootConfig: KubeConfig, filePath: string): UpdateClusterModel[] {
-  const validConfigs = [];
+export function configToModels(rootConfig: KubeConfig, filePath: string): [UpdateClusterModel, ClusterConfigData][] {
+  const validConfigs: ReturnType<typeof configToModels> = [];
 
-  for (const { config, error } of splitConfig(rootConfig)) {
-    if (error) {
-      logger.debug(`${logPrefix} context failed validation: ${error}`, { context: config.currentContext, filePath });
+  for (const { config, validationResult } of splitConfig(rootConfig)) {
+    if (validationResult.error) {
+      logger.debug(`${logPrefix} context failed validation: ${validationResult.error}`, { context: config.currentContext, filePath });
     } else {
-      validConfigs.push({
-        kubeConfigPath: filePath,
-        contextName: config.currentContext,
-      });
+      validConfigs.push([
+        {
+          kubeConfigPath: filePath,
+          contextName: config.currentContext,
+        },
+        {
+          clusterServerUrl: validationResult.cluster.server,
+        },
+      ]);
     }
   }
 
@@ -169,7 +175,7 @@ type RootSource = ObservableMap<string, RootSourceValue>;
 
 interface ComputeDiffDependencies {
   directoryForKubeConfigs: string;
-  createCluster: (model: ClusterModel) => Cluster;
+  createCluster: CreateCluster;
   clusterManager: ClusterManager;
 }
 
@@ -184,15 +190,15 @@ export const computeDiff = ({ directoryForKubeConfigs, createCluster, clusterMan
       }
 
       const rawModels = configToModels(config, filePath);
-      const models = new Map(rawModels.map(m => [m.contextName, m]));
+      const models = new Map(rawModels.map(([model, configData]) => [model.contextName, [model, configData] as const]));
 
       logger.debug(`${logPrefix} File now has ${models.size} entries`, { filePath });
 
       for (const [contextName, value] of source) {
-        const model = models.get(contextName);
+        const data = models.get(contextName);
 
         // remove and disconnect clusters that were removed from the config
-        if (!model) {
+        if (!data) {
           // remove from the deleting set, so that if a new context of the same name is added, it isn't marked as deleting
           clusterManager.deleting.delete(value[0].id);
 
@@ -207,17 +213,17 @@ export const computeDiff = ({ directoryForKubeConfigs, createCluster, clusterMan
         // diff against that
 
         // or update the model and mark it as not needed to be added
-        value[0].updateModel(model);
+        value[0].updateModel(data[0]);
         models.delete(contextName);
         logger.debug(`${logPrefix} Updated old cluster from sync`, { filePath, contextName });
       }
 
-      for (const [contextName, model] of models) {
+      for (const [contextName, [model, configData]] of models) {
         // add new clusters to the source
         try {
           const clusterId = createHash("md5").update(`${filePath}:${contextName}`).digest("hex");
 
-          const cluster = ClusterStore.getInstance().getById(clusterId) || createCluster({ ...model, id: clusterId });
+          const cluster = ClusterStore.getInstance().getById(clusterId) || createCluster({ ...model, id: clusterId }, configData);
 
           if (!cluster.apiUrl) {
             throw new Error("Cluster constructor failed, see above error");

src/main/create-cluster/create-cluster.injectable.ts

@@ -32,7 +32,7 @@ const createClusterInjectable = getInjectable({
       createVersionDetector: di.inject(createVersionDetectorInjectable),
     };
 
-    return (model) => new Cluster(dependencies, model);
+    return (model, configData) => new Cluster(dependencies, model, configData);
   },
 
   injectionToken: createClusterInjectionToken,