PHPLIB-892: Require crypt_shared and/or mongocryptd for tests utilizing enterprise CSFLE features #1072
Conversation
GromNaN
force-pushed
the
PHPLIB-892
branch
2 times, most recently
from
d50f912 to
2354b3f
Compare
GromNaN
changed the title
| @@ -1821,9 +1821,7 @@ public function testQueryableEncryption(): void | |||
| $this->markTestSkipped('Explicit encryption tests require MongoDB 7.0 or later'); | |||
| } | |||
|
|
|||
| if (! $this->isEnterprise()) { | |||
There was a problem hiding this comment.
In PHPLIB-892 you wrote:
I'll run the
DocumentationExamplesTest::testQueryableEncryption()withcrypt_sharedand/ormongocryptdavailable with the community server. If it passes, let's remove theisEnterprise()method and change the logic inDocumentationExamplesTest::testQueryableEncryption()to instead check if eithercrypt_sharedormongocryptdis available.
skipIfClientSideEncryptionIsNotSupported() doesn't check for either of those things. Did something change since you wrote that comment?
Also, in an earlier comment on that issue I pointed out that DocumentationExamplesTest::testQueryableEncryption() did require crypt_shared and/or mongocryptd (since it uses queryable encryption) but the prose test did not (since it explicitly encrypts). You could run the prose tests w/o crypt_shared and mongocryptd to verify that.
There was a problem hiding this comment.
@GromNaN: In light of mongodb/specifications#1439, I would be in favor of adding a check for crypt_shared and/or mongocryptd to the base skipIfClientSideEncryptionIsNotSupported() method.
A call to skipIfClientSideEncryptionIsNotSupported() could then be used here (after the topology and 7.0+ version check). I'm aware the version check in skipIfClientSideEncryptionIsNotSupported() would be redundant, but that's OK.
You could then also remove the explicit crypt_shared/mongocryptd check in ClientSideEncryptionSpecTest::setUp().
With that done, skipIfClientSideEncryptionIsNotSupported() would become consistent with UnifiedTestRunner::isClientSideEncryptionSupported() and all of our tests would perform the same checks.
There was a problem hiding this comment.
I'm aware the version check in skipIfClientSideEncryptionIsNotSupported() would be redundant, but that's OK.
There is a slight difference between the server version check and the feature compatibility version check in skipIfClientSideEncryptionIsNotSupported. So this is not redundant.
There was a problem hiding this comment.
I followed your instructions.
There was a problem hiding this comment.
Aye, technically different checks. I said "redundant" as our test suite is never run on mixed-version or upgraded replica sets, so FCV will always equate the server version.
|
The skipped tests effectively fail when both are disabled.
|
jmikola
changed the title
tests/SpecTests/ClientSideEncryption/Prose21_AutomaticDataEncryptionKeysTest.php
Outdated
Show resolved
Hide resolved
| @@ -357,30 +353,6 @@ private function isClientSideEncryptionSupported(): bool | |||
| return static::isCryptSharedLibAvailable() || static::isMongocryptdAvailable(); | |||
| } | |||
|
|
|||
| private static function isCryptSharedLibAvailable(): bool | |||
There was a problem hiding this comment.
@eramongodb, @kevinAlbs: The csfle runOnRequirement dates back to mongodb/specifications@0f65908 and DRIVERS-2017. Reading the existing language in the unified spec, there's no indication that crypt_shared and/or mongocryptd are required.
If true, the tests MUST only run if the driver and server support Client-Side Field Level Encryption. A server supports CSFLE if it is version 4.2.0 or higher. If false, tests MUST only run if CSFLE is not enabled. If this field is omitted, there is no CSFLE requirement.
The PHPLIB unified test runner currently checks three things:
- Server FCV >= 4.2
- PHPC is compiled with libmongocrypt (not just libmongoc)
- At least one of
crypt_sharedandmongocryptdis available
I realize that the only unified CSFLE tests at the moment are for key management, which isn't an enterprise feature. But ignoring that for a moment and assuming legacy tests will eventually be ported to the unified format, was it your intention that the csfle requirement also ensure that queryable encryption be supported?
If yes, then I'd like to clarify this in the unified test spec. If no, then I think we should remove the crypt_shared/mongocryptd check from PHPLIB's unified test runner (and maybe still clarify that in the unified spec).
There was a problem hiding this comment.
At least one of
crypt_sharedandmongocryptdis available
I expect crypt_shared or mongocryptd would be required once the unified test format supports automatic encryption. I expect it is not necessary for the current unified test format. I suggest keeping the check for crypt_shared or mongocryptd, as I expect it will be needed in the future.
was it your intention that the
csflerequirement also ensure that queryable encryption be supported?
I think yes. Both "Client-Side Field Level Encryption" and "Queryable Encryption" require libmongocrypt. As of DRIVERS-2454, "In-Use Encryption" can be used to refer to both features.
There was a problem hiding this comment.
Thanks. I opened mongodb/specifications#1439 and DRIVERS-2661 to update the unified/legacy test docs.
There was a problem hiding this comment.
The csfle requirement was added as an (overly?) simple all-or-nothing check for whether anything related to Client Side Encryption is required by the test, which implicitly requires - or at least, was assumed to require - either mongocryptd (the only consideration back then) or crypt_shared (a new option since the wording was added), since most CSE functions cannot fulfill their specified behavior without them, including the Key Management API. I support the clarification of wording that documents the mongocryptd/crypt_shared requirement; submitted an approving review for mongodb/specifications#1439 accordingly.
Remove isEnterprise helper
…se FunctionalTestCase
…pIfClientSideEncryptionIsNotSupported
| } | ||
|
|
||
| return false; | ||
| return FunctionalTestCase::isCryptSharedLibAvailable() || FunctionalTestCase::isMongocryptdAvailable(); |
There was a problem hiding this comment.
This is one thing I had in mind when creating PHPLIB-1170.
GromNaN
changed the title
Fix PHPLIB-892.
According to the doc, automatic CSFLE is only supported by Enterprise (and Atlas) servers. But this is a client-side feature, the tests are skipped when Client-Side encryption is not supported.