added awsRequest for aws-sdk call backoff and retry in customResources #8338

pratik-vii · 2020-10-02T19:42:39Z

Closes: #8333

codecov-commenter · 2020-10-02T19:44:51Z

Codecov Report

Merging #8338 into master will increase coverage by 0.13%.
The diff coverage is 9.23%.

@@            Coverage Diff             @@
##           master    #8338      +/-   ##
==========================================
+ Coverage   88.01%   88.15%   +0.13%     
==========================================
  Files         249      249              
  Lines        9305     9300       -5     
==========================================
+ Hits         8190     8198       +8     
+ Misses       1115     1102      -13

Impacted Files	Coverage Δ
...rces/resources/apiGatewayCloudWatchRole/handler.js	`0.00% <0.00%> (ø)`
...urces/resources/cognitoUserPool/lib/permissions.js	`0.00% <0.00%> (ø)`
...esources/resources/cognitoUserPool/lib/userPool.js	`0.00% <0.00%> (ø)`
...Resources/resources/eventBridge/lib/eventBridge.js	`0.00% <0.00%> (ø)`
...Resources/resources/eventBridge/lib/permissions.js	`0.00% <0.00%> (ø)`
...ins/aws/customResources/resources/s3/lib/bucket.js	`0.00% <0.00%> (ø)`
...ws/customResources/resources/s3/lib/permissions.js	`0.00% <0.00%> (ø)`
lib/plugins/aws/customResources/resources/utils.js	`24.52% <30.00%> (+3.31%)`	⬆️
lib/plugins/aws/provider/awsProvider.js	`93.04% <0.00%> (ø)`
...kage/compile/events/apiGateway/lib/method/index.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 7e4871b...5875539. Read the comment docs.

medikoo

@pratik-vii thanks for giving it a spin.

In given form this change is not effective. Note, that no custom resource logic was configured to new utility. We need to ensure that all of the functions rely on this utility, instead of invoking aws-sdk directly.

Also this implementation does not look complete. I believe serviceInstance needs to be resolved on spot, as it's here: https://github.com/serverless/test/blob/7dfc92e1d0d93c91f1c9affc872afa41fcb1c89b/aws-request.js

pratik-vii · 2020-10-05T13:37:24Z

@medikoo

In given form this change is not effective

I was planning to integrate it once signature is approved, I had doubt to whether to keep aws-sdk instantiation in resource or utility itself. Which you answered already!

I believe serviceInstance needs to be resolved on spot, as it's here:

Sure. Will do. I was thinking of doing the same pattern, but I thought to give responsibility of configuring the aws-sdk instance to resource.

…o aws-request-util

…tom resource

medikoo

Thank you @pratik-vii for update. It looks way better now.

Please see my comments. Also please test it by running integration tests, it's the only way to confirm, we did not break anything with this refactor

medikoo · 2020-10-06T08:06:20Z

lib/plugins/aws/customResources/resources/apiGatewayCloudWatchRole/handler.js

-  const apiGateway = new ApiGateway({ region });
-  const assignedRoleArn = (await apiGateway.getAccount().promise()).cloudwatchRoleArn;
+  const assignedRoleArn = (
+    await awsRequest({ name: 'APIGateway', params: { region } }, 'getAccount').promise()


awsRequest should return promise directly, there should be no need for promise()

medikoo · 2020-10-06T08:06:55Z

lib/plugins/aws/customResources/resources/utils.js

+  }
+);
+
+function awsRequest(service, method, ...args) {


Let's use async/await

medikoo · 2020-10-06T08:07:22Z

lib/plugins/aws/customResources/resources/utils.js

@@ -1,5 +1,7 @@
 'use strict';

+const AWS = require('aws-sdk');
+const _ = require('lodash');


We should not introduce any dependencies here (note this is packaged for lambda, and we want to avoid npm install)

Yeah. Will remove the usage of lodash.

- changed awsRequest to async - removed unnecessary promise in awsRequest

pratik-vii · 2020-10-06T16:58:29Z

@medikoo Thanks for the feedback.

I have updated the PR,

Removed usage of lodash
Changed awsRequest function to async function

I have tested the changes with dependent integration testcases
apiGateway.test.js
cognitoUserPool.test.js
eventBridge.test.js
s3.test.js

medikoo

Thank you @pratik-vii , that looks very good! I have just few minor suggestions. Let me know what you think

medikoo · 2020-10-07T07:29:01Z

lib/plugins/aws/customResources/resources/apiGatewayCloudWatchRole/handler.js

-        // temporary state where just created AWS role is not being ready for use (yet)
-        await wait(10000);
-        return updateAccount(++counter);
-      }


I think we need to keep that try/catch, as in this case we want to catch also non-retryable errors, while awsRequest retries only if error is retryable

medikoo · 2020-10-07T07:29:19Z

lib/plugins/aws/customResources/resources/utils.js

+            requestTry < MAX_AWS_REQUEST_TRY &&
+            error &&
+            (error.statusCode === 429 ||
+              (error.retryable && error.statusCode !== 403 && error.code !== 'CredentialsError'))


