feat: let providers declare a record identity for duplicate checks

documentation/advanced-features/writing-providers.md

@@ -295,6 +295,30 @@ Enable optional capabilities in the `nameProvider.go` file and run the integrati
 
 FYI: If a provider's capabilities changes, run `go generate` to update the documentation.
 
+### Record identity for providers with per-line records
+
+Some providers store the same name and type several times, splitting the answers by record line, region or routing policy. DNSPod lines, Huawei Cloud lines, Gcore GeoDNS and ClouDNS geodns work that way. Those records share a label, type and RDATA, so validation would report them as duplicates.
+
+Such a provider can declare `RecordIdentity` in its `DspFuncs`:
+
+{% code title="nameProvider.go" %}
+```go
+fns := providers.DspFuncs{
+    Initializer:    newNameDsp,
+    RecordAuditor:  AuditRecords,
+    RecordIdentity: recordIdentity,
+}
+```
+{% endcode %}
+
+`recordIdentity` takes one `models.RecordConfig` and returns the text that makes the record distinct, for example `"line_id=10=1"`. Validation appends that text to the key it uses for duplicate detection. A provider that does not declare it keeps the default rules.
+
+The function runs during validation, before the provider has read the zone, so it must depend only on the record it is given. Anything that needs the live zone belongs in the comparable function that the provider passes to `diff2.ByRecord`, not here. Turning a configured line name into a line ID is an example: only the zone knows the mapping.
+
+When a domain has several providers, the first one that declares an identity function is used. The exception applies to the whole domain, so a second provider on the same domain inherits it; two providers that both store per-line records should declare the same rule.
+
+`dnscontrol check` does not read `creds.json`, so a provider declared as `NewDnsProvider("name")` has no type at that point and its identity function does not run. Use the two-argument form when `check` should see it.
+
 ## Step 13: Automated code tests
 
 We use a number of automated code-checking systems. Please run your code through all of them and fix all warnings and errors.  Some of the automated fixes may not alway sbe perfect. Therefore, it is best to commit your code before running these and verify that you agree with the changes.

documentation/provider/tencentdns.md

