feat(provider): rfc2136 - #1192
Conversation
qdm12
left a comment
There was a problem hiding this comment.
Nice PR 💪 🎖️ Just some minor comments 😉 thanks!!!
| "domain": "domain.com", | ||
| "owner": "@", |
There was a problem hiding this comment.
the code now understands the owner/domain from just the domain string
| "domain": "domain.com", | |
| "owner": "@", | |
| "domain": "domain.com", |
| ### Compulsory parameters | ||
|
|
||
| - `"domain"` is the domain to update. It can be `example.com` (root domain) or `sub.example.com` (subdomain of `example.com`). | ||
| - `"server"` is the address of the name server accepting the dynamic updates. The port defaults to `53` if it is not specified. IPv6 addresses must be enclosed in square brackets, for example `[2001:db8::1]:53`. |
There was a problem hiding this comment.
to simplify things let's just enforce specifying a port always
| // addressWithDefaultPort adds the default DNS port to the server address | ||
| // given, if the address does not already specify a port. | ||
| func addressWithDefaultPort(server string) (address string) { |
There was a problem hiding this comment.
let's enforce to specify the port, to simplify code/documentation/usage to a single way to do it
| // TCP is used instead of UDP to avoid truncated responses, since | ||
| // updates are infrequent enough for the extra round trips not to matter. |
| header := dns.RR_Header{ | ||
| Name: dns.Fqdn(utils.BuildDomainName(p.owner, p.domain)), | ||
| Rrtype: dns.TypeA, | ||
| Class: dns.ClassINET, | ||
| Ttl: p.ttl, | ||
| } | ||
|
|
||
| var record dns.RR | ||
| if ip.Is6() { | ||
| header.Rrtype = dns.TypeAAAA | ||
| record = &dns.AAAA{Hdr: header, AAAA: net.IP(ip.AsSlice())} | ||
| } else { | ||
| record = &dns.A{Hdr: header, A: net.IP(ip.AsSlice())} |
There was a problem hiding this comment.
nit just for simplicity/easier reading
| header := dns.RR_Header{ | |
| Name: dns.Fqdn(utils.BuildDomainName(p.owner, p.domain)), | |
| Rrtype: dns.TypeA, | |
| Class: dns.ClassINET, | |
| Ttl: p.ttl, | |
| } | |
| var record dns.RR | |
| if ip.Is6() { | |
| header.Rrtype = dns.TypeAAAA | |
| record = &dns.AAAA{Hdr: header, AAAA: net.IP(ip.AsSlice())} | |
| } else { | |
| record = &dns.A{Hdr: header, A: net.IP(ip.AsSlice())} | |
| header := dns.RR_Header{ | |
| Name: dns.Fqdn(utils.BuildDomainName(p.owner, p.domain)), | |
| Class: dns.ClassINET, | |
| Ttl: p.ttl, | |
| } | |
| var record dns.RR | |
| if ip.Is6() { | |
| header.Rrtype = dns.TypeAAAA | |
| record = &dns.AAAA{Hdr: header, AAAA: net.IP(ip.AsSlice())} | |
| } else { | |
| header.Rrtype = dns.TypeA | |
| record = &dns.A{Hdr: header, A: net.IP(ip.AsSlice())} |
I was thinking initially uh oh this does not account for AAAA? 🤔 😄
| case dns.RcodeServerFailure: | ||
| return errors.ErrDNSServerSide | ||
| default: | ||
| return errors.ErrUnknownResponse |
There was a problem hiding this comment.
| return errors.ErrUnknownResponse | |
| return fmt.Errorf("%w", errors.ErrUnknownResponse) |
| return errors.ErrAuth | ||
| case dns.RcodeRefused: | ||
| return errors.ErrBadRequest | ||
| case dns.RcodeNotZone, dns.RcodeNameError: | ||
| return errors.ErrZoneNotFound | ||
| case dns.RcodeFormatError: | ||
| return errors.ErrBadRequest | ||
| case dns.RcodeServerFailure: | ||
| return errors.ErrDNSServerSide |
There was a problem hiding this comment.
nit use fmt.Errorf to enforce callers to use errors.Is instead of ==
| return errors.ErrAuth | |
| case dns.RcodeRefused: | |
| return errors.ErrBadRequest | |
| case dns.RcodeNotZone, dns.RcodeNameError: | |
| return errors.ErrZoneNotFound | |
| case dns.RcodeFormatError: | |
| return errors.ErrBadRequest | |
| case dns.RcodeServerFailure: | |
| return errors.ErrDNSServerSide | |
| return fmt.Errorf("%w", errors.ErrAuth) | |
| case dns.RcodeRefused: | |
| return fmt.Errorf("%w", errors.ErrBadRequest) | |
| case dns.RcodeNotZone, dns.RcodeNameError: | |
| return fmt.Errorf("%w", errors.ErrZoneNotFound) | |
| case dns.RcodeFormatError: | |
| return fmt.Errorf("%w", errors.ErrBadRequest) | |
| case dns.RcodeServerFailure: | |
| return fmt.Errorf("%w", errors.ErrDNSServerSide) |
| assert.Equal(t, testCase.expectedZone, provider.zone) | ||
| assert.Equal(t, testCase.expectedTTL, provider.ttl) | ||
| assert.Equal(t, testCase.expectedSrv, provider.server) | ||
| assert.Equal(t, testCase.expectedAlgo, provider.tsigAlgorithm) | ||
| assert.Equal(t, testCase.expectedKeyNm, provider.tsigKeyName) |
There was a problem hiding this comment.
nit assert the provider against an expectedProvider struct directly, no need to assert field by field
|
If you can also please merge in the master branch in your branch, there is a fix for the CI 🤷 thanks! |
- Update records on any authoritative name server accepting RFC 2136 dynamic DNS updates, such as BIND, Knot DNS or PowerDNS - Optionally sign updates with a TSIG key, or send them unsigned for servers authorizing by address ACL instead - Delete the record set and add the new record in a single message, so the server applies it atomically and creates the record if it does not exist yet - Send updates over TCP, since they are infrequent and it avoids truncated responses for signed messages - First provider not using HTTP, so the http client given to Update is unused; github.com/miekg/dns is already a direct dependency Closes qdm12#781 Closes qdm12#1155 Closes qdm12#325
- Require the server address to specify a port, instead of defaulting to 53 when it is missing - Drop the owner field from the documentation example, since the owner is parsed from the domain string - Set the record type in both branches building the record, so the AAAA case is easier to spot - Wrap the rcode errors with fmt.Errorf, to enforce callers using errors.Is - Assert the whole provider struct in the constructor test, instead of field by field
|
Thanks for your quick and thorough review @qdm12 - I rebased the end-to-end test locally and reran it also. I assume you don't want to have that in the repo? |
Adds a provider updating records over DNS UPDATE messages (RFC 2136), optionally signed with a TSIG key. It works against any authoritative name server accepting dynamic updates, such as BIND, Knot DNS or PowerDNS.
Closes #781
Closes #1155
Closes #325
#325 asks for Dyn's TSIG updates, which is RFC 2136 with the server pinned to
update.dyndns.com, so a configurable server covers it too.One thing worth flagging
This is the first provider that does not use HTTP, so the
*http.Clientpassed toUpdateis unused. No interface change was needed, but it is a precedent, so say the word if you would rather it were done differently.github.com/miekg/dnswas already a direct dependency, sogo.modis untouched.Design notes
zoneis optional and defaults todomain, which is right whenever the domain is the zone apex. It can be set for a domain sitting in a differently named zone.ErrAuth, REFUSED toErrBadRequest, NOTZONE/NXDOMAIN toErrZoneNotFound, SERVFAIL toErrDNSServerSide.Testing
Build the image with:
And then run with image
qmcgaw/ddns-updater:rfc2136and a configuration such as:{ "settings": [ { "provider": "rfc2136", "domain": "example.com", "owner": "home", "server": "ns1.example.com:53", "tsig_key_name": "ddns-key", "tsig_secret": "base64secret==", "ip_version": "ipv4" } ] }Configuration documentation
Verified against BIND 9.20 in Docker: updating an existing record, creating a missing one, AAAA records, the zone apex, unsigned updates on a permissive zone, and the rejection paths (wrong TSIG secret, unsigned update on a key protected zone, unknown zone). Also ran the updater itself against it end to end.
If it is useful, I kept those tests on a branch stacked on this one, behind an
integrationbuild tag with a BIND container definition intestdata: https://github.com/francisrath/ddns-updater/tree/rfc2136-integration. I left them out of this PR since nothing else in the repo carries test fixtures, but happy to fold them in if you want them.