From fb0b2f01098cacb986a3ff31ffb602a88cc46eec Mon Sep 17 00:00:00 2001 From: Luca Stocchi <49404737+lstocchi@users.noreply.github.com> Date: Tue, 7 May 2024 09:23:09 +0200 Subject: [PATCH] fix: stop informer not more valid and handle similar contexts (#6934) * fix: stop informer not more valid and handle similar contexts Signed-off-by: lstocchi * fix: add tests Signed-off-by: lstocchi * Update packages/main/src/plugin/kubernetes-context-state.spec.ts Co-authored-by: axel7083 <42176370+axel7083@users.noreply.github.com> Signed-off-by: Luca Stocchi <49404737+lstocchi@users.noreply.github.com> * Update packages/main/src/plugin/kubernetes-context-state.spec.ts Co-authored-by: axel7083 <42176370+axel7083@users.noreply.github.com> Signed-off-by: Luca Stocchi <49404737+lstocchi@users.noreply.github.com> * fix: remove undefined currentContext check on context changed Signed-off-by: lstocchi --------- Signed-off-by: lstocchi Signed-off-by: Luca Stocchi <49404737+lstocchi@users.noreply.github.com> Co-authored-by: axel7083 <42176370+axel7083@users.noreply.github.com> --- .../plugin/kubernetes-context-state.spec.ts | 538 +++++++++++++++++- .../src/plugin/kubernetes-context-state.ts | 106 +++- 2 files changed, 611 insertions(+), 33 deletions(-) diff --git a/packages/main/src/plugin/kubernetes-context-state.spec.ts b/packages/main/src/plugin/kubernetes-context-state.spec.ts index 2edc10725f9..3f8064def8d 100644 --- a/packages/main/src/plugin/kubernetes-context-state.spec.ts +++ b/packages/main/src/plugin/kubernetes-context-state.spec.ts @@ -19,9 +19,10 @@ import type { ErrorCallback, KubernetesObject, ObjectCallback } from '@kubernetes/client-node'; import * as kubeclient from '@kubernetes/client-node'; import { KubeConfig, makeInformer } from '@kubernetes/client-node'; -import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; +import { afterEach, beforeAll, beforeEach, describe, expect, test, vi } from 'vitest'; import type { ApiSenderType } from './api.js'; +import type { KubeContext } from './kubernetes-context.js'; import type { ContextGeneralState, ResourceName } from './kubernetes-context-state.js'; import { ContextsManager, ContextsStates } from './kubernetes-context-state.js'; @@ -307,6 +308,7 @@ describe('update', async () => { apiSenderSendMock.mockReset(); await client.update(kubeConfig); + vi.advanceTimersToNextTimer(); vi.advanceTimersToNextTimer(); expect(apiSenderSendMock).toHaveBeenCalledWith('kubernetes-contexts-general-state-update', expectedMap); expect(apiSenderSendMock).toHaveBeenCalledWith('kubernetes-current-context-general-state-update', { @@ -357,6 +359,7 @@ describe('update', async () => { }, } as ContextGeneralState); + vi.advanceTimersToNextTimer(); vi.advanceTimersToNextTimer(); expect(apiSenderSendMock).toHaveBeenCalledWith('kubernetes-contexts-general-state-update', expectedMap); expect(apiSenderSendMock).toHaveBeenCalledWith('kubernetes-current-context-general-state-update', { @@ -1233,8 +1236,10 @@ describe('update', async () => { await client.update(kubeConfig); - expect(informerStopMock).toHaveBeenCalledTimes(1); - expect(informerStopMock).toHaveBeenCalledWith('context1', informerPath); + expect(informerStopMock).toHaveBeenCalledTimes(3); + expect(informerStopMock).toHaveBeenNthCalledWith(1, 'context2', '/api/v1/namespaces/ns2/pods'); + expect(informerStopMock).toHaveBeenNthCalledWith(2, 'context2', '/apis/apps/v1/namespaces/ns2/deployments'); + expect(informerStopMock).toHaveBeenNthCalledWith(3, 'context1', informerPath); expect(client.getContextResources('context1', resource as ResourceName).length).toBe(0); }); }); @@ -1306,10 +1311,25 @@ describe('update', async () => { await client.update(kubeConfig); - expect(informerStopMock).toHaveBeenCalledTimes(1); - expect(informerStopMock).toHaveBeenCalledWith('context1', '/api/v1/namespaces/ns1/services'); - expect(makeInformerMock).toHaveBeenCalledTimes(1); - expect(makeInformerMock).toHaveBeenCalledWith( + expect(informerStopMock).toHaveBeenCalledTimes(3); + expect(informerStopMock).toHaveBeenNthCalledWith(1, 'context2', '/api/v1/namespaces/ns2/pods'); + expect(informerStopMock).toHaveBeenNthCalledWith(2, 'context2', '/apis/apps/v1/namespaces/ns2/deployments'); + expect(informerStopMock).toHaveBeenNthCalledWith(3, 'context1', '/api/v1/namespaces/ns1/services'); + expect(makeInformerMock).toHaveBeenCalledTimes(3); + expect(makeInformerMock).toHaveBeenNthCalledWith( + 1, + expect.any(KubeConfig), + '/api/v1/namespaces/ns2/pods', + expect.anything(), + ); + expect(makeInformerMock).toHaveBeenNthCalledWith( + 2, + expect.any(KubeConfig), + '/apis/apps/v1/namespaces/ns2/deployments', + expect.anything(), + ); + expect(makeInformerMock).toHaveBeenNthCalledWith( + 3, expect.any(KubeConfig), '/api/v1/namespaces/ns2/services', expect.anything(), @@ -1385,9 +1405,23 @@ describe('update', async () => { await client.update(kubeConfig); - expect(informerStopMock).toHaveBeenCalledTimes(1); - expect(informerStopMock).toHaveBeenCalledWith('context1', '/api/v1/namespaces/ns1/services'); - expect(makeInformerMock).not.toHaveBeenCalled(); + expect(informerStopMock).toHaveBeenCalledTimes(3); + expect(informerStopMock).toHaveBeenNthCalledWith(1, 'context2', '/api/v1/namespaces/ns2/pods'); + expect(informerStopMock).toHaveBeenNthCalledWith(2, 'context2', '/apis/apps/v1/namespaces/ns2/deployments'); + expect(informerStopMock).toHaveBeenNthCalledWith(3, 'context1', '/api/v1/namespaces/ns1/services'); + expect(makeInformerMock).toHaveBeenCalledTimes(2); + expect(makeInformerMock).toHaveBeenNthCalledWith( + 1, + expect.any(KubeConfig), + '/api/v1/namespaces/ns2/pods', + expect.anything(), + ); + expect(makeInformerMock).toHaveBeenNthCalledWith( + 2, + expect.any(KubeConfig), + '/apis/apps/v1/namespaces/ns2/deployments', + expect.anything(), + ); }); test('should not ignore events sent a short time before', async () => { @@ -1481,6 +1515,103 @@ describe('update', async () => { expect(apiSenderSendMock).toHaveBeenCalledWith('kubernetes-current-context-routes-update', []); }); + test('changing context that have same name as the old one should start service informer again', async () => { + vi.useFakeTimers(); + const makeInformerMock = vi.mocked(makeInformer); + makeInformerMock.mockImplementation( + ( + kubeconfig: kubeclient.KubeConfig, + path: string, + _listPromiseFn: kubeclient.ListPromise, + ) => { + return new FakeInformer(kubeconfig.currentContext, path, 0, undefined, [], []); + }, + ); + client = new ContextsManager(apiSender); + const kubeConfig = new kubeclient.KubeConfig(); + const config = { + clusters: [ + { + name: 'cluster1', + server: 'server1', + }, + ], + users: [ + { + name: 'user1', + }, + ], + contexts: [ + { + name: 'context1', + cluster: 'cluster1', + user: 'user1', + namespace: 'ns1', + }, + { + name: 'context2', + cluster: 'cluster1', + user: 'user1', + namespace: 'ns2', + }, + ], + currentContext: 'context1', + }; + kubeConfig.loadFromOptions(config); + await client.update(kubeConfig); + vi.advanceTimersToNextTimer(); + vi.advanceTimersToNextTimer(); + + makeInformerMock.mockClear(); + + // service informer is started + client.registerGetCurrentContextResources('services'); + expect(makeInformerMock).toHaveBeenCalledTimes(1); + expect(makeInformerMock).toHaveBeenCalledWith( + expect.any(KubeConfig), + '/api/v1/namespaces/ns1/services', + expect.anything(), + ); + + makeInformerMock.mockClear(); + + config.clusters = [ + { + name: 'cluster1', + server: 'server2', + }, + ]; + kubeConfig.loadFromOptions(config); + + expect(informerStopMock).not.toHaveBeenCalled(); + + await client.update(kubeConfig); + + expect(informerStopMock).toHaveBeenCalledTimes(3); + expect(informerStopMock).toHaveBeenNthCalledWith(1, 'context1', '/api/v1/namespaces/ns1/pods'); + expect(informerStopMock).toHaveBeenNthCalledWith(2, 'context1', '/apis/apps/v1/namespaces/ns1/deployments'); + expect(informerStopMock).toHaveBeenNthCalledWith(3, 'context1', '/api/v1/namespaces/ns1/services'); + expect(makeInformerMock).toHaveBeenCalledTimes(3); + expect(makeInformerMock).toHaveBeenNthCalledWith( + 1, + expect.any(KubeConfig), + '/api/v1/namespaces/ns1/pods', + expect.anything(), + ); + expect(makeInformerMock).toHaveBeenNthCalledWith( + 2, + expect.any(KubeConfig), + '/apis/apps/v1/namespaces/ns1/deployments', + expect.anything(), + ); + expect(makeInformerMock).toHaveBeenNthCalledWith( + 3, + expect.any(KubeConfig), + '/api/v1/namespaces/ns1/services', + expect.anything(), + ); + }); + test('dispose', async () => { vi.useFakeTimers(); vi.mocked(makeInformer).mockImplementation( @@ -1525,7 +1656,6 @@ describe('update', async () => { expect(apiSenderSendMock).not.toHaveBeenCalled(); }); }); - describe('ContextsStates tests', () => { test('hasInformer should check if informer exists for context', () => { const client = new ContextsStates(); @@ -1569,3 +1699,389 @@ describe('ContextsStates tests', () => { expect(client.isReachable('context3')).toBeFalsy(); }); }); +describe('isContextInKubeconfig', () => { + let client: ContextsManager; + beforeAll(async () => { + vi.mocked(makeInformer).mockImplementation( + ( + kubeconfig: kubeclient.KubeConfig, + path: string, + _listPromiseFn: kubeclient.ListPromise, + ) => { + const connectResult = new Error('err'); + return new FakeInformer(kubeconfig.currentContext, path, 0, connectResult, [], []); + }, + ); + const kubeConfig = new kubeclient.KubeConfig(); + const config = { + clusters: [ + { + name: 'cluster', + server: 'server', + }, + { + name: 'cluster1', + server: 'server1', + }, + ], + users: [ + { + name: 'user', + }, + { + name: 'user1', + }, + ], + contexts: [ + { + name: 'context', + cluster: 'cluster', + user: 'user', + namespace: 'ns', + }, + ], + currentContext: 'context', + }; + kubeConfig.loadFromOptions(config); + client = new ContextsManager(apiSender); + await client.update(kubeConfig); + }); + test('return false if cluster does not exists on kubeconfig', () => { + const context: KubeContext = { + name: 'context', + cluster: 'cluster2', + user: 'user', + clusterInfo: { + server: 'server2', + name: 'cluster2', + }, + }; + const exists = client.isContextInKubeconfig(context); + expect(exists).toBeFalsy(); + }); + test('return false if cluster on kubeconfig have different server', () => { + const context: KubeContext = { + name: 'context', + cluster: 'cluster', + user: 'user', + clusterInfo: { + server: 'server2', + name: 'cluster', + }, + }; + const exists = client.isContextInKubeconfig(context); + expect(exists).toBeFalsy(); + }); + test('return false if user does not exists on kubeconfig', () => { + const context: KubeContext = { + name: 'context', + cluster: 'cluster', + user: 'user2', + clusterInfo: { + server: 'server', + name: 'cluster', + }, + }; + const exists = client.isContextInKubeconfig(context); + expect(exists).toBeFalsy(); + }); + test('return false if context does not exists on kubeconfig', () => { + const context: KubeContext = { + name: 'context1', + cluster: 'cluster', + user: 'user', + clusterInfo: { + server: 'server', + name: 'cluster', + }, + }; + const exists = client.isContextInKubeconfig(context); + expect(exists).toBeFalsy(); + }); + test('return false if context server on kubeconfig have different server', () => { + const context: KubeContext = { + name: 'context', + cluster: 'cluster1', + user: 'user', + clusterInfo: { + server: 'server1', + name: 'cluster1', + }, + }; + const exists = client.isContextInKubeconfig(context); + expect(exists).toBeFalsy(); + }); + test('return false if context user on kubeconfig is different', () => { + const context: KubeContext = { + name: 'context', + cluster: 'cluster', + user: 'user1', + clusterInfo: { + server: 'server', + name: 'cluster', + }, + }; + const exists = client.isContextInKubeconfig(context); + expect(exists).toBeFalsy(); + }); + test('return true if the current context exists on the kubeconfig', () => { + const context: KubeContext = { + name: 'context', + cluster: 'cluster', + user: 'user', + clusterInfo: { + server: 'server', + name: 'cluster', + }, + }; + const exists = client.isContextInKubeconfig(context); + expect(exists).toBeTruthy(); + }); +}); +describe('isContextChanged', () => { + let client: ContextsManager; + beforeAll(async () => { + vi.mocked(makeInformer).mockImplementation( + ( + kubeconfig: kubeclient.KubeConfig, + path: string, + _listPromiseFn: kubeclient.ListPromise, + ) => { + const connectResult = new Error('err'); + return new FakeInformer(kubeconfig.currentContext, path, 0, connectResult, [], []); + }, + ); + const kubeConfig = new kubeclient.KubeConfig(); + const config = { + clusters: [ + { + name: 'cluster', + server: 'server', + }, + { + name: 'cluster1', + server: 'server1', + }, + ], + users: [ + { + name: 'user', + }, + { + name: 'user1', + }, + ], + contexts: [ + { + name: 'context', + cluster: 'cluster', + user: 'user', + namespace: 'ns', + }, + ], + currentContext: 'context', + }; + kubeConfig.loadFromOptions(config); + client = new ContextsManager(apiSender); + await client.update(kubeConfig); + }); + test('return true if current context is different from the latest', () => { + const context: KubeConfig = { + clusters: [ + { + name: 'cluster', + server: 'server', + }, + ], + users: [ + { + name: 'user', + }, + ], + contexts: [ + { + name: 'context2', + cluster: 'cluster', + user: 'user', + namespace: 'ns', + }, + ], + currentContext: 'context2', + } as KubeConfig; + const changed = client.isContextChanged(context); + expect(changed).toBeTruthy(); + }); + test('return true if current context is undefined', () => { + const context: KubeConfig = { + clusters: [ + { + name: 'cluster', + server: 'server', + }, + ], + users: [ + { + name: 'user', + }, + ], + contexts: [ + { + name: 'context2', + cluster: 'cluster', + user: 'user', + namespace: 'ns', + }, + ], + } as KubeConfig; + const changed = client.isContextChanged(context); + expect(changed).toBeTruthy(); + }); + test('return true if current context has a different user compared to the latest', () => { + const context: KubeConfig = { + clusters: [ + { + name: 'cluster', + server: 'server', + }, + ], + users: [ + { + name: 'user2', + }, + ], + contexts: [ + { + name: 'context', + cluster: 'cluster', + user: 'user', + namespace: 'ns', + }, + ], + currentContext: 'context', + getCurrentUser: () => { + return { + name: 'user2', + } as kubeclient.User; + }, + getCurrentCluster: () => { + return { + name: 'cluster', + server: 'server', + } as kubeclient.Cluster; + }, + } as KubeConfig; + const changed = client.isContextChanged(context); + expect(changed).toBeTruthy(); + }); + test('return true if current context has a different cluster compared to the latest', () => { + const context: KubeConfig = { + clusters: [ + { + name: 'cluster2', + server: 'server2', + }, + ], + users: [ + { + name: 'user', + }, + ], + contexts: [ + { + name: 'context', + cluster: 'cluster2', + user: 'user', + namespace: 'ns', + }, + ], + currentContext: 'context', + getCurrentUser: () => { + return { + name: 'user', + } as kubeclient.User; + }, + getCurrentCluster: () => { + return { + name: 'cluster2', + server: 'server2', + } as kubeclient.Cluster; + }, + } as KubeConfig; + const changed = client.isContextChanged(context); + expect(changed).toBeTruthy(); + }); + test('return true if current context has a different cluster server but same cluster name compared to the latest', () => { + const context: KubeConfig = { + clusters: [ + { + name: 'cluster', + server: 'server2', + }, + ], + users: [ + { + name: 'user', + }, + ], + contexts: [ + { + name: 'context', + cluster: 'cluster', + user: 'user', + namespace: 'ns', + }, + ], + currentContext: 'context', + getCurrentUser: () => { + return { + name: 'user', + } as kubeclient.User; + }, + getCurrentCluster: () => { + return { + name: 'cluster', + server: 'server2', + } as kubeclient.Cluster; + }, + } as KubeConfig; + const changed = client.isContextChanged(context); + expect(changed).toBeTruthy(); + }); + test('return false if current context is the same to the latest', () => { + const context: KubeConfig = { + clusters: [ + { + name: 'cluster', + server: 'server', + }, + ], + users: [ + { + name: 'user', + }, + ], + contexts: [ + { + name: 'context', + cluster: 'cluster', + user: 'user', + namespace: 'ns', + }, + ], + currentContext: 'context', + getCurrentUser: () => { + return { + name: 'user', + } as kubeclient.User; + }, + getCurrentCluster: () => { + return { + name: 'cluster', + server: 'server', + } as kubeclient.Cluster; + }, + } as KubeConfig; + const changed = client.isContextChanged(context); + expect(changed).toBeFalsy(); + }); +}); diff --git a/packages/main/src/plugin/kubernetes-context-state.ts b/packages/main/src/plugin/kubernetes-context-state.ts index 296bc382315..7fbe868bff4 100644 --- a/packages/main/src/plugin/kubernetes-context-state.ts +++ b/packages/main/src/plugin/kubernetes-context-state.ts @@ -336,7 +336,7 @@ class SecondaryResourceWatchersRegistry { export class ContextsManager { private kubeConfig = new KubeConfig(); private states = new ContextsStates(); - private currentContext: string | undefined; + private currentContext: KubeContext | undefined; private secondaryWatchers = new SecondaryResourceWatchersRegistry(); private dispatchContextsGeneralStateTimer: NodeJS.Timeout | undefined; @@ -359,15 +359,76 @@ export class ContextsManager { this.connectionDelayTimers.set(resourceName, connectionDelay); } + isContextInKubeconfig(context: KubeContext): boolean { + // if there is no cluster on the kubeconfig with the value of context.cluster -> false + const cluster = this.kubeConfig.getCluster(context.cluster); + if (!cluster || cluster.server !== context.clusterInfo?.server) { + return false; + } + + // if there is no user on the kubeconfig with the value of context.user -> false + const user = this.kubeConfig.getUser(context.user); + if (!user) { + return false; + } + + // if there is no context on the kubeconfig with the value of context.context -> false + const contextObj = this.kubeConfig.getContextObject(context.name); + /* eslint-disable-next-line no-null/no-null */ + return contextObj !== null && contextObj.cluster === context.cluster && contextObj.user === context.user; + } + + isContextChanged(kubeconfig: KubeConfig): boolean { + // if the current context name in the kubeconfig is different from the latest context we saved -> true + if (kubeconfig.currentContext !== this.currentContext?.name) { + return true; + } + + // by retrieving the context from the kubeconfig, if the user is different from the latest context we saved -> true + const user = kubeconfig.getCurrentUser(); + if (user?.name !== this.currentContext?.user) { + return true; + } + + // by retrieving the cluster from the kubeconfig, if the name or server url are different from the latest context we saved -> true + const cluster = kubeconfig.getCurrentCluster(); + return ( + cluster?.name !== this.currentContext?.cluster || cluster?.server !== this.currentContext?.clusterInfo?.server + ); + } + // update is the reconcile function, it gets as input the last known kube config // and starts/stops informers for different kube contexts, depending on these inputs async update(kubeconfig: KubeConfig): Promise { this.kubeConfig = kubeconfig; let contextChanged = false; - if (kubeconfig.currentContext !== this.currentContext) { + if (this.isContextChanged(kubeconfig)) { contextChanged = true; - this.currentContext = kubeconfig.currentContext; + const currentCluster = kubeconfig.getCurrentCluster(); + this.currentContext = { + name: kubeconfig.currentContext, + user: kubeconfig.getCurrentUser()?.name ?? '', + cluster: currentCluster?.name ?? '', + clusterInfo: { + name: currentCluster?.name ?? '', + server: currentCluster?.server ?? '', + }, + }; } + + // Delete informers for removed contexts + // We also remove the state of the current context if it has changed. It may happen that the name of the context is the same but it is pointing to a different cluster + let removed = false; + for (const name of this.states.getContextsNames()) { + if ( + !this.kubeConfig.contexts.find(c => c.name === name) || + (contextChanged && name === this.currentContext?.name) + ) { + await this.states.dispose(name); + removed = true; + } + } + // Add informers for new contexts let added = false; for (const context of this.kubeConfig.contexts) { @@ -378,28 +439,21 @@ export class ContextsManager { } } - // Delete informers for removed contexts - let removed = false; - for (const name of this.states.getContextsNames()) { - if (!this.kubeConfig.contexts.find(c => c.name === name)) { - await this.states.dispose(name); - removed = true; - } - } - // Delete secondary informers (others than pods/deployments) on non-current contexts if (contextChanged) { - const nonCurrentContexts = this.kubeConfig.contexts.filter(ctx => ctx.name !== this.currentContext); + const nonCurrentContexts = this.kubeConfig.contexts.filter(ctx => ctx.name !== this.currentContext?.name); for (const ctx of nonCurrentContexts) { const contextName = ctx.name; await this.states.disposeSecondaryInformers(contextName); } // Restart informers for secondary resources if watchers are subscribing for this resource - for (const resourceName of secondaryResources) { - if (this.secondaryWatchers.hasSubscribers(resourceName)) { - console.debug(`start watching ${resourceName} in context ${this.currentContext}`); - this.startResourceInformer(this.currentContext, resourceName); + if (this.currentContext?.name) { + for (const resourceName of secondaryResources) { + if (this.secondaryWatchers.hasSubscribers(resourceName)) { + console.debug(`start watching ${resourceName} in context ${this.currentContext}`); + this.startResourceInformer(this.currentContext.name, resourceName); + } } } } @@ -787,7 +841,15 @@ export class ContextsManager { return; } options.timer = setTimeout(() => { - this.restartInformer(informer, context, options); + // before restarting the failed informer we should check if the context is still present in the kubeconfig. + // It is possible that if we start an informer on an old cluster it will keep failing without any chance to be stopped. + // That happens bc, in the library, when executing the listFn + // it throws here (as the cluster is not reachable) https://github.com/kubernetes-client/javascript/blob/b8b0ec522bf42086f7c2e1b8648b478b3f584fa8/src/cache.ts#L161 + // so the watch request is not initialized -> https://github.com/kubernetes-client/javascript/blob/b8b0ec522bf42086f7c2e1b8648b478b3f584fa8/src/cache.ts#L181 + // and then the stop action cannot be executed -> https://github.com/kubernetes-client/javascript/blob/b8b0ec522bf42086f7c2e1b8648b478b3f584fa8/src/cache.ts#L134 + if (this.isContextInKubeconfig(context)) { + this.restartInformer(informer, context, options); + } }, nextTimeout); }); } @@ -866,19 +928,19 @@ export class ContextsManager { if (isSecondaryResourceName(resourceName)) { this.secondaryWatchers.subscribe(resourceName); } - if (!this.currentContext) { + if (!this.currentContext?.name) { return []; } - if (this.states.hasInformer(this.currentContext, resourceName)) { + if (this.states.hasInformer(this.currentContext.name, resourceName)) { console.debug(`already watching ${resourceName} in context ${this.currentContext}`); return this.states.getContextResources(this.kubeConfig.currentContext, resourceName); } - if (!this.states.isReachable(this.currentContext)) { + if (!this.states.isReachable(this.currentContext.name)) { console.debug(`skip watching ${resourceName} in context ${this.currentContext}, as the context is not reachable`); return []; } console.debug(`start watching ${resourceName} in context ${this.currentContext}`); - this.startResourceInformer(this.currentContext, resourceName); + this.startResourceInformer(this.currentContext.name, resourceName); return []; }