Skip to content

Support configuration of KMS provider credentials with a Supplier #894

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
merged 7 commits into from
Mar 23, 2022

Conversation

jyemin
Copy link
Collaborator

@jyemin jyemin commented Mar 16, 2022

Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

It seems redundant to have and use both:

    private final Map<String, Map<String, Object>> kmsProviders;
    private final Map<String, Supplier<Map<String, Object>>> kmsProviderSupplierMap;

Should the maps be merged by the builder? - It may make it easier to debug if users have the same key in both settings (eg a warning could be logged).

This way internally only getKmsProviderSupplierMap is used? and getKmsProviders() could be deprecated. The builder could keep both setters for simplicity as translating a value to a supplier of a value is trivial.

@jyemin
Copy link
Collaborator Author

jyemin commented Mar 17, 2022

Should the maps be merged by the builder?

It's actually required to have the same key in both maps. The kmsProviders map is for when you know the credentials up front and they don't need to be refreshed for the lifetime of the MongoClient. The kmsProviderSupplierMap is for when you don't know the credentials up front and/or they need to be refreshed periodically. But if you need to use the latter, it's still required that you have an empty map in the former, e.g. map.put("aws", Collections.emptyMap()), just to indicate that you want to use "aws" at all.

@jyemin jyemin requested a review from rozza March 17, 2022 16:09
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. MONGOCRYPT-394 is in review. I do not think it impacts the changes in this PR.

Comment on lines 299 to 311
/**
* Gets the KMS provider to Supplier map.
*
* <p>
* If the {@link #getKmsProviders()} map contains an empty map as its value, the driver will use a {@link Supplier} configured for
* the same provider in this map to obtain a non-empty map that contains the credential for the provider.
* </p>
*
* @return the KMS provider to Supplier map
* @see #getKmsProviders()
* @since 4.6
*/
public Map<String, Supplier<Map<String, Object>>> getKmsProviderSupplierMap() {
Copy link
Member

Choose a reason for hiding this comment

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

I found that both the method name and the documentation is confusing because of the provider/supplier word play (the same is applicable to the corresponding setter and to the counterpart in the ClientEncryptionSettings class). The problem is further aggravated by the method getKmsProviders, for which getKmsProviderProperties appears to be a more appropriate name, or better just getKmsProperties (we can't change it). In order to mitigate the problem I propose the following:

  • Rename the method to getKmsProviderPropertySuppliers or getKmsPropertySuppliers. We don't have the "Map" suffix in the getKmsProviders method, and it does not seem helpful here because a user may see it's a Map from the explicit return type. Having "Properties" part in the name is inconsistent with the name of the method getKmsProviders, but is helpful as it separates the "Provider" and "Supplier" parts.
  • Rename the kmsProviderSupplierMap field/parameter accordingly in this class and in MongoCryptHelper, Crypt.
  • Never use just "provider" in the documentation of the method, use "KMS provider" instead.
  • Never use just "Supplier" or "supplier" in the documentation of the method, use "Supplier of properties" instead.

Example:

  /**
     * This method is similar to {@link #getKmsProviders()},
     * but instead of getting properties for KMS providers,
     * it gets {@link Supplier}s of properties.
     * <p>If {@link #getKmsProviders()} returns empty properties for a KMS provider,
     * the driver will use a {@link Supplier} of properties configured for the KMS provider
     * to obtain a non-empty properties.</p>
     *
     * @return A {@link Map} where keys identify KMS providers,
     * and values specify {@link Supplier}s of properties for the KMS providers.
     * @see #getKmsProviders()
     * @since 4.6
     */

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went with kmsProviderPropertySuppliers as the property name. PTAL

I haven't touched the Javadoc yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For Javadoc changes, it would be easier if you propose the changes directly by adding a commit.

Copy link
Member

Choose a reason for hiding this comment

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

The proposed Java API documentation changes: stIncMale@57f5aac.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reviewed and pushed

@jyemin jyemin requested a review from stIncMale March 21, 2022 19:51
Copy link
Member

@stIncMale stIncMale left a comment

Choose a reason for hiding this comment

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

.

@stIncMale stIncMale self-requested a review March 23, 2022 16:15
@jyemin jyemin merged commit e9a555b into mongodb:master Mar 23, 2022
@jyemin jyemin deleted the j4504 branch March 23, 2022 16:15
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.

4 participants