cmd/devp2p: use original TTL for Route53 deletes

Deletion requests are not accepted unless both TTL and value match the
existing record. Also improve tests to catch this case.

cmd/devp2p/dns_route53.go

@@ -58,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)
@@ -88,6 +93,7 @@ func (c *route53Client) deploy(name string, t *dnsdisc.Tree) error {
 		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 {
@@ -149,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, existing map[string][]string) []*route53.Change {
+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 {
@@ -159,13 +165,13 @@ func (c *route53Client) computeChanges(name string, records map[string]string, e
 
 	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))
@@ -180,13 +186,13 @@ func (c *route53Client) computeChanges(name string, records map[string]string, e
 	}
 
 	// 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)))
+		log.Info(fmt.Sprintf("Deleting %s = %q", path, combineTXT(set.values)))
-		changes = append(changes, newTXTChange("DELETE", path, 1, values))
+		changes = append(changes, newTXTChange("DELETE", path, set.ttl, set.values))
 	}
 
 	sortChanges(changes)
@@ -234,20 +240,22 @@ func changeSize(ch *route53.Change) int {
 }
 
 // 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
 	})
@@ -255,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

cmd/devp2p/dns_route53_test.go

@@ -10,9 +10,13 @@ import (
 // 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{
+	testTree0 := map[string]recordSet{
-		"fdxn3sn67na5dka4j2gok7bvqi.n": {"enrtree-branch:"},
+		"2kfjogvxdqtxxugbh7gs7naaai.n": {ttl: 3333, values: []string{
-		"n":                            {"enrtree-root:v1 e=FDXN3SN67NA5DKA4J2GOK7BVQI l=FDXN3SN67NA5DKA4J2GOK7BVQI seq=0 sig=Me7k32DXwG06XmqzNW4b3opMrGXQV9Atv6hmJrVPuNx0QZ-PugeoYlMMndDxyN8cOkWGCxOze26WOyD2JAsAvgE"},
+			`"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{
@@ -32,7 +36,7 @@ func TestRoute53ChangeSort(t *testing.T) {
 				ResourceRecords: []*route53.ResourceRecord{{
 					Value: sp(`"enr:-HW4QOFzoVLaFJnNhbgMoDXPnOvcdVuj7pDpqRvh6BRDO68aVi5ZcjB3vzQRZH2IcLBGHzo8uUN3snqmgTiE56CH3AMBgmlkgnY0iXNlY3AyNTZrMaECC2_24YYkYHEgdzxlSNKQEnHhuNAbNlMlWJxrJxbAFvA"`),
 				}},
-				TTL:  ip(2147483647),
+				TTL:  ip(treeNodeTTL),
 				Type: sp("TXT"),
 			},
 		},
@@ -43,7 +47,7 @@ func TestRoute53ChangeSort(t *testing.T) {
 				ResourceRecords: []*route53.ResourceRecord{{
 					Value: sp(`"enrtree://AM5FCQLWIZX2QFPNJAP7VUERCCRNGRHWZG3YYHIUV7BVDQ5FDPRT2@morenodes.example.org"`),
 				}},
-				TTL:  ip(2147483647),
+				TTL:  ip(treeNodeTTL),
 				Type: sp("TXT"),
 			},
 		},
@@ -54,7 +58,7 @@ func TestRoute53ChangeSort(t *testing.T) {
 				ResourceRecords: []*route53.ResourceRecord{{
 					Value: sp(`"enr:-HW4QAggRauloj2SDLtIHN1XBkvhFZ1vtf1raYQp9TBW2RD5EEawDzbtSmlXUfnaHcvwOizhVYLtr7e6vw7NAf6mTuoCgmlkgnY0iXNlY3AyNTZrMaECjrXI8TLNXU0f8cthpAMxEshUyQlK-AM0PW2wfrnacNI"`),
 				}},
-				TTL:  ip(2147483647),
+				TTL:  ip(treeNodeTTL),
 				Type: sp("TXT"),
 			},
 		},
@@ -65,7 +69,7 @@ func TestRoute53ChangeSort(t *testing.T) {
 				ResourceRecords: []*route53.ResourceRecord{{
 					Value: sp(`"enrtree-branch:2XS2367YHAXJFGLZHVAWLQD4ZY,H4FHT4B454P6UXFD7JCYQ5PWDY,MHTDO6TMUBRIA2XWG5LUDACK24"`),
 				}},
-				TTL:  ip(2147483647),
+				TTL:  ip(treeNodeTTL),
 				Type: sp("TXT"),
 			},
 		},
@@ -76,7 +80,7 @@ func TestRoute53ChangeSort(t *testing.T) {
 				ResourceRecords: []*route53.ResourceRecord{{
 					Value: sp(`"enr:-HW4QLAYqmrwllBEnzWWs7I5Ev2IAs7x_dZlbYdRdMUx5EyKHDXp7AV5CkuPGUPdvbv1_Ms1CPfhcGCvSElSosZmyoqAgmlkgnY0iXNlY3AyNTZrMaECriawHKWdDRk2xeZkrOXBQ0dfMFLHY4eENZwdufn1S1o"`),
 				}},
-				TTL:  ip(2147483647),
+				TTL:  ip(treeNodeTTL),
 				Type: sp("TXT"),
 			},
 		},
@@ -87,7 +91,19 @@ func TestRoute53ChangeSort(t *testing.T) {
 				ResourceRecords: []*route53.ResourceRecord{{
 					Value: sp(`"enrtree-root:v1 e=JWXYDBPXYWG6FX3GMDIBFA6CJ4 l=C7HRFPF3BLGF3YR4DY5KX3SMBE seq=1 sig=o908WmNp7LibOfPsr4btQwatZJ5URBr2ZAuxvK4UWHlsB9sUOTJQaGAlLPVAhM__XJesCHxLISo94z5Z2a463gA"`),
 				}},
-				TTL:  ip(1),
+				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"),
 			},
 		},
@@ -96,9 +112,9 @@ func TestRoute53ChangeSort(t *testing.T) {
 			ResourceRecordSet: &route53.ResourceRecordSet{
 				Name: sp("fdxn3sn67na5dka4j2gok7bvqi.n"),
 				ResourceRecords: []*route53.ResourceRecord{{
-					Value: sp("enrtree-branch:"),
+					Value: sp(`"enrtree-branch:"`),
 				}},
-				TTL:  ip(1),
+				TTL:  ip(treeNodeTTL),
 				Type: sp("TXT"),
 			},
 		},
@@ -112,7 +128,7 @@ func TestRoute53ChangeSort(t *testing.T) {
 
 	wantSplit := [][]*route53.Change{
 		wantChanges[:4],
-		wantChanges[4:7],
+		wantChanges[4:8],
 	}
 	split := splitChanges(changes, 600)
 	if !reflect.DeepEqual(split, wantSplit) {

cmd/devp2p/dnscmd.go

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