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

feat(elbv2): default config for internal load balancer will fall back to Isolated subnets #5696

Merged

Conversation

flemjame-at-amazon
Copy link
Contributor

@flemjame-at-amazon flemjame-at-amazon commented Jan 7, 2020


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

Added a secondary default subnet selection for Load Balancers such that, if I have an isolated VPC like so:

Vpc vpc = new Vpc(construct, "IsolatedVpc", VpcProps.builder()
        .subnetConfiguration(
                Arrays.asList(
                        SubnetConfiguration.builder()
                                .subnetType(SubnetType.ISOLATED)
                                .name("isolated").build()
                ))
        .build());

I don't have to specify the subnets for an internal load balancer. Currently, users must specify their Isolated subnets like so:

NetworkLoadBalancer nlb = new NetworkLoadBalancer(this, "NLB", NetworkLoadBalancerProps.builder()
        .internetFacing(false)
        .vpcSubnets(SubnetSelection.builder()
                .subnetType(SubnetType.ISOLATED)
                .build())
        .vpc(vpc)
        .build());

With this change, if there are no Private subnets and internetFacing is false, then Isolated subnets will be selected, allowing users to code their NLB like so:

NetworkLoadBalancer nlb = new NetworkLoadBalancer(this, "NLB", NetworkLoadBalancerProps.builder()
        .internetFacing(false)
        .vpc(vpc)
        .build());

This doesn't break the default behavior, which places a LB into Private subnets in the event that both Private and Isolated subnets exist. Users who are already specifying Isolated subnets can continue to do so, as I did not change the logic if the user specifies vpcSubnets.

Commit Message

feat(elbv2): default config for internal load balancer will fall back to Isolated subnets

Internal load balancers used to be always created in Private subnets, and hence fail if the VPC does not include Private subnets.

Now using the default VPC "private subnets" fallback mechanism which fixes this case.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

rix0rrr
rix0rrr previously requested changes Jan 9, 2020
const vpcSubnets = ifUndefined(baseProps.vpcSubnets,
{ subnetType: internetFacing ? ec2.SubnetType.PUBLIC : ec2.SubnetType.PRIVATE });
{ subnetType: vpcSubnetType });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently updated Vpc to have some of this behavior already (private => isolated as a fallback).

Can you try the following and see if that behaves the same?

const vpcSubnets = ... (internetFacing ? { subnetType: PUBLIC } : {});

I.e. leave subnetType empty, rely on the Vpc default to try private => isolated => public in that order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That just worked, no additional changes to tests needed.

@mergify mergify bot dismissed rix0rrr’s stale review January 9, 2020 22:42

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot dismissed rix0rrr’s stale review January 13, 2020 17:11

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@rix0rrr rix0rrr changed the title feat(aws-elasticloadbalancingv2): Allow creation of internal load balancers in an Isolated VPC without requiring a subnet configuration feat(elbv2): default config for internal load balancer will fall back to Isolated subnets Jan 14, 2020
@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit 8b9c1fc into aws:master Jan 14, 2020
@flemjame-at-amazon flemjame-at-amazon deleted the lb-isolated-subnet-default-support branch September 2, 2020 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants