cmd/devp2p: ensure Route53 changes are sorted

cmd/devp2p/dns_route53.go

@@ -19,6 +19,7 @@ package main
 import (
 	"errors"
 	"fmt"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -82,18 +83,20 @@ func (c *route53Client) deploy(name string, t *dnsdisc.Tree) error {
 	}
 
 	// Compute DNS changes.
-	records := t.ToTXT(name)
+	existing, err := c.collectRecords(name)
-	changes, err := c.computeChanges(name, records)
 	if err != nil {
 		return err
 	}
+	log.Info(fmt.Sprintf("Found %d TXT records", len(existing)))
+	records := t.ToTXT(name)
+	changes := c.computeChanges(name, records, existing)
 	if len(changes) == 0 {
 		log.Info("No DNS changes needed")
 		return nil
 	}
 
 	// Submit change batches.
-	batches := splitChangeBatches(changes)
+	batches := splitChanges(changes, route53ChangeLimit)
 	for i, changes := range batches {
 		log.Info(fmt.Sprintf("Submitting %d changes to Route53", len(changes)))
 		batch := new(route53.ChangeBatch)
@@ -146,7 +149,7 @@ func (c *route53Client) findZoneID(name string) (string, error) {
 }
 
 // computeChanges creates DNS changes for the given record.
-func (c *route53Client) computeChanges(name string, records map[string]string) ([]*route53.Change, error) {
+func (c *route53Client) computeChanges(name string, records map[string]string, existing map[string][]string) []*route53.Change {
 	// Convert all names to lowercase.
 	lrecords := make(map[string]string, len(records))
 	for name, r := range records {
@@ -154,13 +157,6 @@ func (c *route53Client) computeChanges(name string, records map[string]string) (
 	}
 	records = lrecords
 
-	// Get existing records.
-	existing, err := c.collectRecords(name)
-	if err != nil {
-		return nil, err
-	}
-	log.Info(fmt.Sprintf("Found %d TXT records", len(existing)))
-
 	var changes []*route53.Change
 	for path, val := range records {
 		ttl := 1
@@ -192,18 +188,31 @@ func (c *route53Client) computeChanges(name string, records map[string]string) (
 		log.Info(fmt.Sprintf("Deleting %s = %q", path, combineTXT(values)))
 		changes = append(changes, newTXTChange("DELETE", path, 1, values))
 	}
-	return changes, nil
+
+	sortChanges(changes)
+	return changes
 }
 
-// splitChangeBatches splits up DNS changes such that each change batch
+// sortChanges ensures DNS changes are in leaf-added -> root-changed -> leaf-deleted order.
-// is smaller than the limit.
+func sortChanges(changes []*route53.Change) {
-func splitChangeBatches(changes []*route53.Change) [][]*route53.Change {
+	score := map[string]int{"CREATE": 1, "UPSERT": 2, "DELETE": 3}
+	sort.Slice(changes, func(i, j int) bool {
+		if *changes[i].Action == *changes[j].Action {
+			return *changes[i].ResourceRecordSet.Name < *changes[j].ResourceRecordSet.Name
+		}
+		return score[*changes[i].Action] < score[*changes[j].Action]
+	})
+}
+
+// splitChanges splits up DNS changes such that each change batch
+// is smaller than the given RDATA limit.
+func splitChanges(changes []*route53.Change, limit int) [][]*route53.Change {
 	var batches [][]*route53.Change
 	var batchSize int
 	for _, ch := range changes {
 		// Start new batch if this change pushes the current one over the limit.
 		size := changeSize(ch)
-		if len(batches) == 0 || batchSize+size > route53ChangeLimit {
+		if len(batches) == 0 || batchSize+size > limit {
 			batches = append(batches, nil)
 			batchSize = 0
 		}
@@ -213,6 +222,7 @@ func splitChangeBatches(changes []*route53.Change) [][]*route53.Change {
 	return batches
 }
 
+// changeSize returns the RDATA size of a DNS change.
 func changeSize(ch *route53.Change) int {
 	size := 0
 	for _, rr := range ch.ResourceRecordSet.ResourceRecords {

cmd/devp2p/dns_route53_test.go

@@ -0,0 +1,124 @@
+package main
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/aws/aws-sdk-go/service/route53"
+)
+
+// This test checks that computeChanges/splitChanges create DNS changes in
+// leaf-added -> root-changed -> leaf-deleted order.
+func TestRoute53ChangeSort(t *testing.T) {
+	testTree0 := map[string][]string{
+		"fdxn3sn67na5dka4j2gok7bvqi.n": {"enrtree-branch:"},
+		"n":                            {"enrtree-root:v1 e=FDXN3SN67NA5DKA4J2GOK7BVQI l=FDXN3SN67NA5DKA4J2GOK7BVQI seq=0 sig=Me7k32DXwG06XmqzNW4b3opMrGXQV9Atv6hmJrVPuNx0QZ-PugeoYlMMndDxyN8cOkWGCxOze26WOyD2JAsAvgE"},
+	}
+
+	testTree1 := map[string]string{
+		"n":                            "enrtree-root:v1 e=JWXYDBPXYWG6FX3GMDIBFA6CJ4 l=C7HRFPF3BLGF3YR4DY5KX3SMBE seq=1 sig=o908WmNp7LibOfPsr4btQwatZJ5URBr2ZAuxvK4UWHlsB9sUOTJQaGAlLPVAhM__XJesCHxLISo94z5Z2a463gA",
+		"C7HRFPF3BLGF3YR4DY5KX3SMBE.n": "enrtree://AM5FCQLWIZX2QFPNJAP7VUERCCRNGRHWZG3YYHIUV7BVDQ5FDPRT2@morenodes.example.org",
+		"JWXYDBPXYWG6FX3GMDIBFA6CJ4.n": "enrtree-branch:2XS2367YHAXJFGLZHVAWLQD4ZY,H4FHT4B454P6UXFD7JCYQ5PWDY,MHTDO6TMUBRIA2XWG5LUDACK24",
+		"2XS2367YHAXJFGLZHVAWLQD4ZY.n": "enr:-HW4QOFzoVLaFJnNhbgMoDXPnOvcdVuj7pDpqRvh6BRDO68aVi5ZcjB3vzQRZH2IcLBGHzo8uUN3snqmgTiE56CH3AMBgmlkgnY0iXNlY3AyNTZrMaECC2_24YYkYHEgdzxlSNKQEnHhuNAbNlMlWJxrJxbAFvA",
+		"H4FHT4B454P6UXFD7JCYQ5PWDY.n": "enr:-HW4QAggRauloj2SDLtIHN1XBkvhFZ1vtf1raYQp9TBW2RD5EEawDzbtSmlXUfnaHcvwOizhVYLtr7e6vw7NAf6mTuoCgmlkgnY0iXNlY3AyNTZrMaECjrXI8TLNXU0f8cthpAMxEshUyQlK-AM0PW2wfrnacNI",
+		"MHTDO6TMUBRIA2XWG5LUDACK24.n": "enr:-HW4QLAYqmrwllBEnzWWs7I5Ev2IAs7x_dZlbYdRdMUx5EyKHDXp7AV5CkuPGUPdvbv1_Ms1CPfhcGCvSElSosZmyoqAgmlkgnY0iXNlY3AyNTZrMaECriawHKWdDRk2xeZkrOXBQ0dfMFLHY4eENZwdufn1S1o",
+	}
+
+	wantChanges := []*route53.Change{
+		{
+			Action: sp("CREATE"),
+			ResourceRecordSet: &route53.ResourceRecordSet{
+				Name: sp("2xs2367yhaxjfglzhvawlqd4zy.n"),
+				ResourceRecords: []*route53.ResourceRecord{{
+					Value: sp(`"enr:-HW4QOFzoVLaFJnNhbgMoDXPnOvcdVuj7pDpqRvh6BRDO68aVi5ZcjB3vzQRZH2IcLBGHzo8uUN3snqmgTiE56CH3AMBgmlkgnY0iXNlY3AyNTZrMaECC2_24YYkYHEgdzxlSNKQEnHhuNAbNlMlWJxrJxbAFvA"`),
+				}},
+				TTL:  ip(2147483647),
+				Type: sp("TXT"),
+			},
+		},
+		{
+			Action: sp("CREATE"),
+			ResourceRecordSet: &route53.ResourceRecordSet{
+				Name: sp("c7hrfpf3blgf3yr4dy5kx3smbe.n"),
+				ResourceRecords: []*route53.ResourceRecord{{
+					Value: sp(`"enrtree://AM5FCQLWIZX2QFPNJAP7VUERCCRNGRHWZG3YYHIUV7BVDQ5FDPRT2@morenodes.example.org"`),
+				}},
+				TTL:  ip(2147483647),
+				Type: sp("TXT"),
+			},
+		},
+		{
+			Action: sp("CREATE"),
+			ResourceRecordSet: &route53.ResourceRecordSet{
+				Name: sp("h4fht4b454p6uxfd7jcyq5pwdy.n"),
+				ResourceRecords: []*route53.ResourceRecord{{
+					Value: sp(`"enr:-HW4QAggRauloj2SDLtIHN1XBkvhFZ1vtf1raYQp9TBW2RD5EEawDzbtSmlXUfnaHcvwOizhVYLtr7e6vw7NAf6mTuoCgmlkgnY0iXNlY3AyNTZrMaECjrXI8TLNXU0f8cthpAMxEshUyQlK-AM0PW2wfrnacNI"`),
+				}},
+				TTL:  ip(2147483647),
+				Type: sp("TXT"),
+			},
+		},
+		{
+			Action: sp("CREATE"),
+			ResourceRecordSet: &route53.ResourceRecordSet{
+				Name: sp("jwxydbpxywg6fx3gmdibfa6cj4.n"),
+				ResourceRecords: []*route53.ResourceRecord{{
+					Value: sp(`"enrtree-branch:2XS2367YHAXJFGLZHVAWLQD4ZY,H4FHT4B454P6UXFD7JCYQ5PWDY,MHTDO6TMUBRIA2XWG5LUDACK24"`),
+				}},
+				TTL:  ip(2147483647),
+				Type: sp("TXT"),
+			},
+		},
+		{
+			Action: sp("CREATE"),
+			ResourceRecordSet: &route53.ResourceRecordSet{
+				Name: sp("mhtdo6tmubria2xwg5ludack24.n"),
+				ResourceRecords: []*route53.ResourceRecord{{
+					Value: sp(`"enr:-HW4QLAYqmrwllBEnzWWs7I5Ev2IAs7x_dZlbYdRdMUx5EyKHDXp7AV5CkuPGUPdvbv1_Ms1CPfhcGCvSElSosZmyoqAgmlkgnY0iXNlY3AyNTZrMaECriawHKWdDRk2xeZkrOXBQ0dfMFLHY4eENZwdufn1S1o"`),
+				}},
+				TTL:  ip(2147483647),
+				Type: sp("TXT"),
+			},
+		},
+		{
+			Action: sp("UPSERT"),
+			ResourceRecordSet: &route53.ResourceRecordSet{
+				Name: sp("n"),
+				ResourceRecords: []*route53.ResourceRecord{{
+					Value: sp(`"enrtree-root:v1 e=JWXYDBPXYWG6FX3GMDIBFA6CJ4 l=C7HRFPF3BLGF3YR4DY5KX3SMBE seq=1 sig=o908WmNp7LibOfPsr4btQwatZJ5URBr2ZAuxvK4UWHlsB9sUOTJQaGAlLPVAhM__XJesCHxLISo94z5Z2a463gA"`),
+				}},
+				TTL:  ip(1),
+				Type: sp("TXT"),
+			},
+		},
+		{
+			Action: sp("DELETE"),
+			ResourceRecordSet: &route53.ResourceRecordSet{
+				Name: sp("fdxn3sn67na5dka4j2gok7bvqi.n"),
+				ResourceRecords: []*route53.ResourceRecord{{
+					Value: sp("enrtree-branch:"),
+				}},
+				TTL:  ip(1),
+				Type: sp("TXT"),
+			},
+		},
+	}
+
+	var client route53Client
+	changes := client.computeChanges("n", testTree1, testTree0)
+	if !reflect.DeepEqual(changes, wantChanges) {
+		t.Fatalf("wrong changes (got %d, want %d)", len(changes), len(wantChanges))
+	}
+
+	wantSplit := [][]*route53.Change{
+		wantChanges[:4],
+		wantChanges[4:7],
+	}
+	split := splitChanges(changes, 600)
+	if !reflect.DeepEqual(split, wantSplit) {
+		t.Fatalf("wrong split batches: got %d, want %d", len(split), len(wantSplit))
+	}
+}
+
+func sp(s string) *string { return &s }
+func ip(i int64) *int64   { return &i }