-
Notifications
You must be signed in to change notification settings - Fork 456
CDRIVER-4489 add mongoc_oidc_cache_t
#2119
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
Conversation
…che_invalidate_token`
…en is not already cached And test behavior
Intended to assert `mongoc_oidc_cache_get_cached_token` returns non-NULL token. Assert immediately after call to clarify.
Actual delay is 100ms, but some Windows tasks observed a 99ms delay. 10ms may be too lenient.
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.
Latest changes tested with this patch: https://spruce.mongodb.com/version/68d5332f0f310c0007df4bf8
Filed CDRIVER-6101 to track assumed unrelated failure task: "Unexpected error running killAllSessions".
mongoc_oidc_cache_set_cached_token(mongoc_oidc_cache_t *cache, const char *token); | ||
|
||
// mongoc_oidc_cache_invalidate_cached_token invalidates if the cached token matches `token`. Thread safe. | ||
// mongoc_oidc_cache_invalidate_token invalidates if the cached token (if any) matches `token`. Thread safe. |
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.
// mongoc_oidc_cache_invalidate_token invalidates if the cached token (if any) matches `token`. Thread safe. | |
// mongoc_oidc_cache_invalidate_token invalidates the cached token if it matches `token`. Thread safe. |
Wording tweak suggestion (a bit too many "if"s).
bson_shared_mutex_lock(&cache->lock); | ||
|
||
bson_free(cache->token); | ||
cache->token = NULL; | ||
cache->token = bson_strdup(token); | ||
bson_shared_mutex_unlock(&cache->lock); |
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.
bson_shared_mutex_lock(&cache->lock); | |
bson_free(cache->token); | |
cache->token = NULL; | |
cache->token = bson_strdup(token); | |
bson_shared_mutex_unlock(&cache->lock); | |
bson_shared_mutex_lock(&cache->lock); | |
char *const old_token = cache->token; | |
cache->token = bson_strdup(token); | |
bson_shared_mutex_unlock(&cache->lock); | |
bson_free(old_token); |
Minor optimization: move deallocation out of critical zone + remove redundant assignment.
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.
Updated here and in mongoc_oidc_cache_invalidate_token
.
mongoc_oidc_callback_params_t *params = mongoc_oidc_callback_params_new(); | ||
mongoc_oidc_callback_params_set_user_data(params, mongoc_oidc_callback_get_user_data(cache->callback)); | ||
// From spec: "If CSOT is not applied, then the driver MUST use 1 minute as the timeout." | ||
// The timeout parameter (when set) is meant to be directly compared against bson_get_monotonic_time(). It is a | ||
// time point, not a duration. | ||
mongoc_oidc_callback_params_set_timeout( | ||
params, mlib_microseconds_count(mlib_time_add(mlib_now(), mlib_duration(1, min)).time_since_monotonic_start)); |
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.
Minor optimization: move callback param object initialization up and out of the critical section.
|
||
cache->last_called = mlib_now(); | ||
cache->ever_called = true; | ||
mongoc_oidc_callback_params_destroy(params); |
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.
Minor optimization: move callback param object deallocation down and out of the critical section.
cache->token = bson_strdup(token); // Cache a copy. | ||
|
||
unlock_and_return: | ||
bson_shared_mutex_unlock(&cache->lock); |
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.
Minor suggestion: consider scoping critical sections with {}
:
{
// Obtain write-lock:
bson_shared_mutex_lock(&cache->lock);
...
unlock_and_return:
bson_shared_mutex_unlock(&cache->lock);
}
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.
Latest changes include 51adf05 to switch from (1, min)
to (60, sec)
to work around CDRIVER-6102.
bson_shared_mutex_lock(&cache->lock); | ||
|
||
bson_free(cache->token); | ||
cache->token = NULL; | ||
cache->token = bson_strdup(token); | ||
bson_shared_mutex_unlock(&cache->lock); |
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.
Updated here and in mongoc_oidc_cache_invalidate_token
.
Patch: https://spruce.mongodb.com/version/68cd50d489be4200072d0dd3
Summary
Add internal OIDC cache:
mongoc_oidc_cache_t
Background & Motivation
mongoc_oidc_cache_t
is an internal component tested in isolation. Subsequent PRs will usemongoc_oidc_cache_t
to resolve CDRIVER-4689.mongoc_oidc_cache_t
implements behaviors described in Credential Caching.mongoc_oidc_cache_t
implements the "Client Cache" (not the "Connection Cache").mongoc_oidc_cache_set_usleep_fn
is added to permit a custom sleep function (related to CDRIVER-4736).