Merge pull request #3635 from giantswarm/handle-missing-txt-records

Handle migration to the new TXT format: use ForceUpdate strategy

controller/controller.go

@@ -195,8 +195,6 @@ func (c *Controller) RunOnce(ctx context.Context) error {
 		return err
 	}
 
-	missingRecords := c.Registry.MissingRecords()
-
 	registryEndpointsTotal.Set(float64(len(records)))
 	regARecords, regAAAARecords := countAddressRecords(records)
 	registryARecords.Set(float64(regARecords))
@@ -218,29 +216,6 @@ func (c *Controller) RunOnce(ctx context.Context) error {
 	verifiedAAAARecords.Set(float64(vAAAARecords))
 	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,

controller/controller_test.go

@@ -311,51 +311,6 @@ 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,
@@ -683,60 +638,6 @@ func TestARecords(t *testing.T) {
 	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"},
-					},
-				},
-			},
-		})
-}
-
 func TestAAAARecords(t *testing.T) {
 	testControllerFiltersDomains(
 		t,

plan/plan.go

@@ -37,8 +37,6 @@ type Plan struct {
 	Current []*endpoint.Endpoint
 	// List of desired records
 	Desired []*endpoint.Endpoint
-	// List of missing records to be created, use for the migrations (e.g. old-new TXT format)
-	Missing []*endpoint.Endpoint
 	// Policies under which the desired changes are calculated
 	Policies []Policy
 	// List of changes necessary to move towards desired state
@@ -177,11 +175,6 @@ func (p *Plan) Calculate() *Plan {
 		changes = pol.Apply(changes)
 	}
 
-	// Handle the migration of the TXT records created before the new format (introduced in v0.12.0)
-	if len(p.Missing) > 0 {
-		changes.Create = append(changes.Create, filterRecordsForPlan(p.Missing, p.DomainFilter, append(p.ManagedRecords, endpoint.RecordTypeTXT))...)
-	}
-
 	plan := &Plan{
 		Current:        p.Current,
 		Desired:        p.Desired,

plan/plan_test.go

@@ -51,9 +51,6 @@ type PlanTestSuite struct {
 	domainFilterFiltered2            *endpoint.Endpoint
 	domainFilterFiltered3            *endpoint.Endpoint
 	domainFilterExcluded             *endpoint.Endpoint
-	domainFilterFilteredTXT1         *endpoint.Endpoint
-	domainFilterFilteredTXT2         *endpoint.Endpoint
-	domainFilterExcludedTXT          *endpoint.Endpoint
 }
 
 func (suite *PlanTestSuite) SetupTest() {
@@ -233,21 +230,6 @@ func (suite *PlanTestSuite) SetupTest() {
 		Targets:    endpoint.Targets{"1.1.1.1"},
 		RecordType: "A",
 	}
-	suite.domainFilterFilteredTXT1 = &endpoint.Endpoint{
-		DNSName:    "a-foo.domain.tld",
-		Targets:    endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
-		RecordType: "TXT",
-	}
-	suite.domainFilterFilteredTXT2 = &endpoint.Endpoint{
-		DNSName:    "cname-bar.domain.tld",
-		Targets:    endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
-		RecordType: "TXT",
-	}
-	suite.domainFilterExcludedTXT = &endpoint.Endpoint{
-		DNSName:    "cname-bar.otherdomain.tld",
-		Targets:    endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
-		RecordType: "TXT",
-	}
 }
 
 func (suite *PlanTestSuite) TestSyncFirstRound() {
@@ -661,21 +643,6 @@ func (suite *PlanTestSuite) TestDomainFiltersUpdate() {
 	validateEntries(suite.T(), changes.Delete, expectedDelete)
 }
 
-func (suite *PlanTestSuite) TestMissing() {
-	missing := []*endpoint.Endpoint{suite.domainFilterFilteredTXT1, suite.domainFilterFilteredTXT2, suite.domainFilterExcludedTXT}
-	expectedCreate := []*endpoint.Endpoint{suite.domainFilterFilteredTXT1, suite.domainFilterFilteredTXT2}
-
-	p := &Plan{
-		Policies:       []Policy{&SyncPolicy{}},
-		Missing:        missing,
-		DomainFilter:   endpoint.NewDomainFilter([]string{"domain.tld"}),
-		ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME},
-	}
-
-	changes := p.Calculate().Changes
-	validateEntries(suite.T(), changes.Create, expectedCreate)
-}
-
 func (suite *PlanTestSuite) TestAAAARecords() {
 
 	current := []*endpoint.Endpoint{}

registry/aws_sd_registry.go

@@ -67,11 +67,6 @@ func (sdr *AWSSDRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, er
 	return records, nil
 }
 
-// MissingRecords returns nil because there is no missing records for AWSSD registry
-func (sdr *AWSSDRegistry) MissingRecords() []*endpoint.Endpoint {
-	return nil
-}
-
 // ApplyChanges filters out records not owned the External-DNS, additionally it adds the required label
 // inserted in the AWS SD instance as a CreateID field
 func (sdr *AWSSDRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) error {

registry/noop.go

@@ -45,11 +45,6 @@ func (im *NoopRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, erro
 	return im.provider.Records(ctx)
 }
 
-// MissingRecords returns nil because there is no missing records for Noop registry
-func (im *NoopRegistry) MissingRecords() []*endpoint.Endpoint {
-	return nil
-}
-
 // ApplyChanges propagates changes to the dns provider
 func (im *NoopRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) error {
 	return im.provider.ApplyChanges(ctx, changes)

registry/registry.go

@@ -35,7 +35,6 @@ type Registry interface {
 	PropertyValuesEqual(attribute string, previous string, current string) bool
 	AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint
 	GetDomainFilter() endpoint.DomainFilterInterface
-	MissingRecords() []*endpoint.Endpoint
 }
 
 // TODO(ideahitme): consider moving this to Plan

registry/txt.go

@@ -30,7 +30,10 @@ import (
 	"sigs.k8s.io/external-dns/provider"
 )
 
-const recordTemplate = "%{record_type}"
+const (
+	recordTemplate              = "%{record_type}"
+	providerSpecificForceUpdate = "txt/force-update"
+)
 
 // TXTRegistry implements registry interface with ownership implemented via associated TXT records
 type TXTRegistry struct {
@@ -50,9 +53,6 @@ type TXTRegistry struct {
 
 	managedRecordTypes []string
 
-	// missingTXTRecords stores TXT records which are missing after the migration to the new format
-	missingTXTRecords []*endpoint.Endpoint
-
 	// encrypt text records
 	txtEncryptEnabled bool
 	txtEncryptAESKey  []byte
@@ -117,7 +117,6 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
 	}
 
 	endpoints := []*endpoint.Endpoint{}
-	missingEndpoints := []*endpoint.Endpoint{}
 
 	labelMap := map[string]endpoint.Labels{}
 	txtRecordsMap := map[string]struct{}{}
@@ -174,17 +173,11 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
 			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 {
-						missingDesiredTXTs = append(missingDesiredTXTs, desiredTXT)
+						ep.WithProviderSpecific(providerSpecificForceUpdate, "true")
 					}
 				}
-				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...)
-				}
 			}
 		}
 	}
@@ -195,17 +188,9 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
 		im.recordsCacheRefreshTime = time.Now()
 	}
 
-	im.missingTXTRecords = missingEndpoints
-
 	return endpoints, nil
 }
 
