Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Connection config provider #3403
Changes from 13 commits
88f69a5
ccbde75
d2cbf1a
a8aa0af
79a2660
7d1b8f5
a2427b4
d219159
400b829
cc87967
4050772
ddae371
37c805c
1ecd885
a4df60d
1c6eff4
5484292
2b6460d
42d7d41
7059d5f
2837c15
6d0da47
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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?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.
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 processedconfig.connection
resolve the connection settings for only one time. So callingwill 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?
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 toinitConnectionConfigProvider
to emphasise it mutates the state.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 newclient
is attached and a newConnectionConfoProvider
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
https://github.com/knex/knex/pull/3403/files#diff-cbc1ae6887774cf34c8d4bc73dece12fR10
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:
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?
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
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:
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
where the linter is showing an error at line 105
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.
@Ali-Dalal
add await in test
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