From ad2f028d7616e88d887cbf1de26082809d7cc68b Mon Sep 17 00:00:00 2001 From: Tom Limoncelli Date: Sat, 18 Mar 2023 10:54:59 -0400 Subject: [PATCH] BUG: TTL consistency check should be on ResourceSet, not Label (#2200) --- pkg/normalize/validate.go | 36 +++++++------------- pkg/normalize/validate_test.go | 60 +++++++++++++++++++++++++++++++--- 2 files changed, 66 insertions(+), 30 deletions(-) diff --git a/pkg/normalize/validate.go b/pkg/normalize/validate.go index 5b4fe327c7..51540eadea 100644 --- a/pkg/normalize/validate.go +++ b/pkg/normalize/validate.go @@ -477,7 +477,7 @@ func ValidateAndNormalizeConfig(config *models.DNSConfig) (errs []error) { // Check for duplicates errs = append(errs, checkDuplicates(d.Records)...) // Check for different TTLs under the same label - errs = append(errs, checkLabelHasMultipleTTLs(d.Records)...) + errs = append(errs, checkRecordSetHasMultipleTTLs(d.Records)...) // Validate FQDN consistency for _, r := range d.Records { if r.NameFQDN == "" || !strings.HasSuffix(r.NameFQDN, d.Name) { @@ -614,8 +614,8 @@ func uniq(s []string) []string { return result } -func checkLabelHasMultipleTTLs(records []*models.RecordConfig) (errs []error) { - // The RFCs say that all records at a particular label should have +func checkRecordSetHasMultipleTTLs(records []*models.RecordConfig) (errs []error) { + // The RFCs say that all records at a particular recordset should have // the same TTL. Most providers don't care, and if they do the // dnscontrol provider code usually picks the lowest TTL for all of them. @@ -650,26 +650,6 @@ func checkLabelHasMultipleTTLs(records []*models.RecordConfig) (errs []error) { sort.Strings(labels) slices.Compact(labels) - // Less clear error message: - // for _, label := range labels { - // if len(m[label]) > 1 { - // result := "" - // for ttl, v := range m[label] { - // result += fmt.Sprintf(" %d:", ttl) - - // rtypes := make([]string, len(v)) - // i := 0 - // for k := range v { - // rtypes[i] = k - // i++ - // } - - // result += strings.Join(rtypes, "/") - // } - // errs = append(errs, Warning{fmt.Errorf("inconsistent TTLs at %q:%v", label, result)}) - // } - // } - // Invert for a more clear error message: for _, label := range labels { if len(m[label]) > 1 { @@ -682,8 +662,14 @@ func checkLabelHasMultipleTTLs(records []*models.RecordConfig) (errs []error) { r[rtype][ttl] = true } } - result := formatInconsistency(r) - errs = append(errs, Warning{fmt.Errorf("inconsistent TTLs at %q: %s", label, result)}) + + // Report any cases where a RecordSet has > 1 different TTLs + for rtype := range r { + if len(r[rtype]) > 1 { + result := formatInconsistency(r) + errs = append(errs, Warning{fmt.Errorf("inconsistent TTLs at %q: %s", label, result)}) + } + } } } diff --git a/pkg/normalize/validate_test.go b/pkg/normalize/validate_test.go index 1efd4d2789..24d51d63ba 100644 --- a/pkg/normalize/validate_test.go +++ b/pkg/normalize/validate_test.go @@ -354,27 +354,77 @@ func TestCheckDuplicates_dup_ns(t *testing.T) { } } -func TestCheckLabelHasMultipleTTLs(t *testing.T) { +func TestCheckRecordSetHasMultipleTTLs_err_1type_2ttl(t *testing.T) { records := []*models.RecordConfig{ // different ttl per record makeRC("zzz", "example.com", "4.4.4.4", models.RecordConfig{Type: "A", TTL: 111}), makeRC("zzz", "example.com", "4.4.4.5", models.RecordConfig{Type: "A", TTL: 222}), } - errs := checkLabelHasMultipleTTLs(records) + errs := checkRecordSetHasMultipleTTLs(records) if len(errs) == 0 { t.Error("Expected error on multiple TTLs under the same label, but got none") } } -func TestCheckLabelHasNoMultipleTTLs(t *testing.T) { +func TestCheckRecordSetHasMultipleTTLs_noerr_1type_1ttl(t *testing.T) { records := []*models.RecordConfig{ // different ttl per record makeRC("zzz", "example.com", "4.4.4.4", models.RecordConfig{Type: "A", TTL: 111}), makeRC("zzz", "example.com", "4.4.4.5", models.RecordConfig{Type: "A", TTL: 111}), } - errs := checkLabelHasMultipleTTLs(records) + errs := checkRecordSetHasMultipleTTLs(records) if len(errs) != 0 { - t.Errorf("Expected 0 errors on records having the same TTL under the same label, but got %d", len(errs)) + t.Errorf("Expected 0 errors (same type, same TTL), but got %d", len(errs)) + } +} + +func TestCheckRecordSetHasMultipleTTLs_noerr_2type_2ttl(t *testing.T) { + records := []*models.RecordConfig{ + // different record types, different TTLs + makeRC("zzz", "example.com", "4.4.4.4", models.RecordConfig{Type: "A", TTL: 333}), + makeRC("zzz", "example.com", "4.4.4.5", models.RecordConfig{Type: "NS", TTL: 444}), + } + errs := checkRecordSetHasMultipleTTLs(records) + if len(errs) != 0 { + t.Errorf("Expected 0 errors (different types, different TTLs), but got %d: %v", len(errs), errs) + } +} + +func TestCheckRecordSetHasMultipleTTLs_noerr_2type_1ttl(t *testing.T) { + records := []*models.RecordConfig{ + // different record types, different TTLs + makeRC("zzz", "example.com", "4.4.4.4", models.RecordConfig{Type: "A", TTL: 333}), + makeRC("zzz", "example.com", "4.4.4.5", models.RecordConfig{Type: "NS", TTL: 333}), + } + errs := checkRecordSetHasMultipleTTLs(records) + if len(errs) != 0 { + t.Errorf("Expected 0 errors (different types, same TTLs) but got %d: %v", len(errs), errs) + } +} + +func TestCheckRecordSetHasMultipleTTLs_err_3type_2ttl(t *testing.T) { + records := []*models.RecordConfig{ + // different record types, different TTLs + makeRC("zzz", "example.com", "4.4.4.4", models.RecordConfig{Type: "A", TTL: 555}), + makeRC("zzz", "example.com", "4.4.4.4", models.RecordConfig{Type: "A", TTL: 555}), + makeRC("zzz", "example.com", "4.4.4.5", models.RecordConfig{Type: "NS", TTL: 666}), + } + errs := checkRecordSetHasMultipleTTLs(records) + if len(errs) != 0 { + t.Errorf("Expected 0 errors (differnt types, no errors), but got %d: %v", len(errs), errs) + } +} + +func TestCheckRecordSetHasMultipleTTLs_err_3type_3ttl(t *testing.T) { + records := []*models.RecordConfig{ + // different record types, different TTLs + makeRC("zzz", "example.com", "4.4.4.4", models.RecordConfig{Type: "A", TTL: 777}), + makeRC("zzz", "example.com", "4.4.4.4", models.RecordConfig{Type: "A", TTL: 888}), + makeRC("zzz", "example.com", "4.4.4.5", models.RecordConfig{Type: "NS", TTL: 999}), + } + errs := checkRecordSetHasMultipleTTLs(records) + if len(errs) != 1 { + t.Errorf("Expected 0 errors (differnt types, 1 error), but got %d: %v", len(errs), errs) } }