ROUTE53: Fix R53_ALIAS creation failure (#938)

Fixes https://github.com/StackExchange/dnscontrol/issues/937

* Add integration test to repro https://github.com/StackExchange/dnscontrol/issues/937
* Reformat integration tests to be more readable.
* ROUTE53: Create CNAME/R53_ALIAS in best order
* Each R53_ALIAS_* should be its own change, not combined.
* Refactor and simplify (omg it has gotten ugly)

integrationTest/integration_test.go

@@ -915,23 +915,97 @@ func makeTests(t *testing.T) []*TestGroup {
 			tc("ALIAS at root", alias("@", "foo.com.")),
 			tc("change it", alias("@", "foo2.com.")),
 			tc("ALIAS at subdomain", alias("test", "foo.com.")),
+			tc("change it", alias("test", "foo2.com.")),
 		),
 
 		testgroup("AZURE_ALIAS",
 			requires(providers.CanUseAzureAlias),
-			tc("create dependent A records", a("foo.a", "1.2.3.4"), a("quux.a", "2.3.4.5")),
+			tc("create dependent A records",
-			tc("ALIAS to A record in same zone", a("foo.a", "1.2.3.4"), a("quux.a", "2.3.4.5"), azureAlias("bar.a", "A", "/subscriptions/**subscription-id**/resourceGroups/**resource-group**/providers/Microsoft.Network/dnszones/**current-domain-no-trailing**/A/foo.a")),
+				a("foo.a", "1.2.3.4"),
-			tc("change it", a("foo.a", "1.2.3.4"), a("quux.a", "2.3.4.5"), azureAlias("bar.a", "A", "/subscriptions/**subscription-id**/resourceGroups/**resource-group**/providers/Microsoft.Network/dnszones/**current-domain-no-trailing**/A/quux.a")),
+				a("quux.a", "2.3.4.5"),
-			tc("create dependent CNAME records", cname("foo.cname", "google.com"), cname("quux.cname", "google2.com")),
+			),
-			tc("ALIAS to CNAME record in same zone", cname("foo.cname", "google.com"), cname("quux.cname", "google2.com"), azureAlias("bar", "CNAME", "/subscriptions/**subscription-id**/resourceGroups/**resource-group**/providers/Microsoft.Network/dnszones/**current-domain-no-trailing**/CNAME/foo.cname")),
+			tc("ALIAS to A record in same zone",
-			tc("change it", cname("foo.cname", "google.com"), cname("quux.cname", "google2.com"), azureAlias("bar.cname", "CNAME", "/subscriptions/**subscription-id**/resourceGroups/**resource-group**/providers/Microsoft.Network/dnszones/**current-domain-no-trailing**/CNAME/quux.cname")),
+				a("foo.a", "1.2.3.4"),
+				a("quux.a", "2.3.4.5"),
+				azureAlias("bar.a", "A", "/subscriptions/**subscription-id**/resourceGroups/**resource-group**/providers/Microsoft.Network/dnszones/**current-domain-no-trailing**/A/foo.a"),
+			),
+			tc("change it",
+				a("foo.a", "1.2.3.4"),
+				a("quux.a", "2.3.4.5"),
+				azureAlias("bar.a", "A", "/subscriptions/**subscription-id**/resourceGroups/**resource-group**/providers/Microsoft.Network/dnszones/**current-domain-no-trailing**/A/quux.a"),
+			),
+			tc("create dependent CNAME records",
+				cname("foo.cname", "google.com"),
+				cname("quux.cname", "google2.com"),
+			),
+			tc("ALIAS to CNAME record in same zone",
+				cname("foo.cname", "google.com"),
+				cname("quux.cname", "google2.com"),
+				azureAlias("bar", "CNAME", "/subscriptions/**subscription-id**/resourceGroups/**resource-group**/providers/Microsoft.Network/dnszones/**current-domain-no-trailing**/CNAME/foo.cname"),
+			),
+			tc("change it",
+				cname("foo.cname", "google.com"),
+				cname("quux.cname", "google2.com"),
+				azureAlias("bar.cname", "CNAME", "/subscriptions/**subscription-id**/resourceGroups/**resource-group**/providers/Microsoft.Network/dnszones/**current-domain-no-trailing**/CNAME/quux.cname"),
+			),
 		),
 
-		testgroup("R53_ALIAS",
+		testgroup("R53_ALIAS2",
 			requires(providers.CanUseRoute53Alias),
-			tc("create dependent records", a("foo", "1.2.3.4"), a("quux", "2.3.4.5")),
+			tc("create dependent records",
-			tc("ALIAS to A record in same zone", a("foo", "1.2.3.4"), a("quux", "2.3.4.5"), r53alias("bar", "A", "foo.**current-domain**")),
+				a("kyle", "1.2.3.4"),
-			tc("change it", a("foo", "1.2.3.4"), a("quux", "2.3.4.5"), r53alias("bar", "A", "quux.**current-domain**")),
+				a("cartman", "2.3.4.5"),
+			),
+			tc("ALIAS to A record in same zone",
+				a("kyle", "1.2.3.4"),
+				a("cartman", "2.3.4.5"),
+				r53alias("kenny", "A", "kyle.**current-domain**"),
+			),
+			tc("modify an r53 alias",
+				a("kyle", "1.2.3.4"),
+				a("cartman", "2.3.4.5"),
+				r53alias("kenny", "A", "cartman.**current-domain**"),
+			),
+		),
+
+		testgroup("R53_ALIAS_ORDER",
+			requires(providers.CanUseRoute53Alias),
+			tc("create target cnames",
+				cname("dev-system18", "ec2-54-91-33-155.compute-1.amazonaws.com."),
+				cname("dev-system19", "ec2-54-91-99-999.compute-1.amazonaws.com."),
+			),
+			tc("add an alias to 18",
+				cname("dev-system18", "ec2-54-91-33-155.compute-1.amazonaws.com."),
+				cname("dev-system19", "ec2-54-91-99-999.compute-1.amazonaws.com."),
+				r53alias("dev-system", "CNAME", "dev-system18.**current-domain**"),
+			),
+			tc("modify alias to 19",
+				cname("dev-system18", "ec2-54-91-33-155.compute-1.amazonaws.com."),
+				cname("dev-system19", "ec2-54-91-99-999.compute-1.amazonaws.com."),
+				r53alias("dev-system", "CNAME", "dev-system19.**current-domain**"),
+			),
+			tc("remove alias",
+				cname("dev-system18", "ec2-54-91-33-155.compute-1.amazonaws.com."),
+				cname("dev-system19", "ec2-54-91-99-999.compute-1.amazonaws.com."),
+			),
+			tc("add an alias back",
+				cname("dev-system18", "ec2-54-91-33-155.compute-1.amazonaws.com."),
+				cname("dev-system19", "ec2-54-91-99-999.compute-1.amazonaws.com."),
+				r53alias("dev-system", "CNAME", "dev-system19.**current-domain**"),
+			),
+			tc("remove cnames",
+				r53alias("dev-system", "CNAME", "dev-system19.**current-domain**"),
+			),
+			clear(),
+			tc("create cname+alias in one step",
+				cname("dev-system18", "ec2-54-91-33-155.compute-1.amazonaws.com."),
+				r53alias("dev-system", "CNAME", "dev-system18.**current-domain**"),
+			),
+			clear(),
+			tc("create alias+cname in one step",
+				r53alias("dev-system", "CNAME", "dev-system18.**current-domain**"),
+				cname("dev-system18", "ec2-54-91-33-155.compute-1.amazonaws.com."),
+			),
 		),
 	}
 

providers/route53/route53Provider.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"log"
 	"sort"
 	"strings"
 	"time"
@@ -251,23 +252,51 @@ func (r *route53Provider) GetDomainCorrections(dc *models.DomainConfig) ([]*mode
 		}
 	}
 
