Autodetect IPv6 connectivity from minion to master #39289

bobrik · 2017-02-09T19:36:52Z

What does this PR do?

It removes the need to specify ipv6: true in minion configuration for IPv6-only salt masters.

What issues does this PR fix or reference?

This is related to #39118.

Previous Behavior

IPv6 only master require setting ipv6: true in the config, otherwise minions fail to connect, because IPv6 is explicitly disabled by default.

New Behavior

IPv6 only masters work out of the box. Yay future.

Tests written?

Nope.

bobrik · 2017-02-09T19:41:14Z

Note that currently this can cause funny outcomes when salt master is running on dual-stack machine, but only listens on legacy IPv4. I see no other way around it other than actually trying to connect to salt master in dns_check.

bobrik · 2017-02-09T19:59:52Z

Added a second commit where we actually try to connect before considering the address valid. This makes it work in all combinations: v4 only, v6 only and dual stack (even if only one IP family is actually listening for connections).

cachedout · 2017-02-09T21:24:13Z

salt/utils/__init__.py

                    break
+                except:


No bare exceptions, please. Could you change this to except Exception:?

Sure thing, done.

cachedout

It looks like this totally breaks master/minion communication in the test suite. Was this working properly in your testing?

bobrik · 2017-02-10T00:41:55Z

Yes. I checked again with ipv4 only master and no ipv6 config set, and ipv6 only master with both ipv6 config set and not set.

With added logging:

diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py
index 20c7ba88cc..8cecb7d678 100644
--- a/salt/transport/zeromq.py
+++ b/salt/transport/zeromq.py
@@ -376,6 +376,7 @@ class AsyncZeroMQPubChannel(salt.transport.mixins.auth.AESPubClientMixin, salt.t
         if not self.auth.authenticated:
             yield self.auth.authenticate()
         self.publish_port = self.auth.creds['publish_port']
+        print("connecting", self.master_pub)
         self._socket.connect(self.master_pub)

     @property

$ echo v4 && sudo cp ./minion.v4 /etc/salt/minion && sudo salt-call test.ping && echo v6 auto && sudo cp ./minion.v6-auto /etc/salt/minion && sudo salt-call test.ping && echo v6 explicit && sudo cp ./minion.v6-explicit /etc/salt/minion && sudo salt-call test.ping
v4
('connecting', 'tcp://1.2.3.4:4505')
local:
    True
v6 auto
('connecting', 'tcp://[a:b:c:d]:4505')
local:
    True
v6 explicit
('connecting', 'tcp://[a:b:c:d]:4505')
local:
    True

bobrik · 2017-02-10T01:14:02Z

Ok, I see what happened. SOCK_DGRAM only checked route availability, but did not check that connection will ever succeed. In my tests I only checked remote addresses and no actual dual-stack, but localhost is very much dual-stack: 127.0.0.1 and ::1.

I changed connection detection to actually make a connection and fixed logging as a bonus.

Let's see if this works.

bobrik · 2017-02-10T07:16:43Z

Syndic tests are failing now. This is because tests are checking configuration and do not spawn any listeners on syndic master port. Not really sure what's the best way forward.

bobrik · 2017-02-14T20:19:02Z

@cachedout it's passing tests now. Please take another look.

cachedout

Just a couple comments and questions inline. Let me know what you think. Thanks!

cachedout · 2017-02-15T17:53:39Z

salt/utils/__init__.py

                    break
-            if not addr:
+                except Exception:
+                    pass


Broad exceptions with a pass statement always scare me. What sort of exceptions are we expecting here? Can we narrow this down?

I managed to narrow it down to socket.error. Sometimes it's socket.gaierror, but socket.error covers that:

>>> import socket >>> socket.gaierror <class 'socket.gaierror'> >>> issubclass(socket.gaierror, socket.error) True

cachedout · 2017-02-15T17:55:40Z

doc/ref/configuration/minion.rst

@@ -81,9 +81,10 @@ The option can can also be set to a list of masters, enabling
 ``ipv6``
 --------

-Default: ``False``
+Default: ``None``


Could you explain the reasoning for this please? I like to ensure we have a solid explanation for changing defaults. :]

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.

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!

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.

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.

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.

cachedout · 2017-02-15T17:56:43Z

salt/minion.py

@@ -132,7 +132,7 @@
 # 6. Handle publications


-def resolve_dns(opts, fallback=True):
+def resolve_dns(opts, connect=True, fallback=True):


We prefer to add new kwargs at the end of the list just in case anybody is calling this with positional arguments.

cachedout · 2017-02-15T17:58:26Z

salt/utils/__init__.py

@@ -716,10 +716,11 @@ def ip_bracket(addr):
    return addr


-def dns_check(addr, safe=False, ipv6=False):
+def dns_check(addr, port, connect=True, safe=False, ipv6=None):


Same comment as above with new kwargs being at the end.

bobrik · 2017-02-17T22:36:21Z

Is there anything else I should address?

This fixes saltstack#39995. The previous work happened in saltstack#39289.

This was a regression in saltstack#39289 Fixes saltstack#40908

Autodetect IPv6 connectivity from minion to master

55965ce

Rewrite dns_check to try to connect to address

29f3766

cachedout suggested changes Feb 9, 2017

View reviewed changes

Avoid bare exceptions in dns_check

c494839

cachedout reviewed Feb 9, 2017

View reviewed changes

Actually connect to master instead of checking route availability

9a34fbe

Properly log address that failed to resolve or pass connection check

af95786

bobrik force-pushed the autodetect-ipv6 branch from e4bc7d4 to af95786 Compare February 10, 2017 06:42

Do no try to connect to salt master in syndic config test

e8a2cc0

bobrik force-pushed the autodetect-ipv6 branch from 7e814ba to e8a2cc0 Compare February 14, 2017 19:16

cachedout reviewed Feb 15, 2017

View reviewed changes

Narrow down connection exception to socket.error

0df6b92

bobrik force-pushed the autodetect-ipv6 branch from 385d473 to 220f6d7 Compare February 15, 2017 21:57

Move new kwargs to the end of argument list

2761a1b

bobrik force-pushed the autodetect-ipv6 branch from 220f6d7 to 2761a1b Compare February 16, 2017 06:15

cachedout merged commit c109658 into saltstack:2016.3 Feb 22, 2017

This was referenced Mar 1, 2017

Restore ipv6 connectivity and "master: <ip>:<port>" support #39766

Merged

salt-minion fails with IPv6 #39336

Closed

bobrik mentioned this pull request Mar 18, 2017

Use the first address if cannot connect to any #40141

Merged

bobrik added a commit to bobrik/salt that referenced this pull request Mar 19, 2017

Use the first address if cannot connect to any

af1545d

This fixes saltstack#39995. The previous work happened in saltstack#39289.

gtmanfred mentioned this pull request Apr 27, 2017

allow master_port to be a string #40933

Merged

gtmanfred added a commit to gtmanfred/salt that referenced this pull request Apr 27, 2017

allow master_port to be a string

194423c

This was a regression in saltstack#39289 Fixes saltstack#40908

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autodetect IPv6 connectivity from minion to master #39289

Autodetect IPv6 connectivity from minion to master #39289

bobrik commented Feb 9, 2017

bobrik commented Feb 9, 2017

bobrik commented Feb 9, 2017 •

edited

cachedout Feb 9, 2017

bobrik Feb 9, 2017

cachedout left a comment

bobrik commented Feb 10, 2017

bobrik commented Feb 10, 2017

bobrik commented Feb 10, 2017

bobrik commented Feb 14, 2017

cachedout left a comment

cachedout Feb 15, 2017

bobrik Feb 15, 2017

cachedout Feb 15, 2017

bobrik Feb 15, 2017

cachedout Feb 19, 2017

ignatk Feb 19, 2017

bobrik Feb 19, 2017

cachedout Feb 22, 2017

cachedout Feb 15, 2017

bobrik Feb 15, 2017

cachedout Feb 15, 2017

bobrik Feb 15, 2017

bobrik commented Feb 17, 2017

Autodetect IPv6 connectivity from minion to master #39289

Autodetect IPv6 connectivity from minion to master #39289

Conversation

bobrik commented Feb 9, 2017

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

New Behavior

Tests written?

bobrik commented Feb 9, 2017

bobrik commented Feb 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cachedout left a comment

Choose a reason for hiding this comment

bobrik commented Feb 10, 2017

bobrik commented Feb 10, 2017

bobrik commented Feb 10, 2017

bobrik commented Feb 14, 2017

cachedout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobrik commented Feb 17, 2017

bobrik commented Feb 9, 2017 •

edited