Copilot commented on code in PR #51350:
URL: https://github.com/apache/arrow/pull/51350#discussion_r4021892442
##########
cpp/src/parquet/encryption/key_toolkit.h:
##########
@@ -77,7 +77,8 @@ class PARQUET_EXPORT KeyToolkit {
void RotateMasterKeys(const KmsConnectionConfig& kms_connection_config,
const std::string& parquet_file_path,
const std::shared_ptr<::arrow::fs::FileSystem>&
file_system,
- bool double_wrapping, double cache_lifetime_seconds);
+ bool double_wrapping, double cache_lifetime_seconds,
+ bool read_kms_url);
Review Comment:
`KeyToolkit` is an exported public class, but this newly added parameter has
no default. Existing callers of `KeyToolkit::RotateMasterKeys` using the
previous five-argument signature will stop compiling, unlike callers of the
higher-level `CryptoFactory` API. Give this parameter a `= false` default (or
preserve the old overload).
##########
cpp/src/parquet/encryption/file_key_unwrapper.cc:
##########
@@ -133,25 +133,31 @@ KeyWithMasterId
FileKeyUnwrapper::GetDataEncryptionKey(const KeyMaterial& key_ma
return KeyWithMasterId(std::move(data_key), master_key_id);
}
+void FileKeyUnwrapper::EnableReadingKmsUrl() { read_kms_url_ = true; }
+
std::shared_ptr<KmsClient>
FileKeyUnwrapper::GetKmsClientFromConfigOrKeyMaterial(
const KeyMaterial& key_material) {
std::string& kms_instance_id = kms_connection_config_.kms_instance_id;
if (kms_instance_id.empty()) {
kms_instance_id = key_material.kms_instance_id();
if (kms_instance_id.empty()) {
throw ParquetException(
- "KMS instance ID is missing both in both kms connection
configuration and file "
+ "KMS instance ID is missing in both the KMS connection configuration
and file "
"key material");
}
}
std::string& kms_instance_url = kms_connection_config_.kms_instance_url;
if (kms_instance_url.empty()) {
- kms_instance_url = key_material.kms_instance_url();
- if (kms_instance_url.empty()) {
- throw ParquetException(
- "KMS instance ID is missing both in both kms connection
configuration and file "
- "key material");
+ if (read_kms_url_) {
+ kms_instance_url = key_material.kms_instance_url();
+ if (kms_instance_url.empty()) {
+ throw ParquetException(
+ "KMS instance URL is missing in both the KMS connection
configuration and "
+ "the file key material");
+ }
+ } else {
+ kms_instance_url = KmsClient::kKmsInstanceUrlDefault;
Review Comment:
Because `KeyToolkit` caches clients by access token and instance ID only
(not URL), this assignment does not enforce the new per-read default when a
`CryptoFactory` has already created a client for the same token/ID with
`read_kms_url=True`: a later `read_kms_url=False` read reuses that client and
can still send the token to the earlier file URL. Include the URL in the cache
key or otherwise isolate/invalidate URL-derived clients so the false setting is
effective.
--
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]
