feat: Support Multiple ALB Ingresses

[n888]

Enhancement proposal: #2471

Changes:

• Update the ALB datatype to have a field called AdditionalIngresses to allow for multiple additional ingresses instead of just just the one

• This naming convention is modeled after AdditionalIngressAnnotations

• Modify the SetWeight method to create a canary for each ingress and apply new weight to all canary ingresses

• Update validations to validate all ingresses.

• Designed to be fully backwards compatible

• Modeled after the Nginx Multi Ingress patch: feat: support N nginx ingresses #2467

• Open to all direction/comments/suggestions, Thank you

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
• The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
• I've signed my commits with DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My builds are green. Try syncing with master if they are not.
• My organization is added to USERS.md.

[github-actions]

Go Published Test Results

2 030 tests   2 030 ✔️  2m 39s ⏱️
   118 suites         0 💤
       1 files           0 ❌

Results for commit e39aff0.

♻️ This comment has been updated with latest results.

[github-actions]

E2E Tests Published Test Results

    4 files      4 suites   3h 35m 28s ⏱️
102 tests   84 ✔️   6 💤 12 ❌
434 runs  384 ✔️ 24 💤 26 ❌

For more details on these failures, see this check.

Results for commit e39aff0.

♻️ This comment has been updated with latest results.

[codecov]

Codecov Report

Patch coverage: 80.12% and project coverage change: +0.02 🎉

Comparison is base (8db151f) 81.65% compared to head (e39aff0) 81.68%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2639      +/-   ##
==========================================
+ Coverage   81.65%   81.68%   +0.02%     
==========================================
  Files         133      133              
  Lines       20203    20309     +106     
==========================================
+ Hits        16497    16589      +92     
- Misses       2851     2862      +11     
- Partials      855      858       +3     

Impacted Files	Coverage Δ
utils/ingress/ingress.go	96.36% <73.33%> (-1.69%)	⬇️
.../apis/rollouts/validation/validation_references.go	89.68% <75.67%> (-0.76%)	⬇️
rollout/trafficrouting/alb/alb.go	80.91% <80.65%> (+1.82%)	⬆️
rollout/controller.go	81.08% <86.36%> (+<0.01%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[zachaller]

Can we switch to dropping the additional portion of the spec and just making it plural ingresses can you also add docs and e2e tests.

[huynhsontung]

@zachaller Do we want to keep the singular ingress field? We named it additionalIngresses to exist alongside with ingress. Removing ingress would make a breaking change on the spec. Having both ingress and ingresses would be confusing imo.

[zachaller]

@zachaller Do we want to keep the singular ingress field? We named it additionalIngresses to exist alongside with ingress. Removing ingress would make a breaking change on the spec. Having both ingress and ingresses would be confusing imo.

Yes we will keep the singular ignress for backward compatability. There would be validation that only lets you use one or the other. You can take a look at the recent PR merged for nginx and also istio and other traffic routers use the plural form to mark multiple items supported. Then whenever we decide to break api we would probably drop the singular form.

[n888]

Thanks for the info @zachaller @huynhsontung, I will get on the ingresses change and e2e tests.

[huynhsontung]

@zachaller Any updates on the review process? It's been a while and this feature is quite critical for us as well.

[zachaller]

I will be focusing on open source here for a bit and this is one of my top priorities after I get some notification engine stuff done. I will also make sure this gets included in 1.6

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[zachaller]

Sorry for the long review time, thanks for the contributions!

[n888]

Thank you @zachaller @huynhsontung & Argo Team 💯

[thecosmicfrog]

This is fantastic @n888 @zachaller @huynhsontung! As it happens, we are looking into Argo Rollouts for our canary rollout requirements and... you guessed it... we need support for multiple ALB ingresses. What excellent timing!

Can you advise when this feature will be released? We are using the Helm chart for installing the controller and CRDs. Thanks!

[zachaller]

It will be in 1.6, rc should come out this week hopefully.

[thecosmicfrog]

Great, thanks @zachaller!