feat: Add support for Agent Identity bound tokens #1821
Conversation
This change introduces support for requesting certificate-bound access tokens for Agent Identities on GKE and Cloud Run.
| has_logged_warning = True | ||
| pass | ||
|
|
||
| time.sleep(interval) |
There was a problem hiding this comment.
This doesn't matter at all, but I think the sleep could be inside the except, to remove that pass
There was a problem hiding this comment.
Thanks for the suggestion! I've kept the time.sleep() outside the except block to handle two
distinct scenarios while polling for the certificate:
- The certificate configuration file (GOOGLE_API_CERTIFICATE_CONFIG) is not yet available. This
triggers the except block. - The configuration file is available, but the certificate file path specified inside it is not
yet on the filesystem. This case does not trigger an exception.
In both scenarios, we need to pause and retry. If we move the sleep into the except block, there won't be any pause and sleep for the second scenario would .
I've added a comment to the code to make this polling logic clearer for future developers.
google/auth/_agent_identity_utils.py
Outdated
| try: | ||
| from cryptography import x509 | ||
| from cryptography.x509.oid import ExtensionOID | ||
| except ImportError: |
There was a problem hiding this comment.
Do we have any tests that try running these functions without cryptography imported?
I don't really have enough context to know if that would be necessary, but IIRC I think it's pretty easy to un-import for specific test functions, so maybe consider if it would be
| ) | ||
| assert not _agent_identity_utils._is_agent_identity_certificate(mock_cert) | ||
|
|
||
| def test__is_agent_identity_certificate_not_spiffe_uri(self): |
There was a problem hiding this comment.
| def test__is_agent_identity_certificate_not_spiffe_uri(self): | |
| def test_is_agent_identity_certificate_not_spiffe_uri(self): |
|
|
||
| @mock.patch("google.auth._agent_identity_utils.parse_certificate") | ||
| @mock.patch("google.auth._agent_identity_utils.get_agent_identity_certificate_path") | ||
| def test_get_and_parse_agent_identity_certificate_success( |
There was a problem hiding this comment.
Lets add a test case to simulate a sequence of failures followed by success:
Iteration 1: os.path.exists(config) -> False
Iteration 2: os.path.exists(config) -> True, but open() raises IOError (incomplete write).
Iteration 3: open() succeeds, but json.load raises ValueError (malformed JSON).
Iteration 4: Everything succeeds.
Assert that get_agent_identity_certificate_path returns successfully after these 4 attempts
daniel-sanche
added
the
do not merge
|
Added do not merge, for now, so this can be tested before release. We plan to merge this around early December |
4 participants
This change introduces support for requesting certificate-bound access tokens for Agent Identities on GKE and Cloud Run. The design doc: go/sdk-agent-identity