sewer icon indicating copy to clipboard operation
sewer copied to clipboard

maxChecks cli arg --> ACME_AUTH_STATUS_MAX_CHECKS

Open alanbacon opened this issue 4 years ago • 6 comments

Thank you for contributing to sewer.
Every contribution to sewer is important to us.
You may not know it, but you have just contributed to making the world a more safer and secure place.

Contributor offers to license certain software (a “Contribution” or multiple “Contributions”) to sewer, and sewer agrees to accept said Contributions, under the terms of the MIT License. Contributor understands and agrees that sewer shall have the irrevocable and perpetual right to make and distribute copies of any Contribution, as well as to create and distribute collective works and derivative works of any Contribution, under the MIT License.

Now,

What(What have you changed?)

added a cli flag to set the number of attempts sewer makes to verfify the DNS challenge

Why(Why did you change it?)

Because sewer was timing out when trying to verify the DNS challenge, and so I needed it to wait for longer

alanbacon avatar Apr 13 '20 16:04 alanbacon

Codecov Report

Merging #166 into master will not change coverage by %. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #166   +/-   ##
=======================================
  Coverage   86.71%   86.71%           
=======================================
  Files          18       18           
  Lines        1024     1024           
=======================================
  Hits          888      888           
  Misses        136      136           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f3eee58...3c0839f. Read the comment docs.

codecov[bot] avatar Apr 13 '20 17:04 codecov[bot]

@komuw I managed to format this PR correcting using black, would you mind helping me with the formatting on my other PR thanks

alanbacon avatar Apr 13 '20 17:04 alanbacon

@alanbacon wouldn't it be better, at the CLI level, to request the total time to wait before giving up? Yes, I know that's not how Client expresses it now... that might be a related (or separate?) change. Maybe just define the defaults for request_timeout and max_tries in the client module so that cli can query them before initializing its Client instance.

I assume you've used your patches - what sort of total timeout did you need? Just looking for a datapoint here. Thanks.

mmaney avatar Apr 14 '20 01:04 mmaney

@mmaney A total timeout flag would be much better you're right, I just quickly hashed this together and thought that the project might want the change also. I used 10 x 8s ~= 1m:30s. The current default of 3 x 8s 24s is a little on the tight side for me, maybe my provider (gandi) just isn't all that great I don't know. 24s is sort of number that i feel will work maybe 90% of the time.

alanbacon avatar Apr 14 '20 01:04 alanbacon

@alanbacon Okay... it's been a long day, I need to sleep on this. We've had a couple of mentions of timeout issues, and that wait-for-all-validations loop is rather a mess in other ways. Thanks to this discussion, I'm thinking it should be looping not until N queries have been made (and there was a bug until recently where the last try was never accepted), but until the seperate VALIDATION_TIMEOUT period of time has passed. This is a deeper refactoring of get_certificate and check_authorization_status that I've had somewhere down my ToDo list for a while. I think you've shown me why I wasn't entirely happy with the cleanup of the counting loop I'd had in mind.

mmaney avatar Apr 14 '20 03:04 mmaney

With the get_certificate mess targeted for 0.8.5, I expect fixing this (by adding a simple timeout instead of MAX_COUNT) will be a side effect. The rest of that work will be the challenge. ;-/

mmaney avatar Sep 10 '20 20:09 mmaney