GODRIVER-2391 add encryptedFields to CreateCollection and Drop #913

kevinAlbs · 2022-04-19T14:24:34Z

GODRIVER-2391
GODRIVER-2404

Summary

Add EncryptedFieldsMap to AutoEncryptionOpts
Add EncryptedFields to CreateCollectionOptions and required behavior.
Add DropCollectionOptions with EncryptedFields and required behavior.

Background & Motivation

Please see mongodb/specifications#1183 for the specification changes.

The name for the FLE 2.0 feature is still being decided by product. I left it out of public API documentation.

The fle2-CreateCollection test is written in the Client Side Encryption specification test format.

Get canary test passing.

… tests

add fle2-CreateCollection.json test

…ptions

It is redundant with new spec tests

commandName => command_name databaseName => database_name

Split into: - getEncryptedFieldsFromMap - getEncryptedFieldsFromServer

benjirewis

Looking great! I have some initial comments about the changes to Drop. I think I'll look at CreateCollection and the test runner changes after we discuss those.

benjirewis · 2022-05-06T19:24:07Z

mongo/collection.go

@@ -1664,7 +1664,94 @@ func (coll *Collection) Indexes() IndexView {

 // Drop drops the collection on the server. This method ignores "namespace not found" errors so it is safe to drop
 // a collection that does not exist on the server.
-func (coll *Collection) Drop(ctx context.Context) error {
+func (coll *Collection) Drop(ctx context.Context, opts ...*options.DropCollectionOptions) error {


Just to clarify, this is not a breaking change because opts is a variadic parameter?

Great question. I think this is a possible backwards break.

Consider a possible interface in user code:

// MockCollection implements methods of a mongo.Collection. type MockCollection interface { Drop(ctx context.Context) error // (Other methods) }

With the proposed changes, mongo.Collection no longer implements MockCollection.

I am not sure how common that pattern is. And IMO it would be a straightforward change for users to update interface definitions.

I had not considered this could be a backwards breaking change in drivers when adding to the specification. Since encryptedFields can also be obtained from the EncryptedFieldsMap and server-side. IMO I prefer erring towards reducing risk of breaking user code rather than strict specification conformance. We can consider adding an argument in a major release.

Updated to remove DropCollectionOptions and skip the tests that require it.

mongo/collection.go

mongo/options/dropcollectionoptions.go

benjirewis · 2022-05-06T20:10:58Z

mongo/options/dropcollectionoptions.go

+	// EncryptedFields configures encrypted fields for encrypted collections.
+	//
+	// This option is only valid for MongoDB versions >= 6.0
+	EncryptedFields interface{}


Is there any more meaningful type representation for EncryptedFields? interface{} can certainly be helpful if there is a need for forward compatibility. But, what exactly is the structure of encryptedFields on the server? map[string]string?

Is there any more meaningful type representation for EncryptedFields?

The rationale in the scope is consistency with JSONSchema representation. See Q8 in the scope:

Q8: Should encryptedFieldsEncryptedFieldConfig be BSON or a class?
Proposed Answer: BSON. This is consistent with the schema in the SchemaMap. Users only need to construct encryptedFieldsEncryptedFieldConfig for ClientEncryption.CreateEncryptedCollection.

AutoEncryptionOptions.SchemaMap is map[string]interface{}.

But, what exactly is the structure of encryptedFields on the server? map[string]string?

The server IDL has the definition. Here is an example:

{ "encryptedFields" : { "escCollection" : "fle2.testColl.esc", "eccCollection" : "fle2.testColl.ecc", "ecocCollection" : "fle2.testColl.ecoc", "fields" : [ { "keyId" : UUID("11d58b8a-0c6c-4d69-a0bd-70c6d9befae9"), "path" : "encryptedField", "bsonType" : "string", "queries" : { "queryType" : "equality", "contention" : NumberLong(0) } }, { "keyId" : UUID("11d58b8a-0c6c-4d69-a0bd-70c6d9befae9"), "path" : "nested.otherEncryptedField", "bsonType" : "string", "queries" : [ { "queryType" : "equality", "contention" : NumberLong(0) } ] } ] } }

benjirewis

Thanks for the quick response to the feedback! A few more questions.

mongo/collection.go

mongo/database.go

benjirewis · 2022-05-09T20:35:09Z

mongo/database.go

+}
+
+// getEncryptedStateCollectionName returns the encrypted state collection name associated with dataCollectionName.
+func getEncryptedStateCollectionName(efBSON *bsoncore.Document, dataCollectionName string, stateCollectionSuffix string) (*string, error) {


Why does this return *string instead of just string?

I want to return nil on error.

Hmm ok; I suppose you could just return "" on error, since you're probably not checking the string value if the returned error is non-nil. Up to you, though.

Returning a string and the string zero-value ("") on error is typically more idiomatic.

Updated to return string.

mongo/database.go

benjirewis · 2022-05-09T20:48:47Z

mongo/integration/unified_spec_test.go

@@ -55,6 +56,9 @@ var (
 		// i/o timeout.
 		"Ignore network timeout error on find":             godriver2123SkipReason,
 		"Network error on minPoolSize background creation": godriver2123SkipReason,
+		"CreateCollection from encryptedFields.":           godriver2413SkipReason,


Does this test also try to drop the collection with encrypted fields as an option for Drop? Why are we skipping this one?

Yes. It uses Drop with encrypted fields for test cleanup. Added a non-specification test: TestFLE2CreateCollection to test the behavior from this specification test.

Awesome thanks for adding that local test.

Tests skipped spec test behavior from the "CreateCollection from encryptedFields." test.

benjirewis

LGTM! Great work 😄

benjirewis · 2022-05-11T14:31:53Z

mongo/database.go

+		return nil, fmt.Errorf("expected encryptedFields of %v to be document, got %v", collectionName, rawValue.Type)
+	}
+
+	return &encryptedFields, nil


Any reason to pass by reference here?

No. Updated to return without taking address.

benjirewis · 2022-05-11T14:32:57Z

mongo/database.go

+}
+
+// getEncryptedStateCollectionName returns the encrypted state collection name associated with dataCollectionName.
+func getEncryptedStateCollectionName(efBSON *bsoncore.Document, dataCollectionName string, stateCollectionSuffix string) (*string, error) {


Hmm ok; I suppose you could just return "" on error, since you're probably not checking the string value if the returned error is non-nil. Up to you, though.

benjirewis · 2022-05-11T14:38:02Z

mongo/integration/unified_spec_test.go

@@ -55,6 +56,9 @@ var (
 		// i/o timeout.
 		"Ignore network timeout error on find":             godriver2123SkipReason,
 		"Network error on minPoolSize background creation": godriver2123SkipReason,
+		"CreateCollection from encryptedFields.":           godriver2413SkipReason,


Awesome thanks for adding that local test.

matthewdale · 2022-05-16T06:09:47Z

mongo/database.go

+}
+
+// getEncryptedStateCollectionName returns the encrypted state collection name associated with dataCollectionName.
+func getEncryptedStateCollectionName(efBSON *bsoncore.Document, dataCollectionName string, stateCollectionSuffix string) (*string, error) {


Returning a string and the string zero-value ("") on error is typically more idiomatic.

matthewdale · 2022-05-16T06:13:33Z

mongo/database.go

+}
+
+// getEncryptedStateCollectionName returns the encrypted state collection name associated with dataCollectionName.
+func getEncryptedStateCollectionName(efBSON *bsoncore.Document, dataCollectionName string, stateCollectionSuffix string) (*string, error) {


Can the efBSON param be a bsoncore.Document instead of a *bsoncore.Document?

matthewdale

Looks good 👍 , pending resolution of the questions about the getEncryptedStateCollectionName function signature.

kevinAlbs added 9 commits April 16, 2022 10:02

add encryptedFieldConfigMap to AutoEncryptionOpts

c21deed

add failing test for CreateCollection

f30aa4c

refactor: add createCollectionHelper

3425a80

add encryptedFieldConfig to create operation

a416de8

implement database.createEncryptedCollection.

567504d

Get canary test passing.

add failing TestDropEncryptedCollection

424ea83

add collection.dropEncrypedCollection

2af9be1

refactor: add a private createCollection helper

872bead

create data collection after state collections

350e5b5

kevinAlbs mentioned this pull request Apr 19, 2022

DRIVERS-2284 add FLE 2 behavior for CreateCollection() and Collection.Drop(). mongodb/specifications#1183

Merged

kevinAlbs added 18 commits April 19, 2022 12:54

use CreateCollection helper in legacy spec test runner

5ae3873

support encryptedFieldConfigMap in legacy spec test runner

0febcd0

remove unused ctx param from createCollectionOperation

a374532

add 6.0 and (replicaset or sharded) as run on requirements for canary…

9574dbd

… tests

add AddTestServerAPIVersion to canary tests

3de05b4

add fle2-CreateCollection.json test

06f956c

add "encryptedFieldConfigMap with cyclic entries does not loop"

63204bd

Use listCollections in Drop

6629625

add fle2-CreateCollection.json test

add EncryptedFieldConfig to CreateCollectionOptions

8b9fba0

add DropCollectionOptions and EncryptedFieldConfig to DropCollectionO…

1f64cca

…ptions

resync fle2-CreateCollection test

782ae3a

rename EncryptedFieldConfig to EncryptedFields

4073944

revert debug changes to cmd_monitoring_helpers_test

002f81a

remove client_encryption_test.go

3f61509

It is redundant with new spec tests

amend comments on dropEncryptedCollection

95dd22c

fixup comments in database

e5ceb1f

update name of receiver in dropcollectionoptions

b75cfe1

fix commandStarted format

d1e398b

commandName => command_name databaseName => database_name

kevinAlbs mentioned this pull request May 2, 2022

DRIVERS-2284 fix command_started_event field names mongodb/specifications#1203

Merged

5 tasks

split getEncryptedFields

b35dec4

Split into: - getEncryptedFieldsFromMap - getEncryptedFieldsFromServer

benjirewis reviewed May 6, 2022

View reviewed changes

kevinAlbs added 7 commits May 9, 2022 09:45

remove DropCollectionOptions and skip relevant tests

3e0b225

rename efc to ef

d4acfd1

add comments to Drop and CreateCollection

d34674f

fix typos

b2116bd

use one-liners for drop and createCollection calls

7971997

add more elaborate comment

2f69d4c

add getEncryptedStateCollectionName helper

a99d205

kevinAlbs requested a review from benjirewis May 9, 2022 14:34

benjirewis reviewed May 9, 2022

View reviewed changes

kevinAlbs added 8 commits May 10, 2022 11:31

remove unnecessary nil check

a382c45

use one-liner

d580e19

fixup comments

9487579

lowercase error message

c2389e4

remove unnecessary decl of efBSON

3d07b89

spelling

21852fe

fixup index error message

6230273

add TestFLE2CreateCollection

191bc4f

Tests skipped spec test behavior from the "CreateCollection from encryptedFields." test.

kevinAlbs requested a review from benjirewis May 10, 2022 16:42

benjirewis approved these changes May 11, 2022

View reviewed changes

matthewdale reviewed May 16, 2022

View reviewed changes

matthewdale approved these changes May 16, 2022

View reviewed changes

kevinAlbs added 6 commits May 16, 2022 18:54

remove unnecessary pointer return

a4aa9db

return string, not *string

dbfed8e

take bsoncore.Document, not *bsoncore.Document

8ed2222

Merge branch 'master' into create-drop-encrypted-collection

5b5bb7a

fix merge

2f205bf

fix indent-error-flow lint errors

1f8d163

kevinAlbs merged commit bb37ecd into mongodb:master May 17, 2022

prestonvasquez mentioned this pull request Nov 1, 2023

GODRIVER-2413 Add encryptedFields to Collection.Drop #1446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-2391 add encryptedFields to CreateCollection and Drop #913

GODRIVER-2391 add encryptedFields to CreateCollection and Drop #913

kevinAlbs commented Apr 19, 2022 •

edited

Loading

benjirewis left a comment

benjirewis May 6, 2022

kevinAlbs May 9, 2022

benjirewis May 6, 2022

kevinAlbs May 9, 2022

benjirewis left a comment

benjirewis May 9, 2022

kevinAlbs May 10, 2022

benjirewis May 11, 2022

matthewdale May 16, 2022

kevinAlbs May 16, 2022

benjirewis May 9, 2022

kevinAlbs May 10, 2022

benjirewis May 11, 2022

benjirewis left a comment

benjirewis May 11, 2022

kevinAlbs May 16, 2022

benjirewis May 11, 2022

benjirewis May 11, 2022

matthewdale May 16, 2022

matthewdale May 16, 2022

kevinAlbs May 16, 2022

matthewdale left a comment

GODRIVER-2391 add encryptedFields to CreateCollection and Drop #913

GODRIVER-2391 add encryptedFields to CreateCollection and Drop #913

Conversation

kevinAlbs commented Apr 19, 2022 • edited Loading

Summary

Background & Motivation

benjirewis 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

benjirewis 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

benjirewis 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

matthewdale left a comment

Choose a reason for hiding this comment

kevinAlbs commented Apr 19, 2022 •

edited

Loading