Fix planning for multi-cluster dual stack record types

When AAAA multi-target / dual stack support was
added via kubernetes-sigs#2461 it broke ownership of domains across
different clusters with different ingress records types.

For example if 2 clusters manage the same zone,
1 cluster uses A records and the other uses CNAME
records, when each record type is treated as a separate
planning record, it will cause ownership to bounce back
and forth and records to be constantly created and
deleted.

This change introduces a new configuration point that
which by default will only cause AAAA records to be
managed independently of A and CNAME records.
This should preserve the dual stack / multi-target use
case while preserving ownership of across different
clusters. This configuration can be overridden using
the new option `--dual-stack-record-types`. For
example to revert back to the current behavior where
all record types are treated as independent the
configuration would be:

`--dual-stack-record-types AAAA --dual-stack-record-types A --dual-stack-record-types CNAME`

controller/controller.go

@@ -182,6 +182,8 @@ type Controller struct {
 	nextRunAtMux sync.Mutex
 	// DNS record types that will be considered for management
 	ManagedRecordTypes []string
+	// DualStackRecords records types to treat as separate planning rows
+	DualStackRecords []string
 	// MinEventSyncInterval is used as window for batching events
 	MinEventSyncInterval time.Duration
 }
@@ -223,6 +225,7 @@ func (c *Controller) RunOnce(ctx context.Context) error {
 		DomainFilter:       endpoint.MatchAllDomainFilters{c.DomainFilter, c.Registry.GetDomainFilter()},
 		PropertyComparator: c.Registry.PropertyValuesEqual,
 		ManagedRecords:     c.ManagedRecordTypes,
+		DualStackRecords:   c.DualStackRecords,
 	}
 
 	plan = plan.Calculate()

main.go

@@ -429,6 +429,7 @@ func main() {
 		Interval:             cfg.Interval,
 		DomainFilter:         domainFilter,
 		ManagedRecordTypes:   cfg.ManagedDNSRecordTypes,
+		DualStackRecords:     cfg.DualStackRecordTypes,
 		MinEventSyncInterval: cfg.MinEventSyncInterval,
 	}
 

pkg/apis/externaldns/types.go

