feat: Agentic Identities in Cloudrun#854
feat: Agentic Identities in Cloudrun#854vverman wants to merge 3 commits intogoogleapis:agentic-identities-cloudrunfrom
Conversation
feywind
left a comment
feywind
left a comment
There was a problem hiding this comment.
There are a few suggestions in here, but it looks fine! If you want to make changes, I'll have to re-approve due to new GitHub rules.
| * Helper function to delay execution. | ||
| * @param ms Time to sleep in milliseconds. | ||
| */ | ||
| async function sleep(ms: number): Promise<void> { |
There was a problem hiding this comment.
I've been doing it like this for a while now too, but out of curiosity, I looked it up, and there's an official alternative now:
https://nodejs.org/docs/latest-v18.x/api/timers.html#timerspromisessettimeoutdelay-value-options
| @@ -0,0 +1,20 @@ | |||
| -----BEGIN CERTIFICATE----- | |||
There was a problem hiding this comment.
Just double-checking, this isn't something that's going to leak info for being in the repo?
| } | ||
| } | ||
| } catch (error) { | ||
| // Ignore errors during polling, will retry. |
There was a problem hiding this comment.
Might be nice to put a log (log.debug?) of some kind here so users can see the retries if they want.
| await sleep(interval); | ||
| } | ||
|
|
||
| throw new Error( |
There was a problem hiding this comment.
It might be nice to dump this out to log.error too.
| > { | ||
| // Check opt-out. | ||
| if (process.env[PREVENT_SHARING_ENV_VAR]?.toLowerCase() === 'false') { | ||
| return undefined; |
There was a problem hiding this comment.
Potentially a good place for a log.info or log.debug to confirm for users.
| if (!certConfigPath) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Might be a nice place for a log.debug, telling the user the library has picked up their env var.
| import * as fs from 'fs'; | ||
| import {log as makeLog} from 'google-logging-utils'; | ||
|
|
||
| const log = makeLog('google-auth-library:agentidentity'); |
There was a problem hiding this comment.
Thanks for doing this! I'm putting a few suggested places below that we might want more logging.
|
|
||
| beforeEach(async () => { | ||
| sandbox = sinon.createSandbox(); | ||
| clock = TestUtils.useFakeTimers(sandbox); |
There was a problem hiding this comment.
I dunno if you've hit this, but I've run into several problems lately where sinon is actually hooking a lot more than it used to in useFakeTimers (stuff like process.nextTick). You can turn off the ones you don't want here. It may not affect this, but FYI to keep in mind.
| @@ -0,0 +1,193 @@ | |||
| // Copyright 2025 Google LLC | |||
There was a problem hiding this comment.
I think it's not necessary, but if you wanted to check any of the log lines coming out of agentidentity.ts I can provide examples of how to hook that stuff during tests.
feywind
changed the title
2 participants
This feature implements the ability for agentic identities to authenticate themselves via X509 cert bound tokens. We are limiting the scope here to only cloud run based agentic workloads.
Please refer this design doc for more information on this feature