+	// updateOrder is the order that the updates will happen.
+	// The order should be sorted by NameFQDN, then Type, with R53_ALIAS_*
+	// types sorted after all other types. R53_ALIAS_* needs to be last
+	// because they are order dependent (aliases must refer to labels
+	// that already exist).
+	var updateOrder []models.RecordKey
+	// Collect the keys
+	for k, _ := range updates {
+		updateOrder = append(updateOrder, k)
+	}
+	// Sort themm
+	sort.Slice(updateOrder, func(i, j int) bool {
+		if updateOrder[i].Type == updateOrder[j].Type {
+			return updateOrder[i].NameFQDN < updateOrder[j].NameFQDN
+		}
+
+		if strings.HasPrefix(updateOrder[i].Type, "R53_ALIAS_") {
+			return false
+		}
+		if strings.HasPrefix(updateOrder[j].Type, "R53_ALIAS_") {
+			return true
+		}
+
+		if updateOrder[i].NameFQDN == updateOrder[j].NameFQDN {
+			return updateOrder[i].Type < updateOrder[j].Type
+		}
+		return updateOrder[i].NameFQDN < updateOrder[j].NameFQDN
+	})
+
 	// we collect all changes into one of two categories now:
 	// pure deletions where we delete an entire record set,
 	// or changes where we upsert an entire record set.
 	dels := []*r53.Change{}
+	delDesc := []string{}
 	changes := []*r53.Change{}
 	changeDesc := []string{}
-	delDesc := []string{}
 
