New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Digitalocean provider #171

Merged
merged 14 commits into from Aug 10, 2017

Conversation

Projects
None yet
3 participants
@Deraen
Contributor

Deraen commented Aug 4, 2017

Fixes #125

The tests pass. Digitalocean has support for SRV records, but DO requires that the names follow format _service._protocol, e.g. _ts3._tcp., and the test cases use just @. So I disabled SRV support for now.

I haven't used Go before, but the diff looks sane so hopefully I managed to handle the dependencies correctly.

@captncraig

This comment has been minimized.

Show comment
Hide comment
@captncraig

captncraig Aug 4, 2017

Member

Wow! exciting to have this. I'll give it a thorough look over this weekend.

Member

captncraig commented Aug 4, 2017

Wow! exciting to have this. I'll give it a thorough look over this weekend.

@Deraen

This comment has been minimized.

Show comment
Hide comment
@Deraen

Deraen Aug 4, 2017

Contributor

Note: Digitalocean only supports PTR records together with Droplets (virtual machines), not through DNS api.

Contributor

Deraen commented Aug 4, 2017

Note: Digitalocean only supports PTR records together with Droplets (virtual machines), not through DNS api.

@tlimoncelli

This comment has been minimized.

Show comment
Hide comment
@tlimoncelli

tlimoncelli Aug 5, 2017

Contributor

Re: #171 (comment) Yes, please change the test to be conforming. I've added #173 to remind us to enforce this validation in the future.

Contributor

tlimoncelli commented Aug 5, 2017

Re: #171 (comment) Yes, please change the test to be conforming. I've added #173 to remind us to enforce this validation in the future.

@Deraen

This comment has been minimized.

Show comment
Hide comment
@Deraen

Deraen Aug 7, 2017

Contributor

I started testing this with real data and noticed that retrieving domain records from DO requires using paging to retrieve all the results. Fixed that now.

What would be correct way to implement GetNameservers? I saw that other providers also used the default list, but looks like this causes there always to be difference in TTL?

I have DefaultTTL modifier on the domain, no NAMESERVER entries.

----- DNS Provider: digitalocean... 3 corrections
#1: MODIFY NS metosin.fi: (ns3.digitalocean.com. ttl=1800) -> (ns3.digitalocean.com. ttl=300), DO ID: 1128468
#2: MODIFY NS metosin.fi: (ns2.digitalocean.com. ttl=1800) -> (ns2.digitalocean.com. ttl=300), DO ID: 1128467
#3: MODIFY NS metosin.fi: (ns1.digitalocean.com. ttl=1800) -> (ns1.digitalocean.com. ttl=300), DO ID: 1128466

Though limiting default nameservers to zero (DnsProvider(digitalocean, 0)) and providing NAMESERVER entries has the same problem, even if I add TTL modifier to NAMESERVER entries.

Contributor

Deraen commented Aug 7, 2017

I started testing this with real data and noticed that retrieving domain records from DO requires using paging to retrieve all the results. Fixed that now.

What would be correct way to implement GetNameservers? I saw that other providers also used the default list, but looks like this causes there always to be difference in TTL?

I have DefaultTTL modifier on the domain, no NAMESERVER entries.

----- DNS Provider: digitalocean... 3 corrections
#1: MODIFY NS metosin.fi: (ns3.digitalocean.com. ttl=1800) -> (ns3.digitalocean.com. ttl=300), DO ID: 1128468
#2: MODIFY NS metosin.fi: (ns2.digitalocean.com. ttl=1800) -> (ns2.digitalocean.com. ttl=300), DO ID: 1128467
#3: MODIFY NS metosin.fi: (ns1.digitalocean.com. ttl=1800) -> (ns1.digitalocean.com. ttl=300), DO ID: 1128466

Though limiting default nameservers to zero (DnsProvider(digitalocean, 0)) and providing NAMESERVER entries has the same problem, even if I add TTL modifier to NAMESERVER entries.

@captncraig

This comment has been minimized.

Show comment
Hide comment
@captncraig

captncraig Aug 7, 2017

Member

You can add {ns_ttl: 1800} to your domains to override the ttls on the auto-generated NS records. You can set that with DEFAULTS({ns_ttl: 1800}) to apply it to all domains.

That is a bit of undocumented oddness I'll admit.

Member

captncraig commented Aug 7, 2017

You can add {ns_ttl: 1800} to your domains to override the ttls on the auto-generated NS records. You can set that with DEFAULTS({ns_ttl: 1800}) to apply it to all domains.

That is a bit of undocumented oddness I'll admit.

@Deraen

This comment has been minimized.

Show comment
Hide comment
@Deraen

Deraen Aug 7, 2017

Contributor

Thanks, I now have 6 domains configured and preview shows 0 corrections.

Contributor

Deraen commented Aug 7, 2017

Thanks, I now have 6 domains configured and preview shows 0 corrections.

@captncraig

Looks really good. Passes tests for me. Just a few small things to look at.

@@ -290,11 +293,13 @@ var tests = []*TestCase{
tc("Change it", cname("foo", "google2.com.")),
tc("Change to A record", a("foo", "1.2.3.4")),
tc("Change back to CNAME", cname("foo", "google.com.")),
tc("Record pointing to @", cname("foo", "**current-domain**")),

This comment has been minimized.

@Deraen

Deraen Aug 7, 2017

Contributor

I managed to create tests for this by using special value for test cases and rewriting the value to domainName on runTests. Not very pretty but works.

@Deraen

Deraen Aug 7, 2017

Contributor

I managed to create tests for this by using special value for test cases and rewriting the value to domainName on runTests. Not very pretty but works.

This comment has been minimized.

@tlimoncelli

tlimoncelli Aug 7, 2017

Contributor

Great! I think we'll be able to make more interesting tests that way.

@tlimoncelli

tlimoncelli Aug 7, 2017

Contributor

Great! I think we'll be able to make more interesting tests that way.

@tlimoncelli

This comment has been minimized.

Show comment
Hide comment
@tlimoncelli

tlimoncelli Aug 7, 2017

Contributor

FYI: I just updated provider-list.md to clarify the responsibilities of someone that submits a new provider. Basically you'll be asked to fix future bugs, but the DNSControl maintainers will take responsibility for maintaining a test suite that lets you do that easily. May we list you as the maintainer of this provider? (your name is commented out in provider-list.md)

Contributor

tlimoncelli commented Aug 7, 2017

FYI: I just updated provider-list.md to clarify the responsibilities of someone that submits a new provider. Basically you'll be asked to fix future bugs, but the DNSControl maintainers will take responsibility for maintaining a test suite that lets you do that easily. May we list you as the maintainer of this provider? (your name is commented out in provider-list.md)

@Deraen

This comment has been minimized.

Show comment
Hide comment
@Deraen

Deraen Aug 7, 2017

Contributor

@tlimoncelli This is OK. I plan on eventually migrating our domains somewhere else (one reason I wanted to use dnscontrol), so I might not use this myself a lot, but looks like maintaining this should be quite easy.

Contributor

Deraen commented Aug 7, 2017

@tlimoncelli This is OK. I plan on eventually migrating our domains somewhere else (one reason I wanted to use dnscontrol), so I might not use this myself a lot, but looks like maintaining this should be quite easy.

@tlimoncelli tlimoncelli merged commit ad27cc9 into StackExchange:master Aug 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tlimoncelli

This comment has been minimized.

Show comment
Hide comment
@tlimoncelli

tlimoncelli Aug 10, 2017

Contributor

Thanks for contributing a new provider! You rock!

Contributor

tlimoncelli commented Aug 10, 2017

Thanks for contributing a new provider! You rock!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment