Add periodic config/secrets reload #31
Conversation
By default it happens every 60s and the interval is configurable.
`select` does not run each case concurrently, so there's no need to synchronize here.
| for { | ||
| select { | ||
| case <-time.After(time.Duration(configReloadIntervalSeconds) * time.Second): | ||
| if err := o.InitOrReloadObsctlConfig(); err != nil { |
There was a problem hiding this comment.
Hmm, so generally this is good. But InitOrReloadObsctlConfig(), attempts to read and load obsctl from the disk first, so it won't actually reload the secrets here I think.
The reason we save it on disk and load if needed is so that we don't end up regularly calling SSO for new tokens, but that we can use the present one till full expiry (usually ~15m).
But to unblock, maybe we can add a bool param to the method to skip reading from disk step and call it with true here? :)
There was a problem hiding this comment.
Good catch!
IMHO, in all the cases where InitOrReloadObsctlConfig is called, we want it to get new tenant credentials from the k8s API (even before talking to SSO to try to get a token), because they could have changed. Otherwise we are not truly reloading any configuration as everything that will be reloaded was already there and nothing "new" is detected or used unless someone's modifying the obsctl files in the filesystem.
I added some small logic that will remove and re-add tenant in obsctl only if their OIDC credentials have changed (client id, secret, audience, issuer url, or offline access attributes).
Now, obsctl-reloader create clients a client for every single tenant discovered from secrets to confirm they are good (in the if !o.skipClientCheck block). This means SSO is called for all of them anyway. The client creation only skips calling SSO if there's a non-expired token in it, which is never the case because we never update the secret with the access tokens.
Considering that if the tenant test client creation fails we skip the error and keep loading other clients, I think we can remove that check from inside the configuration loading and let it happen when we call the Observatorium API. This way we can take advantage of the access token that is cached by obsctl-reloader for existing tenants and should only fetch new tokens from SSO for the tenants that got reloaded. WDYT, @saswatamcode?
There was a problem hiding this comment.
Ack, that makes sense! Let's test this out somehow (would need to update somebody's secret to a wrong thing and then a right thing). :)
| for { | ||
| select { | ||
| case <-time.After(time.Duration(configReloadIntervalSeconds) * time.Second): | ||
| if err := o.InitOrReloadObsctlConfig(); err != nil { |
There was a problem hiding this comment.
Ack, that makes sense! Let's test this out somehow (would need to update somebody's secret to a wrong thing and then a right thing). :)
3 participants
By default it happens every 60s and the interval is configurable.