Skip to content

Conversation

@msambol
Copy link
Contributor

@msambol msambol commented Nov 14, 2023

Closes #26735.


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

@aws-cdk-automation aws-cdk-automation requested a review from a team November 14, 2023 03:47
@github-actions github-actions bot added admired-contributor [Pilot] contributed between 13-24 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Nov 14, 2023
@msambol
Copy link
Contributor Author

msambol commented Nov 14, 2023

I was looking at adding addSecurityGroup as well but it's a little convoluted because security groups get set in super. I can spend more time on this if we want it.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 14, 2023
mikewrighton
mikewrighton previously approved these changes Nov 14, 2023
@mergify
Copy link
Contributor

mergify bot commented Nov 14, 2023

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

@mergify mergify bot dismissed mikewrighton’s stale review November 14, 2023 21:42

Pull request has been modified.

@msambol msambol requested a review from mikewrighton November 15, 2023 12:45
@msambol
Copy link
Contributor Author

msambol commented Nov 17, 2023

@mikewrighton This good to merge?

mikewrighton
mikewrighton previously approved these changes Nov 20, 2023
@mergify
Copy link
Contributor

mergify bot commented Nov 20, 2023

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

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 20, 2023
@mergify
Copy link
Contributor

mergify bot commented Nov 20, 2023

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

@vinayak-kukreja
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Nov 20, 2023

update

❌ Mergify doesn't have permission to update

Details

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/pr-linter.yml without workflows permission

@mergify mergify bot dismissed mikewrighton’s stale review November 20, 2023 21:58

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 20, 2023
@mergify
Copy link
Contributor

mergify bot commented Nov 21, 2023

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

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 21, 2023
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: f859f78
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 645d4e9 into aws:main Nov 21, 2023
@mergify
Copy link
Contributor

mergify bot commented Nov 21, 2023

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

mergify bot pushed a commit that referenced this pull request Jan 16, 2024
# Summary
Implement an `IConnectable` interface to a NetworkLoadBalancer. 

# Why need this change?
AWS CDK has great features for abstraction. `IConnectable` interface is one of this. `IConnectable` simplifies the management of security groups. AWS CDK add support security group to NLB at #27978. However, Currently NLB not implement `IConnectable`, so customers can't use useful interface in AWS CDK.

# Example use case

```ts
declare const lb: elbv2.NetworkLoadBalancer;
declare const asg: autoscaling.AutoScalingGroup;

asg.connections.allowFrom(lb, ec2.Port.tcp(8088)); // currently can not
```

Closes #26735

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit that referenced this pull request Oct 29, 2025
…by default (under feature flag) (#34675)

### Issue # (if applicable)

Closes #34606.

### Reason for this change

Currently, CDK's L2 constructs allow setting security groups for NLBs, but this requires explicit configuration. 

```ts
declare const sg1: ec2.ISecurityGroup;

const lb = new elbv2.NetworkLoadBalancer(this, 'LB', {
  vpc,
  securityGroups: [sg1], // configure SG explicitly
});
```

This was not originally intended - NLB security group support was implemented later, and the current specification exists to maintain backward compatibility.

#27978
#28494


However, when comparing NLBs without security groups to NLBs with security groups configured, the latter has significantly more advantages. Furthermore, once an NLB is created without security groups, it's impossible to add security group configuration later.

Therefore, I propose using feature flags to make security group configuration the default for NLBs in CDK.

### Description of changes

- Add `@aws-cdk/aws-elasticloadbalancingv2:networkLoadBalancerWithSecurityGroupByDefault` feature flag
- Create security groups by default when feature flags are enabled

### Describe any new or updated permissions being added

None

### Description of how you validated changes

Add both unit and integ tests

### Other information

[This implementation](#34606 (comment)) was also proposed in the issue, but it was not implemented because it was difficult to detect when referenced from other Connectables as follows case2.


```ts
declare const nlb: elbv2.INetworkLoadBalancer;
declare const other: IConnectable;

// case1
nlb.connections.allowTo(other, ec2.Port.tcp(1234));

// case2
other.connections.allowTo(nlb, ec2.Port.tcp(2181));
```



### 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

admired-contributor [Pilot] contributed between 13-24 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(aws-elasticloadbalancingv2): (network-load-balancer-supports-security-groups)

4 participants