feat: Support `kmsKeyArn` for `deploy function` #8697

ifitzsimmons · 2020-12-31T05:31:50Z

Closes: #8692: provider.kmsKeyArn and functions[].kmsKeyArn are not supported with single function deployment}

codecov · 2020-12-31T06:04:05Z

Codecov Report

Merging #8697 (88a85e9) into master (dc9f180) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8697      +/-   ##
==========================================
- Coverage   87.73%   87.70%   -0.03%     
==========================================
  Files         264      264              
  Lines        9851     9852       +1     
==========================================
- Hits         8643     8641       -2     
- Misses       1208     1211       +3

Impacted Files	Coverage Δ
lib/plugins/aws/deployFunction.js	`95.53% <100.00%> (+0.67%)`	⬆️
lib/Serverless.js	`92.24% <0.00%> (-3.11%)`	⬇️
lib/utils/createFromTemplate.js	`84.84% <0.00%> (-2.00%)`	⬇️
lib/plugins/interactiveCli/initializeService.js	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc9f180...88a85e9. Read the comment docs.

pgrzesik

Hello @ifitzsimmons, thanks for submitting your PR 🙇 There are a few remarks that I believe should be addressed before finalizing this PR, if anything is not clear please let me know 💯

pgrzesik · 2021-01-04T10:41:05Z

lib/plugins/aws/deployFunction.js

