Pull request: Do not ping when there's a single IP in the response

Merge in DNS/dnsproxy from fastip-single-ip to master

Updates AdguardTeam/AdGuardHome#3315

Squashed commit of the following:

commit 9dfb8ee
Author: Andrey Meshkov <am@adguard.com>
Date:   Tue Jul 6 12:38:53 2021 +0300

    use NoError

commit fd840ee
Author: Andrey Meshkov <am@adguard.com>
Date:   Tue Jul 6 11:40:54 2021 +0300

    Do not ping when there's a single IP in the response

fastip/ping.go

@@ -36,6 +36,13 @@ func (f *FastestAddr) pingAll(host string, ips []net.IP) (bool, *pingResult) {
 		return false, nil
 	}
 
+	if len(ips) == 1 {
+		return true, &pingResult{
+			ip:      ips[0],
+			success: true,
+		}
+	}
+
 	// channel that we will use to get the ping result
 	ch := make(chan *pingResult, len(ips)*len(f.tcpPorts))
 

fastip/ping_test.go

@@ -6,42 +6,63 @@ import (
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+func TestPingSingleIP(t *testing.T) {
+	ip := net.ParseIP("127.0.0.1")
+	f := NewFastestAddr()
+	found, res := f.pingAll("test", []net.IP{ip})
+	require.True(t, found)
+	require.NotNil(t, res)
+	require.True(t, res.success)
+	require.Equal(t, ip, res.ip)
+
+	// There was no ping so the port is zero
+	require.Equal(t, uint(0), res.tcpPort)
+
+	// Nothing in the cache since there was no ping
+	ce := f.cacheFind(ip)
+	require.Nil(t, ce)
+}
+
 func TestPingSuccess(t *testing.T) {
 	// Listener that we're using for TCP checks
 	listener, err := net.Listen("tcp", ":0")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	ip := net.ParseIP("127.0.0.1")
 	port := uint(listener.Addr().(*net.TCPAddr).Port)
 	defer listener.Close()
 
 	f := NewFastestAddr()
 	f.tcpPorts = []uint{port}
 
-	found, res := f.pingAll("test", []net.IP{ip})
-	assert.True(t, found)
-	assert.NotNil(t, res)
-	assert.True(t, res.success)
-	assert.Equal(t, ip, res.ip)
+	// We need at least two IPs so adding a random remote IP here
+	ips := []net.IP{ip, net.ParseIP("8.8.8.8")}
+	found, res := f.pingAll("test", ips)
+	require.True(t, found)
+	require.NotNil(t, res)
+	require.True(t, res.success)
+	require.Equal(t, ip, res.ip)
+	require.Equal(t, port, res.tcpPort)
 
-	// don't forget to check cache
+	// don't forget to check the cache
 	ce := f.cacheFind(ip)
-	assert.NotNil(t, ce)
-	assert.Equal(t, 0, ce.status)
+	require.NotNil(t, ce)
+	require.Equal(t, 0, ce.status)
 }
 
 func TestPingFail(t *testing.T) {
 	ip := net.ParseIP("127.0.0.1")
-	port := uint(getFreePort())
+	ip2 := net.ParseIP("127.0.0.2")
+	port := getFreePort()
 
 	f := NewFastestAddr()
 	f.tcpPorts = []uint{port}
 
-	found, res := f.pingAll("test", []net.IP{ip})
-	assert.False(t, found)
-	assert.Nil(t, res)
+	found, res := f.pingAll("test", []net.IP{ip, ip2})
+	require.False(t, found)
+	require.Nil(t, res)
 
 	if runtime.GOOS != "windows" {
 		// it appears that on Windows connectex has some strange behavior
@@ -51,21 +72,21 @@ func TestPingFail(t *testing.T) {
 
 		// don't forget to check cache
 		ce := f.cacheFind(ip)
-		assert.NotNil(t, ce)
-		assert.Equal(t, 1, ce.status)
+		require.NotNil(t, ce)
+		require.Equal(t, 1, ce.status)
 	}
 }
 
 func TestPingFastest(t *testing.T) {
 	// Listener that we're using for TCP checks
 	listener, err := net.Listen("tcp", ":0")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 	ip := net.ParseIP("127.0.0.1")
 	port := uint(listener.Addr().(*net.TCPAddr).Port)
 	defer listener.Close()
 
 	f := NewFastestAddr()
-	f.tcpPorts = []uint{port, 443}
+	f.tcpPorts = []uint{port, 443} // add 443 since it's definitely used by 8.8.8.8
 
 	// test ips
 	ips := []net.IP{ip}
@@ -75,15 +96,16 @@ func TestPingFastest(t *testing.T) {
 	ips = append(ips, net.ParseIP("8.8.8.8"))
 
 	found, res := f.pingAll("test", ips)
-	assert.True(t, found)
-	assert.NotNil(t, res)
-	assert.True(t, res.success)
-	assert.Equal(t, ip, res.ip)
+	require.True(t, found)
+	require.NotNil(t, res)
+	require.True(t, res.success)
+	require.Equal(t, ip, res.ip)
+	require.Equal(t, port, res.tcpPort)
 
 	// don't forget to check cache
 	ce := f.cacheFind(ip)
-	assert.NotNil(t, ce)
-	assert.Equal(t, 0, ce.status)
+	require.NotNil(t, ce)
+	require.Equal(t, 0, ce.status)
 }
 
 func getFreePort() uint {