Skip to content

feat(NODE-6392): add timeoutMS support to ClientEncryption helpers part 1 #4281

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 28, 2024
Merged

feat(NODE-6392): add timeoutMS support to ClientEncryption helpers part 1 #4281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ceaef63
feat(NODE-6392): add timeoutMS support to ClientEncryption helpers
nbbeeken Oct 25, 2024
c27df1d
chore: fixes
nbbeeken Oct 28, 2024
a0a0637
fix: ensure key is inserted
nbbeeken Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/client-side-encryption/auto_encrypter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,10 @@ export class AutoEncrypter {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore: TS complains as this always returns true on versions where it is present.
if (net.getDefaultAutoSelectFamily) {
Object.assign(clientOptions, autoSelectSocketOptions(this._client.options));
// AutoEncrypter is made inside of MongoClient constructor while options are being parsed,
// we do not have access to the options that are in progress.
// TODO(NODE-NODE-6449): AutoEncrypter does not use client options for autoSelectFamily
Object.assign(clientOptions, autoSelectSocketOptions(this._client.s?.options ?? {}));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not opposed to adjusting our usages of client.options to client.s.options but unless we have some sort of mechanism of enforcing it, I worry that its a losing battle and that usages of client.options will slip in (like many imports from bson got past us in review until we linted for it).

If this is something we care about - maybe we could defer this and see if we can write a lint rule? We could probably do this with a typescript-eslint plugin.

Or deprecate options in favor of a non-Object.freeze() getter and then switch to the other getter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unlike the BSON import which is just a 'nice-to-have' single location for all BSON interactions, the options getter on the client has real cost spreading and freezing on every access, we fixed a ~10% performance regression when we reverted its introduction to the Session constructor, so it is worth correcting it where we notice it.

I do agree though we should be able to remove the s.options and change the getter to just storing the options directly on the client without breaking anything.

This specific case is a bug: https://jira.mongodb.org/browse/NODE-6449

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did consider making these changes to main, but most of the usages are introduced for CSOT, maybe this will close the small gap in perf we saw. fyi: @W-A-James

Copy link
Contributor

@baileympearson baileympearson Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand how this fixes the bug you reported. We still instantiate an auto encrypter before adding the options to client.s - all you've done is explicitly default to {}.

Unlike sessions, none of the places you fixed in this PR are hot code paths (this usage included). I disagree that this is burning and that important. I'm trying to say that, if you believe this needs to be addressed, please file a ticket for it so we can fix it somehow and put guardrails around this so we can't make this mistake. Otherwise more usages of client.options will continue to pop up (for example - there are literally more occurrences of this in Aditi's PR).

edit: except cursors

Copy link
Contributor Author

@nbbeeken nbbeeken Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand how this fixes the bug you reported.

Because it doesn't! 😄 The ticket I filed is for us to fix the bug, I did change the code to be explicit as a reminder for anyone who drops by this line of code.

there are literally more occurrences of this in Aditi's PR

We should change the occurrences we can catch because of what we know to be true about the cost of using the incorrect API rather than giving up any attempt to improve what we are implementing.

I filed a ticket to fix this particular bug but at that time we can make sure we're not making the same mistake elsewhere and potentially lint or just correct the API altogether

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a ticket to either 1. find a tooling solution to prevent us from using client.options or 2. remove the freeze or 3. some other fix that ensures we don't make that mistake going forward?

}

this._mongocryptdClient = new MongoClient(this._mongocryptdManager.uri, clientOptions);
Expand Down Expand Up @@ -392,7 +395,7 @@ export class AutoEncrypter {
promoteLongs: false,
proxyOptions: this._proxyOptions,
tlsOptions: this._tlsOptions,
socketOptions: autoSelectSocketOptions(this._client.options)
socketOptions: autoSelectSocketOptions(this._client.s.options)
});

return deserialize(await stateMachine.execute(this, context, options.timeoutContext), {
Expand All @@ -413,7 +416,7 @@ export class AutoEncrypter {
...options,
proxyOptions: this._proxyOptions,
tlsOptions: this._tlsOptions,
socketOptions: autoSelectSocketOptions(this._client.options)
socketOptions: autoSelectSocketOptions(this._client.s.options)
});

return await stateMachine.execute(
Expand Down
45 changes: 32 additions & 13 deletions src/client-side-encryption/client_encryption.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ import { type MongoClient, type MongoClientOptions } from '../mongo_client';
import { type Filter, type WithId } from '../mongo_types';
import { type CreateCollectionOptions } from '../operations/create_collection';
import { type DeleteResult } from '../operations/delete';
import { MongoDBCollectionNamespace } from '../utils';
import { TimeoutContext } from '../timeout';
import { MongoDBCollectionNamespace, resolveTimeoutOptions } from '../utils';
import * as cryptoCallbacks from './crypto_callbacks';
import {
MongoCryptCreateDataKeyError,
Expand Down Expand Up @@ -74,6 +75,8 @@ export class ClientEncryption {
_tlsOptions: CSFLEKMSTlsOptions;
/** @internal */
_kmsProviders: KMSProviders;
/** @internal */
_timeoutMS?: number;

/** @internal */
_mongoCrypt: MongoCrypt;
Expand Down Expand Up @@ -120,6 +123,8 @@ export class ClientEncryption {
this._proxyOptions = options.proxyOptions ?? {};
this._tlsOptions = options.tlsOptions ?? {};
this._kmsProviders = options.kmsProviders || {};
const { timeoutMS } = resolveTimeoutOptions(client, options);
this._timeoutMS = timeoutMS;

if (options.keyVaultNamespace == null) {
throw new MongoCryptInvalidArgumentError('Missing required option `keyVaultNamespace`');
Expand Down Expand Up @@ -212,7 +217,7 @@ export class ClientEncryption {
const stateMachine = new StateMachine({
proxyOptions: this._proxyOptions,
tlsOptions: this._tlsOptions,
socketOptions: autoSelectSocketOptions(this._client.options)
socketOptions: autoSelectSocketOptions(this._client.s.options)
});

const dataKey = deserialize(await stateMachine.execute(this, context)) as DataKey;
Expand Down Expand Up @@ -270,10 +275,14 @@ export class ClientEncryption {
const stateMachine = new StateMachine({
proxyOptions: this._proxyOptions,
tlsOptions: this._tlsOptions,
socketOptions: autoSelectSocketOptions(this._client.options)
socketOptions: autoSelectSocketOptions(this._client.s.options)
});

const { v: dataKeys } = deserialize(await stateMachine.execute(this, context));
const timeoutContext = TimeoutContext.create(
resolveTimeoutOptions(this._client, { timeoutMS: this._timeoutMS })
);

const { v: dataKeys } = deserialize(await stateMachine.execute(this, context, timeoutContext));
if (dataKeys.length === 0) {
return {};
}
Expand Down Expand Up @@ -303,7 +312,8 @@ export class ClientEncryption {
.db(dbName)
.collection<DataKey>(collectionName)
.bulkWrite(replacements, {
writeConcern: { w: 'majority' }
writeConcern: { w: 'majority' },
timeoutMS: timeoutContext.csotEnabled() ? timeoutContext?.remainingTimeMS : undefined
});

return { bulkWriteResult: result };
Expand Down Expand Up @@ -332,7 +342,7 @@ export class ClientEncryption {
return await this._keyVaultClient
.db(dbName)
.collection<DataKey>(collectionName)
.deleteOne({ _id }, { writeConcern: { w: 'majority' } });
.deleteOne({ _id }, { writeConcern: { w: 'majority' }, timeoutMS: this._timeoutMS });
}

/**
Expand All @@ -355,7 +365,7 @@ export class ClientEncryption {
return this._keyVaultClient
.db(dbName)
.collection<DataKey>(collectionName)
.find({}, { readConcern: { level: 'majority' } });
.find({}, { readConcern: { level: 'majority' }, timeoutMS: this._timeoutMS });
}

/**
Expand All @@ -381,7 +391,7 @@ export class ClientEncryption {
return await this._keyVaultClient
.db(dbName)
.collection<DataKey>(collectionName)
.findOne({ _id }, { readConcern: { level: 'majority' } });
.findOne({ _id }, { readConcern: { level: 'majority' }, timeoutMS: this._timeoutMS });
}

/**
Expand All @@ -408,7 +418,10 @@ export class ClientEncryption {
return await this._keyVaultClient
.db(dbName)
.collection<DataKey>(collectionName)
.findOne({ keyAltNames: keyAltName }, { readConcern: { level: 'majority' } });
.findOne(
{ keyAltNames: keyAltName },
{ readConcern: { level: 'majority' }, timeoutMS: this._timeoutMS }
);
}

/**
Expand Down Expand Up @@ -442,7 +455,7 @@ export class ClientEncryption {
.findOneAndUpdate(
{ _id },
{ $addToSet: { keyAltNames: keyAltName } },
{ writeConcern: { w: 'majority' }, returnDocument: 'before' }
{ writeConcern: { w: 'majority' }, returnDocument: 'before', timeoutMS: this._timeoutMS }
);

return value;
Expand Down Expand Up @@ -503,7 +516,8 @@ export class ClientEncryption {
.collection<DataKey>(collectionName)
.findOneAndUpdate({ _id }, pipeline, {
writeConcern: { w: 'majority' },
returnDocument: 'before'
returnDocument: 'before',
timeoutMS: this._timeoutMS
});

return value;
Expand Down Expand Up @@ -650,7 +664,7 @@ export class ClientEncryption {
const stateMachine = new StateMachine({
proxyOptions: this._proxyOptions,
tlsOptions: this._tlsOptions,
socketOptions: autoSelectSocketOptions(this._client.options)
socketOptions: autoSelectSocketOptions(this._client.s.options)
});

const { v } = deserialize(await stateMachine.execute(this, context));
Expand Down Expand Up @@ -729,7 +743,7 @@ export class ClientEncryption {
const stateMachine = new StateMachine({
proxyOptions: this._proxyOptions,
tlsOptions: this._tlsOptions,
socketOptions: autoSelectSocketOptions(this._client.options)
socketOptions: autoSelectSocketOptions(this._client.s.options)
});
const context = this._mongoCrypt.makeExplicitEncryptionContext(valueBuffer, contextOptions);

Expand Down Expand Up @@ -818,6 +832,11 @@ export interface ClientEncryptionOptions {
* TLS options for kms providers to use.
*/
tlsOptions?: CSFLEKMSTlsOptions;

/**
* The timeout setting to be used for all the operations on ClientEncryption.
*/
timeoutMS?: number;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/cursor/abstract_cursor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,7 @@ export abstract class AbstractCursor<
if (this.cursorOptions.timeoutMS != null) {
this.timeoutContext ??= new CursorTimeoutContext(
TimeoutContext.create({
serverSelectionTimeoutMS: this.client.options.serverSelectionTimeoutMS,
serverSelectionTimeoutMS: this.client.s.options.serverSelectionTimeoutMS,
timeoutMS: this.cursorOptions.timeoutMS
}),
this
Expand Down Expand Up @@ -925,7 +925,7 @@ export abstract class AbstractCursor<
this.timeoutContext?.clear();
return new CursorTimeoutContext(
TimeoutContext.create({
serverSelectionTimeoutMS: this.client.options.serverSelectionTimeoutMS,
serverSelectionTimeoutMS: this.client.s.options.serverSelectionTimeoutMS,
timeoutMS
}),
this
Expand Down
4 changes: 2 additions & 2 deletions src/gridfs/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ export class GridFSBucket extends TypedEventEmitter<GridFSBucketEvents> {
if (timeoutMS) {
timeoutContext = new CSOTTimeoutContext({
timeoutMS,
serverSelectionTimeoutMS: this.s.db.client.options.serverSelectionTimeoutMS
serverSelectionTimeoutMS: this.s.db.client.s.options.serverSelectionTimeoutMS
});
}

Expand Down Expand Up @@ -241,7 +241,7 @@ export class GridFSBucket extends TypedEventEmitter<GridFSBucketEvents> {
if (timeoutMS) {
timeoutContext = new CSOTTimeoutContext({
timeoutMS,
serverSelectionTimeoutMS: this.s.db.client.options.serverSelectionTimeoutMS
serverSelectionTimeoutMS: this.s.db.client.s.options.serverSelectionTimeoutMS
});
}

Expand Down
5 changes: 3 additions & 2 deletions src/gridfs/upload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
MongoOperationTimeoutError
} from '../error';
import { CSOTTimeoutContext } from '../timeout';
import { type Callback, squashError } from '../utils';
import { type Callback, resolveTimeoutOptions, squashError } from '../utils';
import type { WriteConcernOptions } from '../write_concern';
import { WriteConcern } from './../write_concern';
import type { GridFSFile } from './download';
Expand Down Expand Up @@ -143,7 +143,8 @@ export class GridFSBucketWriteStream extends Writable {
if (options.timeoutMS != null)
this.timeoutContext = new CSOTTimeoutContext({
timeoutMS: options.timeoutMS,
serverSelectionTimeoutMS: this.bucket.s.db.client.options.serverSelectionTimeoutMS
serverSelectionTimeoutMS: resolveTimeoutOptions(this.bucket.s.db.client, {})
.serverSelectionTimeoutMS
});
}

Expand Down
4 changes: 2 additions & 2 deletions src/mongo_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> implements

/** @internal */
get timeoutMS(): number | undefined {
return this.options.timeoutMS;
return this.s.options.timeoutMS;
}

/**
Expand Down Expand Up @@ -706,7 +706,7 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> implements

// Default to db from connection string if not provided
if (!dbName) {
dbName = this.options.dbName;
dbName = this.s.options.dbName;
}

// Copy the options and add out internal override of the not shared flag
Expand Down
2 changes: 1 addition & 1 deletion src/operations/client_bulk_write/executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export class ClientBulkWriteExecutor {

// If no write concern was provided, we inherit one from the client.
if (!this.options.writeConcern) {
this.options.writeConcern = WriteConcern.fromOptions(this.client.options);
this.options.writeConcern = WriteConcern.fromOptions(this.client.s.options);
}

if (this.options.writeConcern?.w === 0) {
Expand Down
2 changes: 1 addition & 1 deletion src/operations/create_collection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ export class CreateCollectionOperation extends CommandOperation<Collection> {

const encryptedFields: Document | undefined =
options.encryptedFields ??
db.client.options.autoEncryption?.encryptedFieldsMap?.[`${db.databaseName}.${name}`];
db.client.s.options.autoEncryption?.encryptedFieldsMap?.[`${db.databaseName}.${name}`];

if (encryptedFields) {
// Creating a QE collection required min server of 7.0.0
Expand Down
2 changes: 1 addition & 1 deletion src/operations/drop.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class DropCollectionOperation extends CommandOperation<boolean> {
const options = this.options;
const name = this.name;

const encryptedFieldsMap = db.client.options.autoEncryption?.encryptedFieldsMap;
const encryptedFieldsMap = db.client.s.options.autoEncryption?.encryptedFieldsMap;
let encryptedFields: Document | undefined =
options.encryptedFields ?? encryptedFieldsMap?.[`${db.databaseName}.${name}`];

Expand Down
Loading