@@ -193,6 +193,7 @@ type Config struct {
 	TransIPPrivateKeyFile              string
 	DigitalOceanAPIPageSize            int
 	ManagedDNSRecordTypes              []string
+	DualStackRecordTypes               []string
 	GoDaddyAPIKey                      string `secure:"yes"`
 	GoDaddySecretKey                   string `secure:"yes"`
 	GoDaddyTTL                         int64
@@ -341,6 +342,7 @@ var defaultConfig = &Config{
 	TransIPPrivateKeyFile:       "",
 	DigitalOceanAPIPageSize:     50,
 	ManagedDNSRecordTypes:       []string{endpoint.RecordTypeA, endpoint.RecordTypeAAAA, endpoint.RecordTypeCNAME},
+	DualStackRecordTypes:        []string{endpoint.RecordTypeAAAA},
 	GoDaddyAPIKey:               "",
 	GoDaddySecretKey:            "",
 	GoDaddyTTL:                  600,
@@ -440,6 +442,7 @@ func (cfg *Config) ParseFlags(args []string) error {
 	app.Flag("crd-source-kind", "Kind of the CRD for the crd source in API group and version specified by crd-source-apiversion").Default(defaultConfig.CRDSourceKind).StringVar(&cfg.CRDSourceKind)
 	app.Flag("service-type-filter", "The service types to take care about (default: all, expected: ClusterIP, NodePort, LoadBalancer or ExternalName)").StringsVar(&cfg.ServiceTypeFilter)
 	app.Flag("managed-record-types", "Record types to manage; specify multiple times to include many; (default: A, AAAA, CNAME) (supported records: CNAME, A, AAAA, NS").Default("A", "AAAA", "CNAME").StringsVar(&cfg.ManagedDNSRecordTypes)
+	app.Flag("dual-stack-record-types", "Dual stack record types to allow independent management of; set empty string to disable. (default: AAAA) (supported records: A, AAAA, CNAME").Default().StringsVar(&cfg.DualStackRecordTypes)
 	app.Flag("default-targets", "Set globally default IP address that will apply as a target instead of source addresses. Specify multiple times for multiple targets (optional)").StringsVar(&cfg.DefaultTargets)
 	app.Flag("target-net-filter", "Limit possible targets by a net filter; specify multiple times for multiple possible nets (optional)").StringsVar(&cfg.TargetNetFilter)
 	app.Flag("exclude-target-net", "Exclude target nets (optional)").StringsVar(&cfg.ExcludeTargetNets)

plan/plan.go

@@ -48,6 +48,8 @@ type Plan struct {
 	PropertyComparator PropertyComparator
 	// DNS record types that will be considered for management
 	ManagedRecords []string
+	// DualStackRecords are treated as different endpoints. Typically, this is just AAAA.
+	DualStackRecords []string
 }
 
 // Changes holds lists of actions to be executed by dns providers
@@ -85,10 +87,12 @@ bar.com |                | [->191.1.1.1, ->190.1.1.1]  |  = create (bar.com -> 1
 type planTable struct {
 	rows     map[planKey]*planTableRow
 	resolver ConflictResolver
+	// rowRecordTypes is a list of record types to manage independently of other types of the same dns name
+	rowRecordTypes []string
 }
 
-func newPlanTable() planTable { // TODO: make resolver configurable
-	return planTable{map[planKey]*planTableRow{}, PerResource{}}
+func newPlanTable(rowRecordTypes []string) planTable { // TODO: make resolver configurable
+	return planTable{map[planKey]*planTableRow{}, PerResource{}, rowRecordTypes}
 }
 
 // planTableRow
@@ -117,8 +121,24 @@ func (t *planTable) newPlanKey(e *endpoint.Endpoint) planKey {
 	key := planKey{
 		dnsName:       normalizeDNSName(e.DNSName),
 		setIdentifier: e.SetIdentifier,
-		recordType:    e.RecordType,
 	}
+
+	// Only include record type if the record should be treated as a separate planning row.
+	// In environments where the same zone is managed by multiple clusters with different ingress
+	// record type could result in the external dns controllers fighting over ownership if each
+	// host record is treated as independent. For example where 1 cluster uses A records as the
+	// bound address and another uses CNAME. If the same ingress host is deployed
+	// to each cluster they will continually create and remove each other's record. By treating
+	// A and CNAME records as the "same" endpoint only the original owning cluster will continue
+	// to managed records as the UpdateNew and UpdateOld records will be filtered out during the
+	// Registry.ApplyChanges phase since the UpdateNew endpoint will inherit the ownership label
+	// of the existing record.
+	for _, rt := range t.rowRecordTypes {
+		if rt == e.RecordType {
+			key.recordType = e.RecordType
+		}
+	}
+
 	if _, ok := t.rows[key]; !ok {
 		t.rows[key] = &planTableRow{}
 	}
@@ -136,7 +156,7 @@ func (c *Changes) HasChanges() bool {
 // state. It then passes those changes to the current policy for further
 // processing. It returns a copy of Plan with the changes populated.
 func (p *Plan) Calculate() *Plan {
-	t := newPlanTable()
+	t := newPlanTable(p.DualStackRecords)
 
 	if p.DomainFilter == nil {
 		p.DomainFilter = endpoint.MatchAllDomainFilters(nil)

plan/plan_test.go

@@ -444,7 +444,7 @@ func (suite *PlanTestSuite) TestIdempotency() {
 	validateEntries(suite.T(), changes.Delete, expectedDelete)
 }
 
-func (suite *PlanTestSuite) TestDifferentTypes() {
+func (suite *PlanTestSuite) TestDifferentTypesWithCustomDualStackRecords() {
 	current := []*endpoint.Endpoint{suite.fooV1Cname}
 	desired := []*endpoint.Endpoint{suite.fooV2Cname, suite.fooA5}
 	expectedCreate := []*endpoint.Endpoint{suite.fooA5}
@@ -453,10 +453,34 @@ func (suite *PlanTestSuite) TestDifferentTypes() {
 	expectedDelete := []*endpoint.Endpoint{}
 
 	p := &Plan{
-		Policies:       []Policy{&SyncPolicy{}},
-		Current:        current,
-		Desired:        desired,
-		ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME},
+		Policies:         []Policy{&SyncPolicy{}},
+		Current:          current,
+		Desired:          desired,
+		ManagedRecords:   []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME},
+		DualStackRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME},
+	}
+
+	changes := p.Calculate().Changes
+	validateEntries(suite.T(), changes.Create, expectedCreate)
+	validateEntries(suite.T(), changes.UpdateNew, expectedUpdateNew)
+	validateEntries(suite.T(), changes.UpdateOld, expectedUpdateOld)
+	validateEntries(suite.T(), changes.Delete, expectedDelete)
+}
+
+func (suite *PlanTestSuite) TestDifferentTypesWithAAAADualStackRecords() {
+	current := []*endpoint.Endpoint{suite.fooA5}
+	desired := []*endpoint.Endpoint{suite.fooV2Cname}
+	expectedCreate := []*endpoint.Endpoint{}
+	expectedUpdateOld := []*endpoint.Endpoint{suite.fooA5}
+	expectedUpdateNew := []*endpoint.Endpoint{suite.fooV2Cname}
+	expectedDelete := []*endpoint.Endpoint{}
+
+	p := &Plan{
+		Policies:         []Policy{&SyncPolicy{}},
+		Current:          current,
+		Desired:          desired,
+		ManagedRecords:   []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME},
+		DualStackRecords: []string{endpoint.RecordTypeAAAA},
 	}
 
 	changes := p.Calculate().Changes
@@ -666,10 +690,11 @@ func (suite *PlanTestSuite) TestDualStackRecords() {
 	expectedCreate := []*endpoint.Endpoint{suite.dsA, suite.dsAAAA}
 
 	p := &Plan{
-		Policies:       []Policy{&SyncPolicy{}},
-		Current:        current,
-		Desired:        desired,
-		ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeAAAA, endpoint.RecordTypeCNAME},
+		Policies:         []Policy{&SyncPolicy{}},
+		Current:          current,
+		Desired:          desired,
+		ManagedRecords:   []string{endpoint.RecordTypeA, endpoint.RecordTypeAAAA, endpoint.RecordTypeCNAME},
+		DualStackRecords: []string{endpoint.RecordTypeAAAA},
 	}
 
 	changes := p.Calculate().Changes

registry/txt_test.go

@@ -1306,6 +1306,69 @@ func TestFailGenerateTXT(t *testing.T) {
 	assert.Equal(t, expectedTXT, gotTXT)
 }
 
+// TestMultiClusterDifferentRecordTypeOwnership validates the registry handles environments where the same zone is managed by
+// external-dns in different clusters and the ingress record type is different. For example one uses A records and the other
+// uses CNAME. In this environment the first cluster that establishes the owner record should maintain ownership even
+// if the same ingress host is deployed to the other. With the introduction of Dual Record support each record type
+// was treated independently and would cause each cluster to fight over ownership. This tests ensure that the default
+// Dual Stack record support only treats AAAA records independently and while keeping A and CNAME record ownership intact.
+func TestMultiClusterDifferentRecordTypeOwnership(t *testing.T) {
+	ctx := context.Background()
+	p := inmemory.NewInMemoryProvider()
+	p.CreateZone(testZone)
+	p.ApplyChanges(ctx, &plan.Changes{
+		Create: []*endpoint.Endpoint{
+			// records on cluster using A record for ingress address
+			newEndpointWithOwner("bar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=cat,external-dns/resource=ingress/default/foo\"", endpoint.RecordTypeTXT, ""),
+			newEndpointWithOwner("bar.test-zone.example.org", "1.2.3.4", endpoint.RecordTypeA, ""),
+		},
+	})
+
+	r, _ := NewTXTRegistry(p, "_owner.", "", "bar", time.Hour, "", []string{}, false, nil)
+	records, _ := r.Records(ctx)
+
+	// new cluster has same ingress host as other cluster and uses CNAME ingress address
+	cname := &endpoint.Endpoint{
+		DNSName:    "bar.test-zone.example.org",
+		Targets:    endpoint.Targets{"cluster-b"},
+		RecordType: "CNAME",
+		Labels: map[string]string{
+			endpoint.ResourceLabelKey: "ingress/default/foo-127",
+		},
+	}
+	desired := []*endpoint.Endpoint{cname}
+
+	pl := &plan.Plan{
+		Policies:         []plan.Policy{&plan.SyncPolicy{}},
+		Current:          records,
+		Desired:          desired,
+		ManagedRecords:   []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME},
+		DualStackRecords: []string{endpoint.RecordTypeAAAA},
+	}
+
+	changes := pl.Calculate()
+	p.OnApplyChanges = func(ctx context.Context, changes *plan.Changes) {
+		got := map[string][]*endpoint.Endpoint{
+			"Create":    changes.Create,
+			"UpdateNew": changes.UpdateNew,
+			"UpdateOld": changes.UpdateOld,
+			"Delete":    changes.Delete,
+		}
+		expected := map[string][]*endpoint.Endpoint{
+			"Create":    {},
+			"UpdateNew": {},
+			"UpdateOld": {},
+			"Delete":    {},
+		}
+		testutils.SamePlanChanges(got, expected)
+	}
+
+	err := r.ApplyChanges(ctx, changes.Changes)
+	if err != nil {
+		t.Error(err)
+	}
+}
+
 /**
 
 helper methods