Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ecs-patterns: enable to specify securityGroups in NetworkLoadBalancedFargateService #29430

Closed
1 of 2 tasks
wafuwafu13 opened this issue Mar 11, 2024 · 3 comments · Fixed by #29431 · May be fixed by NOUIY/aws-solutions-constructs#98 or NOUIY/aws-solutions-constructs#99
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library feature-request A feature should be added or improved. p1

Comments

@wafuwafu13
Copy link
Contributor

Describe the feature

We can't specify securityGroups in NetworkLoadBalancedFargateService now.

Use Case

Default is an empty array, so we have to specify securityGroups.

/**
* After the implementation of `IConnectable` (see https://github.com/aws/aws-cdk/pull/28494), the default
* value for `securityGroups` is set by the `ec2.Connections` constructor to an empty array.
* To keep backward compatibility (`securityGroups` is `undefined` if the related property is not specified)
* a getter has been added.
*/
public get securityGroups(): string[] | undefined {
return this.isSecurityGroupsPropertyDefined || this.connections.securityGroups.length
? this.connections.securityGroups.map(sg => sg.securityGroupId)
: undefined;
}

Proposed Solution

Add securityGroups property to NetworkLoadBalancedFargateServiceProps.

/**
* The properties for the NetworkLoadBalancedFargateService service.
*/
export interface NetworkLoadBalancedFargateServiceProps extends NetworkLoadBalancedServiceBaseProps, FargateServiceBaseProps {
/**
* Determines whether the service will be assigned a public IP address.
*
* @default false
*/
readonly assignPublicIp?: boolean;
/**
* The subnets to associate with the service.
*
* @default - Public subnets if `assignPublicIp` is set, otherwise the first available one of Private, Isolated, Public, in that order.
*/
readonly taskSubnets?: SubnetSelection;
}

Other Information

ApplicationLoadBalancedFargateServiceProps has securityGroups property already.

/**
* The security groups to associate with the service. If you do not specify a security group, a new security group is created.
*
* @default - A new security group is created.
*/
readonly securityGroups?: ISecurityGroup[];

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

v2.132.0

Environment details (OS name and version, etc.)

Darwin tagawahirotakanoMacBook-Pro.local 23.2.0 Darwin Kernel Version 23.2.0: Wed Nov 15 21:53:18 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T6000 arm64

@wafuwafu13 wafuwafu13 added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Mar 11, 2024
@github-actions github-actions bot added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Mar 11, 2024
@pahud
Copy link
Contributor

pahud commented Mar 11, 2024

Yes I think now that NLB supports security group, we probably should expose that as a optional prop to the surface. I will invite the maintainers for more input on this.

@pahud pahud added p2 p1 and removed needs-triage This issue or PR still needs to be triaged. p2 labels Mar 11, 2024
@xazhao
Copy link
Contributor

xazhao commented Mar 12, 2024

The proposed solution looks good to me. I see there's already a PR opened to fix the issue. Thanks for the contribution. Will review the PR.

@mergify mergify bot closed this as completed in #29431 Mar 13, 2024
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

mergify bot pushed a commit that referenced this issue Mar 13, 2024
…argateService` (#29431)

### Issue # (if applicable)

Closes #29430

(related to #29186 (comment))

### Reason for this change


We can't specify `securityGroups` in `NetworkLoadBalancedFargateService` now.

### Description of changes


- Add `securityGroups` property to `NetworkLoadBalancedFargateServiceProps`.
- Add unit test
- Add integ test

### Description of how you validated changes


- Pass unit test
- Pass integ test

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library feature-request A feature should be added or improved. p1
Projects
None yet
3 participants