-// MissingRecords returns the TXT record to be created.
-// The missing records are collected during the run of Records method.
-func (im *TXTRegistry) MissingRecords() []*endpoint.Endpoint {
-	return im.missingTXTRecords
-}
-
 // 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 {

registry/txt_test.go

@@ -875,6 +875,12 @@ func testTXTRegistryMissingRecordsNoPrefix(t *testing.T) {
 				// owner was added from the TXT record's target
 				endpoint.OwnerLabelKey: "owner",
 			},
+			ProviderSpecific: []endpoint.ProviderSpecificProperty{
+				{
+					Name:  "txt/force-update",
+					Value: "true",
+				},
+			},
 		},
 		{
 			DNSName:    "oldformat2.test-zone.example.org",
@@ -883,6 +889,12 @@ func testTXTRegistryMissingRecordsNoPrefix(t *testing.T) {
 			Labels: map[string]string{
 				endpoint.OwnerLabelKey: "owner",
 			},
+			ProviderSpecific: []endpoint.ProviderSpecificProperty{
+				{
+					Name:  "txt/force-update",
+					Value: "true",
+				},
+			},
 		},
 		{
 			DNSName:    "newformat.test-zone.example.org",
@@ -931,32 +943,10 @@ func testTXTRegistryMissingRecordsNoPrefix(t *testing.T) {
 		},
 	}
 
-	expectedMissingRecords := []*endpoint.Endpoint{
-		{
-			DNSName: "cname-oldformat.test-zone.example.org",
-			// owner is taken from the source record (A, CNAME, etc.)
-			Targets:    endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
-			RecordType: endpoint.RecordTypeTXT,
-			Labels: endpoint.Labels{
-				endpoint.OwnedRecordLabelKey: "oldformat.test-zone.example.org",
-			},
-		},
-		{
-			DNSName:    "a-oldformat2.test-zone.example.org",
-			Targets:    endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
-			RecordType: endpoint.RecordTypeTXT,
-			Labels: endpoint.Labels{
-				endpoint.OwnedRecordLabelKey: "oldformat2.test-zone.example.org",
-			},
-		},
-	}
-
 	r, _ := NewTXTRegistry(p, "", "", "owner", time.Hour, "wc", []string{endpoint.RecordTypeCNAME, endpoint.RecordTypeA, endpoint.RecordTypeNS}, false, nil)
 	records, _ := r.Records(ctx)
-	missingRecords := r.MissingRecords()
 
 	assert.True(t, testutils.SameEndpoints(records, expectedRecords))
-	assert.True(t, testutils.SameEndpoints(missingRecords, expectedMissingRecords))
 }
 
 func testTXTRegistryMissingRecordsWithPrefix(t *testing.T) {
@@ -988,6 +978,12 @@ func testTXTRegistryMissingRecordsWithPrefix(t *testing.T) {
 				// owner was added from the TXT record's target
 				endpoint.OwnerLabelKey: "owner",
 			},
+			ProviderSpecific: []endpoint.ProviderSpecificProperty{
+				{
+					Name:  "txt/force-update",
+					Value: "true",
+				},
+			},
 		},
 		{
 			DNSName:    "oldformat2.test-zone.example.org",
@@ -996,6 +992,12 @@ func testTXTRegistryMissingRecordsWithPrefix(t *testing.T) {
 			Labels: map[string]string{
 				endpoint.OwnerLabelKey: "owner",
 			},
+			ProviderSpecific: []endpoint.ProviderSpecificProperty{
+				{
+					Name:  "txt/force-update",
+					Value: "true",
+				},
+			},
 		},
 		{
 			DNSName:    "newformat.test-zone.example.org",
@@ -1035,32 +1037,10 @@ func testTXTRegistryMissingRecordsWithPrefix(t *testing.T) {
 		},
 	}
 
-	expectedMissingRecords := []*endpoint.Endpoint{
-		{
-			DNSName: "txt.cname-oldformat.test-zone.example.org",
-			// owner is taken from the source record (A, CNAME, etc.)
-			Targets:    endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
-			RecordType: endpoint.RecordTypeTXT,
-			Labels: endpoint.Labels{
-				endpoint.OwnedRecordLabelKey: "oldformat.test-zone.example.org",
-			},
-		},
-		{
-			DNSName:    "txt.a-oldformat2.test-zone.example.org",
-			Targets:    endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""},
-			RecordType: endpoint.RecordTypeTXT,
-			Labels: endpoint.Labels{
-				endpoint.OwnedRecordLabelKey: "oldformat2.test-zone.example.org",
-			},
-		},
-	}
-
 	r, _ := NewTXTRegistry(p, "txt.", "", "owner", time.Hour, "wc", []string{endpoint.RecordTypeCNAME, endpoint.RecordTypeA, endpoint.RecordTypeNS}, false, nil)
 	records, _ := r.Records(ctx)
-	missingRecords := r.MissingRecords()
 
 	assert.True(t, testutils.SameEndpoints(records, expectedRecords))
-	assert.True(t, testutils.SameEndpoints(missingRecords, expectedMissingRecords))
 }
 
 func TestCacheMethods(t *testing.T) {