feat(elasticloadbalancingv2): add load balancer lookups #11089

misterjoshua · 2020-10-24T23:46:30Z

Adds fromLookup() methods to both Application and Network load balancers as well as their listeners.

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

gitpod-io · 2020-10-24T23:46:33Z

misterjoshua · 2020-10-27T23:20:59Z

I've looked over my code since I put this PR up. Given that this change entails a new manifest schema version, and that I have implemented looking up security groups by id: Is now a good time to add tag search for security groups?

njlynch

Thanks for the contribution! I would have preferred slightly smaller chunks over something this size -- e.g., just the load balancers by ARNs/tags would have been significant in itself. That being said, we'll take this as-is. I did a first pass of comments, will go deeper on the next round.

packages/@aws-cdk/aws-ec2/lib/security-group.ts

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/base-load-balancer.ts

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/base-listener.ts

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

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-listener.ts

misterjoshua · 2020-10-28T15:42:32Z

Thanks for the review. I'll get to these changes as soon as possible. :)

…onListener

misterjoshua · 2020-10-29T21:55:30Z

@skinny85 I believe I've addressed your review items in the latest commits. I also updated from the main branch. Everything seems to be in working order. I'll continue to keep an eye out for more changes. :)

iliapolo · 2020-11-02T11:40:40Z

@skinny85 @misterjoshua I'm taking a look at the cx-api changes, added do-not-merge in the meantime just in case.

skinny85 · 2020-11-02T18:43:27Z

Thanks @iliapolo !

@rix0rrr if you could take a quick look at just the provider changes, that would be great.

misterjoshua · 2020-11-03T17:21:44Z

While you guys are reviewing, would you prefer that I resolve the merge conflicts now or handle them later?

njlynch · 2020-11-03T17:36:16Z

While you guys are reviewing, would you prefer that I resolve the merge conflicts now or handle them later?

Feel free to resolve them now.

iliapolo · 2020-11-03T19:27:10Z

@njlynch @skinny85 I'm good with the cx-api changes, so removing the do-not-merge I added.

njlynch

Really close, just one legitimate comment and a few stylistics nitpicks.

allowed-breaking-changes.txt

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts

njlynch · 2020-11-05T10:39:20Z

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/base-listener.ts

+        loadBalancerArn: options.userOptions.loadBalancerArn,
+        loadBalancerTags: cxschemaTags,
+        loadBalancerType: options.loadBalancerType,
+      } as cxschema.LoadBalancerListenerContextQuery,


Is this cast really necessary?

The props are a { [key: string]: any }, so the cast isn't strictly necessary. But, I think the type hint is valuable if only for the reader's understanding, as otherwise, the correct type for the context query isn't obvious on inspection. I'll defer to your judgement on this matter if you still feel it should be changed. :)

packages/aws-cdk/lib/context-providers/load-balancers.ts

njlynch

Great! Thanks for the contribution!

mergify · 2020-11-09T11:03:04Z

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

aws-cdk-automation · 2020-11-09T11:31:34Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 488b6d8
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-11-09T11:32:00Z

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

feat: add load balancer lookups

0f2a454

fix: schema version major version bump

1215ffb

misterjoshua marked this pull request as draft October 24, 2020 23:58

misterjoshua changed the title ~~feat: add load balancer lookups~~ feat(elasticloadbalancingv2): add load balancer lookups Oct 24, 2020

misterjoshua added 10 commits October 24, 2020 18:00

fix: yarn update-schema was not run

5efac1b

fix: artifacts from failed auto-refactor

5c542f0

style: doc comment format

e3c6e21

fix: accidental removal of vpc prop

c0175d6

refactor: change dummy security group variable name

fe9d5c0

chore: add test SecurityGroup.fromLookup()

a1a0c74

chore: add tests for security group context provider

b372c87

fix: imports not used

aef71d6

fix: does not identify all traffic egress

d2ab397

chore: add loadAssemblyManifest to allowed breaking changes

9c1ae33

misterjoshua marked this pull request as ready for review October 25, 2020 05:30

misterjoshua added 2 commits October 24, 2020 23:52

fix: artifacts from other failed auto-refactor

cf4da2d

fix: typos in doc comments

5e5ffef

njlynch requested review from njlynch and a team October 26, 2020 11:59

njlynch self-assigned this Oct 26, 2020

njlynch previously requested changes Oct 28, 2020

View reviewed changes

misterjoshua added 6 commits October 28, 2020 10:01

fix: describeLoadBalancers missing NextMarker

83a9dd9

fix: security group should check for tokens

f3b3d92

style: remove securityGroupId key

79a81ec

fix: dummy attributes should be obviously false

2bc7750

refactor: use dummyValue getValue input

0cc3f6a

fix: use === and \!== throughout

1c71d21

misterjoshua added 6 commits October 29, 2020 13:52

refactor: rename ImportedApplicationListenerBase -> ExternalApplicati…

7447424

…onListener

refactor: move listenerArn to ApplicationListenerLookupOptions

85a2091

docs: improve description of allowAllOutbound

8928287

fix: public accessor missing from SecurityGroup.fromLookup

6a4e369

fix: remove out of date comment

f4c332a

Merge branch 'master' of github.com:aws/aws-cdk into elbv2-lookups

8c2c734

Merge branch 'master' into elbv2-lookups

d23f909

iliapolo added the pr/do-not-merge This PR should not be merged at this time. label Nov 2, 2020

misterjoshua mentioned this pull request Nov 2, 2020

feat(ecs-service-extensions): add ALB listener rule support #10938

Closed

Merge branch 'master' of github.com:aws/aws-cdk into elbv2-lookups

f4c84e5

iliapolo removed the pr/do-not-merge This PR should not be merged at this time. label Nov 3, 2020

njlynch reviewed Nov 5, 2020

View reviewed changes

misterjoshua and others added 5 commits November 5, 2020 09:19

fix: missing public modifier

f0ecfcd

style: use ternary for if else

0044974

chore: relocate allowed breaking change per review

2f2a992

Merge branch 'master' into elbv2-lookups

19dc764

Merge branch 'master' into elbv2-lookups

e0d3b9c

njlynch approved these changes Nov 9, 2020

View reviewed changes

Merge branch 'master' into elbv2-lookups

488b6d8

mergify bot merged commit 0153028 into aws:master Nov 9, 2020

njlynch mentioned this pull request Aug 24, 2021

Add Security Group lookup by name #4241

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(elasticloadbalancingv2): add load balancer lookups #11089

feat(elasticloadbalancingv2): add load balancer lookups #11089

misterjoshua commented Oct 24, 2020 •

edited

Loading

gitpod-io bot commented Oct 24, 2020 •

edited

Loading

misterjoshua commented Oct 27, 2020

njlynch left a comment

misterjoshua commented Oct 28, 2020

misterjoshua commented Oct 29, 2020

iliapolo commented Nov 2, 2020

skinny85 commented Nov 2, 2020

misterjoshua commented Nov 3, 2020 •

edited

Loading

njlynch commented Nov 3, 2020

iliapolo commented Nov 3, 2020

njlynch left a comment

njlynch Nov 5, 2020

misterjoshua Nov 5, 2020 •

edited

Loading

njlynch left a comment

mergify bot commented Nov 9, 2020

aws-cdk-automation commented Nov 9, 2020

mergify bot commented Nov 9, 2020

feat(elasticloadbalancingv2): add load balancer lookups #11089

feat(elasticloadbalancingv2): add load balancer lookups #11089

Conversation

misterjoshua commented Oct 24, 2020 • edited Loading

gitpod-io bot commented Oct 24, 2020 • edited Loading

misterjoshua commented Oct 27, 2020

njlynch left a comment

Choose a reason for hiding this comment

misterjoshua commented Oct 28, 2020

misterjoshua commented Oct 29, 2020

iliapolo commented Nov 2, 2020

skinny85 commented Nov 2, 2020

misterjoshua commented Nov 3, 2020 • edited Loading

njlynch commented Nov 3, 2020

iliapolo commented Nov 3, 2020

njlynch left a comment

Choose a reason for hiding this comment

njlynch Nov 5, 2020

Choose a reason for hiding this comment

misterjoshua Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

njlynch left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 9, 2020

aws-cdk-automation commented Nov 9, 2020

AWS CodeBuild CI Report

mergify bot commented Nov 9, 2020

misterjoshua commented Oct 24, 2020 •

edited

Loading

gitpod-io bot commented Oct 24, 2020 •

edited

Loading

misterjoshua commented Nov 3, 2020 •

edited

Loading

misterjoshua Nov 5, 2020 •

edited

Loading