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

[tobytipton]

Create Tests of ECS Deploy Action which show importing ECS service to deploy cross account and region as well as creating ECS service to deploy cross account and region.

Requested as part of #17917

Similar example as to the Modern CDK Pipeline Example in #18042

---

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

[gitpod-io]

[skinny85]

Thanks for the contribution @tobytipton! I have a nice idea how can we make this example more integrated into our documentation - let's make it an integ test, and reference it from the ReadMe of this module!

Pull request has been modified.

[skinny85]

I love the format of these examples @tobytipton, but I'd like to iterate on the contents a little bit more before we merge this in. Thanks for your work on this!

[skinny85]

I'm curious - is there any difference in the pipeline Stacks between the two examples? I understand that the service Stack is different, but I think the Pipeline Stacks are the same...? If that's true, can we remove the duplication between the examples? Either merge them to one example that uses the same PipelineStack, but two different service Stacks, or keep two examples, but extract the PipelineStack into a separate class outside of them.

[tobytipton]

This is probably a matter of how verbose we want the examples to be, the difference is just the stages and stacks.

You thinking have a pipeline which creates and deploys in one stage and another stage which imports a different different service to deploy to?

We could probably do that with comments.

I think we can hide parts of the code from the readme as well right?

I could have the pipeline create in a different class and import it and set the pipeline based on that to show the add of the stack with the action. Which would reduce the somewhat boilerplate pipeline aspects.

[skinny85]

You thinking have a pipeline which creates and deploys in one stage and another stage which imports a different different service to deploy to?

No. I meant having one PipelineStack that takes something (maybe a cdk.Stage, maybe something else, like a new interface containing serviceArn, clustrerName, etc. properties?) as the argument, and then doing the correct thing with the passed argument.

[tobytipton]

I think I have addressed some of this duplication.
The big item we want to make sure we have in the example is the fact the you have to import the service again in the pipeline stack. Which if we pass the stage into the pipelinestack, it would be some what hidden to the documentation/example.

Pull request has been modified.

[tobytipton]

@skinny85 after reviewing this more it does work however it has some serious issues.

Because the ECSDeployAction requires an IBaseService and in order to get that to work cross region/account you have to using the import of the ECS service in your pipeline account.

To import the ECS Service you need to have the service ARN and the cluster. To import the cluster you are required to use a VPC, you can either import a VPC or create a new VPC. If you create a new VPC it will most likely run you out of EIPs if you deploy to multiple accounts/regions, unless you create one VPC and leverage it for all of the clusters (which is probably do able), however that means you have to create a VPC in the pipeline account, which you typically would need or have a use for.

With the new ECS service ARN formatting I am wonder if it would be more ideal to be able to leverage the ECS Service ARN on the EcsDeployAction, which would allow you to get the serviceName and clusterName, which is what is needed for the deploy configuration, and this would simplify the cross account/region ECS Deploy Action.

There would need to be a check on the of the ARN to ensure it is a new style vs old and also probably a check to make sure it isn't a full token, the clusterName and serviceName them self could be a token cause that should get addressed.

There is a related bug on fixing the ECS service ARN as well so it doesn't just generate a token #18246

[skinny85]

@tobytipton that all makes sense to me. Were you planning to contribute the change needed in the ECS library to support importing Services with this new-style ARN? If not, could you at least create an issue for the required changes, so this doesn't slip through the cracks? Thanks!

[tobytipton]

The ECS import of service to be an IBaseService is being worked on already in #18140 the change I am proposing here would be a change to the EcsDeployAction to directly take the new style ARN or the service it self., I can create an issue and feature update for it.

[skinny85]

The ECS import of service to be an IBaseService is being worked on already in #18140

Thanks!

the change I am proposing here would be a change to the EcsDeployAction to directly take the new style ARN or the service it self., I can create an issue and feature update for it.

