From eafb5e33d9f47c2ff5be464598855a6916fe8bac Mon Sep 17 00:00:00 2001 From: Yulong Wu Date: Fri, 29 Nov 2024 12:48:21 +0000 Subject: [PATCH 1/2] Replace URL with String as the key The equals() implementation of URL resolves the hostname into an IP address. When multiple URLs are mapped into the same IP (especially with 1.1.1.1 DNS server), it will cause the following exception: ``` java.lang.IllegalArgumentException: Multiple entries with same key ``` --- .../p2p/hostip/NetworkQueryHostIp.java | 44 +++++++------------ 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/com/radixdlt/p2p/hostip/NetworkQueryHostIp.java b/core/src/main/java/com/radixdlt/p2p/hostip/NetworkQueryHostIp.java index 6d43bc177e..803a8cc148 100644 --- a/core/src/main/java/com/radixdlt/p2p/hostip/NetworkQueryHostIp.java +++ b/core/src/main/java/com/radixdlt/p2p/hostip/NetworkQueryHostIp.java @@ -72,8 +72,6 @@ import com.radixdlt.lang.Result; import com.radixdlt.utils.properties.RuntimeProperties; import java.io.IOException; -import java.net.MalformedURLException; -import java.net.URL; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -98,21 +96,21 @@ final class NetworkQueryHostIp { public record VotedResult( Optional conclusiveHostIp, - ImmutableMap> individualQueryResults) {} + ImmutableMap> individualQueryResults) {} @VisibleForTesting static final String QUERY_URLS_PROPERTY = "network.host_ip_query_urls"; @VisibleForTesting - static final ImmutableList DEFAULT_QUERY_URLS = + static final ImmutableList DEFAULT_QUERY_URLS = ImmutableList.of( - makeurl("https://checkip.amazonaws.com/"), - makeurl("https://ipv4.icanhazip.com/"), - makeurl("https://myexternalip.com/raw"), - makeurl("https://ipecho.net/plain"), - makeurl("https://www.trackip.net/ip"), - makeurl("https://ifconfig.co/ip")); + "https://checkip.amazonaws.com/", + "https://ipv4.icanhazip.com/", + "https://myexternalip.com/raw", + "https://ipecho.net/plain", + "https://www.trackip.net/ip", + "https://ifconfig.co/ip"); - static NetworkQueryHostIp create(Collection urls) { + static NetworkQueryHostIp create(Collection urls) { return new NetworkQueryHostIp(urls); } @@ -121,18 +119,16 @@ static NetworkQueryHostIp create(RuntimeProperties properties) { if (urlsProperty == null || urlsProperty.trim().isEmpty()) { return create(DEFAULT_QUERY_URLS); } - ImmutableList urls = - Arrays.asList(urlsProperty.split(",")).stream() - .map(NetworkQueryHostIp::makeurl) - .collect(ImmutableList.toImmutableList()); + ImmutableList urls = + Arrays.asList(urlsProperty.split(",")).stream().collect(ImmutableList.toImmutableList()); return create(urls); } - private final List hosts; + private final List hosts; private final OkHttpClient okHttpClient; private final Supplier result = Suppliers.memoize(this::get); - NetworkQueryHostIp(Collection urls) { + NetworkQueryHostIp(Collection urls) { if (urls.isEmpty()) { throw new IllegalArgumentException("At least one URL must be specified"); } @@ -153,11 +149,11 @@ VotedResult get() { Collections.shuffle(this.hosts); log.debug("Using hosts {}", this.hosts); final Map successCounts = Maps.newHashMap(); - final ImmutableMap.Builder> queryResults = + final ImmutableMap.Builder> queryResults = ImmutableMap.builder(); int maxCount = 0; Optional maxResult = Optional.empty(); - for (URL url : this.hosts) { + for (String url : this.hosts) { final Result result = query(url); queryResults.put(url, result); if (result.isSuccess()) { @@ -191,7 +187,7 @@ VotedResult get() { return new VotedResult(maxResult, queryResults.build()); } - Result query(URL url) { + Result query(String url) { try { // Some services simply require the headers we set here: final var request = @@ -214,12 +210,4 @@ Result query(URL url) { return Result.error(ex); } } - - private static URL makeurl(String s) { - try { - return new URL(s); - } catch (MalformedURLException ex) { - throw new IllegalStateException("While constructing URL for " + s, ex); - } - } } From 02ea27be49c9295c3f00916997625c9fa002f010 Mon Sep 17 00:00:00 2001 From: Yulong Wu Date: Fri, 29 Nov 2024 14:31:37 +0000 Subject: [PATCH 2/2] Fix tests --- .../p2p/hostip/NetworkQueryHostIp.java | 14 ++++- .../p2p/hostip/NetworkQueryHostIpTest.java | 57 ++++++++----------- 2 files changed, 34 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/com/radixdlt/p2p/hostip/NetworkQueryHostIp.java b/core/src/main/java/com/radixdlt/p2p/hostip/NetworkQueryHostIp.java index 803a8cc148..78a7f01f71 100644 --- a/core/src/main/java/com/radixdlt/p2p/hostip/NetworkQueryHostIp.java +++ b/core/src/main/java/com/radixdlt/p2p/hostip/NetworkQueryHostIp.java @@ -128,6 +128,10 @@ static NetworkQueryHostIp create(RuntimeProperties properties) { private final OkHttpClient okHttpClient; private final Supplier result = Suppliers.memoize(this::get); + NetworkQueryHostIp() { + this(DEFAULT_QUERY_URLS); + } + NetworkQueryHostIp(Collection urls) { if (urls.isEmpty()) { throw new IllegalArgumentException("At least one URL must be specified"); @@ -140,20 +144,24 @@ int count() { return this.hosts.size(); } + List hosts() { + return this.hosts; + } + public VotedResult queryNetworkHosts() { return result.get(); } VotedResult get() { // Make sure we don't DoS the first one on the list - Collections.shuffle(this.hosts); - log.debug("Using hosts {}", this.hosts); + Collections.shuffle(this.hosts()); + log.debug("Using hosts {}", this.hosts()); final Map successCounts = Maps.newHashMap(); final ImmutableMap.Builder> queryResults = ImmutableMap.builder(); int maxCount = 0; Optional maxResult = Optional.empty(); - for (String url : this.hosts) { + for (String url : this.hosts()) { final Result result = query(url); queryResults.put(url, result); if (result.isSuccess()) { diff --git a/core/src/test/java/com/radixdlt/p2p/hostip/NetworkQueryHostIpTest.java b/core/src/test/java/com/radixdlt/p2p/hostip/NetworkQueryHostIpTest.java index 95ac5e6563..5def5ce9b2 100644 --- a/core/src/test/java/com/radixdlt/p2p/hostip/NetworkQueryHostIpTest.java +++ b/core/src/test/java/com/radixdlt/p2p/hostip/NetworkQueryHostIpTest.java @@ -65,18 +65,19 @@ package com.radixdlt.p2p.hostip; import static org.junit.Assert.*; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.*; +import com.radixdlt.lang.Result; import com.radixdlt.utils.properties.RuntimeProperties; -import java.io.ByteArrayInputStream; import java.io.IOException; -import java.net.HttpURLConnection; -import java.net.URL; -import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Collections; -import java.util.List; +import java.util.Map; import java.util.Optional; import org.junit.Test; +import org.mockito.Mockito; public class NetworkQueryHostIpTest { @@ -112,43 +113,31 @@ public void testCollectionEmpty() { @Test public void testCollectionNotEmptyQueryNotSuccessful() throws IOException { - NetworkQueryHostIp nqhip = NetworkQueryHostIp.create(List.of(makeUrl(404, "not found"))); - Optional host = nqhip.queryNetworkHosts().conclusiveHostIp(); - assertFalse(host.isPresent()); - } - - @Test - public void testCollectionNotEmptyQueryIoException() throws IOException { - URL url = mock(URL.class); - doReturn("https://mock.url/with-io-problems").when(url).toString(); - doThrow(new IOException("test exception")).when(url).openConnection(); - NetworkQueryHostIp nqhip = NetworkQueryHostIp.create(List.of(url)); + NetworkQueryHostIp nqhip = + makeNetworkQueryHostIp(Map.of("a", Result.error(new IOException("")))); Optional host = nqhip.queryNetworkHosts().conclusiveHostIp(); assertFalse(host.isPresent()); } @Test public void testCollectionAllDifferent() throws IOException { - List urls = - List.of( - makeUrl(200, "127.0.0.1"), - makeUrl(200, "127.0.0.2"), - makeUrl(200, "127.0.0.3"), - makeUrl(200, "127.0.0.4")); - NetworkQueryHostIp nqhip = NetworkQueryHostIp.create(urls); + NetworkQueryHostIp nqhip = + makeNetworkQueryHostIp( + Map.of( + "a", Result.success(new HostIp("1")), + "b", Result.success(new HostIp("2")), + "c", Result.success(new HostIp("2")), + "d", Result.success(new HostIp("4")))); Optional host = nqhip.queryNetworkHosts().conclusiveHostIp(); - assertFalse(host.isPresent()); + assertEquals(Optional.of(new HostIp("2")), host); } - private static URL makeUrl(int status, String body) throws IOException { - HttpURLConnection conn = mock(HttpURLConnection.class); - doReturn(status).when(conn).getResponseCode(); - doReturn(new ByteArrayInputStream(body.getBytes(StandardCharsets.UTF_8))) - .when(conn) - .getInputStream(); - URL url = mock(URL.class); - doReturn(conn).when(url).openConnection(); - doReturn("https://mock.url/?result=%s".formatted(body)).when(url).toString(); - return url; + NetworkQueryHostIp makeNetworkQueryHostIp(Map> responses) { + NetworkQueryHostIp nqhip = spy(NetworkQueryHostIp.class); + doReturn(new ArrayList(responses.keySet())).when(nqhip).hosts(); + for (var entry : responses.entrySet()) { + doReturn(entry.getValue()).when(nqhip).query(Mockito.eq(entry.getKey())); + } + return nqhip; } }