From 63aa03cc82f277457933f129c41bd48e4ccc6686 Mon Sep 17 00:00:00 2001 From: goenning Date: Fri, 20 May 2022 16:13:31 +0100 Subject: [PATCH] code review Signed-off-by: goenning --- ...reIdentityKubernetesAuthTranslator.test.ts | 31 +++++++++++++++---- .../AzureIdentityKubernetesAuthTranslator.ts | 26 ++++++++++------ .../KubernetesAuthTranslatorGenerator.test.ts | 10 +++--- .../KubernetesAuthTranslatorGenerator.ts | 6 ++-- .../src/service/KubernetesFanOutHandler.ts | 4 ++- 5 files changed, 54 insertions(+), 23 deletions(-) diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AzureIdentityKubernetesAuthTranslator.test.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AzureIdentityKubernetesAuthTranslator.test.ts index 574b5838f9..a28f825434 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AzureIdentityKubernetesAuthTranslator.test.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AzureIdentityKubernetesAuthTranslator.test.ts @@ -72,7 +72,7 @@ describe('AzureIdentityKubernetesAuthTranslator tests', () => { it('should issue new token 15 minutes befory expiry', async () => { const authTranslator = new AzureIdentityKubernetesAuthTranslator( logger, - new StaticTokenCredential(16 * 60 * 1000), // token expires in 16m + new StaticTokenCredential(16 * 60 * 1000), // token expires in 16min ); const response = await authTranslator.decorateClusterDetailsWithAuth(cd); @@ -87,25 +87,44 @@ describe('AzureIdentityKubernetesAuthTranslator tests', () => { it('should re-use existing token if there is afailure', async () => { const authTranslator = new AzureIdentityKubernetesAuthTranslator( logger, - new StaticTokenCredential(16 * 60 * 1000), // new tokens expires in 16m + new StaticTokenCredential(16 * 60 * 1000), // new tokens expires in 16min ); const response = await authTranslator.decorateClusterDetailsWithAuth(cd); expect(response.serviceAccountToken).toEqual('MY_TOKEN_1'); - jest.useFakeTimers().setSystemTime(Date.now() + 2 * 60 * 1000); // advance time by 2mins + jest.useFakeTimers().setSystemTime(Date.now() + 2 * 60 * 1000); // advance time by 2min const response2 = await authTranslator.decorateClusterDetailsWithAuth(cd); expect(response2.serviceAccountToken).toEqual('MY_TOKEN_2'); - jest.useFakeTimers().setSystemTime(Date.now() + 2 * 60 * 1000); // advance time by 2mins + jest.useFakeTimers().setSystemTime(Date.now() + 2 * 60 * 1000); // advance time by 2min const response3 = await authTranslator.decorateClusterDetailsWithAuth(cd); expect(response3.serviceAccountToken).toEqual('MY_TOKEN_2'); - jest.useFakeTimers().setSystemTime(Date.now() + 2 * 60 * 1000); // advance time by 2mins - const response4 = await authTranslator.decorateClusterDetailsWithAuth(cd); expect(response4.serviceAccountToken).toEqual('MY_TOKEN_4'); }); + + it('should throw if existing token expired and failed to fetch a new one', async () => { + const authTranslator = new AzureIdentityKubernetesAuthTranslator( + logger, + new StaticTokenCredential(16 * 60 * 1000), // new tokens expires in 16min + ); + + const response = await authTranslator.decorateClusterDetailsWithAuth(cd); + expect(response.serviceAccountToken).toEqual('MY_TOKEN_1'); + + jest.useFakeTimers().setSystemTime(Date.now() + 2 * 60 * 1000); // advance time by 2min + + const response2 = await authTranslator.decorateClusterDetailsWithAuth(cd); + expect(response2.serviceAccountToken).toEqual('MY_TOKEN_2'); + + jest.useFakeTimers().setSystemTime(Date.now() + 17 * 60 * 1000); // advance time by 17min + + await expect( + authTranslator.decorateClusterDetailsWithAuth(cd), + ).rejects.toThrow(); + }); }); diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AzureIdentityKubernetesAuthTranslator.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AzureIdentityKubernetesAuthTranslator.ts index 9b913cccff..5b8a709270 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AzureIdentityKubernetesAuthTranslator.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AzureIdentityKubernetesAuthTranslator.ts @@ -29,7 +29,7 @@ export class AzureIdentityKubernetesAuthTranslator implements KubernetesAuthTranslator { private accessToken: AccessToken = { token: '', expiresOnTimestamp: 0 }; - private newToken: Promise | undefined; + private newTokenPromise: Promise | undefined; constructor( private readonly logger: Logger, @@ -49,15 +49,15 @@ export class AzureIdentityKubernetesAuthTranslator } private async getToken(): Promise { - if (this.isTokenValid()) { + if (!this.tokenRequiresRefresh()) { return this.accessToken.token; } - if (!this.newToken) { - this.newToken = this.fetchNewToken(); + if (!this.newTokenPromise) { + this.newTokenPromise = this.fetchNewToken(); } - return this.newToken; + return this.newTokenPromise; } private async fetchNewToken(): Promise { @@ -74,16 +74,24 @@ export class AzureIdentityKubernetesAuthTranslator this.accessToken = newAccessToken; } catch (err) { this.logger.error('Unable to fetch Azure token', err); - // don't throw the error, so the existing token will be re-used until we're able to fetch a new token + + // only throw the error if the token has already expired, otherwise re-use existing until we're able to fetch a new token + if (this.tokenExpired()) { + throw err; + } } - this.newToken = undefined; + this.newTokenPromise = undefined; return this.accessToken.token; } - private isTokenValid(): boolean { + private tokenRequiresRefresh(): boolean { // Set tokens to expire 15 minutes before its actual expiry time const expiresOn = this.accessToken.expiresOnTimestamp - 15 * 60 * 1000; - return expiresOn >= Date.now(); + return Date.now() >= expiresOn; + } + + private tokenExpired(): boolean { + return Date.now() >= this.accessToken.expiresOnTimestamp; } } diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts index dc0d589148..123b05456c 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts @@ -29,19 +29,19 @@ describe('getKubernetesAuthTranslatorInstance', () => { it('can return an auth translator for google auth', () => { const authTranslator: KubernetesAuthTranslator = - sut.getKubernetesAuthTranslatorInstance(logger, 'google'); + sut.getKubernetesAuthTranslatorInstance('google', { logger }); expect(authTranslator instanceof GoogleKubernetesAuthTranslator).toBe(true); }); it('can return an auth translator for aws auth', () => { const authTranslator: KubernetesAuthTranslator = - sut.getKubernetesAuthTranslatorInstance(logger, 'aws'); + sut.getKubernetesAuthTranslatorInstance('aws', { logger }); expect(authTranslator instanceof AwsIamKubernetesAuthTranslator).toBe(true); }); it('can return an auth translator for serviceAccount auth', () => { const authTranslator: KubernetesAuthTranslator = - sut.getKubernetesAuthTranslatorInstance(logger, 'serviceAccount'); + sut.getKubernetesAuthTranslatorInstance('serviceAccount', { logger }); expect( authTranslator instanceof ServiceAccountKubernetesAuthTranslator, ).toBe(true); @@ -49,13 +49,13 @@ describe('getKubernetesAuthTranslatorInstance', () => { it('can return an auth translator for oidc auth', () => { const authTranslator: KubernetesAuthTranslator = - sut.getKubernetesAuthTranslatorInstance(logger, 'oidc'); + sut.getKubernetesAuthTranslatorInstance('oidc', { logger }); expect(authTranslator instanceof OidcKubernetesAuthTranslator).toBe(true); }); it('throws an error when asked for an auth translator for an unsupported auth type', () => { expect(() => - sut.getKubernetesAuthTranslatorInstance(logger, 'linode'), + sut.getKubernetesAuthTranslatorInstance('linode', { logger }), ).toThrow( 'authProvider "linode" has no KubernetesAuthTranslator associated with it', ); diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.ts index 92c267cec9..fa69ad42b5 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.ts @@ -25,8 +25,10 @@ import { OidcKubernetesAuthTranslator } from './OidcKubernetesAuthTranslator'; export class KubernetesAuthTranslatorGenerator { static getKubernetesAuthTranslatorInstance( - logger: Logger, authProvider: string, + options: { + logger: Logger; + }, ): KubernetesAuthTranslator { switch (authProvider) { case 'google': { @@ -36,7 +38,7 @@ export class KubernetesAuthTranslatorGenerator { return new AwsIamKubernetesAuthTranslator(); } case 'azure': { - return new AzureIdentityKubernetesAuthTranslator(logger); + return new AzureIdentityKubernetesAuthTranslator(options.logger); } case 'serviceAccount': { return new ServiceAccountKubernetesAuthTranslator(); diff --git a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts index e65754a8cb..275347ff82 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts @@ -293,8 +293,10 @@ export class KubernetesFanOutHandler { this.authTranslators[provider] = KubernetesAuthTranslatorGenerator.getKubernetesAuthTranslatorInstance( - this.logger, provider, + { + logger: this.logger, + }, ); return this.authTranslators[provider]; }