From 0c8f6a69bda2034ef00e1766dd0a9058dfd03dd6 Mon Sep 17 00:00:00 2001
From: Will Hegedus <whegedus@linode.com>
Date: Wed, 20 Apr 2022 13:08:38 -0400
Subject: [PATCH] feat!: handle IP address comparison

Previously there was no distinction between an IP address and any other string
when doing a comparison to determine which is "less" when determining which endpoint to actually create.

This explicitly handles IP addresses and will always prefer
them over non-IP addresses when determining which of two targets is less.
---
 endpoint/endpoint.go      | 17 ++++++++++++++++-
 endpoint/endpoint_test.go | 39 +++++++++++++++++++++++++++++++++++++++
 plan/conflict_test.go     |  2 +-
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/endpoint/endpoint.go b/endpoint/endpoint.go
index 9a78ffc97..e6e92da18 100644
--- a/endpoint/endpoint.go
+++ b/endpoint/endpoint.go
@@ -18,6 +18,7 @@ package endpoint
 
 import (
 	"fmt"
+	"net/netip"
 	"sort"
 	"strings"
 
@@ -107,7 +108,21 @@ func (t Targets) IsLess(o Targets) bool {
 
 	for i, e := range t {
 		if e != o[i] {
-			return e < o[i]
+			// Explicitly prefers IP addresses (e.g. A records) over FQDNs (e.g. CNAMEs).
+			// This prevents behavior like `1-2-3-4.example.com` being "less" than `1.2.3.4` when doing lexicographical string comparison.
+			ipA, _ := netip.ParseAddr(e)
+			ipB, _ := netip.ParseAddr(o[i])
+
+			switch {
+			case ipA.IsValid() && ipB.IsValid():
+				return ipA.Less(ipB)
+			case ipA.IsValid() && !ipB.IsValid():
+				return true
+			case !ipA.IsValid() && ipB.IsValid():
+				return false
+			default:
+				return e < o[i]
+			}
 		}
 	}
 	return false
diff --git a/endpoint/endpoint_test.go b/endpoint/endpoint_test.go
index 05104adbf..81e7f4c33 100644
--- a/endpoint/endpoint_test.go
+++ b/endpoint/endpoint_test.go
@@ -76,3 +76,42 @@ func TestSameFailures(t *testing.T) {
 		}
 	}
 }
+
+func TestIsLess(t *testing.T) {
+	testsA := []Targets{
+		{""},
+		{"1.2.3.4"},
+		{"1.2.3.4"},
+		{"example.org", "example.com"},
+		{"8.8.8.8", "8.8.4.4"},
+		{"1-2-3-4.example.org", "EXAMPLE.ORG"},
+		{"1-2-3-4.example.org", "EXAMPLE.ORG", "1.2.3.4"},
+		{"example.com", "example.org"},
+	}
+	testsB := []Targets{
+		{"", ""},
+		{"1-2-3-4.example.org"},
+		{"1.2.3.5"},
+		{"example.com", "examplea.org"},
+		{"8.8.8.8"},
+		{"1.2.3.4", "EXAMPLE.ORG"},
+		{"1-2-3-4.example.org", "EXAMPLE.ORG"},
+		{"example.com", "example.org"},
+	}
+	expected := []bool{
+		true,
+		true,
+		true,
+		true,
+		false,
+		false,
+		false,
+		false,
+	}
+
+	for i, d := range testsA {
+		if d.IsLess(testsB[i]) != expected[i] {
+			t.Errorf("%v < %v is expected to be %v", d, testsB[i], expected[i])
+		}
+	}
+}
diff --git a/plan/conflict_test.go b/plan/conflict_test.go
index ff2005223..f40f7d4c3 100644
--- a/plan/conflict_test.go
+++ b/plan/conflict_test.go
@@ -114,7 +114,7 @@ func (suite *ResolverSuite) TestStrictResolver() {
 	suite.Equal(suite.fooV1Cname, suite.perResource.ResolveCreate([]*endpoint.Endpoint{suite.fooV2Cname, suite.fooV1Cname}), "should pick min one")
 
 	//test that perResource resolver preserves resource if it still exists
-	suite.Equal(suite.bar127A, suite.perResource.ResolveUpdate(suite.bar127A, []*endpoint.Endpoint{suite.bar127AAnother, suite.bar127A}), "should pick min for update when same resource endpoint occurs multiple times (remove after multiple-target support") // TODO:remove this test
+	suite.Equal(suite.bar127AAnother, suite.perResource.ResolveUpdate(suite.bar127A, []*endpoint.Endpoint{suite.bar127AAnother, suite.bar127A}), "should pick min for update when same resource endpoint occurs multiple times (remove after multiple-target support") // TODO:remove this test
 	suite.Equal(suite.bar127A, suite.perResource.ResolveUpdate(suite.bar127A, []*endpoint.Endpoint{suite.bar192A, suite.bar127A}), "should pick existing resource")
 	suite.Equal(suite.fooV2Cname, suite.perResource.ResolveUpdate(suite.fooV2Cname, []*endpoint.Endpoint{suite.fooV2Cname, suite.fooV2CnameDuplicate}), "should pick existing resource even if targets are same")
 	suite.Equal(suite.fooA5, suite.perResource.ResolveUpdate(suite.fooV1Cname, []*endpoint.Endpoint{suite.fooA5, suite.fooV2Cname}), "should pick new if resource was deleted")