cmd/devp2p: submit Route53 changes in batches (#20524)

This change works around the 32k RDATA character limit per change
request and fixes several issues in the deployer which prevented it from
working for our production trees.

cmd/devp2p/dns_cloudflare.go

@@ -130,9 +130,9 @@ func (c *cloudflareClient) uploadRecords(name string, records map[string]string)
 		if !exists {
 			// Entry is unknown, push a new one to Cloudflare.
 			log.Info(fmt.Sprintf("Creating %s = %q", path, val))
-			ttl := 1
+			ttl := rootTTL
 			if path != name {
-				ttl = 2147483647 // Max TTL permitted by Cloudflare
+				ttl = treeNodeTTL // Max TTL permitted by Cloudflare
 			}
 			_, err = c.CreateDNSRecord(c.zoneID, cloudflare.DNSRecord{Type: "TXT", Name: path, Content: val, TTL: ttl})
 		} else if old.Content != val {

cmd/devp2p/dns_route53.go

@@ -19,6 +19,7 @@ package main
 import (
 	"errors"
 	"fmt"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -31,6 +32,10 @@ import (
 	"gopkg.in/urfave/cli.v1"
 )
 
+// The Route53 limits change sets to this size. DNS changes need to be split
+// up into multiple batches to work around the limit.
+const route53ChangeLimit = 30000
+
 var (
 	route53AccessKeyFlag = cli.StringFlag{
 		Name:   "access-key-id",
@@ -53,6 +58,11 @@ type route53Client struct {
 	zoneID string
 }
 
+type recordSet struct {
+	values []string
+	ttl    int64
+}
+
 // newRoute53Client sets up a Route53 API client from command line flags.
 func newRoute53Client(ctx *cli.Context) *route53Client {
 	akey := ctx.String(route53AccessKeyFlag.Name)
@@ -78,31 +88,39 @@ func (c *route53Client) deploy(name string, t *dnsdisc.Tree) error {
 	}
 
 	// Compute DNS changes.
-	records := t.ToTXT(name)
-	changes, err := c.computeChanges(name, records)
+	existing, err := c.collectRecords(name)
 	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 request.
-	log.Info(fmt.Sprintf("Submitting %d changes to Route53", len(changes)))
-	batch := new(route53.ChangeBatch)
-	batch.SetChanges(changes)
-	batch.SetComment(fmt.Sprintf("enrtree update of %s at seq %d", name, t.Seq()))
-	req := &route53.ChangeResourceRecordSetsInput{HostedZoneId: &c.zoneID, ChangeBatch: batch}
-	resp, err := c.api.ChangeResourceRecordSets(req)
-	if err != nil {
-		return err
-	}
+	// Submit change batches.
+	batches := splitChanges(changes, route53ChangeLimit)
+	for i, changes := range batches {
+		log.Info(fmt.Sprintf("Submitting %d changes to Route53", len(changes)))
+		batch := new(route53.ChangeBatch)
+		batch.SetChanges(changes)
+		batch.SetComment(fmt.Sprintf("enrtree update %d/%d of %s at seq %d", i+1, len(batches), name, t.Seq()))
+		req := &route53.ChangeResourceRecordSetsInput{HostedZoneId: &c.zoneID, ChangeBatch: batch}
+		resp, err := c.api.ChangeResourceRecordSets(req)
+		if err != nil {
+			return err
+		}
 
-	// Wait for the change to be applied.
-	log.Info(fmt.Sprintf("Waiting for change request %s", *resp.ChangeInfo.Id))
-	wreq := &route53.GetChangeInput{Id: resp.ChangeInfo.Id}
-	return c.api.WaitUntilResourceRecordSetsChanged(wreq)
+		log.Info(fmt.Sprintf("Waiting for change request %s", *resp.ChangeInfo.Id))
+		wreq := &route53.GetChangeInput{Id: resp.ChangeInfo.Id}
+		if err := c.api.WaitUntilResourceRecordSetsChanged(wreq); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 // checkZone verifies zone information for the given domain.
@@ -137,7 +155,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]recordSet) []*route53.Change {
 	// Convert all names to lowercase.
 	lrecords := make(map[string]string, len(records))
 	for name, r := range records {
@@ -145,22 +163,15 @@ 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
+		ttl := int64(rootTTL)
 		if path != name {
-			ttl = 2147483647
+			ttl = int64(treeNodeTTL)
 		}
 
 		prevRecords, exists := existing[path]
-		prevValue := combineTXT(prevRecords)
+		prevValue := combineTXT(prevRecords.values)
 		if !exists {
 			// Entry is unknown, push a new one
 			log.Info(fmt.Sprintf("Creating %s = %q", path, val))
@@ -175,32 +186,76 @@ func (c *route53Client) computeChanges(name string, records map[string]string) (
 	}
 
 	// Iterate over the old records and delete anything stale.
-	for path, values := range existing {
+	for path, set := range existing {
 		if _, ok := records[path]; ok {
 			continue
 		}
 		// Stale entry, nuke it.
-		log.Info(fmt.Sprintf("Deleting %s = %q", path, combineTXT(values)))
-		changes = append(changes, newTXTChange("DELETE", path, 1, values))
+		log.Info(fmt.Sprintf("Deleting %s = %q", path, combineTXT(set.values)))
+		changes = append(changes, newTXTChange("DELETE", path, set.ttl, set.values))
 	}
-	return changes, nil
+
+	sortChanges(changes)
+	return changes
+}
+
+// sortChanges ensures DNS changes are in leaf-added -> root-changed -> leaf-deleted order.
+func sortChanges(changes []*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 > limit {
+			batches = append(batches, nil)
+			batchSize = 0
+		}
+		batches[len(batches)-1] = append(batches[len(batches)-1], ch)
+		batchSize += size
+	}
+	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 {
+		if rr.Value != nil {
+			size += len(*rr.Value)
+		}
+	}
+	return size
 }
 
 // collectRecords collects all TXT records below the given name.
-func (c *route53Client) collectRecords(name string) (map[string][]string, error) {
+func (c *route53Client) collectRecords(name string) (map[string]recordSet, error) {
 	log.Info(fmt.Sprintf("Retrieving existing TXT records on %s (%s)", name, c.zoneID))
 	var req route53.ListResourceRecordSetsInput
 	req.SetHostedZoneId(c.zoneID)
-	existing := make(map[string][]string)
+	existing := make(map[string]recordSet)
 	err := c.api.ListResourceRecordSetsPages(&req, func(resp *route53.ListResourceRecordSetsOutput, last bool) bool {
 		for _, set := range resp.ResourceRecordSets {
 			if !isSubdomain(*set.Name, name) || *set.Type != "TXT" {
 				continue
 			}
-			name := strings.TrimSuffix(*set.Name, ".")
+			s := recordSet{ttl: *set.TTL}
 			for _, rec := range set.ResourceRecords {
-				existing[name] = append(existing[name], *rec.Value)
+				s.values = append(s.values, *rec.Value)
 			}
+			name := strings.TrimSuffix(*set.Name, ".")
+			existing[name] = s
 		}
 		return true
 	})
@@ -208,7 +263,7 @@ func (c *route53Client) collectRecords(name string) (map[string][]string, error)
 }
 
 // newTXTChange creates a change to a TXT record.
-func newTXTChange(action, name string, ttl int, values []string) *route53.Change {
+func newTXTChange(action, name string, ttl int64, values []string) *route53.Change {
 	var c route53.Change
 	var r route53.ResourceRecordSet
 	var rrs []*route53.ResourceRecord
@@ -219,7 +274,7 @@ func newTXTChange(action, name string, ttl int, values []string) *route53.Change
 	}
 	r.SetType("TXT")
 	r.SetName(name)
-	r.SetTTL(int64(ttl))
+	r.SetTTL(ttl)
 	r.SetResourceRecords(rrs)
 	c.SetAction(action)
 	c.SetResourceRecordSet(&r)

cmd/devp2p/dns_route53_test.go

@@ -0,0 +1,156 @@
+// Copyright 2020 The go-ethereum Authors
+// This file is part of go-ethereum.
+//
+// go-ethereum is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// go-ethereum is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with go-ethereum. If not, see <http://www.gnu.org/licenses/>.
+
+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]recordSet{
+		"2kfjogvxdqtxxugbh7gs7naaai.n": {ttl: 3333, values: []string{
+			`"enr:-HW4QO1ml1DdXLeZLsUxewnthhUy8eROqkDyoMTyavfks9JlYQIlMFEUoM78PovJDPQrAkrb3LRJ-"`,
+			`"vtrymDguKCOIAWAgmlkgnY0iXNlY3AyNTZrMaEDffaGfJzgGhUif1JqFruZlYmA31HzathLSWxfbq_QoQ4"`,
+		}},
+		"fdxn3sn67na5dka4j2gok7bvqi.n": {ttl: treeNodeTTL, values: []string{`"enrtree-branch:"`}},
+		"n":                            {ttl: rootTTL, values: []string{`"enrtree-root:v1 e=2KFJOGVXDQTXXUGBH7GS7NAAAI l=FDXN3SN67NA5DKA4J2GOK7BVQI seq=0 sig=v_-J_q_9ICQg5ztExFvLQhDBGMb0lZPJLhe3ts9LAcgqhOhtT3YFJsl8BWNDSwGtamUdR-9xl88_w-X42SVpjwE"`}},
+	}
+
+	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(treeNodeTTL),
+				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(treeNodeTTL),
+				Type: sp("TXT"),
+			},
+		},
+		{
+			Action: sp("CREATE"),
+			ResourceRecordSet: &route53.ResourceRecordSet{
+				Name: sp("h4fht4b454p6uxfd7jcyq5pwdy.n"),
+				ResourceRecords: []*route53.ResourceRecord{{
+					Value: sp(`"enr:-HW4QAggRauloj2SDLtIHN1XBkvhFZ1vtf1raYQp9TBW2RD5EEawDzbtSmlXUfnaHcvwOizhVYLtr7e6vw7NAf6mTuoCgmlkgnY0iXNlY3AyNTZrMaECjrXI8TLNXU0f8cthpAMxEshUyQlK-AM0PW2wfrnacNI"`),
+				}},
+				TTL:  ip(treeNodeTTL),
+				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(treeNodeTTL),
+				Type: sp("TXT"),
+			},
+		},
+		{
+			Action: sp("CREATE"),
+			ResourceRecordSet: &route53.ResourceRecordSet{
+				Name: sp("mhtdo6tmubria2xwg5ludack24.n"),
+				ResourceRecords: []*route53.ResourceRecord{{
+					Value: sp(`"enr:-HW4QLAYqmrwllBEnzWWs7I5Ev2IAs7x_dZlbYdRdMUx5EyKHDXp7AV5CkuPGUPdvbv1_Ms1CPfhcGCvSElSosZmyoqAgmlkgnY0iXNlY3AyNTZrMaECriawHKWdDRk2xeZkrOXBQ0dfMFLHY4eENZwdufn1S1o"`),
+				}},
+				TTL:  ip(treeNodeTTL),
+				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(rootTTL),
+				Type: sp("TXT"),
+			},
+		},
+		{
+			Action: sp("DELETE"),
+			ResourceRecordSet: &route53.ResourceRecordSet{
+				Name: sp("2kfjogvxdqtxxugbh7gs7naaai.n"),
+				ResourceRecords: []*route53.ResourceRecord{
+					{Value: sp(`"enr:-HW4QO1ml1DdXLeZLsUxewnthhUy8eROqkDyoMTyavfks9JlYQIlMFEUoM78PovJDPQrAkrb3LRJ-"`)},
+					{Value: sp(`"vtrymDguKCOIAWAgmlkgnY0iXNlY3AyNTZrMaEDffaGfJzgGhUif1JqFruZlYmA31HzathLSWxfbq_QoQ4"`)},
+				},
+				TTL:  ip(3333),
+				Type: sp("TXT"),
+			},
+		},
+		{
+			Action: sp("DELETE"),
+			ResourceRecordSet: &route53.ResourceRecordSet{
+				Name: sp("fdxn3sn67na5dka4j2gok7bvqi.n"),
+				ResourceRecords: []*route53.ResourceRecord{{
+					Value: sp(`"enrtree-branch:"`),
+				}},
+				TTL:  ip(treeNodeTTL),
+				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:8],
+	}
+	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 }

cmd/devp2p/dnscmd.go

@@ -96,6 +96,11 @@ var (
 	}
 )
 
+const (
+	rootTTL     = 1
+	treeNodeTTL = 2147483647
+)
+
 // dnsSync performs dnsSyncCommand.
 func dnsSync(ctx *cli.Context) error {
 	var (