-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Autodetect IPv6 connectivity from minion to master #39289
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
55965ce
Autodetect IPv6 connectivity from minion to master
bobrik 29f3766
Rewrite dns_check to try to connect to address
bobrik c494839
Avoid bare exceptions in dns_check
bobrik 9a34fbe
Actually connect to master instead of checking route availability
bobrik af95786
Properly log address that failed to resolve or pass connection check
bobrik e8a2cc0
Do no try to connect to salt master in syndic config test
bobrik 0df6b92
Narrow down connection exception to socket.error
bobrik 2761a1b
Move new kwargs to the end of argument list
bobrik File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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.
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.
Could you explain the reasoning for this please? I like to ensure we have a solid explanation for changing defaults. :]
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 reasoning is to enable IPv6 connectivity automatically if user doesn't override the setting. If master address resolves to IPv6 and responds on that address, we take advantage of that immediately.
Just like browsers don't force users to specify that they want google.com over IPv6, we don't force salt users to do the similar thing with salt masters. I think it's much smoother user experience.
It also enables mixed IPv4/IPv6 deployments with multiple masters and that is very useful during migration to IPv6 only stacks.
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.
Gotcha. I agree entirely that this is a better user experience but I am not very excited about making that change in a minor, bugfix only release. I would accept this PR if we didn't change the default behavior or as it is into the develop branch. Between those two choices, I think that putting this into the develop branch is the better course of action but I'd like to hear your thoughts. Thanks!
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.
It feels here like changing a default value does not change the behavior, but just adds additional behavior: previously by default it tried IPv4 only, but now it tries IPv4 and IPv6.
Also, I bet for most Salt users (at least for me) who did not try IPv6 specifically fact that IPv6 is disabled by default is more a surprise than an expectation.
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 default behavior is all-IPv4 for salt master and salt minion, we don't change that. This patch doesn't change behavior for this case. For people who run IPv6-only deployments already, this patch doesn't change behavior either, because those people already have to set
ipv6: true
on both minion and master.The point here is to enable new behavior out of the box, not change anything existing and working.
This patch only changes behavior for the case that was not possible before, so it's hard to break anything existing in the wild. The only thing I can think of is a deployment where salt master works on IPv4, listens and accepts connections on IPv6, but otherwise breaks on IPv6. You have to try very hard to achieve that and I double anyone has would have this issue.
In fact,
ipv6
option wasn't even documented, I changed that recently in #39131.If you still feel very strongly about development branch, I can do that.
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.
You both make very strong points. I appreciate the explanation and you taking the time to walk me through your reasoning. We'll get this in.