[DPE-5089] support external connections with node port #21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MiaAltieri
commented
Aug 15, 2024
MiaAltieri
changed the title
marcoppenheimer
approved these changes
Aug 15, 2024
Gu1nness
approved these changes
Aug 16, 2024
Co-authored-by: Mehdi Bendriss <[email protected]>
Gu1nness
approved these changes
Aug 19, 2024
delgod
requested changes
Aug 20, 2024
Co-authored-by: Mehdi Bendriss <[email protected]>
MiaAltieri
force-pushed
the
DPE-5089-use-node-port
branch
from
fe4e630 to
30becd8
Compare
MiaAltieri
force-pushed
the
DPE-5089-use-node-port
branch
from
1948e35 to
7c3bebb
Compare
MiaAltieri
force-pushed
the
DPE-5089-use-node-port
branch
from
46d7cd7 to
e1c68d1
Compare
delgod
requested changes
Aug 27, 2024
| if expose_external not in Config.ExternalConnections.VALID_EXTERNAL_CONFIG: | ||
| return | ||
|
|
||
| self.app_peer_data["expose-external"] = expose_external |
There was a problem hiding this comment.
I don't think we need to store value to peer data
There was a problem hiding this comment.
I think we should, since the user could incorrectly set the config, I would like to store the most recent correct config
delgod
approved these changes
Aug 28, 2024
Co-authored-by: Mehdi Bendriss <[email protected]>
Co-authored-by: Mehdi Bendriss <[email protected]>
Co-authored-by: Mehdi Bendriss <[email protected]>
Mehdi-Bendriss
approved these changes
Aug 28, 2024
MiaAltieri
force-pushed
the
DPE-5089-use-node-port
branch
2 times, most recently
from
c5400cf to
8d357a7
Compare
MiaAltieri
force-pushed
the
DPE-5089-use-node-port
branch
from
8d357a7 to
428a40d
Compare
MiaAltieri
force-pushed
the
DPE-5089-use-node-port
branch
from
bc82624 to
cd1fa10
Compare
MiaAltieri
force-pushed
the
DPE-5089-use-node-port
branch
from
b14b35d to
dce5a1c
Compare
MiaAltieri
force-pushed
the
DPE-5089-use-node-port
branch
from
dce5a1c to
79e0707
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
External connections are not supported on K8s router
Solution
Support them using nodeport in the same way other charms do
Future Work
What we have here is a good basis to start. When we support: TLS, Upgrade, clients, we will need to make modifications to continue the support of NodePort
Kudos
@phvalguima + @marcoppenheimer knowledge and wisdom for pairing and explaining a lot of this to me. Thank you 🙏