-
Notifications
You must be signed in to change notification settings - Fork 592
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(gateway_discovery) enable gateway discovery in db mode #4828
Conversation
f142554
to
cea7042
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4828 +/- ##
=======================================
- Coverage 75.5% 75.5% -0.1%
=======================================
Files 167 167
Lines 18881 18903 +22
=======================================
+ Hits 14273 14278 +5
- Misses 3783 3795 +12
- Partials 825 830 +5
☔ View full report in Codecov by Sentry. |
cea7042
to
25559a6
Compare
E2E (targeted) tests with KIND-based clusters were started at https://github.com/Kong/kubernetes-ingress-controller/actions/runs/6503776252 |
06417b3
to
7d93499
Compare
E2E (targeted) tests with KIND-based clusters were started at https://github.com/Kong/kubernetes-ingress-controller/actions/runs/6505244182 |
E2E (targeted) tests with KIND-based clusters were started at https://github.com/Kong/kubernetes-ingress-controller/actions/runs/6505946279 |
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.
I think we need to clarify what exactly of discovery we wanted to port. Some aspects of it don't really make sense for DB mode.
I think what we actually want to target is that both DB and DB-less mode only require indicating the admin Service in configuration, but not change how DB mode sends config.
We have the configuring the target admin service as a prerequisite for discovery already, so we should simply send a single batch of update requests to the Service, and let kube-proxy deal with distributing that to a ready instance per its normal rules. We don't need to send updates to every replica, so we shouldn't need to bypass kube-proxy and write our own instance logic.
Use of ClusterIP=None
a in DB-less discovery though, and I don't know if there's a good way to reconcile that without requiring different Service config for DB mode. We could send a single request to the headless Service hostname and let DNS select an instance at random.
Using our own logic will ignore broader system networking policy. It wouldn't honor something like https://kuma.io/docs/2.4.x/policies/locality-aware/, for example. I don't know how well headless plays with that in general, so that's maybe wrapped up in #4698.
We presumably do want to handle mTLS, but it looks this is handled somewhat outside discovery itself--the cert configuration takes effect regardless of whether the request comes through kube-proxy or not AFAICT.
11e674c
to
a8b9162
Compare
E2E (targeted) tests with KIND-based clusters were started at https://github.com/Kong/kubernetes-ingress-controller/actions/runs/6624257331 |
E2E (targeted) tests with KIND-based clusters were started at https://github.com/Kong/kubernetes-ingress-controller/actions/runs/6731367753 |
f92f126
to
eca994d
Compare
E2E (targeted) tests with KIND-based clusters were started at https://github.com/Kong/kubernetes-ingress-controller/actions/runs/6734361214 |
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.
🚢
@rainest Do you have any comments still that you'd like to raise here? |
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.
No longer seeing extra updates after the SHA change, so I no longer have anything:
2023-11-02T16:41:32Z debug Sending configuration to gateway clients {"v": 1, "urls": ["https://10.244.0.59:8444"]}
2023-11-02T16:41:32Z info Successfully synced configuration to Kong {"url": "https://10.244.0.59:8444", "update_strategy": "DBMode", "v": 0}
2023-11-02T16:41:35Z debug Sending configuration to gateway clients {"v": 1, "urls": ["https://10.244.0.59:8444"]}
2023-11-02T16:41:38Z debug Sending configuration to gateway clients {"v": 1, "urls": ["https://10.244.0.59:8444"]}
2023-11-02T16:41:41Z debug Sending configuration to gateway clients {"v": 1, "urls": ["https://10.244.0.59:8444"]}
2023-11-02T16:41:44Z debug Sending configuration to gateway clients {"v": 1, "urls": ["https://10.244.0.59:8444"]}
2023-11-02T16:41:47Z debug Sending configuration to gateway clients {"v": 1, "urls": ["https://10.244.0.59:8444"]}
2023-11-02T16:41:50Z debug Sending configuration to gateway clients {"v": 1, "urls": ["https://10.244.0.60:8444"]}
2023-11-02T16:41:53Z debug Sending configuration to gateway clients {"v": 1, "urls": ["https://10.244.0.59:8444"]}
What this PR does / why we need it:
enables gateway discovery in DB mode. In DB mode, if multiple gateways are behind the Kong admin API service, KIC will discover all ready clients and send config to one of them in sending configurations.
Which issue this PR fixes:
fixes #4751
Special notes for your reviewer:
This PR could not cover the case where multiple gateways are using different DBs. This case is tracked in #4845.
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