acme_certificate fails to destroy after certificate expired

[mengesb]

When the LE certificate has already expired, Terraform will fail its run which results in a failure to clean up the state file. Not sure if this is addressable in the DNS provider or not, I do know I can manually delete state however it would be great if there was a more graceful approach - of if in the wisdom of the provider it's attempting to delete an expired certificate it accepts that LE will return a 403 from /acme/revoke-cert; or skip this all together.

Log: https://gist.github.com/mengesb/ecc2bee6c3be26899b2f2b4c8c781e9b

[vancluever]

@mengesb thanks for the report - sounds like we're not handling the "already gone" scenario in acme_certificate. This is not a DNS provider thing but more a thing at the resource level.

If this error is distinguishable from any other 403 or if we can adequately check expiry without having to attempt to revoke it should be simple enough to fix.

[mengesb]

So looking at the trace, it looks like It attempts a POST operation before verifying the certificate end date.

Where's the mechanisim that checks the end date and then decides if it is within the period to issue a renewal? You can short-circuit this method in the case of expired certificates and simply destroy the resource without calling the POST ; or accept that a 403 on the revoke for an expired certificate is valid

[mengesb]

So I looked into certDaysRemaining, based on return (expiry - now) / 86400, nil , I'm guessing you're returning a float?

So potentially here acme/resource_acme_certificate.go#L158, we could call certDaysRemaining and if > 0, perform the code at if ok ???

[vancluever]

certDaysRemaining actually returns an int64, so we'd want to include 0 in that calculation as well (<= 0) or else errors will still be possible for a day after expiry.

Considering this also catches certs that expire day of destruction, I'd personally be more comfortable that was more exact - to the second, in other words. Maybe just move most of the code to a certSecondsRemaining function and have certDaysRemaining compose off of that.

[mengesb]

I took a stab at making the necessary commits. Trouble is, I'm not sure how to generate a certificate that expires in a short amount of time, then process the revocation code. Perhaps you can help with that... otherwise we'd have to wait potentially 90d ... unless there's a way to set the expiry to at most 1d then verify. Happy to take feedback.

I'm pretty sure I modified certDaysRemaining to correctly compose off of the new certSecondsRemaining

[vancluever]

@mengesb yeah. I figured this might be an issue.

I'll take a look when I have time. I have in the past written some lower-level state hacks to manipulate the state in the middle of test steps. We could possibly do that, but It could very well be impossible as the certs will be signed by Let's Encrypt and thus not really editable.

The other approach might be we just check for the error message, and then we can revoke the certificate out of band before trying destroy.

I'll think about it for a few days and see what I come up with.

[vancluever]

Hey @mengesb, this should now be fixed as of 1.1.2 with the merge of #42. Cheers!