Add refresh_interval config option #43
Conversation
There was a problem hiding this comment.
This is a really cool enhancement that fits into a broad set of roadmap items (when I say roadmap I mean "things entirely in my head that pop up from time to time" :) that I would generally refer to as "performance tuning". I haven't really put any effort into defining them yet, but I see where you're coming from on changing the refresh interval when you're mostly dealing with tokens that have long (or perhaps infinite) lifetimes.
As far as scalability goes, are you concerned that with a sufficient volume of entries in the data store, it will be unable to complete a refresh check in that 1 minute interval? My testing has shown that even with slower backends like GCS, iterating and reading data is pretty fast, so if most things aren't expiring it moves through the check quite quickly. If you have an idea of where that caps out I'd be interested in adding a benchmark to make sure we're keeping ourselves honest about what this plugin can/can't do...
As far as the existing test for this code goes, it's a crappy test and I need to rewrite it. It has spurious failures anyway, so it's not doing much good. I'll get back to you on that.
pkg/backend/token_authcode.go
Outdated
| storage: rd.storage, | ||
| keyer: keyer, | ||
| c, err := rd.backend.getCache(ctx, rd.storage) | ||
| if err == nil && c != nil && c.Config.RefreshInterval != 0 { |
There was a problem hiding this comment.
One case that I'm thinking about is what happens if someone disables the refreshing and then wants to subsequently re-enable it. I think, just reading this, that the check to re-enable will occur at whatever the last-used refresh interval was. So consider someone setting it to 86400, then 0, then 60. It won't run again until a day has passed (or the plugin restarts, which is also confusing?).
It would be nice to be able to somehow "wake up" this descriptor again whenever the config changes so we can re-evaluate what we should do. The invalidate() method in lifecycle.go takes care of that somewhat right now. I'll do some noodling on what options we have here and get back to you too.
There was a problem hiding this comment.
That issue occurred to me but I thought it would be so rare that it wasn't worth worrying about or doing anything that would affect performance such as always checking at most once every minute.
DrDaveD
force-pushed
the
add-refresh-interval
branch
from
71db960 to
d0059c7
Compare
This adds a new config option "refresh_interval" which controls the number of seconds between periodically refreshing all the credentials, instead of making it always be one minute. When set to zero, it disables periodic refreshing and makes it be only on demand. I want it for disabling periodic refresh (because I don't think it scales, and because my refresh token are set to not expire for 30 days), but I thought other people might want to keep refreshing with a longer interval so I thought this was a good way to make it flexible.
I wanted to write some tests for this but I'm having trouble figuring out how TestPeriodicRefresh() works. Maybe you could explain it.