-
Notifications
You must be signed in to change notification settings - Fork 591
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(status) add UDP publish service #3325
Conversation
23c2fd1
to
698ee54
Compare
Codecov ReportBase: 73.8% // Head: 73.6% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #3325 +/- ##
=======================================
- Coverage 73.8% 73.6% -0.2%
=======================================
Files 114 115 +1
Lines 13930 14011 +81
=======================================
+ Hits 10288 10321 +33
- Misses 2985 3020 +35
- Partials 657 670 +13
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
698ee54
to
ce05c83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new methods specific to UDP in AddressFinder
are not referenced. Have there been any changes on implementation from using one AddressFinder
with different methods to using different AddressFinder
s? If another address finder is used to find UDP loadbalancer addresses, the UDP specific methods should be deleted in AddressFinder
.
ce05c83
to
8f95583
Compare
Too much hubris about copy/pasta and unit testing only being viable 😄 Should now properly exclude garbage extra code, confirms that (at least the service IP lookup version, units should cover override address version) status updates match the args in integration now. |
Add CLI arguments for setting a UDP publish service and address overrides. If set, these will be used to populate status information for UDPIngresses. If not set, UDPIngress status will fall back to the default publish service/override, with the expectation that it is a dual-transport LoadBalancer.
312d9a2
to
4a3130f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally good. some minor comments.
Also, I think CHANGELOG updates should be included in this PR.
@rainest I think that this could be simplified by dropping the UDP specifics from the |
I like the idea of deduplicating code 👍 |
Unfortunately wasn't able to review this alongside the context plumbing and merge conflict. Those changes probably broke this, but the idea seems reasonable. Maybe merge this and then consolidate them in a separate PR? |
The main intention should be visible from this diff: https:/Kong/kubernetes-ingress-controller/compare/c335c292efa05666deb88cfb401f850de3b93cd3..feat/udp-publish-review. I think that would be worth to simplify it straight away in this PR, WDYT? |
8ab82bd
to
ea0b959
Compare
This PR unfortunately repeatedly rammed into confluence of other changes that required refactoring that keeps dragging it out. I think the change makes sense to do, but ultimately unless the code is readily available I'd rather get this merged and get #2544 and #3237 at least satisfied. If you have time to update the changes, feel free to push them to this branch, but if not, I'd prefer to merge this and handle further refactoring after. Perfect being the enemy of good and all, unless it'd mean a user-facing functionality change, time value of getting this wrapped up outweighs the benefit of the back-and-forth costs to get it perfectly clean under the hood. |
What this PR does / why we need it:
Add CLI arguments for setting a UDP publish service and address overrides. If set, these will be used to populate status information for UDPIngresses. If not set, UDPIngress status will fall back to the default publish service/override, with the expectation that it is a dual-transport LoadBalancer.
Planned for future use with Gateway UDP listener matching in #3237
Which issue this PR fixes:
Fix #2544
Special notes for your reviewer:
This falls back to the default Service. We may actually want to not do this and instead explicitly require both, but make UDP optional (in which case we simply don't set any status info for UDP stuff). Depends on whether we prefer to match current behavior AFAIK.
Ready for preliminary review. Missing changelog pending some other PR adding a 2.9.0, but code is done. I figure the behavior here is more controversial than #3336 so trying and adding the new changelog section there first, planning to rebase after that and add this change's log after.
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR