fix(ecs-patterns): allow imported load balancers as inputs #5461

bvtujo · 2019-12-17T17:38:28Z

This PR adds functionality in aws-ecs-patterns to allow imported Network and Application load balancers to be used when initializing the LoadBalancedEC2Service and LoadBalancedFargateService constructs. By necessity, this means:

editing the Network and Application load balancer constructs to add an optional IVpc property to be used when calling the fromLoadBalancerAttributes static methods, and
changing the error which is thrown when calling addTargets on imported load balancers.

Unit tests are added in aws-ecs-patterns to test the new import functionality and in aws-elasticloadbalancingv2 to ensure that addTargets behaves properly when a Vpc is or is not specified for imported load balancers.

Resolves: #5209

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

Commit Message

fix(ecs-patterns): allow imported load balancers as inputs

This PR adds functionality in aws-ecs-patterns to allow imported Network and Application load balancers to be used when initializing the LoadBalancedEC2Service and LoadBalancedFargateService constructs. By necessity, this means:

editing the Network and Application load balancer constructs to add an optional IVpc property to be used when calling the fromLoadBalancerAttributes static methods, and
changing the error which is thrown when calling addTargets on imported load balancers.

Unit tests are added in aws-ecs-patterns to test the new import functionality and in aws-elasticloadbalancingv2 to ensure that addTargets behaves properly when a Vpc is or is not specified for imported load balancers.

Resolves: #5209

aws-cdk-automation · 2019-12-17T17:58:51Z

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 · 2019-12-17T18:01:00Z

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 · 2019-12-17T18:23:02Z

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 · 2019-12-18T19:15:43Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

packages/@aws-cdk/aws-ecs-patterns/test/ec2/test.l3s.ts

packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.load-balanced-fargate-service.ts

Pull request has been modified.

aws-cdk-automation · 2019-12-20T00:22:03Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

…ted lbs

aws-cdk-automation · 2019-12-20T01:02:40Z

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 · 2019-12-20T01:25:13Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

Pull request has been modified.

piradeepk

This is awesome! Ship it!

@rix0rrr please take a look at the imported load balancer construct changes

aws-cdk-automation · 2020-02-05T18:06:41Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

Pull request has been modified.

aws-cdk-automation · 2020-02-05T19:50:33Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

piradeepk

Ship it!

Pull request has been modified.

piradeepk

Ship it!

aws-cdk-automation · 2020-02-07T16:46:09Z

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 · 2020-02-10T09:16:14Z

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts

@@ -297,12 +300,14 @@ export abstract class ApplicationLoadBalancedServiceBase extends cdk.Construct {
      internetFacing
    };

-    this.loadBalancer = props.loadBalancer !== undefined ? props.loadBalancer : new ApplicationLoadBalancer(this, 'LB', lbProps);
+    this.loadBalancer = props.loadBalancer !== undefined ? props.loadBalancer as ApplicationLoadBalancer


Wait, this is dangerous. Why is the input argument changed to IApplicationLoadBalancer, but it's downcast to a concrete class here?

To completely explain: this cast is not necessarily correct. For example, for an imported load balancer this cast is DEFINITELY NOT correct, as an ApplicationLoadBalancer is going to have methods that an ImportedApplicationLoadBalancer does not have, and if you try to use those you will get crashes.

If the problem is that this.loadBalancer has been typed as an ApplicationLoadBalancer and the jsii-diff tool won't let you change it to IApplicationLoadBalancer for fear of breaking backwards compatibility, and so you must trick the type system, this is not the way to do it! A way to "properly" handle this case while not breaking backwards compatibility would be something like this:

class { private readonly _applicationLoadBalancer?: ApplicationLoadBalancer; constructor() { if (props.loadBalancer instanceof ApplicationLoadBalancer) { this._applicationLoadBalancer = props.loadBalancer; } } public get applicationLoadBalancer(): ApplicationLoadBalancer { if (!this._applicationLoadBalancer) { throw new Error('.applicationLoadBalancer can only accessed if the class was constructed with an owned load balancer, not an imported one'); } return this._applicationLoadBalancer; } }

That is, we still promise that the return type of applicationLoadBalancer will be a proper ApplicationLoadBalancer, but we just don't make that guarantee in 100% of the cases anymore.

