Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add logging for CredentialsProviderError #1290

Merged
merged 6 commits into from
May 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/late-glasses-jam.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@smithy/credential-provider-imds": minor
"@smithy/shared-ini-file-loader": minor
"@smithy/node-config-provider": minor
"@smithy/property-provider": minor
---

new logging-compatible signature for CredentialsProviderError
31 changes: 18 additions & 13 deletions packages/credential-provider-imds/src/fromContainerMetadata.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { CredentialsProviderError } from "@smithy/property-provider";
import { AwsCredentialIdentityProvider } from "@smithy/types";
import { AwsCredentialIdentityProvider, Logger } from "@smithy/types";
import { RequestOptions } from "http";
import { parse } from "url";

Expand Down Expand Up @@ -31,10 +31,12 @@ export const fromContainerMetadata = (init: RemoteProviderInit = {}): AwsCredent
const { timeout, maxRetries } = providerConfigFromInit(init);
return () =>
retry(async () => {
const requestOptions = await getCmdsUri();
const requestOptions = await getCmdsUri({ logger: init.logger });
const credsResponse = JSON.parse(await requestFromEcsImds(timeout, requestOptions));
if (!isImdsCredentials(credsResponse)) {
throw new CredentialsProviderError("Invalid response received from instance metadata service.");
throw new CredentialsProviderError("Invalid response received from instance metadata service.", {
logger: init.logger,
});
}
return fromImdsCredentials(credsResponse);
}, maxRetries);
Expand Down Expand Up @@ -65,7 +67,7 @@ const GREENGRASS_PROTOCOLS = {
"https:": true,
};

