Make auto-renew use built-in renew function #3392
Conversation
backend/internal/certificate.js
Outdated
| @@ -30,6 +30,7 @@ const internalCertificate = { | |||
| intervalTimeout: 1000 * 60 * 60, // 1 hour | |||
| interval: null, | |||
| intervalProcessing: false, | |||
| renewBeforeExpirationBy: [7, 'days'], | |||
There was a problem hiding this comment.
According to the certbot docs, renew (which is what was being used prior) will find all certificates that expire within 30 days. Maybe I should change this to 30 days?
There was a problem hiding this comment.
I would prefer 30 days to avoid expiration notification emails from LE.
stevecrozz
force-pushed
the
auto-renew-uses-bulitin-renew
branch
from
57e70bb
to
c17273a
Compare
|
I tested this image. I think there are still some issues. I have 3 certificates, all needed to be replaced. |
|
@flightsupport thanks for testing! This makes me think I'll need to run these renew calls in sequence instead of in parallel. It seems like they have to run one-by-one. I actually don't have a good test environment to verify, but I will attempt this change anyway. |
|
I pulled the latest image. Now the renewals work. However for every certficate there's still an error (might by unrelated):
|
|
@stevecrozz did you have a look at the error to see if it is related to this PR? |
I think it probably is related to my work here unfortunately. I need to step through with a debugger because I don't see the issue yet. One simpler thing you could try if you feel like it is to remove that 'catch' clause and see if you can get a real stack trace. |
|
I can confirm that this error is being caused by this change and because it's throwing, some expected code paths are not being taken. I'll leave notes on the affected file soon. |
backend/internal/certificate.js
Outdated
| @@ -46,58 +47,45 @@ const internalCertificate = { | |||
| internalCertificate.intervalProcessing = true; | |||
| logger.info('Renewing SSL certs close to expiry...'); | |||
There was a problem hiding this comment.
Can you please change this line to:
logger.info('Renewing SSL certs expiring within ' + internalCertificate.renewBeforeExpirationBy[0] + ' ' + internalCertificate.renewBeforeExpirationBy[1] + ' ...');
d2149af
to
9c54d1b
Compare
|
Docker Image for build 5 is available on DockerHub as Note: ensure you backup your NPM instance before testing this PR image! Especially if this PR contains database changes. |
|
Test with build 5. |
As a complete newcomer to this project, this is my attempt to fix certificate auto-renew issues #1916 where manual renew works, but automatic renew does not. This changes proposes we replace the functionality inside automatic renewal with a call to the same function used for manual renewal.
Specifically, I have replaced the certbot 'renew everything' command with a DB-driven one-by-one renewal. Determining which certificates need renewal is a pretty naive time based DB query, and access control is completely bypassed with
Promise.resolve({ permission_visibility: 'all' }because I assume it isn't needed for a background job.I'm not sure yet how to best contribute to this project, and I'm sure there are some issues with this so please do guide me as I'm brand new here.