Implement prose tests checking that neither mongocryptd nor its client are created when they are not supposed to be
#1113
Conversation
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Glad this worked out so easily.
driver-core/src/main/com/mongodb/internal/NoCheckedAutoCloseable.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I replaced this file with the one from mongodb/specifications@33a03f2 (the latest of the spec commits in JAVA-4741 and JAVA-4794).
There was a problem hiding this comment.
I also needed to use this Java system property, so I refactored this code a bit.
There was a problem hiding this comment.
I also needed to use this key, so I refactored this code a bit. See UnifiedClientEncryptionHelper.
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
… `ConnectionTracker` instead
|
@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? |
|
I'd like to take a look so please hold off on merging |
There was a problem hiding this comment.
Glad this worked out so easily.
driver-core/src/main/com/mongodb/internal/NoCheckedAutoCloseable.java
Outdated
Show resolved
Hide resolved
...unctional/com/mongodb/client/AbstractClientSideEncryptionNotCreateMongocryptdClientTest.java
Show resolved
Hide resolved
3 participants
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:
#8JAVA-4794#20JAVA-4741There 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