Implement prose tests checking that neither `mongocryptd` nor its client are created when they are not supposed to be #1113

stIncMale · 2023-05-04T18:46:47Z

Note that while JAVA-4794 adds one test, the other tests in the same section 8. Bypass Spawning mongocryptd appear to be missing in our codebase, so I also implemented them.

The "Commits" WUI view does something weird to the commit messages. The actual commit messages are something like the following, and point to the ID of the group in the spec:

Add tests #8 JAVA-4794
Add test #20 JAVA-4741

There are small deviations in some tests comparing to their prose description, made to make our life better in a situation when the tests fail. I created DRIVERS-2624 with a PR suggesting to include the changes in the spec.

JAVA-4741
JAVA-4794

stIncMale · 2023-05-04T18:57:28Z

.evergreen/run-csfle-tests-with-mongocryptd.sh

+          --tests "*.ClientEncryption*" --tests "*.ClientSideEncryption*" \
      driver-sync:test \
-          --tests com.mongodb.client.ClientSideEncryptionTest \
-          --tests com.mongodb.client.unified.ClientSideEncryptionTest \
+          --tests "*.ClientEncryption*" --tests "*.ClientSideEncryption*" \
      driver-reactive-streams:test \
-          --tests com.mongodb.reactivestreams.client.ClientSideEncryptionTest \
-          --tests com.mongodb.reactivestreams.client.unified.ClientSideEncryptionTest \
+          --tests "*.ClientEncryption*" --tests "*.ClientSideEncryption*" \
      driver-scala:integrationTest \
-          --tests org.mongodb.scala.ClientSideEncryptionTest
+          --tests "*.ClientEncryption*" --tests "*.ClientSideEncryption*"


This change together with the assumeFalse/assumeTrue(CRYPT_SHARED_LIB_PATH_SYS_PROP_VALUE != null) checks in AbstractClientSideEncryptionNotSpawnMongocryptdTest solves the issue discussed in https://mongodb.slack.com/archives/CFPHLTRMK/p1683160528003019: some of the tests added in this PR not only do not require crypt_shared, but require the library not to be loaded, which is why they are run separately (in different process) from those that load crypt_shared (once it is loaded, it can't be unloaded).

Glad this worked out so easily.

driver-core/src/main/com/mongodb/internal/NoCheckedAutoCloseable.java

stIncMale · 2023-05-04T19:01:14Z

driver-core/src/test/resources/client-side-encryption/README.rst

@@ -163,8 +163,6 @@ Do the following before running spec tests:



I replaced this file with the one from mongodb/specifications@33a03f2 (the latest of the spec commits in JAVA-4741 and JAVA-4794).

stIncMale · 2023-05-04T19:03:36Z

driver-sync/src/test/functional/com/mongodb/client/AbstractClientSideEncryptionTest.java

-        String cryptSharedLibPath = System.getProperty("org.mongodb.test.crypt.shared.lib.path", "");
-        if (!cryptSharedLibPath.isEmpty()) {
-            extraOptions.put("cryptSharedLibPath", cryptSharedLibPath);
-        }


I also needed to use this Java system property, so I refactored this code a bit.

stIncMale · 2023-05-04T19:04:07Z

...ync/src/test/functional/com/mongodb/client/AbstractClientSideEncryptionAutoDataKeysTest.java

-                kmsProviderProperties -> kmsProviderProperties.put("key", Base64.getDecoder().decode(
-                        "Mng0NCt4ZHVUYUJCa1kxNkVyNUR1QURhZ2h2UzR2d2RrZzh0cFBwM3R6NmdWMDFBMUN3YkQ5aXRRMkhGRGdQV09wOGVNYUMxT2k3NjZKelhaQmRCZ"
-                                + "GJkTXVyZG9uSjFk")),


I also needed to use this key, so I refactored this code a bit. See UnifiedClientEncryptionHelper.

stIncMale · 2023-05-04T19:21:45Z

...unctional/com/mongodb/client/AbstractClientSideEncryptionNotCreateMongocryptdClientTest.java

