Improve IAM role internal handling and resolution by issea1015 · Pull Request #10067 · serverless/serverless

issea1015 · 2021-10-07T14:30:23Z

Closes: #8396

Broken Tests:

{
    "test/unit/lib/plugins/aws/package/compile/events/cloudFront.test.js": [
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should throw an error if the region is not us-east-1",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should create corresponding resources when cloudFront events are given",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should create different origins for different domains with the same path",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should create different origins for the same domains with the same path but different protocols",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should create different origins with different ids for different domains in the same function",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should use previous created origin for the same params",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should create origins with all values given as an object",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should correctly deep merge arrays with objects",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should throw if more than one cloudfront event with different origins were defined as a default",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should use behavior without PathPattern as a DefaultCacheBehavior",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should create DefaultCacheBehavior if non behavior without PathPattern were defined",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should create DefaultCacheBehavior if non behavior without PathPattern were defined but isDefaultOrigin flag was set",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should throw if non of the cloudfront event with different origins were defined as a default",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should use previous created behavior for the same params and different event types",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should create behavior with all values given as an object",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should throw if more than one behavior with the same PathPattern",
        "AwsCompileCloudFrontEvents > #compileCloudFrontEvents() > should throw if some of the event type in any behavior is not unique"
    ],
    "test/unit/lib/plugins/aws/package/compile/events/kafka.test.js": [
        "test/unit/lib/plugins/aws/package/compile/events/kafka.test.js > when there are kafka events defined > should update default IAM role with SecretsManager statement"
    ],
    "test/unit/lib/plugins/aws/package/compile/events/msk/index.test.js": [
        "AwsCompileMSKEvents > when there are msk events defined > should update default IAM role with MSK statement"
    ],
    "test/unit/lib/plugins/aws/package/compile/events/rabbitmq.test.js": [
        "test/unit/lib/plugins/aws/package/compile/events/rabbitmq.test.js > when there are rabbitmq events defined > should update default IAM role with DescribeBroker statement",
        "test/unit/lib/plugins/aws/package/compile/events/rabbitmq.test.js > when there are rabbitmq events defined > should update default IAM role with SecretsManager statement"
    ],
    "test/unit/lib/plugins/aws/package/compile/events/sqs.test.js": [
        "AwsCompileSQSEvents > #compileSQSEvents() > should remove all non-alphanumerics from queue names for the resource logical ids",
        "AwsCompileSQSEvents > #compileSQSEvents() > when a queue ARN is given > should create event source mappings when a queue ARN is given",
        "AwsCompileSQSEvents > #compileSQSEvents() > when a queue ARN is given > should allow specifying SQS Queues as CFN reference types",
        "AwsCompileSQSEvents > #compileSQSEvents() > when a queue ARN is given > should add the necessary IAM role statements"
    ],
    "test/unit/lib/plugins/aws/package/compile/events/stream.test.js": [
        "AwsCompileStreamEvents > #compileStreamEvents() > should remove all non-alphanumerics from stream names for the resource logical ids",
        "AwsCompileStreamEvents > #compileStreamEvents() > when a DynamoDB stream ARN is given > should create event source mappings when a DynamoDB stream ARN is given",
        "AwsCompileStreamEvents > #compileStreamEvents() > when a DynamoDB stream ARN is given > should allow specifying DynamoDB and Kinesis streams as CFN reference types",
        "AwsCompileStreamEvents > #compileStreamEvents() > when a DynamoDB stream ARN is given > should allow specifying OnFailure destinations as CFN reference types",
        "AwsCompileStreamEvents > #compileStreamEvents() > when a DynamoDB stream ARN is given > should add the necessary IAM role statements",
        "AwsCompileStreamEvents > #compileStreamEvents() > when a Kinesis stream ARN is given > should create event source mappings when a Kinesis stream ARN is given",
        "AwsCompileStreamEvents > #compileStreamEvents() > when a Kinesis stream ARN is given > should create stream consumer when a Kinesis stream with consumer \"true\" is given",
        "AwsCompileStreamEvents > #compileStreamEvents() > when a Kinesis stream ARN is given > should add the necessary IAM role statements"
    ],
    "test/unit/lib/plugins/aws/package/compile/events/websockets/lib/api.test.js": [
        "#compileApi() > should add the websockets policy"
    ],
    "test/unit/lib/plugins/aws/package/compile/functions.test.js": [
        "AwsCompileFunctions > #compileFunctions() > when using onError config > when IamRoleLambdaExecution is used > should create necessary resources if a SNS arn is provided",
        "AwsCompileFunctions > #compileFunctions() > when using awsKmsKeyArn config > when IamRoleLambdaExecution is used > should create necessary resources if a KMS key arn is provided",
        "AwsCompileFunctions > #compileFunctions() > when using tracing config > when IamRoleLambdaExecution is used > should create necessary resources if a tracing config is provided"
    ]
}

Broken Tests - Should be handled in this PR:

test/unit/lib/plugins/aws/package/compile/events/kafka.test.js
test/unit/lib/plugins/aws/package/compile/events/msk/index.test.js
test/unit/lib/plugins/aws/package/compile/events/rabbitmq.test.js

Broken Tests - Should be handled in separate PRs:

test/unit/lib/plugins/aws/package/compile/events/cloudFront.test.js
test/unit/lib/plugins/aws/package/compile/events/sqs.test.js
test/unit/lib/plugins/aws/package/compile/events/stream.test.js
test/unit/lib/plugins/aws/package/compile/events/websockets/lib/api.test.js
test/unit/lib/plugins/aws/package/compile/functions.test.js

…ole-compilation

medikoo

Thank you @issea1015, that looks outstanding!

In general it looks very good. I've just proposed some side improvements. Let me know what you think

codecov · 2021-10-12T15:16:36Z

Codecov Report

Merging #10067 (aa54a80) into master (654c507) will decrease coverage by 72.80%.
The diff coverage is 18.03%.

❗ Current head aa54a80 differs from pull request most recent head 0e3e182. Consider uploading reports for the commit 0e3e182 to get more accurate results

@@             Coverage Diff             @@
##           master   #10067       +/-   ##
===========================================
- Coverage   85.36%   12.56%   -72.81%     
===========================================
  Files         340      335        -5     
  Lines       13990    13610      -380     
===========================================
- Hits        11943     1710    -10233     
- Misses       2047    11900     +9853

Impacted Files	Coverage Δ
lib/plugins/aws/package/compile/events/activemq.js	`0.00% <0.00%> (-100.00%)`	⬇️
...b/plugins/aws/package/compile/events/cloudFront.js	`3.82% <0.00%> (-93.02%)`	⬇️
lib/plugins/aws/package/compile/events/kafka.js	`0.00% <0.00%> (-98.62%)`	⬇️
...ib/plugins/aws/package/compile/events/msk/index.js	`0.00% <0.00%> (-100.00%)`	⬇️
lib/plugins/aws/package/compile/events/sqs.js	`6.66% <0.00%> (-93.34%)`	⬇️
lib/plugins/aws/package/compile/events/stream.js	`2.75% <0.00%> (-95.48%)`	⬇️
...s/aws/package/compile/events/websockets/lib/api.js	`16.66% <0.00%> (-83.34%)`	⬇️
lib/plugins/aws/package/compile/functions.js	`3.89% <0.00%> (-90.93%)`	⬇️
lib/plugins/aws/package/lib/mergeIamTemplates.js	`5.88% <0.00%> (-94.12%)`	⬇️
lib/plugins/aws/package/lib/utils.js	`25.00% <25.00%> (ø)`
... and 271 more

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 654c507...0e3e182. Read the comment docs.

medikoo

Thank you @issea1015 it looks very very good. We're getting close :)

Please see my comments and let me know what you think

issea1015 · 2021-10-18T14:09:33Z

@medikoo Thank you for all this walking me through! I've just resolved all threads. Could you take a look again?

medikoo

Thank you @issea1015 that looks really good.

Please see my comments, also it appears one of the tests in CIN started to fail (when run individually), have you checked what could be the case?

issea1015 · 2021-10-20T14:08:07Z

Thank you for the review. I've resolved every thread except the test one:

@medikoo Could you let me know what do you mean by "tests in CIN"?

