feat: Support periodic reload for api key #45
Conversation
lym953
force-pushed
the
yiming.luo/api-key-force-reload
branch
from
bab0903 to
e0e68a8
Compare
lym953
changed the title
crates/dogstatsd/src/api_key.rs
Outdated
| None => false, | ||
| // Reload only if it has been longer than reload interval since last reload | ||
| Some(reload_interval) => { | ||
| Instant::now() > last_reload_time + reload_interval |
There was a problem hiding this comment.
Due to memory order semantics, this could still return true twice for two different threads (rare! But possible).
You may want acquire/release for the last_reload_time variable.
My very favorite post in this is here, which I'm sure you'll enjoy.
There was a problem hiding this comment.
Nice post! Glad to learn about this.
I added a separate lock loading_api_key, so we don't need to lock api_key or last_reload_time when calling secret manager. Then I added acquire/release semantics for this lock. Could you take a look?
crates/dogstatsd/src/api_key.rs
Outdated
| // Check if reload is needed without acquiring write lock for api_key. | ||
| // If no, return the api key directly. If yes, acquire the write lock and reload the api key. | ||
| if self.should_load_api_key().await { | ||
| let mut api_key_write = api_key.write().await; |
There was a problem hiding this comment.
Would it be simpler to model the API key as a combined struct so we write-lock only once?
struct ApiKeyState {
api_key: Option<String>,
last_reload_time: Option<Instant>,
}
api_key_state: Arc<RwLock<ApiKeyState>>Then you could use try_lock and if it fails, infer that another thread is already refreshing the key?
I'm not sure what you'd actually DO in the case where Thread 1 is refreshing the key and Thread 2 hits the refresh, which will be very likely as it's time based.
Presumably you either assume that the old key is still valid for some time, so thread 2 can simply keep using the old key.
If that's invalid, you could wait until the lock drops and read the new key, but that will block all flushing.
Today it looks like this will block flushing until we refresh the key anyway, is that expected?
There was a problem hiding this comment.
Would it be simpler to model the API key as a combined struct so we write-lock only once?
Done
Presumably you either assume that the old key is still valid for some time, so thread 2 can simply keep using the old key.
I think we can assume this and require customers to keep a grace period for the old key.
Today it looks like this will block flushing until we refresh the key anyway, is that expected?
I didn't figure out a good way to avoid this before, but it should be fixed in the latest commit.
| // This prevents duplicate loads from multiple threads | ||
| if self.should_load_api_key().await { | ||
| let api_key_value = (resolver_fn)().await; | ||
| *api_key_state.write().await = ApiKeyState { |
There was a problem hiding this comment.
Acquire the RwLock for api_key after resolver_fn() is called, so the AWS secret manager call won't block reads from other threads.
lym953
force-pushed
the
yiming.luo/api-key-force-reload
branch
from
ebeeedf to
bb7aa47
Compare
2 participants
What does this PR do?
Support periodic reload of api key, i.e. when api key is read, if it has been longer than a threshold since the last load, then reload the api key.
Motivation
Some customers regularly rotate their api key in a secret. We need to provide a way for them to update our cached key.
DataDog/datadog-lambda-extension#834
Additional Notes
To use this in Lambda extension.
Jira: https://datadoghq.atlassian.net/browse/SVLS-7572
Describe how to test/QA your changes
Added unit tests.
Will also test with Lambda extension.