Skip to content

[CDRIVER-4568] Limit options available for createEncryptedCollection #1218

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

Merged

Conversation

vector-of-bool
Copy link
Contributor

Refer: CDRIVER-4568

This changeset changes the parameters available in create-encrypted-collection to specifying only the masterKey to be used for datakey creation. The prior version allowed all datakey options to be passed through, but most of them were conflicting or nonsensical in this context. Refer to the linked DRIVERS ticket.

The required changes also re-specified the test cases to test both local as well as aws KMS providers. The test cases in question have been updated to run over both configurations.

@vector-of-bool vector-of-bool force-pushed the CDRIVER-4568-cec-no-keyAlt branch from e200808 to 77d9dba Compare March 14, 2023 00:47
@vector-of-bool vector-of-bool marked this pull request as ready for review March 14, 2023 17:59
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.

Minor feedback; otherwise LGTM.

test_create_encrypted_collection_simple (void *unused)
_do_cec_test (void (*test) (const char *kmsProvider))
{
// Run the test using the "local" key:
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit unclear what this comment is documenting. Is it documenting that local should be tested before AWS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's become a redundant comment. Gone.

@@ -16,7 +16,7 @@ Synopsis
const bson_t *in_options,
bson_t *out_options,
const char *kms_provider,
const mongoc_client_encryption_datakey_opts_t *dk_opts,
const bson_t *opt_masterKey,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest adding a way to extend options. When this API becomes stable, it may be helpful to have a way to add supported options. Consider adding a mongoc_client_encryption_cec_opts_t type to support only this option:

:man_page: mongoc_client_encryption_cec_opts_t

mongoc_client_encryption_cec_opts_t
===================================

Synopsis
--------

.. code-block:: c

  typedef struct _mongoc_client_encryption_cec_opts_t mongoc_client_encryption_cec_opts_t;

Used to set options for :symbol:`mongoc_client_encryption_create_encrypted_collection()`.

.. only:: html

  Functions
  ---------

  .. toctree::
    :titlesonly:
    :maxdepth: 1

    mongoc_client_encryption_cec_opts_new
    mongoc_client_encryption_cec_opts_destroy
    mongoc_client_encryption_cec_opts_set_masterkey

.. seealso::

  | :symbol:`mongoc_client_encryption_create_encrypted_collection()`

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 see this pattern throughout the C driver and I question its ergonomics over using _v2/_v3 interfaces that add the additional options, which is used in most C APIs I'm familiar with. Using dynamically allocated options-objects adds significant complexity at call sites.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair point. The option structs may not add much benefit. Adding options to the structs still requires an API addition of the function call. The alternative API addition of a _v2 / _v3 may be preferable.

@vector-of-bool vector-of-bool merged commit e24a7a2 into mongodb:master Mar 29, 2023
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