diff --git a/pkg/networkutils/networkutils.go b/pkg/networkutils/networkutils.go index ecf6bd3c5b49..9279408dd87c 100644 --- a/pkg/networkutils/networkutils.go +++ b/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 { diff --git a/pkg/networkutils/networkutils_test.go b/pkg/networkutils/networkutils_test.go index 83d1f0000b50..e4d96b04e38a 100644 --- a/pkg/networkutils/networkutils_test.go +++ b/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) diff --git a/pkg/providers/tinkerbell/assert_test.go b/pkg/providers/tinkerbell/assert_test.go index 183ea2c539f1..c77a070f7f2e 100644 --- a/pkg/providers/tinkerbell/assert_test.go +++ b/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() diff --git a/pkg/providers/tinkerbell/tinkerbell.go b/pkg/providers/tinkerbell/tinkerbell.go index c9f508aad96b..91d00c6b4dcb 100644 --- a/pkg/providers/tinkerbell/tinkerbell.go +++ b/pkg/providers/tinkerbell/tinkerbell.go @@ -29,8 +29,8 @@ import ( ) const ( - maxRetries = 30 - backOffPeriod = 5 * time.Second + maxRetries = 30 + backOffPeriod = 5 * time.Second ) var ( diff --git a/pkg/providers/validator/validate_test.go b/pkg/providers/validator/validate_test.go index e90516f5d8d9..af9bdf541a6c 100644 --- a/pkg/providers/validator/validate_test.go +++ b/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))