diff --git a/models/record.go b/models/record.go index 4b6e4693d..71415060e 100644 --- a/models/record.go +++ b/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 { diff --git a/pkg/normalize/validate.go b/pkg/normalize/validate.go index 21e5c0efd..0e629e6d9 100644 --- a/pkg/normalize/validate.go +++ b/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 diff --git a/pkg/normalize/validate_test.go b/pkg/normalize/validate_test.go index 2086082b8..9cca46fdc 100644 --- a/pkg/normalize/validate_test.go +++ b/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{ diff --git a/providers/diff/diff.go b/providers/diff/diff.go index ce3e1223e..3f84ca1ab 100644 --- a/providers/diff/diff.go +++ b/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 }