impl(auth): add universe_domain support for MDS#5329
impl(auth): add universe_domain support for MDS#5329alvarowolfx merged 2 commits intogoogleapis:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5329 +/- ##
========================================
Coverage 97.79% 97.79%
========================================
Files 220 220
Lines 45938 46039 +101
========================================
+ Hits 44924 45025 +101
Misses 1014 1014 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/auth/src/credentials/mds.rs
Outdated
| let universe_domain = response.ok(); | ||
| let _ = self.universe_domain.set(universe_domain.clone()); |
There was a problem hiding this comment.
If there is an error, the None will get cached indefinitely since we can't overwrite it due to the OnceLock. Even though the fetching code will be run again it won't be actually able to store future successful results.
Can we also add a test covering this case?
There was a problem hiding this comment.
Nevermind it seems like this is intentional, and the Some() will hold an option inside it.
A test covering this would be nice though anyway.
There was a problem hiding this comment.
This is going to be improved with retries. It's a tricky think since in normal environments (GDU - googleapis.com), this call will fail, since the universe domain endpoint doesn't exist (404) on MDS. But we don't want to keep hitting it on a GDU, since in some parts it can make calls slower if is always trying to resolve the universe domain. With retries, I'm only gonna save on the OnceLock if the error is not retryable. I'll at least add a comment saying that
There was a problem hiding this comment.
actually, I can already write that code, since the error being returned is the same and I already can check if is transient. Working on it
2 participants
Towards #3646