Continuously retry updating the cert secret #280
Conversation
Uses retry with exponential backoff to retry updating the cert secret if there's an error. Sync's with caller via a stop channel, caller waits on stop channel before starting a new update thread.
Co-authored-by: Tom Proctor <[email protected]>
…ert-secret-reliably
| result.Cert = []byte(cert) | ||
| result.Key = []byte(key) | ||
| if err != nil { | ||
| return result, err |
There was a problem hiding this comment.
I know that this is being done elsewhere above the code, but I wonder if it's better to return an empty object since result has been partially set at this point (unless that's something that we explicitly want to do).
result.CACert = s.caCert
There was a problem hiding this comment.
I think this is probably technically ok because the result is discarded if there's an error, but I'll let Theron confirm.
There was a problem hiding this comment.
It is indeed being discarded in notify.Run() if there's an error. Which is good, because the next block there is comparing the new result to the old one, so it would also think an empty result is a new result. All this to say I think we might as well leave it up to the caller when returning an error, and technically s.caCert is valid at this point and will continue to be used on the next pass.
helper/cert/source_gen.go
Outdated
| } | ||
| } | ||
| if s.LeaderElector != nil { | ||
| s.retryUpdateSecret(ctx, leaderCh, result) |
There was a problem hiding this comment.
This sync call might affect <-retryTicker.C in Notify.Run(). For instance, if this call blocks for 6s it will be skipping the next tick (and every increment of 5 seconds that this blocks skips 1 tick). This may or may not be fine depending if there are other things within that loop that is expecting the ticker to run every 5 seconds.
There was a problem hiding this comment.
This looks fine to me, the Notify.Run() loop only notifies upstream when there is a new result to report, so I think it should be ok to reattempt until we get something worth reporting, and we're still cancellable via the context.
| result.Cert = []byte(cert) | ||
| result.Key = []byte(key) | ||
| if err != nil { | ||
| return result, err |
There was a problem hiding this comment.
I think this is probably technically ok because the result is discarded if there's an error, but I'll let Theron confirm.
helper/cert/source_gen.go
Outdated
| } | ||
| } | ||
| if s.LeaderElector != nil { | ||
| s.retryUpdateSecret(ctx, leaderCh, result) |
There was a problem hiding this comment.
This looks fine to me, the Notify.Run() loop only notifies upstream when there is a new result to report, so I think it should be ok to reattempt until we get something worth reporting, and we're still cancellable via the context.
Co-authored-by: Calvin Leung Huang <[email protected]>
Retries until the certificate would be renewed. Actually return an error if all attempts time out.
Uses retry with exponential backoff to retry updating the cert secret if there's an error. Retries until the certificate would be renewed. Co-authored-by: Tom Proctor <[email protected]> Co-authored-by: Calvin Leung Huang <[email protected]>
Uses retry with exponential backoff to retry updating the cert secret
if there's an error. Sync's with caller via a stop channel, caller
waits on stop channel before starting a new update thread.