Implementation of multiple targets based on PR kubernetes-sigs#404 and …

…kubernetes-sigs#396 (kubernetes-sigs#418)

* Endpoint.Target is now Endpoint.Targets. This is its own type representing mutliple targets for a single DNS name while adding some convenience for sorting and comparing

* Made everything compile and tests run through with the new Endpoint.Targets

* The ingress source can now properly handle multiple target ips per host

* Added custom conflict resolver, to better understand how conflict resolution has to work for me

* My custom conflict resolver behaves a bit different than the PerResource resolver, therefore I needed to modify the expected test result

Removed unnecessary FIXME

* The ingress source now creates CNAME endpoints with multiple targets to let the DNS provider decide how to handle multiple CNAME targets. This could be interesting for weighted targets etc.

* Adopted the expected results to the new way we create endpoints for CNAMEs

* Removed Add method from Targets since manipulating the slice through here is unnecessary complicated and doesn't deliver enough convenience

* Reverted ConflictResolver to the original one. There is some discussing to do what the best way is to handle conflicts

* Added missing documenting comment to IsLess of Targets

* Added documenting comments to Targets,Targets.Same and NewTargets to clarify their intention and usage

* Service source now also generates endpoints with multiple targets

* Service and Ingress source now sort all Targets for every Endpoint to make order of Targets predictable

* Endpoints generated by the Google Cloud DNS provider now also have sorted Targets to make order of Targets predictable

* Modified provider dyn to be able to compile with multi target changes

* Fixed small nitpicks, so my code is acceptable

* Fixed merge method after updating to new Targets. Replacing '!=' with .Same of course needs a boolean negation

* Tests for dyn provider now also use the new Targets instead of Target

* Simplified extractServiceIps as implied by linki to make it more readable

* ref: change service ClusterIP retrieval again

* Added entry to CHANGELOG.md describing the new features contained in this PR

CHANGELOG.md

