feat(ecs): allow adding multiple security groups when creating an ecs service

[robertd]

Closes #2540

@pkandasamy91 @rix0rrr please review.

This PR will allow users to add multiple security groups when creating ECS service (ec2 or Fargate). Currently only single SG can be added to an ECS service while CFN expects list of SGs. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ecs-service-awsvpcconfiguration.html

BREAKING CHANGES:

• ecs: securityGroup: ISecurityGroup replaced by securityGroups: ISecurityGroup[] when
  creating ECS service (ec2 or Fargate) in @aws-cdk/aws-ecs

---

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

[mergify]

Pull Request Checklist

• Testing

• Unit test added (prefer to add a new test rather than modify existing tests)
• CLI change? Re-run/add CLI integration tests

• Documentation

• Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
• README: update module README
• Design: for significant features, follow the design process

• Title uses the format type(scope): text

• Type: fix, feat, refactor go into CHANGELOG, chore is hidden
• Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
• Style: use all lower-case, do not end with a period

• Description

• Rationale: describe rationale of change and approach taken
• Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
• Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

• Sensitive Modules (requires 2 PR approvers)

• IAM document library (in @aws-cdk/aws-iam)
• EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
• Grant APIs (if not based on official documentation with a reference)

[mergify]

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

[robertd]

@pkandasamy91 Do you think this can make it in 1.8.0? We have some production ECS services that depend on this fix?

[eladb]

Hey @pkandasamy91 - ECS is a stable module, we can't introduce breaking changes.

[eladb]

Breaking changes are not allowed in stable modules unless they are bugs that were unusable.

[robertd]

@eladb I understand that this is a breaking change, but cloudformation expects list of SGs, not a single SG. We have some production ECS Services that require multiple SGs, but we don't have ability to create custom SGs (due to DevOps team policy) and we can only use pre-created SGs. We are currently manually updating CFN with additional SGs after initial IaC push with cdk, which is far from ideal. Any help is much appreciated. 👍

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ecs-service-awsvpcconfiguration.html

[skinny85]

The solution here is very simple: introduce a new property, securityGroups: SecurityGroup[], and honor both properties at the same time. It should be very easy to verify the behavior using unit tests.

[skinny85]

@robertd I think escape hatches are exactly what you need for this case - no need to manually edit the CloudFormation template.

[robertd]

@skinny85 I've tried different override before unsuccessfully, but this one seemed to work just fine. Thanks. 👍

My initial try:

const resource = ecsService.node.findChild("Service") as ecs.CfnService;
resource.networkConfiguration.awsvpcConfiguration.securityGroups = [ "sg-12345", "sg-54321"];

Working example:

const cfnService = ecsService.node.findChild("Service") as ecs.CfnService;

//Inject cfn override for multiple SGs
cfnService.addOverride("Properties.NetworkConfiguration.AwsvpcConfiguration.SecurityGroups", [
  secGroupA.securityGroupId, 
  secGroupB.securityGroupId
]);

Output CFN:

NetworkConfiguration:
        AwsvpcConfiguration:
          AssignPublicIp: DISABLED
          SecurityGroups:
            - sg-12345
            - sg-67890
          Subnets:
            - subnet-12345
            - subnet-54321
            - subnet-67890

[robertd]

The solution here is very simple: introduce a new property, securityGroups: SecurityGroup[], and honor both properties at the same time. It should be very easy to verify the behavior using unit tests.

@skinny85 I can give it a try implementing this, but I'm more in favor of setting this straight, rather than adding an additional parameter. I understand the implications downstream, but users would only have to add extra brackets (AFAIK).

Anyway, let's say we do go this route. Do we mark securityGroups as deprecated then?

Talking off the top of my head... I also wonder if implementation would get hairy since we would end up having to deal with securityGroup and securityGroups, not to mention of having to handle cases if both are populated by user.

[eladb]

