Skip to content
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.

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

MONGOCRYPT-768 refactor to support multiple schemas #953

Merged
merged 37 commits into from
Feb 14, 2025

Conversation

kevinAlbs
Copy link
Contributor

@kevinAlbs kevinAlbs commented Feb 12, 2025

Summary

Add mc_schema_broker_t to manage schemas for an auto encryption operation.

Tested with this libmongocrypt: https://spruce.mongodb.com/version/67abfe5152bbba0007c15d63
Tested with the C driver pointing to this branch: https://spruce.mongodb.com/version/67ac860a932f4c0007855354

Background & Motivation

No meaningful changes to behavior are expected. This refactor is intended to help support $lookup in MONGOCRYPT-741. Supporting $lookup requires multiple schemas in one operation. mc_schema_broker_t supports multiple schemas to the same database.

The term schema refers to either a JSON Schema (for CSFLE) or an encryptedFields document (for QE).

SPM-2472 adds a new field to mongocryptd/crypt_shared: csfleEncryptionSchemas. This is to support passing multiple CSFLE schemas. encryptionInformation already supports passing multiple QE schemas. The syntax for csfleEncryptionSchemas is documented in Technical Design: Support $lookup in CSFLE and QE.

Test runner improvements

Additional test runner improvements were made to help test mc_schema_broker_t.

Wildcard test selection

Support trailing wildcard in test selection to ease running related tests:

# Run all tests with names starting with `test_mc_schema_broker`:
./cmake-build/test-mongocrypt "test_mc_schema_broker*"

TMP_BSONF

Several tests use the following pattern to declare BSON inline:

TMP_BSON(BSON_STR({"foo" : "bar"}));

Using BSON_STR has the advantage of improved formatting and syntax highlighting. However, a %s is formatted to % s:

TMP_BSON(BSON_STR({"foo" : % s}), "bar"); // Does not work!

This PR adds TMP_BSONF which supports MC_BSON and MC_STR formatting tokens to insert a bson_t* or C string:

bson_t *expect = TMP_BSONF(BSON_STR({
                                "find" : "coll",
                                "csfleEncryptionSchemas" : {
                                    "db.coll" : {"schema" : MC_BSON, "isRemoteSchema" : false},
                                    "db.coll2" : {"schema" : MC_BSON, "isRemoteSchema" : false}
                                }
                            }),
                            jsonSchema,
                            jsonSchema2);

TEST_FILE_AS_BSON

A new TEST_FILE_AS_BSON is added to load a file as a bson_t* rather than a mongocrypt_binary_t*. This was convenient for testing mc_schema_broker_t which accepts bson_t* in the interface.

To manage schemas for an auto encryption operation. Supports more than one schema.
The schema broker requires the name to identify the collection in the result.
The server replies with this field, but some mock data did not include it.
Catches a regression found when adding the schema broker
@kevinAlbs kevinAlbs changed the title Try2.rebased.schemabroker.m723 MONGOCRYPT-768 refactor to support multiple schemas in auto encryption Feb 12, 2025
@kevinAlbs kevinAlbs changed the title MONGOCRYPT-768 refactor to support multiple schemas in auto encryption MONGOCRYPT-768 refactor to support multiple schemas Feb 12, 2025
@kevinAlbs kevinAlbs force-pushed the try2.rebased.schemabroker.M723 branch from 07505cc to bf688f7 Compare February 12, 2025 13:46
@kevinAlbs kevinAlbs marked this pull request as ready for review February 12, 2025 13:55
test/test-mongocrypt.c Outdated Show resolved Hide resolved
test/test-mongocrypt.c Outdated Show resolved Hide resolved
test/test-mongocrypt.c Outdated Show resolved Hide resolved
test/test-mongocrypt.c Outdated Show resolved Hide resolved
test/test-mongocrypt.c Outdated Show resolved Hide resolved
test/test-mongocrypt.c Outdated Show resolved Hide resolved
test/test-mongocrypt.c Outdated Show resolved Hide resolved
test/test-mongocrypt.c Outdated Show resolved Hide resolved
@eramongodb eramongodb self-requested a review February 12, 2025 22:47
Copy link
Contributor Author

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Further testing motivated additional changes that seemed appropriate for this PR:

  • Missing use of mc_schema_broker_append_listCollections_filter: c8b6a28
  • Incorrect assumption that target collection has encryptedFields: bdcb389
  • Fix naming of field in csfleEncryptionSchemas (schema => jsonSchema): e683b33

test/test-mongocrypt.c Outdated Show resolved Hide resolved
src/mc-schema-broker-private.h Outdated Show resolved Hide resolved
src/mc-schema-broker-private.h Outdated Show resolved Hide resolved
src/mongocrypt-ctx-encrypt.c Outdated Show resolved Hide resolved
src/mc-schema-broker.c Show resolved Hide resolved
src/mc-schema-broker.c Outdated Show resolved Hide resolved
src/mc-schema-broker-private.h Outdated Show resolved Hide resolved
src/mc-schema-broker.c Outdated Show resolved Hide resolved
kevinAlbs and others added 9 commits February 13, 2025 12:57
prefer "JSON Schema" to refer to the concept, and "jsonSchema" to refer to the field.

Co-authored-by: Ezra Chung <[email protected]>
Related to MONGOCRYPT-774. The shared library was previously referred to as "csfle" but has since been renamed to "crypt_shared".
Handle the special case first, then the default.
Also test explain with crypt_shared.
Bypassing the create command does not need a schema. `_finalize` checks if any schemas have QE.
`_fle2_collect_keys_for_compaction` is called twice. On the first call, if there is no associated schema, do nothing.
@kevinAlbs kevinAlbs requested a review from eramongodb February 13, 2025 19:44
Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

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

A few suggestions remaining; otherwise, LGTM.

// "explain": { "find": "to-mongocryptd" },
// "encryptionInformation": {}
// }
BSON_ASSERT(bson_iter_init_find(&iter, cmd, "explain"));
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be demoted to a runtime error rather than an assertion (otherwise the "expected 'explain' to be a document" error below could also be asserted).

src/mc-schema-broker.c Outdated Show resolved Hide resolved
src/mc-schema-broker.c Outdated Show resolved Hide resolved
src/mc-schema-broker.c Outdated Show resolved Hide resolved
@kevinAlbs kevinAlbs merged commit c3dff01 into mongodb:master Feb 14, 2025
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants