rfc2136: merge Endpoints with same name/type into single Endpoint

map multiple DNS records with the same name/type into the external-dns which models such records as a single Endpoint with multiple entries in the Targets attribute. Solution: Generalize the solution introduced to the DigitalOcean provider by #1595 by refactoring the mergeEndpointsByNameType function into the endpoint module. Then use that function in the RFC2136 provider. Authored-by: Tom Dyas <[email protected]> Signed-off-by: Bogdan Dobrelya <[email protected]>
kubernetes-sigs · Jul 15, 2024 · 8cba8a0 · 8cba8a0
1 parent c45d0de
commit 8cba8a0
Show file tree

Hide file tree

Showing 6 changed files with 73 additions and 67 deletions.
diff --git a/charts/external-dns/CHANGELOG.md b/charts/external-dns/CHANGELOG.md
@@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Added support for `extraContainers` argument. ([#4432](https://github.com/kubernetes-sigs/external-dns/pull/4432)) _@omerap12_
 - Added support for setting `excludeDomains` argument.  ([#4380](https://github.com/kubernetes-sigs/external-dns/pull/4380)) _@bford-evs_
+- Fixed reconciling `A` records that have more than 1 target for `rfc2136` provider. ([4613#](https://github.com/kubernetes-sigs/external-dns/pull/4613)) _@tdyas_
 
 ### Changed
 

diff --git a/endpoint/endpoint.go b/endpoint/endpoint.go
@@ -376,6 +376,39 @@ type DNSEndpointList struct {
 	Items           []DNSEndpoint `json:"items"`
 }
 
+// Given a slice of Endpoint, merge those Endpoints with the same Name and Type into a single Endpoint
+// with multiple Targets. Returns a slice of Endpoint with the merged Endpoints.
+func MergeEndpointsByNameType(endpoints []*Endpoint) []*Endpoint {
+	endpointsByNameType := map[string][]*Endpoint{}
+
+	for _, e := range endpoints {
+		key := fmt.Sprintf("%s-%s", e.DNSName, e.RecordType)
+		endpointsByNameType[key] = append(endpointsByNameType[key], e)
+	}
+
+	// If no merge occurred, just return the existing endpoints.
+	if len(endpointsByNameType) == len(endpoints) {
+		return endpoints
+	}
+
+	// Otherwise, construct a new list of endpoints with the endpoints merged.
+	var result []*Endpoint
+	for _, endpoints := range endpointsByNameType {
+		dnsName := endpoints[0].DNSName
+		recordType := endpoints[0].RecordType
+
+		var targets Targets
+		for _, e := range endpoints {
+			targets = append(targets, e.Targets...)
+		}
+
+		e := NewEndpoint(dnsName, recordType, targets...)
+		result = append(result, e)
+	}
+
+	return result
+}
+
 // RemoveDuplicates returns a slice holding the unique endpoints.
 // This function doesn't contemplate the Targets of an Endpoint
 // as part of the primary Key

diff --git a/endpoint/endpoint_test.go b/endpoint/endpoint_test.go
@@ -17,8 +17,11 @@ limitations under the License.
 package endpoint
 
 import (
+	"sort"
 	"reflect"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestNewEndpoint(t *testing.T) {
@@ -114,6 +117,39 @@ func TestSameFailures(t *testing.T) {
 	}
 }
 
+func TestMergeRecordsByNameType(t *testing.T) {
+	xs := []*Endpoint{
+		NewEndpoint("foo.example.com", "A", "1.2.3.4"),
+		NewEndpoint("bar.example.com", "A", "1.2.3.4"),
+		NewEndpoint("foo.example.com", "A", "5.6.7.8"),
+		NewEndpoint("foo.example.com", "CNAME", "somewhere.out.there.com"),
+	}
+
+	merged := MergeEndpointsByNameType(xs)
+
+	assert.Equal(t, 3, len(merged))
+	sort.SliceStable(merged, func(i, j int) bool {
+		if merged[i].DNSName != merged[j].DNSName {
+			return merged[i].DNSName < merged[j].DNSName
+		}
+		return merged[i].RecordType < merged[j].RecordType
+	})
+	assert.Equal(t, "bar.example.com", merged[0].DNSName)
+	assert.Equal(t, "A", merged[0].RecordType)
+	assert.Equal(t, 1, len(merged[0].Targets))
+	assert.Equal(t, "1.2.3.4", merged[0].Targets[0])
+
+	assert.Equal(t, "foo.example.com", merged[1].DNSName)
+	assert.Equal(t, "A", merged[1].RecordType)
+	assert.Equal(t, 2, len(merged[1].Targets))
+	assert.ElementsMatch(t, []string{"1.2.3.4", "5.6.7.8"}, merged[1].Targets)
+
+	assert.Equal(t, "foo.example.com", merged[2].DNSName)
+	assert.Equal(t, "CNAME", merged[2].RecordType)
+	assert.Equal(t, 1, len(merged[2].Targets))
+	assert.Equal(t, "somewhere.out.there.com", merged[2].Targets[0])
+}
+
 func TestIsLess(t *testing.T) {
 	testsA := []Targets{
 		{""},

diff --git a/provider/digitalocean/digital_ocean.go b/provider/digitalocean/digital_ocean.go
@@ -116,38 +116,6 @@ func (p *DigitalOceanProvider) Zones(ctx context.Context) ([]godo.Domain, error)
 	return result, nil
 }
 
-// Merge Endpoints with the same Name and Type into a single endpoint with multiple Targets.
-func mergeEndpointsByNameType(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
-	endpointsByNameType := map[string][]*endpoint.Endpoint{}
-
-	for _, e := range endpoints {
-		key := fmt.Sprintf("%s-%s", e.DNSName, e.RecordType)
-		endpointsByNameType[key] = append(endpointsByNameType[key], e)
-	}
-
-	// If no merge occurred, just return the existing endpoints.
-	if len(endpointsByNameType) == len(endpoints) {
-		return endpoints
-	}
-
-	// Otherwise, construct a new list of endpoints with the endpoints merged.
-	var result []*endpoint.Endpoint
-	for _, endpoints := range endpointsByNameType {
-		dnsName := endpoints[0].DNSName
-		recordType := endpoints[0].RecordType
-
-		targets := make([]string, len(endpoints))
-		for i, e := range endpoints {
-			targets[i] = e.Targets[0]
-		}
-
-		e := endpoint.NewEndpoint(dnsName, recordType, targets...)
-		result = append(result, e)
-	}
-
-	return result
-}
-
 // Records returns the list of records in a given zone.
 func (p *DigitalOceanProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, error) {
 	zones, err := p.Zones(ctx)
@@ -181,7 +149,7 @@ func (p *DigitalOceanProvider) Records(ctx context.Context) ([]*endpoint.Endpoin
 
 	// Merge endpoints with the same name and type (e.g., multiple A records for a single
 	// DNS name) into one endpoint with multiple targets.
-	endpoints = mergeEndpointsByNameType(endpoints)
+	endpoints = endpoint.MergeEndpointsByNameType(endpoints)
 
 	// Log the endpoints that were found.
 	log.WithFields(log.Fields{

diff --git a/provider/digitalocean/digital_ocean_test.go b/provider/digitalocean/digital_ocean_test.go
@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"os"
 	"reflect"
-	"sort"
 	"testing"
 
 	"github.com/digitalocean/godo"
@@ -671,36 +670,3 @@ func TestDigitalOceanAllRecords(t *testing.T) {
 		t.Errorf("expected to fail, %s", err)
 	}
 }
-
-func TestDigitalOceanMergeRecordsByNameType(t *testing.T) {
-	xs := []*endpoint.Endpoint{
-		endpoint.NewEndpoint("foo.example.com", "A", "1.2.3.4"),
-		endpoint.NewEndpoint("bar.example.com", "A", "1.2.3.4"),
-		endpoint.NewEndpoint("foo.example.com", "A", "5.6.7.8"),
-		endpoint.NewEndpoint("foo.example.com", "CNAME", "somewhere.out.there.com"),
-	}
-
-	merged := mergeEndpointsByNameType(xs)
-
-	assert.Equal(t, 3, len(merged))
-	sort.SliceStable(merged, func(i, j int) bool {
-		if merged[i].DNSName != merged[j].DNSName {
-			return merged[i].DNSName < merged[j].DNSName
-		}
-		return merged[i].RecordType < merged[j].RecordType
-	})
-	assert.Equal(t, "bar.example.com", merged[0].DNSName)
-	assert.Equal(t, "A", merged[0].RecordType)
-	assert.Equal(t, 1, len(merged[0].Targets))
-	assert.Equal(t, "1.2.3.4", merged[0].Targets[0])
-
-	assert.Equal(t, "foo.example.com", merged[1].DNSName)
-	assert.Equal(t, "A", merged[1].RecordType)
-	assert.Equal(t, 2, len(merged[1].Targets))
-	assert.ElementsMatch(t, []string{"1.2.3.4", "5.6.7.8"}, merged[1].Targets)
-
-	assert.Equal(t, "foo.example.com", merged[2].DNSName)
-	assert.Equal(t, "CNAME", merged[2].RecordType)
-	assert.Equal(t, 1, len(merged[2].Targets))
-	assert.Equal(t, "somewhere.out.there.com", merged[2].Targets[0])
-}
diff --git a/provider/rfc2136/rfc2136.go b/provider/rfc2136/rfc2136.go
@@ -221,6 +221,8 @@ OuterLoop:
 		eps = append(eps, ep)
 	}
 
+	eps = endpoint.MergeEndpointsByNameType(eps)
+
 	return eps, nil
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -221,6 +221,8 @@ OuterLoop: @@
     		eps = append(eps, ep)
     	}
+    	eps = endpoint.MergeEndpointsByNameType(eps)
     	return eps, nil
     }
@@ Expand Down @@