feat(ecs): add `BaseService.fromServiceArnWithCluster()` for use in CodePipeline by tobytipton · Pull Request #18530 · aws/aws-cdk

tobytipton · 2022-01-19T14:39:00Z

This adds support for importing a ECS Cluster via the Arn, and not requiring the VPC or Security Groups.

This will generate an ICluster which can be used in Ec2Service.fromEc2ServiceAttributes() and FargateService.fromFargateServiceAttributes() to get an IBaseService which can be used in the EcsDeployAction to allow for cross account/region deployments in CodePipelines.

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

gitpod-io · 2022-01-19T14:39:04Z

skinny85

Thanks for the contribution @tobytipton!

I thought (and researched!) the issue some more, and I think I came to the following conclusion.

The ideal experience we want is:

  // the name fromServiceNewFormatArn() is just a proposal, of course
  const myBaseService: IBaseService = ???.fromServiceNewFormatArn(this, 'Service',
    'arn:aws:ecs:us-west-2:123456:service/my-cluster/my-service');

This way, myBaseService can be passed into EcsDeployAction, and the only thing we need is the ARN of the ECS Service in the new format to make that happen, no more bullshit with VPCs, etc.

Of course, the question is what class should be substituted for ???.

Like we discussed in #18059, we can't really make that happen in FargateService.fromFargateServiceArn(), nor in Ec2Service.fromEc2ServiceArn(), because those return IService, and changing that can have other consequences.

But, I think I found another great candidate - BaseService!

So, if we add that method there, I think this will solve the CodePipeline usecase that we're after here.

However, if you agree we should go with this new static method in BaseService, I think what we want in Cluster is actually fromClusterName(), not fromClusterArn(), because that will save us the hassle of re-building the ARN, just for it to be parsed again.

What do you think about this direction @tobytipton?

(Of course, we should validate in ???.fromServiceNewFormatArn() that the ARN is indeed in the new ECS format)

skinny85 · 2022-01-20T00:30:02Z

packages/@aws-cdk/aws-ecs/test/cluster.test.ts

+    const clusterName = 'my-cluster';
+    const region = 'service-region';
+    const account = 'service-account';
+    const clusterArn = `arn:aws:ecs:${region}:${account}:service/${clusterName}`;


This is not the Cluster ARN (it should be arn:aws:ecs:<region>:<account>:cluster/<cluster-name>).

skinny85 · 2022-01-20T00:38:06Z

packages/@aws-cdk/aws-ecs/lib/cluster.ts

+   */
+  public static fromClusterArn(scope: Construct, id: string, clusterArn: string): ICluster {
+    const stack = Stack.of(scope);
+    const clusterName = stack.splitArn(clusterArn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;


Instead of this cast, let's just handle the case when resourceName is undefined (throw an exception).

See here for similar code in other modules.

This will also require a separate unit test.

Pull request has been modified.

tobytipton · 2022-01-20T14:44:28Z

Thanks for the contribution @tobytipton!

I thought (and researched!) the issue some more, and I think I came to the following conclusion.

The ideal experience we want is:
  // the name fromServiceNewFormatArn() is just a proposal, of course
  const myBaseService: IBaseService = ???.fromServiceNewFormatArn(this, 'Service',
    'arn:aws:ecs:us-west-2:123456:service/my-cluster/my-service');
This way, myBaseService can be passed into EcsDeployAction, and the only thing we need is the ARN of the ECS Service in the new format to make that happen, no more bullshit with VPCs, etc.

Of course, the question is what class should be substituted for ???.

Like we discussed in #18059, we can't really make that happen in FargateService.fromFargateServiceArn(), nor in Ec2Service.fromEc2ServiceArn(), because those return IService, and changing that can have other consequences.

But, I think I found another great candidate - BaseService!

So, if we add that method there, I think this will solve the CodePipeline usecase that we're after here.

However, if you agree we should go with this new static method in BaseService, I think what we want in Cluster is actually fromClusterName(), not fromClusterArn(), because that will save us the hassle of re-building the ARN, just for it to be parsed again.

What do you think about this direction @tobytipton?

(Of course, we should validate in ???.fromServiceNewFormatArn() that the ARN is indeed in the new ECS format)

At first thought fromClusterName() would most likely need the region and account associated with it, to be able to generate the clusterArn, right?. Which the clusterName is all that is needed for our codepipeline use case so It would in theory solve that, but I can see that being a limitation which doesn't fulfill all use cases vs usage of the ARN.

The IBaseService requires an cluster as an ICluster, which would mean fromClusterName() or fromClusterArn() have to deal with the vpc not actually being set. I am thinking fromClusterArn() is still the correct method because of possible other use cases we aren't currently thinking about. Constructing a clusterArn inside of the fromServiceNewFormatArn() should be and make it easier for end users.

I think fromServiceNewFormatArn() actually might still be a value add to be able to return back the IBaseService without getting the cluster from the clusterArn and then pass that in with the serviceArn to get the cluster. From an end user standpoint getting the IBaseService required to call the EcsDeployAction with one call passing in the serviceArn would be a lot cleaner.

Also I would wonder if the name fromServiceClusterFormatArn() might be more ideal long term since new is relative.

If you are good with the BaseService.fromServiceClusterFormatArn() in addition to the Cluster.fromClusterArn() do you want me to add that to this PR or create a new PR after this PR?

skinny85 · 2022-01-20T17:30:04Z

At first thought fromClusterName() would most likely need the region and account associated with it, to be able to generate the clusterArn, right?. Which the clusterName is all that is needed for our codepipeline use case so It would in theory solve that, but I can see that being a limitation which doesn't fulfill all use cases vs usage of the ARN.

Every resource has the account and region of the Stack they are part of available, so constructing the ARN will not be an issue. We have many fromXyzName() methods in the CDK already, so this is a common pattern.

The IBaseService requires an cluster as an ICluster, which would mean fromClusterName() or fromClusterArn() have to deal with the vpc not actually being set. I am thinking fromClusterArn() is still the correct method because of possible other use cases we aren't currently thinking about. Constructing a clusterArn inside of the fromServiceNewFormatArn() should be and make it easier for end users.

Not sure what you mean. The method you added in this PR already deals with vpc. So this is not an issue anymore. We will use the same technique in fromClusterName() as you're doing in fromClusterArn() now.

If you are good with the BaseService.fromServiceClusterFormatArn() in addition to the Cluster.fromClusterArn() do you want me to add that to this PR or create a new PR after this PR?

Yes, let's add the BaseService.fromServiceArnWithCluster(): IBaseService method in this PR, and the implementation will call Cluster.fromClusterName() to get an ICluster instance for the IBaseService. I would get rid of fromClusterArn() for now - we can always add it later as needed.

tobytipton · 2022-01-20T18:45:59Z

Every resource has the account and region of the Stack they are part of available, so constructing the ARN will not be an issue. We have many fromXyzName() methods in the CDK already, so this is a common pattern.

You are referencing using the scope to generate the Arn, like this correct?

const clusterArn = cdk.Stack.of(scope).formatArn({
      service: 'ecs',
      resource: 'cluster',
      resourceName: clusterName,
});

Which is leveraging the scope to generate the clusterArn. In the case where we have a pipeline which is cross region/account the scope would typically be related to the pipeline itself which would result in the clusterArn to have the pipeline region/account rather than the expect service region/account.

The idea would be in the pipeline stack there would be something like

const service = BaseService.fromServiceArnWithCluster(this, 'importService',
  'arn:aws:ecs:service-region:service-account:service/cluster-name/service-name'
);

Which means the scope would be related to the pipeline stack not the stack which has the env for the service-region and service-account.

Which is why I am thinking leveraging fromClusterArn is more ideal then fromClusterName, if you think fromClusterName is more appropriate though I can change it.

skinny85 · 2022-01-20T19:15:57Z

Yes, you are correct @tobytipton. My bad.

Let's leave Cluster.fromClusterArn().

tobytipton · 2022-01-20T21:53:23Z

Updated with BaseService.fromServiceArnWithCluster and related tests.

skinny85

Looks great @tobytipton! I have some minor comments on the tests and docs, but the functionality itself is great. This will make it sooo much easier to use the EcsDeployAction in the CDK - awesome work!

packages/@aws-cdk/aws-ecs/README.md

skinny85 · 2022-01-20T22:34:30Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

 export abstract class BaseService extends Resource
  implements IBaseService, elbv2.IApplicationLoadBalancerTarget, elbv2.INetworkLoadBalancerTarget, elb.ILoadBalancerTarget {
+  /**
+   * Import an existing ECS/Fargate Service using the service cluster format.


Let's add a note in the docs that the service ARN must be in the "new" format, with maybe a link to the AWS docs about it.

skinny85 · 2022-01-20T22:35:05Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+    if (resourceName.split('/').length !== 2) {
+      throw new Error(`resource name ${resourceName} from service ARN: ${serviceArn} is not using the ARN cluster format`);
+    }
+    const clusterName = resourceName.split('/')[0];
+    const serviceName = resourceName.split('/')[1];


We're repeating the resourceName.split('/') expression 3 times here.

Let's assign it to a local variable instead.

skinny85 · 2022-01-20T22:35:59Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+      public readonly cluster = cluster;
+
+    }


Weird formatting here:

Suggested change

public readonly cluster = cluster;

}

public readonly cluster = cluster;

}

skinny85 · 2022-01-20T22:40:40Z

packages/@aws-cdk/aws-ecs/lib/cluster.ts

  }

+  /**
+   * Import an existing cluster to the stack from the cluster ARN.


Let's add a note in these docs that the Cluster returned does not allow accessing the vpc property.

skinny85 · 2022-01-20T22:46:00Z

packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts

+
    test('allows setting enable execute command', () => {
      // GIVEN
      const stack = new cdk.Stack();


What do you think of adding a simple test in the @aws-cdk/aws-codepipeline-actions module for this? We already have a test file for the EcsDeployAction class.

Added the test, did it for cross account and region.

skinny85 · 2022-01-20T22:47:18Z

packages/@aws-cdk/aws-ecs/test/cluster.test.ts

+  test('throws error when import ECS Cluster without resource name in arn', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const clusterArn = 'arn:aws:ecs:service-region:service-account:cluster';


Again, I would probably inline this variable.

skinny85 · 2022-01-20T22:50:33Z

packages/@aws-cdk/aws-ecs/test/cluster.test.ts

+    const clusterName = 'my-cluster';
+    const region = 'service-region';
+    const account = 'service-account';
+    const clusterArn = `arn:aws:ecs:${region}:${account}:cluster/${clusterName}`;


I would also inline this variable.

skinny85 · 2022-01-20T22:51:39Z

packages/@aws-cdk/aws-ecs/test/base-service.test.ts

+describe('When import an ECS Service', () => {
+  test('with serviceArnWithCluster', () => {
+    // GIVEN
+    const stack = new cdk.Stack();


Since we're writing brand-new tests here, let's make them beautiful by extracting this const stack = new cdk.Stack(); line that is repeated in every test to a beforeEach() method.

skinny85 · 2022-01-20T22:57:51Z

packages/@aws-cdk/aws-ecs/test/base-service.test.ts

+    //THEN
+    expect(() => {
+      ecs.BaseService.fromServiceArnWithCluster(stack, 'Service', serviceArn);
+    }).toThrowError(`resource name ${serviceName} from service ARN`);


Kind of a weird error message to assert here, but OK (I would probably write an assertion on the "is not using the ARN cluster format" part of the error message, seems more relevant to this test).

skinny85 · 2022-01-20T23:08:21Z

The build should succeed when you update from master after #18576 gets merged.

…peline actions

Pull request has been modified.

tobytipton · 2022-01-21T14:06:52Z

packages/@aws-cdk/aws-codepipeline-actions/README.md

+      actionName: 'DeployAction',
+      service: service,
+      input: buildOutput,
+    }),


We might want to add an note in here that for cross accounts you will want to provide the role, because if not a role name will be generated for you which doesn't actually get created, what do you think?

Hmm, not sure I understand what you mean...?

If the Action is cross-account, a new Role, with a well-known name, will be generated for you in the correct account (if you don't specify the role property).

If you don't provide role on the ECSDeployAction a roleArn will be provided in the code pipeline actions for you.
If you are deploying across accounts, that role will most likely not exist in the other account.

From my ecs deploy action test.

RoleArn: { 'Fn::Join': [ '', [ 'arn:', { Ref: 'AWS::Partition', }, ':iam::service-account:role/pipelinestack-support-serloyecsactionrole49867f847238c85af7c0', ], ],

In order for the deployment to work I would need to create an IAM Role with the name pipelinestack-support-serloyecsactionrole49867f847238c85af7c0 in the service account, in order for the pipeline to be able to deploy.

In a self-mutating CDK pipeline the self mutation will actually fail because the account doesn't exist so it can't update the Policy for the KMS key, bucket and Pipeline Role's Policy. I would guess an normal pipeline might have the same issue as well.

Providing an IAM role which is already created in the other account resolves that, typically we have been leveraging the CDK bootstrap deploy role and adding permissions to deploy ECS matching this is typically how we have been working around this. Write up from other PR on role.

I mean, that's not just some random Role name that we just make up 🙂. There is another Stack in the application that gets generated, in the target account, that contains that Role, and the Pipeline Stack depends on that Stack, so cdk deploy PipelineStack should correctly include it, deploy it first, and then everything should work.

If it doesn't, that's a bug we need to fix.

However, it's fine if you want to mention the Role in the ReadMe.

Interesting, I guess I never noticed that other support stack being created for the service account, to create that role. I have see the other stack which is in the other region from the pipeline which gets generated in the pipeline account. I added a little blurb about the role, I know I have seen issues with the CDK pipeline when adding a new region having an issue if the role referenced hasn't been created.

skinny85

Looks great @tobytipton. I only have a few comments on the wording of the docs, but I'll just fix those myself, and merge this in. Awesome work!

packages/@aws-cdk/aws-codepipeline-actions/README.md

packages/@aws-cdk/aws-ecs/README.md

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

packages/@aws-cdk/aws-ecs/lib/cluster.ts

Pull request has been modified.

mergify · 2022-01-22T00:17:36Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-01-22T01:05:43Z

AWS CodeBuild CI Report

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

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

mergify · 2022-01-22T01:06:00Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

* origin/master: (31 commits) feat(iotevents): add DetectorModel L2 Construct (aws#18049) feat(ecs): add `BaseService.fromServiceArnWithCluster()` for use in CodePipeline (aws#18530) docs(s3): remove vestigial documentation (aws#18604) chore(cli): LogMonitor test fails randomly due to Date.now() (aws#18601) chore(codedeploy): migrate tests to use the Assertions module (aws#18585) docs(stepfunctions): fix typo (aws#18583) chore(eks-legacy): migrate tests to `assertions` (aws#18596) fix(cli): hotswap should wait for lambda's `updateFunctionCode` to complete (aws#18536) fix(apigatewayv2): websocket api: allow all methods in grant manage connections (aws#18544) chore(dynamodb): migrate tests to assertions (aws#18560) fix(aws-apigateway): cross region authorizer ref (aws#18444) feat(lambda-nodejs): Allow setting mainFields for esbuild (aws#18569) docs(cfnspec): update CloudFormation documentation (aws#18587) feat(assertions): support for conditions (aws#18577) fix(ecs-patterns): Fix Network Load Balancer Port assignments in ECS Patterns (aws#18157) chore(region-info): ap-southeast-3 (Jakarta) ELBV2_ACCOUNT (aws#18300) fix(pipelines): CodeBuild projects are hard to tell apart (aws#18492) fix(ecs): only works in 'aws' partition (aws#18496) chore(app-delivery): migrate unit tests to Assertions (aws#18574) chore: migrate kaizen3031593's modules to assertions (aws#18205) ...

…odePipeline (aws#18530) This adds support for importing a ECS Cluster via the Arn, and not requiring the VPC or Security Groups. This will generate an ICluster which can be used in `Ec2Service.fromEc2ServiceAttributes()` and `FargateService.fromFargateServiceAttributes()` to get an `IBaseService` which can be used in the `EcsDeployAction` to allow for cross account/region deployments in CodePipelines. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

tobytipton added 2 commits January 19, 2022 09:24

Add Cluster.fromClusterArn to support cluster import

fb55002

throw errors on connections and hasEc2Capacity as well

1721144

github-actions bot assigned madeline-k Jan 19, 2022

github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Jan 19, 2022

update readme with fromClusterArn

aa35e12

tobytipton mentioned this pull request Jan 19, 2022

chore(codepipeline): add example of an ECS Deploy Action #18059

Closed

skinny85 previously requested changes Jan 20, 2022

View reviewed changes

fix clusterArn and fix test for resource name

6dfaf31

add BaseService.fromServiceArnWithCluster

fc96ff4

skinny85 previously requested changes Jan 20, 2022

View reviewed changes

madeline-k removed their assignment Jan 21, 2022

tobytipton added 2 commits January 21, 2022 08:58

remove fargate and ec2 tests, fix docs, add test and readme in codepi…

9160bc0

…peline actions

Merge branch 'master' into fromClusterArn

d1a1fd5

tobytipton commented Jan 21, 2022

View reviewed changes

tobytipton added 4 commits January 21, 2022 09:50

fix test after merge from master

25a0e6b

fix typo in readme

d7a7ef7

remove extra '}' in readme

359f1ef

add note able role to readme

f84c6ae

skinny85 previously requested changes Jan 21, 2022

View reviewed changes

Applu doc wording suggestions

3b7afce

skinny85 changed the title ~~feat(ecs): Add Cluster.fromClusterArn to support cluster import~~ feat(ecs): add BaseService.fromServiceArnWithCluster() for use in CodePipeline Jan 21, 2022

github-actions bot assigned madeline-k Jan 21, 2022

skinny85 approved these changes Jan 21, 2022

View reviewed changes

skinny85 assigned skinny85 and unassigned madeline-k Jan 21, 2022

Merge branch 'master' into fromClusterArn

e83ae42

mergify bot merged commit 3d192a9 into aws:master Jan 22, 2022

tobytipton deleted the fromClusterArn branch January 24, 2022 12:39

Conversation

tobytipton commented Jan 19, 2022

Uh oh!

gitpod-io bot commented Jan 19, 2022

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobytipton commented Jan 20, 2022

Uh oh!

skinny85 commented Jan 20, 2022

Uh oh!

tobytipton commented Jan 20, 2022

Uh oh!

skinny85 commented Jan 20, 2022

Uh oh!

tobytipton commented Jan 20, 2022

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 commented Jan 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 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!

mergify bot commented Jan 22, 2022

Uh oh!

aws-cdk-automation commented Jan 22, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jan 22, 2022

Uh oh!

Reviewers

Assignees

skinny85 Jan 21, 2022 •

edited

Loading