-	for k, recs := range updates {
+	for _, k := range updateOrder {
-		chg := &r53.Change{}
+		recs := updates[k]
-		var rrset *r53.ResourceRecordSet
+		// If there are no records in our desired state for a key, this
-		// if there are no records in our desired state for a key, then we just delete it from r53
+		// indicates we should delete all records at that key.
 		if len(recs) == 0 {
-			dels = append(dels, chg)
+			// To delete, we submit the original resource set we got from r53.
-			chg.Action = sPtr("DELETE")
+			var rrset *r53.ResourceRecordSet
-			delDesc = append(delDesc, strings.Join(namesToUpdate[k], "\n"))
+			// Find the original resource set:
-			// on delete just submit the original resource set we got from r53.
 			for _, r := range r.originalRecords {
 				if unescape(r.Name) == k.NameFQDN && (*r.Type == k.Type || k.Type == "R53_ALIAS_"+*r.Type) {
 					rrset = r
@@ -275,32 +304,62 @@ func (r *route53Provider) GetDomainCorrections(dc *models.DomainConfig) ([]*mode
 				}
 			}
 			if rrset == nil {
+				// This should not happen.
 				return nil, fmt.Errorf("no record set found to delete. Name: '%s'. Type: '%s'", k.NameFQDN, k.Type)
 			}
-		} else {
+			// Assemble the change and add it to the list:
-			changes = append(changes, chg)
+			chg := &r53.Change{
-			changeDesc = append(changeDesc, strings.Join(namesToUpdate[k], "\n"))
+				Action:            sPtr("DELETE"),
-			// on change or create, just build a new record set from our desired state
+				ResourceRecordSet: rrset,
-			chg.Action = sPtr("UPSERT")
-			rrset = &r53.ResourceRecordSet{
-				Name: sPtr(k.NameFQDN),
-				Type: sPtr(k.Type),
 			}
-			for _, r := range recs {
+			dels = append(dels, chg)
-				val := r.GetTargetCombined()
+			delDesc = append(delDesc, strings.Join(namesToUpdate[k], "\n"))
-				if r.Type != "R53_ALIAS" {
+		} else {
+			// If it isn't a delete, it must be either a change or create. In
+			// either case, we build a new record set from the desired state and
+			// UPSERT it.
+
+			if strings.HasPrefix(k.Type, "R53_ALIAS_") {
+				// Each R53_ALIAS_* requires an individual change.
+				if len(recs) != 1 {
+					log.Fatal("Only one R53_ALIAS_ permitted on a label")
+				}
+				for _, r := range recs {
+					rrset := aliasToRRSet(zone, r)
+					rrset.Name = sPtr(k.NameFQDN)
+					// Assemble the change and add it to the list:
+					chg := &r53.Change{
+						Action:            sPtr("UPSERT"),
+						ResourceRecordSet: rrset,
+					}
+					changes = append(changes, chg)
+					changeDesc = append(changeDesc, strings.Join(namesToUpdate[k], "\n"))
+				}
+			} else {
+				// All other keys combine their updates into one rrset:
+				rrset := &r53.ResourceRecordSet{
+					Name: sPtr(k.NameFQDN),
+					Type: sPtr(k.Type),
+				}
+				for _, r := range recs {
+					val := r.GetTargetCombined()
 					rr := &r53.ResourceRecord{
 						Value: &val,
 					}
 					rrset.ResourceRecords = append(rrset.ResourceRecords, rr)
 					i := int64(r.TTL)
 					rrset.TTL = &i // TODO: make sure that ttls are consistent within a set
-				} else {
-					rrset = aliasToRRSet(zone, r)
 				}
+				// Assemble the change and add it to the list:
+				chg := &r53.Change{
+					Action:            sPtr("UPSERT"),
+					ResourceRecordSet: rrset,
+				}
+				changes = append(changes, chg)
+				changeDesc = append(changeDesc, strings.Join(namesToUpdate[k], "\n"))
 			}
+
 		}
-		chg.ResourceRecordSet = rrset
 	}
 
 	addCorrection := func(msg string, req *r53.ChangeResourceRecordSetsInput) {
@@ -405,17 +464,16 @@ func getAliasMap(r *models.RecordConfig) map[string]string {
 }
 
 func aliasToRRSet(zone *r53.HostedZone, r *models.RecordConfig) *r53.ResourceRecordSet {
-	rrset := &r53.ResourceRecordSet{
+	target := r.GetTargetField()
-		Name: sPtr(r.GetLabelFQDN()),
-		Type: sPtr(r.R53Alias["type"]),
-	}
 	zoneID := getZoneID(zone, r)
 	targetHealth := false
-	target := r.GetTargetField()
+	rrset := &r53.ResourceRecordSet{
-	rrset.AliasTarget = &r53.AliasTarget{
+		Type: sPtr(r.R53Alias["type"]),
-		DNSName:              &target,
+		AliasTarget: &r53.AliasTarget{
-		HostedZoneId:         aws.String(zoneID),
+			DNSName:              &target,
-		EvaluateTargetHealth: &targetHealth,
+			HostedZoneId:         aws.String(zoneID),
+			EvaluateTargetHealth: &targetHealth,
+		},
 	}
 	return rrset
 }