-
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: add support for single controller, multi gateway deployments #3268
Conversation
9625713
to
ec47f2c
Compare
Codecov ReportBase: 73.9% // Head: 74.0% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #3268 +/- ##
=======================================
+ Coverage 73.9% 74.0% +0.1%
=======================================
Files 111 113 +2
Lines 13688 13896 +208
=======================================
+ Hits 10116 10287 +171
- Misses 2933 2963 +30
- Partials 639 646 +7
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. |
ec47f2c
to
c0d5766
Compare
d745fe5
to
c5ffc77
Compare
1d05fb8
to
ccbfbc2
Compare
ccbfbc2
to
7e7e120
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.
Good stuff here! :) Left some comments, mostly in regards to readability. From the functional perspective - I think we're fine.
A general comment regarding testing: I'd love to have the code running the config updates unit tested as it's something that we're lacking, even before the changes this PR introduces. Now extending that, we stack even more complexity (concurrency 😬) in there which still remains untested. If possible, I'd try to make that unit-testable by extracting some interfaces. Doesn't have to be done in this PR though. WDYT?
I think we could add some integration test cases for configuring more than 1 kong instances and KongAdminURLs. |
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.
Thanks for this effort, @pmalek. It looks like a big bunch of complex code 🚀
ac8f3d7
to
7896368
Compare
7896368
to
21119c8
Compare
ae82347
to
5317416
Compare
10e431d
to
1159f64
Compare
1159f64
to
447ecc4
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.
✅
Two notes from my side:
- Let's not forget about backfilling our test suite with tests covering this functionality before closing the issue.
- In cases in which the PR evolves and the comments are being addressed (like this one), I would prefer to have granular commits history to be able to see changes that happened since my last review - it would save some time looking for the places that were changed. Was there any specific reason to always force push the changes as a single commit?
IMHO it's messy and I always feel that I need to come up with a contrived commit message that won't matter in the end, like : "wip", "review comments" and so on. Gerrit workflow for the win! If only git allowed something like this :( Feedback noted! One more point on this is that I prefer to have clean history and with these types of commits it becomes hard to work on multiple branches that depend on each other (or work on the same piece of code). When you don't rebase and create more and more commits then you end up in merged spaghetti history (arguable) that's nearly impossible to use with rebasing workflow. With 1 (or handful but manageable amount) it's easier to reason about the history and easier to rebase. |
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.
🚢
Let's not forget about backfilling our test suite with tests covering this functionality before closing the issue.
+1 on this
What this PR does / why we need it:
This PR implements multi Gateways support via single KIC instance.
The configuration is being done the same way as it has been before i.e. through the
--kong-admin-url
flag and corresponding environment variableCONTROLLER_KONG_ADMIN_URL
but when more than 1 address is provided then,
(comma) is to be used as a separator.Which issue this PR fixes:
Related #702
Special notes for your reviewer:
There are a handful of simplifications like e.g. not sending telemetry reports for each kong instance since the telemetry report schema only allows for one
db
setting and one kong version to be set.There are corresponding notes and linked issues in the source comments.
No test for this yet. I might create it as part of this one (will make it even bigger :/) or as a separate PR. Up to reviewers.
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