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

fix(upstreams) deduplicate like targets #5817

Merged
merged 3 commits into from
Apr 9, 2024
Merged

fix(upstreams) deduplicate like targets #5817

merged 3 commits into from
Apr 9, 2024

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Apr 4, 2024

What this PR does / why we need it:

Combine targets with the same address or hostname by summing their weights into a single target. Previously, same address targets would result in duplicates and would be rejected by Kong.

Which issue this PR fixes:

Fix #5761. Summing weights wasn't actually that hard!

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

Combine targets with the same address or hostname by summing their
weights into a single target. Previously, same address targets would
result in duplicates and would be rejected by Kong.
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.0%. Comparing base (17786d0) to head (6d65c48).
Report is 16 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5817     +/-   ##
=======================================
+ Coverage   73.9%   74.0%   +0.1%     
=======================================
  Files        176     176             
  Lines      18207   18204      -3     
=======================================
+ Hits       13455   13471     +16     
+ Misses      3749    3732     -17     
+ Partials    1003    1001      -2     

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

@rainest rainest mentioned this pull request Apr 5, 2024
1 task
internal/dataplane/translator/translator.go Outdated Show resolved Hide resolved
internal/dataplane/translator/translator.go Outdated Show resolved Hide resolved
internal/dataplane/translator/translator.go Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/S labels Apr 8, 2024
@rainest
Copy link
Contributor Author

rainest commented Apr 8, 2024

UAT confirmed this appears to resolve the issue. Added unit tests, should be good to go for review now.

@rainest rainest marked this pull request as ready for review April 8, 2024 21:27
@rainest rainest requested a review from a team as a code owner April 8, 2024 21:27
@rainest rainest requested a review from pmalek April 8, 2024 21:27
Move target deduplication into a helper and add a unit test.
@rainest rainest merged commit 3e4d430 into main Apr 9, 2024
38 checks passed
@rainest rainest deleted the fix/dedup-target branch April 9, 2024 17:23
team-k8s-bot pushed a commit that referenced this pull request Apr 9, 2024
Combine targets with the same address or hostname by summing their
weights into a single target. Previously, same address targets would
result in duplicates and would be rejected by Kong.

Move target deduplication into a helper and add a unit test.

(cherry picked from commit 3e4d430)
rainest added a commit that referenced this pull request Apr 9, 2024
rainest added a commit that referenced this pull request Apr 10, 2024
@mithunkrb
Copy link

Hey @rainest , Thanks a lot for fixing this issue. We are also encountering this issue when using KIC with argo rollouts(using canary), wondering when can we expect the next release/patch release of kic having this fix?

@rainest
Copy link
Contributor Author

rainest commented Apr 27, 2024

@mithunkrb probably in 3.2, with a rough timeline of ~1.5months out.

https://hub.docker.com/r/kong/nightly-ingress-controller/tags?page=1&ordering=last_updated is available if you want it earlier.

Note that we did find a bug in the fix, so you may want to wait til the build after that merges. Not sure what all it affects, but it definitely affects any Services that use the ingress.kubernetes.io/service-upstream annotation.

@mithunkrb
Copy link

Thank you @rainest for the update, it helps! For the time being would go with the nightly build(once its available) having the fix for the mentioned bug, and would upgrade to 3.2 once we have that available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle same-selector Service targets
4 participants