+                        .keyVaultNamespace(KEY_VAULT_NAMESPACE.getFullName())
+                        .extraOptions(Stream.of(
+                                new SimpleImmutableEntry<>("cryptSharedLibPath", CRYPT_SHARED_LIB_PATH_SYS_PROP_VALUE),
+                                new SimpleImmutableEntry<>("mongocryptdURI", format("mongodb://%s:%d/db?serverSelectionTimeoutMS=%d",


The prose description does not specify serverSelectionTimeoutMS, however it speeds up the test in a situation when the test fails.

...unctional/com/mongodb/client/AbstractClientSideEncryptionNotCreateMongocryptdClientTest.java

stIncMale · 2023-05-04T19:33:34Z

...unctional/com/mongodb/client/AbstractClientSideEncryptionNotCreateMongocryptdClientTest.java

+    static int findAvailableMongocryptdLoopbackPort() {
+        try (ServerSocket serverSocket = new ServerSocket()) {
+            serverSocket.bind(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0));
+            int foundPort = serverSocket.getLocalPort();
+            if (foundPort != DEFAULT_MONGOCRYPTD_PORT) {
+                return foundPort;
+            } else {
+                return findAvailableMongocryptdLoopbackPort();
+            }
+        } catch (IOException e) {
+            throw new RuntimeException("Failed to find an available port", e);
+        }
+    }


We could have just used serverSocket.bind(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0)) right away in ConnectionTracker.start instead of introducing the findAvailableMongocryptdLoopbackPort method. However, the same approach is not helpful for the tests in AbstractClientSideEncryptionNotSpawnMongocryptdTest. Additionally, making sure that we never use the default mongocryptd port in AbstractClientSideEncryptionNotSpawnMongocryptdTest also improve the isolation between different tests.

To avoid using different approaches, I simply use findAvailableMongocryptdLoopbackPort in both AbstractClientSideEncryptionNotCreateMongocryptdClientTest and AbstractClientSideEncryptionNotSpawnMongocryptdTest.

stIncMale · 2023-05-04T19:43:40Z

.../test/functional/com/mongodb/client/AbstractClientSideEncryptionNotSpawnMongocryptdTest.java

+    private void setUpKeyVaultNamespace() {
+        try (MongoClient client = createMongoClient(MongoClientSettings.builder().build())) {
+            MongoDatabase db = client.getDatabase(KEY_VAULT_NAMESPACE.getDatabaseName());
+            db.drop();
+            db.getCollection(KEY_VAULT_NAMESPACE.getCollectionName(), BsonDocument.class)
+                    .withWriteConcern(WriteConcern.MAJORITY)
+                    .insertOne(EXTERNAL_KEY);
+        }
+    }


The prose tests do not require having any keys in the KEY_VAULT_NAMESPACE: only viaMongocryptdBypassSpawn attempts to insert an encrypted field, and that insertion is supposed to fail. However, when the driver misbehaves or tests fail (in my case, viaMongocryptdBypassSpawn was failing if viaLoadingSharedLibrary runs before it in the same process due to viaLoadingSharedLibrary loading crypt_shared), things may get too difficult to understand if the encryption is not properly set up.

stIncMale · 2023-05-04T19:45:04Z

.../test/functional/com/mongodb/client/AbstractClientSideEncryptionNotSpawnMongocryptdTest.java

+                // We pick a random available `mongocryptd` port and also include it in the PID file path
+                // to reduce the chances of different test runs interfering with each other. The interference
+                // may come from the fact that once spawned, `mongocryptd` stays up and running for some time,
+                // which may cause failures in other runs if they use the same `mongocryptd` port / PID file.
+                format("--pidfilepath=bypass-spawning-mongocryptd-%d.pid", mongocryptdSocketAddress.getPort()),
+                format("--port=%d", mongocryptdSocketAddress.getPort())));


The comment in the code explains why this is done the way it is, even though the prose tests do not require anything like that.

JAVA-4741

JAVA-4794

… `ConnectionTracker` instead

stIncMale · 2023-05-12T18:05:45Z

@jyemin Are you OK with me merging this tests-only PR based on the approval of one reviewer, or do you want to review it?

jyemin · 2023-05-12T22:50:13Z

I'd like to take a look so please hold off on merging

mongodb/specifications@9e122d9 and mongodb/specifications#1415 (comment)

jyemin · 2023-05-05T21:22:53Z

.evergreen/run-csfle-tests-with-mongocryptd.sh

+          --tests "*.ClientEncryption*" --tests "*.ClientSideEncryption*" \
      driver-sync:test \
-          --tests com.mongodb.client.ClientSideEncryptionTest \
-          --tests com.mongodb.client.unified.ClientSideEncryptionTest \
+          --tests "*.ClientEncryption*" --tests "*.ClientSideEncryption*" \
      driver-reactive-streams:test \
