Fix NetworkUtilsTests

[original-brownbear]

• Follow up to Fix NetworkUtilsTests #42109:
  • Now that we know that the test failures come from veth type network interfaces being created/destroyed concurrently to the test run, we can fix this test by simply ignoring these interfaces to prevent making a syscall to them when they don't exist anymore

[elasticmachine]

Pinging @elastic/es-core-infra

[original-brownbear]

Jenkins run elasticsearch-ci/oss-distro-docs

[original-brownbear]

Jenkins run elasticsearch-ci/bwc

[jasontedor]

This feels like a hack to me, which has caused me to examine this test closely and see if we can do this another way (without having to add a hack that is a function of the fact that we run our tests in parallel in CI).

Going way back in history, this test was added in #19537 to address a problem we had where Elasticsearch could not listen on virtual interfaces by name. The test added here is testing too much. It's checking that we can lookup an interface by name, and also that the addresses match. Yet, that almost amount to a self-test. Here's the test as its structured today:

foreach network interface:
  if the interface is not or the interface does not have any addresses, continue
  get all the addresses from the interface
  get the name of the interface and use `NetworkUtils#getAddressesForInterface` to lookup the interface by name

Now, how does NetworkUtils#getAddressesForInterface work? Basically its:

foreach network interface:
  if the interface name matches in the input interface name, break out of the loop with that interface
  then some sanity checks that amount to making sure the interface is up and has some addresses
  return the addresses

So it's basically the same code as the expectations set up in the test. Instead, I think that all we need to test is that we can lookup an interface by name. Let's change the test to do that and that only:

diff --git a/server/src/main/java/org/elasticsearch/common/network/NetworkUtils.java b/server/src/main/java/org/elasticsearch/common/network/NetworkUtils.java
index 4e8f871ed30..16630ca46f1 100644
--- a/server/src/main/java/org/elasticsearch/common/network/NetworkUtils.java
+++ b/server/src/main/java/org/elasticsearch/common/network/NetworkUtils.java
@@ -200,10 +200,14 @@ public abstract class NetworkUtils {
     static InetAddress[] getAllAddresses() throws IOException {
         return filterAllAddresses(address -> true, "no up-and-running addresses found");
     }
+
+    static Optional<NetworkInterface> maybeGetInterfaceByName(final List<NetworkInterface> networkInterfaces, final String name) {
+        return networkInterfaces.stream().filter((netIf) -> name.equals(netIf.getName())).findFirst();
+    }
     
     /** Returns addresses for the given interface (it must be marked up) */
     static InetAddress[] getAddressesForInterface(String name) throws SocketException {
-        Optional<NetworkInterface> networkInterface = getInterfaces().stream().filter((netIf) -> name.equals(netIf.getName())).findFirst();
+        Optional<NetworkInterface> networkInterface = maybeGetInterfaceByName(getInterfaces(), name);
 
         if (networkInterface.isPresent() == false) {
             throw new IllegalArgumentException("No interface named '" + name + "' found, got " + getInterfaces());
diff --git a/server/src/test/java/org/elasticsearch/common/network/NetworkUtilsTests.java b/server/src/test/java/org/elasticsearch/common/network/NetworkUtilsTests.java
index ddc40961b07..6f6d3a55a40 100644
--- a/server/src/test/java/org/elasticsearch/common/network/NetworkUtilsTests.java
+++ b/server/src/test/java/org/elasticsearch/common/network/NetworkUtilsTests.java
@@ -20,13 +20,15 @@
 package org.elasticsearch.common.network;
 
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.hamcrest.OptionalMatchers;
 
 import java.net.InetAddress;
 import java.net.NetworkInterface;
-import java.net.SocketException;
-import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
 
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
 
 /**
  * Tests for network utils. Please avoid using any methods that cause DNS lookups!
@@ -80,27 +82,13 @@ public class NetworkUtilsTests extends ESTestCase {
         assertArrayEquals(new InetAddress[] { InetAddress.getByName("::1") }, NetworkUtils.filterIPV6(addresses));
     }
 
-    /**
-     * Test that selecting by name is possible and properly matches the addresses on all interfaces and virtual
-     * interfaces.
-     *
-     * Note that to avoid that this test fails when interfaces are down or they do not have addresses assigned to them,
-     * they are ignored.
-     */
+    // test that selecting by name is possible
     public void testAddressInterfaceLookup() throws Exception {
-        for (NetworkInterface netIf : NetworkUtils.getInterfaces()) {
-            try {
-                if (!netIf.isUp() || Collections.list(netIf.getInetAddresses()).isEmpty()) {
-                    continue;
-                }
-            } catch (SocketException e) {
-                throw new AssertionError("Failed to check if interface [" + netIf + "] is up", e);
-            }
-
-            String name = netIf.getName();
-            InetAddress[] expectedAddresses = Collections.list(netIf.getInetAddresses()).toArray(new InetAddress[0]);
-            InetAddress[] foundAddresses = NetworkUtils.getAddressesForInterface(name);
-            assertArrayEquals(expectedAddresses, foundAddresses);
+        final List<NetworkInterface> networkInterfaces = NetworkUtils.getInterfaces();
+        for (NetworkInterface netIf : networkInterfaces) {
+            final Optional<NetworkInterface> maybeNetworkInterface = NetworkUtils.maybeGetInterfaceByName(networkInterfaces, netIf.getName());
+            assertThat(maybeNetworkInterface, OptionalMatchers.isPresent());
+            assertThat(maybeNetworkInterface.get().getName(), equalTo(netIf.getName()));
         }
     }

What do you think?

[original-brownbear]

Thanks @jasontedor, that makes perfect sense. I applied your changes here now (+renamed that test method).

[jasontedor]

LGTM.

[jasontedor]

Thanks @original-brownbear!