-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Allow connector to accept loginAuth to pass it down to sqlAdminFetcher #204
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
@Anoupz Thanks for the contribution! Would you mind maybe adding a quick test to https://github.com/GoogleCloudPlatform/cloud-sql-nodejs-connector/blob/main/test/connector.ts that covers passing in a credential? |
What about the
|
@Anoupz the way the Connector works is it has two credential Google Auth objects, each with different access scopes. The Both of these credentials must belong to the same IAM Principal in order to successfully connect with IAM database authentication. Thus, we need to support setting both of these to the custom credentials passed in. In Python we can pass in a single credentials object because the |
Humm, When I tested this I did not encounter any issue. Let me check again |
@Anoupz are you using IAM database authentication to login for your use-case or regular username/password database auth? |
I am using IAM database authentication. |
Sorry, I was not able to reproduce it. Can anyone help me to update this and what should be updated with? I only pass the loginAuth since the getEphemeralCertificate function would be able to use the passed in loginAuth and get the access token and the client. |
The PR here might be a good thing to build on: #210 and will make it easy to pass in an auth object. |
Thanks @enocom. @edosrecki you can combine these two changes or I can merge your changes on mine. Feel free to edit it. |
@jackwotherspoon, @Anoupz, @ruyadorno Can't we do the following: constructor(options?: { authClient?: AuthClient }) {
const adminAuth = new GoogleAuth({
authClient: options.authClient,
scopes: ['https://www.googleapis.com/auth/sqlservice.admin'],
});
const loginAuth = new GoogleAuth({
authClient: options.authClient,
scopes: ['https://www.googleapis.com/auth/sqlservice.login'],
})
// ...
} If a custom |
That looks good to me. @ruyadorno WDYT? |
@jackwotherspoon @enocom @ruyadorno @Anoupz I just tried out what I suggested above, and it does not work due to a TypeScript error.
Therefore we cannot pass just any I created an issue here: googleapis/google-api-nodejs-client#3357 to start the discussion. |
Nevermind, @enocom clarified to me that's not even the same feature, it's just misleadingly sharing the same name. |
Change Description
Allow connector to accept custom auth as arg to be able to pass it down to sqlAdminFetcher which can rely on custom auth or default auth if none is provided.
Relevant issues: