From 7db14b1646b299da403fc152765fbc40cd2970b0 Mon Sep 17 00:00:00 2001 From: AllanZhengYP Date: Thu, 18 Feb 2021 11:30:54 -0800 Subject: [PATCH] fix(credential-provider-node): read config and credentials files only once (#2045) * chore(credential-provider-node): refactor provider chain * fix(credential-provider-node): read config files only once * docs(credential-provider-*): mark laodedConfigs param internal * chore(credential-provider-node): make functions arrow --- packages/credential-provider-ini/src/index.ts | 2 + .../credential-provider-node/package.json | 1 + .../src/index.spec.ts | 38 +++++++++++++++++-- .../credential-provider-node/src/index.ts | 18 +++++---- .../credential-provider-process/src/index.ts | 5 +++ .../src/fromSharedConfigFiles.ts | 2 + 6 files changed, 54 insertions(+), 12 deletions(-) diff --git a/packages/credential-provider-ini/src/index.ts b/packages/credential-provider-ini/src/index.ts index 3e855cafb6dd..50a8aa2b3c4f 100755 --- a/packages/credential-provider-ini/src/index.ts +++ b/packages/credential-provider-ini/src/index.ts @@ -53,6 +53,8 @@ export interface FromIniInit extends SharedConfigInit { /** * A promise that will be resolved with loaded and parsed credentials files. * Used to avoid loading shared config files multiple times. + * + * @internal */ loadedConfig?: Promise; diff --git a/packages/credential-provider-node/package.json b/packages/credential-provider-node/package.json index 98024f93ffa2..7309bb2f86dc 100644 --- a/packages/credential-provider-node/package.json +++ b/packages/credential-provider-node/package.json @@ -31,6 +31,7 @@ "@aws-sdk/credential-provider-ini": "3.4.1", "@aws-sdk/credential-provider-process": "3.4.1", "@aws-sdk/property-provider": "3.4.1", + "@aws-sdk/shared-ini-file-loader": "3.4.1", "@aws-sdk/types": "3.4.1", "tslib": "^1.8.0" }, diff --git a/packages/credential-provider-node/src/index.spec.ts b/packages/credential-provider-node/src/index.spec.ts index e78c66f042e1..6a34ef21017b 100644 --- a/packages/credential-provider-node/src/index.spec.ts +++ b/packages/credential-provider-node/src/index.spec.ts @@ -10,6 +10,17 @@ jest.mock("@aws-sdk/credential-provider-env", () => { }); import { fromEnv } from "@aws-sdk/credential-provider-env"; +const loadedConfig = { + credentialsFile: { + foo: { aws_access_key_id: "key", aws_secret_access_key: "secret" }, + }, + configFile: { bar: { aws_access_key_id: "key", aws_secret_access_key: "secret" } }, +}; +jest.mock("@aws-sdk/shared-ini-file-loader", () => ({ + loadSharedConfigFiles: jest.fn().mockReturnValue(loadedConfig), +})); +import { loadSharedConfigFiles } from "@aws-sdk/shared-ini-file-loader"; + jest.mock("@aws-sdk/credential-provider-ini", () => { const iniProvider = jest.fn(); return { @@ -39,6 +50,7 @@ jest.mock("@aws-sdk/credential-provider-imds", () => { ENV_CMDS_RELATIVE_URI: "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI", }; }); + import { ENV_CMDS_FULL_URI, ENV_CMDS_RELATIVE_URI, @@ -78,6 +90,7 @@ beforeEach(() => { (fromProcess as any).mockClear(); (fromContainerMetadata as any).mockClear(); (fromInstanceMetadata as any).mockClear(); + (loadSharedConfigFiles as any).mockClear(); }); afterAll(() => { @@ -200,6 +213,23 @@ describe("defaultProvider", () => { expect((fromInstanceMetadata() as any).mock.calls.length).toBe(0); }); + it("should read config files only once for all providers", async () => { + const creds = { + accessKeyId: "foo", + secretAccessKey: "bar", + }; + + (fromEnv() as any).mockImplementation(() => Promise.reject(new ProviderError("Keep moving!"))); + (fromIni() as any).mockImplementation(() => Promise.reject(new ProviderError("Nothing here!"))); + (fromProcess() as any).mockImplementation(() => Promise.reject(new ProviderError("Nor here!"))); + (fromInstanceMetadata() as any).mockImplementation(() => Promise.resolve(creds)); + + await expect(defaultProvider()()).resolves; + expect((loadSharedConfigFiles as any).mock.calls.length).toBe(1); + expect((fromIni as any).mock.calls[1][0]).toMatchObject({ loadedConfig: loadSharedConfigFiles() }); + expect((fromProcess as any).mock.calls[1][0]).toMatchObject({ loadedConfig: loadSharedConfigFiles() }); + }); + it("should pass configuration on to the ini provider", async () => { const iniConfig: FromIniInit = { profile: "foo", @@ -226,7 +256,7 @@ describe("defaultProvider", () => { await expect(defaultProvider(iniConfig)()).resolves; expect((fromIni as any).mock.calls.length).toBe(1); - expect((fromIni as any).mock.calls[0][0]).toBe(iniConfig); + expect((fromIni as any).mock.calls[0][0]).toEqual({ ...iniConfig, loadedConfig }); }); it("should pass configuration on to the process provider", async () => { @@ -249,7 +279,7 @@ describe("defaultProvider", () => { await expect(defaultProvider(processConfig)()).resolves; expect((fromProcess as any).mock.calls.length).toBe(1); expect((fromProcess as any).mock.calls.length).toBe(1); - expect((fromProcess as any).mock.calls[0][0]).toBe(processConfig); + expect((fromProcess as any).mock.calls[0][0]).toEqual({ ...processConfig, loadedConfig }); }); it("should pass configuration on to the IMDS provider", async () => { @@ -273,7 +303,7 @@ describe("defaultProvider", () => { await expect(defaultProvider(imdsConfig)()).resolves; expect((fromInstanceMetadata as any).mock.calls.length).toBe(1); - expect((fromInstanceMetadata as any).mock.calls[0][0]).toBe(imdsConfig); + expect((fromInstanceMetadata as any).mock.calls[0][0]).toEqual({ ...imdsConfig, loadedConfig }); }); it("should pass configuration on to the ECS IMDS provider", async () => { @@ -298,7 +328,7 @@ describe("defaultProvider", () => { await expect(defaultProvider(ecsImdsConfig)()).resolves; expect((fromContainerMetadata as any).mock.calls.length).toBe(1); - expect((fromContainerMetadata as any).mock.calls[0][0]).toBe(ecsImdsConfig); + expect((fromContainerMetadata as any).mock.calls[0][0]).toEqual({ ...ecsImdsConfig, loadedConfig }); }); it("should return the same promise across invocations", async () => { diff --git a/packages/credential-provider-node/src/index.ts b/packages/credential-provider-node/src/index.ts index 1f3bd44bf0fe..409124e0ade7 100644 --- a/packages/credential-provider-node/src/index.ts +++ b/packages/credential-provider-node/src/index.ts @@ -9,6 +9,7 @@ import { import { ENV_PROFILE, fromIni, FromIniInit } from "@aws-sdk/credential-provider-ini"; import { fromProcess, FromProcessInit } from "@aws-sdk/credential-provider-process"; import { chain, memoize, ProviderError } from "@aws-sdk/property-provider"; +import { loadSharedConfigFiles } from "@aws-sdk/shared-ini-file-loader"; import { CredentialProvider } from "@aws-sdk/types"; export const ENV_IMDS_DISABLED = "AWS_EC2_METADATA_DISABLED"; @@ -41,20 +42,21 @@ export const ENV_IMDS_DISABLED = "AWS_EC2_METADATA_DISABLED"; * @see fromContainerMetadata The function used to source credentials from the * ECS Container Metadata Service */ -export function defaultProvider(init: FromIniInit & RemoteProviderInit & FromProcessInit = {}): CredentialProvider { - const { profile = process.env[ENV_PROFILE] } = init; - const providerChain = profile - ? chain(fromIni(init), fromProcess(init)) - : chain(fromEnv(), fromIni(init), fromProcess(init), remoteProvider(init)); +export const defaultProvider = (init: FromIniInit & RemoteProviderInit & FromProcessInit = {}): CredentialProvider => { + const options = { profile: process.env[ENV_PROFILE], ...init }; + if (!options.loadedConfig) options.loadedConfig = loadSharedConfigFiles(init); + const providers = [fromIni(options), fromProcess(options), remoteProvider(options)]; + if (!options.profile) providers.unshift(fromEnv()); + const providerChain = chain(...providers); return memoize( providerChain, (credentials) => credentials.expiration !== undefined && credentials.expiration.getTime() - Date.now() < 300000, (credentials) => credentials.expiration !== undefined ); -} +}; -function remoteProvider(init: RemoteProviderInit): CredentialProvider { +const remoteProvider = (init: RemoteProviderInit): CredentialProvider => { if (process.env[ENV_CMDS_RELATIVE_URI] || process.env[ENV_CMDS_FULL_URI]) { return fromContainerMetadata(init); } @@ -64,4 +66,4 @@ function remoteProvider(init: RemoteProviderInit): CredentialProvider { } return fromInstanceMetadata(init); -} +}; diff --git a/packages/credential-provider-process/src/index.ts b/packages/credential-provider-process/src/index.ts index e42cf359b0b4..84fa3d973ca9 100755 --- a/packages/credential-provider-process/src/index.ts +++ b/packages/credential-provider-process/src/index.ts @@ -4,6 +4,9 @@ import { ParsedIniData, SharedConfigFiles, SharedConfigInit } from "@aws-sdk/sha import { CredentialProvider, Credentials } from "@aws-sdk/types"; import { exec } from "child_process"; +/** + * @internal + */ export const ENV_PROFILE = "AWS_PROFILE"; export interface FromProcessInit extends SharedConfigInit { @@ -15,6 +18,8 @@ export interface FromProcessInit extends SharedConfigInit { /** * A promise that will be resolved with loaded and parsed credentials files. * Used to avoid loading shared config files multiple times. + * + * @internal */ loadedConfig?: Promise; } diff --git a/packages/node-config-provider/src/fromSharedConfigFiles.ts b/packages/node-config-provider/src/fromSharedConfigFiles.ts index d20dcfd28a6c..c306b2db9e85 100644 --- a/packages/node-config-provider/src/fromSharedConfigFiles.ts +++ b/packages/node-config-provider/src/fromSharedConfigFiles.ts @@ -26,6 +26,8 @@ export interface SharedConfigInit extends BaseSharedConfigInit { /** * A promise that will be resolved with loaded and parsed credentials files. * Used to avoid loading shared config files multiple times. + * + * @internal */ loadedConfig?: Promise; }