Fix network binding for ipv4/ipv6 #12942

rmuir · 2015-08-17T16:20:39Z

Elasticsearch doesn't work well today with modern machines that might support both ipv4 and ipv6. The worst problem here that curl http://localhost... does not work e.g. on mac (#12906), because we aren't bound to any v6 address.

Equally bad, we only bind to loopback by default for 2.0, so if you want to connect to the network "for real" you might provide an interface name, but that always tends to do the wrong thing too (#12915), e.g. pick link local or some other useless address.

This is a compromise fix that tries to keep things simple:

we bind to multiple addresses when a specified host/interface name has multiple addresses. If you dont like this, then specify a single address.
we still only publish by default to one (this would require more work).
no changes for ipv6 multicast or anything like that yet.

The default for which address to publish when bound to multiple addresses is pretty simple, we prefer ipv4 by default (java.net.preferIPv4Stack) and I think we should until things like multicast are fixed to work correct over ipv6. Otherwise we prefer "real addresses" > site local > link local and so on.

Some things are still a bit messy, because real cleanups need to not be right before a beta release.

Closes #12906
Closes #12915

Next up: multicast and then http

dakrone · 2015-08-17T17:42:41Z

So, an interesting byproduct of this is that if I have an ES node bind to the public interface with bin/elasticsearch -Des.network.host=_en0_ and then start up another with no options (so it binds to loopback interfaces only), the loopback one detects and joins a cluster with the public facing one.

I'm not sure if this is desired or not, I guess I would expect the loopback one only to be able to form clusters with other nodes bound to the loopback interface. What do you think?

dakrone · 2015-08-17T17:46:14Z

Also, this is an OSX-only issue, Linux doesn't have this loopback <-> non-loopback communication weirdness.

rmuir · 2015-08-17T17:47:38Z

That is not related to this change! That is #12914 and I deferred it because its Mac OS X specific, and because its not related to these changes.

dakrone · 2015-08-17T17:50:20Z

That is not related to this change!

Okay, I totally agree about the deferring it. Just wanted to make sure it was something we are aware of (which of course there is the other issue open for it).

dakrone · 2015-08-17T17:52:36Z

core/src/main/java/org/elasticsearch/common/network/NetworkService.java

+            bindHost = settings.get(GLOBAL_NETWORK_BINDHOST_SETTING, settings.get(GLOBAL_NETWORK_HOST_SETTING));
+        }
+        if (bindHost == null) {
+            bindHost = defaultValue2;


Do you prefer the double-if? If not I guess you could do:

if (bindHost == null) { bindHost = settings.get(GLOBAL_NETWORK_BINDHOST_SETTING, settings.get(GLOBAL_NETWORK_HOST_SETTING, defaultValue2)); } return resolveInetAddress(bindHost);

Instead

I have changes coming in a commit. Basically it was too hard for me to see the various fallbacks for the default case (null).

dakrone · 2015-08-17T18:00:18Z

Left a few comments but generally LGTM. I tested this locally on OSX and Linux. Would be great to have someone manually test on Windows as well just for a sanity check.

s1monw · 2015-08-17T18:14:46Z

README.textile

@@ -50,16 +50,16 @@ h3. Indexing
 Let's try and index some twitter like information. First, let's create a twitter user, and add some tweets (the @twitter@ index will be created automatically):

 <pre>
-curl -XPUT 'http://127.0.0.1:9200/twitter/user/kimchy' -d '{ "name" : "Shay Banon" }'
+curl -XPUT 'http://localhost:9200/twitter/user/kimchy' -d '{ "name" : "Shay Banon" }'


who is this shay banon guy after all?

s1monw · 2015-08-17T18:28:42Z

core/src/main/java/org/elasticsearch/http/netty/NettyHttpServerTransport.java


+        InetSocketAddress boundAddress = (InetSocketAddress) serverChannels.get(0).getLocalAddress();


When I first looked at this I wondered if this is prone to concurrent modification exception? I wondered if we should make the serverChannels final and use a CopyOnWriteArrayList instead. Reading without sync would be safe. But then looking at the entire file I think we should just make the list final and clear it before we close each channel? the volatile confuses me and I think it's not needd

All the concurrency in these methods is super confusing. Its doing really slow stuff like binding to ports, which should not happen all the time. I am happy to fix the concurrency, it will be to make everything synchronized: this is all nuts.

I agree - we can do this in a follow up!

s1monw · 2015-08-17T18:31:25Z

Left mostly minor comments! This looks great to me! I will run tests with node.mode=network quickly as well on that.

s1monw · 2015-08-17T18:42:37Z

core/src/main/java/org/elasticsearch/transport/netty/NettyTransport.java

@@ -146,7 +146,7 @@
    // node id to actual channel
    protected final ConcurrentMap<DiscoveryNode, NodeChannels> connectedNodes = newConcurrentMap();
    protected final Map<String, ServerBootstrap> serverBootstraps = newConcurrentMap();
-    protected final Map<String, Channel> serverChannels = newConcurrentMap();
+    protected final Map<String, List<Channel>> serverChannels = newConcurrentMap();
    protected final Map<String, BoundTransportAddress> profileBoundAddresses = newConcurrentMap();


this must be a ConcurrentMap you use putIfAbsent

s1monw · 2015-08-17T19:11:17Z

I ran tests with -Des.node.mode=network and they pass +1 to push here is my LGTM

thanks rob!

clintongormley · 2015-08-18T11:16:24Z

Merged in b3a9abb and 68307aa

In elastic#12942, the NettyTransport and NettyHttpServerTransport were updated to allow for binding to multiple addresses. However, the BoundTransportAddress holder only exposed the first address that the transport was bound to and this object is used to populate the values returned to the user via our APIs. This change exposes all of the bound addresses in the BoundTransportAddress holder, which allows for an accurate representation of all interfaces that elasticsearch is bound to and listening on.

In #12942, the NettyTransport and NettyHttpServerTransport were updated to allow for binding to multiple addresses. However, the BoundTransportAddress holder only exposed the first address that the transport was bound to and this object is used to populate the values returned to the user via our APIs. This change exposes all of the bound addresses in the BoundTransportAddress holder, which allows for an accurate representation of all interfaces that elasticsearch is bound to and listening on.

rmuir added 6 commits August 17, 2015 11:06

Fix transport / interface code

9b9413a

Next up: multicast and then http

fix http too

89547d7

remove nocommit

88c462e

fix docs, fix another bug in multicast (publish host = bad here!)

b55d092

Merge branch 'master' into network_cleanup

e198bb2

localhost all the way down

0faf71c

rmuir added the v2.0.0-beta1 label Aug 17, 2015

clintongormley added >bug :Distributed/Network Http and internode communication implementations labels Aug 17, 2015

Add some unit tests for utility methods

bd7065f

dakrone reviewed Aug 17, 2015
View reviewed changes

s1monw reviewed Aug 17, 2015
View reviewed changes

Cleanup/fix logic around custom resolvers

c7b9f3a

s1monw reviewed Aug 17, 2015
View reviewed changes

fix java 7 compilation oops

7e60833

s1monw added the blocker label Aug 17, 2015

rmuir mentioned this pull request Aug 17, 2015

multicast ping should work over ipv6 #12945

Closed

clintongormley closed this Aug 18, 2015

clintongormley added the v2.1.0 label Aug 18, 2015

dakrone mentioned this pull request Aug 18, 2015

Multicast discovery is broken when network.host is set an IPv6 address #4989

Closed

rmuir mentioned this pull request Aug 21, 2015

Fully support IPv6 #10535

Closed

jpountz removed the v2.1.0 label Aug 21, 2015

jaymode mentioned this pull request Sep 15, 2015

Expose all addresses that the transports are bound to #13586

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix network binding for ipv4/ipv6 #12942

Fix network binding for ipv4/ipv6 #12942

rmuir commented Aug 17, 2015

dakrone commented Aug 17, 2015

dakrone commented Aug 17, 2015

rmuir commented Aug 17, 2015

dakrone commented Aug 17, 2015

dakrone Aug 17, 2015

rmuir Aug 17, 2015

dakrone commented Aug 17, 2015

s1monw Aug 17, 2015

s1monw Aug 17, 2015

rmuir Aug 17, 2015

s1monw Aug 17, 2015

s1monw commented Aug 17, 2015

s1monw Aug 17, 2015

s1monw commented Aug 17, 2015

clintongormley commented Aug 18, 2015


		InetSocketAddress boundAddress = (InetSocketAddress) serverChannels.get(0).getLocalAddress();

Fix network binding for ipv4/ipv6 #12942

Fix network binding for ipv4/ipv6 #12942

Conversation

rmuir commented Aug 17, 2015

dakrone commented Aug 17, 2015

dakrone commented Aug 17, 2015

rmuir commented Aug 17, 2015

dakrone commented Aug 17, 2015

dakrone Aug 17, 2015

Choose a reason for hiding this comment

rmuir Aug 17, 2015

Choose a reason for hiding this comment

dakrone commented Aug 17, 2015

s1monw Aug 17, 2015

Choose a reason for hiding this comment

s1monw Aug 17, 2015

Choose a reason for hiding this comment

rmuir Aug 17, 2015

Choose a reason for hiding this comment

s1monw Aug 17, 2015

Choose a reason for hiding this comment

s1monw commented Aug 17, 2015

s1monw Aug 17, 2015

Choose a reason for hiding this comment

s1monw commented Aug 17, 2015

clintongormley commented Aug 18, 2015