adamreeve commented on code in PR #40732:
URL: https://github.com/apache/arrow/pull/40732#discussion_r1535224399
##########
cpp/src/parquet/encryption/file_key_unwrapper.cc:
##########
@@ -30,33 +30,37 @@ FileKeyUnwrapper::FileKeyUnwrapper(
const std::string& file_path,
const std::shared_ptr<::arrow::fs::FileSystem>& file_system)
: FileKeyUnwrapper(std::move(key_toolkit), /*key_toolkit=*/nullptr,
- kms_connection_config, cache_lifetime_seconds,
file_path,
- file_system) {}
+ kms_connection_config, cache_lifetime_seconds,
+ /*key_material_store=*/nullptr, file_path, file_system)
{}
FileKeyUnwrapper::FileKeyUnwrapper(
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)
: FileKeyUnwrapper(/*key_toolkit_owner=*/nullptr, key_toolkit,
kms_connection_config,
- cache_lifetime_seconds, file_path, file_system) {}
+ cache_lifetime_seconds, /*key_material_store=*/nullptr,
file_path,
+ file_system) {}
FileKeyUnwrapper::FileKeyUnwrapper(
KeyToolkit* key_toolkit, const KmsConnectionConfig& kms_connection_config,
double cache_lifetime_seconds,
std::shared_ptr<FileKeyMaterialStore> key_material_store)
: FileKeyUnwrapper(/*key_toolkit_owner=*/nullptr, key_toolkit,
kms_connection_config,
- cache_lifetime_seconds, /*file_path=*/"",
+ cache_lifetime_seconds, std::move(key_material_store),
Review Comment:
This is unexpected. It looks like during the process of working on #34181 I
refactored things so that this parameter was no longer used but didn't realise
this. The `FileKeyUnwrapper` created in `KeyToolkit::RotateMasterKeys` is only
ever used to call `GetDataEncryptionKey`, which takes a `KeyMaterial` parameter
so bypasses the `key_material_store_` completely, and there's no reason it
should need to be passed in to the constructor.
I also added an incorrect comment about this constructor overload in
`file_key_unwrapper.h`, saying, "This is useful for key rotation to allow
accessing the key material store after it is used." I guess I got myself
confused with the use of the `temp_key_material_store` passed to the
`FileKeyWrapper`.
It seems to me that this overload can be removed completely, and
`KeyToolkit::RotateMasterKeys` can use the other constructor. The `file_path`
and `filesystem` could be provided but won't be used.
--
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]
