diff --git a/documentation/advanced-features/writing-providers.md b/documentation/advanced-features/writing-providers.md index e7f417a63b..69124b886e 100644 --- a/documentation/advanced-features/writing-providers.md +++ b/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. diff --git a/documentation/provider/tencentdns.md b/documentation/provider/tencentdns.md index 0f36991e65..751c8187ea 100644 --- a/documentation/provider/tencentdns.md +++ b/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. diff --git a/pkg/normalize/validate.go b/pkg/normalize/validate.go index 9ba0079e8b..e50571d490 100644 --- a/pkg/normalize/validate.go +++ b/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)) diff --git a/pkg/normalize/validate_identity_test.go b/pkg/normalize/validate_identity_test.go new file mode 100644 index 0000000000..0ed329e8aa --- /dev/null +++ b/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) + } +} diff --git a/pkg/normalize/validate_test.go b/pkg/normalize/validate_test.go index d539ed6568..8c5fe1ae88 100644 --- a/pkg/normalize/validate_test.go +++ b/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") } diff --git a/pkg/providers/providers.go b/pkg/providers/providers.go index 4f33d86f55..21a79bf412 100644 --- a/pkg/providers/providers.go +++ b/pkg/providers/providers.go @@ -50,11 +50,30 @@ type DspInitializerWithOptions func(map[string]string, json.RawMessage, CreateOp // detailing records that this provider can not support. type RecordAuditor func(models.Records) []error +// RecordIdentityFunc returns the identity text of a record: text that +// distinguishes two records which share a label, rType and RDATA but are stored +// by the provider as separate objects (DNSPod record lines, Route 53 routing +// policies). Validation uses it for duplicate detection, and validation runs +// before the provider has read the zone, so the function must depend only on +// the record it is given. +type RecordIdentityFunc func(*models.RecordConfig) string + // DspFuncs lists functions registered with a provider. type DspFuncs struct { Initializer DspInitializer InitializerWithOptions DspInitializerWithOptions RecordAuditor RecordAuditor + RecordIdentity RecordIdentityFunc +} + +// GetRecordIdentity returns the provider's RecordIdentity function, or nil if +// the provider does not declare one. +func GetRecordIdentity(dType string) RecordIdentityFunc { + p, ok := DNSProviderTypes[dType] + if !ok { + return nil + } + return p.RecordIdentity } // DNSProviderTypes stores initializer for each DSP. diff --git a/providers/tencentdns/tencentdnsProvider.go b/providers/tencentdns/tencentdnsProvider.go index 718c0f5868..5e4853ba99 100644 --- a/providers/tencentdns/tencentdnsProvider.go +++ b/providers/tencentdns/tencentdnsProvider.go @@ -37,8 +37,9 @@ func init() { const providerName = "TENCENTDNS" const providerMaintainer = "@cylonchau" fns := providers.DspFuncs{ - Initializer: newTencentDNSDsp, - RecordAuditor: AuditRecords, + Initializer: newTencentDNSDsp, + RecordAuditor: AuditRecords, + RecordIdentity: recordIdentity, } providers.RegisterDomainServiceProviderType(providerName, fns, features) providers.RegisterRegistrarType(providerName, newTencentDNSReg) @@ -242,6 +243,27 @@ func recordMetadataComparable(existingRecords models.Records) diff2.ComparableFu } } +// recordIdentity returns the identity text used by validation-time duplicate +// detection. Validation runs before the provider has read the zone, so this +// reads nothing but the record itself: the line ID when set, otherwise the line +// name. Weight stays out, because it is not part of the key the service uses. +func recordIdentity(rc *models.RecordConfig) string { + if rc.Metadata != nil { + if lineID := rc.Metadata[metaRecordLineID]; lineID != "" { + return "line_id=" + lineID + } + if line := rc.Metadata[metaRecordLine]; line != "" { + // The default line has one name and one ID. + if line == defaultRecordLine { + return "line_id=" + defaultRecordLineID + } + return "line=" + line + } + } + // A record without line metadata answers on the default line. + return "line_id=" + defaultRecordLineID +} + func (p *tencentdnsProvider) GetZoneRecordsCorrections(dc *models.DomainConfig, existingRecords models.Records) ([]*models.Correction, int, error) { var corrections []*models.Correction diff --git a/providers/tencentdns/tencentdnsProvider_test.go b/providers/tencentdns/tencentdnsProvider_test.go index e8856aa025..49b2967c08 100644 --- a/providers/tencentdns/tencentdnsProvider_test.go +++ b/providers/tencentdns/tencentdnsProvider_test.go @@ -325,6 +325,21 @@ func makeLineRecord(domain, target string, metadata map[string]string) *models.R return rc } +// The validation identity must read nothing but the record it is given. A +// record without line metadata answers on the default line, and weight stays +// out because the service keys records without it. +func TestRecordIdentityReadsOnlyTheRecord(t *testing.T) { + assert.Equal(t, "line_id=0", recordIdentity(makeLineRecord("example.com", "1.2.3.4", nil))) + assert.Equal(t, "line_id=10=1", recordIdentity(makeLineRecord("example.com", "1.2.3.4", + map[string]string{metaRecordLineID: "10=1"}))) + assert.Equal(t, "line=电信", recordIdentity(makeLineRecord("example.com", "1.2.3.4", + map[string]string{metaRecordLine: "电信"}))) + assert.Equal(t, "line_id=10=1", recordIdentity(makeLineRecord("example.com", "1.2.3.4", + map[string]string{metaRecordLineID: "10=1", metaRecordWeight: "10"}))) + assert.Equal(t, "line_id=10=1", recordIdentity(makeLineRecord("example.com", "1.2.3.4", + map[string]string{metaRecordLineID: "10=1", metaRecordWeight: "20"}))) +} + func TestMinTTLForGrade(t *testing.T) { packages := []*dnspod.PackageDetailItem{ {