Refactor ip in use algorithm (#4268)

pkg/networkutils/networkutils.go

@@ -1,9 +1,11 @@
 package networkutils
 
 import (
+	"errors"
 	"fmt"
 	"net"
 	"strconv"
+	"syscall"
 	"time"
 )
 
@@ -24,23 +26,21 @@ func ValidateIP(ip string) error {
 	return nil
 }
 
-// IsIPInUse performs a soft check to see if there are any services listening on a selection of common ports at
-// ip by trying to establish a TCP connection. Ports checked include: 22, 23, 80, 443 and 6443 (Kubernetes API Server).
-// Each connection attempt allows up-to 500ms for a response.
-//
-// todo(chrisdoherty) change to an icmp approach to eliminate the need for ports.
+// IsIPInUse performs a best effort check to see if an IP address is in use. It is not completely
+// reliable as testing if an IP is in use is inherently difficult, particularly with non-trivial
+// network topologies.
 func IsIPInUse(client NetClient, ip string) bool {
-	ports := []string{"22", "23", "80", "443", "6443"}
-	for _, port := range ports {
-		address := net.JoinHostPort(ip, port)
-		conn, err := client.DialTimeout("tcp", address, 500*time.Millisecond)
-		if err == nil {
-			conn.Close()
-			return true
-		}
+	// Dial and immediately close the connection if it was established as its superfluous for
+	// our check. We use port 80 as its common and is more likely to get through firewalls
+	// than other ports.
+	conn, err := client.DialTimeout("tcp", net.JoinHostPort(ip, "80"), 500*time.Millisecond)
+	if err == nil {
+		conn.Close()
 	}
 
-	return false
+	// If we establish a connection or we receive a response assume that address is in use.
+	// The latter case covers situations like an IP in use but the port requested is not open.
+	return err == nil || errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.ECONNRESET)
 }
 
 func IsPortInUse(client NetClient, host string, port string) bool {

pkg/networkutils/networkutils_test.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"net"
 	"reflect"
+	"syscall"
 	"testing"
 	"time"
 
@@ -41,13 +42,24 @@ func TestIsIPInUsePass(t *testing.T) {
 
 	client := mocks.NewMockNetClient(ctrl)
 	client.EXPECT().DialTimeout(gomock.Any(), gomock.Any(), gomock.Any()).
-		Times(5).
 		Return(nil, errors.New("no connection"))
 
 	res := networkutils.IsIPInUse(client, "10.10.10.10")
 	g.Expect(res).To(gomega.BeFalse())
 }
 
+func TestIsIPInUseConnectionRefused(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	g := gomega.NewWithT(t)
+
+	client := mocks.NewMockNetClient(ctrl)
+	client.EXPECT().DialTimeout(gomock.Any(), gomock.Any(), gomock.Any()).
+		Return(nil, syscall.ECONNREFUSED)
+
+	res := networkutils.IsIPInUse(client, "10.10.10.10")
+	g.Expect(res).To(gomega.BeTrue())
+}
+
 func TestIsIPInUseFail(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	g := gomega.NewWithT(t)

pkg/providers/tinkerbell/assert_test.go

@@ -153,7 +153,6 @@ func TestNewIPNotInUseAssertion_NotInUseSucceeds(t *testing.T) {
 	netClient := mocks.NewMockNetClient(ctrl)
 	netClient.EXPECT().
 		DialTimeout(gomock.Any(), gomock.Any(), gomock.Any()).
-		Times(5).
 		Return(nil, errors.New("failed to connect"))
 
 	clusterSpec := NewDefaultValidClusterSpecBuilder().Build()
@@ -187,7 +186,6 @@ func TestAssertTinkerbellIPNotInUse_NotInUseSucceeds(t *testing.T) {
 	netClient := mocks.NewMockNetClient(ctrl)
 	netClient.EXPECT().
 		DialTimeout(gomock.Any(), gomock.Any(), gomock.Any()).
-		Times(5).
 		Return(nil, errors.New("failed to connect"))
 
 	clusterSpec := NewDefaultValidClusterSpecBuilder().Build()

pkg/providers/tinkerbell/tinkerbell.go

@@ -29,8 +29,8 @@ import (
 )
 
 const (
-	maxRetries                = 30
-	backOffPeriod             = 5 * time.Second
+	maxRetries    = 30
+	backOffPeriod = 5 * time.Second
 )
 
 var (

pkg/providers/validator/validate_test.go

@@ -16,7 +16,7 @@ import (
 	"github.com/aws/eks-anywhere/pkg/providers/validator"
 )
 
-func TestValidateControlPlaneIpUniqueness(t *testing.T) {
+func TestValidateControlPlaneIPUniqueness(t *testing.T) {
 	g := NewWithT(t)
 	cluster := &v1alpha1.Cluster{
 		Spec: v1alpha1.ClusterSpec{
@@ -30,7 +30,6 @@ func TestValidateControlPlaneIpUniqueness(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	client := mocks.NewMockNetClient(ctrl)
 	client.EXPECT().DialTimeout(gomock.Any(), gomock.Any(), gomock.Any()).
-		Times(5).
 		Return(nil, errors.New("no connection"))
 	ipValidator := validator.NewIPValidator(validator.CustomNetClient(client))