Skip to content
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

Dns dynadot #4510

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Dns dynadot #4510

wants to merge 2 commits into from

Conversation

HRHDaniel
Copy link
Contributor

@HRHDaniel HRHDaniel commented Feb 18, 2023

@Neilpang - while implementing this, I ran into significant issues with the dynadot api and their dns implementation. Primarily that they state propagation can take 5 minutes to 48 hours! Additionally, I observed that for consistent results, the script must sleep and wait for propagation after EVERY change. This means when adding two TXT records, a sleep must occur waiting for propagation after EACH of these calls, not just after both of them. A sleep is also required after a TXT remove before moving on to additional test cases. The minimum sleep I was able to make work consistently was 30 minutes after each operation. While I was able to see consistent successful outcomes across all the platforms I tested on, this causes the automated tests to time out. For the docker step in the tests, there's 8 different docker images that the tests run on. When adding 2 TXT records in these tests, that's a 30 minute sleep necessary for each. 8 images x 2 TXT records x 30 minute sleeps = at least 8 hours in just the sleep time. GitHub is aborting the process after 6 hours, so I am currently unable to get the tests passing even though the script works. I put even more detail in the comments of dns_dynadot.sh

For a single certificate request, this implementation will be very reliable. Given appropriate sleep (recommended 30 minutes) this will also work for the vast majority of dynadot users. However, given the sleep time required for the number of back to back test cases and 6 hour run limitation, the tests cannot succeed. I still wanted to provide this script since it is the best available given the state of dynadot's implementation and can meet the needs for the majority of dynadot users.

@github-actions
Copy link

Welcome
Please make sure you're read our DNS API Dev Guide and DNS-API-Test.
Then reply on this message, otherwise, your code will not be reviewed or merged.
We look forward to reviewing your Pull request shortly ✨

@HRHDaniel HRHDaniel force-pushed the dns_dynadot branch 14 times, most recently from 918768d to 0d61bfc Compare February 25, 2023 01:54
@HRHDaniel
Copy link
Contributor Author

Issue for bug reporting: #4523

@HRHDaniel
Copy link
Contributor Author

dynadot instructions added to https://github.com/acmesh-official/acme.sh/wiki/dnsapi2

@Neilpang
Copy link
Member

If we have to wait 30 minutes for every change, I don't think you need: DYNADOT_ADD_DNS_SLEEP and DYNADOT_REMOVE_DNS_SLEEP

Just show a message and use _sleep() function to sleep for the time.

I don't think you need DYNADOTAPI_SKIP_REMOVE either. you can just remove the records and go ahead. you don't have wait for the result to propgate.

@HRHDaniel
Copy link
Contributor Author

If we have to wait 30 minutes for every change, I don't think you need: DYNADOT_ADD_DNS_SLEEP and DYNADOT_REMOVE_DNS_SLEEP

Just show a message and use _sleep() function to sleep for the time.

There's a couple reasons I made these configurable rather than a fixed sleep. If someone only needs a single, non-wildcard certificate, they likely don't need this sleep at all. If they are only requesting a single wildcard and no other certificates for the domain, then 90% of the time they could get away with a 5 minute sleep. Even if it failed occasionally at 5 minutes, if they have try again or have a cron job that runs the next morning, they could stick to 5 minutes. Any other scenario I'd recommend at least 30 minutes for dynadot, maybe even higher depending on the number of operations and their experience and reliability needs.

I don't think you need DYNADOTAPI_SKIP_REMOVE either. you can just remove the records and go ahead. you don't have wait for the result to propgate.

The remove isn't a true "remove this record". Dynadot doesn't provide that option in the API. Instead, we have to provide the full list of all dns records for the domain. We just provide that full list without including the TXT record we want to remove. Because the order that they propagate these requests isn't guaranteed, and has proven to frequently propagate out of order, not waiting or skipping the removals caused a couple of issues:

  1. Say the domain has an A record and we added two TXT records to issue a wildcard certificate so the domain settings look something like:
A: ##.##.##.##
TXT: ASDF
TXT: QWER

Our first remove call would "set" the domain settings to:

A: ##.##.##.##
TXT: QWER

Then our second remove call would "set" the domain settings to:

A: ##.##.##.##

If these propagate out of order, the second call ends up removing both records, but then the first call would actually put a record back, resulting in a build up of left over TXT records over time.

  1. During the tests (or if anyone was creating multiple certificates for multiple subdomains) after the remove is called and we move on to the next subdomain certificate we begin adding the new TXT records. If we call the ADD before the prior remove call has finished propagating, then we can lose this new TXT record before validation if these propagate out of order. This was observed frequently in the tests. By providing both DYNADOT_REMOVE_DNS_SLEEP and DYNADOTAPI_SKIP_REMOVE I give the end user the option of whether they want a longer run time by sleeping to allow the removes to propagate, or a faster runtime by skipping the removes, but having to manually clean up the TXT records.
    If someone only needs a single non-wildcard certificate, they also don't need this remove sleep, as there won't be multiple calls to remove and won't be additional adds after it finishes, so that's the only case that there is no issue with not waiting.

It isn't pretty, but unfortunately, that was the way dynadot was behaving and the only options they have given us in the API.

@CNXudiandian
Copy link

Excuse me,Could I use dynadot DNS API at now?And this pull request still pending?

@HRHDaniel
Copy link
Contributor Author

Excuse me,Could I use dynadot DNS API at now?And this pull request still pending?

Dynadot worked when I opened this PR. However due to the limitations of their API there are significant drawbacks to the implementation. If you to make more than one update (multiple certs or a wildcard cert, you need to make sure there is a significant delay between API calls. This delay is configurable, see the docs for this plugin. However, because of the delays, the automated tests kept taking to long and could not pass before timing out. I do not know if this PR will be merged without those tests passing. You can use dynadot directly if you'd branch.

Because of the poor API and difficulty implementing this, I decided dynadot was but the correct host for my DNS records and I transferred to another provider, so I can no longer validate this implementation works or support it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants