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

[DRIVERS-2241] Wording to add usage and API information for the csfle dynamic library #1165

Merged
merged 16 commits into from Apr 14, 2022

Conversation

vector-of-bool
Copy link
Contributor

@vector-of-bool vector-of-bool commented Mar 30, 2022

These changes include updates for client-side-encryption on how to use and expose the csfle dynamic library to perform command-marking in place of mongocryptd.

This does not yet include test cases for csfle.

Refer: DRIVERS-2241

@vector-of-bool vector-of-bool requested review from a team as code owners March 30, 2022 01:23
@vector-of-bool vector-of-bool requested review from JamesKovacs, comandeo and kevinAlbs and removed request for a team March 30, 2022 01:23
source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
2. If `extraOptions.csfleDisableSystemLibrary <cse.extraoptions_>`__ *was not*
specified as ``true`` by the user, append the literal string "``$SYSTEM``" to
the path list. (`Refer <cse.csfle.dollar-system_>`_)
3. Append each search path in |opt-paths-fallback|.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the server shared library planning docs, the expectation was that, in typical situations, the shared library would be deployed as part of application directories, not as part of the system libraries.

Considering this, would it be worth simplifying this interface to merge the csfleSearchPathsPrefer/csfleSearchPathsFallback/csfleDisableSystemLibrary options and only specify a single list of search paths, which may or may not include $SYSTEM? We already need to make users aware that $ORIGIN exists as an expansion.

(If the desire here is to make the shared library an opt-out feature rather than an opt-in one, we could make specifying an empty search path list different from specifying no search path list at all, i.e. make the default ["$SYSTEM"] rather than [].)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I interpretted the discussion as to whether it would be system-installed to be still tentative (although I would fall on the side of application-level deployment rather than system installation).

Given that we already need to document search path behavior, I think it would be feasible to merge these options and allow "set an empty path list" to be the "disable csfle" option. It will minimize the moving parts.

Although that still leaves an open question as to what the default should be (as related to your other comment). A default of ["$SYSTEM"] is reasonable if we prefer a system-wide installation, but a default of ["$ORIGIN"] would be better for application-level deployment (i.e. the user would only need to have csfle reside alongside the libmongocrypt library).

Either way, I like the idea to reduce the moving parts and merge these options and replace the "disable" toggle.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the server shared library planning docs, the expectation was that, in typical situations, the shared library would be deployed as part of application directories, not as part of the system libraries.

I missed that in scoping. I was under the impression that we should prefer a system-wide installation. I now see in the Technical Design a note to "Copy to app directory".

Although that still leaves an open question as to what the default should be (as related to your other comment). A default of ["$SYSTEM"] is reasonable if we prefer a system-wide installation, but a default of ["$ORIGIN"] would be better for application-level deployment (i.e. the user would only need to have csfle reside alongside the libmongocrypt library).

Drivers that a package libmongocrypt may need to override these defaults. That is Java, Python, C#, and Node.js (see How is libmongocrypt packaged for drivers?).

Would it make sense for drivers that rely on system install for libmongocrypt to use $SYSTEM as the default? If so, then using $SYSTEM as the default makes sense to me.

@vector-of-bool vector-of-bool requested a review from a team as a code owner March 31, 2022 00:55
Drivers MUST disable auto encryption when the 'bypassAutoEncryption' option
is |true| and not try to
`spawn mongocryptd <cse.managing-mongocryptd>` nor
`load csfle <cse.enabling-csfle>`. Automatic encryption may be
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it an explicit error to specify CSFLE library path options and bypassAutoEncryption, or will the CSFLE library path just be ignored? Either way is good for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

For precedence, it is not currently an error to specify both extraOptions.mongocryptdBypassSpawn
and extraOptions.mongocryptdSpawnPath. I am OK with either choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it would be an error, but it might be suspicious. Perhaps a "drivers MAY warn" would be useful?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer not using "MAY". It creates more possibility of inconsistency between driver implementations.

My vote goes for ignoring. There is little harm in ignoring. There is precedent with mongocryptdBypassSpawn and mongocryptdSpawnPath.

