Use cert rotate parameter #2541
Conversation
| ) | ||
| if err != nil { | ||
| return results.WithError(err) | ||
| } | ||
|
|
||
| // handle CA expiry via requeue | ||
| results.WithResult(reconcile.Result{ | ||
| RequeueAfter: certificates.ShouldRotateIn(time.Now(), httpCa.Cert.NotAfter, rotation.RotateBefore), | ||
| RequeueAfter: certificates.ShouldRotateIn(time.Now(), httpCa.Cert.NotAfter, caRotation.RotateBefore), |
There was a problem hiding this comment.
Should we also make that call for the other certs?
There was a problem hiding this comment.
I am not sure I follow your comment here @sebgl. Do you mean we should requeue within the validity period of the individual certificates and not just before the CA expires?
There was a problem hiding this comment.
Yeah I mean also requeueing after we reconcile HTTP certificates below. Since we have caRotation and certRotation that can be different, it would make sense to make sure we requeue before any of these are reached? Maybe I'm missing something here.
There was a problem hiding this comment.
Should we not requeue on the minimum of the two?
There was a problem hiding this comment.
Yes! That's why I thought we would call results.WithResult () twice (one for CA, one for cert 10 lines below), and then we let the results aggregation do its job of picking the most appropriate requeue?
There was a problem hiding this comment.
SGTM (I always forget that we already pick the shortest requeue in the aggregation)
| ) | ||
| if err != nil { | ||
| return results.WithError(err) | ||
| } | ||
|
|
||
| // handle CA expiry via requeue | ||
| results.WithResult(reconcile.Result{ | ||
| RequeueAfter: certificates.ShouldRotateIn(time.Now(), httpCa.Cert.NotAfter, rotation.RotateBefore), | ||
| RequeueAfter: certificates.ShouldRotateIn(time.Now(), httpCa.Cert.NotAfter, caRotation.RotateBefore), |
There was a problem hiding this comment.
I am not sure I follow your comment here @sebgl. Do you mean we should requeue within the validity period of the individual certificates and not just before the CA expires?
pebrc
dismissed
their stale review
Let's do the additional WithResult call with the cert level rotation params before merging
|
Yep! That was the plan, sorry I was on call so I let this sit. I should have put in an update though indicating the changes @sebgl suggested out of band |
| @@ -74,6 +74,15 @@ func Reconcile( | |||
| if err != nil { | |||
| return results.WithError(err) | |||
| } | |||
|
|
|||
| primaryCert, err := certificates.GetPrimaryCertificate(httpCertificates.CertPem()) | |||
There was a problem hiding this comment.
It is a bit unfortunate that we have to parse the PEM again that we just parsed and or encoded in the reconcile function. But I guess changing this would be a larger refactoring of our certificate generation logic, so happy to track this in a follow up task.
There was a problem hiding this comment.
Agreed. I wasn't very happy about it, but the needs of both of them are discrete enough (one needs to construct the whole chain, the other just needs the expiration date of one cert) that I couldn't think of a better way to fix it without a larger scale change. One option might be for ReconcileHTTPCertificates() to return a Results, which could include errors and a resync time.
Fix #2540
Previously we did not actually use the certificate rotation parameters, and only used the CA rotation parameters.
Also increases the NotBefore buffer from 1 minute to 10 minutes for the CA to match the certificate buffer. If clock skews are that far out there's probably other issues, but it definitely happens.
I didn't add tests for the new behavior because currently we do not really have tests at this level. I can def see how adding integration tests for this would be helpful though, so if you want me to add them I'm happy to do so.