From 724d5ff6c72c6d201487f8fd9b4a000a571f69af Mon Sep 17 00:00:00 2001 From: Tom Limoncelli Date: Thu, 12 Nov 2020 00:30:40 -0500 Subject: [PATCH] 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 | 94 ++++++++++++++++++--- providers/route53/route53Provider.go | 122 ++++++++++++++++++++------- 2 files changed, 174 insertions(+), 42 deletions(-) diff --git a/integrationTest/integration_test.go b/integrationTest/integration_test.go index 542a8a039..430d17737 100644 --- a/integrationTest/integration_test.go +++ b/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("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")), - 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")), + tc("create dependent A records", + a("foo.a", "1.2.3.4"), + a("quux.a", "2.3.4.5"), + ), + 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"), + ), + 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("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**")), - tc("change it", a("foo", "1.2.3.4"), a("quux", "2.3.4.5"), r53alias("bar", "A", "quux.**current-domain**")), + tc("create dependent records", + a("kyle", "1.2.3.4"), + 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."), + ), ), } diff --git a/providers/route53/route53Provider.go b/providers/route53/route53Provider.go index 0c49e59e8..b666d02f8 100644 --- a/providers/route53/route53Provider.go +++ b/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 { - chg := &r53.Change{} - var rrset *r53.ResourceRecordSet - // if there are no records in our desired state for a key, then we just delete it from r53 + for _, k := range updateOrder { + recs := updates[k] + // If there are no records in our desired state for a key, this + // indicates we should delete all records at that key. if len(recs) == 0 { - dels = append(dels, chg) - chg.Action = sPtr("DELETE") - delDesc = append(delDesc, strings.Join(namesToUpdate[k], "\n")) - // on delete just submit the original resource set we got from r53. + // To delete, we submit the original resource set we got from r53. + var rrset *r53.ResourceRecordSet + // Find the original resource set: 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 { - changes = append(changes, chg) - changeDesc = append(changeDesc, strings.Join(namesToUpdate[k], "\n")) - // on change or create, just build a new record set from our desired state - chg.Action = sPtr("UPSERT") - rrset = &r53.ResourceRecordSet{ - Name: sPtr(k.NameFQDN), - Type: sPtr(k.Type), + // Assemble the change and add it to the list: + chg := &r53.Change{ + Action: sPtr("DELETE"), + ResourceRecordSet: rrset, } - for _, r := range recs { - val := r.GetTargetCombined() - if r.Type != "R53_ALIAS" { + dels = append(dels, chg) + delDesc = append(delDesc, strings.Join(namesToUpdate[k], "\n")) + } 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{ - Name: sPtr(r.GetLabelFQDN()), - Type: sPtr(r.R53Alias["type"]), - } + target := r.GetTargetField() zoneID := getZoneID(zone, r) targetHealth := false - target := r.GetTargetField() - rrset.AliasTarget = &r53.AliasTarget{ - DNSName: &target, - HostedZoneId: aws.String(zoneID), - EvaluateTargetHealth: &targetHealth, + rrset := &r53.ResourceRecordSet{ + Type: sPtr(r.R53Alias["type"]), + AliasTarget: &r53.AliasTarget{ + DNSName: &target, + HostedZoneId: aws.String(zoneID), + EvaluateTargetHealth: &targetHealth, + }, } return rrset }