Add ability to customize some timeouts in MongoDB database plugin #11600
Conversation
| // Take out the backend lock since we are swapping out the connection | ||
| b.Lock() | ||
| defer b.Unlock() | ||
|
|
||
| // Take the write lock on the instance | ||
| dbi.Lock() | ||
| defer dbi.Unlock() |
There was a problem hiding this comment.
Would it make sense to also move the dbi lock/unlock up so that it's before the lambda defer func above? We're modifying dbi there so we should have that lock held before releasing it right?
There was a problem hiding this comment.
Good catch! I don't think we strictly need to since the DBI instance is being removed and shouldn't be referenced by anything else at this point, but I'll move the dbi lock up just to be safe.
| @@ -206,3 +262,29 @@ func (c *mongoDBConnectionProducer) getTLSAuth() (opts *options.ClientOptions, e | |||
| opts.SetTLSConfig(tlsConfig) | |||
| return opts, nil | |||
| } | |||
|
|
|||
| func (c *mongoDBConnectionProducer) getTimeoutOpts() (opts *options.ClientOptions, err error) { | |||
There was a problem hiding this comment.
nit: Getters don't have to include the get prefix on func names. I know we're not super consistent on this and it's less severe for private methods, but might be good to avoid this going forward.
There was a problem hiding this comment.
Agreed that we should try to be consistent about this, however I argue that this isn't a getter. This is creating a new ClientOptions instance.
There was a problem hiding this comment.
I think that c.timeoutOpts() or c.clientTimeoutOpts() would still make sense even so.
| // Set initialized to true at this point since all fields are set, | ||
| // and the connection can be established at a later time. | ||
| m.Initialized = true | ||
|
|
||
| if req.VerifyConnection { | ||
| _, err := m.Connection(ctx) | ||
| client, err := m.mongoDBConnectionProducer.createClient(ctx) |
There was a problem hiding this comment.
How come we're changing this to call createClient directly instead of having it go through m.Connection to do this? It looks like m.Connection performs all of the logic captured in if req.VerifyConnection { ... }.
There was a problem hiding this comment.
Initialize already has the mutex locked (at the top of the function), & it will deadlock (or panic - I don't recall which) when m.Connection() tries to lock it again.
There was a problem hiding this comment.
I see. Do you know if we have to call _ = c.client.Disconnect(ctx) after the Ping call here? Asking based on the comment within m.Connection of wanting to re-create the session after the ping.
There was a problem hiding this comment.
Good question. We probably should if the Ping fails based on the comment on Disconnect itself to prevent any sort of resource leak. If it succeeds, we set it into the mongoDBConnectionProducer so it is reused the next time Connection is called.
This PR adds the ability to customize
SocketTimeout,ConnectTimeout, andServerSelectionTimeoutwithin MongoDB connections. Vault passes these values to the MongoDB library when it makes a client connection.This also makes some improvements around throughput. Previously, when an operation came to the plugin (creating/destroying a user, etc.), it would lock a mutex for the entirety of the operation. This resulted in the plugin forcing all requests to be single-threaded. This moves the mutex lock a bit lower in the plugin to try to limit its scope and allow multiple threads to be manipulating users at the same time. I did run a bunch of tests to try to produce races and didn't find any within the plugin code. However, I did find a race within the DB engine which I think is fixed in
builtin/logical/database/path_rotate_credentials.go. The lock was being unlocked before the connection was being being closed & removed from the DBI map (b.connections). This probably wasn't a serious issue in practice as the map isn't changed in this manner very frequently.