From 2cbabd859b80eb239e30fab36e418f6fe24b5c4d Mon Sep 17 00:00:00 2001 From: Tom Limoncelli Date: Fri, 4 Aug 2017 12:26:29 -0700 Subject: [PATCH] Make it easier to add new Rtypes. (#169) * NEW: docs/adding-new-rtypes.md * Mark all "if" and "switch" statements with `#rtype_variations` * Make consistent use of `default: panic()` --- cmd/convertzone/main.go | 2 +- docs/adding-new-rtypes.md | 150 ++++++++++++++++++++++++++++++++ docs/index.md | 37 +++++--- models/dns.go | 23 ++++- pkg/normalize/validate.go | 4 +- providers/activedir/domains.go | 10 ++- providers/bind/bindProvider.go | 2 +- providers/bind/prettyzone.go | 4 +- providers/namedotcom/records.go | 8 +- 9 files changed, 213 insertions(+), 27 deletions(-) create mode 100644 docs/adding-new-rtypes.md diff --git a/cmd/convertzone/main.go b/cmd/convertzone/main.go index 5f67bc407..8b4feea6b 100644 --- a/cmd/convertzone/main.go +++ b/cmd/convertzone/main.go @@ -109,7 +109,7 @@ func rrFormat(zonename string, filename string, r io.Reader, defaultTTL uint32, if !dsl { // TSV format: fmt.Printf("%s\t%s\t%s\t%s\t%s\n", name, ttl, classStr, typeStr, target) } else { // DSL format: - switch hdr.Rrtype { + switch hdr.Rrtype { // #rtype_variations case dns.TypeMX: m := strings.SplitN(target, "\t", 2) target = m[0] + ", '" + m[1] + "'" diff --git a/docs/adding-new-rtypes.md b/docs/adding-new-rtypes.md new file mode 100644 index 000000000..63a65fc9d --- /dev/null +++ b/docs/adding-new-rtypes.md @@ -0,0 +1,150 @@ +--- +layout: default +--- + +# Creating new DNS Resource Types (Rtypes) + +Everyone is familiar with A, AAAA, CNAME, NS and other Rtypes. +However there are new record types being added all the time (possibly +too many). Each new record type requires special handling by +DNSControl. + +If a record simply has a single "target", then there is little to +do because it is handled similarly to A, CNAME, and so on. However +if there are multiple fields within the record you have more work +to do. + +Our general philosophy is: + +* Internally the individual fields of a record are kept separate. If a particular provider combines them into one big string, that kind of thing is done in the provider code at the end of the food chain. For example, an MX record has a Target (`aspmx.l.google.com.`) and a preference (`10`). Some systems combine this into one string (`10 aspmx.l.google.com.`). We keep the two values separate in `RecordConfig` and leave it up to the individual providers to merge them when required. An earlier implementation kept everything combined and we found ourselves constantly parsing and re-parsing the target. It was inefficient and lead to many bugs. +* Anywhere we have a special case for a particular Rtype, we use a `switch` statement and have a `case` for every single record type, usually with a `default:` case that calls `panic()`. This way developers adding a new record type will quickly find where they need to add code (the panic will tell them where). Before we did this, missing implementation code would go unnoticed for months. +* Keep things alphabetical. If you are adding your record type to a case statement, function library, or whatever, please list it alphabetically along with the others when possible. + +## Step 1: Update `RecordConfig` in `models/dns.go` + +If the record has any unique fields, add them to `RecordConfig`. +The field name should be the record type, then the field name as +used in `github.com/miekg/dns/types.go`. For example, the `CAA` +record has a field called `Flag`, therefore the field name in +`RecordConfig` is CaaFlag (not `CaaFlags` or `CAAFlags`). + +Here are some examples: + +``` +type RecordConfig struct { + ... + MxPreference uint16 `json:"mxpreference,omitempty"` // FIXME(tlim): Rename to MxPreference + SrvPriority uint16 `json:"srvpriority,omitempty"` + SrvWeight uint16 `json:"srvweight,omitempty"` + SrvPort uint16 `json:"srvport,omitempty"` + CaaTag string `json:"caatag,omitempty"` + CaaFlag uint8 `json:"caaflag,omitempty"` + ... +} +``` + +## Step 2: Add a capability for the record + +You'll need to mark which providers support this record type. The +initial PR should implement this record for the `bind` provider at +a minimum. + +* Add the capability to the file `dnscontrol/providers/providers.go` (look for `CanUseAlias` and add +it to the end of the list.) +* Mark the `bind` provider as supporting this record type by updating `dnscontrol/providers/bind/bindProvider.go` (look for `providers.CanUs` and you'll see what to do). + +## Step 2: Add a helper function + +Add a function to `pkg/js/helpers.js` for the new record type. This +is the Javascript file that defines `dnsconfig.js`'s functions like +`A()` and `MX()`. Look at the definition of A, MX and CAA for good +examples to use as a base. + +Please add the function alphabetically with the others. + +## Step 3: Search for `#rtype_variations` + +Anywhere a rtype requires special handling has been marked with a +comment that includes the string `#rtype_variations`. Search for +this string and add your new type to this code. + +## Step 4: Add a `parse_tests` test case. + +Add at least one test case to the `pkg/js/parse_tests` directory. +Test `013-mx.js` is a very simple one and is good for cloning. + +Run these tests via: + + cd dnscontrol/pkg/js + go test ./... + +If this works, then you know the `dnsconfig.js` and `helpers.js` +code is working correctly. + +As you debug, if there are places that haven't been marked +`#rtype_variations` that should be, add such a comment. +Every time you do this, an angel gets its wings. + +## Step 5: Add an `integrationTest` test case. + +Add at least one test case to the `integrationTest/integration_test.go` +file. Look for `var tests =` and add the test to the end of this +list. + +Each entry in the list is a new state. For example: + +``` + //MX + tc("Empty"), <<< 1 + tc("MX record", mx("@", 5, "foo.com.")), <<< 2 + tc("Change MX pref", mx("@", 10, "foo.com.")), <<< 3 +``` + +Line 1: An `tc()` entry with no records (just a comment). The test +system will delete all records from the domain to make the domain +match this empty configuration. This creates a "clean slate" +situation. + +Line 2: A `tc()` entry with 1 record. To get to this state, the +provider will have to add the record. If this works, basic functionality +for the MX record type has been achieved. + +Line 3: A `tc()` entry with 1 record, with a different priority. +To get to this state, the provider will have to either change the +priority on an existing record, or delete the old record and insert +a new one. Either way, this test case assures us that the diff'ing +functionality is working properly. + +If you look at the tests for `CAA`, it inserts a few records then +attempts to modify each field of a record one at a time. This test +was useful because it turns out we hadn't written the code to +properly see a change in priority. We fixed this bug before the +code made it into production. + +Also notice that some tests include `.IfHasCapability()`. This +limits the test to providers with certain capabilities. You'll +want to use this feature so that the tests only run on providers +that support your new record type. + +To run the integration test with the BIND provider: + + cd dnscontrol/integrationTest + go test -v -verbose -provider BIND + +Once the code works for BIND, consider submitting a PR at this point. + +As you debug, if there are places that haven't been marked +`#rtype_variations` that should be, add such a comment. +If you fail to do this, God kills a cute little kitten. + +## Step 6: Support more providers + +Now add support other providers. Add the `providers.CanUse...` +flag to the provider and re-run the integration tests: + +For example, this will run the tests on Amazon AWS Route53: + + export R53_DOMAIN=dnscontroltest-r53.com # Use a test domain. + export R53_KEY_ID=CHANGE_TO_THE_ID + export R53_KEY='CHANGE_TO_THE_KEY' + go test -v -verbose -provider ROUTE53 diff --git a/docs/index.md b/docs/index.md index 0e994f7e2..e3aba151a 100644 --- a/docs/index.md +++ b/docs/index.md @@ -5,24 +5,35 @@ layout: default Dnscontrol is a platform for seamlessly managing your dns configuration across any number of DNS hosts, both in the cloud or in your own infrastructure. It manages all of the domains for the Stack Exchange network. -## [Getting Started]({{site.github.url}}/getting-started): A walk-through of the basics. +## Getting Started -## [Providers]({{site.github.url}}/provider-list): Which DNS providers are supported. +### [Getting Started]({{site.github.url}}/getting-started): A walk-through of the basics. -## [Language Reference]({{site.github.url}}/js): Description of the DNSControl language (DSL). +### [Providers]({{site.github.url}}/provider-list): Which DNS providers are supported. -## [Examples]({{site.github.url}}/examples): The DNSControl language by example. +### [Examples]({{site.github.url}}/examples): The DNSControl language by example. -## [Migrating]({{site.github.url}}/migrating): Migrating zones to DNSControl. - -## [Testing]({{site.github.url}}/unittests): Unit Testing DNS Data. - -## [github](https://github.com/StackExchange/dnscontrol): Get the source! +### [Migrating]({{site.github.url}}/migrating): Migrating zones to DNSControl. +## Reference -## FAQs and design notes +### [Language Reference]({{site.github.url}}/js): Description of the DNSControl language (DSL). -- [Why CNAME/MX/NS targets require a trailing "dot"]({{site.github.url}}/why-the-dot) -- [Writing Providers]({{site.github.url}}/writing-providers) -- [ALIAS records in dnscontrol]({{site.github.url}}/alias) +### [ALIAS / ANAME records in dnscontrol]({{site.github.url}}/alias) + +### [Why CNAME/MX/NS targets require a trailing "dot"]({{site.github.url}}/why-the-dot) + + +## Advanced Usage + +### [Testing]({{site.github.url}}/unittests): Unit Testing for you DNS Data. + + +## Developer info + +### [github](https://github.com/StackExchange/dnscontrol): Get the source! + +### [Writing Providers]({{site.github.url}}/writing-providers) + +### [Adding new DNS record types]({{site.github.url}}/adding-new-rtypes) diff --git a/models/dns.go b/models/dns.go index c40a4ec5f..446388180 100644 --- a/models/dns.go +++ b/models/dns.go @@ -85,7 +85,9 @@ func (r *RecordConfig) String() (content string) { } content = fmt.Sprintf("%s %s %s %d", r.Type, r.NameFQDN, r.Target, r.TTL) - switch r.Type { + switch r.Type { // #rtype_variations + case "A", "AAAA", "CNAME", "PTR", "TXT": + // Nothing special. case "MX": content += fmt.Sprintf(" priority=%d", r.MxPreference) case "SOA": @@ -93,7 +95,10 @@ func (r *RecordConfig) String() (content string) { case "CAA": content += fmt.Sprintf(" caatag=%s caaflag=%d", r.CaaTag, r.CaaFlag) default: - // assume nothing special for A,CNAME,AAAA, and other simple types. + msg := fmt.Sprintf("rc.String rtype %v unimplemented", r.Type) + panic(msg) + // We panic so that we quickly find any switch statements + // that have not been updated for a new RR type. } for k, v := range r.Metadata { content += fmt.Sprintf(" %s=%s", k, v) @@ -168,7 +173,7 @@ func (rc *RecordConfig) ToRR() dns.RR { } // Fill in the data. - switch rdtype { + switch rdtype { // #rtype_variations case dns.TypeA: rr.(*dns.A).A = net.ParseIP(rc.Target) case dns.TypeAAAA: @@ -205,6 +210,8 @@ func (rc *RecordConfig) ToRR() dns.RR { rr.(*dns.TXT).Txt = []string{rc.Target} default: panic(fmt.Sprintf("ToRR: Unimplemented rtype %v", rc.Type)) + // We panic so that we quickly find any switch statements + // that have not been updated for a new RR type. } return rr @@ -269,11 +276,19 @@ func (dc *DomainConfig) Punycode() error { if err != nil { return err } - if rec.Type == "MX" || rec.Type == "CNAME" { + switch rec.Type { // #rtype_variations + case "ALIAS", "MX", "NS", "CNAME", "SRV": rec.Target, err = idna.ToASCII(rec.Target) if err != nil { return err } + case "A", "CAA": + // Nothing to do. + default: + msg := fmt.Sprintf("Punycode rtype %v unimplemented", rec.Type) + panic(msg) + // We panic so that we quickly find any switch statements + // that have not been updated for a new RR type. } } return nil diff --git a/pkg/normalize/validate.go b/pkg/normalize/validate.go index 380fd846e..be6989875 100644 --- a/pkg/normalize/validate.go +++ b/pkg/normalize/validate.go @@ -127,7 +127,7 @@ func checkTargets(rec *models.RecordConfig, domain string) (errs []error) { errs = append(errs, err) } } - switch rec.Type { + switch rec.Type { // #rtype_variations case "A": check(checkIPv4(target)) case "AAAA": @@ -192,7 +192,7 @@ func importTransform(srcDomain, dstDomain *models.DomainConfig, transforms []tra } return rec2 } - switch rec.Type { + switch rec.Type { // #rtype_variations case "A": trs, err := transform.TransformIPToList(net.ParseIP(rec.Target), transforms) if err != nil { diff --git a/providers/activedir/domains.go b/providers/activedir/domains.go index 3bfec4514..cf02fb066 100644 --- a/providers/activedir/domains.go +++ b/providers/activedir/domains.go @@ -165,7 +165,7 @@ func (r *RecordConfigJson) unpackRecord(origin string) *models.RecordConfig { rc.Type = r.Type rc.TTL = r.TTL - switch rc.Type { + switch rc.Type { // #rtype_variations case "A": rc.Target = r.Data case "CNAME": @@ -200,7 +200,7 @@ func (c *adProvider) generatePowerShellCreate(domainname string, rec *models.Rec text += fmt.Sprintf(` -ZoneName "%s"`, domainname) text += fmt.Sprintf(` -Name "%s"`, rec.Name) text += fmt.Sprintf(` -TimeToLive $(New-TimeSpan -Seconds %d)`, rec.TTL) - switch rec.Type { + switch rec.Type { // #rtype_variations case "CNAME": text += fmt.Sprintf(` -HostNameAlias "%s"`, content) case "A": @@ -209,6 +209,8 @@ func (c *adProvider) generatePowerShellCreate(domainname string, rec *models.Rec text = fmt.Sprintf("\r\n"+`echo "Skipping NS update (%v %v)"`+"\r\n", rec.Name, rec.Target) default: panic(fmt.Errorf("ERROR: generatePowerShellCreate() does not yet handle recType=%s recName=%#v content=%#v)\n", rec.Type, rec.Name, content)) + // We panic so that we quickly find any switch statements + // that have not been updated for a new RR type. } text += "\r\n" @@ -220,7 +222,7 @@ func (c *adProvider) generatePowerShellModify(domainname, recName, recType, oldC var queryField, queryContent string - switch recType { + switch recType { // #rtype_variations case "A": queryField = "IPv4address" queryContent = `"` + oldContent + `"` @@ -229,6 +231,8 @@ func (c *adProvider) generatePowerShellModify(domainname, recName, recType, oldC queryContent = `"` + oldContent + `"` default: panic(fmt.Errorf("ERROR: generatePowerShellModify() does not yet handle recType=%s recName=%#v content=(%#v, %#v)\n", recType, recName, oldContent, newContent)) + // We panic so that we quickly find any switch statements + // that have not been updated for a new RR type. } text := "\r\n" // Skip a line. diff --git a/providers/bind/bindProvider.go b/providers/bind/bindProvider.go index b8d1fffda..4600128d1 100644 --- a/providers/bind/bindProvider.go +++ b/providers/bind/bindProvider.go @@ -88,7 +88,7 @@ func rrToRecord(rr dns.RR, origin string, replaceSerial uint32) (models.RecordCo rc.NameFQDN = strings.ToLower(strings.TrimSuffix(header.Name, ".")) rc.Name = strings.ToLower(dnsutil.TrimDomainName(header.Name, origin)) rc.TTL = header.Ttl - switch v := rr.(type) { + switch v := rr.(type) { // #rtype_variations case *dns.A: rc.Target = v.A.String() case *dns.AAAA: diff --git a/providers/bind/prettyzone.go b/providers/bind/prettyzone.go index 8633fa3c5..6ddd82a89 100644 --- a/providers/bind/prettyzone.go +++ b/providers/bind/prettyzone.go @@ -39,7 +39,7 @@ func (z *zoneGenData) Less(i, j int) bool { if rrtypeA != rrtypeB { return zoneRrtypeLess(rrtypeA, rrtypeB) } - switch rrtypeA { + switch rrtypeA { // #rtype_variations case dns.TypeNS, dns.TypeTXT: // pass through. case dns.TypeA: @@ -82,6 +82,8 @@ func (z *zoneGenData) Less(i, j int) bool { } default: panic(fmt.Sprintf("zoneGenData Less: unimplemented rtype %v", dns.TypeToString[rrtypeA])) + // We panic so that we quickly find any switch statements + // that have not been updated for a new RR type. } return a.String() < b.String() } diff --git a/providers/namedotcom/records.go b/providers/namedotcom/records.go index 61b87842d..9507d6032 100644 --- a/providers/namedotcom/records.go +++ b/providers/namedotcom/records.go @@ -105,7 +105,7 @@ func (r *nameComRecord) toRecord() *models.RecordConfig { TTL: uint32(ttl), Original: r, } - switch r.Type { + switch r.Type { // #rtype_variations case "A", "AAAA", "ANAME", "CNAME", "NS", "TXT": // nothing additional. case "MX": @@ -121,6 +121,8 @@ func (r *nameComRecord) toRecord() *models.RecordConfig { rc.Target = parts[2] + "." default: panic(fmt.Sprintf("toRecord unimplemented rtype %v", r.Type)) + // We panic so that we quickly find any switch statements + // that have not been updated for a new RR type. } return rc } @@ -173,7 +175,7 @@ func (n *nameDotCom) createRecord(rc *models.RecordConfig, domain string) error if dat.Hostname == "@" { dat.Hostname = "" } - switch rc.Type { + switch rc.Type { // #rtype_variations case "A", "AAAA", "ANAME", "CNAME", "MX", "NS", "TXT": // nothing case "SRV": @@ -181,6 +183,8 @@ func (n *nameDotCom) createRecord(rc *models.RecordConfig, domain string) error dat.Priority = rc.SrvPriority default: panic(fmt.Sprintf("createRecord rtype %v unimplemented", rc.Type)) + // We panic so that we quickly find any switch statements + // that have not been updated for a new RR type. } resp, err := n.post(n.apiCreateRecord(domain), dat) if err != nil {