I think we can limit this to (error.statusCode === 429 || error.retryable), as in Lambda we should not expect credential errors, so it's not the case to consider

medikoo · 2020-10-07T07:30:49Z

lib/plugins/aws/customResources/resources/utils.js

+            (error.statusCode === 429 ||
+              (error.retryable && error.statusCode !== 403 && error.code !== 'CredentialsError'))
+          ) {
+            return wait(4000 + Math.random() * 3000).then(() => callAws(++requestTry));


7 seconds, is a lot in lambda, let's make it maybe 2000 + Math.random() * 1000, and maybe increase DEFAULT_MAX_AWS_REQUEST_TRY to 8

medikoo · 2020-10-07T07:31:18Z

lib/plugins/aws/customResources/resources/utils.js

+  const callAws = requestTry => {
+    return serviceInstance[method](...args)
+      .promise()
+      .then(


Let's use async/await

…o aws-request-util

- changed backoff time to (2000 + Math.random() * 1000)

medikoo

Thank youi @pratik-vii ! That looks great

added awsRequest for aws-sdk call backoff and retry in customResources

a025816

update for prettier

49b9f15

pratik-vii marked this pull request as draft October 2, 2020 20:34

using existing env variable SLS_AWS_REQUEST_MAX_RETRIES

cbafc9c

pratik-vii marked this pull request as ready for review October 3, 2020 06:02

medikoo suggested changes Oct 5, 2020

View reviewed changes

medikoo assigned pratik-vii Oct 5, 2020

medikoo added cat/deployment enhancement labels Oct 5, 2020

pratik-vii marked this pull request as draft October 5, 2020 13:45

pratik-vii added 7 commits October 5, 2020 20:38

added functionality to resolve aws service instance in awsRequest

8f0b61d

integrated awsRequest with s3 custom resource

2c43a32

integrated awsRequest with Event Bridge custom resource

53d52aa

Merge branch 'master' of https://github.com/serverless/serverless int…

0a0725b

…o aws-request-util

integrated awsRequest with CognitoUserPool custom resource

d2afc2c

integrated awsRequest with APIGatewayCloudWatchRole custom resource

8a4523b

fix case of APIGateway for awsRequest in APIGatewayCloudWatchRole cus…

ff2a285

…tom resource

pratik-vii requested a review from medikoo October 5, 2020 15:53

pratik-vii marked this pull request as ready for review October 5, 2020 15:53

fix case of APIGateway for awsRequest in APIGatewayCloudWatchRole cus…

3d1a697

…tom resource

medikoo suggested changes Oct 6, 2020

View reviewed changes

pratik-vii added 2 commits October 6, 2020 19:41

removed lodash dependency

ae40f38

changed awsRequest to async and removed unnecessary promise

5875539

- changed awsRequest to async - removed unnecessary promise in awsRequest

pratik-vii requested a review from medikoo October 6, 2020 16:58

medikoo suggested changes Oct 7, 2020

View reviewed changes

pratik-vii added 2 commits October 7, 2020 13:35

reverted changes of try/catch for APIGateway update account

803324d

Merge branch 'master' of https://github.com/serverless/serverless int…

646043c

…o aws-request-util

added async/await for awsRequest

ea0863f

- changed backoff time to (2000 + Math.random() * 1000)

pratik-vii requested a review from medikoo October 7, 2020 10:05

medikoo approved these changes Oct 7, 2020

View reviewed changes

medikoo merged commit a3ebc01 into serverless:master Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added awsRequest for aws-sdk call backoff and retry in customResources #8338

added awsRequest for aws-sdk call backoff and retry in customResources #8338

pratik-vii commented Oct 2, 2020 •

edited by medikoo

codecov-commenter commented Oct 2, 2020 •

edited

medikoo left a comment

pratik-vii commented Oct 5, 2020 •

edited

medikoo left a comment

medikoo Oct 6, 2020

medikoo Oct 6, 2020

medikoo Oct 6, 2020

pratik-vii Oct 6, 2020

pratik-vii commented Oct 6, 2020

medikoo left a comment

medikoo Oct 7, 2020

medikoo Oct 7, 2020

medikoo Oct 7, 2020

medikoo Oct 7, 2020

medikoo left a comment

Navigation Menu

added awsRequest for aws-sdk call backoff and retry in customResources #8338

added awsRequest for aws-sdk call backoff and retry in customResources #8338

Conversation

pratik-vii commented Oct 2, 2020 • edited by medikoo

codecov-commenter commented Oct 2, 2020 • edited

Codecov Report

medikoo left a comment

Choose a reason for hiding this comment

pratik-vii commented Oct 5, 2020 • edited

medikoo 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

pratik-vii commented Oct 6, 2020

medikoo 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

medikoo left a comment

Choose a reason for hiding this comment

pratik-vii commented Oct 2, 2020 •

edited by medikoo

codecov-commenter commented Oct 2, 2020 •

edited

pratik-vii commented Oct 5, 2020 •

edited