Not ideal, but the best we can do given the contracts we uphold.

Thanks for the detailed feedback, this was really helpful in understanding the guards we put against backwards compatibility.

rix0rrr · 2020-02-10T09:16:30Z

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts


    if (props.certificate !== undefined && props.protocol !== undefined && props.protocol !== ApplicationProtocol.HTTPS) {
      throw new Error('The HTTPS protocol must be used when a certificate is given');
    }
-    const protocol = props.protocol !== undefined ? props.protocol : (props.certificate ? ApplicationProtocol.HTTPS : ApplicationProtocol.HTTP);
+    const protocol = props.protocol !== undefined ? props.protocol :
+    (props.certificate ? ApplicationProtocol.HTTPS : ApplicationProtocol.HTTP);


Indentation please.

Resolved in 57cd88a

rix0rrr · 2020-02-10T09:16:46Z

packages/@aws-cdk/aws-ecs-patterns/lib/base/network-load-balanced-service-base.ts

@@ -268,7 +270,7 @@ export abstract class NetworkLoadBalancedServiceBase extends cdk.Construct {
      internetFacing
    };

-    this.loadBalancer = props.loadBalancer !== undefined ? props.loadBalancer : new NetworkLoadBalancer(this, 'LB', lbProps);
+    this.loadBalancer = props.loadBalancer !== undefined ? props.loadBalancer as NetworkLoadBalancer : new NetworkLoadBalancer(this, 'LB', lbProps);


Same comment.

rix0rrr · 2020-02-10T09:19:43Z

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/nlb/network-load-balancer.ts

+   * @default - When not provided, listeners cannot be created on imported load
+   * balancers.
+   */
+  readonly loadBalancerVpc?: ec2.IVpc;


On the application load balancer this prop is called vpc, why is it called loadBalancerVpc here? Please make both the same. vpc will suffice.

Resolved in 57cd88a

…orted-lb

…into bvtujo/allow-imported-lb

Pull request has been modified.

piradeepk

Great work @bvtujo!

mergify · 2020-02-12T18:56:29Z

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

bvtujo requested a review from a team December 17, 2019 17:38

bvtujo requested review from rix0rrr and SoManyHs as code owners December 17, 2019 17:38

bvtujo requested review from piradeepk and removed request for a team, rix0rrr and SoManyHs December 17, 2019 17:39

piradeepk changed the title ~~feat(aws-ecs-patterns): allow imported load balancers in l3 constructs~~ feat(aws-ecs-patterns): allow imported load balancers in alb/nlb constructs Dec 17, 2019

piradeepk changed the title ~~feat(aws-ecs-patterns): allow imported load balancers in alb/nlb constructs~~ feat(ecs-patterns): allow imported load balancers in alb/nlb constructs Dec 17, 2019

piradeepk requested a review from SoManyHs December 17, 2019 17:43

bvtujo force-pushed the bvtujo/allow-imported-lb branch from 0ad5580 to 74d8bcc Compare December 17, 2019 18:02

SomayaB assigned SoManyHs and piradeepk Dec 17, 2019

SomayaB added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Dec 17, 2019

bvtujo mentioned this pull request Dec 18, 2019

aws-ecs-patterns: Substituting IApplicationLoadBalancer for ApplicationLoadBalancer results in JSII breaking change alert #5465

Closed

bvtujo force-pushed the bvtujo/allow-imported-lb branch from 74d8bcc to c21a848 Compare December 18, 2019 18:44

piradeepk previously requested changes Dec 18, 2019

View reviewed changes

feat(ecs-patterns): allow imported load balancers in alb/nlb constructs

2e576f4

bvtujo force-pushed the bvtujo/allow-imported-lb branch from c21a848 to 2e576f4 Compare December 20, 2019 00:08

bvtujo added 2 commits December 19, 2019 16:29

fix failing tests

1aeea91

Check load balancer resources in unit tests involving previously crea…

08dcda3

…ted lbs

Merge branch 'master' into bvtujo/allow-imported-lb

30b6c16

piradeepk previously approved these changes Feb 5, 2020

View reviewed changes

Merge branch 'master' into bvtujo/allow-imported-lb

0e4c0b1

bvtujo mentioned this pull request Feb 5, 2020

