fix(amplify-provider-awscloudformation): consolidate REST API policies #6904

cjihrig · 2021-03-18T06:03:20Z

Issue #, if available: #2084

Description of changes:
This commit updates the REST API IAM policy logic. Instead of using an inline policy per REST API, the logic now combines multiple REST API policies into managed IAM policies.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov · 2021-03-18T06:17:25Z

Codecov Report

Merging #6904 (47e65c7) into master (8775152) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6904   +/-   ##
=======================================
  Coverage   56.43%   56.43%           
=======================================
  Files         445      445           
  Lines       21859    21859           
  Branches     4373     4373           
=======================================
  Hits        12336    12336           
  Misses       8745     8745           
  Partials      778      778

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 8775152...47e65c7. Read the comment docs.

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts

akshbhu · 2021-03-25T19:23:07Z

Discussed Offline , Some nits Otherwise LGTM!!
Nice job on tests @cjihrig !!

Refs: aws-amplify/amplify-cli#6904

...lify-category-api/src/__tests__/provider-utils/awscloudformation/legacy-add-resource.test.ts

...y-category-api/src/__tests__/provider-utils/awscloudformation/legacy-update-resource.test.ts

packages/amplify-cli-core/src/feature-flags/featureFlags.ts

edwardfoyle · 2021-04-02T21:36:30Z

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts

