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: enable importing public-facing ALBs #5438

Merged
merged 55 commits into from
Nov 8, 2023
Merged

Conversation

huanjani
Copy link
Contributor

@huanjani huanjani commented Nov 3, 2023

Import an existing ALB that is within the deployment environment's VPC by adding

http:
  alb: [name or ARN of ALB]

to a Load-Balanced Web Service's manifest.

Related: #3319, #1457, #3936.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@huanjani huanjani requested a review from a team as a code owner November 3, 2023 00:29
@huanjani huanjani requested review from iamhopaul123 and removed request for a team November 3, 2023 00:29
@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2023

Codecov Report

Attention: 49 lines in your changes are missing coverage. Please review.

Comparison is base (baf2a78) 69.91% compared to head (413fa79) 69.94%.
Report is 10 commits behind head on mainline.

Additional details and impacted files
@@             Coverage Diff              @@
##           mainline    #5438      +/-   ##
============================================
+ Coverage     69.91%   69.94%   +0.02%     
============================================
  Files           299      301       +2     
  Lines         45497    45901     +404     
  Branches        295      295              
============================================
+ Hits          31810    32105     +295     
- Misses        12141    12232      +91     
- Partials       1546     1564      +18     
Files Coverage Δ
...ustom-resources/lib/alb-rule-priority-generator.js 98.82% <ø> (ø)
...al/pkg/deploy/cloudformation/stack/transformers.go 88.41% <100.00%> (+0.29%) ⬆️
internal/pkg/describe/lb_web_service.go 65.47% <ø> (ø)
internal/pkg/describe/uri.go 78.71% <100.00%> (+2.28%) ⬆️
internal/pkg/manifest/http.go 76.76% <ø> (ø)
internal/pkg/aws/elbv2/elbv2.go 91.53% <88.00%> (-1.63%) ⬇️
...rnal/pkg/deploy/cloudformation/stack/lb_web_svc.go 83.01% <66.66%> (-0.58%) ⬇️
internal/pkg/template/workload.go 50.59% <29.41%> (-2.33%) ⬇️
internal/pkg/cli/deploy/lbws.go 50.41% <20.51%> (-6.66%) ⬇️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Nov 3, 2023

🍕 Here are the new binary sizes!

Name New size (kiB) size (kiB) Delta (%)
macOS (amd) 52348 51976 +0.72
macOS (arm) 53212 52812 +0.76
linux (amd) 46044 45716 +0.72
linux (arm) 45312 45056 +0.57
windows (amd) 43496 43168 +0.76

internal/pkg/cli/deploy/lbws.go Outdated Show resolved Hide resolved
internal/pkg/deploy/cloudformation/stack/transformers.go Outdated Show resolved Hide resolved
Type: AWS::ElasticLoadBalancingV2::ListenerRule
Properties:
Actions:
- Type: redirect
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe not in this release, but we might want to consider the http.redirect_to_https manifest field like in

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

The PR mostly looks good to me. Awesome! My main concern is regarding the new imported-alb-resources file (+1 to wx's fb and see comments below)

internal/pkg/template/workload.go Outdated Show resolved Hide resolved
internal/pkg/template/workload.go Outdated Show resolved Hide resolved
internal/pkg/template/workload.go Outdated Show resolved Hide resolved
site/content/docs/concepts/services.en.md Outdated Show resolved Hide resolved
Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@Lou1415926 Lou1415926 left a comment

Choose a reason for hiding this comment

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

feel free to remove the label whenever you are ready!

internal/pkg/describe/uri.go Outdated Show resolved Hide resolved
@Lou1415926 Lou1415926 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 8, 2023
@huanjani huanjani removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 8, 2023
@mergify mergify bot merged commit 80d4794 into aws:mainline Nov 8, 2023
12 checks passed
@huanjani huanjani deleted the imported-albs branch November 16, 2023 22:38
mergify bot pushed a commit that referenced this pull request Jun 3, 2024
Related: #5438. 
Integ test changes in #5483.

#5483 adds the Load Balancer DNS name to the service as an env var (and therefore appears in `svc show` output. However, it is not (yet) included in URI output as a recommended action in `svc deploy` output. 

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
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

4 participants