feat: add typescript load balanced fargate service using shared load balancer aws-samples/aws-cdk-examples#240

Closed

Merge branch 'master' into bvtujo/allow-imported-lb

3a0b3bc

piradeepk previously approved these changes Feb 7, 2020

View reviewed changes

rix0rrr previously requested changes Feb 10, 2020

View reviewed changes

rix0rrr self-assigned this Feb 10, 2020

bvtujo added 3 commits February 11, 2020 16:26

Merge branch 'master' of github.com:aws/aws-cdk into bvtujo/allow-imp…

9ba9d5c

…orted-lb

Refactor .loadBalancer properties to use getter pattern

57cd88a

Merge branch 'bvtujo/allow-imported-lb' of github.com:bvtujo/aws-cdk …

acedf99

…into bvtujo/allow-imported-lb

rix0rrr changed the title ~~feat(ecs-patterns): allow imported load balancers as inputs~~ fix(ecs-patterns): allow imported load balancers as inputs Feb 12, 2020

rix0rrr previously approved these changes Feb 12, 2020

View reviewed changes

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Feb 12, 2020

Merge branch 'master' into bvtujo/allow-imported-lb

cc61cb0

piradeepk approved these changes Feb 12, 2020

View reviewed changes

mergify bot merged commit 7f8c90d into aws:master Feb 12, 2020

This was referenced Feb 24, 2020

Unclear how to use ecs-patterns NetworkMultipleTargetGroupsEc2Service #6263

Closed

Cognito authorizer support for ecs-patterns/ApplicationLoadBalanced*Service #5849

Closed

allisaurus mentioned this pull request Jun 3, 2020

Create ECS Service with existing task definition ARN #6240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ecs-patterns): allow imported load balancers as inputs #5461

fix(ecs-patterns): allow imported load balancers as inputs #5461

bvtujo commented Dec 17, 2019 •

edited by rix0rrr

Loading

aws-cdk-automation commented Dec 17, 2019

aws-cdk-automation commented Dec 17, 2019

aws-cdk-automation commented Dec 17, 2019

aws-cdk-automation commented Dec 18, 2019

aws-cdk-automation commented Dec 20, 2019

aws-cdk-automation commented Dec 20, 2019

aws-cdk-automation commented Dec 20, 2019

piradeepk left a comment

aws-cdk-automation commented Feb 5, 2020

aws-cdk-automation commented Feb 5, 2020

piradeepk left a comment

piradeepk left a comment

aws-cdk-automation commented Feb 7, 2020

rix0rrr Feb 10, 2020

rix0rrr Feb 10, 2020 •

edited

Loading

bvtujo Feb 12, 2020

rix0rrr Feb 10, 2020

bvtujo Feb 12, 2020

rix0rrr Feb 10, 2020

rix0rrr Feb 10, 2020

bvtujo Feb 12, 2020

piradeepk left a comment

mergify bot commented Feb 12, 2020

fix(ecs-patterns): allow imported load balancers as inputs #5461

fix(ecs-patterns): allow imported load balancers as inputs #5461

Conversation

bvtujo commented Dec 17, 2019 • edited by rix0rrr Loading

Commit Message

aws-cdk-automation commented Dec 17, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 17, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 17, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 18, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 20, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 20, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 20, 2019

AWS CodeBuild CI Report

piradeepk left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 5, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 5, 2020

AWS CodeBuild CI Report

piradeepk left a comment

Choose a reason for hiding this comment

piradeepk left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 7, 2020

AWS CodeBuild CI Report

rix0rrr Feb 10, 2020

Choose a reason for hiding this comment

rix0rrr Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

bvtujo Feb 12, 2020

Choose a reason for hiding this comment

rix0rrr Feb 10, 2020

Choose a reason for hiding this comment

bvtujo Feb 12, 2020

Choose a reason for hiding this comment

rix0rrr Feb 10, 2020

Choose a reason for hiding this comment

rix0rrr Feb 10, 2020

Choose a reason for hiding this comment

bvtujo Feb 12, 2020

Choose a reason for hiding this comment

piradeepk left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 12, 2020

bvtujo commented Dec 17, 2019 •

edited by rix0rrr

Loading

rix0rrr Feb 10, 2020 •

edited

Loading