RUST-1382 FLE-related `Client` initialization #690

abr-egn · 2022-07-07T17:43:19Z

This allows creating a Client with auto-encryption enabled, and spins up the various bits of internal state that are needed for that.

abr-egn · 2022-07-07T17:47:54Z

src/client/fle/options.rs

+pub type KmsProvidersTlsOptions = HashMap<KmsProvider, TlsOptions>;
+
+impl AutoEncryptionOpts {
+    pub(crate) fn extra_option<'a, Opt: ExtraOption<'a, Bson>>(


This machinery incurs a bit of extra boilerplate here but saves it at the usage sites; it also ensures that code using extra_options didn't accidentally get the field type wrong.

I really like this approach--it makes the requirement of having to use a map for this stuff not nearly as bad.

abr-egn · 2022-07-07T17:49:39Z

src/client/mod.rs

    options: ClientOptions,
    session_pool: ServerSessionPool,
+    #[cfg(feature = "fle")]
+    fle: std::sync::Mutex<Option<fle::ClientState>>,


It's a little awkward to have to wrap this in a Mutex since it's not mutated after client construction, but since making the state for FLE needs the main Client as part of its own initialization parameters, I didn't see a way around this.

We could potentially get around this by creating a WeakClient type or some such, but I think we're going to need some form of lock anyways in order to invoke the mutable mongocrypt methods, so it's not a big deal. Since ClientState stores Clients within it, we need to be really careful not to use them while holding the std lock guard, as we can easily deadlock (maybe the compiler will enforce this?). To avoid this, we could introduce a wrapper type or something that manages handling the lock, or alternatively just use an async one.

Also, we may want a RwLock, where the read portion can be used to access the clients and the write portion for mutating the state. Though maybe its best to defer a decision on these questions until the implementation moves a little further and the use cases are clearer.

Good call on the deadlock risk - updated to use an async mutex. Agreed that it'll be easier to see whether Mutex or RwLock is better based on usage, so I've left it as that for now.

I realized this needed to be a weak ref in any case, since otherwise there's a potential for creating a reference loop with the main Client holding a ref to itself. I also updated it to a RwLock because even very early further implementation points to that being the way to go.

abr-egn · 2022-07-07T17:51:53Z

src/client/mod.rs

+    /// Creates a new `Client` connected to the cluster specified by `options` with auto-encryption
+    /// enabled.
+    #[cfg(feature = "fle")]
+    pub async fn with_fle_options(


Ideally AutoEncryptionOpts could have just been another field in ClientOptions. Unfortunately, initializing FLE state requires async because it has to construct a client to the mongocryptd from an arbitrary URI. Since there needs to be a distinct creation method to accommodate that, it seemed worth keeping the options distinct to make it clear that if you're setting those options you have to use this method.

src/client/fle/options.rs

patrickfreed · 2022-07-08T17:53:55Z

src/client/fle/options.rs

+pub type KmsProvidersTlsOptions = HashMap<KmsProvider, TlsOptions>;
+
+impl AutoEncryptionOpts {
+    pub(crate) fn extra_option<'a, Opt: ExtraOption<'a, Bson>>(


I really like this approach--it makes the requirement of having to use a map for this stuff not nearly as bad.

patrickfreed · 2022-07-08T18:23:21Z

src/client/mod.rs

    options: ClientOptions,
    session_pool: ServerSessionPool,
+    #[cfg(feature = "fle")]
+    fle: std::sync::Mutex<Option<fle::ClientState>>,


We could potentially get around this by creating a WeakClient type or some such, but I think we're going to need some form of lock anyways in order to invoke the mutable mongocrypt methods, so it's not a big deal. Since ClientState stores Clients within it, we need to be really careful not to use them while holding the std lock guard, as we can easily deadlock (maybe the compiler will enforce this?). To avoid this, we could introduce a wrapper type or something that manages handling the lock, or alternatively just use an async one.

Also, we may want a RwLock, where the read portion can be used to access the clients and the write portion for mutating the state. Though maybe its best to defer a decision on these questions until the implementation moves a little further and the use cases are clearer.

Cargo.toml

src/client/mod.rs

src/error.rs

patrickfreed

One more naming nitpick, but otherwise LGTM!

patrickfreed · 2022-07-14T17:32:13Z

src/client/mod.rs

+    /// Creates a new `Client` connected to the cluster specified by `options` with auto-encryption
+    /// enabled.
+    #[cfg(feature = "csfle")]
+    pub async fn with_encryption_opts(


for consistency with with_options, i think we might want to name this with_encryption_options actually.

patrickfreed · 2022-07-14T17:35:49Z

Cargo.toml

 hmac = "0.12.1"
 lazy_static = "1.4.0"
 md-5 = "0.10.1"
+mongocrypt = { git = "https://github.com/mongodb/libmongocrypt-rust.git", branch = "main", optional = true }


looks like we might need to add a cargo-deny ignore for this in particular. I imagine we have one for bson already, otherwise that would trigger too right?

Yup, fixed.

isabelatkinson · 2022-07-14T19:33:23Z

src/client/csfle.rs

+}
+
+impl ClientState {
+    pub(super) async fn new(client: &Client, opts: AutoEncryptionOpts) -> Result<Option<Self>> {


Why does this need to return a Result<Option<Self>>? Is there any case in which Ok(None) should be returned?

Good catch - that was left over from earlier structure. It now just returns Result<Self>.

kmahar

looking good! a few minor comments.

src/client/csfle.rs

kmahar · 2022-07-14T20:39:14Z

Cargo.toml

 hmac = "0.12.1"
 lazy_static = "1.4.0"
 md-5 = "0.10.1"
+mongocrypt = { git = "https://github.com/mongodb/libmongocrypt-rust.git", branch = "main", optional = true }


looks like adding this dependency breaks MSRV compilation, since libmongocrypt-rust uses Rust 2021 edition and Rust 1.53 isn't aware of that existing. based on the data I shared, I think we're safe to bump our MSRV to 1.56. should we either go ahead with that or temporarily downgrade libmongocrypt to Rust 2018 to avoid breaking our MSRV compile tasks when this merged?

I'm inclined to go ahead with the MSRV bump - I'll send that in its own PR.

src/client/csfle/options.rs

kmahar

LGTM!

abr-egn commented Jul 7, 2022

View reviewed changes

abr-egn requested review from isabelatkinson, kmahar and patrickfreed July 7, 2022 17:52

patrickfreed reviewed Jul 8, 2022

View reviewed changes

abr-egn force-pushed the RUST-1382/fle-client-init branch 2 times, most recently from af42cad to d4aa608 Compare July 12, 2022 17:00

patrickfreed approved these changes Jul 14, 2022

View reviewed changes

patrickfreed reviewed Jul 14, 2022

View reviewed changes

isabelatkinson reviewed Jul 14, 2022

View reviewed changes

kmahar reviewed Jul 14, 2022

View reviewed changes

abr-egn force-pushed the RUST-1382/fle-client-init branch from 2b6f72f to 13a16a5 Compare July 15, 2022 14:41

kmahar approved these changes Jul 19, 2022

View reviewed changes

abr-egn requested a review from isabelatkinson July 25, 2022 14:49

isabelatkinson approved these changes Jul 25, 2022

View reviewed changes

abr-egn added 15 commits July 25, 2022 12:02

opts

bf93066

opts docs

e053c4f

auxiliary client init

0333d4d

init crypt

040b400

split fle into its own mod

2d076b9

spawn mongocryptd

4c5e76d

keys for extra_options

840d50f

mongocryptd client

6d41287

split out fle options

38e6886

rustfmt

0795ee8

document state of fle

a24a0ee

point mongocrypt dep back at git

6e6a343

review updates

4487f6e

weak client refs

45b755a

store auto enc opts

3f7cc19

abr-egn added 2 commits July 25, 2022 12:02

deny and bikeshed

d148262

review updates

6e4f1b1

abr-egn force-pushed the RUST-1382/fle-client-init branch from 13a16a5 to 6e4f1b1 Compare July 25, 2022 16:02

abr-egn merged commit 9692371 into mongodb:main Jul 25, 2022

RUST-1382 FLE-related Client initialization #690

RUST-1382 FLE-related Client initialization #690

Uh oh!

Conversation

abr-egn commented Jul 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrickfreed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmahar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kmahar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RUST-1382 FLE-related `Client` initialization #690

RUST-1382 FLE-related `Client` initialization #690