Skip to content

Commit

Permalink
BUG: TTL consistency check should be on ResourceSet, not Label (#2200)
Browse files Browse the repository at this point in the history
  • Loading branch information
@tlimoncelli
tlimoncelli committed Mar 18, 2023
1 parent e0dadba commit ad2f028
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 30 deletions.
36 changes: 11 additions & 25 deletions pkg/normalize/validate.go
View file
Expand Up @@ -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) {
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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 {
Expand All @@ -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)})
}
}
}
}

Expand Down
60 changes: 55 additions & 5 deletions pkg/normalize/validate_test.go
View file
Expand Up @@ -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)
}
}

Expand Down

0 comments on commit ad2f028

Please sign in to comment.