Merge a122776 into b325616

kubernetes-sigs · Apr 9, 2020 · 1eaf06d · 1eaf06d
2 parents b325616 + a122776
commit 1eaf06d
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 13 deletions.
diff --git a/provider/aws.go b/provider/aws.go
@@ -388,9 +388,9 @@ func (p *AWSProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) e
 
 	combinedChanges := make([]*route53.Change, 0, len(changes.Create)+len(changes.UpdateNew)+len(changes.Delete))
 
+	combinedChanges = append(combinedChanges, p.newChanges(route53.ChangeActionDelete, changes.Delete, records, zones)...)
 	combinedChanges = append(combinedChanges, p.newChanges(route53.ChangeActionCreate, changes.Create, records, zones)...)
 	combinedChanges = append(combinedChanges, p.newChanges(route53.ChangeActionUpsert, changes.UpdateNew, records, zones)...)
-	combinedChanges = append(combinedChanges, p.newChanges(route53.ChangeActionDelete, changes.Delete, records, zones)...)
 
 	return p.submitChanges(ctx, combinedChanges, zones)
 }

diff --git a/registry/txt.go b/registry/txt.go
@@ -40,6 +40,8 @@ type TXTRegistry struct {
 	recordsCache            []*endpoint.Endpoint
 	recordsCacheRefreshTime time.Time
 	cacheInterval           time.Duration
+
+	existingTXTRecords      map[string]endpoint.Labels
 }
 
 // NewTXTRegistry returns new TXTRegistry object
@@ -69,6 +71,8 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
 		return im.recordsCache, nil
 	}
 
+	im.existingTXTRecords = map[string]endpoint.Labels{}
+
 	records, err := im.provider.Records(ctx)
 	if err != nil {
 		return nil, err
@@ -95,6 +99,9 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
 		if err != nil {
 			return nil, err
 		}
+
+		im.existingTXTRecords[record.DNSName] = labels
+
 		key := fmt.Sprintf("%s::%s", im.mapper.toEndpointName(record.DNSName), record.SetIdentifier)
 		labelMap[key] = labels
 	}
@@ -129,30 +136,42 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes)
 		UpdateOld: filterOwnedRecords(im.ownerID, changes.UpdateOld),
 		Delete:    filterOwnedRecords(im.ownerID, changes.Delete),
 	}
+
+	for _, r := range filteredChanges.Delete {
+		txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), endpoint.RecordTypeTXT, r.Labels.Serialize(true)).WithSetIdentifier(r.SetIdentifier)
+		txt.ProviderSpecific = r.ProviderSpecific
+
+		// when we delete TXT records for which value has changed (due to new label) this would still work because
+		// !!! TXT record value is uniquely generated from the Labels of the endpoint. Hence old TXT record can be uniquely reconstructed
+		filteredChanges.Delete = append(filteredChanges.Delete, txt)
+
+		if im.cacheInterval > 0 {
+			im.removeFromCache(r)
+		}
+	}
+
 	for _, r := range filteredChanges.Create {
 		if r.Labels == nil {
 			r.Labels = make(map[string]string)
 		}
 		r.Labels[endpoint.OwnerLabelKey] = im.ownerID
 		txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), endpoint.RecordTypeTXT, r.Labels.Serialize(true)).WithSetIdentifier(r.SetIdentifier)
 		txt.ProviderSpecific = r.ProviderSpecific
-		filteredChanges.Create = append(filteredChanges.Create, txt)
 
-		if im.cacheInterval > 0 {
-			im.addToCache(r)
-		}
-	}
+		if e, ok := im.existingTXTRecords[txt.DNSName]; ok {
+			if endpointOwner, ok := e[endpoint.OwnerLabelKey]; ok && endpointOwner == im.ownerID {
+				log.Debugf("Existing TXT record found for %s, deleting it.\n", txt.DNSName)
 
-	for _, r := range filteredChanges.Delete {
-		txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), endpoint.RecordTypeTXT, r.Labels.Serialize(true)).WithSetIdentifier(r.SetIdentifier)
-		txt.ProviderSpecific = r.ProviderSpecific
+				filteredChanges.Delete = append(filteredChanges.Delete, txt)
+			} else {
+				log.Debugf(`Existing TXT record found for %s, but it cannot be deleted because owner id does not match, found: "%s", required: "%s"`, txt.DNSName, endpointOwner, im.ownerID)
+			}
+		}
 
-		// when we delete TXT records for which value has changed (due to new label) this would still work because
-		// !!! TXT record value is uniquely generated from the Labels of the endpoint. Hence old TXT record can be uniquely reconstructed
-		filteredChanges.Delete = append(filteredChanges.Delete, txt)
+		filteredChanges.Create = append(filteredChanges.Create, txt)
 
 		if im.cacheInterval > 0 {
-			im.removeFromCache(r)
+			im.addToCache(r)
 		}
 	}