-
Notifications
You must be signed in to change notification settings - Fork 374
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
deSEC: Implements support for long / multistring txt records #1204
Conversation
this implements the token validation using the /auth/account api endpoint as suggested in StackExchange#1177 instead of fetching the domain list
relates to StackExchange#996 where we had insufficient error output due to unknown api error format
the previous commit was broken this is now working (CRUD)
I think this can be merged when you are ready. |
we try to use the Retry-After header to determine how long we should sleep until retry
You may also want to add a cut-off for the wait time, let's say 120 seconds? (In some weird edge cases, it could happen that the daily limit is hit etc., and you probably don't want dnscontrol to stall.) |
You mean if the Retry-After is like 86400 seconds because its in the daily limit? I'm not sure if thats what you mean :D |
There is a general limit of 2000 requests per day per user (see docs). If you hit that within two hours (let's say -- but that's almost one request per second), you will have to wait 22 hours (79200 seconds). I would assume that people don't want to wait for such long time. -- Does dnscontrol have a general timeout parameter? If so, you might want to error out if you can see from the |
we cut off if the Retry-After header exceeds 3 minutes because this might be the daily limit.
This PR includes various improvements and resolves #996 and #1177 which @tmkis2 just confirmed. I will now stop to add more commits to this so that @tlimoncelli can complete the review and merge if everything is fine. |
So much to love here! Thanks for this! (especially the rate limiting!) |
This PR will resolve #996 and #1177 the multi/long txt part is working but i suggest to wait with the merge until @tmkis2 confirmed that its working with over 500 domains since i do not have that many domains to test it.