BUG: TTL consistency check should be on ResourceSet, not Label (#2200)

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) {
+func checkRecordSetHasMultipleTTLs(records []*models.RecordConfig) (errs []error) {
-	// The RFCs say that all records at a particular label should have
+	// 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)})
+				}
+			}
 		}
 	}
 

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)
 	}
 }