feat(NODE-6390): Add timeoutMS support to auto encryption #4265

aditi-khare-mongoDB · 2024-10-02T21:30:00Z

Description

Auto encryption encrypt and decrypt helpers now respect timeoutMS.

What is changing?

Added timeoutContext to stateMachine.execute call in AutoEncrypter.encrypt/decrypt
Integration tests for StateMachine helpers respecting timeoutMS
Integration tests for CryptoConnection.command respecting timeoutMS
Spec unit test for Auto Encryption

Is there new documentation needed for these changes?

No

What is the motivation for this change?

CSOT

Release Highlight

Double check the following

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: type(NODE-xxxx)[!]: description
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

…ver selection

#4157)

Co-authored-by: Warren James <[email protected]>

…4186)

…#4237)

…ver selection

#4157)

Co-authored-by: Warren James <[email protected]>

…4186)

…#4237)

lint fix prose test 2

…4243) Co-authored-by: Warren James <[email protected]> Co-authored-by: Neal Beeken <[email protected]> Co-authored-by: Bailey Pearson <[email protected]>

…ion (#4273)

Co-authored-by: Warren James <[email protected]>

aditi-khare-mongoDB · 2024-10-15T14:29:38Z

test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts

-        });
+        childProcess = spawn(
+          'mongocryptd',
+          ['--port', mongocryptdTestPort, '--ipv6', '--pidfilepath', new ObjectId().toHexString()],


Adding a random pidfilepath ensure it doesn't use the default mongod pid path which can cause it to randomly fail

baileympearson

Because evergreen is frozen today, I can't test the pidfile changes. But I think they should work.

baileympearson · 2024-10-15T20:12:20Z

src/client-side-encryption/auto_encrypter.ts

+      await stateMachine.execute(
+        this,
+        context,
+        options.timeoutContext?.csotEnabled() ? options.timeoutContext : undefined


Suggested change

options.timeoutContext?.csotEnabled() ? options.timeoutContext : undefined

options.timeoutContext

It doesn't matter if the context is a csot context or a legacy context, we should be able to pass it in regardless.

Sounds good!

baileympearson · 2024-10-16T16:40:45Z

test/integration/client-side-encryption/client_side_encryption.test.ts

@@ -0,0 +1,251 @@
+import { expect } from 'chai';
+import * as sinon from 'sinon';
+


It is poorly named, but driver.test.ts contains Node-driver specific tests for FLE. Can we move the tests from this file into driver.test.ts?

Moved them!

baileympearson · 2024-10-16T16:43:45Z

test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts

-        });
+        childProcess = spawn(
+          'mongocryptd',
+          ['--port', mongocryptdTestPort, '--ipv6', '--pidfilepath', new ObjectId().toHexString()],


Could we create the file in the temporary directory instead of the current directory?

const pidFile = path.join( os.tmpDir(), new ObjectId().toHexString() )

Also - how would you feel about making this change to the two other places our tests spawn mongocryptd? (search for spawn('mongo and three results should appear).

Yep, made the change to the other instances too; I think it's a good call.

baileympearson · 2024-10-16T16:45:34Z

test/integration/client-side-encryption/client_side_encryption.test.ts

+      });
+    });
+
+    describe('State machine', function () {


Do we have coverage for fetchCollectionInfo as well?

Just added it in, good catch!

baileympearson · 2024-10-16T16:46:09Z

test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts

        client = new MongoClient(`mongodb://localhost:${mongocryptdTestPort}/?timeoutMS=1000`, {
-          monitorCommands: true
+          family: 6,
+          monitorCommands: true,
+          serverSelectionTimeoutMS: 2000


This was nice-to-have for debugging locally, but doesn't need to be in the test permanently.

Suggested change

client = new MongoClient(`mongodb://localhost:${mongocryptdTestPort}/?timeoutMS=1000`, {

monitorCommands: true

family: 6,

monitorCommands: true,

serverSelectionTimeoutMS: 2000

client = new MongoClient(`mongodb://localhost:${mongocryptdTestPort}/?timeoutMS=1000`, {

monitorCommands: true,

Noted, just removed them!

baileympearson · 2024-10-16T16:49:39Z

test/integration/client-side-encryption/client_side_encryption.test.ts

+        });
+
+        it(
+          'the command should fail due to a timeout error',


I think this test needs to fail something inside encryption. Otherwise, we don't have any coverage that asserts that we correctly pass the timeout context from CryptoConnection.command() -> AutoEncrypter.encrypt()/decrypt() -> StateMachine.execute() - this test currently ensures that we pass the timeout context from CryptoConnection.command() -> Connection.command().

I think this could pretty easily be achieved by configuring an auto encrypted client to use the same cluster as the keyvault (we can provided a keyvault client to the MongoClient when configuring auto encryption), and setting a failpoint on a find (used for list keys).

Sounds good, made the change - added in data key creation

baileympearson · 2024-10-16T17:04:20Z

test/integration/client-side-encryption/client_side_encryption.test.ts

+            {
+              autoEncryption: {
+                keyVaultNamespace: 'admin.datakeys',
+                kmsProviders: {


We usually obtain kmsProviders from the CSFLE_KMS_PROVIDERS environment variable (you can search for other usages of this in tests. I think range explicit encryption prose tests use this for a local kms provider, which is what you'd want). This requires FLE secrets to run locally - I can help you set this up if you'd like.

Maybe we can set up FLE secrets locally another day - I'm fine just testing on EVG for this work. Switched it over to use CSFLE_KMS_PROVIDERS + require FLE variables to run.

baileympearson · 2024-10-16T17:04:56Z

test/integration/client-side-encryption/client_side_encryption.test.ts

+      });
+
+      context('when client is provided timeoutMS and command hangs', function () {
+        let encryptedClient;


(just a suggestion)

Suggested change

let encryptedClient;

let encryptedClient: MongoClient;

Then you get red underlines and type hinting when writing tests using the client.

Made the change in the other parts of the file too!

baileympearson · 2024-10-16T17:06:09Z

test/integration/client-side-encryption/client_side_encryption.test.ts

+          const err = await encryptedClient
+            .db('test')
+            .collection('test')
+            .aggregate([])
+            .toArray()
+            .catch(e => e);
+          expect(err).to.deep.equal([]);


Usually, if we expect something not to fail, we don't catch the error because mocha considers a test to fail if an error is thrown:

Suggested change

const err = await encryptedClient

.db('test')

.collection('test')

.aggregate([])

.toArray()

.catch(e => e);

expect(err).to.deep.equal([]);

await encryptedClient

.db('test')

.collection('test')

.aggregate([])

.toArray()

(this applies to other "should not fail" tests in this file too)

Sounds good!

W-A-James and others added 30 commits September 12, 2024 13:43

feat(NODE-6090): Implement CSOT logic for connection checkout and ser…

3c2ec0a

…ver selection

test(NODE-6120): Implement Unified test runner changes for CSOT (#4121)

909578f

refactor(NODE-6187): refactor to use TimeoutContext abstraction (#4131)

e101750

refactor(NODE-6230): executeOperation to use iterative retry mechanism (

e4efd3f

#4157)

feat(NODE-5682): set maxTimeMS on commands and preempt I/O (#4174)

22082c9

Co-authored-by: Warren James <[email protected]>

feat(NODE-6231): Add CSOT behaviour for retryable reads and writes (#…

bf95fa4

…4186)

feat(NODE-6312): add error transformation for server timeouts (#4192)

c63d102

feat(NODE-6313): add CSOT support to sessions and transactions (#4199)

1eab23d

feat(NODE-6304): add CSOT support for non-tailable cursors (#4195)

4c4b0a9

fix(NODE-6374): MongoOperationTimeoutError inherits MongoRuntimeError (…

558d416

…#4237)

test: remove empty skipped context blocks (#4238)

3ed4a14

feat(NODE-5844): add iscryptd to ServerDescription (#4239)

d3438ea

temp

ff561e3

temp

164780c

chore: plumb timeoutMS around more

12a7e2e

feat(NODE-6090): Implement CSOT logic for connection checkout and ser…

999f23d

…ver selection

test(NODE-6120): Implement Unified test runner changes for CSOT (#4121)

0355404

refactor(NODE-6187): refactor to use TimeoutContext abstraction (#4131)

5ef3d69

refactor(NODE-6230): executeOperation to use iterative retry mechanism (

7139b8f

#4157)

feat(NODE-5682): set maxTimeMS on commands and preempt I/O (#4174)

acfb4fc

Co-authored-by: Warren James <[email protected]>

feat(NODE-6231): Add CSOT behaviour for retryable reads and writes (#…

4efff95

…4186)

feat(NODE-6312): add error transformation for server timeouts (#4192)

1997f81

feat(NODE-6313): add CSOT support to sessions and transactions (#4199)

cc3ef8f

feat(NODE-6304): add CSOT support for non-tailable cursors (#4195)

38affae

fix(NODE-6374): MongoOperationTimeoutError inherits MongoRuntimeError (…

738188b

…#4237)

test: remove empty skipped context blocks (#4238)

c4a7c2c

feat(NODE-5844): add iscryptd to ServerDescription (#4239)

5aa6d4c

chore: allow clientBulkWrite to use TimeoutContext (#4251)

17a2fde

Merge branch 'NODE-6090' into NODE-6389

aead2f1

half testing

88ca990

lint fix prose test 2

feat(NODE-6389): add support for timeoutMS in StateMachine.execute() (#…

a645d9f

…4243) Co-authored-by: Warren James <[email protected]> Co-authored-by: Neal Beeken <[email protected]> Co-authored-by: Bailey Pearson <[email protected]>

baileympearson force-pushed the NODE-6090 branch from 3591368 to a645d9f Compare October 10, 2024 20:40

nbbeeken and others added 2 commits October 11, 2024 16:44

fix(NODE-6412): read stale response from previously timed out connect…

c0d6ec9

…ion (#4273)

feat(NODE-6403): add CSOT support to client bulk write (#4261)

7df1a70

Co-authored-by: Warren James <[email protected]>

aditi-khare-mongoDB force-pushed the NODE-6390/auto-encryption-csot branch from cd8aaa4 to 1d9034b Compare October 14, 2024 18:00

Merge branch 'NODE-6090' into NODE-6390/auto-encryption-csot

0bdb03f

nbbeeken force-pushed the NODE-6090 branch from 7df1a70 to 450b163 Compare October 14, 2024 18:15

aditi-khare-mongoDB and others added 9 commits October 14, 2024 14:17

Merge branch 'NODE-6090' into NODE-6390/auto-encryption-csot

ae2165d

add back newline

dd8f4b2

remove extra newline

d773ae3

more newline from rebase

d93b61a

ci please work...

05bee51

delete extraneous folder

e58772b

remove test with unclosed socket

0296525

remove test with unclosed socket

6af0aa4

remove misc change

0031477

aditi-khare-mongoDB commented Oct 15, 2024

View reviewed changes

aditi-khare-mongoDB marked this pull request as ready for review October 15, 2024 14:47

baileympearson self-assigned this Oct 15, 2024

baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Oct 15, 2024

aditi-khare-mongoDB added 5 commits October 15, 2024 13:59

fix metadata logic

f565e9f

fix skippes test rquirement

15c3d72

lint fix

92cc6dd

final test..

08056e8

temp

fa66268

baileympearson requested changes Oct 16, 2024

View reviewed changes

aditi-khare-mongoDB added 4 commits October 16, 2024 17:35

requested changes

f2572c9

lint

0dab0ca

requested changes

2a6a2d1

drop collection

9530160

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-6390): Add timeoutMS support to auto encryption #4265

feat(NODE-6390): Add timeoutMS support to auto encryption #4265

aditi-khare-mongoDB commented Oct 2, 2024 •

edited

Loading

aditi-khare-mongoDB Oct 15, 2024

baileympearson left a comment

baileympearson Oct 15, 2024

aditi-khare-mongoDB Oct 16, 2024

baileympearson Oct 16, 2024

aditi-khare-mongoDB Oct 16, 2024

baileympearson Oct 16, 2024

aditi-khare-mongoDB Oct 16, 2024

baileympearson Oct 16, 2024

aditi-khare-mongoDB Oct 16, 2024

baileympearson Oct 16, 2024

aditi-khare-mongoDB Oct 16, 2024

baileympearson Oct 16, 2024

aditi-khare-mongoDB Oct 17, 2024

baileympearson Oct 16, 2024

aditi-khare-mongoDB Oct 16, 2024

baileympearson Oct 16, 2024

aditi-khare-mongoDB Oct 16, 2024

baileympearson Oct 16, 2024

baileympearson Oct 16, 2024

aditi-khare-mongoDB Oct 16, 2024

	options.timeoutContext?.csotEnabled() ? options.timeoutContext : undefined
	options.timeoutContext

		@@ -0,0 +1,251 @@
		import { expect } from 'chai';
		import * as sinon from 'sinon';

feat(NODE-6390): Add timeoutMS support to auto encryption #4265

Are you sure you want to change the base?

feat(NODE-6390): Add timeoutMS support to auto encryption #4265

Conversation

aditi-khare-mongoDB commented Oct 2, 2024 • edited Loading

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Double check the following

Choose a reason for hiding this comment

baileympearson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-khare-mongoDB commented Oct 2, 2024 •

edited

Loading