fix(auth): Skip RAB lookup in case of non-email response from MDS.#13331
fix(auth): Skip RAB lookup in case of non-email response from MDS.#13331vverman wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates ComputeEngineCredentials to return null for the Regional Access Boundary (RAB) URL if the service account is null or does not contain '@', which can happen with default service accounts in GKE environments. It also updates RegionalAccessBoundaryManager to handle a null URL gracefully, and adds a unit test to verify this behavior. There are no review comments, so I have no feedback to provide.
| if (url == null) { | ||
| future.set(null); | ||
| return; | ||
| } |
There was a problem hiding this comment.
qq, can you remind me again why we need to do future.set(null);?
There was a problem hiding this comment.
I was using a SettableFuture as a gate to deduplicate RAB refreshes which let's us set the future to indicate to dependent threads as to the result of the refresh.
However, I realized the same can be done with AtomicBoolean which is more easier to maintain. Hence updated the logic.
…olean for ease of reading.
| // In GKE environments, the default service account might return a non-email placeholder. | ||
| // Since RAB lookup requires a valid email-based service account, we skip RAB lookup | ||
| // in non-email scenarios by returning null. |
There was a problem hiding this comment.
nit: Since this is a temp change, can you link to the internal ticket tracking this (b/XXXX)
| LoggingUtils.log( | ||
| LOGGER_PROVIDER, | ||
| Level.INFO, | ||
| Collections.emptyMap(), | ||
| "Regional Access Boundary lookup is skipped for this instance because it is a non-email instance."); |
There was a problem hiding this comment.
two small nits:
- I know the spec says 1-time INFO log (I think this should be fine given the skipRAB flag). For Java SDK, I think that might be too noisy for an internal/ hidden impl. Is it possible to change the warning level?
- Can we try a bit softer wording without mentioning RAB? It may be me, but
lookup skipped ... non-email instancesounds a bit like an error and not super actionable for users.
Maybe something like (ff to change this to something clearer): Unable to retrieve this instance's email and will skip the regional request routing. Proceeding with request
In ComputeEngineCredentials when running on GKE platform, the getAccount() call may return a value which isn't an email.
In this case the right behaviour is to skip RAB lookup which is what this PR does.
Added tests.