-          --tests com.mongodb.reactivestreams.client.ClientSideEncryptionTest \
-          --tests com.mongodb.reactivestreams.client.unified.ClientSideEncryptionTest \
+          --tests "*.ClientEncryption*" --tests "*.ClientSideEncryption*" \
      driver-scala:integrationTest \
-          --tests org.mongodb.scala.ClientSideEncryptionTest
+          --tests "*.ClientEncryption*" --tests "*.ClientSideEncryption*"


Glad this worked out so easily.

.evergreen/run-csfle-tests-with-mongocryptd.sh

driver-core/src/main/com/mongodb/internal/NoCheckedAutoCloseable.java

...unctional/com/mongodb/client/AbstractClientSideEncryptionNotCreateMongocryptdClientTest.java

https://docs.gradle.org/current/userguide/java_testing.html#test_filtering

jyemin

LGTM

stIncMale commented May 4, 2023

View reviewed changes

driver-core/src/main/com/mongodb/internal/NoCheckedAutoCloseable.java Outdated Show resolved Hide resolved

stIncMale commented May 4, 2023

View reviewed changes

...unctional/com/mongodb/client/AbstractClientSideEncryptionNotCreateMongocryptdClientTest.java Outdated Show resolved Hide resolved

stIncMale commented May 4, 2023

View reviewed changes

Add test mongodb#20

f77fbc8

JAVA-4741

stIncMale force-pushed the JAVA-4741 branch from 89c3652 to 43ecf39 Compare May 4, 2023 19:49

Add tests mongodb#8

7925496

JAVA-4794

stIncMale force-pushed the JAVA-4741 branch from 43ecf39 to 7925496 Compare May 4, 2023 22:59

stIncMale requested a review from vbabanin May 5, 2023 00:24

stIncMale self-assigned this May 5, 2023

jyemin self-requested a review May 5, 2023 13:26

stIncMale added 2 commits May 5, 2023 17:58

Remove NoCheckedAutoCloseable and use @SuppressWarnings("try") on…

2e1a236

… `ConnectionTracker` instead

Add the requested comment

0a70bc6

vbabanin approved these changes May 11, 2023

View reviewed changes

vbabanin approved these changes May 12, 2023

View reviewed changes

Implement improvements from

6241f65

mongodb/specifications@9e122d9 and mongodb/specifications#1415 (comment)

jyemin reviewed May 18, 2023

View reviewed changes

Update test patterns in run-csfle-tests-with-mongocryptd.sh

5008efc

https://docs.gradle.org/current/userguide/java_testing.html#test_filtering

stIncMale requested a review from jyemin May 18, 2023 21:18

jyemin approved these changes May 18, 2023

View reviewed changes

stIncMale merged commit 64c030f into mongodb:master May 19, 2023
42 checks passed

stIncMale deleted the JAVA-4741 branch May 19, 2023 00:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement prose tests checking that neither `mongocryptd` nor its client are created when they are not supposed to be #1113

Implement prose tests checking that neither `mongocryptd` nor its client are created when they are not supposed to be #1113

stIncMale commented May 4, 2023 •

edited

stIncMale May 4, 2023

jyemin May 5, 2023

stIncMale May 4, 2023

stIncMale May 4, 2023

stIncMale May 4, 2023 •

edited

stIncMale May 4, 2023

stIncMale May 4, 2023 •

edited

stIncMale May 4, 2023

stIncMale May 4, 2023

stIncMale commented May 12, 2023

jyemin commented May 12, 2023

jyemin May 5, 2023

jyemin left a comment

		@@ -163,8 +163,6 @@ Do the following before running spec tests:

Implement prose tests checking that neither mongocryptd nor its client are created when they are not supposed to be #1113

Implement prose tests checking that neither mongocryptd nor its client are created when they are not supposed to be #1113

Conversation

stIncMale commented May 4, 2023 • edited

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

stIncMale May 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stIncMale May 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stIncMale commented May 12, 2023

jyemin commented May 12, 2023

Choose a reason for hiding this comment

jyemin left a comment

Choose a reason for hiding this comment

Implement prose tests checking that neither `mongocryptd` nor its client are created when they are not supposed to be #1113

Implement prose tests checking that neither `mongocryptd` nor its client are created when they are not supposed to be #1113

stIncMale commented May 4, 2023 •

edited

stIncMale May 4, 2023 •

edited

stIncMale May 4, 2023 •

edited