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(trafficrouting): support ingresses with multiple service ports in TrafficRouting.ALB spec #3551

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ArenSH
Copy link

@ArenSH ArenSH commented May 2, 2024

Overview

This request introduces new .ServicePorts field for TrafficRouting.ALB to manage ingresses with multiple ports.

Implementation details

There were several options to add ability to manage ingresses with multiple ports:

  1. Change behavior and format of .Ingresses field (❌)

    Instead of a list of strings, the format of .Ingresses could've been changed to a list of {name: string, servicePorts: []int32} objects. This option is the best one from the consistency and user experience point of view. Unfortunately, it is not possible to implement without introducing braking changes. Even if the controller adjusted to accept both formats (list of strings and list of objects), older clients will fail to interact with the new controller or will overwrite and corrupt .Ingresses field on attempt to change it.

  2. Use new api version, like v1alpha2 (❌)

    This is a major change, and it should be made by maintainers of the project, not a contributor like me.

  3. Add new .ServicePorts field (✅)

    Although it adds a bit more confusion to users (having to make sense of ingress/ingresses/servicePort/servicePorts fields), it is the least invasive one. This field is optional and allows specifying multiple ports for some ingresses (other ones will fall back to .ServicePort value)

Changes

The .ServicePorts fields slightly changes behavior and validation of ALBTrafficRouting:

  • .ServicePort is now optional field. The validation will check that for all referenced ingresses there is either an entry in .ServicePorts list or .ServicePort is specified as fallback.
  • Multiple ports for ingress in .Ingress can be specified using .ServicePorts.
  • Each ingress in .Ingresses list must have service ports specified either with an entry in .ServicePorts or with .ServicePort.
  • Entries in .ServicePorts must refer to ingress specified in .Ingresses list (or to .Ingress if .Ingresses is nil)

To avoid confusion, it is recommended to use .Ingress + .ServicePort only for a single ingress with a single port. In all other cases .Ingresses + .ServicePorts is preferred.

Links:

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.

@ArenSH ArenSH force-pushed the alb-multiple-service-ports-for-ingresses branch from 226d0fe to db15088 Compare May 2, 2024 09:05
@ArenSH ArenSH changed the title feat(api): support ingresses with multiple service ports in TrafficRouting.ALB spec feat(trafficrouting): support ingresses with multiple service ports in TrafficRouting.ALB spec May 2, 2024
Copy link
Contributor

github-actions bot commented May 2, 2024

Go Published Test Results

2 205 tests   2 205 ✅  2m 58s ⏱️
  119 suites      0 💤
    1 files        0 ❌

Results for commit 1554b99.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 99.40476% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.99%. Comparing base (b8a9bf5) to head (0179f8b).

Files with missing lines Patch % Lines
rollout/trafficrouting/alb/alb.go 98.82% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3551      +/-   ##
==========================================
+ Coverage   83.88%   83.99%   +0.10%     
==========================================
  Files         163      163              
  Lines       18560    18609      +49     
==========================================
+ Hits        15569    15630      +61     
+ Misses       2119     2112       -7     
+ Partials      872      867       -5     

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

Copy link
Contributor

github-actions bot commented May 2, 2024

E2E Tests Published Test Results

  4 files    4 suites   3h 24m 52s ⏱️
111 tests 102 ✅  6 💤 3 ❌
450 runs  420 ✅ 24 💤 6 ❌

For more details on these failures, see this check.

Results for commit 1554b99.

♻️ This comment has been updated with latest results.

@ArenSH ArenSH force-pushed the alb-multiple-service-ports-for-ingresses branch from 4b6e4b1 to ac50687 Compare May 2, 2024 19:00
@zachaller zachaller added this to the v1.8 milestone May 3, 2024
@ArenSH ArenSH force-pushed the alb-multiple-service-ports-for-ingresses branch from ac50687 to 7dcf17c Compare May 6, 2024 08:16
Copy link

sonarcloud bot commented May 6, 2024

Quality Gate Passed Quality Gate passed

Issues
41 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
23.4% Duplication on New Code

See analysis details on SonarCloud

@evill
Copy link

evill commented May 6, 2024

Great thanks @ArenSH . We were waiting for this feature for a long time to use it in our 30+ services. Possibility to use multi-port will unblock onboarding of ArgoRollouts for our system. Hope to see these feature in next ArgoRollout release :)

@ArenSH ArenSH force-pushed the alb-multiple-service-ports-for-ingresses branch from 7dcf17c to 8d54526 Compare June 12, 2024 12:44
Copy link

sonarcloud bot commented Jun 12, 2024

Quality Gate Passed Quality Gate passed

Issues
41 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
20.1% Duplication on New Code

See analysis details on SonarCloud

@ArenSH ArenSH force-pushed the alb-multiple-service-ports-for-ingresses branch 2 times, most recently from 94c9ac9 to 8b93239 Compare June 17, 2024 14:18
@ArenSH ArenSH force-pushed the alb-multiple-service-ports-for-ingresses branch from 8b93239 to c7a9095 Compare July 8, 2024 11:42
@ArenSH ArenSH force-pushed the alb-multiple-service-ports-for-ingresses branch from c7a9095 to 1554b99 Compare July 15, 2024 11:58
@ArenSH ArenSH force-pushed the alb-multiple-service-ports-for-ingresses branch from 1554b99 to e861faf Compare August 19, 2024 14:07
Copy link
Contributor

github-actions bot commented Aug 19, 2024

Published E2E Test Results

  4 files    4 suites   3h 17m 6s ⏱️
113 tests 101 ✅  7 💤 5 ❌
458 runs  425 ✅ 28 💤 5 ❌

For more details on these failures, see this check.

Results for commit 0179f8b.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Aug 19, 2024

Published Unit Test Results

2 309 tests   2 309 ✅  3m 2s ⏱️
  128 suites      0 💤
    1 files        0 ❌

Results for commit 0179f8b.

♻️ This comment has been updated with latest results.

@jphuynh
Copy link

jphuynh commented Sep 16, 2024

Is anyone reviewing this PR? We are interested in the functionality and would love to see it included as soon as possible 🙏

…ow specifing multiple ports per ingress

Signed-off-by: Armen Shakhbazian <[email protected]>
@ArenSH ArenSH force-pushed the alb-multiple-service-ports-for-ingresses branch from e861faf to bf024a7 Compare September 17, 2024 12:01
…fficRouting.ALB.ServicePorts and adjust existing ones

Signed-off-by: Armen Shakhbazian <[email protected]>
…afficRouting.ALB.ServicePorts

Signed-off-by: Armen Shakhbazian <[email protected]>
@ArenSH ArenSH force-pushed the alb-multiple-service-ports-for-ingresses branch from bf024a7 to 0179f8b Compare September 17, 2024 13:23
Copy link

sonarcloud bot commented Sep 17, 2024

@ArenSH
Copy link
Author

ArenSH commented Sep 17, 2024

@zachaller @alexmt I'm not very experienced with github process, so please let me know if anything left to be done from my side.

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.

4 participants