Check for duplicate records much earlier (#467)

* Check for duplicate records much earlier.

* Change GetTargetDiffable to ToDiffable

* fixup!

models/record.go

@@ -3,6 +3,7 @@ package models
 import (
 	"fmt"
 	"log"
+	"sort"
 	"strings"
 
 	"github.com/miekg/dns"
@@ -176,6 +177,31 @@ func (rc *RecordConfig) GetLabelFQDN() string {
 	return rc.NameFQDN
 }
 
+// ToDiffable returns a string that is comparable by a differ.
+// extraMaps: a list of maps that should be included in the comparison.
+func (rc *RecordConfig) ToDiffable(extraMaps ...map[string]string) string {
+	content := fmt.Sprintf("%v ttl=%d", rc.GetTargetCombined(), rc.TTL)
+	for _, valueMap := range extraMaps {
+		// sort the extra values map keys to perform a deterministic
+		// comparison since Golang maps iteration order is not guaranteed
+
+		// FIXME(tlim) The keys of each map is sorted per-map, not across
+		// all maps. This may be intentional since we'd have no way to
+		// deal with duplicates.
+
+		keys := make([]string, 0)
+		for k := range valueMap {
+			keys = append(keys, k)
+		}
+		sort.Strings(keys)
+		for _, k := range keys {
+			v := valueMap[k]
+			content += fmt.Sprintf(" %s=%s", k, v)
+		}
+	}
+	return content
+}
+
 // ToRR converts a RecordConfig to a dns.RR.
 func (rc *RecordConfig) ToRR() dns.RR {
 

pkg/normalize/validate.go

@@ -387,6 +387,8 @@ func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) {
 		if err != nil {
 			errs = append(errs, err)
 		}
+		// Check for duplicates
+		errs = append(errs, checkDuplicates(d.Records)...)
 		// Validate FQDN consistency
 		for _, r := range d.Records {
 			if r.NameFQDN == "" || !strings.HasSuffix(r.NameFQDN, d.Name) {
@@ -416,6 +418,18 @@ func checkCNAMEs(dc *models.DomainConfig) (errs []error) {
 	return
 }
 
+func checkDuplicates(records []*models.RecordConfig) (errs []error) {
+	seen := map[string]*models.RecordConfig{}
+	for _, r := range records {
+		diffable := fmt.Sprintf("%s %s %s", r.GetLabelFQDN(), r.Type, r.ToDiffable())
+		if seen[diffable] != nil {
+			errs = append(errs, errors.Errorf("Exact duplicate record found: %s", diffable))
+		}
+		seen[diffable] = r
+	}
+	return errs
+}
+
 func checkProviderCapabilities(dc *models.DomainConfig) error {
 	types := []struct {
 		rType string

pkg/normalize/validate_test.go

@@ -213,6 +213,54 @@ func TestCAAValidation(t *testing.T) {
 	}
 }
 
+func TestCheckDuplicates(t *testing.T) {
+	records := []*models.RecordConfig{
+		// The only difference is the target:
+		makeRC("www", "example.com", "4.4.4.4", models.RecordConfig{Type: "A"}),
+		makeRC("www", "example.com", "5.5.5.5", models.RecordConfig{Type: "A"}),
+		// The only difference is the rType:
+		makeRC("aaa", "example.com", "uniquestring.com.", models.RecordConfig{Type: "NS"}),
+		makeRC("aaa", "example.com", "uniquestring.com.", models.RecordConfig{Type: "PTR"}),
+		// The only difference is the TTL.
+		makeRC("zzz", "example.com", "4.4.4.4", models.RecordConfig{Type: "A", TTL: 111}),
+		makeRC("zzz", "example.com", "4.4.4.4", models.RecordConfig{Type: "A", TTL: 222}),
+		// Three records each with a different target.
+		makeRC("@", "example.com", "ns1.foo.com.", models.RecordConfig{Type: "NS"}),
+		makeRC("@", "example.com", "ns2.foo.com.", models.RecordConfig{Type: "NS"}),
+		makeRC("@", "example.com", "ns3.foo.com.", models.RecordConfig{Type: "NS"}),
+	}
+	errs := checkDuplicates(records)
+	if len(errs) != 0 {
+		t.Errorf("Expect duplicate NOT found but found %q", errs)
+	}
+}
+
+func TestCheckDuplicates_dup_a(t *testing.T) {
+	records := []*models.RecordConfig{
+		// A records that are exact dupliates.
+		makeRC("@", "example.com", "1.1.1.1", models.RecordConfig{Type: "A"}),
+		makeRC("@", "example.com", "1.1.1.1", models.RecordConfig{Type: "A"}),
+	}
+	errs := checkDuplicates(records)
+	if len(errs) == 0 {
+		t.Error("Expect duplicate found but found none")
+	}
+}
+
+func TestCheckDuplicates_dup_ns(t *testing.T) {
+	records := []*models.RecordConfig{
+		// Three records, the last 2 are duplicates.
+		// NB: This is a common issue.
+		makeRC("@", "example.com", "ns1.foo.com.", models.RecordConfig{Type: "NS"}),
+		makeRC("@", "example.com", "ns2.foo.com.", models.RecordConfig{Type: "NS"}),
+		makeRC("@", "example.com", "ns2.foo.com.", models.RecordConfig{Type: "NS"}),
+	}
+	errs := checkDuplicates(records)
+	if len(errs) == 0 {
+		t.Error("Expect duplicate found but found none")
+	}
+}
+
 func TestTLSAValidation(t *testing.T) {
 	config := &models.DNSConfig{
 		Domains: []*models.DomainConfig{

providers/diff/diff.go

@@ -42,11 +42,17 @@ type differ struct {
 
 // get normalized content for record. target, ttl, mxprio, and specified metadata
 func (d *differ) content(r *models.RecordConfig) string {
+	// NB(tlim): This function will eventually be replaced by calling
+	// r.GetTargetDiffable().  In the meanwhile, this function compares
+	// its output with r.GetTargetDiffable() to make sure the same
+	// results are generated.  Once we have confidence, this function will go away.
 	content := fmt.Sprintf("%v ttl=%d", r.GetTargetCombined(), r.TTL)
+	var allMaps []map[string]string
 	for _, f := range d.extraValues {
 		// sort the extra values map keys to perform a deterministic
 		// comparison since Golang maps iteration order is not guaranteed
 		valueMap := f(r)
+		allMaps = append(allMaps, valueMap)
 		keys := make([]string, 0)
 		for k := range valueMap {
 			keys = append(keys, k)
@@ -57,6 +63,10 @@ func (d *differ) content(r *models.RecordConfig) string {
 			content += fmt.Sprintf(" %s=%s", k, v)
 		}
 	}
+	control := r.ToDiffable(allMaps...)
+	if control != content {
+		panic("OOPS! control != content")
+	}
 	return content
 }