Allow to listen on virtual interfaces. #19537

daschl · 2016-07-21T14:11:25Z

Previously when trying to listen on virtual interfaces during
bootstrap the application would stop working - the interface
couldn't be found by the NetworkUtils class.

The NetworkUtils utilize the underlying JDK NetworkInterface
class which, when asked to lookup by name only takes physical
interfaces into account, failing at virtual (or subinterfaces)
ones (returning null).

Note that when interating over all interfaces, both physical and
virtual ones are taken into account.

This changeset asks for all known interfaces, iterates over them
and matches on the given name as part of the loop, allowing it
to catch both physical and virtual interfaces.

As a result, elasticsearch can now also serve on virtual
interfaces.

A test case has been added which at least makes sure that all
iterable interfaces can be found by their respective name. (It's
not easily possible in a unit test to "fake" virtual interfaces).

Closes #17473

Previously when trying to listen on virtual interfaces during bootstrap the application would stop working - the interface couldn't be found by the NetworkUtils class. The NetworkUtils utilize the underlying JDK NetworkInterface class which, when asked to lookup by name only takes physical interfaces into account, failing at virtual (or subinterfaces) ones (returning null). Note that when interating over all interfaces, both physical and virtual ones are taken into account. This changeset asks for all known interfaces, iterates over them and matches on the given name as part of the loop, allowing it to catch both physical and virtual interfaces. As a result, elasticsearch can now also serve on virtual interfaces. A test case has been added which at least makes sure that all iterable interfaces can be found by their respective name. (It's not easily possible in a unit test to "fake" virtual interfaces). Fixes elastic#17473

elasticmachine · 2016-07-21T14:11:26Z

Can one of the admins verify this patch?

daschl · 2016-07-21T14:12:02Z

I signed the CLA before sending this PR, maybe it didn't go "through" yet.

daschl · 2016-07-21T14:18:31Z

In case someone wants to verify, you can easily create a virtual interface on linux like:

sudo ifconfig eth0:1 10.2.16

Now in the config set the network.host to _eth0:1_.

Starting 2.3.4 prints

Exception in thread "main" java.lang.IllegalArgumentException: No interface named 'eth0:1' found, got [name:lo (lo), name:eth0 (eth0), name:eth0:1 (eth0:1), name:eth1 (eth1)]
    at org.elasticsearch.common.network.NetworkUtils.getAddressesForInterface(NetworkUtils.java:232)
    at org.elasticsearch.common.network.NetworkService.resolveInternal(NetworkService.java:262)
    at org.elasticsearch.common.network.NetworkService.resolveInetAddresses(NetworkService.java:209)
    at org.elasticsearch.common.network.NetworkService.resolveBindHostAddresses(NetworkService.java:122)
    at org.elasticsearch.transport.netty.NettyTransport.bindServerBootstrap(NettyTransport.java:424)
    at org.elasticsearch.transport.netty.NettyTransport.doStart(NettyTransport.java:321)
    at org.elasticsearch.common.component.AbstractLifecycleComponent.start(AbstractLifecycleComponent.java:68)
    at org.elasticsearch.transport.TransportService.doStart(TransportService.java:182)
    at org.elasticsearch.common.component.AbstractLifecycleComponent.start(AbstractLifecycleComponent.java:68)
    at org.elasticsearch.node.Node.start(Node.java:278)
    at org.elasticsearch.bootstrap.Bootstrap.start(Bootstrap.java:206)
    at org.elasticsearch.bootstrap.Bootstrap.init(Bootstrap.java:272)
    at org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:35)

while with the change applied:

[2016-07-21 14:08:00,095][INFO ][transport                ] [Murmur II] publish_address {10.2.0.16:9300}, bound_addresses {10.2.0.16:9300}
[2016-07-21 14:08:00,100][INFO ][bootstrap                ] [Murmur II] bound or publishing to a non-loopback or non-link-local address, enforcing bootstrap checks

jasontedor · 2016-07-21T14:27:24Z

It's not easily possible in a unit test to "fake" virtual interfaces

This is not right, testing is always possible. For example, this simple change adds a package-private method that can easily be tested with "fake" virtual interfaces:

