-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: support shorter refresh durations #103
Conversation
internal/alloydb/instance.go
Outdated
d := certExpiry.Sub(now) | ||
if d <= time.Hour { | ||
return 55 * time.Minute | ||
} | ||
return time.Until(now.Add(time.Duration(d / 2))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is quite what we want to do. If something odd happens and a certificate with a duration of less than 1 hour incurs, this will still wait 55 minutes to refresh that cert.
What we really want the logic to be is in the certExpiry is <= 1hr (although maybe ~65 minutes gives us a better margin of error) then we use a window of 5 minutes.
How about something like this:
// default window is half of the certs expected duration
w := time.Until(certExpiry) / 2
if w < time.Hour {
w = 5 * time.Minute
}
return w
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've extended the edge case checking.
The change here updates the calculation of how long to wait before starting a refresh cycle. This is a port of GoogleCloudPlatform/alloydb-go-connector#103.
The change here updates the calculation of how long to wait before starting a refresh cycle. This is a port of GoogleCloudPlatform/alloydb-go-connector#103.
No description provided.