Skip to content

Porkbun does not work concurrently (ppreview) #2995

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.

Sign up for GitHub

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

Jump to bottom
Open
Syntaxheld opened this issue Jun 7, 2024 · 15 comments
Open

Porkbun does not work concurrently (ppreview) #2995

Syntaxheld opened this issue Jun 7, 2024 · 15 comments
Labels
provider-PORKBUN

Comments

@Syntaxheld
Copy link

Syntaxheld commented Jun 7, 2024 •
edited
Loading

Describe the bug
Testing the new concurrency feature (ppreview) on domains handled by the Porkbun provider it fails to handle those:

INFO#1: Zone "example.com" does not exist. Can not create because "porkbun" does not implement ZoneCreator

But the zone exists ... all works well when running the normal preview.

To Reproduce

  1. Have (multiple) domains in dnscontrol setup that use Porkbun
  2. dnscontrol ppreview --cmode all
  3. Multiple Zone "example.com" does not exist. Can not create because "porkbun" does not implement ZoneCreator issues.

Expected behavior
For Porkbun to work concurrently.

DNS Provider

  • Porkbun

Additional context
Tested with dnscontrol v4.11.0.

I've checked the "Concurrency Verified" column in https://docs.dnscontrol.org/getting-started/providers ... but I was not sure if the ❌ means "verified & failed" or "not verified".

At least in my testing, when I use dnscontrol ppreview --cmode all --domains example1.com,example2.com it still works (most of the time). It only starts throwing issues regularly when I test it with at least 3 different zones.

CC @imlonghao (maintainer of the porkbun provider)
@imlonghao
Copy link
Collaborator

I was not sure if the ❌ means "verified & failed" or "not verified".

For Porkbun, it means "not verified" yet.

Zone "example.com" does not exist.

This error was caused by a missing error catch for JSON parsing, the real error message should be the follow, I will fill a PR for this.

failed parsing nameserver list from porkbun: invalid character '<' looking for beginning of value

dnscontrol/providers/porkbun/api.go

Line 157 in 32fe275

json.Unmarshal(bodyString, &ns)

I'm not sure whether they have a hidden rate limit or something, in my test, when there are more than 3 domains in concurrently, an HTTP 503 error was thrown.

<html>
<head><title>503 Service Temporarily Unavailable</title></head>
<body>
<center><h1>503 Service Temporarily Unavailable</h1></center>
<hr><center>openresty</center>
</body>
</html>

I will contact the support for this, asking whether they have a rate limit or just the server crashed.

@imlonghao imlonghao mentioned this issue Jun 15, 2024
Merged
@imlonghao
Copy link
Collaborator

I got a response from Porkbun support.

The rate limit is 1 request per second.

So the Porkbun provider is not supposed to work concurrently.

@tlimoncelli
Copy link
Contributor

1 per second? wow, that's pretty low.

It might be useful to implement a re-try on 429 (if porkbun sends a proper 429 when the rate limit is exceeded). Even without concurrency it is possible that we would exceed that limit.

@Syntaxheld
Copy link
Author

Syntaxheld commented Feb 4, 2025 •
edited
Loading

Any chance this maybe works now, since the split to a separate API host?

Quoting from #3153 (review)

No more AWSALB, nor the weird rate limit.
I run the integration test and all tests success all at once, not hitting any rate limit compare to #3019.

@tlimoncelli
Copy link
Contributor

I don't have a porkbun account, but if you do, you can test by adding the --cmode all flag. It will run all providers concurrently, even if they're set to providers.CanConcur: providers.Cannot().

@jamesog
Copy link

jamesog commented Feb 4, 2025

Looks like I still get rate limited by Porkbun with --cmode all.

@tlimoncelli
Copy link
Contributor

I thought you might. Did dnscontrol pause and retry or fail?

@jamesog
Copy link

jamesog commented Feb 4, 2025 

CONCURRENTLY gathering 13 zone(s)
SERIALLY gathering 0 zone(s)
Waiting for concurrent gathering(s) to complete...WARNING: Rate limiting.. waiting for 10 second(s)
WARNING: Rate limiting.. waiting for 10 second(s)
WARNING: Rate limiting.. waiting for 10 second(s)
DONE

With some, but not all domains outputting

INFO#1: Domain "jamesog.dev" provider porkbun Error: failed parsing url forwarding record list from porkbun: invalid character '<' looking for beginning of value

(They return HTML when they 429.)

@tlimoncelli
Copy link
Contributor

Sounds like the provider needs to handle 429s to make this work.

@jamesog
Copy link

jamesog commented Feb 5, 2025

I've played about with the retry logic a bit, including adding explicit support for 429, but the Porkbun API still has pretty aggressive restrictions.

Even if I add longish sleeps between each retry, I intermittently (and non-deterministically, AFAICT) get either 503 or 400 when running in concurrent mode. In both cases it returns HTML, despite their API docs saying:

Any HTTP response code other than 200 is an error. Errors will also be indicated in the "status" element of the returned JSON. An HTTP response of 403 means that additional authentication information is required, such as a two factor code.

I think for now Porkbun will have to be left out of concurrency :-(

(The provider could still do with handling non-JSON responses better, though.)

@tlimoncelli
Copy link
Contributor

@jamesog Thanks for investigating this!

We've seen similar problems with other providers. For example, gcloud's retry.go has a special case where it will retry a 502 exactly once. It's a bit hacky, but it works.

I think it would be worthwhile to add support for 429 even for non-concurrent use.

@gotjoshua
Copy link
Contributor

thanks for all the details in this thread! I think i managed to grok the
Concurrency Verified column in the provider matrix now... but not well enough to write a doc page for it.

Any of you able to write a concise definition for that column?

@tlimoncelli
Copy link
Contributor

Hi @gotjoshua ! Thanks for looking into this.

"Concurrency Verified": A checkmark means "this has been tested and will run concurrently". The red "X" means "it hasn't been tested, we don't know if it will work concurrently".

Here's how to perform a test:

  1. Build dnscontrol with the -race flag: go build -race (this makes a special binary)
  2. Run this special binary: dnscontrol preview with a configuration that lists at least 4 domains using Porkbun. More domains is better.
  3. The special binary runs much slower (1/10th the speed) because it is doing a lot of checking. If it reports problems, the error message will indicate where the problem is. It might not be 100% accurate, but it will be in the right area.

I would be glad to help fix any problems!

Tom

@winterqt

This comment has been minimized.

Sign in to view
@gotjoshua
Copy link
Contributor

The red "X" means "it hasn't been tested, we don't know if it will work concurrently".

why not use the ? for that
and the red x for tested and not working?

started a PR draft with your suggestion @tlimoncelli :
#3510

@gotjoshua gotjoshua mentioned this issue Mar 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
provider-PORKBUN
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@jamesog @cafferata @imlonghao @tlimoncelli @Syntaxheld @gotjoshua @winterqt