@@ -100,6 +100,26 @@ D("example.com", REG_TENCENT, DnsProvider(DSP_TENCENT),
 
 Available line names, IDs, and weighted-routing features depend on the domain's DNSPod plan and site. Use the DNSPod `DescribeRecordLineList` API to obtain the valid line values for the domain. Using `tencentdns_line_id` avoids ambiguity and is recommended when managing records across different Tencent Cloud sites.
 
+### Per-line answers for one name
+
+Each line stores its own record, so one name and type may appear once per line. Records that share a target are still stored as separate records:
+
+{% code title="dnsconfig.js" %}
+```javascript
+D("example.com", REG_TENCENT, DnsProvider(DSP_TENCENT),
+    CNAME("www", "origin.example.net.", {tencentdns_line_id: "10=1"}),
+    CNAME("www", "origin.example.net.", {tencentdns_line_id: "10=3"}),
+    CNAME("www", "origin.example.net.", {tencentdns_line_id: "10=2"})
+);
+```
+{% endcode %}
+
+Duplicate detection compares the key DNSPod itself uses: name, line, type and value. The line ID is preferred, with the line name as a fallback. Use one style per line, because `tencentdns_line: "电信"` and `tencentdns_line_id: "10=1"` describe the same line in two ways, and validation has no zone to match them with.
+
+A record without line metadata answers on the default line, so it is the same record as one that sets `tencentdns_line_id: "0"` explicitly.
+
+Because validation cannot resolve a line name, a pair that describes one line in both styles passes `check` and is rejected by the service when the change is pushed.
+
 ### Why use `ALIAS` for DNSPod
 
 DNSPod does not natively support the `ALIAS` record type.

pkg/normalize/validate.go

@@ -550,8 +550,9 @@ func ValidateAndNormalizeConfig(config *models.DNSConfig) (errs []error) {
 	}
 
 	for _, d := range config.Domains {
+		identityFn := domainRecordIdentity(d)
 		// Check that CNAMES don't have to co-exist with any other records
-		errs = append(errs, checkCNAMEs(d)...)
+		errs = append(errs, checkCNAMEs(d, identityFn)...)
 		// Check that only one SOA record exist for a zone
 		errs = append(errs, checkMultipleSOAs(d)...)
 		// Check that if any advanced record types are used in a domain, every provider for that domain supports them
@@ -560,7 +561,7 @@ func ValidateAndNormalizeConfig(config *models.DNSConfig) (errs []error) {
 			errs = append(errs, err)
 		}
 		// Check for duplicates
-		errs = append(errs, checkDuplicates(d.Records)...)
+		errs = append(errs, checkDuplicates(d.Records, identityFn)...)
 		// Check for different TTLs under the same label
 		errs = append(errs, checkRecordSetHasMultipleTTLs(d.Records)...)
 		// Check for inconsistent R53 weighted routing metadata within a group
@@ -632,14 +633,57 @@ func checkAutoDNSSEC(dc *models.DomainConfig) (errs []error) {
 	return
 }
 
-func checkCNAMEs(dc *models.DomainConfig) (errs []error) {
+// recordIdentityString returns the string used to detect duplicate records.
+// It is the label, the rType, the RDATA and any provider-declared identity
+// text (for example DNSPod's record line).
+func recordIdentityString(r *models.RecordConfig, extra func(*models.RecordConfig) string) string {
+	id := fmt.Sprintf("%s %s %s", r.GetLabelFQDN(), r.Type, r.ComparableV3)
+	if x := providerIdentity(r, extra); x != "" {
+		id += " " + x
+	}
+	return id
+}
+
+// domainRecordIdentity returns the identity function declared by one of the
+// domain's DNS providers, or nil if none declares one.
+func domainRecordIdentity(d *models.DomainConfig) func(*models.RecordConfig) string {
+	for _, provider := range d.DNSProviderInstances {
+		if provider.ProviderType == "-" {
+			continue
+		}
+		if f := providers.GetRecordIdentity(provider.ProviderType); f != nil {
+			return f
+		}
+	}
+	return nil
+}
+
+// providerIdentity returns the provider-declared identity text for a record,
+// or "" when no provider declares one.
+func providerIdentity(r *models.RecordConfig, extra func(*models.RecordConfig) string) string {
+	if extra == nil {
+		return ""
+	}
+	return extra(r)
+}
+
+func checkCNAMEs(dc *models.DomainConfig, extra func(*models.RecordConfig) string) (errs []error) {
 	cnames := map[string]bool{}
 	proxiedCnames := map[string]bool{}
+	seenIdentity := map[string]bool{}
 	for _, r := range dc.Records {
 		if r.Type == "CNAME" {
-			if cnames[r.GetLabel()] {
+			// Without a provider-declared identity this is exactly the old
+			// rule: one CNAME per label. With one, two CNAMEs may share a
+			// label as long as the provider treats them as separate objects.
+			id := r.GetLabel()
+			if x := providerIdentity(r, extra); x != "" {
+				id += "|" + x
+			}
+			if seenIdentity[id] {
 				errs = append(errs, fmt.Errorf("%s: cannot have multiple CNAMEs with same name: %s", r.FilePos, r.GetLabelFQDN()))
 			}
+			seenIdentity[id] = true
 			cnames[r.GetLabel()] = true
 			if p, ok := r.Metadata["cloudflare_proxy"]; ok && (p == "on" || p == "full") {
 				proxiedCnames[r.GetLabel()] = true
@@ -677,10 +721,10 @@ func checkMultipleSOAs(dc *models.DomainConfig) (errs []error) {
 	return
 }
 
-func checkDuplicates(records models.Records) (errs []error) {
+func checkDuplicates(records models.Records, extra func(*models.RecordConfig) string) (errs []error) {
 	seen := make(map[string]*models.RecordConfig)
 	for _, r := range records {
-		diffable := fmt.Sprintf("%s %s %s", r.GetLabelFQDN(), r.Type, r.ComparableV3)
+		diffable := recordIdentityString(r, extra)
 
 		if seen[diffable] != nil {
 			errs = append(errs, fmt.Errorf("exact duplicate record found: %s", diffable))

pkg/normalize/validate_identity_test.go

@@ -0,0 +1,198 @@
+package normalize
+
+import (
+	"strings"
+	"testing"
+
+	dnsv2 "codeberg.org/miekg/dns"
+
+	"github.com/DNSControl/dnscontrol/v5/models"
+	"github.com/DNSControl/dnscontrol/v5/pkg/providers"
+	_ "github.com/DNSControl/dnscontrol/v5/providers/tencentdns"
+)
+
+const lineZone = "example.com"
+
+// A provider that stores one record per line and declares no identity function
+// keeps the duplicate rules that existed before the hook.
+const plainProviderType = "TEST_NO_IDENTITY"
+
+func init() {
+	providers.RegisterDomainServiceProviderType(plainProviderType, providers.DspFuncs{}, providers.DocumentationNotes{})
+}
+
+// Four lines answer one name: two share a target, two point elsewhere.
+var lineRoutes = []struct {
+	line   string
+	target string
+}{
+	{"0", "origin.example.net."},
+	{"10=1", "origin.example.net."},
+	{"10=3", "backup.example.net."},
+	{"10=2", "backup.example.net."},
+}
+
+func lineDomain(providerType string) *models.DomainConfig {
+	dc := models.MustNewDomainConfig(lineZone)
+	dc.RegistrarName = "NONE"
+	dc.DNSProviderNames = map[string]int{"lines": 1}
+	dc.DNSProviderInstances = []*models.DNSProviderInstance{{
+		Name:         "lines",
+		ProviderType: providerType,
+	}}
+	return dc
+}
+
+func addLineRecords(dc *models.DomainConfig, rType uint16, target string) {
+	for _, route := range lineRoutes {
+		r := dc.MustNewRecordConfig("edge", 60, rType, target)
+		r.Metadata["tencentdns_line_id"] = route.line
+		dc.AddRecordConfig(r)
+	}
+}
+
+func validateDomain(t *testing.T, dc *models.DomainConfig) []error {
+	t.Helper()
+	return ValidateAndNormalizeConfig(&models.DNSConfig{
+		Domains: []*models.DomainConfig{dc},
+	})
+}
+
+func TestPerLineCNAMEsShareOneName(t *testing.T) {
+	dc := lineDomain("TENCENTDNS")
+	for _, route := range lineRoutes {
+		r := dc.MustNewRecordConfig("edge", 60, dnsv2.TypeCNAME, route.target)
+		r.Metadata["tencentdns_line_id"] = route.line
+		dc.AddRecordConfig(r)
+	}
+
+	if errs := validateDomain(t, dc); len(errs) != 0 {
+		t.Fatalf("expected no validation errors, got %v", errs)
+	}
+}
+
+func TestPerLineAddressesMayShareOneTarget(t *testing.T) {
+	dc := lineDomain("TENCENTDNS")
+	addLineRecords(dc, dnsv2.TypeA, "192.0.2.10")
+
+	if errs := validateDomain(t, dc); len(errs) != 0 {
+		t.Fatalf("expected no validation errors, got %v", errs)
+	}
+}
+
+func TestPerLineRecordsStillFailWithoutProviderIdentity(t *testing.T) {
+	if _, ok := providers.DNSProviderTypes[plainProviderType]; !ok {
+		t.Fatalf("test setup: %s is not registered", plainProviderType)
+	}
+	if providers.GetRecordIdentity(plainProviderType) != nil {
+		t.Fatalf("test setup: %s declares an identity function", plainProviderType)
+	}
+	dc := lineDomain(plainProviderType)
+	addLineRecords(dc, dnsv2.TypeA, "192.0.2.10")
+
+	errs := validateDomain(t, dc)
+	if len(errs) == 0 {
+		t.Fatal("expected duplicate errors for a provider that declares no record identity")
+	}
+	if !strings.Contains(errs[0].Error(), "exact duplicate record found") {
+		t.Fatalf("unexpected first error: %v", errs[0])
+	}
+}
+
+func TestTwoCNAMEsOnOneLineRemainAnError(t *testing.T) {
+	dc := lineDomain("TENCENTDNS")
+	first := dc.MustNewRecordConfig("edge", 60, dnsv2.TypeCNAME, lineRoutes[0].target)
+	first.Metadata["tencentdns_line_id"] = lineRoutes[0].line
+	dc.AddRecordConfig(first)
+	second := dc.MustNewRecordConfig("edge", 60, dnsv2.TypeCNAME, "other.example.net.")
+	second.Metadata["tencentdns_line_id"] = lineRoutes[0].line
+	dc.AddRecordConfig(second)
+
+	errs := validateDomain(t, dc)
+	if len(errs) == 0 {
+		t.Fatal("expected an error for two CNAMEs on the same line")
+	}
+	if !strings.Contains(errs[0].Error(), "cannot have multiple CNAMEs with same name") {
+		t.Fatalf("unexpected first error: %v", errs[0])
+	}
+}
+
+func TestWeightDoesNotSplitTheIdentity(t *testing.T) {
+	dc := lineDomain("TENCENTDNS")
+	for _, weight := range []string{"10", "20"} {
+		r := dc.MustNewRecordConfig("weighted", 60, dnsv2.TypeA, "203.0.113.10")
+		r.Metadata["tencentdns_line_id"] = "10=1"
+		r.Metadata["tencentdns_weight"] = weight
+		dc.AddRecordConfig(r)
+	}
+
+	errs := validateDomain(t, dc)
+	if len(errs) == 0 {
+		t.Fatal("expected duplicate detection: the service keys records without weight")
+	}
+	if !strings.Contains(errs[0].Error(), "exact duplicate record found") {
+		t.Fatalf("unexpected first error: %v", errs[0])
+	}
+}
+
+func TestLineNamesAloneAlsoCarryIdentity(t *testing.T) {
+	dc := lineDomain("TENCENTDNS")
+	for _, line := range []string{"电信", "联通"} {
+		r := dc.MustNewRecordConfig("named", 60, dnsv2.TypeA, "203.0.113.10")
+		r.Metadata["tencentdns_line"] = line
+		dc.AddRecordConfig(r)
+	}
+
+	if errs := validateDomain(t, dc); len(errs) != 0 {
+		t.Fatalf("expected no validation errors, got %v", errs)
+	}
+}
+
+// A record without line metadata answers on the default line, so it is the same
+// record as one that names the default line explicitly.
+func TestDefaultLineMatchesAnExplicitDefaultLine(t *testing.T) {
+	dc := lineDomain("TENCENTDNS")
+	dc.AddRecordConfig(dc.MustNewRecordConfig("default-host", 60, dnsv2.TypeA, "192.0.2.10"))
+	explicit := dc.MustNewRecordConfig("default-host", 60, dnsv2.TypeA, "192.0.2.10")
+	explicit.Metadata["tencentdns_line_id"] = "0"
+	dc.AddRecordConfig(explicit)
+
+	errs := validateDomain(t, dc)
+	if len(errs) == 0 {
+		t.Fatal("expected duplicate detection: no line means the default line")
+	}
+	if !strings.Contains(errs[0].Error(), "exact duplicate record found") {
+		t.Fatalf("unexpected first error: %v", errs[0])
+	}
+}
+
+// The default line has both a name and an ID, and both describe one record.
+func TestDefaultLineNameMatchesTheDefaultLineID(t *testing.T) {
+	dc := lineDomain("TENCENTDNS")
+	dc.AddRecordConfig(dc.MustNewRecordConfig("default-name", 60, dnsv2.TypeA, "192.0.2.10"))
+	named := dc.MustNewRecordConfig("default-name", 60, dnsv2.TypeA, "192.0.2.10")
+	named.Metadata["tencentdns_line"] = "默认"
+	dc.AddRecordConfig(named)
+
+	errs := validateDomain(t, dc)
+	if len(errs) == 0 {
+		t.Fatal("expected duplicate detection: the default line has one name and one ID")
+	}
+}
+
+// Validation runs before the provider reads the zone, so it cannot resolve a
+// line name into a line ID. A configuration that describes one line in both
+// styles passes here, and the service rejects the duplicate at push time.
+func TestMixedLineStylesPassValidation(t *testing.T) {
+	dc := lineDomain("TENCENTDNS")
+	byName := dc.MustNewRecordConfig("mixed", 60, dnsv2.TypeA, "192.0.2.10")
+	byName.Metadata["tencentdns_line"] = "电信"
+	dc.AddRecordConfig(byName)
+	byID := dc.MustNewRecordConfig("mixed", 60, dnsv2.TypeA, "192.0.2.10")
+	byID.Metadata["tencentdns_line_id"] = "10=1"
+	dc.AddRecordConfig(byID)
+
+	if errs := validateDomain(t, dc); len(errs) != 0 {
+		t.Fatalf("expected no validation errors, got %v", errs)
+	}
+}

pkg/normalize/validate_test.go

@@ -322,7 +322,7 @@ func TestCNAMECloudflareProxied(t *testing.T) {
 	dc.AddRecordConfig(recCNAME)
 	recMX := dc.MustNewRecordConfig("mail", 0, dnsv2.TypeMX, 10, "smtp.example.com.")
 	dc.AddRecordConfig(recMX)
-	errs := checkCNAMEs(dc)
+	errs := checkCNAMEs(dc, nil)
 	if len(errs) != 0 {
 		t.Errorf("Expected no errors for proxied CNAME + MX, got: %v", errs)
 	}
@@ -334,7 +334,7 @@ func TestCNAMECloudflareProxied(t *testing.T) {
 	recMX2 := dc2.MustNewRecordConfig("mail", 0, dnsv2.TypeMX, 10, "smtp.example.com.")
 	dc2.AddRecordConfig(recCNAME2)
 	dc2.AddRecordConfig(recMX2)
-	errs2 := checkCNAMEs(dc2)
+	errs2 := checkCNAMEs(dc2, nil)
 	if len(errs2) == 0 {
 		t.Error("Expected error for non-proxied CNAME + MX, got none")
 	}
@@ -355,7 +355,7 @@ func TestCheckDuplicates(t *testing.T) {
 	dc.AddTestRC(t, "@", 0, dnsv2.TypeNS, "ns3.foo.com.")
 
 	// NOTE: The comparison ignores ttl. Therefore we don't test that.
-	errs := checkDuplicates(dc.Records)
+	errs := checkDuplicates(dc.Records, nil)
 	if len(errs) != 0 {
 		t.Errorf("Expected duplicate NOT found but found %q", errs)
 	}
@@ -368,7 +368,7 @@ func TestCheckDuplicates_dup_a(t *testing.T) {
 	dc.AddTestRC(t, "@", 0, dnsv2.TypeA, "1.1.1.1")
 	dc.AddTestRC(t, "@", 0, dnsv2.TypeA, "1.1.1.1")
 
-	errs := checkDuplicates(dc.Records)
+	errs := checkDuplicates(dc.Records, nil)
 	if len(errs) == 0 {
 		t.Error("Expect duplicate found but found none")
 	}
@@ -383,7 +383,7 @@ func TestCheckDuplicates_dup_ns(t *testing.T) {
 	dc.AddTestRC(t, "@", 0, dnsv2.TypeNS, "ns2.foo.com.")
 	dc.AddTestRC(t, "@", 0, dnsv2.TypeNS, "ns2.foo.com.")
 
-	errs := checkDuplicates(dc.Records)
+	errs := checkDuplicates(dc.Records, nil)
 	if len(errs) == 0 {
 		t.Error("Expect duplicate found but found none")
 	}