From 13e525ad71276a346c02e275cf4f2e250b667367 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Domaga=C5=82a?= Date: Tue, 18 Sep 2018 19:45:35 +0200 Subject: [PATCH 1/4] Fail lambda if shouldFail flag provided --- src/middlewares/__tests__/secretsManager.js | 53 ++++++++++++++++++++- src/middlewares/secretsManager.js | 4 ++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/middlewares/__tests__/secretsManager.js b/src/middlewares/__tests__/secretsManager.js index c6f6951cd..86896c3e3 100644 --- a/src/middlewares/__tests__/secretsManager.js +++ b/src/middlewares/__tests__/secretsManager.js @@ -52,6 +52,7 @@ describe('🔒 SecretsManager Middleware', () => { promise = promise.then(() => { return new Promise((resolve, reject) => { handler(event, context, (error, response) => { + if (error) reject(error) try { cb(error, {event, context, response}) resolve() @@ -68,7 +69,7 @@ describe('🔒 SecretsManager Middleware', () => { } }) }) - promise.then(done).catch(err => done(err)) + promise.then(done).catch(done) } test(`It should set secrets to context`, (done) => { @@ -217,6 +218,56 @@ describe('🔒 SecretsManager Middleware', () => { }) }) + test(`It should fail if "shouldFail" flag provided and call failed`, (done) => { + const errorMessage = 'Internal Error / Secret doesn\'t exist' + getSecretValueMock.mockReturnValueOnce({ + promise: () => Promise.reject(new Error(errorMessage)) + }) + + const errHandler = err => { + getSecretValueMock.mockClear() + expect(err.message).toEqual(errorMessage) + done() + } + return testScenario({ + mockResponse: {}, + middlewareOptions: { + secrets: { + KEY_NAME: 'failed_call' + }, + shouldFail: true + }, + callbacks: [ + (_, {context}) => { + throw new Error('Not supposed to be called') + } + ], + done: errHandler + }) + }) + + test(`It should resolve if "shouldFail" flag not provided and call failed`, (done) => { + const errorMessage = 'Internal Error / Secret doesn\'t exist' + getSecretValueMock.mockReturnValueOnce({ + promise: () => Promise.reject(new Error(errorMessage)) + }) + return testScenario({ + mockResponse: {}, + middlewareOptions: { + secrets: { + KEY_NAME: 'failed_call' + } + }, + callbacks: [ + (_, {context}) => { + expect(getSecretValueMock).toBeCalled() + getSecretValueMock.mockClear() + } + ], + done + }) + }) + test(`It should only refresh once per cache expiry window`, (done) => { // with cache expiry of 50ms, test what happens when one refresh fails, and // that the middleware doesn't retry for another 50ms diff --git a/src/middlewares/secretsManager.js b/src/middlewares/secretsManager.js index 84e8d9e3e..bd70a1c44 100644 --- a/src/middlewares/secretsManager.js +++ b/src/middlewares/secretsManager.js @@ -4,6 +4,7 @@ module.exports = opts => { const defaults = { awsSdkOptions: {}, secrets: {}, // e.g. { RDS_SECRET: 'dev/rds_login', API_SECRET: '...' } + shouldFail: false, cache: false, cacheExpiryInMillis: undefined, secretsLoaded: false, @@ -57,6 +58,9 @@ module.exports = opts => { // if we already have a cached secrets, then reset the timestamp so we don't // keep retrying on every invocation which can cause performance problems // when there's temporary problems with Secrets Manager + if (options.shouldFail) { + throw err + } if (options.secretsCache) { options.secretsLoadedAt = new Date() } From 305c6ba7174235c0aaba4263c4fec9a3a5492a65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Domaga=C5=82a?= Date: Tue, 18 Sep 2018 19:55:32 +0200 Subject: [PATCH 2/4] add docs --- README.md | 1 + README.md.hb | 1 + docs/middlewares.md | 1 + 3 files changed, 3 insertions(+) diff --git a/README.md b/README.md index 6a726b7b5..f056311c1 100644 --- a/README.md +++ b/README.md @@ -515,6 +515,7 @@ Currently available middlewares: - [`jsonBodyParser`](/docs/middlewares.md#jsonbodyparser): Automatically parses HTTP requests with JSON body and converts the body into an object. Also handles gracefully broken JSON if used in combination of `httpErrorHandler`. - [`s3KeyNormalizer`](/docs/middlewares.md#s3keynormalizer): Normalizes key names in s3 events. + - [`Secrets Manager`](/docs/middlewares.md#secretsmanager): Fetches parameters from AWS Secrets Manager. - [`ssm`](/docs/middlewares.md#ssm): Fetches parameters from [AWS Systems Manager Parameter Store](https://docs.aws.amazon.com/systems-manager/latest/userguide/systems-manager-paramstore.html). - [`validator`](/docs/middlewares.md#validator): Automatically validates incoming events and outgoing responses against custom schemas - [`urlEncodeBodyParser`](/docs/middlewares.md#urlencodebodyparser): Automatically parses HTTP requests with URL encoded body (typically the result of a form submit). diff --git a/README.md.hb b/README.md.hb index d35dcb0c4..4ab64468a 100644 --- a/README.md.hb +++ b/README.md.hb @@ -515,6 +515,7 @@ Currently available middlewares: - [`jsonBodyParser`](/docs/middlewares.md#jsonbodyparser): Automatically parses HTTP requests with JSON body and converts the body into an object. Also handles gracefully broken JSON if used in combination of `httpErrorHandler`. - [`s3KeyNormalizer`](/docs/middlewares.md#s3keynormalizer): Normalizes key names in s3 events. + - [`Secrets Manager`](/docs/middlewares.md#secretsmanager): Fetches parameters from AWS Secrets Manager. - [`ssm`](/docs/middlewares.md#ssm): Fetches parameters from [AWS Systems Manager Parameter Store](https://docs.aws.amazon.com/systems-manager/latest/userguide/systems-manager-paramstore.html). - [`validator`](/docs/middlewares.md#validator): Automatically validates incoming events and outgoing responses against custom schemas - [`urlEncodeBodyParser`](/docs/middlewares.md#urlencodebodyparser): Automatically parses HTTP requests with URL encoded body (typically the result of a form submit). diff --git a/docs/middlewares.md b/docs/middlewares.md index 2a198c7cf..a991e675d 100644 --- a/docs/middlewares.md +++ b/docs/middlewares.md @@ -487,6 +487,7 @@ For each secret, you also provide the name under which its value should be added - `secrets` (object) : Map of secrets to fetch from Secrets Manager, where the key is the destination, and value is secret name in Secrets Manager. Example: `{secrets: {RDS_LOGIN: 'dev/rds_login'}}` - `awsSdkOptions` (object) (optional): Options to pass to AWS.SecretsManager class constructor. +- `shouldFail` (boolean) (optional): Defaults to `false`. Set it to `true` if you want your lambda to fail in case call to AWS Secrets Manager fails (secrets don't exist or internal error) NOTES: * Lambda is required to have IAM permission for `secretsmanager:GetSecretValue` action From f522e13a34b7b02370a79409a888e78e88832556 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Domaga=C5=82a?= Date: Tue, 18 Sep 2018 20:56:12 +0200 Subject: [PATCH 3/4] review remarks: changed name of flag, updated tests --- docs/middlewares.md | 2 +- src/middlewares/__tests__/secretsManager.js | 10 +++++----- src/middlewares/secretsManager.js | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/middlewares.md b/docs/middlewares.md index a991e675d..22ff5917a 100644 --- a/docs/middlewares.md +++ b/docs/middlewares.md @@ -487,7 +487,7 @@ For each secret, you also provide the name under which its value should be added - `secrets` (object) : Map of secrets to fetch from Secrets Manager, where the key is the destination, and value is secret name in Secrets Manager. Example: `{secrets: {RDS_LOGIN: 'dev/rds_login'}}` - `awsSdkOptions` (object) (optional): Options to pass to AWS.SecretsManager class constructor. -- `shouldFail` (boolean) (optional): Defaults to `false`. Set it to `true` if you want your lambda to fail in case call to AWS Secrets Manager fails (secrets don't exist or internal error) +- `throwOnFailedCall` (boolean) (optional): Defaults to `false`. Set it to `true` if you want your lambda to fail in case call to AWS Secrets Manager fails (secrets don't exist or internal error) NOTES: * Lambda is required to have IAM permission for `secretsmanager:GetSecretValue` action diff --git a/src/middlewares/__tests__/secretsManager.js b/src/middlewares/__tests__/secretsManager.js index 86896c3e3..ebf16bb2f 100644 --- a/src/middlewares/__tests__/secretsManager.js +++ b/src/middlewares/__tests__/secretsManager.js @@ -52,7 +52,7 @@ describe('🔒 SecretsManager Middleware', () => { promise = promise.then(() => { return new Promise((resolve, reject) => { handler(event, context, (error, response) => { - if (error) reject(error) + if (error) return reject(error) try { cb(error, {event, context, response}) resolve() @@ -218,7 +218,7 @@ describe('🔒 SecretsManager Middleware', () => { }) }) - test(`It should fail if "shouldFail" flag provided and call failed`, (done) => { + test(`It should fail if "throwOnFailedCall" flag provided and call failed`, (done) => { const errorMessage = 'Internal Error / Secret doesn\'t exist' getSecretValueMock.mockReturnValueOnce({ promise: () => Promise.reject(new Error(errorMessage)) @@ -235,10 +235,10 @@ describe('🔒 SecretsManager Middleware', () => { secrets: { KEY_NAME: 'failed_call' }, - shouldFail: true + throwOnFailedCall: true }, callbacks: [ - (_, {context}) => { + () => { throw new Error('Not supposed to be called') } ], @@ -246,7 +246,7 @@ describe('🔒 SecretsManager Middleware', () => { }) }) - test(`It should resolve if "shouldFail" flag not provided and call failed`, (done) => { + test(`It should resolve if "throwOnFailedCall" flag not provided and call failed`, (done) => { const errorMessage = 'Internal Error / Secret doesn\'t exist' getSecretValueMock.mockReturnValueOnce({ promise: () => Promise.reject(new Error(errorMessage)) diff --git a/src/middlewares/secretsManager.js b/src/middlewares/secretsManager.js index bd70a1c44..47ab9dc5a 100644 --- a/src/middlewares/secretsManager.js +++ b/src/middlewares/secretsManager.js @@ -4,7 +4,7 @@ module.exports = opts => { const defaults = { awsSdkOptions: {}, secrets: {}, // e.g. { RDS_SECRET: 'dev/rds_login', API_SECRET: '...' } - shouldFail: false, + throwOnFailedCall: false, cache: false, cacheExpiryInMillis: undefined, secretsLoaded: false, @@ -58,7 +58,7 @@ module.exports = opts => { // if we already have a cached secrets, then reset the timestamp so we don't // keep retrying on every invocation which can cause performance problems // when there's temporary problems with Secrets Manager - if (options.shouldFail) { + if (options.throwOnFailedCall) { throw err } if (options.secretsCache) { From d199f48a685c8afb4bb5dfea69e2aa2092a2246c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Domaga=C5=82a?= Date: Wed, 19 Sep 2018 00:35:19 +0200 Subject: [PATCH 4/4] return from cache if call failed but secrets are available --- docs/middlewares.md | 2 +- src/middlewares/__tests__/secretsManager.js | 36 +++++++++++++++++++++ src/middlewares/secretsManager.js | 3 +- 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/docs/middlewares.md b/docs/middlewares.md index 22ff5917a..dd9e7a687 100644 --- a/docs/middlewares.md +++ b/docs/middlewares.md @@ -487,7 +487,7 @@ For each secret, you also provide the name under which its value should be added - `secrets` (object) : Map of secrets to fetch from Secrets Manager, where the key is the destination, and value is secret name in Secrets Manager. Example: `{secrets: {RDS_LOGIN: 'dev/rds_login'}}` - `awsSdkOptions` (object) (optional): Options to pass to AWS.SecretsManager class constructor. -- `throwOnFailedCall` (boolean) (optional): Defaults to `false`. Set it to `true` if you want your lambda to fail in case call to AWS Secrets Manager fails (secrets don't exist or internal error) +- `throwOnFailedCall` (boolean) (optional): Defaults to `false`. Set it to `true` if you want your lambda to fail in case call to AWS Secrets Manager fails (secrets don't exist or internal error). It will only print error if secrets are already cached. NOTES: * Lambda is required to have IAM permission for `secretsmanager:GetSecretValue` action diff --git a/src/middlewares/__tests__/secretsManager.js b/src/middlewares/__tests__/secretsManager.js index ebf16bb2f..9c3503557 100644 --- a/src/middlewares/__tests__/secretsManager.js +++ b/src/middlewares/__tests__/secretsManager.js @@ -268,6 +268,42 @@ describe('🔒 SecretsManager Middleware', () => { }) }) + test(`It should resolve if "throwOnFailedCall" flag provided but item already cached`, (done) => { + const errorMessage = 'Internal Error / Secret doesn\'t exist' + return testScenario({ + mockResponse: { + SecretString: JSON.stringify({Username: 'username', Password: 'password'}) + }, + middlewareOptions: { + secrets: { + KEY_NAME: 'rds_key' + }, + throwOnFailedCall: true + }, + callbacks: [ + // invocation 1: fetched + (_, {context}) => { + hasRDSLogin(context) + expect(getSecretValueMock).toBeCalled() + + getSecretValueMock.mockClear() + + // set up next attempt to fail + getSecretValueMock.mockReturnValueOnce({ + promise: () => Promise.reject(new Error(errorMessage)) + }) + }, + // invocation 2: failed but content taken from cache + (_, {context}) => { + hasRDSLogin(context) + expect(getSecretValueMock).toBeCalled() + getSecretValueMock.mockClear() + } + ], + done + }) + }) + test(`It should only refresh once per cache expiry window`, (done) => { // with cache expiry of 50ms, test what happens when one refresh fails, and // that the middleware doesn't retry for another 50ms diff --git a/src/middlewares/secretsManager.js b/src/middlewares/secretsManager.js index 47ab9dc5a..3cd053fc6 100644 --- a/src/middlewares/secretsManager.js +++ b/src/middlewares/secretsManager.js @@ -58,9 +58,10 @@ module.exports = opts => { // if we already have a cached secrets, then reset the timestamp so we don't // keep retrying on every invocation which can cause performance problems // when there's temporary problems with Secrets Manager - if (options.throwOnFailedCall) { + if (options.throwOnFailedCall && !options.secretsCache) { throw err } + if (options.secretsCache) { options.secretsLoadedAt = new Date() }