Skip to content

Conversation

@deekthesqueak
Copy link
Contributor

The AWS SDK has the ability to set the KMSKeyArn when creating a Lambda function.
https://docs.aws.amazon.com/lambda/latest/dg/API_CreateFunction.html

This PR adds the ability to set the KMSKeyArn. By default the value is not included in the params.

'Runtime': params.Runtime,
'VpcConfig': params.VpcConfig,
'Environment': params.Environment,
'KMSKeyArn': params.KMSKeyArn,
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I wasn't sure if I should include a conditional check here or not to add KMSKeyArn to the params.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This is a possible way to conditionally add it:

diff --git a/lib/main.js b/lib/main.js index 3f5735e..9791c7e 100644 --- a/lib/main.js+++ b/lib/main.js@@ -466,7 +466,7 @@ Lambda.prototype._uploadExisting = (lambda, params) =>{}, (err) =>{if (err) return reject(err) - lambda.updateFunctionConfiguration({+ lambda.updateFunctionConfiguration(Object.assign({ 'FunctionName': params.FunctionName, 'Description': params.Description, 'Handler': params.Handler, @@ -476,10 +476,10 @@ Lambda.prototype._uploadExisting = (lambda, params) =>{'Runtime': params.Runtime, 'VpcConfig': params.VpcConfig, 'Environment': params.Environment, - 'KMSKeyArn': params.KMSKeyArn, 'DeadLetterConfig': params.DeadLetterConfig, 'TracingConfig': params.TracingConfig - }, (err, data) =>{+ }, (typeof params.KMSKeyArn !== undefined) ?{'KMSKeyArn': params.KMSKeyArn}:{}),+ (err, data) =>{ if (err) return reject(err) resolve(data) })

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, the only way to find that out is to test it. Sometimes the AWS SDK makes a problem from passing undefined parameters. I think the way your patch does it is the "safe" way and I would recommend updating it to that.

@DeviaVirDeviaVir self-requested a review July 19, 2017 11:53
constDEPLOY_TIMEOUT=process.env.DEPLOY_TIMEOUT||120000
constDOCKER_IMAGE=process.env.DOCKER_IMAGE||''
constDEPLOY_ZIPFILE=process.env.DEPLOY_ZIPFILE||''
constAWS_KMS_KEY_ARN=process.env.AWS_KMS_KEY_ARN||''
Copy link
ContributorAuthor

@deekthesqueakdeekthesqueakJul 20, 2017

Choose a reason for hiding this comment

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

KMSKeyArn will only clear with an empty string. If left undefined no change will be processed. With this change not including the environment variable will clear it.

There isn't a need to conditionally add it.

Copy link
Collaborator

@DeviaVirDeviaVir left a comment

Choose a reason for hiding this comment

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

👍 thanks for taking to time to add this!

@DeviaVirDeviaVir merged commit 1472843 into motdotla:masterJul 20, 2017
@joshwiens
Copy link

joshwiens commented Sep 22, 2017

@DeviaVir - Any chance you could tag / publish a minor off of master, we could of course run off a hash from git but in this case the addition above is going straight to prod & I would prefer to not have to go the git repo route as a version in a prod app. Thanks :)

@DeviaVir
Copy link
Collaborator

@d3viant0ne yep, I'll publish a tag today.

@DeviaVirDeviaVir mentioned this pull request Sep 22, 2017
@DeviaVir
Copy link
Collaborator

@d3viant0ne https://www.npmjs.com/package/node-lambda0.11.4 now available.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@deekthesqueak@joshwiens@DeviaVir