source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
@jmikola jmikola changed the title [DRIVERS-2241] Wording to add usage and API informatoin for the csfle dynamic library [DRIVERS-2241] Wording to add usage and API information for the csfle dynamic library Mar 31, 2022
Copy link
Contributor

@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.

The additional refactoring is nice. Do you mind moving the larger chunks of refactoring to a separate PR? Other DBX engineers will likely refer to the PR to see the new content. I think it will be easier to interpret if the diff is limited to the new content.

2. If `extraOptions.csfleDisableSystemLibrary <cse.extraoptions_>`__ *was not*
specified as ``true`` by the user, append the literal string "``$SYSTEM``" to
the path list. (`Refer <cse.csfle.dollar-system_>`_)
3. Append each search path in |opt-paths-fallback|.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the server shared library planning docs, the expectation was that, in typical situations, the shared library would be deployed as part of application directories, not as part of the system libraries.

I missed that in scoping. I was under the impression that we should prefer a system-wide installation. I now see in the Technical Design a note to "Copy to app directory".

Although that still leaves an open question as to what the default should be (as related to your other comment). A default of ["$SYSTEM"] is reasonable if we prefer a system-wide installation, but a default of ["$ORIGIN"] would be better for application-level deployment (i.e. the user would only need to have csfle reside alongside the libmongocrypt library).

Drivers that a package libmongocrypt may need to override these defaults. That is Java, Python, C#, and Node.js (see How is libmongocrypt packaged for drivers?).

Would it make sense for drivers that rely on system install for libmongocrypt to use $SYSTEM as the default? If so, then using $SYSTEM as the default makes sense to me.

Drivers MUST disable auto encryption when the 'bypassAutoEncryption' option
is |true| and not try to
`spawn mongocryptd <cse.managing-mongocryptd>` nor
`load csfle <cse.enabling-csfle>`. Automatic encryption may be
Copy link
Contributor

Choose a reason for hiding this comment

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

For precedence, it is not currently an error to specify both extraOptions.mongocryptdBypassSpawn
and extraOptions.mongocryptdSpawnPath. I am OK with either choice.

source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
``mongo_csfle_v1`` (with the appropriate file extension or filename suffix for
the host platform). This library will be loaded by `libmongocrypt` when the
``mongocrypt_init`` function is invoked
`(from the libmongocrypt C API) <lmc-c-api_>`_ based on the search critera that
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`(from the libmongocrypt C API) <lmc-c-api_>`_ based on the search critera that
`(from the libmongocrypt C API) <lmc-c-api_>`_ based on the search criteria that

source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
@vector-of-bool
Copy link
Contributor Author

@kevinAlbs I've stripped a lot of the not-necessary cleanup changes. Should be much more streamlined now.

Copy link
Contributor

@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.

LGTM pending the open question about $DEFAULT. Great work! Please re-request my review if you need further input.

I would like to wait until there is a C PoC to merge.

I have asked for a reviewer from the from a language team that packages libmongocrypt.

@kevinAlbs kevinAlbs requested a review from jyemin April 4, 2022 17:06
@kevinAlbs
Copy link
Contributor

Added @jyemin as a reviewer.

Copy link
Contributor

@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.

After talking with @jyemin, I realize The DBX scope answers the question Should csfle be available in package managers? with "No".

I have two new open questions about simplifying the API. I have left comments inline.

source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Just approving again to re-affirm that the current state of this PR LGTM

@vector-of-bool
Copy link
Contributor Author

I've revived the search-paths functionality in a limited fashion, not for the user-API, but to be an option to driver testing. Being able to allow drivers more control over the csfle search behavior is highly beneficial for testing purposes.

@jyemin jyemin self-requested a review April 12, 2022 18:46
@jyemin jyemin self-requested a review April 14, 2022 22:00
@vector-of-bool vector-of-bool merged commit a28f9c7 into mongodb:master Apr 14, 2022
@JamesKovacs JamesKovacs removed their request for review April 14, 2022 22:26
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.

None yet

5 participants