StackExchange / dnscontrol

Infrastructure as code for DNS!
https://dnscontrol.org/
MIT License
3.13k stars 397 forks source link

NS1: Add support for ALIAS, PTR, and NAPTR #775

Closed mratmeyer closed 4 years ago

mratmeyer commented 4 years ago

Hello,

According to the NS1 API Docs, it looks like they have support for ALIAS, PTR, and NAPTR records, which aren't currently available in DNSControl. Would it be possible to get these activated in DNSControl?

https://ns1.com/api#putcreate-a-dns-record

Thanks

tlimoncelli commented 4 years ago

Seems reasonable! This might be a good starter project if you are interested in diving into the code. PTR and NAPTR might "just work" depending on how the code was written. We may just never have tested it.

I don't have a test account with NS1, so I can't do this myseslf.

I'd be glad to walk you through the process. The process is:

  1. Make sure the integration tests work as-is
export NS1_TOKEN="insert-your-token-here"
export NS1_DOMAIN="testdomain.tld"
cd integrationTest
go test -v -verbose -provider NS1

This just makes sure everything is working.

  1. Enable the features.

In providers/ns1/ns1provider.go look at the var docNotes = providers.DocumentationNotes{ list and add the features to it. Look at providers/bind/bindProvider.go for examples.

  1. Re-run the integration tests.

My guess is that PTR and NAPTR might "just work". ALIAS might require some more effort.

CC: @captncraig

mratmeyer commented 4 years ago

Hi, thanks so much for the instructions. I cloned the repo and ran the integration test, but without making any changes, I got this error. === RUN TestDualProviders TestDualProviders: integration_test.go:272: Clearing everything TestDualProviders: integration_test.go:278: Adding nameservers from another provider TestDualProviders: integration_test.go:266: #1: CREATE NS ratmeyer.org ns1.example.com. ttl=300 CREATE NS ratmeyer.org ns2.example.com. ttl=300 TestDualProviders: integration_test.go:281: Running again to ensure stability --- PASS: TestDualProviders (4.77s) FAIL exit status 1 FAIL github.com/StackExchange/dnscontrol/v3/integrationTest 43.398s I tried it with another domain in a different TLD as well and it ran into the same issue. It didn't seem like there were any problems in the test, it just abruptly cut off at the end.

tlimoncelli commented 4 years ago

Ah, its failing on the test of the provider's ability to share a domain with another provider. I wonder how long that has been broken (or if it ever worked). That said, restrict the test to just the main test by adding -run TestDNSProviders to the end of the command line.

tlimoncelli commented 4 years ago

P.S. The tests delete all the records from the domain to give it a "fresh start". I'm guessing you don't want that to happen on ratmeyer.org. You might want to use a scratch domain.

mratmeyer commented 4 years ago

No worries, I use the .com for everything but I have .net and .org as well so I just used them for testing.

mratmeyer commented 4 years ago

Hmm, it looks like even with the -run TestDNSProviders flag its still giving me the issue. Do you think it would be safe for me to just ignore it for now for testing the new records?

Multi_notsupported)***:Empty (0.21s) --- PASS: TestDNSProviders/ratmeyer.org/25:DSSKIPPED(CanUseDS_not_supported):Empty (0.25s) --- PASS: TestDNSProviders/ratmeyer.org/26:DS_(childrenonly)SKIPPED(CanUseDSForChildren_not_supported):Empty (0.24s) --- PASS: TestDNSProviders/ratmeyer.org/27:ALIAS_SKIPPED(CanUseAlias_not_supported):Empty (0.24s) --- PASS: TestDNSProviders/ratmeyer.org/28:AZUREALIASSKIPPED(CanUseAzureAlias_not_supported):Empty (0.20s) --- PASS: TestDNSProviders/ratmeyer.org/29:R53ALIASSKIPPED(CanUseRoute53Alias_not_supported):Empty (0.18s) FAIL exit status 1 FAIL github.com/StackExchange/dnscontrol/v3/integrationTest 28.931s

tlimoncelli commented 4 years ago

Yes, it would be safe, however we should look at the failed tests.

The "SKIPPED" lines are fine. It just means that that it is skipping a test for features that haven't been implemented yet. Search integration_test.go for providers.CanUseDS ... you'll see how that works (there are comments at the top too).

I do notice that the test that you pasted above failed... but it must have been an earlier test that wasn't included in the comment. Could you include the failures too?

Thanks!

mratmeyer commented 4 years ago

Sure, I've just uploaded the logs all to Pastebin.

go test -v -verbose -provider NS1 - https://pastebin.com/Hju1K1p7 go test -v -verbose -provider NS1 -run TestDNSProviders - https://pastebin.com/n4zqqhLf

mratmeyer commented 4 years ago

Alright, so it looks like PTR worked on its own, NAPTR failed but I don't need that so I won't worry about it, and ALIAS returns an unparsable record error but if I go to my NS1 console the record does actually get created. Possibly a guess on what's causing this- in the NS1 console there's an option for override TTL so maybe that extra metadata is throwing off the parsing? I also tested TXTMulti and NS1 passed for this too, which is great.

tlimoncelli commented 4 years ago

That's great news! Congrats!

If the ALIAS record is being created, that's a good sign. Is the error happening when we get the response back from telling the API to create the record, or when we loop back and verify that the record was created?

Error when we create the record: Most likely we're parsing the response wrong.

