feat(ecs-service-extensions): allow taskRole to be passed in on creation of an ECS service

[brentryan]

This PR should address issue #13304

---

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

[gitpod-io]

[mergify]

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

[nathanpeck]

This looks good to me! One minor nit: it would be ideal to provide a separate test case for passing a role in, and perhaps also provide handling for the case where a user passes in a role with mutable set to false, like this:

const role = iam.Role.fromRoleArn(this, 'Role', 'arn:aws:iam::123456789012:role/MyExistingRole', {
  // Set 'mutable' to 'false' to use the role as-is and prevent adding new
  // policies to it. The default is 'true', which means the role may be
  // modified as part of the deployment.
  mutable: false,
});

I believe this would cause the ecs-service-extensions to fail, as many of the extensions attempt to mutate the role, so if it is not mutable it will fail. We would ideally have a test case for this, and verify that the resulting error message is sensible, something like "When passing an IAM role it must be mutable, so that extensions can add their required policies to the role"

Let me know if this is something you can add, if not we can still merge this in and fill that in later.

[gitpod-io]

[brentryan]

@nathanpeck I finally got a few minutes to look at adding this tests but I couldn't get anything to fail like you had expected with mutable: false.

Here's a look at what I attempted to add and run yarn test

Another thing related to this that wasn't clear to me as a contributor. In order to make a new integration test case like this, it says to setup your AWS credentials first and then you can run yarn integ to generate the synth'd cloudformation. However, it's not clear how to setup the credentials so they work with this mechanism. I've tried 3 or 4 different variations of AWS CLI/CDK credential mechanisms that we use and none of them seemed to work for this so I'm not sure what it's looking for exactly.

Here's what the error looked like if you can help at all:
https://github.com/brentryan/aws-cdk/pull/1/files

$ yarn integ integ.custom-role-service.js                                                                                                       
yarn run v1.22.5
$ cdk-integ integ.custom-role-service.js
Synthesizing integ.custom-role-service.js.
Selected stack: aws-ecs-integ
Deploying integ.custom-role-service.js...
[Warning at /aws-ecs-integ/greeter-service/SecurityGroup] Ignoring Egress rule since 'allowAllOutbound' is set to true; To add customize rules, set allowAllOutbound=false on the SecurityGroup
[Warning at /aws-ecs-integ/greeter-service/SecurityGroup] Ignoring Egress rule since 'allowAllOutbound' is set to true; To add customize rules, set allowAllOutbound=false on the SecurityGroup
aws-ecs-integ: deploying...

 ❌  aws-ecs-integ failed: Error: Unable to resolve AWS account to use. It must be either configured when you define your CDK or through the environment
    at SdkProvider.resolveEnvironment (/Users/bryan/dev/aws-cdk/packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts:208:13)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at CloudFormationDeployments.prepareSdkFor (/Users/bryan/dev/aws-cdk/packages/aws-cdk/lib/api/cloudformation-deployments.ts:223:33)
    at CloudFormationDeployments.deployStack (/Users/bryan/dev/aws-cdk/packages/aws-cdk/lib/api/cloudformation-deployments.ts:155:70)
    at CdkToolkit.deploy (/Users/bryan/dev/aws-cdk/packages/aws-cdk/lib/cdk-toolkit.ts:180:24)
    at initCommandLine (/Users/bryan/dev/aws-cdk/packages/aws-cdk/bin/cdk.ts:209:9)

I manually constructed the snapshot just to validate that yarn test would work. Once I got that in place then yarn test does work fine with this and no errors occur. So it's not clear to me with this testing framework where you expect the failure to happen with mutable: false.

Any help you can provide to get past this final hump?

[brentryan]

@nathanpeck I was able to get the integration tests to fully deploy and work correctly with yarn integ and yarn test. However, neither failed like you expected. This leads me to believe that this test really isn't providing any more value and can be left out. Digging into some of the CDK code a bit it seems like there's logic in there to avoid mutation if mutable=false so it doesn't error out, it just skip mutation. Or possibly there's nothing in these tests that are really trying to mutate the taskRole.

Thoughts? Are we good to proceed without this test or did you want something different?

[brentryan]

Actually, this has all the details around mutable #5569

Which introduced a bug which was fixed here #6090

But it appears that this bug still exists or came back because when I use mutable=false it's still attaching policies and allowing for mutation.

@rix0rrr @skinny85 Any pointers you can provide?

[skinny85]

Actually, this has all the details around mutable #5569

Which introduced a bug which was fixed here #6090

But it appears that this bug still exists or came back because when I use mutable=false it's still attaching policies and allowing for mutation.

@rix0rrr @skinny85 Any pointers you can provide?

@brentryan can you show a small example where a Role is imported with mutable set to false, but it's still being mutated?

[brentryan]

@skinny85 See example test I made here: https://github.com/brentryan/aws-cdk/pull/1/files#diff-a085d1e5ae122c741bc547a19a09ae61602186c399be47cf92d4c1397462c73fR21

Then if you look at the cdk synth that it generated for that imported role you'll see it's adding additional policies: https://github.com/brentryan/aws-cdk/pull/1/files#diff-b027e5c65a787f95cbf40cf8008beb1b59fa0872c2607f2530b83f5387e9eb2fR1019

You may need to click "load diff" to render the large cloudformation in your browser and look at line 1019.

You can see an example of where the role in this test case is being mutated https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk-containers/ecs-service-extensions/lib/extensions/appmesh.ts#L210

[nathanpeck]

That is an interesting find. The code in ecs-service-extensions that mutates the role can be found here: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk-containers/ecs-service-extensions/lib/extensions/cloudwatch-agent.ts#L51-L60

Basically it is creating a new iam.Policy construct so maybe that is not being picked up as a mutation?

[skinny85]

That is an interesting find. The code in ecs-service-extensions that mutates the role can be found here: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk-containers/ecs-service-extensions/lib/extensions/cloudwatch-agent.ts#L51-L60

Basically it is creating a new iam.Policy construct so maybe that is not being picked up as a mutation?

Ah, that's probably what it is! Nice catch @nathanpeck.

I guess this should be solved in the IAM library then.

[brentryan]

@skinny85 Are you creating a bug against the iam package for this? Do you want me to do anything with regards to that?

@nathanpeck How do you want me to handle this given the bug in iam?

[nathanpeck]

I don't want to block this PR on a possible bug in the IAM module, especially since this bug may require investigation to see if fixing it in IAM would cause unintended sideffects in other modules. I'll approve this PR for merge.

Pull request has been modified.

[brentryan]

@nathanpeck I clicked the "Update Branch" button by accident. Need another approval it looks like. Sorry.

[brentryan]

@nathanpeck @skinny85 Sorry to be a bother, but anything we can do to kick this along? I'm not sure what's normal for how long to wait or who else to contact.

[nathanpeck]

Sorry it looks like it was hung up on one of the checks. I hit the button to retry the checks, and merge again.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 5f65df8
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[nathanpeck]

It looks like it successfully merged this time! Thanks so much for the contribution @brentryan