VERCEL: Implement Vercel DNS Provider (#3379)

[SukkaW]

#3379

The PR is still WIP. The PR is now ready for review.

Also, I am not familiar with Go at all. All kinds of reviews are welcome.

---

Please create the GitHub label 'provider-VERCEL'

[SukkaW]

Just an update, I have finished implementing the provider. I will create an intergration test case later.

[SukkaW]

So, Vercel has some automatically created records (like immutable NS, automatically created CAA records, etc.) that can not be deleted. How do I implement this?

Also, Vercel enforces a minimum TTL of 60, how do I incorporate that? Done in GetZoneRecordsCorrections.

[tlimoncelli]

So, Vercel has some automatically created records (like immutable NS, automatically created CAA records, etc.) that can not be deleted. How do I implement this?

That's an interesting problem. Luckily we've seen this before.

It should be a simple matter of "pretend those records don't exist". GetZoneRecords() can skip over them rather than convert them (line 122 for _, r := range records {).

Then everything else should "just work" because you are using diff2.ByRecord() and future add/change/deletes will not affect the existing records.

Hope that helps!

[tlimoncelli]

For someone "not familiar with Go at all" this is quite excellent! I only had a couple minor suggestions!

[SukkaW]

That's an interesting problem. Luckily we've seen this before.

It should be a simple matter of "pretend those records don't exist". GetZoneRecords() can skip over them rather than convert them (line 122 for _, r := range records {).

Then everything else should "just work" because you are using diff2.ByRecord() and future add/change/deletes will not affect the existing records.

Hope that helps!

@tlimoncelli Thanks! I have implemented this in 8f017ea (#3542).

I do notice that when running the intergration tests, there is a CREATE dnscontrol-intergration.test NS ns1.vercel-dns.com. ttl=300. It appears that DNSControl is trying to update NS record at the apex (which the operation is not permitted by the Vercel API, and those apex NS records are not listed in the API response).

Maybe I should return nil, nil or []*models.Nameserver{}, nil in GetNameservers to prevent DNSControl from creating apex NS records at all? Update: done in 96ffaa5 (#3542)

[SukkaW]

Also, I noticed that the Vercel API has somewhat strict API rate limiting, something like 50 modifications/deletions per minute, 100 creations per minute, and 500 records retrieval per minute. That would be enough for small/personal domains (Vercel's DNS is not meant for serious deployment anyway), but how would I finish intergration tests though?

[tlimoncelli]

Also, I noticed that the Vercel API has somewhat strict API rate limiting, something like 50 modifications/deletions per minute, 100 creations per minute, and 500 records retrieval per minute. That would be enough for small/personal domains (Vercel's DNS is not meant for serious deployment anyway), but how would I finish intergration tests though?

That's an interesting problem.

In the short term, we can just run the integration tests a few at a time using the -start and -end flags. If the code fails at test 30, you can do the next group by running -start 30 -end 60, etc.

In the long term, we should detect the error that Vercel gives when we hit the limit, sleep, then try again. There are some modules that can help with that. Some providers code their own, others use github.com/failsafe-go/failsafe-go

[SukkaW]

That's an interesting problem.

In the short term, we can just run the integration tests a few at a time using the -start and -end flags. If the code fails at test 30, you can do the next group by running -start 30 -end 60, etc.

In the long term, we should detect the error that Vercel gives when we hit the limit, sleep, then try again. There are some modules that can help with that. Some providers code their own, others use github.com/failsafe-go/failsafe-go

I ended up skipping the pager test, as the preparation (creating 101 records) needs an hour, which is way too much. Vercel, in the end, is not a serious DNS provider, and I have already ensured that I have handled the pagination.

On the other hand, I copied and modified the rate limit implementation from Hetnezer's provider, as it behaves mostly the same as Vercel's API, and can easily be ported.

[tlimoncelli]

That's an interesting problem.
In the short term, we can just run the integration tests a few at a time using the -start and -end flags. If the code fails at test 30, you can do the next group by running -start 30 -end 60, etc.
In the long term, we should detect the error that Vercel gives when we hit the limit, sleep, then try again. There are some modules that can help with that. Some providers code their own, others use github.com/failsafe-go/failsafe-go

I ended up skipping the pager test, as the preparation (creating 101 records) needs an hour, which is way too much. Vercel, in the end, is not a serious DNS provider, and I have already ensured that I have handled the pagination.

On the other hand, I copied and modified the rate limit implementation from Hetnezer's provider, as it behaves mostly the same as Vercel's API, and can easily be ported.

Sounds good! (Many providers skip the "pager" tests for similar reasons)

Tom

[SukkaW]

@tlimoncelli

Currently, I am failing CAA:CAA_many_records integration test, but it is because Vercel doesn't believe CAA @ issue ; contains a valid FQDN. Which rejectif should I use here in my audit record?

Update: I implemented a custom rejectif for this. A unit test is also created.

[SukkaW]

@tlimoncelli

Also, it turns out that vercel limits CAA with extra fields. They only allow cansignhttpexchanges and forbid validationmethods or accounturi. Most likely, it is a bug on their side, because their API throws:

invalid_value - Unexpected "cansignhttpexchanges" value.

I don't want to deal with this for now, so I have implemented a custom rejectif rejectifCaaTargetContainsUnsupportedFields, and I have written the unit test in auditrecords_test.go. I have also added a Vercel vendor-specific integration test case to validate that Vercel does accept cansignhttpexchanges.

Also, all integration tests are now passed on my machine locally.

[SukkaW]

The PR should be ready for review now. Thanks @tlimoncelli for the massive help!

Also, all integration tests are now passed on my machine locally.

Also, having been playing around with my provider with my production domain already, it works so far:

[nekomeowww]

you could use defer resp.Body.Close() if suitable, optional suggestion.

[SukkaW]

I did update to use defer, not sure if it is ok though. cc @tlimoncelli what do you think?

[nekomeowww]

Generally LGTM.

[tlimoncelli]

Please run bin/generate.sh to fix the formatting of integrationTest/profiles.json.

[SukkaW]

Please run bin/generate.sh to fix the formatting of integrationTest/profiles.json.

Done in 6cf861a (#3542). cc @tlimoncelli

[SukkaW]

@tlimoncelli Hi, I have just rebased my PR against the latest main branch and resolved all the conflicts.

[tlimoncelli]

Thank you for contributing this provider! I'll merge it shortly.

I'd love to have Vercel part of the automated testing that we do. If you can arrange for a test account, I'd gladly add it. Instructions are here: https://docs.dnscontrol.org/developer-info/byo-secrets#donate-secrets-to-the-project

[SukkaW]

Thank you for contributing this provider! I'll merge it shortly.

I'd love to have Vercel part of the automated testing that we do. If you can arrange for a test account, I'd gladly add it. Instructions are here: https://docs.dnscontrol.org/developer-info/byo-secrets#donate-secrets-to-the-project

Sure! I would like to create a new draft PR to see if the intergration tests pass from my forked repo first, and I can donate those credentials after that.

[tlimoncelli]

Sure! I would like to create a new draft PR to see if the intergration tests pass from my forked repo first, and I can donate those credentials after that.

perfect!