Taking an ARN is out of the question (we don't take strings in the CDK for these type of properties).

Once #18140 lands, I don't think you'll need any changes in EcsDeployAction, no? It already takes the service as an argument.

[tobytipton]

The issue is still you need to pass both a cluster and serviceArn to get an IBaseService to pass to EcsDeployAction.

#18140 is providing support for a serviceArn which include the clusterName, the current doesn't support the new ARN which means if you pass in format with the new ARN will yield a serviceName of clusterName/serviceName in the ECS Deploy Action.

As this example shows getting an service to get an IBaseService to pass to EcsDeployAction to be able to attach it to the pipeline requires either creating or importing a VPC to get the cluster from parameters, to allow the cluster to be used import of the service. Which doing that in a pipeline account isn't ideal, since you probably don't have a need for a VPC in your pipeline account if you are only using the account for orchestrating changes to various stacks/stages.

Having to create an VPC in the pipeline account can have issues if you have more than one CDK package/repo you are using, which is doing multiple pipelines, because there is a limit on how many Elastic IPs, which each VPC takes an Elastic IPs to create the Nat Gateway. I guess you could create the one VPC and then import in allow your CDK packages, by adding it to the cdk.context.json, which would be a possible workaround. However you are still creating a VPC which isn't needed, by anything but to allow the import of an ECS service to deploy to it.

If we don't want to pass in a string ARN to EcsDeployAction then I can think of these options.

1. Modify the Ec2Service.fromEc2ServiceAttributes() and FargateService.fromFargateServiceAttributes() to not require a cluster which means the VPC isn't need, which doesn't seem ideal because that could have other issues.

2. Modify the Ec2Service.fromEc2ServiceArn() and FargateService.fromFargateServiceArn() to set the clusterName based on the Arn. Then update EcsDeployAction to take service as an IBaseServce or ec2Service as an IEc2Service or fargateService as an IFargateService and use the appropriate service type to set the environment and clusterName and serviceName.

[skinny85]

1. Modify the Ec2Service.fromEc2ServiceAttributes() and FargateService.fromFargateServiceAttributes() to not require a cluster which means the VPC isn't need, which doesn't seem ideal because that could have other issues.

I think the changes needed for that are actually in Cluster.fromClusterAttributes(), because neither fromEc2ServiceAttributes() nor fromFargateServiceAttributes() require a VPC today.

I think it's not unreasonable to make vpc in fromClusterAttributes() optional, and simply throw if anyone accesses the vpc property of a Cluster imported this way. This would allow folks to use a service imported that way (without a VPC) in EcsDeployAction.

Would you be interested in contributing this change @tobytipton?

Then update EcsDeployAction to take service as an IBaseServce or ec2Service as an IEc2Service or fargateService as an IFargateService and use the appropriate service type to set the environment and clusterName and serviceName.

I actually don't think this is feasible, nor a great experience, to be honest. I would be against doing that.

2. Modify the Ec2Service.fromEc2ServiceArn() and FargateService.fromFargateServiceArn() to set the clusterName based on the Arn.

What we could try to do here is to make IFargateService and IEc2Service extend IBaseService instead of IService, because IBaseService contains the Cluster. I'm not sure exactly what the ramifications of that would be, but this is a possible alternative to explore for sure.

---

Let me know what you think, and whether you'd be interested in picking up either of the alternatives I presented above.

[tobytipton]

I think the changes needed for that are actually in Cluster.fromClusterAttributes(), because neither fromEc2ServiceAttributes() nor fromFargateServiceAttributes() require a VPC today.

I think it's not unreasonable to make vpc in fromClusterAttributes() optional, and simply throw if anyone accesses the vpc property of a Cluster imported this way. This would allow folks to use a service imported that way (without a VPC) in EcsDeployAction.

Would you be interested in contributing this change @tobytipton?

I can work on a PR for that change.

Then update EcsDeployAction to take service as an IBaseServce or ec2Service as an IEc2Service or fargateService as an IFargateService and use the appropriate service type to set the environment and clusterName and serviceName.

I actually don't think this is feasible, nor a great experience, to be honest. I would be against doing that.

2. Modify the Ec2Service.fromEc2ServiceArn() and FargateService.fromFargateServiceArn() to set the clusterName based on the Arn.

What we could try to do here is to make IFargateService and IEc2Service extend IBaseService instead of IService, because IBaseService contains the Cluster. I'm not sure exactly what the ramifications of that would be, but this is a possible alternative to explore for sure.

That could work but I think it would actually require the VPC being optional in the cluster, because you would have to use using the fromClusterAttributes() to set the cluster to make it extend the IBaseService to have access to the cluster.

I will work on PR for making vpc optional on the fromClusterAttributes().

[skinny85]

Thanks @tobytipton!

[skinny85]

Actually, now that I think about it... How about we introduce a new method to Cluster, fromClusterArn(), instead of changing fromClusterAttributes()? What do you think about that @tobytipton?

[tobytipton]

Actually, now that I think about it... How about we introduce a new method to Cluster, fromClusterArn(), instead of changing fromClusterAttributes()? What do you think about that @tobytipton?

That could be due able, and might solve a different issue.
The issue I am thinking of is ICluster requires a VPC to be set, which is what fromClusterAttributes returns and what fromEc2ServiceAttributes() and fromFargateServiceAttributes() wants for the cluster. If we modify ICluster to make VPC optional it could cause issues with having to change to require vpc when calling it, unless I missed something in my tests.

So if we do the fromClusterArn() maybe we have that return back a different interface like IClusterArn and make ICluster extend that vs IResource then we can change fromEc2ServiceAttributes() and fromFargateServiceAttributes() to require IClusterArn rather than ICluster. And because ICluster extends IClusterArn it shouldn't break any existing usage.

Please let me know if you have a better name for IClusterArn.

[tobytipton]

Actually, now that I think about it... How about we introduce a new method to Cluster, fromClusterArn(), instead of changing fromClusterAttributes()? What do you think about that @tobytipton?

That could be due able, and might solve a different issue. The issue I am thinking of is ICluster requires a VPC to be set, which is what fromClusterAttributes returns and what fromEc2ServiceAttributes() and fromFargateServiceAttributes() wants for the cluster. If we modify ICluster to make VPC optional it could cause issues with having to change to require vpc when calling it, unless I missed something in my tests.

So if we do the fromClusterArn() maybe we have that return back a different interface like IClusterArn and make ICluster extend that vs IResource then we can change fromEc2ServiceAttributes() and fromFargateServiceAttributes() to require IClusterArn rather than ICluster. And because ICluster extends IClusterArn it shouldn't break any existing usage.

Please let me know if you have a better name for IClusterArn.

So adding fromClusterArn() requires it to return the same interface as the cluster based on the signature check which seems to no apply to the fromClusterAttributes based on this.

Should I try and make the return still be an ICluster with the vpc optional which is going to have other issues, or is there a different name we should use than fromClusterArn for example importClusterFromArn?

[skinny85]

Let's make fromClusterArn() return an ICluster, but any attempt to access the vpc property of the returned ICluster instance should throw an exception, with a message explaining that "vpc is not available for a Cluster imported using fromClusterArn(), please use fromClusterAttributes() instead", or something similar.

You can implement a property with a function in TypeScript like this.

[tobytipton]

Thanks the property function was the part I was missing here is the PR for the change #18530

I can update this once we have that merged in.

[tobytipton]

I updated the example after #18530. This might not be needed now as it is should be easier to follow, but I leave that up to you.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 694e07f
• Result: FAILED
• Build Logs (available for 30 days)

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

[skinny85]

@tobytipton what do you think? Is this still valuable, or does #18530 make this so easy, that a big example is not really needed anymore?

BTW, the build is failing:

aws-cdk-lib: aws-codepipeline-actions/test/integ.ecs-pipeline-cross-region-account-existing.lit.ts:10:59 - error TS2307: Cannot find module './ecs-pipeline-cross-region-account-helpers' or its corresponding type declarations.
aws-cdk-lib: 10 import { EcsServiceCrossRegionAccountPipelineStack } from './ecs-pipeline-cross-region-account-helpers';
aws-cdk-lib:                                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
aws-cdk-lib: aws-codepipeline-actions/test/integ.ecs-pipeline-cross-region-account-new.lit.ts:10:59 - error TS2307: Cannot find module './ecs-pipeline-cross-region-account-helpers' or its corresponding type declarations.
aws-cdk-lib: 10 import { EcsServiceCrossRegionAccountPipelineStack } from './ecs-pipeline-cross-region-account-helpers';
aws-cdk-lib:                                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[tobytipton]

I think this larger example isn't probably needed anymore since we already covered this rather well in #18530 I will go ahead and close this out.