Yes, we should mark securityGroup as @deprecated and add a new securityGroups property. When we release the next major version of the CDK we will remove all APIs marked as deprecated. Until then, we simply can't break the API of stable modules.

[robertd]

Thanks @eladb ... I'll update this PR accordingly.

edit: I'll try 😄

[robertd]

I've tried few implementations but I'm not satisfied with any of them. 😞

Current implementation expects a single securityGroup to be passed to configureAwsVpcNetworking method in both fargate-service.ts and ec2-service.ts

// fargate-service.ts & ec2-service.ts
this.configureAwsVpcNetworking(props.cluster.vpc, props.assignPublicIp, props.vpcSubnets, props.securityGroup);

That means that configureAwsVpcNetworking method in base-service.ts would have to be updated to receive securityGroups param, which leaves securityGroup vs securityGroups logic to be handled in both fargate-service.ts and ec2-service.ts.

I just can't find clean way to do it and add deprecated warning because any implementation will require post v2.0.0 cleanup of dead code logic mentioned above.

Perhaps we should hold off on this PR until #3398 (v2.0.0)? After all it's only a few weeks away and I already have workaround in place.

Any thoughts?

[mergify]

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

• PR title type(scope): text
  • type: fix, feat, refactor go into CHANGELOG, chore is hidden
  • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
  • text: use all lower-case, do not end with a period, do not include issue refs
• PR Description
  • Rationale: describe rationale of change and approach taken
  • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
  • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
• Testing
  • Unit test added. Prefer to add a new test rather than modify existing tests
  • CLI or init templates change? Re-run/add CLI integration tests
• Documentation
  • README: update module README to describe new features
  • API docs: public APIs must be documented. Copy from official AWS docs when possible
  • Design: for significant features, follow design process

Pull request has been modified.

[vjain16]

@skinny85 I've tried different override before unsuccessfully, but this one seemed to work just fine. Thanks. 👍

My initial try:

const resource = ecsService.node.findChild("Service") as ecs.CfnService;
resource.networkConfiguration.awsvpcConfiguration.securityGroups = [ "sg-12345", "sg-54321"];

Working example:

const cfnService = ecsService.node.findChild("Service") as ecs.CfnService;

//Inject cfn override for multiple SGs
cfnService.addOverride("Properties.NetworkConfiguration.AwsvpcConfiguration.SecurityGroups", [
  secGroupA.securityGroupId, 
  secGroupB.securityGroupId
]);

Output CFN:

NetworkConfiguration:
        AwsvpcConfiguration:
          AssignPublicIp: DISABLED
          SecurityGroups:
            - sg-12345
            - sg-67890
          Subnets:
            - subnet-12345
            - subnet-54321
            - subnet-67890

is there any way I can just add the additional security group instead of replacing it . my problem is the CDK construct ecsPatterns.ApplicationLoadBalancedEc2Service or ecsPatterns.ApplicationLoadBalancedFargateService add some lb security group on the fly and I don't want to touch them , instead I want to add my security group to it .

[piradeepk]

@robertd this looks great, one minor suggestion that I have is, why don't we keep the logic for a single security group as well as separate the logic for a single security group vs an array of security groups.

I have an example that I built for the event targets, but could be applied to this service as well. It should explain what I mean above: https://github.com/pkandasamy91/aws-cdk/pull/2/files#diff-ff9924269c86d48cca4fcca9e89bbd73R89-R100

[robertd]

@pkandasamy91 Please review now and let me know what you think. I'm still unsure where the logic for creating security group if not provided should go. Please advise.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[robertd]

Hi @pkandasamy91. Did you get a chance to review this?

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[efekarakus]

I'm closing this PR and resolved the conflicts in #7813! We'll track the changes there, thanks for the contribution 😊

[robertd]

Yay!!! Glad to see this getting picked up in #7813.

[robertd]

@efekarakus Sorry for not turning on Allow edits and access to secrets by maintainers. For whatever reason I thought it was on. My bad.

[efekarakus]

No problem! thanks a lot for all your work :D