@@ -1,5 +1,6 @@
   - DigitalOcean: DigitalOcean creates entries with host in them twice (#459) @njuettner
   - Bugfix: Retrive all DNSimple response pages (#468) @jbowes
+  - external-dns does now provide support for multiple targets for A records. This is currently only supported by the Google Cloud DNS provider (#418) @dereulenspiegel
   - Graceful handling of misconfigure password for dyn provider (#470) @jvassev
   - Don't log sensitive data on start (#463) @jvassev
   - Google: Improve logging to help trace misconfigurations (#388) @stealthybox

controller/controller_test.go

@@ -48,25 +48,25 @@ func (p *mockProvider) ApplyChanges(changes *plan.Changes) error {
 	}
 
 	for i := range changes.Create {
-		if changes.Create[i].DNSName != p.ExpectChanges.Create[i].DNSName || changes.Create[i].Target != p.ExpectChanges.Create[i].Target {
+		if changes.Create[i].DNSName != p.ExpectChanges.Create[i].DNSName || !changes.Create[i].Targets.Same(p.ExpectChanges.Create[i].Targets) {
 			return errors.New("created record is wrong")
 		}
 	}
 
 	for i := range changes.UpdateNew {
-		if changes.UpdateNew[i].DNSName != p.ExpectChanges.UpdateNew[i].DNSName || changes.UpdateNew[i].Target != p.ExpectChanges.UpdateNew[i].Target {
+		if changes.UpdateNew[i].DNSName != p.ExpectChanges.UpdateNew[i].DNSName || !changes.UpdateNew[i].Targets.Same(p.ExpectChanges.UpdateNew[i].Targets) {
 			return errors.New("delete record is wrong")
 		}
 	}
 
 	for i := range changes.UpdateOld {
-		if changes.UpdateOld[i].DNSName != p.ExpectChanges.UpdateOld[i].DNSName || changes.UpdateOld[i].Target != p.ExpectChanges.UpdateOld[i].Target {
+		if changes.UpdateOld[i].DNSName != p.ExpectChanges.UpdateOld[i].DNSName || !changes.UpdateOld[i].Targets.Same(p.ExpectChanges.UpdateOld[i].Targets) {
 			return errors.New("delete record is wrong")
 		}
 	}
 
 	for i := range changes.Delete {
-		if changes.Delete[i].DNSName != p.ExpectChanges.Delete[i].DNSName || changes.Delete[i].Target != p.ExpectChanges.Delete[i].Target {
+		if changes.Delete[i].DNSName != p.ExpectChanges.Delete[i].DNSName || !changes.Delete[i].Targets.Same(p.ExpectChanges.Delete[i].Targets) {
 			return errors.New("delete record is wrong")
 		}
 	}
@@ -91,11 +91,11 @@ func TestRunOnce(t *testing.T) {
 	source.On("Endpoints").Return([]*endpoint.Endpoint{
 		{
 			DNSName: "create-record",
-			Target:  "1.2.3.4",
+			Targets: endpoint.Targets{"1.2.3.4"},
 		},
 		{
 			DNSName: "update-record",
-			Target:  "8.8.4.4",
+			Targets: endpoint.Targets{"8.8.4.4"},
 		},
 	}, nil)
 
@@ -104,25 +104,25 @@ func TestRunOnce(t *testing.T) {
 		[]*endpoint.Endpoint{
 			{
 				DNSName: "update-record",
-				Target:  "8.8.8.8",
+				Targets: endpoint.Targets{"8.8.8.8"},
 			},
 			{
 				DNSName: "delete-record",
-				Target:  "4.3.2.1",
+				Targets: endpoint.Targets{"4.3.2.1"},
 			},
 		},
 		&plan.Changes{
 			Create: []*endpoint.Endpoint{
-				{DNSName: "create-record", Target: "1.2.3.4"},
+				{DNSName: "create-record", Targets: endpoint.Targets{"1.2.3.4"}},
 			},
 			UpdateNew: []*endpoint.Endpoint{
-				{DNSName: "update-record", Target: "8.8.4.4"},
+				{DNSName: "update-record", Targets: endpoint.Targets{"8.8.4.4"}},
 			},
 			UpdateOld: []*endpoint.Endpoint{
-				{DNSName: "update-record", Target: "8.8.8.8"},
+				{DNSName: "update-record", Targets: endpoint.Targets{"8.8.8.8"}},
 			},
 			Delete: []*endpoint.Endpoint{
-				{DNSName: "delete-record", Target: "4.3.2.1"},
+				{DNSName: "delete-record", Targets: endpoint.Targets{"4.3.2.1"}},
 			},
 		},
 	)

endpoint/endpoint.go

@@ -18,6 +18,7 @@ package endpoint
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 )
 
@@ -38,12 +39,78 @@ func (ttl TTL) IsConfigured() bool {
 	return ttl > 0
 }
 
+// Targets is a representation of a list of targets for an endpoint.
+type Targets []string
+
+// NewTargets is a convenience method to create a new Targets object from a vararg of strings
+func NewTargets(target ...string) Targets {
+	t := make(Targets, 0, len(target))
+	t = append(t, target...)
+	return t
+}
+
+func (t Targets) String() string {
+	return strings.Join(t, ";")
+}
+
+func (t Targets) Len() int {
+	return len(t)
+}
+
+func (t Targets) Less(i, j int) bool {
+	return t[i] < t[j]
+}
+
+func (t Targets) Swap(i, j int) {
+	t[i], t[j] = t[j], t[i]
+}
+
+// Same compares to Targets and returns true if they are completely identical
+func (t Targets) Same(o Targets) bool {
+	if len(t) != len(o) {
+		return false
+	}
+	sort.Stable(t)
+	sort.Stable(o)
+
+	for i, e := range t {
+		if e != o[i] {
+			return false
+		}
+	}
+	return true
+}
+
+// IsLess should fulfill the requirement to compare two targets and chosse the 'lesser' one.
+// In the past target was a simple string so simple string comparison could be used. Now we define 'less'
+// as either being the shorter list of targets or where the first entry is less.
+// FIXME We really need to define under which circumstances a list Targets is considered 'less'
+// than another.
+func (t Targets) IsLess(o Targets) bool {
+	if len(t) < len(o) {
+		return true
+	}
+	if len(t) > len(o) {
+		return false
+	}
+
+	sort.Sort(t)
+	sort.Sort(o)
+
+	for i, e := range t {
+		if e != o[i] {
+			return e < o[i]
+		}
+	}
+	return false
+}
+
 // Endpoint is a high-level way of a connection between a service and an IP
 type Endpoint struct {
 	// The hostname of the DNS record
 	DNSName string
-	// The target the DNS record points to
-	Target string
+	// The targets the DNS record points to
+	Targets Targets
 	// RecordType type of record, e.g. CNAME, A, TXT etc
 	RecordType string
 	// TTL for the record
@@ -61,13 +128,13 @@ func NewEndpoint(dnsName, target, recordType string) *Endpoint {
 func NewEndpointWithTTL(dnsName, target, recordType string, ttl TTL) *Endpoint {
 	return &Endpoint{
 		DNSName:    strings.TrimSuffix(dnsName, "."),
-		Target:     strings.TrimSuffix(target, "."),
+		Targets:    Targets{strings.TrimSuffix(target, ".")},
 		RecordType: recordType,
 		Labels:     NewLabels(),
 		RecordTTL:  ttl,
 	}
 }
 
 func (e *Endpoint) String() string {
-	return fmt.Sprintf("%s %d IN %s %s", e.DNSName, e.RecordTTL, e.RecordType, e.Target)
+	return fmt.Sprintf("%s %d IN %s %s", e.DNSName, e.RecordTTL, e.RecordType, e.Targets)
 }

endpoint/endpoint_test.go

@@ -22,15 +22,15 @@ import (
 
 func TestNewEndpoint(t *testing.T) {
 	e := NewEndpoint("example.org", "foo.com", "CNAME")
-	if e.DNSName != "example.org" || e.Target != "foo.com" || e.RecordType != "CNAME" {
+	if e.DNSName != "example.org" || e.Targets[0] != "foo.com" || e.RecordType != "CNAME" {
 		t.Error("endpoint is not initialized correctly")
 	}
 	if e.Labels == nil {
 		t.Error("Labels is not initialized")
 	}
 
 	w := NewEndpoint("example.org.", "load-balancer.com.", "")
-	if w.DNSName != "example.org" || w.Target != "load-balancer.com" || w.RecordType != "" {
+	if w.DNSName != "example.org" || w.Targets[0] != "load-balancer.com" || w.RecordType != "" {
 		t.Error("endpoint is not initialized correctly")
 	}
 }

internal/testutils/endpoint.go

@@ -33,21 +33,20 @@ func (b byAllFields) Less(i, j int) bool {
 		return true
 	}
 	if b[i].DNSName == b[j].DNSName {
-		if b[i].Target < b[j].Target {
-			return true
-		}
-		if b[i].Target == b[j].Target {
+		// This rather bad, we need a more complex comparison for Targets, which considers all elements
+		if b[i].Targets.Same(b[j].Targets) {
 			return b[i].RecordType <= b[j].RecordType
 		}
-		return false
+		return b[i].Targets.String() <= b[j].Targets.String()
+
 	}
 	return false
 }
 
 // SameEndpoint returns true if two endpoints are same
 // considers example.org. and example.org DNSName/Target as different endpoints
 func SameEndpoint(a, b *endpoint.Endpoint) bool {
-	return a.DNSName == b.DNSName && a.Target == b.Target && a.RecordType == b.RecordType &&
+	return a.DNSName == b.DNSName && a.Targets.Same(b.Targets) && a.RecordType == b.RecordType &&
 		a.Labels[endpoint.OwnerLabelKey] == b.Labels[endpoint.OwnerLabelKey] && a.RecordTTL == b.RecordTTL &&
 		a.Labels[endpoint.ResourceLabelKey] == b.Labels[endpoint.ResourceLabelKey]
 }

internal/testutils/endpoint_test.go

@@ -27,31 +27,31 @@ func ExampleSameEndpoints() {
 	eps := []*endpoint.Endpoint{
 		{
 			DNSName: "example.org",
-			Target:  "load-balancer.org",
+			Targets: endpoint.Targets{"load-balancer.org"},
 		},
 		{
 			DNSName:    "example.org",
-			Target:     "load-balancer.org",
+			Targets:    endpoint.Targets{"load-balancer.org"},
 			RecordType: endpoint.RecordTypeTXT,
 		},
 		{
 			DNSName:    "abc.com",
-			Target:     "something",
+			Targets:    endpoint.Targets{"something"},
 			RecordType: endpoint.RecordTypeTXT,
 		},
 		{
 			DNSName:    "abc.com",
-			Target:     "1.2.3.4",
+			Targets:    endpoint.Targets{"1.2.3.4"},
 			RecordType: endpoint.RecordTypeA,
 		},
 		{
 			DNSName:    "bbc.com",
-			Target:     "foo.com",
+			Targets:    endpoint.Targets{"foo.com"},
 			RecordType: endpoint.RecordTypeCNAME,
 		},
 		{
 			DNSName:    "cbc.com",
-			Target:     "foo.com",
+			Targets:    endpoint.Targets{"foo.com"},
 			RecordType: "CNAME",
 			RecordTTL:  endpoint.TTL(60),
 		},

plan/conflict.go

@@ -64,7 +64,7 @@ func (s PerResource) ResolveUpdate(current *endpoint.Endpoint, candidates []*end
 
 // less returns true if endpoint x is less than y
 func (s PerResource) less(x, y *endpoint.Endpoint) bool {
-	return x.Target < y.Target
+	return x.Targets.IsLess(y.Targets)
 }
 
 // TODO: with cross-resource/cross-cluster setup alternative variations of ConflictResolver can be used

plan/conflict_test.go

@@ -45,63 +45,63 @@ func (suite *ResolverSuite) SetupTest() {
 	// initialize endpoints used in tests
 	suite.fooV1Cname = &endpoint.Endpoint{
 		DNSName:    "foo",
-		Target:     "v1",
+		Targets:    endpoint.Targets{"v1"},
 		RecordType: "CNAME",
 		Labels: map[string]string{
 			endpoint.ResourceLabelKey: "ingress/default/foo-v1",
 		},
 	}
 	suite.fooV2Cname = &endpoint.Endpoint{
 		DNSName:    "foo",
-		Target:     "v2",
+		Targets:    endpoint.Targets{"v2"},
 		RecordType: "CNAME",
 		Labels: map[string]string{
 			endpoint.ResourceLabelKey: "ingress/default/foo-v2",
 		},
 	}
 	suite.fooV2CnameDuplicate = &endpoint.Endpoint{
 		DNSName:    "foo",
-		Target:     "v2",
+		Targets:    endpoint.Targets{"v2"},
 		RecordType: "CNAME",
 		Labels: map[string]string{
 			endpoint.ResourceLabelKey: "ingress/default/foo-v2-duplicate",
 		},
 	}
 	suite.fooA5 = &endpoint.Endpoint{
 		DNSName:    "foo",
-		Target:     "5.5.5.5",
+		Targets:    endpoint.Targets{"5.5.5.5"},
 		RecordType: "A",
 		Labels: map[string]string{
 			endpoint.ResourceLabelKey: "ingress/default/foo-5",
 		},
 	}
 	suite.bar127A = &endpoint.Endpoint{
 		DNSName:    "bar",
-		Target:     "127.0.0.1",
+		Targets:    endpoint.Targets{"127.0.0.1"},
 		RecordType: "A",
 		Labels: map[string]string{
 			endpoint.ResourceLabelKey: "ingress/default/bar-127",
 		},
 	}
 	suite.bar127AAnother = &endpoint.Endpoint{ //TODO: remove this once we move to multiple targets under same endpoint
 		DNSName:    "bar",
-		Target:     "8.8.8.8",
+		Targets:    endpoint.Targets{"8.8.8.8"},
 		RecordType: "A",
 		Labels: map[string]string{
 			endpoint.ResourceLabelKey: "ingress/default/bar-127",
 		},
 	}
 	suite.bar192A = &endpoint.Endpoint{
 		DNSName:    "bar",
-		Target:     "192.168.0.1",
+		Targets:    endpoint.Targets{"192.168.0.1"},
 		RecordType: "A",
 		Labels: map[string]string{
 			endpoint.ResourceLabelKey: "ingress/default/bar-192",
 		},
 	}
 	suite.legacyBar192A = &endpoint.Endpoint{
 		DNSName:    "bar",
-		Target:     "192.168.0.1",
+		Targets:    endpoint.Targets{"192.168.0.1"},
 		RecordType: "A",
 	}
 }
@@ -120,7 +120,7 @@ func (suite *ResolverSuite) TestStrictResolver() {
 	// should actually get the updated record (note ttl is different)
 	newFooV1Cname := &endpoint.Endpoint{
 		DNSName:    suite.fooV1Cname.DNSName,
-		Target:     suite.fooV1Cname.Target,
+		Targets:    suite.fooV1Cname.Targets,
 		Labels:     suite.fooV1Cname.Labels,
 		RecordType: suite.fooV1Cname.RecordType,
 		RecordTTL:  suite.fooV1Cname.RecordTTL + 1, // ttl is different

plan/plan.go

@@ -166,7 +166,7 @@ func inheritOwner(from, to *endpoint.Endpoint) {
 }
 
 func targetChanged(desired, current *endpoint.Endpoint) bool {
-	return desired.Target != current.Target
+	return !desired.Targets.Same(current.Targets)
 }
 
 func shouldUpdateTTL(desired, current *endpoint.Endpoint) bool {