fix: restore auth_token initialization for secret and parameter manager clients#5371
Open
shaun0927 wants to merge 2 commits intogoogle:mainfrom
Open
fix: restore auth_token initialization for secret and parameter manager clients#5371shaun0927 wants to merge 2 commits intogoogle:mainfrom
shaun0927 wants to merge 2 commits intogoogle:mainfrom
Conversation
Both integration helpers advertised an auth_token constructor path, but the implementation instantiated google.auth.credentials.Credentials directly. That base class is abstract, so callers hit a TypeError before any request could be made. This patch switches both helpers to a concrete OAuth2 Credentials object and adds regression coverage that exercises the real constructor path. The SecretManager helper also now rejects service_account_json + auth_token up front so its behavior matches its own documented contract. Constraint: Keep the change narrow to the validated auth helper regressions Rejected: Leave the existing mocked constructor tests in place | they masked the real runtime failure Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep token-path tests using the real credentials class so abstract-constructor regressions do not reappear Tested: .venv/bin/pytest tests/unittests/integrations/secret_manager/test_secret_client.py tests/unittests/integrations/parameter_manager/test_parameter_client.py -q Not-tested: Full repo test suite; lint tooling was not installed in the local venv
Collaborator
|
Hi @shaun0927 , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Please fix formatting errors by running autoformat.sh |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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):
2. Or, if no issue exists, describe the change:
Problem:
Both
SecretManagerClientandParameterManagerClientadvertise anauth_tokenconstructor path, but the implementation instantiatesgoogle.auth.credentials.Credentials(...)directly. That base class is abstract, so callers hit aTypeErrorbefore the underlying Google Cloud client is constructed.SecretManagerClientalso documents thatservice_account_jsonandauth_tokenare mutually exclusive, but the current implementation silently accepts both inputs and ignores the token.Solution:
google.oauth2.credentials.Credentials(token=auth_token)for theauth_tokenpath.SecretManagerClienttest that confirms conflicting credential inputs raiseValueError.Testing Plan
Unit Tests:
Passed locally:
Manual End-to-End (E2E) Tests:
I also re-ran the original local reproductions with patched cloud clients to confirm the behavior change:
And for the conflicting-input case:
Checklist
Additional context
I intentionally kept this PR focused on the reproducible auth helper regressions that are still present in the latest stable release and current
main.