Skip to content

feat: port GDCH credentials support to Node.js Auth SDK#8301

Draft
macastelaz wants to merge 1 commit into
mainfrom
gdch-credentials
Draft

feat: port GDCH credentials support to Node.js Auth SDK#8301
macastelaz wants to merge 1 commit into
mainfrom
gdch-credentials

Conversation

@macastelaz
Copy link
Copy Markdown

Add support for GDCH Credentials to the Node.js Auth SDK

Fixes #8289

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the GdchClient to support Google Distributed Cloud Hosted (GDCH) credentials, including token exchange logic using JWT assertions and integration into the GoogleAuth class. Feedback focuses on optimizing performance by using asynchronous file operations for CA certificates and replacing new Date().getTime() with Date.now() for consistency.

Comment on lines +183 to +184
const ca = fs.readFileSync(this.caCertPath);
requestOpts.agent = new https.Agent({ ca });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using fs.readFileSync in an async method blocks the event loop. It is recommended to use the asynchronous fs.promises.readFile instead. Additionally, consider caching the https.Agent or the CA certificate buffer to avoid re-reading the file and re-creating the agent on every token refresh.

Suggested change
const ca = fs.readFileSync(this.caCertPath);
requestOpts.agent = new https.Agent({ ca });
const ca = await fs.promises.readFile(this.caCertPath);
requestOpts.agent = new https.Agent({ ca });

};

if (tokenResponse.expires_in) {
tokens.expiry_date = new Date().getTime() + tokenResponse.expires_in * 1000;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use Date.now() instead of new Date().getTime() for better performance and consistency with other parts of the file (e.g., line 223).

Suggested change
tokens.expiry_date = new Date().getTime() + tokenResponse.expires_in * 1000;
tokens.expiry_date = Date.now() + tokenResponse.expires_in * 1000;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

auth: Add support for Google Distributed Cloud service identity authentication

1 participant