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 5 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
…olean for ease of reading.
lqiu96
changed the title
| // 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)
There was a problem hiding this comment.
This actualy isn't a temp change. MDS already returns a non-email value for certain GKE instances so this PR is actually to handle that scenario.
There was a problem hiding this comment.
Apologies, I meant that the skip RAB is a temp change for now. Can you link the internal tracking ticket in the PR description or as a comment in the code TODO(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
There was a problem hiding this comment.
Added the log you suggested. I think the design mentions a one time INFO log hence I left that as is.
There was a problem hiding this comment.
Thanks, on a second look, I think INFO with a one-time non error message is OK.
|
Some errors in the CI: Hmm, this may be related to some changes we made into GDCH to resolve a customer issue. Can you try and pull in the latest changes into the RAB feature branch? |
3 participants
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.