+  ]);
+}
+
+function computePolicySizeIncrease(methodLength: number, pathLength: number, nameLength: number): number {


does this compute an increase, or just the new policy size?

It computes the increase, and is added to the current policy size on line 122.

edwardfoyle · 2021-04-02T21:39:21Z

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts

+        // Initial size of 100 for version, statement, etc.
+        options.policyDocSize = 100 + policySizeIncrease;
+        options.roleCount++;
+        options.managedPolicy = createManagedPolicy(this, `${namePrefix}${options.roleCount}`, (roleName as unknown) as string);


won't this overwrite the previous policy on each iteration?

Only when a new policy needs to be created (either because it's the first iteration, or because the current policy is "full").

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts

edwardfoyle · 2021-04-02T21:48:29Z

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts

+        type: 'String',
+      });
+
+      const state: ApiGatewayPolicyCreationState = {


why does this need to be outside of the forEach closure below?

It doesn't strictly need to be. I hoisted it out of that loop to avoid creating a bunch of extra garbage objects. It could be hoisted up even further, but my thought was that the number of API gateways is probably small, but the number of paths could be a good bit larger.

edwardfoyle · 2021-04-02T21:51:12Z

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts

+    ':',
+    api,
+    '/',
+    (cdk.Fn.conditionIf('ShouldNotCreateEnvResources', 'Prod', env) as unknown) as string,


why is 'Prod' the default here?

That's the current behavior in packages/amplify-category-api/resources/awscloudformation/cloudformation-templates/apigw-cloudformation-template-default.json.ejs.

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts

attilah

It looks good in general as we discussed offline, a few points:

Check with @renebrandel about the necessity of feature flags based on the scenarios we talked about
Remove manual upload code and put the policy stack beside the root stack template, so it will be present in current cloud backend zip
It would be nice to only regenerate this stack during push if ANY of the apigw resources are having changes, otherwise no reason to do so

lgtm-com · 2021-04-08T05:10:49Z

This pull request introduces 1 alert when merging 8f02597 into 3b6298d - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

packages/amplify-category-api/src/provider-utils/awscloudformation/legacy-add-resource.ts

attilah

Thanks for making the changes, LGTM, a few nits added.

packages/amplify-e2e-core/src/categories/api.ts

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts

This commit updates the REST API IAM policy logic. Instead of using an inline policy per REST API, the logic now combines multiple REST API policies into managed IAM policies. Refs: #2084

…policies

github-actions · 2021-04-27T16:06:43Z

👋 Hi, this pull request was referenced in the v4.50.0 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v4.50.0.

cjihrig requested a review from a team as a code owner March 18, 2021 06:03

kaustavghosh06 requested review from akshbhu and kaustavghosh06 March 19, 2021 18:15

kaustavghosh06 assigned akshbhu Mar 23, 2021

cjihrig marked this pull request as draft March 23, 2021 22:53

cjihrig marked this pull request as ready for review March 24, 2021 19:40

akshbhu reviewed Mar 25, 2021

View reviewed changes

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts Outdated Show resolved Hide resolved

akshbhu reviewed Mar 25, 2021

View reviewed changes

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts Outdated Show resolved Hide resolved

akshbhu reviewed Mar 25, 2021

View reviewed changes

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts Outdated Show resolved Hide resolved

akshbhu reviewed Mar 25, 2021

View reviewed changes

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts Outdated Show resolved Hide resolved

akshbhu approved these changes Mar 25, 2021

View reviewed changes

cjihrig pushed a commit to cjihrig/docs that referenced this pull request Apr 2, 2021

Add documentation for CLI FF generateConsolidatedManagedPolicies

878edf8

Refs: aws-amplify/amplify-cli#6904

cjihrig mentioned this pull request Apr 2, 2021

feat(cli/ff): Document Feature Flag re: consolidateManagedPolicies aws-amplify/docs#3106

Closed

edwardfoyle reviewed Apr 2, 2021

View reviewed changes

cjihrig requested a review from attilah April 5, 2021 18:30

attilah suggested changes Apr 7, 2021

View reviewed changes

cjihrig requested a review from attilah April 12, 2021 14:46

attilah reviewed Apr 13, 2021

View reviewed changes

packages/amplify-category-api/src/provider-utils/awscloudformation/legacy-add-resource.ts Outdated Show resolved Hide resolved

attilah approved these changes Apr 13, 2021

View reviewed changes

packages/amplify-e2e-core/src/categories/api.ts Show resolved Hide resolved

packages/amplify-provider-awscloudformation/src/utils/consolidate-apigw-policies.ts Outdated Show resolved Hide resolved

cjihrig-aws added 8 commits April 13, 2021 13:28

fix(amplify-provider-awscloudformation): consolidate REST API policies

a1e7d57

This commit updates the REST API IAM policy logic. Instead of using an inline policy per REST API, the logic now combines multiple REST API policies into managed IAM policies. Refs: #2084

fixup! fix(amplify-provider-awscloudformation): consolidate REST API …

dc86a33

…policies

fixup! fix(amplify-provider-awscloudformation): consolidate REST API …

8e8b89e

…policies

fixup! fix(amplify-provider-awscloudformation): consolidate REST API …

902de2c

…policies

fixup! fix(amplify-provider-awscloudformation): consolidate REST API …

12e0ab5

…policies

fix(cli): relocate upload logic

0d20562

fix(cli): remove feature flag

27402c5

fix(cli): update existing rest apis on the fly

9d2cdae

cjihrig-aws added 3 commits April 13, 2021 13:29

fix(cli): remove roles from rest template

4e8f142

fix(cli): prevent duplicate policy names

2c8fd27

fix: address nits

47e65c7

edwardfoyle merged commit 5cfff17 into aws-amplify:master Apr 19, 2021

cjihrig deleted the consolidate branch April 20, 2021 00:33

github-actions bot added the referenced-in-release Issues referenced in a published release changelog label Apr 27, 2021

jakemitchellxyz mentioned this pull request Sep 6, 2021

Missing Feature Flag & Error: Cannot exceed quota for PoliciesPerRole: 10 #8108

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(amplify-provider-awscloudformation): consolidate REST API policies #6904

fix(amplify-provider-awscloudformation): consolidate REST API policies #6904

cjihrig commented Mar 18, 2021

codecov bot commented Mar 18, 2021 •

edited

Loading

akshbhu commented Mar 25, 2021

edwardfoyle Apr 2, 2021

cjihrig Apr 5, 2021

edwardfoyle Apr 2, 2021

cjihrig Apr 5, 2021

edwardfoyle Apr 2, 2021

cjihrig Apr 5, 2021

edwardfoyle Apr 2, 2021

cjihrig Apr 5, 2021

attilah left a comment

lgtm-com bot commented Apr 8, 2021

attilah left a comment

github-actions bot commented Apr 27, 2021

fix(amplify-provider-awscloudformation): consolidate REST API policies #6904

fix(amplify-provider-awscloudformation): consolidate REST API policies #6904

Conversation

cjihrig commented Mar 18, 2021

codecov bot commented Mar 18, 2021 • edited Loading

Codecov Report

akshbhu commented Mar 25, 2021

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

attilah left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 8, 2021

attilah left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 27, 2021

codecov bot commented Mar 18, 2021 •

edited

Loading