-
Notifications
You must be signed in to change notification settings - Fork 52
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
azure: Don't panic when token refresh fails #391
azure: Don't panic when token refresh fails #391
Conversation
Instead of crashing the whole broker when we fail to refresh an `azblob.TokenCredential`, we should retry forever and log noisily instead.
So, I validated that this change does what it claims to: if there's an error fetching/refreshing an access token for our service account, instead of panicking we will log noisily and retry in an exponential backoff capped to once every 5 minutes. That being said... is this actually what we want to do in all cases? As I was trying to find ways to cause this error, I came up with 4 different possible causes:
Think it's worth fleshing out validation to cover the first two cases here? Confirmed that if I provide it invalid azure creds it fails in a noisy loop and backs off as designed:
And also if you give it a bad tenant ID it'll also backoff and retry. I wasn't able to reproduce the original error that caused here (looked like a transient network or service error). Note that it backs off up to 5m and then no more.
|
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.
LGTM
Instead of crashing the whole broker when we fail to refresh an
azblob.TokenCredential
, we should retry forever and log noisily instead.This change is