diff --git a/core/src/main/java/org/elasticsearch/common/network/NetworkUtils.java b/core/src/main/java/org/elasticsearch/common/network/NetworkUtils.java
index e15073e..6f098ef 100644
--- a/core/src/main/java/org/elasticsearch/common/network/NetworkUtils.java
+++ b/core/src/main/java/org/elasticsearch/common/network/NetworkUtils.java
@@ -227,8 +227,12 @@ public abstract class NetworkUtils {

     /** Returns addresses for the given interface (it must be marked up) */
     static InetAddress[] getAddressesForInterface(String name) throws SocketException {
+        return getAddressesForInterface(name, getInterfaces());
+    }
+
+    static InetAddress[] getAddressesForInterface(String name, List<NetworkInterface> interfaces) throws SocketException {
         NetworkInterface intf = null;
-        for (NetworkInterface networkInterface : getInterfaces()) {
+        for (NetworkInterface networkInterface : interfaces) {
             if (name.equals(networkInterface.getName())) {
                 intf = networkInterface;
                 break;
@@ -236,7 +240,7 @@ public abstract class NetworkUtils {
         }

         if (intf == null) {
-            throw new IllegalArgumentException("No interface named '" + name + "' found, got " + getInterfaces());
+            throw new IllegalArgumentException("No interface named '" + name + "' found, got " + interfaces);
         }
         if (!intf.isUp()) {
             throw new IllegalArgumentException("Interface '" + name + "' is not up and running");

daschl · 2016-07-21T14:31:34Z

@jasontedor gotcha, I was actually referring to faking an actual virtual interface that would reproduce the scenario when interacting with the original JVM methods. Any idea how that could be done?

Do you think I should make the change you suggested? Not sure it would provide more coverage for the specific case in question.

rmuir · 2016-07-21T14:53:25Z

The change is good. It should have been calling this method all along: it even calls it for error messages (making the bug more confusing, because it shows you the exact one you tried to set in the list).

jasontedor · 2016-07-21T14:57:34Z

I was actually referring to faking an actual virtual interface that would reproduce the scenario when interacting with the original JVM methods. Any idea how that could be done?

It's fine to just mock the behavior of the JVM method as I suggested.

Do you think I should make the change you suggested?

Yes, it should be possible to write a test case that would fail without the change and will pass with your suggested change (which is good).

daschl · 2016-07-21T15:16:06Z

@jasontedor sounds good I'll make the changes :)

daschl · 2016-07-22T05:27:59Z

@jasontedor a quick follow up with some question/observations from my digging:

Looks like it's not as easy as I thought because the NetworkInterface has no way of setting it to virtual - it only has those static initializers which don't provide the semantics we need here I think. Mocking it is also not an option since its a final class, and from the dependencies I can see that powermock is not used in ES right now (apologies if I missed a different way how ES allows to mock final classes, let me know if there is one).

So I propose doing one of the four things (please share your thoughts if there is something I have missed):

Stick with the one test I added previously, which makes sure that all the listed interfaces (which includes virtual and physical) can also be looked up by name, this is not the case before the patch and the test case would previously fail on a system where there is a virtual interface defined.
If there is a way to mock the final class in ES right now let me know, that is definitely an option.
The most complex one I assume, but if there is a way we could setup a virtual interface if the box is a linux machine and then run the test.. but its probably a little out of scope for this level of testing. Is there a higher level test harness that could cover this case?
Oh, and we could introduce another layer of indirection over the NetworkInterface so it is mockable.

rmuir · 2016-07-22T11:12:18Z

I agree, I think NetworkInterface is mostly initialized via native code, we have no chance.

To be fair to @jasontedor's point, the bug probably happens because we don't test virtual interfaces :)

But at the same time, we should not try to do something super-elaborate, or compromise the code itself with useless abstractions to do it. We should just test what is reasonable.

jasontedor · 2016-07-22T11:49:02Z

I agree, I think NetworkInterface is mostly initialized via native code, we have no chance.

Sadly, this is right. It is possible to get this tested, but I agree with Robert's point that it's probably not worth it for just this one test. If we were to go over this code and get it all tested, it might be worth doing what I suggest in this gist, but it's too far here.

jasontedor · 2016-07-22T11:50:04Z

ok to test

daschl · 2016-07-22T12:21:03Z

Looks like the git checkout failed https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request+multijob-intake/92/console

jasontedor · 2016-07-22T13:36:24Z

test this please

jasontedor · 2016-07-22T16:33:29Z

Thanks @daschl.

karlra · 2016-07-22T16:47:17Z

Thank you for fixing this, @daschl :)

dakrone · 2016-07-22T17:28:08Z

@jasontedor and @daschl - this actually causes test failures if a virtual interface is not up:

Suite: org.elasticsearch.common.network.NetworkUtilsTests
  2> REPRODUCE WITH: gradle :core:test -Dtests.seed=43BC27A5D082E629 -Dtests.class=org.elasticsearch.common.network.NetworkUtilsTests -Dtests.method="testAddressInterfaceLookup" -Dtests.security.manager=true -Dtests.locale=es-PA -Dtests.timezone=America/Indiana/Indianapolis
ERROR   0.18s | NetworkUtilsTests.testAddressInterfaceLookup <<< FAILURES!
   > Throwable #1: java.lang.IllegalArgumentException: Interface 'virbr0' is not up and running
   >    at __randomizedtesting.SeedInfo.seed([43BC27A5D082E629:C4C740E215C89F5F]:0)
   >    at org.elasticsearch.common.network.NetworkUtils.getAddressesForInterface(NetworkUtils.java:242)
   >    at org.elasticsearch.common.network.NetworkUtilsTests.testAddressInterfaceLookup(NetworkUtilsTests.java:90)
   >    at java.lang.Thread.run(Thread.java:745)
  2> NOTE: leaving temporary files on disk at: /home/hinmanm/es/elasticsearch/core/build/testrun/test/J0/temp/org.elasticsearch.common.network.NetworkUtilsTests_43BC27A5D082E629-001
  2> NOTE: test params are: codec=Asserting(Lucene60): {}, docValues:{}, maxPointsInLeafNode=619, maxMBSortInHeap=7.4794493142308305, sim=RandomSimilarity(queryNorm=false,coord=no): {}, locale=es-PA, timezone=America/Indiana/Indianapolis
  2> NOTE: Linux 4.6.3-300.fc24.x86_64 amd64/Oracle Corporation 1.8.0_77 (64-bit)/cpus=4,threads=1,free=481288632,total=514850816
  2> NOTE: All tests run in this JVM: [NetworkUtilsTests]
Completed [1/1] in 1.20s, 1 test, 1 error <<< FAILURES!

(Reproduces every time for me), virb0 is a virtual interface used for bridging VirtualBox VMs, but stays down while nothing is running.

jasontedor · 2016-07-22T17:30:26Z

Reverted in c27237b.

jasontedor · 2016-07-22T17:33:09Z

The test included in the fix here will blow up on any machine that has an interface that is down.

daschl · 2016-07-22T18:05:36Z

Okay good to know, I'll rework the test case on monday and resubmit it.

Previously when trying to listen on virtual interfaces during bootstrap the application would stop working - the interface couldn't be found by the NetworkUtils class. The NetworkUtils utilize the underlying JDK NetworkInterface class which, when asked to lookup by name only takes physical interfaces into account, failing at virtual (or subinterfaces) ones (returning null). Note that when interating over all interfaces, both physical and virtual ones are taken into account. This changeset asks for all known interfaces, iterates over them and matches on the given name as part of the loop, allowing it to catch both physical and virtual interfaces. As a result, elasticsearch can now also serve on virtual interfaces. A test case has been added which makes sure that all iterable interfaces can be found by their respective name. Note that this PR is a second iteration over the previously merged but later reverted elastic#19537 because it causes tests to fail when interfaces are down. The test has been modified to take this into account now. Closes elastic#17473 Relates elastic#19537

Previously when trying to listen on virtual interfaces during bootstrap the application would stop working - the interface couldn't be found by the NetworkUtils class. The NetworkUtils utilize the underlying JDK NetworkInterface class which, when asked to lookup by name only takes physical interfaces into account, failing at virtual (or subinterfaces) ones (returning null). Note that when interating over all interfaces, both physical and virtual ones are taken into account. This changeset asks for all known interfaces, iterates over them and matches on the given name as part of the loop, allowing it to catch both physical and virtual interfaces. As a result, elasticsearch can now also serve on virtual interfaces. A test case has been added which makes sure that all iterable interfaces can be found by their respective name. Note that this PR is a second iteration over the previously merged but later reverted elastic#19537 because it causes tests to fail when interfaces are down. The test has been modified to take this into account now. Closes elastic#17473 Closes elastic#19568 Relates elastic#19537

Previously when trying to listen on virtual interfaces during bootstrap the application would stop working - the interface couldn't be found by the NetworkUtils class. The NetworkUtils utilize the underlying JDK NetworkInterface class which, when asked to lookup by name only takes physical interfaces into account, failing at virtual (or subinterfaces) ones (returning null). Note that when interating over all interfaces, both physical and virtual ones are taken into account. This changeset asks for all known interfaces, iterates over them and matches on the given name as part of the loop, allowing it to catch both physical and virtual interfaces. As a result, elasticsearch can now also serve on virtual interfaces. A test case has been added which makes sure that all iterable interfaces can be found by their respective name. Note that this PR is a second iteration over the previously merged but later reverted #19537 because it causes tests to fail when interfaces are down. The test has been modified to take this into account now. Closes #17473 Closes #19568 Relates #19537

rmuir added the >bug label Jul 21, 2016

clintongormley added the :Distributed Coordination/Network Http and internode communication implementations label Jul 21, 2016

jasontedor added review v5.0.0-alpha5 labels Jul 21, 2016

jasontedor merged commit 4cb8b62 into elastic:master Jul 22, 2016

daschl mentioned this pull request Jul 25, 2016

Network: Allow to listen on virtual interfaces. #19568

Closed

clintongormley changed the title ~~Network: Allow to listen on virtual interfaces.~~ Allow to listen on virtual interfaces. Jul 26, 2016

clintongormley removed the v5.0.0-alpha5 label Jul 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to listen on virtual interfaces. #19537

Allow to listen on virtual interfaces. #19537

daschl commented Jul 21, 2016 •

edited by jasontedor

Loading

elasticmachine commented Jul 21, 2016

daschl commented Jul 21, 2016

daschl commented Jul 21, 2016

jasontedor commented Jul 21, 2016 •

edited

Loading

daschl commented Jul 21, 2016

rmuir commented Jul 21, 2016

jasontedor commented Jul 21, 2016

daschl commented Jul 21, 2016

daschl commented Jul 22, 2016 •

edited

Loading

rmuir commented Jul 22, 2016

jasontedor commented Jul 22, 2016

jasontedor commented Jul 22, 2016

daschl commented Jul 22, 2016

jasontedor commented Jul 22, 2016

jasontedor commented Jul 22, 2016

karlra commented Jul 22, 2016

dakrone commented Jul 22, 2016

jasontedor commented Jul 22, 2016

jasontedor commented Jul 22, 2016 •

edited

Loading

daschl commented Jul 22, 2016

Allow to listen on virtual interfaces. #19537

Allow to listen on virtual interfaces. #19537

Conversation

daschl commented Jul 21, 2016 • edited by jasontedor Loading

elasticmachine commented Jul 21, 2016

daschl commented Jul 21, 2016

daschl commented Jul 21, 2016

jasontedor commented Jul 21, 2016 • edited Loading

daschl commented Jul 21, 2016

rmuir commented Jul 21, 2016

jasontedor commented Jul 21, 2016

daschl commented Jul 21, 2016

daschl commented Jul 22, 2016 • edited Loading

rmuir commented Jul 22, 2016

jasontedor commented Jul 22, 2016

jasontedor commented Jul 22, 2016

daschl commented Jul 22, 2016

jasontedor commented Jul 22, 2016

jasontedor commented Jul 22, 2016

karlra commented Jul 22, 2016

dakrone commented Jul 22, 2016

jasontedor commented Jul 22, 2016

jasontedor commented Jul 22, 2016 • edited Loading

daschl commented Jul 22, 2016

daschl commented Jul 21, 2016 •

edited by jasontedor

Loading

jasontedor commented Jul 21, 2016 •

edited

Loading

daschl commented Jul 22, 2016 •

edited

Loading

jasontedor commented Jul 22, 2016 •

edited

Loading