You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by "kapoisu (via GitHub)" <gi...@apache.org> on 2025/04/23 15:38:49 UTC
[I] [C++][Parquet] Thoughts about classes in key_toolkit.h [arrow]
kapoisu opened a new issue, #46217:
URL: https://github.com/apache/arrow/issues/46217
### Describe the enhancement requested
KeyToolkit is used internally according to the comment. It looks more like a mere aggregate. I suggest redefine it as a struct and change existing member functions to free functions, which can be implemented by the public interfaces of existing member variables. The only exception which may introduce invariants is the first part of KeyToolket::RotateMasterKeys(),
```c++
const auto now = internal::CurrentTimePoint();
auto lock = last_cache_clean_for_key_rotation_time_mutex_.Lock();
if (now > last_cache_clean_for_key_rotation_time_ +
std::chrono::duration<double>(kCacheCleanPeriodForKeyRotation)) {
kek_write_cache_per_token().Clear();
last_cache_clean_for_key_rotation_time_ = now;
}
lock.Unlock();
```
However, TwoLevelCacheWithExpiration (the object return by kek_write_cache_per_token() in the code above) has nearly identical logic,
```c++
void CheckCacheForExpiredTokens(double cache_cleanup_period_seconds) {
auto lock = mutex_.Lock();
const auto now = internal::CurrentTimePoint();
if (now > (last_cache_cleanup_timestamp_ +
std::chrono::duration<double>(cache_cleanup_period_seconds))) {
RemoveExpiredEntriesNoMutex();
last_cache_cleanup_timestamp_ =
now + std::chrono::duration<double>(cache_cleanup_period_seconds);
}
}
```
except that the timestamp is set to now + cache_cleanup_period_seconds, which looks like a bug.
The same change can be applied to KeyWithMasterId.
### Component(s)
C++
--
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: issues-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts about classes in key_toolkit.h [arrow]
Posted by "kapoisu (via GitHub)" <gi...@apache.org>.
kapoisu commented on issue #46217:
URL: https://github.com/apache/arrow/issues/46217#issuecomment-2833759125
@adamreeve I want to confirm it again. The two-level cache object itself already contains a timestamp that is independent of the expiration time per key. And this timestamp cannot be used to handle the situations you mentioned so that an additional timestamp within the KeyToolkit is necessary?
Btw I've found another potential fix.
https://github.com/apache/arrow/blob/6e84d990379a0fe20a0a89311aa864f40efded23/cpp/src/parquet/encryption/two_level_cache_with_expiration.h#L98C3-L111C21
The insert() does nothing when the key exists. This should be either:
1. Remove the isExpired() check.
2. Update the expiration timestamp when the key is expired.
RemoveExpiredEntriesNoMutex() also seems duplicate. It's currently used by CheckCacheForExpiredTokens only and the mutex is acquired anyway.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts about classes in key_toolkit.h [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on issue #46217:
URL: https://github.com/apache/arrow/issues/46217#issuecomment-2825621423
`KeyToolkit` is technically public so this would be a breaking change, but I don't think this was intentional. Eg. for key rotation users should be using the `CryptoFactory`, so I think it's OK to change the `KeyToolkit` API in theory.
> except that the timestamp is set to now + cache_cleanup_period_seconds, which looks like a bug.
Yes that definitely looks like a bug and should be fixed.
I don't think you can simply remove the cleanup logic from `RotateMasterKeys` though. It's important that that has its own "last clean up" time, as calling `RotateMasterKeys` is an indication that the master keys have been updated so key encryption keys need to be re-encrypted, and the full KEK cache should immediately be cleared including keys that have not yet expired.
The reason this isn't done on every call to `RotateMasterKeys` is that after master keys have been updated you'll often want to re-encrypt DEKs for multiple files and re-use the new KEKs.
There are also other methods on `KeyToolkit` that require access to multiple members, eg. `GetKmsClient`, `RemoveCacheEntriesForToken` and `RemoveCacheEntriesForAllTokens`. So the current structure of `KeyToolkit` being a class with these methods makes sense to me, and I don't see much benefit in changing it to a struct and separate free functions.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts about classes in key_toolkit.h [arrow]
Posted by "kapoisu (via GitHub)" <gi...@apache.org>.
kapoisu commented on issue #46217:
URL: https://github.com/apache/arrow/issues/46217#issuecomment-2845248478
> I don't think you can simply move the `RemoveExpiredEntriesNoMutex` logic into `RemoveExpiredEntriesFromCache`, as the mutex is also needed to handle updating `last_cache_cleanup_timestamp_` in `CheckCacheForExpiredTokens` and prevent repeated clean ups. And `std::mutex` is non-recursive so you can't call `RemoveExpiredEntriesFromCache` while already holding a lock.
I got it. Does last_cache_cleanup_timestamp_ need to be updated within ```RemoveExpiredEntriesFromCache``` or the timestamp is solely for ```CheckCacheForExpiredTokens```? If it did, ```RemoveExpiredEntriesFromCache``` would literally be CheckCacheForExpiredTokens(0.0).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts about classes in key_toolkit.h [arrow]
Posted by "kapoisu (via GitHub)" <gi...@apache.org>.
kapoisu commented on issue #46217:
URL: https://github.com/apache/arrow/issues/46217#issuecomment-2826469372
Thanks for the clarification Adam! I'll submit a PR to fix the timestamp then.
In addition, I noticed that when I built parquet with PARQUET_REQUIRE_ENCRYPTION=ON, the OpenSSL dependency didn't get downloaded as others. Is this the intended behavior?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts about classes in key_toolkit.h [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on issue #46217:
URL: https://github.com/apache/arrow/issues/46217#issuecomment-2824783597
cc @adamreeve
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts about classes in key_toolkit.h [arrow]
Posted by "kapoisu (via GitHub)" <gi...@apache.org>.
kapoisu commented on issue #46217:
URL: https://github.com/apache/arrow/issues/46217#issuecomment-2834471299
> It's also used by `RemoveExpiredEntriesFromCache`, although as far as I can see that's only used in one test.
Yup. I meant ```RemoveExpiredEntriesFromCache``` is just "acquire the mutex and call ```RemoveExpiredEntriesNoMutex```". While ```CheckCacheForExpiredTokens``` calls ```RemoveExpiredEntriesNoMutex```, it also acquires the mutex before that. That means it can call ```RemoveExpiredEntriesFromCache``` directly. Although I'm not sure if moving ```RemoveExpiredEntriesNoMutex```'s logic to ```RemoveExpiredEntriesFromCache``` is a good a idea, considering there may be usages that don't require the mutex in the future?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts on classes in key_toolkit.h [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on issue #46217:
URL: https://github.com/apache/arrow/issues/46217#issuecomment-2846106435
Yeah I think you're right, you could replace `RemoveExpiredEntriesFromCache` with a call to `CheckCacheForExpiredTokens(0.0)`, it would make sense to me that `RemoveExpiredEntriesFromCache` should be updating `last_cache_cleanup_timestamp_`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts on classes in key_toolkit.h [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on issue #46217:
URL: https://github.com/apache/arrow/issues/46217#issuecomment-3024508928
Issue resolved by pull request 46283
https://github.com/apache/arrow/pull/46283
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts about classes in key_toolkit.h [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on issue #46217:
URL: https://github.com/apache/arrow/issues/46217#issuecomment-2836829518
I don't think you can simply move the `RemoveExpiredEntriesNoMutex` logic into `RemoveExpiredEntriesFromCache`, as the mutex is also needed to handle updating `last_cache_cleanup_timestamp_` in `CheckCacheForExpiredTokens` and prevent repeated clean ups. And `std::mutex` is non-recursive so you can't call `RemoveExpiredEntriesFromCache` while already holding a lock.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts about classes in key_toolkit.h [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on issue #46217:
URL: https://github.com/apache/arrow/issues/46217#issuecomment-2833681365
> In addition, I noticed that when I built parquet with PARQUET_REQUIRE_ENCRYPTION=ON, the OpenSSL was required but didn't get downloaded as other dependencies did. Is this the intended behavior?
It looks like this is intentional and we always try to use the system OpenSSL, but I'm not sure what the reason is for this: https://github.com/apache/arrow/blob/6e84d990379a0fe20a0a89311aa864f40efded23/cpp/cmake_modules/ThirdpartyToolchain.cmake#L1586
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts about classes in key_toolkit.h [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on issue #46217:
URL: https://github.com/apache/arrow/issues/46217#issuecomment-2833777178
> @adamreeve I want to confirm it again. The two-level cache object itself already contains a timestamp that is independent of the expiration time per key. And this timestamp cannot be used to handle the situations you mentioned so that an additional timestamp within the KeyToolkit is necessary?
Yes, we can't use the `last_cache_cleanup_timestamp_` in `TwoLevelCacheWithExpiration` to handle the key rotation use case.
> The insert() does nothing when the key exists
Yes that also looks like a bug. I think rather than just updating the expiration timestamp we want to replace the entry with a new empty cache. We could probably replace `insert` with `insert_or_assign`?
> RemoveExpiredEntriesNoMutex() also seems duplicate. It's currently used by CheckCacheForExpiredTokens() only and the mutex is acquired anyway.
It's also used by `RemoveExpiredEntriesFromCache`, although as far as I can see that's only used in one test.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][Parquet] Thoughts on classes in key_toolkit.h [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou closed issue #46217: [C++][Parquet] Thoughts on classes in key_toolkit.h
URL: https://github.com/apache/arrow/issues/46217
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org