When we loop back: The way the integration tests work is: for each test it creates the records, diffs against what exists, and then make the changes reported by the diff (just like when someone uses the tool). However then we do this a second time... this time the diff should be empty. If it isn't, we know something went wrong. It could be that we didn't do the update right, or that we did the update right but we don't know how to download a new zone with this kind of record.

Could you include the full failed text?

By the way... you don't have to run all the tests every time. You can do -start 5 -end 7 to just run tests 5-7.

Hope that helps!

mratmeyer commented 4 years ago

Awesome, thanks! The error is whenever it tries to read the response. If I don't clear the alias from the domain on the NS1 console before I run the test, it gives me an error when it tries to clean the domain.

This is when I run the test for the first time and it creates the alias record: https://pastebin.com/1HxjtGdc This is if I run it again and the alias record now exists: https://pastebin.com/DuRtUfVk

tlimoncelli commented 4 years ago

Ah yes, I see what you are saying. I think the issue is that when it reads the zone, it sees a record type it doesn't understand and panics. I grep'ed for the error message and found the code:

    rec.SetLabelFromFQDN(zr.Domain, domain)
    switch rtype := zr.Type; rtype {
    default:
      if err := rec.PopulateFromString(rtype, ans, domain); err != nil {
        panic(fmt.Errorf("unparsable record received from ns1: %w", err))
      }
    }
    found = append(found, rec)

PopulateFromString is a function we wrote when we realized that 80% of all DNS providers return DNS records with a "target" like "1.2.3.4" (for A records) and "10 foo.com" (for MX records), etc. They were all the same, so we wrote one function that works for most providers.

If you look at providers/cloudflare/cloudflareProvider.go, you'll see it uses PopulateFromString too. First it handles records like MX and SRV which Cloudflare does in its own unique way. Then it called PopulateFromString for all other record types.

The ns1 provider can do the same thing: handle ALIAS and other things then call PopulateFromString. You might consider adding those records to PopulateFromString if the implementation is generic enough.

Tom

mratmeyer commented 4 years ago

Sweet, looking at how DNSimple handles aliases, I was able to modify it a bit for NS1 and it looks like it works! Pretty excited since this will be my first PR. Thanks for your help!

--- PASS: TestDNSProviders (3.31s) --- PASS: TestDNSProviders/maxnet.work (3.31s) --- PASS: TestDNSProviders/maxnet.work/Clean_Slate:Empty (0.14s) --- PASS: TestDNSProviders/maxnet.work/27:ALIAS:ALIAS_at_root (0.57s) --- PASS: TestDNSProviders/maxnet.work/27:ALIAS:change_it (0.72s) --- PASS: TestDNSProviders/maxnet.work/27:ALIAS:ALIAS_at_subdomain (0.93s) --- PASS: TestDNSProviders/maxnet.work/Post_cleanup:Empty (0.52s) PASS ok github.com/StackExchange/dnscontrol/v3/integrationTest 3.624s

tlimoncelli commented 4 years ago

Sweet! That's awesome detective work!

Send me the PR and I'll review it asap!

mratmeyer commented 4 years ago

Unfortunately it looks like NS1 may not have support for TXTMulti. I can upload the TXTMulti records fine, but NS1 automatically converts it into one string so every time I rerun DNSControl it tries to reconvert it. I'm not sure why it passed the test, but I just wanted to confirm before I send in a PR to remove it. Thankfully the ALIAS records are working great!

On that note, the main reason I wanted TXTMulti was so I could use Dual DNS between say R53 and NS1 because the DKIM keys don't get automatically converted in a TXT record in R53. Is there any way I could do provider specific records so say TXT on NS1 and TXTMulti on R53 or ALIAS on NS1 and R53_ALIAS on R53?

tlimoncelli commented 4 years ago

Testing: Interesting! I wonder if we could construct a test that would detect that.

There isn't a simple way to have 2 providers receive different records. That's one problem with things like ALIAS and any feature that works on some providers but not others: it limits your ability to do dual-providers or to move a domain between providers. R53_ALIAS is extremely specific to AWS. There was no other way to make it generic since one of the parameters is your AWS zone-id: not something that would ever exist on any other provider.

There is no generic "do X for one provider, Y for another provider" mechanism right now. You can make a new record type that does the right thing for each provider. We've considered making a "GENERIC_ALIAS()" record that did the right thing (ALIAS, R53_ALIAS, A record). It would take parameters for both the CNAME target and an IP address for when an A record is the only option.

A bigger bottleneck might be that there is a chance that NS1 doesn't support sharing NS records with other providers. The the TestDualProviders test is failing for NS1. (-run TestDualProviders to just run that one test.) It would be easier to fix that, or to move providers :-)

mratmeyer commented 4 years ago

Yeah, I see what you're saying. I chose NS1 because they have a solid free tier and it seems lightweight, fast, and reliable. Luckily, NS1 does support dual providers, I just ran a test and it was able to update the apex NS records. Maybe I'm just overthinking this and I really don't need dual DNS for a personal domain and just using NS1 is fine but I sometimes just like making overcomplicated setups and I usually learn a lot along the way.

Last thing, NS1 has a URLFWD record that lets you make URL redirects and I noticed in the docs there are URL and URL301 records. Are these generic records that would work for any provider if I made a PR in the future?

tlimoncelli commented 4 years ago

Yeah, for a personal domain a single provider is probably fine. Besides a provider like NS1 has very high uptime.

URLFWD: Yes, that's a pseudo record that should do the right thing for each provider that has a redirect service.