Skip to content

Commit

Permalink
Merge pull request #45 from alebedev87/carry-patch-handle-missing-txt…
Browse files Browse the repository at this point in the history 
…-record-final-version

Handle the migration to the new TXT format. Catch up to the merged version
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot committed Jul 28, 2022
2 parents 7511ef2 + 1f63ddf commit 906ba04
Show file tree
Hide file tree
Showing 5 changed files with 188 additions and 21 deletions.
24 changes: 23 additions & 1 deletion controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
99 changes: 99 additions & 0 deletions controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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"},
},
},
},
})
}
4 changes: 2 additions & 2 deletions plan/plan.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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
Expand Down
35 changes: 17 additions & 18 deletions registry/txt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
}
}
}
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
47 changes: 47 additions & 0 deletions registry/txt_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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"},
Expand All @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down

0 comments on commit 906ba04

Please sign in to comment.