Add pre_check_delay option #111
Conversation
ghost
added
the
There was a problem hiding this comment.
Hey @tjhiggins, so a few things regarding this PR:
-
Regarding the main pre-check delay function - can you update this to sleep only 1 second, looping for the period of the delay? The issue with a
time.Sleepfor the whole duration will be that it will block for the entirety, which makes it impossible to interrupt if need be. This could be especially problematic with larger delays. For longer delays, we could possibly set this to a fraction of the delay, with a maximum interval of 5-10sec (maybe use a factor of 6 with a max of 10sec). -
The option needs to be added to
resourceACMECertificateUpdateas well.
acme/resource_acme_certificate.go
Outdated
| wrapFunc := func(domain, fqdn, value string, orig dns01.PreCheckFunc) (bool, error) { | ||
| stop, err := orig(fqdn, value) | ||
| if stop && err == nil { | ||
| log.Printf("[%s] acme: Waiting an additional %s for DNS record propagation.", domain, delay) |
There was a problem hiding this comment.
This log entry should be prefixed with the [DEBUG] log level.
| log.Printf("[%s] acme: Waiting an additional %s for DNS record propagation.", domain, delay) | |
| log.Printf("[DEBUG] [%s] acme: Waiting an additional %s for DNS record propagation.", domain, delay) |
There was a problem hiding this comment.
@vancluever thanks for the review. I probably won't get to this since we switched to having our customers install certmanager out of band. Should I close?
There was a problem hiding this comment.
@tjhiggins don't close it out! I'll finish it up and merge it with your commit. Thanks for the work you've done so far!
There was a problem hiding this comment.
Going to keep this log entry like this particularly as it's run in the lego code path and this is generally how lego structures their log messages.
This adds the pre_check_delay option, which allows for a delay for DNS propagation after all DNS challenges have been added. Propagation delays are an issue on certain DNS providers regardless of the ability of lego and acme_certificate to monitor NS servers for success. The error rate is as bad as approximately 1 in every 10. This option allows for some workaround for problematic providers. Co-authored-by: Chris Marchesi <[email protected]>
vancluever
force-pushed
the
pre-check-delay
branch
from
64a0f5c to
f899053
Compare
|
This is good. I've ran the updated test for this locally (and ironically the first workflow on this PR actually worked, just failed due to a too low drift threshold in the updated test). I'm now in Go module hell though, so I'm just going to merge and work on that in master. Thanks again for the PR @tjhiggins! |
|
@vancluever Nice! Thanks for taking it across the line. Should be useful for future power users. |
Closes: https://github.com/terraform-providers/terraform-provider-acme/issues/102
It looks like certmanager does something similar except they hardcode to 60s
https://github.com/jetstack/cert-manager/blob/master/pkg/issuer/acme/dns/dns.go#L133
Wasn't able to run the tests yet, but I added one