Add support for ClusterIP Services #43
Merged
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
Adds a command line flag to `consul-k8s` that allows the user to enable or disable the syncing of ClusterIP services. Setting the flag to `false` keeps the current behavior, while the default is `true`, allowing the services to be synced. Updates the ClusterIP functionality tests, including adding a test with the functionality disabled. Removes an added log dependency within the tests.
rboyer
reviewed
Dec 7, 2018
rboyer
reviewed
Dec 7, 2018
rboyer
reviewed
Dec 7, 2018
banks
approved these changes
Dec 7, 2018
There was a problem hiding this comment.
Super cursory look over the code and I don't see any obvious issues.
I'm not familiar enough with the code here to spot any subtle misses or reasons the tests might not work out well though so some caveats apply but as a sanity check the code looks sane to me!
There are some complications to how we determine the ports that will need to be revisited. This matches the current behavior until those oddities can be sorted out.
|
Fixes #4 . |
|
I will make sure to follow up and see that we fix that. For now, it made sense to separate these to make it a little cleaner. Thanks again for your changes-- there's been a lot of enthusiasm for this feature and it's great to be able to get it merged! |
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.
This takes the great PR from @jipperinbham that adds ClusterIP service sync support and adds a flag to make it optional. Since there are many Kubernetes environments where ClusterIPs aren't externally routeable, this allows people in those environments an easier way to only sync routeable services.