+      kmsKeyArn = !kmsKeyArn ? functionObj.awsKmsKeyArn : kmsKeyArn;
+
+      this.serverless._logDeprecation(
+        'FUNCTION_DEPRECATED_PROPERTY_AWS_KMS_KEY_ARN',


Here we should use AWS_KMS_KEY_ARN code so it constructs a proper link to deprecation documentation

I think we could also stick to the exact same deprecation message as we have during sls deploy: Starting with next major version, "awsKmsKeyArn" function property will be replaced by "kmsKeyArn"

pgrzesik · 2021-01-04T10:41:12Z

lib/plugins/aws/deployFunction.js

+      kmsKeyArn = !kmsKeyArn ? serviceObj.awsKmsKeyArn : kmsKeyArn;
+
+      this.serverless._logDeprecation(
+        'SERVICE_DEPRECATED_PROPERTY_AWS_KMS_KEY_ARN',


Here we should use AWS_KMS_KEY_ARN code so it constructs a proper link to deprecation documentation

pgrzesik · 2021-01-04T10:45:57Z

lib/plugins/aws/deployFunction.js

-    } else if (serviceObj && 'awsKmsKeyArn' in serviceObj && !_.isObject(serviceObj.awsKmsKeyArn)) {
-      params.KMSKeyArn = serviceObj.awsKmsKeyArn;
+    let kmsKeyArn = functionObj.kmsKeyArn || providerObj.kmsKeyArn;
+    if (functionObj.awsKmsKeyArn) {


What is your opinion on moving the deprecation checks to deploy:function:initialize hook? We use similar approach in https://github.com/serverless/serverless/blob/master/lib/plugins/aws/package/compile/functions.js#L33-L63 which I believe helps with readability and nicely separates deprecation checks with the actual logic

It looks good. A couple of questions:

Can I modify the if checks?

- if (_.get(this.serverless.service.serviceObject, 'awsKmsKeyArn')) + if (this.serverless.service.serviceObject.awsKmsKeyArn) - if (Object.values(this.serverless.service.functions).some(({ awsKmsKeyArn }) => awsKmsKeyArn)) + if (this.options.functionObj.awsKmsKeyArn)

Will the awsKmsKeyArn property ever exist on anything other than the functionObj? If so, I understand the use of the .some() method. Otherwise, we can probably just check functionObj for that property directly. I guess unless functionObj is not guaranteed to exist. Also, why is that function checking the serverless.service.functions object? It looks like the deployFunction class has a functions object on the class's options attribute.

And then just a general question -- when does the hook run?

As for when do hooks run - they're invoked as a part of deploy command, you can see the logic here:

serverless/lib/plugins/deploy.js

Line 58 in b093609

function: {

As for checks:

the use of some is there due to the fact that in functions.js we're considering all functions - here it will be enough if we check only on the one that is considered for deploy

the check for awsKmsKeyArn on serviceObject - here I'm not 100% if there's a use case where where the serviceObject could be empty (I'm not aware of it), but for now I would stick to _.get use - we have a separate issue where we migrate lodash use and we would be able to address it at a later point in time

pgrzesik · 2021-01-04T10:47:45Z

test/unit/lib/plugins/aws/deployFunction.test.js

      awsDeployFunction.options = options;

      await awsDeployFunction.updateFunctionConfiguration();

      expect(normalizeArnRoleStub.calledOnce).to.be.equal(true);
      expect(normalizeArnRoleStub.calledWithExactly('arn:aws:iam::123456789012:role/Admin'));
      expect(updateFunctionConfigurationStub.calledOnce).to.be.equal(true);
-      expect(
-        updateFunctionConfigurationStub.calledWithExactly('Lambda', 'updateFunctionConfiguration', {
+      sinon.assert.calledWith(


I think we could skip these assertions on deprecations - we usually don't do them and it's also expanding a test that is written in "old-way"

I see that we still have that test expanded, is there a reason for that?

pgrzesik · 2021-01-04T10:50:03Z

test/unit/lib/plugins/aws/deployFunction.test.js

@@ -421,6 +430,218 @@ describe('AwsDeployFunction', () => {
        })
      ).to.be.equal(true);
    });
+
+    describe('#KMSKeyArn should be set by "kmsKeyArn" or "awsKmsKeyArn"', () => {


It would be great if all newly introduced tests will be configured with runServerless util as described: https://github.com/serverless/serverless/blob/master/test/README.md#unit-tests

I've been going through the documentation for a little while now and I'm a little embarrassed but I just don't get it. Do you think you could provide me an example for one of these tests and I can take that and run with it?

No worries 🙌 You can find some examples here: https://github.com/serverless/serverless/blob/master/test/unit/lib/plugins/aws/package/compile/functions.test.js#L1444

The general idea behind runServerless is to test outputs/results of certain functionalities instead of testing internal implementation, as it often makes it harder to refactor at later point in time. In case of deployFunction use of runServerless might be a bit tricky, as you'll most likely have to mock AWS calls. You can see an example of how single function deploy can be tested with runServerless - https://github.com/serverless/serverless/pull/8315/files

Please let me know if I can help more here 💯

Let's move them to a top-level describe block in this test file as is suggested in https://github.com/serverless/serverless/blob/master/test/README.md#unit-tests

You might also want to rebase on top of master, because I believe we gained a few tests that rely on runServerless in this module in the meantime

ifitzsimmons · 2021-01-18T22:18:31Z

@pgrzesik Are you waiting on me for anything regarding this PR?

pgrzesik

Thank you @ifitzsimmons and sorry about the delay - I explained the reasons in another PR, please request for re-review after submitting changes and I'll make sure to review as soon as possible. Overall it looks good, I only have a few minor comments, please let me know what do you think 🙇

pgrzesik · 2021-01-19T09:15:15Z

test/unit/lib/plugins/aws/deployFunction.test.js

      awsDeployFunction.options = options;

      await awsDeployFunction.updateFunctionConfiguration();

      expect(normalizeArnRoleStub.calledOnce).to.be.equal(true);
      expect(normalizeArnRoleStub.calledWithExactly('arn:aws:iam::123456789012:role/Admin'));
      expect(updateFunctionConfigurationStub.calledOnce).to.be.equal(true);
-      expect(
-        updateFunctionConfigurationStub.calledWithExactly('Lambda', 'updateFunctionConfiguration', {
+      sinon.assert.calledWith(


I see that we still have that test expanded, is there a reason for that?

pgrzesik · 2021-01-19T09:17:34Z

test/unit/lib/plugins/aws/deployFunction.test.js

@@ -421,6 +430,218 @@ describe('AwsDeployFunction', () => {
        })
      ).to.be.equal(true);
    });
+
+    describe('#KMSKeyArn should be set by "kmsKeyArn" or "awsKmsKeyArn"', () => {


Let's move them to a top-level describe block in this test file as is suggested in https://github.com/serverless/serverless/blob/master/test/README.md#unit-tests

You might also want to rebase on top of master, because I believe we gained a few tests that rely on runServerless in this module in the meantime

pgrzesik · 2021-01-19T09:19:09Z

test/unit/lib/plugins/aws/deployFunction.test.js

+              },
+            },
+            provider: {
+              vpc: {


Are these extra vpc, environment, memorySize and timeout variables needed here? I think if not, let's leave them out as they're not important in this particular test case

pgrzesik

It looks good, unfortunatelly there's a conflicting file that should be resolved before merging + I added a minor suggestion for simplification.
Thanks for being patient and sorry for so many conflicts along the way 😬

pgrzesik · 2021-01-21T09:26:06Z

lib/plugins/aws/deployFunction.js

-      params.KMSKeyArn = functionObj.awsKmsKeyArn;
-    } else if (serviceObj && 'awsKmsKeyArn' in serviceObj && !_.isObject(serviceObj.awsKmsKeyArn)) {
-      params.KMSKeyArn = serviceObj.awsKmsKeyArn;
+    let kmsKeyArn = functionObj.kmsKeyArn || providerObj.kmsKeyArn;


I think this could be simplified even further with `kmsKeyArn = functionObj.kmsKeyArn || providerObjkmsKeyArn || functionObj.awsKmsKeyArn || serviceObj.awsKmsKeyArn;

pgrzesik

One last thing and we're be ready to merge, thank you @ifitzsimmons

pgrzesik · 2021-01-22T06:21:48Z

test/unit/lib/plugins/aws/deployFunction.test.js

@@ -183,6 +184,138 @@ describe('AwsDeployFunction', () => {
    });
  });

+  describe('#KMSKeyArn should be set by "kmsKeyArn" or "awsKmsKeyArn"', () => {


One last thing - let's move that describe into describe from https://github.com/serverless/serverless/pull/8697/files#diff-79623f798e186957caf38cdf7b770ba812c6a8bf7034dc4cb39abb234974e0aeR445 - we try to keep all runServerless based tests separately from the ones written the old way

`function[]` objects for single function deployment

pgrzesik

Thanks @ifitzsimmons - I believe schema needs to be cleaned up a little bit and we are ready to merge 👍

pgrzesik · 2021-01-24T14:10:08Z

lib/plugins/aws/provider.js

@@ -419,6 +419,10 @@ class AwsProvider {
              ],
            },
          },
+          awsKmsArn: {


Isn't it a duplicate of definiton from lib/configSchema.js?

pgrzesik · 2021-01-24T14:11:12Z

lib/plugins/aws/provider.js

@@ -619,6 +623,7 @@ class AwsProvider {
              },
              additionalProperties: false,
            },
+            awsKmsKeyArn: { $ref: '#/definitions/awsKmsArn' },


Is there a reason to add this? I think it's invalid property on provider and we already have kmsKeyArn defined for provider as available

pgrzesik

Thank you for your patience, well done 🙇

ifitzsimmons force-pushed the bugfix/8692-deprecate-kmsKeyArn branch from 4b91d90 to d67a0ad Compare December 31, 2020 06:01

pgrzesik suggested changes Jan 4, 2021

View reviewed changes

pgrzesik added bug cat/deployment cat/lambda labels Jan 4, 2021

pgrzesik assigned ifitzsimmons Jan 4, 2021

ifitzsimmons force-pushed the bugfix/8692-deprecate-kmsKeyArn branch from d67a0ad to fbf448e Compare January 5, 2021 19:51

pgrzesik suggested changes Jan 19, 2021

View reviewed changes

ifitzsimmons force-pushed the bugfix/8692-deprecate-kmsKeyArn branch from fbf448e to 218fbeb Compare January 19, 2021 20:45

ifitzsimmons requested a review from pgrzesik January 19, 2021 21:25

pgrzesik suggested changes Jan 21, 2021

View reviewed changes

ifitzsimmons force-pushed the bugfix/8692-deprecate-kmsKeyArn branch from 218fbeb to 9623f20 Compare January 22, 2021 01:56

ifitzsimmons requested a review from pgrzesik January 22, 2021 02:19

pgrzesik suggested changes Jan 22, 2021

View reviewed changes

8692: Add kmsKeyArn property to provider and

59ec722

`function[]` objects for single function deployment

ifitzsimmons force-pushed the bugfix/8692-deprecate-kmsKeyArn branch from 9623f20 to 59ec722 Compare January 23, 2021 17:50

ifitzsimmons requested a review from pgrzesik January 23, 2021 22:18

pgrzesik suggested changes Jan 24, 2021

View reviewed changes

Removed changes to provider.js

88a85e9

ifitzsimmons requested a review from pgrzesik January 24, 2021 16:50

pgrzesik approved these changes Jan 24, 2021

View reviewed changes

pgrzesik changed the title ~~#8692: Add kmsKeyArn property to provider and function[] objects for single function deployment~~ feat: Support kmsKeyArn for deploy function Jan 24, 2021

pgrzesik merged commit 8a92be9 into serverless:master Jan 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support `kmsKeyArn` for `deploy function` #8697

feat: Support `kmsKeyArn` for `deploy function` #8697

ifitzsimmons commented Dec 31, 2020

codecov bot commented Dec 31, 2020 •

edited

pgrzesik left a comment

pgrzesik Jan 4, 2021

pgrzesik Jan 4, 2021

pgrzesik Jan 4, 2021

pgrzesik Jan 4, 2021

ifitzsimmons Jan 4, 2021 •

edited

pgrzesik Jan 5, 2021

pgrzesik Jan 4, 2021

pgrzesik Jan 19, 2021

pgrzesik Jan 4, 2021

ifitzsimmons Jan 4, 2021

pgrzesik Jan 5, 2021

pgrzesik Jan 19, 2021

ifitzsimmons commented Jan 18, 2021

pgrzesik left a comment

pgrzesik Jan 19, 2021

pgrzesik Jan 19, 2021

pgrzesik Jan 19, 2021

pgrzesik left a comment

pgrzesik Jan 21, 2021

pgrzesik left a comment

pgrzesik Jan 22, 2021

pgrzesik left a comment

pgrzesik Jan 24, 2021

pgrzesik Jan 24, 2021

pgrzesik left a comment

feat: Support kmsKeyArn for deploy function #8697

feat: Support kmsKeyArn for deploy function #8697

Conversation

ifitzsimmons commented Dec 31, 2020

codecov bot commented Dec 31, 2020 • edited

Codecov Report

pgrzesik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifitzsimmons Jan 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifitzsimmons commented Jan 18, 2021

pgrzesik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrzesik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrzesik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrzesik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrzesik left a comment

Choose a reason for hiding this comment

feat: Support `kmsKeyArn` for `deploy function` #8697

feat: Support `kmsKeyArn` for `deploy function` #8697

codecov bot commented Dec 31, 2020 •

edited

ifitzsimmons Jan 4, 2021 •

edited