const getCmdsUri = async (): Promise<RequestOptions> => {
const getCmdsUri = async ({ logger }: { logger?: Logger }): Promise<RequestOptions> => {
if (process.env[ENV_CMDS_RELATIVE_URI]) {
return {
hostname: CMDS_IP,
Expand All @@ -76,17 +78,17 @@ const getCmdsUri = async (): Promise<RequestOptions> => {
if (process.env[ENV_CMDS_FULL_URI]) {
const parsed = parse(process.env[ENV_CMDS_FULL_URI]!);
if (!parsed.hostname || !(parsed.hostname in GREENGRASS_HOSTS)) {
throw new CredentialsProviderError(
`${parsed.hostname} is not a valid container metadata service hostname`,
false
);
throw new CredentialsProviderError(`${parsed.hostname} is not a valid container metadata service hostname`, {
tryNextLink: false,
logger,
});
}

if (!parsed.protocol || !(parsed.protocol in GREENGRASS_PROTOCOLS)) {
throw new CredentialsProviderError(
`${parsed.protocol} is not a valid container metadata service protocol`,
false
);
throw new CredentialsProviderError(`${parsed.protocol} is not a valid container metadata service protocol`, {
tryNextLink: false,
logger,
});
}

return {
Expand All @@ -99,6 +101,9 @@ const getCmdsUri = async (): Promise<RequestOptions> => {
"The container metadata credential provider cannot be used unless" +
` the ${ENV_CMDS_RELATIVE_URI} or ${ENV_CMDS_FULL_URI} environment` +
" variable is set",
false
{
tryNextLink: false,
logger,
}
);
};
24 changes: 15 additions & 9 deletions packages/credential-provider-imds/src/fromInstanceMetadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@ const X_AWS_EC2_METADATA_TOKEN = "x-aws-ec2-metadata-token";
* Instance Metadata Service
*/
export const fromInstanceMetadata = (init: RemoteProviderInit = {}): Provider<InstanceMetadataCredentials> =>
staticStabilityProvider(getInstanceImdsProvider(init), { logger: init.logger });
staticStabilityProvider(getInstanceMetadataProvider(init), { logger: init.logger });

const getInstanceImdsProvider = (init: RemoteProviderInit) => {
/**
* @internal
*/
const getInstanceMetadataProvider = (init: RemoteProviderInit = {}) => {
// when set to true, metadata service will not fetch token
let disableFetchToken = false;
const { logger, profile } = init;
Expand All @@ -47,7 +50,8 @@ const getInstanceImdsProvider = (init: RemoteProviderInit) => {
fallbackBlockedFromProcessEnv = !!envValue && envValue !== "false";
if (envValue === undefined) {
throw new CredentialsProviderError(
`${AWS_EC2_METADATA_V1_DISABLED} not set in env, checking config file next.`
`${AWS_EC2_METADATA_V1_DISABLED} not set in env, checking config file next.`,
{ logger: init.logger }
);
}
return fallbackBlockedFromProcessEnv;
Expand Down Expand Up @@ -98,7 +102,7 @@ const getInstanceImdsProvider = (init: RemoteProviderInit) => {
return retry(async () => {
let creds: AwsCredentialIdentity;
try {
creds = await getCredentialsFromProfile(imdsProfile, options);
creds = await getCredentialsFromProfile(imdsProfile, options, init);
} catch (err) {
if (err.statusCode === 401) {
disableFetchToken = false;
Expand Down Expand Up @@ -152,8 +156,8 @@ const getMetadataToken = async (options: RequestOptions) =>

const getProfile = async (options: RequestOptions) => (await httpRequest({ ...options, path: IMDS_PATH })).toString();

const getCredentialsFromProfile = async (profile: string, options: RequestOptions) => {
const credsResponse = JSON.parse(
const getCredentialsFromProfile = async (profile: string, options: RequestOptions, init: RemoteProviderInit) => {
const credentialsResponse = JSON.parse(
(
await httpRequest({
...options,
Expand All @@ -162,9 +166,11 @@ const getCredentialsFromProfile = async (profile: string, options: RequestOption
).toString()
);

if (!isImdsCredentials(credsResponse)) {
throw new CredentialsProviderError("Invalid response received from instance metadata service.");
if (!isImdsCredentials(credentialsResponse)) {
throw new CredentialsProviderError("Invalid response received from instance metadata service.", {
logger: init.logger,
});
}

return fromImdsCredentials(credsResponse);
return fromImdsCredentials(credentialsResponse);
};
29 changes: 17 additions & 12 deletions packages/node-config-provider/src/fromEnv.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,30 @@ import { fromEnv, GetterFromEnv } from "./fromEnv";

describe("fromEnv", () => {
describe("with env var getter", () => {
const envVarName = "ENV_VAR_NAME";
const ENV_VAR_NAME = "ENV_VAR_NAME";

// Using Record<string, string | undefined> instead of NodeJS.ProcessEnv, in order to not get type errors in non node environments
const envVarGetter: GetterFromEnv<string> = (env: Record<string, string | undefined>) => env[envVarName]!;
const envVarValue = process.env[envVarName];
const envVarGetter: GetterFromEnv<string> = (env: Record<string, string | undefined>) => env[ENV_VAR_NAME]!;
const envVarValue = process.env[ENV_VAR_NAME];
const mockEnvVarValue = "mockEnvVarValue";

const getCredentialsProviderError = (getter: GetterFromEnv<string>) =>
new CredentialsProviderError(`Cannot load config from environment variables with getter: ${getter}`);

beforeEach(() => {
delete process.env[envVarName];
delete process.env[ENV_VAR_NAME];
});

afterAll(() => {
process.env[envVarName] = envVarValue;
process.env[ENV_VAR_NAME] = envVarValue;
});

describe("CredentialsProviderError", () => {
it("is behaving as expected cross-package in jest", () => {
expect(new CredentialsProviderError("msg", {}).message).toEqual("msg");
expect(new CredentialsProviderError("msg", {}).name).toEqual("CredentialsProviderError");
});
});

it(`returns string value in '${envVarName}' env var when set`, () => {
process.env[envVarName] = mockEnvVarValue;
it(`returns string value in '${ENV_VAR_NAME}' env var when set`, () => {
process.env[ENV_VAR_NAME] = mockEnvVarValue;
return expect(fromEnv(envVarGetter)()).resolves.toBe(mockEnvVarValue);
});

Expand All @@ -35,9 +39,10 @@ describe("fromEnv", () => {
return expect(fromEnv(getter)()).resolves.toEqual(value);
});

it(`throws when '${envVarName}' env var is not set`, () => {
it(`throws when '${ENV_VAR_NAME}' env var is not set`, async () => {
expect.assertions(1);
return expect(fromEnv(envVarGetter)()).rejects.toMatchObject(getCredentialsProviderError(envVarGetter));
const error = await fromEnv(envVarGetter)().catch((_) => _);
return expect(error).toEqual(new CredentialsProviderError(`Not found in ENV: ENV_VAR_NAME`, {}));
});

it("throws when the getter function throws", () => {
Expand Down
9 changes: 6 additions & 3 deletions packages/node-config-provider/src/fromEnv.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { CredentialsProviderError } from "@smithy/property-provider";
import { Provider } from "@smithy/types";
import { Logger, Provider } from "@smithy/types";

import { getSelectorName } from "./getSelectorName";

// Using Record<string, string | undefined> instead of NodeJS.ProcessEnv, in order to not get type errors in non node environments
export type GetterFromEnv<T> = (env: Record<string, string | undefined>) => T | undefined;
Expand All @@ -9,7 +11,7 @@ export type GetterFromEnv<T> = (env: Record<string, string | undefined>) => T |
* environment variable.
*/
export const fromEnv =
<T = string>(envVarSelector: GetterFromEnv<T>): Provider<T> =>
<T = string>(envVarSelector: GetterFromEnv<T>, logger?: Logger): Provider<T> =>
async () => {
try {
const config = envVarSelector(process.env);
Expand All @@ -19,7 +21,8 @@ export const fromEnv =
return config as T;
} catch (e) {
throw new CredentialsProviderError(
e.message || `Cannot load config from environment variables with getter: ${envVarSelector}`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we were previously logging the Function.toString() value, which is quite unreadable...

e.message || `Not found in ENV: ${getSelectorName(envVarSelector.toString())}`,
{ logger }
);
}
};
48 changes: 23 additions & 25 deletions packages/node-config-provider/src/fromSharedConfigFiles.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,11 @@ jest.mock("@smithy/shared-ini-file-loader", () => ({
}));

describe("fromSharedConfigFiles", () => {
const configKey = "config_key";
const configGetter: GetterFromConfig<string> = (profile: Profile) => profile[configKey];
const CONFIG_KEY = "config_key";
const configGetter: GetterFromConfig<string> = (profile: Profile) => profile[CONFIG_KEY];

const getCredentialsProviderError = (profile: string, getter: GetterFromConfig<string>) =>
new CredentialsProviderError(
`Cannot load config for profile ${profile} in SDK configuration files with getter: ${getter}`
);
const getCredentialsProviderError = (profile: string) =>
new CredentialsProviderError(`Not found in config files w/ profile [${profile}]: CONFIG_KEY`, {});

describe("loadedConfig", () => {
const mockConfigAnswer = "mockConfigAnswer";
Expand All @@ -36,33 +34,33 @@ describe("fromSharedConfigFiles", () => {
{
message: "returns configValue from default profile",
iniDataInConfig: {
default: { [configKey]: mockConfigAnswer },
default: { [CONFIG_KEY]: mockConfigAnswer },
},
iniDataInCredentials: {
default: { [configKey]: mockCredentialsNotAnswer },
default: { [CONFIG_KEY]: mockCredentialsNotAnswer },
},
configValueToVerify: mockConfigAnswer,
},
{
message: "returns configValue from designated profile",
iniDataInConfig: {
default: { [configKey]: mockConfigNotAnswer },
foo: { [configKey]: mockConfigAnswer },
default: { [CONFIG_KEY]: mockConfigNotAnswer },
foo: { [CONFIG_KEY]: mockConfigAnswer },
},
iniDataInCredentials: {
foo: { [configKey]: mockCredentialsNotAnswer },
foo: { [CONFIG_KEY]: mockCredentialsNotAnswer },
},
profile: "foo",
configValueToVerify: mockConfigAnswer,
},
{
message: "returns configValue from credentials file if preferred",
iniDataInConfig: {
default: { [configKey]: mockConfigNotAnswer },
foo: { [configKey]: mockConfigNotAnswer },
default: { [CONFIG_KEY]: mockConfigNotAnswer },
foo: { [CONFIG_KEY]: mockConfigNotAnswer },
},
iniDataInCredentials: {
foo: { [configKey]: mockCredentialsAnswer },
foo: { [CONFIG_KEY]: mockCredentialsAnswer },
},
profile: "foo",
preferredFile: "credentials",
Expand All @@ -71,7 +69,7 @@ describe("fromSharedConfigFiles", () => {
{
message: "returns configValue from config file if preferred credentials file doesn't contain config",
iniDataInConfig: {
foo: { [configKey]: mockConfigAnswer },
foo: { [CONFIG_KEY]: mockConfigAnswer },
},
iniDataInCredentials: {},
configValueToVerify: mockConfigAnswer,
Expand All @@ -82,7 +80,7 @@ describe("fromSharedConfigFiles", () => {
message: "returns configValue from credential file if preferred config file doesn't contain config",
iniDataInConfig: {},
iniDataInCredentials: {
foo: { [configKey]: mockCredentialsAnswer },
foo: { [CONFIG_KEY]: mockCredentialsAnswer },
},
configValueToVerify: mockCredentialsAnswer,
profile: "foo",
Expand All @@ -93,14 +91,14 @@ describe("fromSharedConfigFiles", () => {
{
message: "rejects if default profile is not present and profile value is not passed",
iniDataInConfig: {
foo: { [configKey]: mockConfigNotAnswer },
foo: { [CONFIG_KEY]: mockConfigNotAnswer },
},
iniDataInCredentials: {},
},
{
message: "rejects if designated profile is not present",
iniDataInConfig: {
default: { [configKey]: mockConfigNotAnswer },
default: { [CONFIG_KEY]: mockConfigNotAnswer },
},
iniDataInCredentials: {},
profile: "foo",
Expand Down Expand Up @@ -129,8 +127,8 @@ describe("fromSharedConfigFiles", () => {
credentialsFile: iniDataInCredentials,
});
(getProfileName as jest.Mock).mockReturnValueOnce(profile ?? "default");
return expect(fromSharedConfigFiles(configGetter, { profile, preferredFile })()).rejects.toMatchObject(
getCredentialsProviderError(profile ?? "default", configGetter)
return expect(fromSharedConfigFiles(configGetter, { profile, preferredFile })()).rejects.toEqual(
getCredentialsProviderError(profile ?? "default")
);
});
});
Expand All @@ -144,18 +142,18 @@ describe("fromSharedConfigFiles", () => {
configFile: {},
credentialsFile: {},
});
return expect(fromSharedConfigFiles(failGetter)()).rejects.toMatchObject(new CredentialsProviderError(message));
return expect(fromSharedConfigFiles(failGetter)()).rejects.toEqual(new CredentialsProviderError(message));
});
});

describe("profile", () => {
const loadedConfigData = {
configFile: {
default: { [configKey]: "configFileDefault" },
foo: { [configKey]: "configFileFoo" },
default: { [CONFIG_KEY]: "configFileDefault" },
foo: { [CONFIG_KEY]: "configFileFoo" },
},
credentialsFile: {
default: { [configKey]: "credentialsFileDefault" },
default: { [CONFIG_KEY]: "credentialsFileDefault" },
},
};

Expand All @@ -166,7 +164,7 @@ describe("fromSharedConfigFiles", () => {
it.each(["foo", "default"])("returns config value from %s profile", (profile) => {
(getProfileName as jest.Mock).mockReturnValueOnce(profile);
return expect(fromSharedConfigFiles(configGetter)()).resolves.toBe(
loadedConfigData.configFile[profile][configKey]
loadedConfigData.configFile[profile][CONFIG_KEY]
);
});
});
Expand Down
6 changes: 4 additions & 2 deletions packages/node-config-provider/src/fromSharedConfigFiles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { CredentialsProviderError } from "@smithy/property-provider";
import { getProfileName, loadSharedConfigFiles, SourceProfileInit } from "@smithy/shared-ini-file-loader";
import { ParsedIniData, Profile, Provider } from "@smithy/types";

import { getSelectorName } from "./getSelectorName";

export interface SharedConfigInit extends SourceProfileInit {
/**
* The preferred shared ini file to load the config. "config" option refers to
Expand Down Expand Up @@ -41,8 +43,8 @@ export const fromSharedConfigFiles =
return configValue;
} catch (e) {
throw new CredentialsProviderError(
e.message ||
`Cannot load config for profile ${profile} in SDK configuration files with getter: ${configSelector}`
e.message || `Not found in config files w/ profile [${profile}]: ${getSelectorName(configSelector.toString())}`,
{ logger: init.logger }
);
}
};
19 changes: 19 additions & 0 deletions packages/node-config-provider/src/getSelectorName.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* Attempts to extract the name of the variable that the functional selector is looking for.
* Improves readability over the raw Function.toString() value.
* @internal
* @param functionString - function's string representation.
*
* @returns constant value used within the function.
*/
export function getSelectorName(functionString: string): string {
kuhe marked this conversation as resolved.
Show resolved Hide resolved
try {
const constants = new Set(Array.from(functionString.match(/([A-Z_]){3,}/g) ?? []));
constants.delete("CONFIG");
constants.delete("CONFIG_PREFIX_SEPARATOR");
constants.delete("ENV");
return [...constants].join(", ");
} catch (e) {
return functionString;
}
}
Loading
Loading