I listed some broken tests in the description of this PR even though I found much more cases when I ran all unit tests. (When I wrote the description of this PR, I've just run the cases under this dir: "test/unit/lib/plugins/aws/package/")

Nonetheless, all of them seem related to .iamConfig, which is supposed to be prepared once we refactor the tests into the runServerless() way.

So again, @medikoo please let me know what do you mean by "tests in CIN" 🙏

medikoo · 2021-10-21T09:07:00Z

So again, @medikoo please let me know what do you mean by "tests in CIN" 🙏

@issea1015 Sorry typo sneaked in. I meant tests in CI, so in our GitHub Actions setup. We can see that Isolated unit tests fails: https://github.com/serverless/serverless/pull/10067/checks?check_run_id=3872231717

You should be able to easily reproduce it by running just this single failing test file individually

…ole-compilation

issea1015 · 2021-10-25T13:29:34Z

Found how this branch is different from master and how it breaks the test

test/unit/lib/plugins/aws/deploy/index.test.js

In this branch, we escape the Resource array having only one item, it makes compiledCfTemplate different from the one built in the master

@medikoo Should I

keep this escaping logic and update the test?
or roll this logic back?

medikoo · 2021-10-25T14:17:39Z

In this branch, we escape the Resource array having only one item, it makes compiledCfTemplate different from the one built in the master

Ok, so if I understand correctly. In new handling, if there's single resource in a policy, we do not wrap it into array for Statement.Resoiurce policy, while it's the case in old version.

I think it's fine to do that, as this I believe introduces determinism. With this PR, single resource will never be wrapped into array.

Concerning tests, it'll be great to refactor it into runServerless variant (if it's not one already)

issea1015 · 2021-10-26T14:08:24Z

I think it's fine to do that, as this I believe introduces determinism. With this PR, single resource will never be wrapped into array.

I believe the same. I updated the failing unit test to align with the changes on IAM role internal handling. Let's see if all other tests running in CI pass or not.

issea1015 · 2021-10-26T14:20:17Z

For future ref. Another test in CI is failed for the same reason to above one. Will fix it in the same way.

test/unit/lib/plugins/aws/deploy/lib/checkForChanges.test.js:1078:60 - "should skip a deployment with identical hashes and package.artifact targeting .serverless directory"

issea1015 · 2021-10-26T14:33:56Z

For future ref. Other tests in CI are failed for the same reason to above one. Will fix

test/unit/lib/plugins/aws/package/compile/events/activemq.test.js:81:86 - "when there are activemq events defined" - "should update default IAM role with DescribeBroker statement"
test/unit/lib/plugins/aws/package/compile/events/activemq.test.js:89:86 - "when there are activemq events defined" - "should update default IAM role with SecretsManager statement"

issea1015 · 2021-10-27T12:50:49Z

I've updated tests-in-CI broken by unwarped Statement.Resoiurce policy.

Now, in CI, we are facing broken tests that are not based on runServerless. As per the issue description, it's time to list up broken tests and to request separate PR per each test file. What do you think? @medikoo

#8396 (comment): Any test file that will host broken tests should be refactored to be based on runServerless (https://github.com/serverless/serverless/tree/master/test#unit-tests), and PR's that refactor each file should be proposed separate (different PR per each test file).

medikoo · 2021-10-27T14:19:45Z

Now, in CI, we are facing broken tests that are not based on runServerless. As per the issue description, it's time to list up broken tests and to request separate PR per each test file. What do you think? @medikoo

Yes, that how ideally it should've handled. First those tests should be refactored, and having that, we can confirm in this PR, we do not break anything by accident

jwsloan · 2021-10-27T14:37:06Z

@medikoo Is there any estimate on a timeline for getting this finished up, merged and a release out with it? It solves the exact problem we've run into with dynamo stream permissions getting duplicated in the default role.

Can someone without prior experience of working on this project (like myself) help get those tests refactored?

medikoo · 2021-10-27T15:02:02Z

@jwsloan in this case we're at the stake of a generous contribution by @issea1015 and ETA is unknown. Still @issea1015 made very impressive progress, and we're leaning towards having that in.

jwsloan · 2021-10-27T18:19:55Z

@jwsloan in this case we're at the stake of a generous contribution by @issea1015 and ETA is unknown. Still @issea1015 made very impressive progress, and we're leaning towards having that in.

We will patiently await the release that includes this, then. :)

issea1015 · 2021-10-28T14:21:11Z

I've updated the list of broken tests in the PR description. And I've classified the ones that should be handled in this PR and those that should be handled in separate PRs.

…ole-compilation

issea1015 added 3 commits October 7, 2021 23:23

Improve IAM role internal handling and resolution

40c932e

Merge remote-tracking branch 'origin/master' into 8396-refactor-iam-r…

f29df76

…ole-compilation

Minimize changes out of scope

d13cca7

medikoo suggested changes Oct 8, 2021

View reviewed changes

medikoo assigned issea1015 Oct 8, 2021

medikoo added refactor cat/aws-iam labels Oct 8, 2021

issea1015 added 8 commits October 8, 2021 23:09

Minimize calculation

b3e7421

Modularize expectToIncludeStatement()

e4a28d3

Write simpler condition

aafa382

Define simple method directly as a class member

38cacc1

Stop testing valueless specifics of implementation

22141b8

Reduce unnecessary deep merge

3a7dcf5

Undo unnecessary rename

66c3639

Add unit test for lib/plugins/aws/package/lib/resolveIamRoles.js

aa54a80

Write more comprehensible code

2ea17d6

medikoo suggested changes Oct 18, 2021

View reviewed changes

issea1015 added 9 commits October 18, 2021 21:46

Have logic more transparent

a7a0a1d

Give more specific variable name on collision

5bd4a8d

Use more explicit variable name

5084fcb

Write more comprehensible code

a746f93

Chain off .only from unit test

8337ae9

Rename function to be close to English syntax

829a7f7

Give more specific variable name on collision

d4774d1

Define variables for reusable values

9c16b1f

Move util to the correct scope

9c38fbf

issea1015 requested a review from medikoo October 18, 2021 14:10

medikoo suggested changes Oct 18, 2021

View reviewed changes

Comment thread lib/plugins/aws/package/lib/resolveIamRoles.js Outdated

Comment thread lib/plugins/aws/package/lib/resolveIamRoles.js Outdated

Comment thread test/utils/iam.js Outdated

issea1015 added 4 commits October 20, 2021 22:26

Simplify logic

f5273c6

Use camelCase names for non-constructor variables

cd54dc8

Use more proper util name

a843a47

Simplify logic

b00b446

Merge remote-tracking branch 'origin/master' into 8396-refactor-iam-r…

99e8499

…ole-compilation

issea1015 requested a review from medikoo October 25, 2021 13:30

Align unit test to the changes on IAM role internal handling

ec94887

Align unit test to the changes on IAM role internal handling

a3bf739

Align unit test to the changes on IAM role internal handling

f3b6510

issea1015 mentioned this pull request Nov 2, 2021

Improve IAM role statements, policies and principals internal handling and resolution #8396

Open

Merge remote-tracking branch 'origin/master' into 8396-refactor-iam-r…

0e3e182

…ole-compilation

Conversation

issea1015 commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Broken Tests:

Broken Tests - Should be handled in this PR:

Broken Tests - Should be handled in separate PRs:

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

issea1015 commented Oct 18, 2021

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

issea1015 commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medikoo commented Oct 21, 2021

Uh oh!

issea1015 commented Oct 25, 2021

Uh oh!

medikoo commented Oct 25, 2021

Uh oh!

issea1015 commented Oct 26, 2021

Uh oh!

issea1015 commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

issea1015 commented Oct 26, 2021

Uh oh!

issea1015 commented Oct 27, 2021

Uh oh!

medikoo commented Oct 27, 2021

Uh oh!

jwsloan commented Oct 27, 2021

Uh oh!

medikoo commented Oct 27, 2021

Uh oh!

jwsloan commented Oct 27, 2021

Uh oh!

issea1015 commented Oct 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

issea1015 commented Oct 7, 2021 •

edited

Loading

codecov Bot commented Oct 12, 2021 •

edited

Loading

issea1015 commented Oct 20, 2021 •

edited

Loading

issea1015 commented Oct 26, 2021 •

edited

Loading