Emit warning in case of label having multiple TTLs (#1489)

* Emit warning in case of label having multiple TTLs

An RRSet (=label) consisting of multiple records with different TTLs is
something not supported by most providers, and should be avoided.
Furthermore it is deprecated in rfc2181#section-5.2

Emit a warning for now during validation, eventually turning it into a full-blown error.

Fixes #1372

* normalize: less verbose checkLabelHasMultipleTTLs

Code would previously emit a warning for each record it found matching a
previously found label but with a different ttl. This could potentially become
too verbose of an output for larger zones.

Split the loop into two loops, one storing labels and their records' TTLs, the
second checking for multiple TTLs, in order to minimize the messages logged to
one message per problematic label, regardless for the number of records involved.

Co-authored-by: Tom Limoncelli <tlimoncelli@stackoverflow.com>

pkg/normalize/validate.go

@@ -450,6 +450,8 @@ 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)...)
 		// Validate FQDN consistency
 		for _, r := range d.Records {
 			if r.NameFQDN == "" || !strings.HasSuffix(r.NameFQDN, d.Name) {
@@ -553,6 +555,37 @@ func checkDuplicates(records []*models.RecordConfig) (errs []error) {
 	return errs
 }
 
+func uniq(s []uint32) []uint32 {
+	seen := make(map[uint32]struct{})
+	var result []uint32
+
+	for _, k := range s {
+		if _, ok := seen[k]; !ok {
+			seen[k] = struct{}{}
+			result = append(result, k)
+		}
+	}
+	return result
+}
+
+func checkLabelHasMultipleTTLs(records []*models.RecordConfig) (errs []error) {
+	m := make(map[string][]uint32)
+	for _, r := range records {
+		label := fmt.Sprintf("%s %s", r.GetLabelFQDN(), r.Type)
+
+		// if we have more records for a given label, append their TTLs here
+		m[label] = append(m[label], r.TTL)
+	}
+
+	for label := range m {
+		// if after the uniq() pass we still have more than one ttl, it means we have multiple TTLs for that label
+		if len(uniq(m[label])) > 1 {
+			errs = append(errs, Warning{fmt.Errorf("multiple TTLs detected for: %s. This should be avoided.", label)})
+		}
+	}
+	return errs
+}
+
 // We pull this out of checkProviderCapabilities() so that it's visible within
 // the package elsewhere, so that our test suite can look at the list of
 // capabilities we're checking and make sure that it's up-to-date.

pkg/normalize/validate_test.go

@@ -2,6 +2,7 @@ package normalize
 
 import (
 	"fmt"
+	"reflect"
 	"testing"
 
 	"github.com/StackExchange/dnscontrol/v3/models"
@@ -332,6 +333,40 @@ func TestCheckDuplicates_dup_ns(t *testing.T) {
 	}
 }
 
+func TestUniq(t *testing.T) {
+	a := []uint32 {1, 2, 2, 3, 4, 5, 5, 6}
+	expected := []uint32 {1, 2, 3, 4, 5, 6}
+
+	r := uniq(a)
+	if !reflect.DeepEqual(r, expected) {
+		t.Error("Deduplicated slice is different than expected")
+	}
+}
+
+func TestCheckLabelHasMultipleTTLs(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)
+	if len(errs) == 0 {
+		t.Error("Expected error on multiple TTLs under the same label, but got none")
+	}
+}
+
+func TestCheckLabelHasNoMultipleTTLs(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)
+	if len(errs) != 0 {
+		t.Errorf("Expected 0 errors on records having the same TTL under the same label, but got %d", len(errs))
+	}
+}
+
 func TestTLSAValidation(t *testing.T) {
 	config := &models.DNSConfig{
 		Domains: []*models.DomainConfig{