oscerd commented on PR #24676: URL: https://github.com/apache/camel/pull/24676#issuecomment-4968510632
Thanks for the review @davsclaus. Both observations checked out — one is now fixed, one I'd like to keep as-is. Details below. ## 1. Log-level inconsistency in `InMemoryKeyLifecycleManager` — **fixed** Good catch, and it turned out to be more than cosmetic. Checking against the three existing implementations, they all agree with each other and the new class was the sole outlier: | Event | `FileBased` | `AwsSecretsManager` | `HashicorpVault` | `InMemory` (before) | |---|---|---|---|---| | `Generating key pair…` | INFO | INFO | INFO | **DEBUG** | | `Generated key pair…` | INFO | INFO | INFO | **DEBUG** | | `Deleted key…` | INFO | INFO | INFO | **DEBUG** | | rotate / expire / revoke | INFO | INFO | INFO | INFO | | internal store plumbing | DEBUG | DEBUG | DEBUG | _(no log)_ | Key generation and key deletion are security-relevant lifecycle events, and `KeyLifecycleManager` implementations are meant to be interchangeable — so as written, swapping `FileBasedKeyLifecycleManager` for `InMemoryKeyLifecycleManager` would have silently dropped those two lines from an INFO-level audit log. Those three calls are now INFO, so all four implementations log the same lifecycle events at the same level, and DEBUG stays reserved for internal plumbing. ## 2. Wildcard import in `KeyAlgorithmSupport` — **keeping as-is** You're right that it was carried over from `FileBasedKeyLifecycleManager` (which this PR does clean up — the now-unused BouncyCastle imports go away along with the four extracted methods). But it is the established pattern for that package rather than a new deviation, and the conditional risk you flagged doesn't materialise: - `HashicorpVaultKeyLifecycleManager` and `AwsSecretsManagerKeyLifecycleManager` both carry the **identical** `import org.bouncycastle.pqc.jcajce.spec.*;` on `main` today. - It covers **12 distinct classes** from that single package in `KeyAlgorithmSupport` — the `BIKE`, `CMCE`, `Falcon`, `Frodo`, `HQC`, `LMSKeyGen`, `NTRU`, `NTRULPRime`, `SABER`, `SNTRUPrime`, `XMSS` and `XMSSMT` parameter specs — which is precisely why the wildcard is there. - `formatter:format` and `impsort:sort` are both bound to `process-sources` in this module, so the module build does run them: it leaves the file untouched, no expansion and nothing to commit. That matches the green CI on this PR, formatting gate included. Expanding it would make `KeyAlgorithmSupport` the only file in the package importing those specs one by one, so I'd rather keep the module self-consistent. Happy to do a separate sweep across all of `camel-pqc` if you'd prefer the explicit style module-wide — that's a bigger change than this PR should carry. --- All 25 `camel-pqc` lifecycle tests still pass (11 `InMemoryKeyLifecycleManagerTest` + 9 `KeyRotationSchedulerTest` + 5 `KeyMetadataCodecTest`), and the full reactor build is green. _Claude Code on behalf of Andrea Cosentino._ -- 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]
