-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Connection config provider #3403
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
Update client to leave the responsibility to determine connection settings type to connectionProvider Update postgres dialect aquireConnection function
class ConnectionConfigProvider { | ||
constructor(client) { | ||
this.client = client; | ||
this.resolvedConnectionSettings = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not resolve settings at this step already? At the very least we can definitely do that if it's a static config.
|
||
async resolveConnectionConfig() { | ||
if (this.resolvedConnectionSettings) return this.resolvedConnectionSettings; | ||
if (isFunction(this.client.config.connection)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do the check once in constructor and set some flag after that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give more details about this, please?
if we need to manage this in ConnectionConfigProvider
constructor, the how-to manipulate the promise as the type of the function is async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is this:
-
In constructor check if it's a function. If it is - set
this.isConfigSourceFunction = true
and do nothing else. If it is not - setthis.isConfigSourceFunction = false
andthis.resolvedConnectionSettings = cloneDeep <...>
-
in
resolveConnectionConfig
check ifthis.resolvedConnectionSettings
is set, if it is, return, if it is not andisConfigSourceFunction
, resolve from function, if neither, probably throw an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line in ConnectionConfigProvider
class does the same, right?
async resolveConnectionConfig() {
if (this.resolvedConnectionSettings) return this.resolvedConnectionSettings;
the lines after check for the type of the connection. if it is a function resolve it, other wise, deepclone it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially. What I'm saying is that everything that can be moved from resolveConnectionConfig
to constructor should ideally be moved, resolveConnectionConfig
should be as simple as possible.
Current implementation is different from what I've described.
const settings = Object.assign({}, this.connectionSettings); | ||
const client = this; | ||
return new Bluebird(async (resolver, rejecter) => { | ||
const settings = await client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we might want to set connectionConfigProvider each time for a new connection anew and not just set it once on a client when it is created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in ConnectionConfigProvider
class I processed config.connection
resolve the connection settings for only one time. So calling
const settings = await client
.connectionConfigProvider()
.resolveConnectionConfig();
will return the processed object direclty without processing anything anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it?
connectionConfigProvider() {
return new ConnectionConfigProvider(this);
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any recommendation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I've said, you can call connectionConfigProvider
once after client is created and configuration is known. And probably it should be renamed to initConnectionConfigProvider
to emphasise it mutates the state.
lib/dialects/mssql/index.js
Outdated
const settings = await client | ||
.connectionConfigProvider() | ||
.resolveConnectionConfig(); | ||
// const settings = Object.assign({}, this.connectionSettings); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is needed. if the config is passed as an async function, then this will cause a problem without resolving the config.
Also, this line
const settings = Object.assign({}, this.connectionSettings);
is not needed any more as ConnectionConfigProvider
is taking the responsibility for this.
I will clarify the need for this PR again in case I miss any clarification.
The main purpose is we need to fetch the config of the DB from an async store (AWS SSM as an example) and this requires passing the config as an async function in order to wait for the config to be loaded from the promise.
Travis 1st stage passes but the other stages failed and I believe it is because of oracledb dialect which fails only in one of the test cases. Can you have a look please and help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please delete the unneeded commented out code from PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: Oracle
this seems directly related:
1) Oracledb driver tests
OracleDb externalAuth
externalAuth and connectString should be sent to the getConnection:
AssertionError: expected getConnection to have been called exactly once, but it was called 0 times
at Context.<anonymous> (test/unit/dialects/oracledb.js:36:25)
I believe you'd need to update some of mocks in oracle tests, as call hierarchy has obviously changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: Oracledb
I believe the issue is because of switching the bludbird callback to async in line 79
const asyncConnection = new Bluebird(async function(resolver, rejecter) {
//...res of the code
}
where the linter is showing an error at line 105
client.driver.fetchAsString = client.fetchAsString;
error:
possible race condition client.driver.fetchAsString might be reassinged based on an outdated value of client.driver.fetchAsString
need to debug the code but the issue I can't get the tests working on my machine due to insufficient storge issue (mac 128 GB)
any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is still relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kibertoad I've checked the tests. only Oracle dialect fails. and honestly, I have no idea why. can you please help? I will do one more round to try to find the problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
externalAuth and connectString should be sent to the getConnection
add await in test
- knexInstance.client
+ await knexInstance.client
.acquireRawConnection()
.then(function(resolve) {}, function(reject) {});
@Ali-Dalal Do you need any help for finishing the PR? |
@kibertoad Yes please. I need clarification on this point
Do we need to resolve the config if it is a function inside the constructor? remember the type of the function is async I am a bit confused because how to resolve connectionSettings in client.js since it is async?
and I agree with you, the dialect is not responsible for processing or taking care of the connection config. But the question, how to place this code inside client.js? |
@Ali-Dalal As I've said before, we can't resolve config from function since it could be async. We could do it if it's a static config, though, but disregard this part, we can refactor it later. Important part at this point is to finalize the implementation :). |
@maximelkin Any chance you could take a look at the Oracle problem to see what is wrong here? I don't have readily available instance nor configured Docker to test easily myself, unfortunately. |
@Ali-Dalal Can you pull changes from master, btw? I wonder if latest fixes in master would resolve the issue. |
// If external authentication dont have to worry about username/password and | ||
// if not need to set the username and password | ||
const oracleDbConfig = client.connectionSettings.externalAuth | ||
? { externalAuth: client.connectionSettings.externalAuth } | ||
const connectionSettings = await client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use async inside new promise callback
it will not catch errors, so you get unhandledRejection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ali-Dalal This needs to be addressed
eslint/eslint#11899 |
@kibertoad @maximelkin guys can you help in the last error from eslint? check travis for more information
|
@Ali-Dalal as @maximelkin has mentioned, "just mark rule require-atomic-updates as ignored, it will be removed shortly due to false-positives* |
@kibertoad @maximelkin done. can you guide me where is the best place to write a unit test for testing async config provider? |
@kibertoad I will write unit tests under |
const client = this; | ||
return new Bluebird(async (resolver, rejecter) => { | ||
const settings = await client | ||
.connectionConfigProvider() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in this line we are still always initializing a new ConnectionConfigProvider, meaning we will resolve credentials again on each connection.
Can we adjust connectionConfigProvider()
method to lazy-instantiate and not reset each time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a new knex
instance defined, a new client
is attached and a new ConnectionConfoProvider
as well and once we call resolve connection, for the first time it will execute the function if the connection is a function then it will mutate the state of the client and set the connection to the resolved one.
I tried it on my machine and the connection is resolved only one time and for the 2nd time it loads the config from
async resolveConnectionConfig() {
if (this.resolvedConnectionSettings) return this.resolvedConnectionSettings;
https://github.com/knex/knex/pull/3403/files#diff-cbc1ae6887774cf34c8d4bc73dece12fR10
Can we adjust connectionConfigProvider() method to lazy-instantiate and not reset each time?
can you give some examples how I can solve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are seeing connection being resolved only once because it is being reused from the connection pool and not recreated on every request - for new connections it is likely that same acquireRawConnection
will be called again and you will have to resolve credentials again.
I'll take a swing at the branch sometime this week to see how to improve the situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easy way to check with multiple connections is to create transactions and never commit / rollback them. In that way the old connections cannot be reused from pool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guys, sorry for coming in late in the process, but I noticed this PR when I was considering opening a similar issue, with the use case in mind being support of Amazon RDS IAM authentication.
For this use case though, the resolved configuration can't be cached indefinitely, because the password needs to be changed every 15 minutes.
How about adding an optional field to the returned configuration object, with a sync function determining whether or not the cached config can be reused?
So the connection config provider code can look something like this:
const { token, tokenExpiration } = await someCallToGetTheToken();
return {
host : 'your_host',
user : 'your_database_user',
password : token,
database : 'myapp_test',
expired = () => {
return tokenExpiration <= Date.now();
}
}
expired
is a synchronous function, returning a boolean determining whether the configuration has expired or or not. If it hasn't expired, it can be reused. If not specified, the previously-resolved configuration is always reused.
What do you think?
Superseded by #3497 |
As requested in this PR #3364
@elhigu @kibertoad Can you please check? if the changes are ok then I will add unit test and update the rest of the dialects