wgtmac commented on code in PR #40329:
URL: https://github.com/apache/arrow/pull/40329#discussion_r1510479145
##########
cpp/src/parquet/encryption/crypto_factory.cc:
##########
@@ -28,9 +28,38 @@
namespace parquet::encryption {
+namespace {
+
+/// Holds a FileKeyUnwrapper and shared pointer to a KeyToolkit to allow
+/// keeping the KeyToolkit alive alongside the FileKeyUnwrapper
+class CryptoFactoryFileKeyRetriever : public DecryptionKeyRetriever {
+ public:
+ CryptoFactoryFileKeyRetriever(
+ std::shared_ptr<KeyToolkit> key_toolkit,
+ const KmsConnectionConfig& kms_connection_config, double
cache_lifetime_seconds,
+ const std::string& file_path = "",
+ const std::shared_ptr<::arrow::fs::FileSystem>& file_system = NULLPTR)
+ : file_key_unwrapper_(key_toolkit.get(), kms_connection_config,
+ cache_lifetime_seconds, file_path, file_system) {
+ key_toolkit_ = key_toolkit;
Review Comment:
nit: this can use std::move() instead.
##########
cpp/src/parquet/encryption/crypto_factory.cc:
##########
@@ -172,8 +201,8 @@ std::shared_ptr<FileDecryptionProperties>
CryptoFactory::GetFileDecryptionProper
const KmsConnectionConfig& kms_connection_config,
const DecryptionConfiguration& decryption_config, const std::string&
file_path,
const std::shared_ptr<::arrow::fs::FileSystem>& file_system) {
- auto key_retriever = std::make_shared<FileKeyUnwrapper>(
- &key_toolkit_, kms_connection_config,
decryption_config.cache_lifetime_seconds,
+ auto key_retriever = std::make_shared<CryptoFactoryFileKeyRetriever>(
Review Comment:
It looks like we hit the issue of member variable lifecycle in the
FileDecryptionProperties class. I see there is already a
`FileDecryptionProperties::DeepClone()` function. Should we expect the end user
to explicitly clone it?
##########
cpp/src/parquet/encryption/crypto_factory.cc:
##########
@@ -28,9 +28,38 @@
namespace parquet::encryption {
+namespace {
+
+/// Holds a FileKeyUnwrapper and shared pointer to a KeyToolkit to allow
+/// keeping the KeyToolkit alive alongside the FileKeyUnwrapper
+class CryptoFactoryFileKeyRetriever : public DecryptionKeyRetriever {
+ public:
+ CryptoFactoryFileKeyRetriever(
+ std::shared_ptr<KeyToolkit> key_toolkit,
+ const KmsConnectionConfig& kms_connection_config, double
cache_lifetime_seconds,
+ const std::string& file_path = "",
+ const std::shared_ptr<::arrow::fs::FileSystem>& file_system = NULLPTR)
+ : file_key_unwrapper_(key_toolkit.get(), kms_connection_config,
+ cache_lifetime_seconds, file_path, file_system) {
+ key_toolkit_ = key_toolkit;
+ }
+
+ std::string GetKey(const std::string& key_metadata) override {
+ return file_key_unwrapper_.GetKey(key_metadata);
+ }
+
+ private:
+ FileKeyUnwrapper file_key_unwrapper_;
+ std::shared_ptr<KeyToolkit> key_toolkit_;
+};
+
+} // namespace
+
+CryptoFactory::CryptoFactory() { key_toolkit_ =
std::make_shared<KeyToolkit>(); }
Review Comment:
nit: move the assignment to initialization list, or simply initializing it
in the header file to get rid of the explicit default constructor.
##########
cpp/src/parquet/encryption/key_management_test.cc:
##########
@@ -323,6 +323,36 @@ TEST_F(TestEncryptionKeyManagement,
KeyRotationWithInternalMaterial) {
EXPECT_THROW(this->RotateKeys(double_wrapping, encryption_no),
ParquetException);
}
+TEST_F(TestEncryptionKeyManagement, UsePropertiesAfterCrytoFactoryDestroyed) {
+ std::shared_ptr<KmsClientFactory> kms_client_factory =
+ std::make_shared<TestOnlyInMemoryKmsClientFactory>(true, key_list_);
Review Comment:
```suggestion
std::make_shared<TestOnlyInMemoryKmsClientFactory>(/*wrap_locally=*/true,
key_list_);
```
Or should we directly use `wrap_locally_`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
