feat: ServiceAccount with ID token option#4474
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
…edential potentially being None.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds the ability to use ID tokens with Service Accounts, which is a great feature for service-to-service authentication. The implementation looks solid, with changes to the ServiceAccount model, the credential exchanger logic, and corresponding unit tests.
I've left a few comments on potential improvements:
- In
service_account_exchanger.py, there are a couple of redundant checks forservice_account_credentialthat can be removed to simplify the code, as this is already handled at the beginning of the function. I also noticed an opportunity to reuse aRequestobject for better performance and consistency. - In the new tests, there's a redundant
monkeypatchcall that can be removed.
Overall, the changes are well-implemented and tested. Addressing these minor points will improve code clarity and maintainability.
src/google/adk/tools/openapi_tool/auth/credential_exchangers/service_account_exchanger.py
Outdated
Show resolved
Hide resolved
src/google/adk/tools/openapi_tool/auth/credential_exchangers/service_account_exchanger.py
Outdated
Show resolved
Hide resolved
src/google/adk/tools/openapi_tool/auth/credential_exchangers/service_account_exchanger.py
Outdated
Show resolved
Hide resolved
tests/unittests/tools/openapi_tool/auth/credential_exchangers/test_service_account_exchanger.py
Outdated
Show resolved
Hide resolved
|
Hi @lpezet , Thank you for your contribution! We appreciate you taking the time to submit this pull request.
|
I've been burnt before contributing to other Google projects, and this really is an Auth concern that, as other comments in the code mentioned, might need more thoughts than my little PR. READ: I'm not going to do much more unless you can tell me this has high chances to be merged. |
|
Hi @lpezet , I appreciate the contribution and I completely understand the hesitation—I want to make sure your effort here is well-spent and doesn’t result in a 'burnt' experience. Regarding the Auth concerns you noted: you’re right to call those out. They are exactly why a deep architectural review is necessary. However, our process is to perform that deep dive only once the PR is in a 'Green' state and adhering to our contributing guide. If you can get the CI passing, I’ll prioritize a review of the logic to give you a definitive answer on the merge path. I want to ensure we're aligned on the design before we go any further. As it stands your PR is still failing mypy-diff test. You can ignore the mypy test. |
|
I remember now: I added the extra value check before exactly because of this error: Somehow, even with the previous "if" logic raising AuthCredentialMissingError when It's a That should fix the following error as well (using |
…-python into feat/service-account-id-token
|
Hi @lpezet , you need to fix the failing unit test. |
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Problem:
ServiceAccountCredentialExchangerimplementation is restricted to OAuth2-type tokens (access token).For Service-to-Service authentication, ID token+audience is needed.
Solution:
Provide attributes in
ServiceAccountauth config to specify ID token-type auth.Testing Plan
Unit Tests:
NB: Some of
test_cli_deploy.pyfail for me but I have not changed anything there:Manual End-to-End (E2E) Tests:
In my setup I have a custom MCP Server deployed to Cloud Run running under a SA "producer", NOT public, and with a SA "consumer" granted "role/run.invoker" on that service.
Paste the following:
Checklist
Additional context