From adf6ad753f8046838795025263036ecba640bdcb Mon Sep 17 00:00:00 2001 From: Alfred Krohmer Date: Mon, 16 Jan 2023 15:57:10 +0100 Subject: [PATCH 1/2] Route53: wrap route53.Change in preparation of retry mechanism --- provider/aws/aws.go | 68 ++++++++++----- provider/aws/aws_test.go | 184 +++++++++++++++++++++++---------------- 2 files changed, 154 insertions(+), 98 deletions(-) diff --git a/provider/aws/aws.go b/provider/aws/aws.go index b1b5e6aa59..9e7ac0a534 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -136,6 +136,22 @@ type Route53API interface { ListTagsForResourceWithContext(ctx context.Context, input *route53.ListTagsForResourceInput, opts ...request.Option) (*route53.ListTagsForResourceOutput, error) } +// wrapper to handle ownership relation throughout the provider implementation +type Route53Change struct { + route53.Change + OwnedRecord string +} + +type Route53Changes []*Route53Change + +func (cs Route53Changes) Route53Changes() []*route53.Change { + ret := []*route53.Change{} + for _, c := range cs { + ret = append(ret, &c.Change) + } + return ret +} + type zonesListCache struct { age time.Time duration time.Duration @@ -467,7 +483,7 @@ func (p *AWSProvider) requiresDeleteCreate(old *endpoint.Endpoint, new *endpoint return false } -func (p *AWSProvider) createUpdateChanges(newEndpoints, oldEndpoints []*endpoint.Endpoint) []*route53.Change { +func (p *AWSProvider) createUpdateChanges(newEndpoints, oldEndpoints []*endpoint.Endpoint) Route53Changes { var deletes []*endpoint.Endpoint var creates []*endpoint.Endpoint var updates []*endpoint.Endpoint @@ -483,7 +499,7 @@ func (p *AWSProvider) createUpdateChanges(newEndpoints, oldEndpoints []*endpoint } } - combined := make([]*route53.Change, 0, len(deletes)+len(creates)+len(updates)) + combined := make(Route53Changes, 0, len(deletes)+len(creates)+len(updates)) combined = append(combined, p.newChanges(route53.ChangeActionCreate, creates)...) combined = append(combined, p.newChanges(route53.ChangeActionUpsert, updates)...) combined = append(combined, p.newChanges(route53.ChangeActionDelete, deletes)...) @@ -514,7 +530,7 @@ func (p *AWSProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) e updateChanges := p.createUpdateChanges(changes.UpdateNew, changes.UpdateOld) - combinedChanges := make([]*route53.Change, 0, len(changes.Delete)+len(changes.Create)+len(updateChanges)) + combinedChanges := make(Route53Changes, 0, len(changes.Delete)+len(changes.Create)+len(updateChanges)) combinedChanges = append(combinedChanges, p.newChanges(route53.ChangeActionCreate, changes.Create)...) combinedChanges = append(combinedChanges, p.newChanges(route53.ChangeActionDelete, changes.Delete)...) combinedChanges = append(combinedChanges, updateChanges...) @@ -523,7 +539,7 @@ func (p *AWSProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) e } // submitChanges takes a zone and a collection of Changes and sends them as a single transaction. -func (p *AWSProvider) submitChanges(ctx context.Context, changes []*route53.Change, zones map[string]*route53.HostedZone) error { +func (p *AWSProvider) submitChanges(ctx context.Context, changes Route53Changes, zones map[string]*route53.HostedZone) error { // return early if there is nothing to change if len(changes) == 0 { log.Info("All records are already up to date") @@ -551,7 +567,7 @@ func (p *AWSProvider) submitChanges(ctx context.Context, changes []*route53.Chan params := &route53.ChangeResourceRecordSetsInput{ HostedZoneId: aws.String(z), ChangeBatch: &route53.ChangeBatch{ - Changes: b, + Changes: b.Route53Changes(), }, } @@ -583,8 +599,8 @@ func (p *AWSProvider) submitChanges(ctx context.Context, changes []*route53.Chan } // newChanges returns a collection of Changes based on the given records and action. -func (p *AWSProvider) newChanges(action string, endpoints []*endpoint.Endpoint) []*route53.Change { - changes := make([]*route53.Change, 0, len(endpoints)) +func (p *AWSProvider) newChanges(action string, endpoints []*endpoint.Endpoint) Route53Changes { + changes := make(Route53Changes, 0, len(endpoints)) for _, endpoint := range endpoints { change, dualstack := p.newChange(action, endpoint) @@ -592,7 +608,7 @@ func (p *AWSProvider) newChanges(action string, endpoints []*endpoint.Endpoint) if dualstack { // make a copy of change, modify RRS type to AAAA, then add new change rrs := *change.ResourceRecordSet - change2 := &route53.Change{Action: change.Action, ResourceRecordSet: &rrs} + change2 := &Route53Change{Change: route53.Change{Action: change.Action, ResourceRecordSet: &rrs}} change2.ResourceRecordSet.Type = aws.String(route53.RRTypeAaaa) changes = append(changes, change2) } @@ -635,11 +651,13 @@ func (p *AWSProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoin // returned Change is based on the given record by the given action, e.g. // action=ChangeActionCreate returns a change for creation of the record and // action=ChangeActionDelete returns a change for deletion of the record. -func (p *AWSProvider) newChange(action string, ep *endpoint.Endpoint) (*route53.Change, bool) { - change := &route53.Change{ - Action: aws.String(action), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String(ep.DNSName), +func (p *AWSProvider) newChange(action string, ep *endpoint.Endpoint) (*Route53Change, bool) { + change := &Route53Change{ + Change: route53.Change{ + Action: aws.String(action), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(ep.DNSName), + }, }, } dualstack := false @@ -736,15 +754,15 @@ func (p *AWSProvider) tagsForZone(ctx context.Context, zoneID string) (map[strin return tagMap, nil } -func batchChangeSet(cs []*route53.Change, batchSize int) [][]*route53.Change { +func batchChangeSet(cs Route53Changes, batchSize int) []Route53Changes { if len(cs) <= batchSize { res := sortChangesByActionNameType(cs) - return [][]*route53.Change{res} + return []Route53Changes{res} } - batchChanges := make([][]*route53.Change, 0) + batchChanges := make([]Route53Changes, 0) - changesByName := make(map[string][]*route53.Change) + changesByName := make(map[string]Route53Changes) for _, v := range cs { changesByName[*v.ResourceRecordSet.Name] = append(changesByName[*v.ResourceRecordSet.Name], v) } @@ -784,7 +802,7 @@ func batchChangeSet(cs []*route53.Change, batchSize int) [][]*route53.Change { return batchChanges } -func sortChangesByActionNameType(cs []*route53.Change) []*route53.Change { +func sortChangesByActionNameType(cs Route53Changes) Route53Changes { sort.SliceStable(cs, func(i, j int) bool { if *cs[i].Action > *cs[j].Action { return true @@ -805,11 +823,11 @@ func sortChangesByActionNameType(cs []*route53.Change) []*route53.Change { } // changesByZone separates a multi-zone change into a single change per zone. -func changesByZone(zones map[string]*route53.HostedZone, changeSet []*route53.Change) map[string][]*route53.Change { - changes := make(map[string][]*route53.Change) +func changesByZone(zones map[string]*route53.HostedZone, changeSet Route53Changes) map[string]Route53Changes { + changes := make(map[string]Route53Changes) for _, z := range zones { - changes[aws.StringValue(z.Id)] = []*route53.Change{} + changes[aws.StringValue(z.Id)] = Route53Changes{} } for _, c := range changeSet { @@ -828,9 +846,11 @@ func changesByZone(zones map[string]*route53.HostedZone, changeSet []*route53.Ch aliasTarget := *rrset.AliasTarget aliasTarget.HostedZoneId = aws.String(cleanZoneID(aws.StringValue(z.Id))) rrset.AliasTarget = &aliasTarget - c = &route53.Change{ - Action: c.Action, - ResourceRecordSet: &rrset, + c = &Route53Change{ + Change: route53.Change{ + Action: c.Action, + ResourceRecordSet: &rrset, + }, } } changes[aws.StringValue(z.Id)] = append(changes[aws.StringValue(z.Id)], c) diff --git a/provider/aws/aws_test.go b/provider/aws/aws_test.go index 58e18f353e..a339159fbd 100644 --- a/provider/aws/aws_test.go +++ b/provider/aws/aws_test.go @@ -688,29 +688,37 @@ func TestAWSApplyChangesDryRun(t *testing.T) { } func TestAWSChangesByZones(t *testing.T) { - changes := []*route53.Change{ + changes := Route53Changes{ { - Action: aws.String(route53.ChangeActionCreate), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String("qux.foo.example.org"), TTL: aws.Int64(1), + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String("qux.foo.example.org"), TTL: aws.Int64(1), + }, }, }, { - Action: aws.String(route53.ChangeActionCreate), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String("qux.bar.example.org"), TTL: aws.Int64(2), + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String("qux.bar.example.org"), TTL: aws.Int64(2), + }, }, }, { - Action: aws.String(route53.ChangeActionDelete), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String("wambo.foo.example.org"), TTL: aws.Int64(10), + Change: route53.Change{ + Action: aws.String(route53.ChangeActionDelete), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String("wambo.foo.example.org"), TTL: aws.Int64(10), + }, }, }, { - Action: aws.String(route53.ChangeActionDelete), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String("wambo.bar.example.org"), TTL: aws.Int64(20), + Change: route53.Change{ + Action: aws.String(route53.ChangeActionDelete), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String("wambo.bar.example.org"), TTL: aws.Int64(20), + }, }, }, } @@ -738,47 +746,59 @@ func TestAWSChangesByZones(t *testing.T) { changesByZone := changesByZone(zones, changes) require.Len(t, changesByZone, 3) - validateAWSChangeRecords(t, changesByZone["foo-example-org"], []*route53.Change{ + validateAWSChangeRecords(t, changesByZone["foo-example-org"], Route53Changes{ { - Action: aws.String(route53.ChangeActionCreate), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String("qux.foo.example.org"), TTL: aws.Int64(1), + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String("qux.foo.example.org"), TTL: aws.Int64(1), + }, }, }, { - Action: aws.String(route53.ChangeActionDelete), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String("wambo.foo.example.org"), TTL: aws.Int64(10), + Change: route53.Change{ + Action: aws.String(route53.ChangeActionDelete), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String("wambo.foo.example.org"), TTL: aws.Int64(10), + }, }, }, }) - validateAWSChangeRecords(t, changesByZone["bar-example-org"], []*route53.Change{ + validateAWSChangeRecords(t, changesByZone["bar-example-org"], Route53Changes{ { - Action: aws.String(route53.ChangeActionCreate), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String("qux.bar.example.org"), TTL: aws.Int64(2), + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String("qux.bar.example.org"), TTL: aws.Int64(2), + }, }, }, { - Action: aws.String(route53.ChangeActionDelete), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String("wambo.bar.example.org"), TTL: aws.Int64(20), + Change: route53.Change{ + Action: aws.String(route53.ChangeActionDelete), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String("wambo.bar.example.org"), TTL: aws.Int64(20), + }, }, }, }) - validateAWSChangeRecords(t, changesByZone["bar-example-org-private"], []*route53.Change{ + validateAWSChangeRecords(t, changesByZone["bar-example-org-private"], Route53Changes{ { - Action: aws.String(route53.ChangeActionCreate), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String("qux.bar.example.org"), TTL: aws.Int64(2), + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String("qux.bar.example.org"), TTL: aws.Int64(2), + }, }, }, { - Action: aws.String(route53.ChangeActionDelete), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String("wambo.bar.example.org"), TTL: aws.Int64(20), + Change: route53.Change{ + Action: aws.String(route53.ChangeActionDelete), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String("wambo.bar.example.org"), TTL: aws.Int64(20), + }, }, }, }) @@ -802,7 +822,7 @@ func TestAWSsubmitChanges(t *testing.T) { ctx := context.Background() zones, _ := provider.Zones(ctx) records, _ := provider.Records(ctx) - cs := make([]*route53.Change, 0, len(endpoints)) + cs := make(Route53Changes, 0, len(endpoints)) cs = append(cs, provider.newChanges(route53.ChangeActionCreate, endpoints)...) require.NoError(t, provider.submitChanges(ctx, cs, zones)) @@ -828,21 +848,25 @@ func TestAWSsubmitChangesError(t *testing.T) { } func TestAWSBatchChangeSet(t *testing.T) { - var cs []*route53.Change + var cs Route53Changes for i := 1; i <= defaultBatchChangeSize; i += 2 { - cs = append(cs, &route53.Change{ - Action: aws.String(route53.ChangeActionCreate), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String(fmt.Sprintf("host-%d", i)), - Type: aws.String("A"), + cs = append(cs, &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("A"), + }, }, }) - cs = append(cs, &route53.Change{ - Action: aws.String(route53.ChangeActionCreate), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String(fmt.Sprintf("host-%d", i)), - Type: aws.String("TXT"), + cs = append(cs, &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("TXT"), + }, }, }) } @@ -856,27 +880,33 @@ func TestAWSBatchChangeSet(t *testing.T) { } func TestAWSBatchChangeSetExceeding(t *testing.T) { - var cs []*route53.Change + var cs Route53Changes const testCount = 50 const testLimit = 11 const expectedBatchCount = 5 const expectedChangesCount = 10 for i := 1; i <= testCount; i += 2 { - cs = append(cs, &route53.Change{ - Action: aws.String(route53.ChangeActionCreate), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String(fmt.Sprintf("host-%d", i)), - Type: aws.String("A"), + cs = append(cs, + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("A"), + }, + }, }, - }) - cs = append(cs, &route53.Change{ - Action: aws.String(route53.ChangeActionCreate), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String(fmt.Sprintf("host-%d", i)), - Type: aws.String("TXT"), + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("TXT"), + }, + }, }, - }) + ) } batchCs := batchChangeSet(cs, testLimit) @@ -890,25 +920,31 @@ func TestAWSBatchChangeSetExceeding(t *testing.T) { } func TestAWSBatchChangeSetExceedingNameChange(t *testing.T) { - var cs []*route53.Change + var cs Route53Changes const testCount = 10 const testLimit = 1 for i := 1; i <= testCount; i += 2 { - cs = append(cs, &route53.Change{ - Action: aws.String(route53.ChangeActionCreate), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String(fmt.Sprintf("host-%d", i)), - Type: aws.String("A"), + cs = append(cs, + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("A"), + }, + }, }, - }) - cs = append(cs, &route53.Change{ - Action: aws.String(route53.ChangeActionCreate), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String(fmt.Sprintf("host-%d", i)), - Type: aws.String("TXT"), + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("TXT"), + }, + }, }, - }) + ) } batchCs := batchChangeSet(cs, testLimit) @@ -933,7 +969,7 @@ func validateAWSZone(t *testing.T, zone *route53.HostedZone, expected *route53.H assert.Equal(t, aws.StringValue(expected.Name), aws.StringValue(zone.Name)) } -func validateAWSChangeRecords(t *testing.T, records []*route53.Change, expected []*route53.Change) { +func validateAWSChangeRecords(t *testing.T, records Route53Changes, expected Route53Changes) { require.Len(t, records, len(expected)) for i := range records { @@ -941,7 +977,7 @@ func validateAWSChangeRecords(t *testing.T, records []*route53.Change, expected } } -func validateAWSChangeRecord(t *testing.T, record *route53.Change, expected *route53.Change) { +func validateAWSChangeRecord(t *testing.T, record *Route53Change, expected *Route53Change) { assert.Equal(t, aws.StringValue(expected.Action), aws.StringValue(record.Action)) assert.Equal(t, aws.StringValue(expected.ResourceRecordSet.Name), aws.StringValue(record.ResourceRecordSet.Name)) assert.Equal(t, aws.StringValue(expected.ResourceRecordSet.Type), aws.StringValue(record.ResourceRecordSet.Type)) From 7dd84a589d4725ccf25d94f8d71b0146fee4bfcc Mon Sep 17 00:00:00 2001 From: Alfred Krohmer Date: Mon, 16 Jan 2023 16:03:16 +0100 Subject: [PATCH 2/2] Route53: retry single changes in a batch if the batch fails If a single change fails during the retry, it will be added to a queue. In the next iteration, changes from this queue will be submitted after all other changes. When submitting single changes, they are always submitted as batches of changes with the same DNS name and ownership relation to avoid inconsistency between the record created and the TXT records. --- endpoint/labels.go | 2 + internal/testutils/endpoint.go | 1 + provider/aws/aws.go | 131 ++++++++++++++++++++++++-------- provider/aws/aws_test.go | 69 +++++++++++++++++ registry/txt.go | 2 + registry/txt_test.go | 132 +++++++++++++++++++-------------- 6 files changed, 252 insertions(+), 85 deletions(-) diff --git a/endpoint/labels.go b/endpoint/labels.go index 544eb3e7a5..797190db41 100644 --- a/endpoint/labels.go +++ b/endpoint/labels.go @@ -32,6 +32,8 @@ const ( OwnerLabelKey = "owner" // ResourceLabelKey is the name of the label that identifies k8s resource which wants to acquire the DNS name ResourceLabelKey = "resource" + // OwnedRecordLabelKey is the name of the label that identifies the record that is owned by the labeled TXT registry record + OwnedRecordLabelKey = "ownedRecord" // AWSSDDescriptionLabel label responsible for storing raw owner/resource combination information in the Labels // supposed to be inserted by AWS SD Provider, and parsed into OwnerLabelKey and ResourceLabelKey key by AWS SD Registry diff --git a/internal/testutils/endpoint.go b/internal/testutils/endpoint.go index 033b2e03de..f8b4657958 100644 --- a/internal/testutils/endpoint.go +++ b/internal/testutils/endpoint.go @@ -62,6 +62,7 @@ func SameEndpoint(a, b *endpoint.Endpoint) bool { return a.DNSName == b.DNSName && a.Targets.Same(b.Targets) && a.RecordType == b.RecordType && a.SetIdentifier == b.SetIdentifier && a.Labels[endpoint.OwnerLabelKey] == b.Labels[endpoint.OwnerLabelKey] && a.RecordTTL == b.RecordTTL && a.Labels[endpoint.ResourceLabelKey] == b.Labels[endpoint.ResourceLabelKey] && + a.Labels[endpoint.OwnedRecordLabelKey] == b.Labels[endpoint.OwnedRecordLabelKey] && SameProviderSpecific(a.ProviderSpecific, b.ProviderSpecific) } diff --git a/provider/aws/aws.go b/provider/aws/aws.go index 9e7ac0a534..b50930d0cb 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -176,6 +176,8 @@ type AWSProvider struct { zoneTagFilter provider.ZoneTagFilter preferCNAME bool zonesCache *zonesListCache + // queue for collecting changes to submit them in the next iteration, but after all other changes + failedChangesQueue map[string]Route53Changes } // AWSConfig contains configuration to create a new AWS provider. @@ -240,6 +242,7 @@ func NewAWSProvider(awsConfig AWSConfig) (*AWSProvider, error) { preferCNAME: awsConfig.PreferCNAME, dryRun: awsConfig.DryRun, zonesCache: &zonesListCache{duration: awsConfig.ZoneCacheDuration}, + failedChangesQueue: make(map[string]Route53Changes), } return provider, nil @@ -556,9 +559,16 @@ func (p *AWSProvider) submitChanges(ctx context.Context, changes Route53Changes, for z, cs := range changesByZone { var failedUpdate bool - batchCs := batchChangeSet(cs, p.batchChangeSize) + // group changes into new changes and into changes that failed in a previous iteration and are retried + retriedChanges, newChanges := findChangesInQueue(cs, p.failedChangesQueue[z]) + p.failedChangesQueue[z] = nil + batchCs := append(batchChangeSet(newChanges, p.batchChangeSize), batchChangeSet(retriedChanges, p.batchChangeSize)...) for i, b := range batchCs { + if len(b) == 0 { + continue + } + for _, c := range b { log.Infof("Desired change: %s %s %s [Id: %s]", *c.Action, *c.ResourceRecordSet.Name, *c.ResourceRecordSet.Type, z) } @@ -571,13 +581,41 @@ func (p *AWSProvider) submitChanges(ctx context.Context, changes Route53Changes, }, } + successfulChanges := 0 + if _, err := p.client.ChangeResourceRecordSetsWithContext(ctx, params); err != nil { - log.Errorf("Failure in zone %s [Id: %s]", aws.StringValue(zones[z].Name), z) - log.Error(err) // TODO(ideahitme): consider changing the interface in cases when this error might be a concern for other components - failedUpdate = true + log.Errorf("Failure in zone %s [Id: %s] when submitting change batch: %v", aws.StringValue(zones[z].Name), z, err) + + changesByOwnership := groupChangesByNameAndOwnershipRelation(b) + + if len(changesByOwnership) > 1 { + log.Debug("Trying to submit change sets one-by-one instead") + + for _, changes := range changesByOwnership { + for _, c := range changes { + log.Debugf("Desired change: %s %s %s [Id: %s]", *c.Action, *c.ResourceRecordSet.Name, *c.ResourceRecordSet.Type, z) + } + params.ChangeBatch = &route53.ChangeBatch{ + Changes: changes.Route53Changes(), + } + if _, err := p.client.ChangeResourceRecordSetsWithContext(ctx, params); err != nil { + failedUpdate = true + log.Errorf("Failed submitting change (error: %v), it will be retried in a separate change batch in the next iteration", err) + p.failedChangesQueue[z] = append(p.failedChangesQueue[z], changes...) + } else { + successfulChanges = successfulChanges + len(changes) + } + } + } else { + failedUpdate = true + } } else { + successfulChanges = len(b) + } + + if successfulChanges > 0 { // z is the R53 Hosted Zone ID already as aws.StringValue - log.Infof("%d record(s) in zone %s [Id: %s] were successfully updated", len(b), aws.StringValue(zones[z].Name), z) + log.Infof("%d record(s) in zone %s [Id: %s] were successfully updated", successfulChanges, aws.StringValue(zones[z].Name), z) } if i != len(batchCs)-1 { @@ -736,9 +774,51 @@ func (p *AWSProvider) newChange(action string, ep *endpoint.Endpoint) (*Route53C change.ResourceRecordSet.HealthCheckId = aws.String(prop.Value) } + if ownedRecord, ok := ep.Labels[endpoint.OwnedRecordLabelKey]; ok { + change.OwnedRecord = ownedRecord + } + return change, dualstack } +// searches for `changes` that are contained in `queue` and returns the `changes` separated by whether they were found in the queue (`foundChanges`) or not (`notFoundChanges`) +func findChangesInQueue(changes Route53Changes, queue Route53Changes) (foundChanges, notFoundChanges Route53Changes) { + if queue == nil { + return Route53Changes{}, changes + } + + for _, c := range changes { + found := false + for _, qc := range queue { + if c == qc { + foundChanges = append(foundChanges, c) + found = true + break + } + } + if !found { + notFoundChanges = append(notFoundChanges, c) + } + } + + return +} + +// group the given changes by name and ownership relation to ensure these are always submitted in the same transaction to Route53; +// grouping by name is done to always submit changes with the same name but different set identifier together, +// grouping by ownership relation is done to always submit changes of records and e.g. their corresponding TXT registry records together +func groupChangesByNameAndOwnershipRelation(cs Route53Changes) map[string]Route53Changes { + changesByOwnership := make(map[string]Route53Changes) + for _, v := range cs { + key := v.OwnedRecord + if key == "" { + key = aws.StringValue(v.ResourceRecordSet.Name) + } + changesByOwnership[key] = append(changesByOwnership[key], v) + } + return changesByOwnership +} + func (p *AWSProvider) tagsForZone(ctx context.Context, zoneID string) (map[string]string, error) { response, err := p.client.ListTagsForResourceWithContext(ctx, &route53.ListTagsForResourceInput{ ResourceType: aws.String("hostedzone"), @@ -762,41 +842,34 @@ func batchChangeSet(cs Route53Changes, batchSize int) []Route53Changes { batchChanges := make([]Route53Changes, 0) - changesByName := make(map[string]Route53Changes) - for _, v := range cs { - changesByName[*v.ResourceRecordSet.Name] = append(changesByName[*v.ResourceRecordSet.Name], v) - } + changesByOwnership := groupChangesByNameAndOwnershipRelation(cs) names := make([]string, 0) - for v := range changesByName { + for v := range changesByOwnership { names = append(names, v) } sort.Strings(names) - for _, name := range names { - totalChangesByName := len(changesByName[name]) - - if totalChangesByName > batchSize { - log.Warnf("Total changes for %s exceeds max batch size of %d, total changes: %d", name, - batchSize, totalChangesByName) + currentBatch := Route53Changes{} + for k, name := range names { + v := changesByOwnership[name] + if len(v) > batchSize { + log.Warnf("Total changes for %v exceeds max batch size of %d, total changes: %d; changes will not be performed", k, batchSize, len(v)) continue } - var existingBatch bool - for i, b := range batchChanges { - if len(b)+totalChangesByName <= batchSize { - batchChanges[i] = append(batchChanges[i], changesByName[name]...) - existingBatch = true - break - } - } - if !existingBatch { - batchChanges = append(batchChanges, changesByName[name]) + if len(currentBatch)+len(v) > batchSize { + // currentBatch would be too large if we add this changeset; + // add currentBatch to batchChanges and start a new currentBatch + batchChanges = append(batchChanges, sortChangesByActionNameType(currentBatch)) + currentBatch = append(Route53Changes{}, v...) + } else { + currentBatch = append(currentBatch, v...) } } - - for i, batch := range batchChanges { - batchChanges[i] = sortChangesByActionNameType(batch) + if len(currentBatch) > 0 { + // add final currentBatch + batchChanges = append(batchChanges, sortChangesByActionNameType(currentBatch)) } return batchChanges diff --git a/provider/aws/aws_test.go b/provider/aws/aws_test.go index a339159fbd..280842bea7 100644 --- a/provider/aws/aws_test.go +++ b/provider/aws/aws_test.go @@ -847,6 +847,65 @@ func TestAWSsubmitChangesError(t *testing.T) { require.Error(t, provider.submitChanges(ctx, cs, zones)) } +func TestAWSsubmitChangesRetryOnError(t *testing.T) { + provider, clientStub := newAWSProvider(t, endpoint.NewDomainFilter([]string{"ext-dns-test-2.teapot.zalan.do."}), provider.NewZoneIDFilter([]string{}), provider.NewZoneTypeFilter(""), defaultEvaluateTargetHealth, false, []*endpoint.Endpoint{}) + + ctx := context.Background() + zones, err := provider.Zones(ctx) + require.NoError(t, err) + + ep1 := endpoint.NewEndpointWithTTL("success.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "1.0.0.1") + ep2 := endpoint.NewEndpointWithTTL("fail.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "1.0.0.2") + ep3 := endpoint.NewEndpointWithTTL("success2.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "1.0.0.3") + + ep2txt := endpoint.NewEndpointWithTTL("fail__edns_housekeeping.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeTXT, endpoint.TTL(recordTTL), "something") // "__edns_housekeeping" is the TXT suffix + ep2txt.Labels = map[string]string{ + endpoint.OwnedRecordLabelKey: "fail.zone-1.ext-dns-test-2.teapot.zalan.do", + } + + // "success" and "fail" are created in the first step, both are submitted in the same batch; this should fail + cs1 := provider.newChanges(route53.ChangeActionCreate, []*endpoint.Endpoint{ep2, ep2txt, ep1}) + input1 := &route53.ChangeResourceRecordSetsInput{ + HostedZoneId: aws.String("/hostedzone/zone-1.ext-dns-test-2.teapot.zalan.do."), + ChangeBatch: &route53.ChangeBatch{ + Changes: cs1.Route53Changes(), + }, + } + clientStub.MockMethod("ChangeResourceRecordSets", input1).Return(nil, fmt.Errorf("Mock route53 failure")) + + // because of the failure, changes will be retried one by one; make "fail" submitted in its own batch fail as well + cs2 := provider.newChanges(route53.ChangeActionCreate, []*endpoint.Endpoint{ep2, ep2txt}) + input2 := &route53.ChangeResourceRecordSetsInput{ + HostedZoneId: aws.String("/hostedzone/zone-1.ext-dns-test-2.teapot.zalan.do."), + ChangeBatch: &route53.ChangeBatch{ + Changes: cs2.Route53Changes(), + }, + } + clientStub.MockMethod("ChangeResourceRecordSets", input2).Return(nil, fmt.Errorf("Mock route53 failure")) + + // "success" should have been created, verify that we still get an error because "fail" failed + require.Error(t, provider.submitChanges(ctx, cs1, zones)) + + // assert that "success" was successfully created and "fail" and its TXT record were not + records, err := provider.Records(ctx) + require.NoError(t, err) + require.True(t, containsRecordWithDNSName(records, "success.zone-1.ext-dns-test-2.teapot.zalan.do")) + require.False(t, containsRecordWithDNSName(records, "fail.zone-1.ext-dns-test-2.teapot.zalan.do")) + require.False(t, containsRecordWithDNSName(records, "fail__edns_housekeeping.zone-1.ext-dns-test-2.teapot.zalan.do")) + + // next batch should contain "fail" and "success2", should succeed this time + cs3 := provider.newChanges(route53.ChangeActionCreate, []*endpoint.Endpoint{ep2, ep2txt, ep3}) + require.NoError(t, provider.submitChanges(ctx, cs3, zones)) + + // verify all records are there + records, err = provider.Records(ctx) + require.NoError(t, err) + require.True(t, containsRecordWithDNSName(records, "success.zone-1.ext-dns-test-2.teapot.zalan.do")) + require.True(t, containsRecordWithDNSName(records, "fail.zone-1.ext-dns-test-2.teapot.zalan.do")) + require.True(t, containsRecordWithDNSName(records, "success2.zone-1.ext-dns-test-2.teapot.zalan.do")) + require.True(t, containsRecordWithDNSName(records, "fail__edns_housekeeping.zone-1.ext-dns-test-2.teapot.zalan.do")) +} + func TestAWSBatchChangeSet(t *testing.T) { var cs Route53Changes @@ -1375,6 +1434,7 @@ func newAWSProviderWithTagFilter(t *testing.T, domainFilter endpoint.DomainFilte zoneTagFilter: zoneTagFilter, dryRun: false, zonesCache: &zonesListCache{duration: 1 * time.Minute}, + failedChangesQueue: make(map[string]Route53Changes), } createAWSZone(t, provider, &route53.HostedZone{ @@ -1449,6 +1509,15 @@ func validateRecords(t *testing.T, records []*route53.ResourceRecordSet, expecte assert.ElementsMatch(t, expected, records) } +func containsRecordWithDNSName(records []*endpoint.Endpoint, dnsName string) bool { + for _, record := range records { + if record.DNSName == dnsName { + return true + } + } + return false +} + func TestRequiresDeleteCreate(t *testing.T) { provider, _ := newAWSProvider(t, endpoint.NewDomainFilter([]string{"foo.bar."}), provider.NewZoneIDFilter([]string{}), provider.NewZoneTypeFilter(""), defaultEvaluateTargetHealth, false, []*endpoint.Endpoint{}) diff --git a/registry/txt.go b/registry/txt.go index 5a652fec0e..faa3b7453a 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -198,6 +198,7 @@ func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpo txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), endpoint.RecordTypeTXT, r.Labels.Serialize(true)) if txt != nil { txt.WithSetIdentifier(r.SetIdentifier) + txt.Labels[endpoint.OwnedRecordLabelKey] = r.DNSName txt.ProviderSpecific = r.ProviderSpecific endpoints = append(endpoints, txt) } @@ -206,6 +207,7 @@ func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpo txtNew := endpoint.NewEndpoint(im.mapper.toNewTXTName(r.DNSName, r.RecordType), endpoint.RecordTypeTXT, r.Labels.Serialize(true)) if txtNew != nil { txtNew.WithSetIdentifier(r.SetIdentifier) + txtNew.Labels[endpoint.OwnedRecordLabelKey] = r.DNSName txtNew.ProviderSpecific = r.ProviderSpecific endpoints = append(endpoints, txtNew) } diff --git a/registry/txt_test.go b/registry/txt_test.go index 9d5f263b95..5e8168045e 100644 --- a/registry/txt_test.go +++ b/registry/txt_test.go @@ -436,38 +436,38 @@ func testTXTRegistryApplyChangesWithPrefix(t *testing.T) { expected := &plan.Changes{ Create: []*endpoint.Endpoint{ newEndpointWithOwnerResource("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress"), - newEndpointWithOwner("txt.new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("txt.cname-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("txt.new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("txt.cname-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), newEndpointWithOwnerResource("multiple.test-zone.example.org", "lb3.loadbalancer.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress").WithSetIdentifier("test-set-3"), - newEndpointWithOwner("txt.multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-3"), - newEndpointWithOwner("txt.cname-multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-3"), + newEndpointWithOwnerAndOwnedRecord("txt.multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-3"), + newEndpointWithOwnerAndOwnedRecord("txt.cname-multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-3"), newEndpointWithOwnerResource("example", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress"), - newEndpointWithOwner("txt.example", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("txt.cname-example", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("txt.example", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "example"), + newEndpointWithOwnerAndOwnedRecord("txt.cname-example", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "example"), }, Delete: []*endpoint.Endpoint{ newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("txt.foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("txt.cname-foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("txt.foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "foobar.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("txt.cname-foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "foobar.test-zone.example.org"), newEndpointWithOwner("multiple.test-zone.example.org", "lb1.loadbalancer.com", endpoint.RecordTypeCNAME, "owner").WithSetIdentifier("test-set-1"), - newEndpointWithOwner("txt.multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-1"), - newEndpointWithOwner("txt.cname-multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-1"), + newEndpointWithOwnerAndOwnedRecord("txt.multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-1"), + newEndpointWithOwnerAndOwnedRecord("txt.cname-multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-1"), }, UpdateNew: []*endpoint.Endpoint{ newEndpointWithOwnerResource("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress-2"), - newEndpointWithOwner("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("txt.cname-tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "", "tar.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("txt.cname-tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "", "tar.test-zone.example.org"), newEndpointWithOwnerResource("multiple.test-zone.example.org", "new.loadbalancer.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress-2").WithSetIdentifier("test-set-2"), - newEndpointWithOwner("txt.multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-2"), - newEndpointWithOwner("txt.cname-multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-2"), + newEndpointWithOwnerAndOwnedRecord("txt.multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-2"), + newEndpointWithOwnerAndOwnedRecord("txt.cname-multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-2"), }, UpdateOld: []*endpoint.Endpoint{ newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("txt.cname-tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "tar.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("txt.cname-tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "tar.test-zone.example.org"), newEndpointWithOwner("multiple.test-zone.example.org", "lb2.loadbalancer.com", endpoint.RecordTypeCNAME, "owner").WithSetIdentifier("test-set-2"), - newEndpointWithOwner("txt.multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-2"), - newEndpointWithOwner("txt.cname-multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-2"), + newEndpointWithOwnerAndOwnedRecord("txt.multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-2"), + newEndpointWithOwnerAndOwnedRecord("txt.cname-multiple.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-2"), }, } p.OnApplyChanges = func(ctx context.Context, got *plan.Changes) { @@ -513,8 +513,8 @@ func testTXTRegistryApplyChangesWithTemplatedPrefix(t *testing.T) { expected := &plan.Changes{ Create: []*endpoint.Endpoint{ newEndpointWithOwnerResource("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress"), - newEndpointWithOwner("prefix.new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("prefixcname.new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("prefix.new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("prefixcname.new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), }, } p.OnApplyChanges = func(ctx context.Context, got *plan.Changes) { @@ -557,8 +557,8 @@ func testTXTRegistryApplyChangesWithTemplatedSuffix(t *testing.T) { expected := &plan.Changes{ Create: []*endpoint.Endpoint{ newEndpointWithOwnerResource("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress"), - newEndpointWithOwner("new-record-1-cnamesuffix.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("new-record-1-suffix.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("new-record-1-cnamesuffix.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("new-record-1-suffix.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), }, } p.OnApplyChanges = func(ctx context.Context, got *plan.Changes) { @@ -636,41 +636,41 @@ func testTXTRegistryApplyChangesWithSuffix(t *testing.T) { expected := &plan.Changes{ Create: []*endpoint.Endpoint{ newEndpointWithOwnerResource("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress"), - newEndpointWithOwner("new-record-1-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-new-record-1-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("new-record-1-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("cname-new-record-1-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), newEndpointWithOwnerResource("multiple.test-zone.example.org", "lb3.loadbalancer.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress").WithSetIdentifier("test-set-3"), - newEndpointWithOwner("multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-3"), - newEndpointWithOwner("cname-multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-3"), + newEndpointWithOwnerAndOwnedRecord("multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-3"), + newEndpointWithOwnerAndOwnedRecord("cname-multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-3"), newEndpointWithOwnerResource("example", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress"), - newEndpointWithOwner("example-txt", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-example-txt", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("example-txt", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "example"), + newEndpointWithOwnerAndOwnedRecord("cname-example-txt", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "example"), newEndpointWithOwnerResource("*.wildcard.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress"), - newEndpointWithOwner("wildcard-txt.wildcard.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-wildcard-txt.wildcard.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("wildcard-txt.wildcard.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "*.wildcard.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("cname-wildcard-txt.wildcard.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, "", "*.wildcard.test-zone.example.org"), }, Delete: []*endpoint.Endpoint{ newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("foobar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-foobar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("foobar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "foobar.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("cname-foobar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "foobar.test-zone.example.org"), newEndpointWithOwner("multiple.test-zone.example.org", "lb1.loadbalancer.com", endpoint.RecordTypeCNAME, "owner").WithSetIdentifier("test-set-1"), - newEndpointWithOwner("multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-1"), - newEndpointWithOwner("cname-multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-1"), + newEndpointWithOwnerAndOwnedRecord("multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-1"), + newEndpointWithOwnerAndOwnedRecord("cname-multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-1"), }, UpdateNew: []*endpoint.Endpoint{ newEndpointWithOwnerResource("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress-2"), - newEndpointWithOwner("tar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-tar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("tar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "", "tar.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("cname-tar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "", "tar.test-zone.example.org"), newEndpointWithOwnerResource("multiple.test-zone.example.org", "new.loadbalancer.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress-2").WithSetIdentifier("test-set-2"), - newEndpointWithOwner("multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-2"), - newEndpointWithOwner("cname-multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-2"), + newEndpointWithOwnerAndOwnedRecord("multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-2"), + newEndpointWithOwnerAndOwnedRecord("cname-multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-2"), }, UpdateOld: []*endpoint.Endpoint{ newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("tar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-tar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("tar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "tar.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("cname-tar-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "tar.test-zone.example.org"), newEndpointWithOwner("multiple.test-zone.example.org", "lb2.loadbalancer.com", endpoint.RecordTypeCNAME, "owner").WithSetIdentifier("test-set-2"), - newEndpointWithOwner("multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-2"), - newEndpointWithOwner("cname-multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "").WithSetIdentifier("test-set-2"), + newEndpointWithOwnerAndOwnedRecord("multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-2"), + newEndpointWithOwnerAndOwnedRecord("cname-multiple-txt.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "multiple.test-zone.example.org").WithSetIdentifier("test-set-2"), }, } p.OnApplyChanges = func(ctx context.Context, got *plan.Changes) { @@ -735,16 +735,16 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) { expected := &plan.Changes{ Create: []*endpoint.Endpoint{ newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("cname-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), newEndpointWithOwner("example", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("example", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-example", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("example", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "example"), + newEndpointWithOwnerAndOwnedRecord("cname-example", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "example"), }, Delete: []*endpoint.Endpoint{ newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "foobar.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("cname-foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "foobar.test-zone.example.org"), }, UpdateNew: []*endpoint.Endpoint{}, UpdateOld: []*endpoint.Endpoint{}, @@ -867,11 +867,17 @@ func testTXTRegistryMissingRecordsNoPrefix(t *testing.T) { // 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", + }, }, } @@ -965,11 +971,17 @@ func testTXTRegistryMissingRecordsWithPrefix(t *testing.T) { // 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", + }, }, } @@ -1112,16 +1124,16 @@ func TestNewTXTScheme(t *testing.T) { expected := &plan.Changes{ Create: []*endpoint.Endpoint{ newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("cname-new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "new-record-1.test-zone.example.org"), newEndpointWithOwner("example", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("example", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-example", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("example", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "example"), + newEndpointWithOwnerAndOwnedRecord("cname-example", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "example"), }, Delete: []*endpoint.Endpoint{ newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), - newEndpointWithOwner("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), - newEndpointWithOwner("cname-foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), + newEndpointWithOwnerAndOwnedRecord("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "foobar.test-zone.example.org"), + newEndpointWithOwnerAndOwnedRecord("cname-foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, "", "foobar.test-zone.example.org"), }, UpdateNew: []*endpoint.Endpoint{}, UpdateOld: []*endpoint.Endpoint{}, @@ -1153,13 +1165,17 @@ func TestGenerateTXT(t *testing.T) { DNSName: "foo.test-zone.example.org", Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""}, RecordType: endpoint.RecordTypeTXT, - Labels: map[string]string{}, + Labels: map[string]string{ + endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org", + }, }, { DNSName: "cname-foo.test-zone.example.org", Targets: endpoint.Targets{"\"heritage=external-dns,external-dns/owner=owner\""}, RecordType: endpoint.RecordTypeTXT, - Labels: map[string]string{}, + Labels: map[string]string{ + endpoint.OwnedRecordLabelKey: "foo.test-zone.example.org", + }, }, } p := inmemory.NewInMemoryProvider() @@ -1196,6 +1212,10 @@ func newEndpointWithOwner(dnsName, target, recordType, ownerID string) *endpoint return newEndpointWithOwnerAndLabels(dnsName, target, recordType, ownerID, nil) } +func newEndpointWithOwnerAndOwnedRecord(dnsName, target, recordType, ownerID, ownedRecord string) *endpoint.Endpoint { + return newEndpointWithOwnerAndLabels(dnsName, target, recordType, ownerID, endpoint.Labels{endpoint.OwnedRecordLabelKey: ownedRecord}) +} + func newEndpointWithOwnerAndLabels(dnsName, target, recordType, ownerID string, labels endpoint.Labels) *endpoint.Endpoint { e := endpoint.NewEndpoint(dnsName, recordType, target) e.Labels[endpoint.OwnerLabelKey] = ownerID