Closed bakkerpeter closed 4 years ago
All looks good, my only suggestions would be
selfTest
as at first glance I thought that was some kind of automated test to verify the client correctly implements ACME! Something like waitForSelfValidation
might make this clearerwaitForSelfValidation
could be a little more helpful by instead of asking how many attempts, what the minimum and maximum wait time should be. These would default to null
for a method-appropriate default (e.g. min=0
, max=30
for http, and maybe min=60
, max=120
for DNS). Then someone using this package gets a reasonable default but if they want to tune it, they can. Just suggestions though, it's your package :)
Thanks for the feedback, I'll look into this later today.
rename selfTest as at first glance I thought that was some kind of automated test to verify the client correctly implements ACME! Something like waitForSelfValidation might make this clearer
I think we should leave this as is for now, as changing the name would induce a breaking change (I think we can use the same method for both HTTP - which is already implemented - and DNS testing.)
My experience with one provider I use (DNSMadeEasy) is that if I ask LE to validate immediately after seeing my local DNS resolver fetch the updated TXT record, it will fail LE's secondary validation checks. A minimum of 60 seconds generally worked, and I never had any validation issues with 90 seconds
Good feedback and based on this, I have changed the backoff factor for the DNS validation (see https://github.com/afosto/yaac/pull/5/commits/6455a33db175c3631cf2ce96f463c6c87e6359ed) . To keep it consistent with the HTTP validation I think we should just stay with an max attempt count. The effect is as follows with default settings (pausing time in seconds between attempts): 3, 4, 4, 4, 5, 5, 5, 6, 7, 8, 9, 12, 15, 23, 45 - totalling 155 seconds if all attempts fail.
This doesn't provide a minimum time delay - it will return as soon as it sees the DNS record, so I'd suggest adding to the documentation to make sure anyone doing DNS validation performs an extra sleep after selfTest has succeeded. I'll send you a suggestion as separate PR.
Added DNS support based on the work of @lordelph. In stead of validating based on the built-in
dns_get_record
I suggest we Cloudflare's API to overcome caching issues within PHP.