diff --git a/controller/controller.go b/controller/controller.go index 689fc9ef9d..83df2f45c2 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -188,11 +188,33 @@ func (c *Controller) RunOnce(ctx context.Context) error { verifiedARecords.Set(float64(len(vRecords))) endpoints = c.Registry.AdjustEndpoints(endpoints) + if len(missingRecords) > 0 { + // Add missing records before the actual plan is applied. + // This prevents the problems when the missing TXT record needs to be + // created and deleted/upserted in the same batch. + missingRecordsPlan := &plan.Plan{ + Policies: []plan.Policy{c.Policy}, + Missing: missingRecords, + DomainFilter: endpoint.MatchAllDomainFilters{c.DomainFilter, c.Registry.GetDomainFilter()}, + PropertyComparator: c.Registry.PropertyValuesEqual, + ManagedRecords: c.ManagedRecordTypes, + } + missingRecordsPlan = missingRecordsPlan.Calculate() + if missingRecordsPlan.Changes.HasChanges() { + err = c.Registry.ApplyChanges(ctx, missingRecordsPlan.Changes) + if err != nil { + registryErrorsTotal.Inc() + deprecatedRegistryErrors.Inc() + return err + } + log.Info("All missing records are created") + } + } + plan := &plan.Plan{ Policies: []plan.Policy{c.Policy}, Current: records, Desired: endpoints, - Missing: missingRecords, DomainFilter: endpoint.MatchAllDomainFilters{c.DomainFilter, c.Registry.GetDomainFilter()}, PropertyComparator: c.Registry.PropertyValuesEqual, ManagedRecords: c.ManagedRecordTypes, diff --git a/controller/controller_test.go b/controller/controller_test.go index 27d01777c2..b18f07e629 100644 --- a/controller/controller_test.go +++ b/controller/controller_test.go @@ -270,6 +270,51 @@ func testControllerFiltersDomains(t *testing.T, configuredEndpoints []*endpoint. } } +type noopRegistryWithMissing struct { + *registry.NoopRegistry + missingRecords []*endpoint.Endpoint +} + +func (r *noopRegistryWithMissing) MissingRecords() []*endpoint.Endpoint { + return r.missingRecords +} + +func testControllerFiltersDomainsWithMissing(t *testing.T, configuredEndpoints []*endpoint.Endpoint, domainFilter endpoint.DomainFilterInterface, providerEndpoints, missingEndpoints []*endpoint.Endpoint, expectedChanges []*plan.Changes) { + t.Helper() + cfg := externaldns.NewConfig() + cfg.ManagedDNSRecordTypes = []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME} + + source := new(testutils.MockSource) + source.On("Endpoints").Return(configuredEndpoints, nil) + + // Fake some existing records in our DNS provider and validate some desired changes. + provider := &filteredMockProvider{ + RecordsStore: providerEndpoints, + } + noop, err := registry.NewNoopRegistry(provider) + require.NoError(t, err) + + r := &noopRegistryWithMissing{ + NoopRegistry: noop, + missingRecords: missingEndpoints, + } + + ctrl := &Controller{ + Source: source, + Registry: r, + Policy: &plan.SyncPolicy{}, + DomainFilter: domainFilter, + ManagedRecordTypes: cfg.ManagedDNSRecordTypes, + } + + assert.NoError(t, ctrl.RunOnce(context.Background())) + assert.Equal(t, 1, provider.RecordsCallCount) + require.Len(t, provider.ApplyChangesCalls, len(expectedChanges)) + for i, change := range expectedChanges { + assert.Equal(t, *change, *provider.ApplyChangesCalls[i]) + } +} + func TestControllerSkipsEmptyChanges(t *testing.T) { testControllerFiltersDomains( t, @@ -517,3 +562,57 @@ func TestARecords(t *testing.T) { assert.Equal(t, math.Float64bits(2), valueFromMetric(sourceARecords)) assert.Equal(t, math.Float64bits(1), valueFromMetric(registryARecords)) } + +// TestMissingRecordsApply validates that the missing records result in the dedicated plan apply. +func TestMissingRecordsApply(t *testing.T) { + testControllerFiltersDomainsWithMissing( + t, + []*endpoint.Endpoint{ + { + DNSName: "record1.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"1.2.3.4"}, + }, + { + DNSName: "record2.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + }, + endpoint.NewDomainFilter([]string{"used.tld"}), + []*endpoint.Endpoint{ + { + DNSName: "record1.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"1.2.3.4"}, + }, + }, + []*endpoint.Endpoint{ + { + DNSName: "a-record1.used.tld", + RecordType: endpoint.RecordTypeTXT, + Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""}, + }, + }, + []*plan.Changes{ + // Missing record had its own plan applied. + { + Create: []*endpoint.Endpoint{ + { + DNSName: "a-record1.used.tld", + RecordType: endpoint.RecordTypeTXT, + Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""}, + }, + }, + }, + { + Create: []*endpoint.Endpoint{ + { + DNSName: "record2.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + }, + }, + }) +} diff --git a/plan/plan.go b/plan/plan.go index f95b268b1a..d166764521 100644 --- a/plan/plan.go +++ b/plan/plan.go @@ -257,7 +257,7 @@ func filterRecordsForPlan(records []*endpoint.Endpoint, domainFilter endpoint.Do log.Debugf("ignoring record %s that does not match domain filter", record.DNSName) continue } - if isManagedRecord(record.RecordType, managedRecords) { + if IsManagedRecord(record.RecordType, managedRecords) { filtered = append(filtered, record) } } @@ -300,7 +300,7 @@ func CompareBoolean(defaultValue bool, name, current, previous string) bool { return v1 == v2 } -func isManagedRecord(record string, managedRecords []string) bool { +func IsManagedRecord(record string, managedRecords []string) bool { for _, r := range managedRecords { if record == r { return true diff --git a/registry/txt.go b/registry/txt.go index 725319f167..2e0c33a3c2 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -144,16 +144,24 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error ep.Labels[k] = v } } - // Handle the migration of TXT records created before the new format (introduced in v0.12.0) - if len(txtRecordsMap) > 0 { - if isManagedRecord(ep, im.managedRecordTypes) { - // get desired TXT records and detect the missing ones - for _, desiredTXT := range im.generateTXTRecord(ep) { + + // Handle the migration of TXT records created before the new format (introduced in v0.12.0). + // The migration is done for the TXT records owned by this instance only. + if len(txtRecordsMap) > 0 && ep.Labels[endpoint.OwnerLabelKey] == im.ownerID { + if plan.IsManagedRecord(ep.RecordType, im.managedRecordTypes) { + // Get desired TXT records and detect the missing ones + desiredTXTs := im.generateTXTRecord(ep) + missingDesiredTXTs := []*endpoint.Endpoint{} + for _, desiredTXT := range desiredTXTs { if _, exists := txtRecordsMap[desiredTXT.DNSName]; !exists { - // add missing TXT record - missingEndpoints = append(missingEndpoints, desiredTXT) + missingDesiredTXTs = append(missingDesiredTXTs, desiredTXT) } } + if len(desiredTXTs) > len(missingDesiredTXTs) { + // Add missing TXT records only if those are managed (by externaldns) ones. + // The unmanaged record has both of the desired TXT records missing. + missingEndpoints = append(missingEndpoints, missingDesiredTXTs...) + } } } } @@ -178,8 +186,8 @@ func (im *TXTRegistry) MissingRecords() []*endpoint.Endpoint { // generateTXTRecord generates both "old" and "new" TXT records. // Once we decide to drop old format we need to drop toTXTName() and rename toNewTXTName func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpoint { - // missing TXT records are added to the set of changes, - // obviously we don't need any other TXT record for them + // Missing TXT records are added to the set of changes. + // Obviously, we don't need any other TXT record for them. if r.RecordType == endpoint.RecordTypeTXT { return nil } @@ -431,12 +439,3 @@ func (im *TXTRegistry) removeFromCache(ep *endpoint.Endpoint) { } } } - -func isManagedRecord(record *endpoint.Endpoint, managedTypes []string) bool { - for _, mt := range managedTypes { - if record.RecordType == mt { - return true - } - } - return false -} diff --git a/registry/txt_test.go b/registry/txt_test.go index 75e88185dc..27d7d66a5e 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -786,6 +786,10 @@ func testTXTRegistryMissingRecordsNoPrefix(t *testing.T) { newEndpointWithOwner("ns-newformat.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), newEndpointWithOwner("newformat.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), newEndpointWithOwner("noheritage.test-zone.example.org", "random", endpoint.RecordTypeTXT, ""), + newEndpointWithOwner("oldformat-otherowner.test-zone.example.org", "bar.loadbalancer.com", endpoint.RecordTypeA, ""), + newEndpointWithOwner("oldformat-otherowner.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=otherowner\"", endpoint.RecordTypeTXT, ""), + endpoint.NewEndpoint("unmanaged1.test-zone.example.org", endpoint.RecordTypeA, "unmanaged1.loadbalancer.com"), + endpoint.NewEndpoint("unmanaged2.test-zone.example.org", endpoint.RecordTypeCNAME, "unmanaged2.loadbalancer.com"), }, }) expectedRecords := []*endpoint.Endpoint{ @@ -814,6 +818,7 @@ func testTXTRegistryMissingRecordsNoPrefix(t *testing.T) { endpoint.OwnerLabelKey: "owner", }, }, + // Only TXT records with the wrong heritage are returned by Records() { DNSName: "noheritage.test-zone.example.org", Targets: endpoint.Targets{"random"}, @@ -823,6 +828,25 @@ func testTXTRegistryMissingRecordsNoPrefix(t *testing.T) { endpoint.OwnerLabelKey: "", }, }, + { + DNSName: "oldformat-otherowner.test-zone.example.org", + Targets: endpoint.Targets{"bar.loadbalancer.com"}, + RecordType: endpoint.RecordTypeA, + Labels: map[string]string{ + // Records() retrieves all the records of the zone, no matter the owner + endpoint.OwnerLabelKey: "otherowner", + }, + }, + { + DNSName: "unmanaged1.test-zone.example.org", + Targets: endpoint.Targets{"unmanaged1.loadbalancer.com"}, + RecordType: endpoint.RecordTypeA, + }, + { + DNSName: "unmanaged2.test-zone.example.org", + Targets: endpoint.Targets{"unmanaged2.loadbalancer.com"}, + RecordType: endpoint.RecordTypeCNAME, + }, } expectedMissingRecords := []*endpoint.Endpoint{ @@ -861,6 +885,10 @@ func testTXTRegistryMissingRecordsWithPrefix(t *testing.T) { newEndpointWithOwner("txt.ns-newformat.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), newEndpointWithOwner("txt.newformat.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), newEndpointWithOwner("noheritage.test-zone.example.org", "random", endpoint.RecordTypeTXT, ""), + newEndpointWithOwner("oldformat-otherowner.test-zone.example.org", "bar.loadbalancer.com", endpoint.RecordTypeA, ""), + newEndpointWithOwner("txt.oldformat-otherowner.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=otherowner\"", endpoint.RecordTypeTXT, ""), + endpoint.NewEndpoint("unmanaged1.test-zone.example.org", endpoint.RecordTypeA, "unmanaged1.loadbalancer.com"), + endpoint.NewEndpoint("unmanaged2.test-zone.example.org", endpoint.RecordTypeCNAME, "unmanaged2.loadbalancer.com"), }, }) expectedRecords := []*endpoint.Endpoint{ @@ -898,6 +926,25 @@ func testTXTRegistryMissingRecordsWithPrefix(t *testing.T) { endpoint.OwnerLabelKey: "", }, }, + { + DNSName: "oldformat-otherowner.test-zone.example.org", + Targets: endpoint.Targets{"bar.loadbalancer.com"}, + RecordType: endpoint.RecordTypeA, + Labels: map[string]string{ + // All the records of the zone are retrieved, no matter the owner + endpoint.OwnerLabelKey: "otherowner", + }, + }, + { + DNSName: "unmanaged1.test-zone.example.org", + Targets: endpoint.Targets{"unmanaged1.loadbalancer.com"}, + RecordType: endpoint.RecordTypeA, + }, + { + DNSName: "unmanaged2.test-zone.example.org", + Targets: endpoint.Targets{"unmanaged2.loadbalancer.com"}, + RecordType: endpoint.RecordTypeCNAME, + }, } expectedMissingRecords := []*endpoint.Endpoint{