Conversation
JdbcComponent and SqlComponent both hold a DataSource that is typically a registry bean (HikariCP, DBCP2, ...) not recreated during a route reload. After a vault-triggered context reload the pool still holds connections authenticated with the old credentials. Implement SecretRotationAware.onSecretRotation() in both components to evict stale connections: - HikariCP: softEvictConnections() called via reflection so that neither module needs a compile-time dependency on HikariCP - Other pools: generic fallback path logs that connections will expire naturally (no action needed for pools that validate on borrow) Both owned (component.setDataSource) and registry-sourced DataSources are handled: the component iterates over all DataSource beans in the registry plus its own dataSource field, if set. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 565 of 695 tested, 0 compile-only — current: 565 all testedMaveniverse Scalpel detected 565 affected modules (current approach: 565). Skip-tests mode would test 565 modules (4 direct + 33 downstream), skip tests for 0 (generated code, meta-modules) Modules Scalpel would test (565)
Build reactor — dependencies compiled but only changed modules were tested (4 modules, 1m 15s total)Total reactor time: 1m 15s
Top 20 slowest modules:
|
apupier
left a comment
apupier
left a comment
There was a problem hiding this comment.
the method name used is suspicious, I'm unable to find it.
in test it is creating dummy DataSource class adding this method artificially so i tis not testing a real case.
searching in Hikari whole repository https://github.com/search?q=repo%3Abrettwooldridge%2FHikariCP+softEvictConnections&type=code it seems to be available only on HikaripoolMXBean and HikariPool but as far as I undertand the code it is supposed to be on a DataSource
|
instead of forging a class with softEvictConnections in tests, we could add a test dependency to HikariCP which could allow to test effectively |
…ns() HikariCP's softEvictConnections() lives on HikariPoolMXBean, not on HikariDataSource itself. The previous code tried to call it directly on the DataSource instance, which would always throw NoSuchMethodException in production. Fix: retrieve the MXBean via getHikariPoolMXBean() (a public method on HikariDataSource) using reflection, then invoke softEvictConnections() on the MXBean. Updated tests simulate the two-step indirection.
|
@apupier You're right — Fixed in c1aa008: we now retrieve the MXBean via |
|
@apupier Thanks for catching that! You're right that
This matches the standard HikariCP pattern documented in their wiki: HikariPoolMXBean poolMBean = dataSource.getHikariPoolMXBean();
poolMBean.softEvictConnections();The tests were also updated to simulate this two-step indirection accurately — the |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Review: CAMEL-24638 — SecretRotationAware for camel-jdbc / camel-sql
Two issues to address before merge.
1. Duplicate eviction logic across two components
evictDataSourceConnections is copy-pasted verbatim between JdbcComponent and SqlComponent — same code, same comments, same log messages, even the same // camel-jdbc does not need comment in SqlComponent. Any future fix (a new pool vendor, a behaviour change, a log-level adjustment) will need to be applied twice. Move it to a shared utility in camel-support or camel-core-engine, or at minimum extract it to a JdbcPoolEvictionSupport helper in one of the two modules and have the other delegate to it.
2. this.dataSource may already be in the registry — double-eviction possible
In onSecretRotation():
Set<DataSource> dataSources = getCamelContext().getRegistry().findByType(DataSource.class);
if (this.dataSource != null) {
dataSources.add(this.dataSource); // ← may already be in the set
}findByType scans the full registry. If the component's own dataSource was registered under any name (the typical Spring/Quarkus setup: a @Bean DataSource bound into the Camel registry and then injected into the component via @Autowired), it will already be in dataSources, and softEvictConnections() will be called on it twice. LinkedHashSet.add() does deduplicate by identity, so this is only safe if both references are the exact same object — which they are in the common case, but relies on identity equality rather than explicit logic.
Use an explicit identity-deduplicated set:
Set<DataSource> dataSources = Collections.newSetFromMap(new IdentityHashMap<>());
dataSources.addAll(getCamelContext().getRegistry().findByType(DataSource.class));
if (this.dataSource != null) {
dataSources.add(this.dataSource);
}This makes the deduplication explicit and immune to DataSource implementations that override equals/hashCode in unexpected ways.
Confirmed OK
- HikariCP API:
getHikariPoolMXBean()is a public method onHikariDataSourcethat returnsHikariPoolMXBean;softEvictConnections()is declared on the interface and implemented byHikariPool. The two-step reflection chain is correct. Verified against HikariCP 5.0.1 bytecode (javap).apupier's original concern about the method location was valid for the previous commit; the current commit is correct. findByTypemutability:SimpleRegistry.findByType()returns a freshLinkedHashSet— mutation via.add()is safe.- Static analysis: semgrep and ast-grep found no issues in the production files.
- Pre-fix test validation:
⚠️ dynamic check skipped — no worktree available for this PR. Static trace: the tests use hand-rolled stubs that accurately mirror the real HikariCP API shape (two-step indirection via MXBean), so the test coverage is structurally sound. Adding a real HikariCPtest-scope dependency (asapupiersuggested) would make this watertight, but is not a blocker given the accurate stub design.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
@apupier — addressed in c1aa008. You're right that // Step 1: getHikariPoolMXBean() — public method on HikariDataSource
Method getPoolMXBean = ds.getClass().getMethod("getHikariPoolMXBean");
Object poolMXBean = getPoolMXBean.invoke(ds);
// Step 2: softEvictConnections() — declared on HikariPoolMXBean, implemented by HikariPool
Method softEvict = poolMXBean.getClass().getMethod("softEvictConnections");
softEvict.invoke(poolMXBean);Both steps use reflection so there's no compile-time dependency on HikariCP. The eviction logic has also been extracted to |
- Extract eviction logic from JdbcComponent/SqlComponent into a shared DataSourceHelper utility class in camel-support, eliminating the duplicate implementation reported in the review. - Fix potential double-eviction by replacing findByType() result (a plain HashSet) + manual add with Collections.newSetFromMap(new IdentityHashMap<>()) so that identity-equality is used for dedup. A DataSource wrapper that delegates equals/hashCode to the wrapped instance could previously cause the same pool to be evicted twice. - Update tests to call DataSourceHelper.evictDataSourceConnections() directly.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: CAMEL-24638 — SecretRotationAware for camel-jdbc / camel-sql
Checking the previous two findings against the new commit.
Issue 2 (IdentityHashMap for deduplication) — ✅ Fully addressed. Collections.newSetFromMap(new IdentityHashMap<>()) is used in both components, and the comment explains the rationale correctly.
Issue 1 (duplicate eviction logic) — evictDataSourceConnections() was correctly extracted to DataSourceHelper, which removes the HikariCP reflection code from the components. But the onSecretRotation() method body is still copy-pasted word-for-word between JdbcComponent and SqlComponent — including the three-line comment block and the IdentityHashMap orchestration. Any future change (new pool vendor, different fallback strategy, log-level adjustment) still needs to be applied to both files.
One additional nit: JdbcComponentSecretRotationAwareTest.evictDataSourceConnections_genericPool_doesNotThrow() declares throws Exception, but DataSourceHelper.evictDataSourceConnections() does not throw a checked exception — this is a false throws clause that misleads readers.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
CAMEL-24638 asks us to refresh the credentials used by the pool and evict old connections. Where does this patch update the existing DataSource with the rotated credential? softEvictConnections() alone can create replacement connections with the original password. Does this also need to support Agroal? DataSourceHelper only recognizes Hikari’s MXBean. An Agroal pool reaches the logging fallback without a flush. Agroal provides flush(FlushMode.GRACEFUL), while its default configuration does not guarantee replacement through borrow validation or maximum lifetime. Please cover the intended pool types with a test that rotates the credential and verifies a subsequent connection authenticates with it. AI-generated by Codex on behalf of @Croway. |
…er; fix LOG access; fix spurious throws - Add DataSourceHelper.evictAllDataSourceConnections(Registry, DataSource, Object) which centralises the IdentityHashMap deduplication + registry scan + component data-source merging logic so both components delegate with a one-liner. - Add a private LOG field to JdbcComponent to fix the compile error (DefaultComponent.LOG is private; the pre-existing createEndpoint() LOG.debug() call could not access it). - Simplify JdbcComponent.onSecretRotation() and SqlComponent.onSecretRotation() to delegate to DataSourceHelper.evictAllDataSourceConnections(). - Remove now-unused imports (Collections, IdentityHashMap, Set) from both components. - Drop spurious 'throws Exception' from JdbcComponentSecretRotationAwareTest .evictDataSourceConnections_genericPool_doesNotThrow() — DataSourceHelper .evictDataSourceConnections() does not throw a checked exception.
|
Could you explain why Please also add a real rotation test using PostgreSQL with password authentication:
Run this scenario for each pool the PR claims to support. What behavior should the reload report if the new credentials cannot establish a connection? The current fallback only logs and continues. AI-generated by Codex on behalf of @Croway. |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: CAMEL-24638 — SecretRotationAware for camel-jdbc / camel-sql
All three findings from the previous review are addressed. One new compile-breaking bug was introduced by the import cleanup.
Previous findings — status
Issue 1 (duplicate onSecretRotation() orchestration) — ✅ Fully resolved. DataSourceHelper.evictAllDataSourceConnections(Registry, DataSource, Object) is now the single implementation; both components delegate with a one-liner. The method is well-documented and has identity-based deduplication in the right place.
Issue 2 (potential double-eviction / IdentityHashMap) — ✅ Fully resolved. Centralised in DataSourceHelper, explicit identity-based dedup with a clear comment explaining why.
Nit (spurious throws Exception on _genericPool_doesNotThrow) — ✅ Fixed. evictDataSourceConnections_genericPool_doesNotThrow() no longer declares throws Exception.
New finding — compile failure (breaks CI)
See inline comment.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
Fixed in bc71bd1: the test failures were caused by |
|
@Croway Good questions — let me clarify the design. Credential update: the credentials are updated before
So Agroal: fair point. This PR starts with HikariCP support (by far the most common pool in Camel deployments) and uses a graceful fallback for unknown pools — log and let connections expire naturally. Adding Agroal Integration test with real rotation: that would be valuable, but it requires a database container (Testcontainers + PostgreSQL), which is a significant test-infra addition for what is fundamentally a connection-eviction callback. The unit tests verify the reflection chain works correctly. A full end-to-end rotation test could be added as a follow-up. |
|
Why scan all DataSources: the As for ownership: DBCP2: you are right that DBCP2 does not pick up password changes after initialization. That is a DBCP2 limitation, not something this PR introduces — the fallback path logs that the pool does not support explicit eviction and relies on natural connection expiry + validation. If we want to support DBCP2 specifically, we would need to call Full rotation test: see the reply above — happy to add it as a follow-up JIRA, but it is a significant test-infra addition (Testcontainers + PostgreSQL + multiple pool vendors) that should not gate this PR. |
|
Thanks @davsclaus — both points addressed in d711425:
CI is green ✅ |
|
Both test points addressed in d711425:
|
|
Thanks @davsclaus — both test points fixed in d711425:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, the HikariCP test is now meaningful. With maxPoolSize=1, it compares the physical connection (via unwrap(Connection.class)) before and after eviction, and the not-started case checks for a null pool MXBean. LGTM.
Claude Code on behalf of davsclaus
|
All review feedback addressed — davsclaus approved (LGTM), gnodet-bot approved, apupier's initial concern (MXBean indirection) was resolved. CI green. Ready for merge. @apupier could you re-review or approve? Your Sep 24 CHANGES_REQUESTED review (about |
apupier
left a comment
apupier
left a comment
There was a problem hiding this comment.
request change for documentation part
comments:
I think the test should really be closer to real use case given the complexity of all the parts together. Here there is the eviction which is called directly on the datasource.
There is no credentials really rotated.
Note that it will be worthy to create a ticket on Camel Quarkus side to mention all the classes and methods used by reflection. They will need to be registered (or at least documented given htat we do not provide Hikari by default) to make it working in native mode.
|
@apupier Thanks for the review. Documentation (registerMbeans): Replied inline — Tests closer to real use case: The latest commit ( Camel Quarkus native reflection registration: Good point — I'll open a follow-up ticket on camel-quarkus. The classes used by reflection are |
…ive mode - Clarify that HikariCP eviction uses direct reflection on getHikariPoolMXBean(), not JMX — registerMbeans=true is not required. - Add note about GraalVM native mode: reflection registration needed for HikariDataSource/HikariPoolMXBean classes and methods. - Applied to both jdbc-component.adoc and sql-component.adoc (source + catalog copies).
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: CAMEL-24638 — SecretRotationAware for camel-jdbc / camel-sql
Checking the two new commits since last APPROVE (b796416).
Previous findings — all confirmed addressed
No outstanding findings remain from any reviewer (apupier, gnodet-bot ×4, davsclaus, Croway, oscerd).
New commits
d711425 — Fix HikariCP integration test — ✅
- Started-pool test now uses
maxPoolSize=1+minimumIdle=1and compares physical connection identity viaconn.unwrap(Connection.class)+assertNotSame. This proves eviction actually replaced the connection, not just that the call didn't throw. Correct. - Not-started-pool test now uses the no-arg
HikariDataSource()constructor + setters (true lazy init, pool only starts on firstgetConnection()), withassertNull(ds.getHikariPoolMXBean())confirming the MXBean is genuinely null. Correct — the config-based constructor (new HikariDataSource(config)) eagerly initializes the pool, which defeated the purpose of the test.
79ad28d — Clarify JMX not required, document reflection for native mode — ✅
- "JMX registration (
registerMbeans=true) is not required" — correct.getHikariPoolMXBean()is a direct method onHikariDataSourcethat returns the internal pool object;registerMbeansonly controls whether the MBean is registered with the JMXMBeanServerfor remote monitoring. - GraalVM native reflection NOTE — correct and useful. The two-step reflection chain (
getHikariPoolMXBean()+softEvictConnections()) would needreflect-config.jsonentries in native mode. - All four doc copies (jdbc source, sql source, jdbc catalog, sql catalog) updated consistently.
Verdict
Docs and test improvements only, no production code changes. All technically accurate, all addressing davsclaus's review feedback. CI is queued (just pushed) — previous CI run was green.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: CAMEL-24638 — Agroal pool eviction support (ae4aa49)
New commit adds Agroal flush(GRACEFUL) detection via reflection and an integration test. The code is correct — but the documentation now contradicts the implementation.
New finding — stale documentation (4 files)
The .adoc docs in all four copies still say:
Currently only HikariCP pools are supported for active eviction
Other pool implementations (including Quarkus Agroal) are not actively evicted
This is now false — DataSourceHelper.evictDataSourceConnections() actively supports Agroal via flush(GRACEFUL). The Javadoc on DataSourceHelper was correctly updated, but the user-facing documentation was not.
See inline comment on jdbc-component.adoc — same fix needed in the other 3 copies.
Confirmed OK
- Agroal reflection approach: The
flush(FlushMode)detection via method-name + enum-type check is robust and won't false-positive on unrelatedflushmethods. - Test coverage: Integration test exercises both started and smoke paths against a real
AgroalDataSource+ h2. - ast-grep:
catch (Exception e)on the new Agroal block (line 152) — same intentional pattern as the existing HikariCP block. Acceptable for reflection error handling. - Test dependency:
agroal-api+agroal-pooladded as test-scope only incamel-sql— no production dependency footprint.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| Currently only HikariCP pools are supported for active eviction via `softEvictConnections()`. | ||
| The eviction is triggered by calling `getHikariPoolMXBean()` directly on the `HikariDataSource` instance | ||
| via reflection — JMX registration (`registerMbeans=true`) is *not* required. | ||
| Other pool implementations (including Quarkus Agroal) are not actively evicted — existing connections | ||
| will be replaced as they expire or are validated by the pool. |
There was a problem hiding this comment.
DataSourceHelper, which actively supports Agroal via flush(GRACEFUL).
Update to reflect both supported pools:
| Currently only HikariCP pools are supported for active eviction via `softEvictConnections()`. | |
| The eviction is triggered by calling `getHikariPoolMXBean()` directly on the `HikariDataSource` instance | |
| via reflection — JMX registration (`registerMbeans=true`) is *not* required. | |
| Other pool implementations (including Quarkus Agroal) are not actively evicted — existing connections | |
| will be replaced as they expire or are validated by the pool. | |
| Currently HikariCP and Agroal (Quarkus default) pools are supported for active eviction. | |
| HikariCP pools are evicted via `softEvictConnections()` called through `getHikariPoolMXBean()` | |
| via reflection — JMX registration (`registerMbeans=true`) is *not* required. | |
| Agroal pools are evicted via `flush(GRACEFUL)` called on `AgroalDataSource` via reflection. | |
| Other pool implementations are not actively evicted — existing connections |
The same fix is needed in sql-component.adoc (both source and catalog copies) and catalog/.../jdbc-component.adoc.
Also update the NOTE block below to mention the Agroal reflection classes that need GraalVM native registration (AgroalDataSource.flush, AgroalDataSource.FlushMode).
There was a problem hiding this comment.
Fixed in 2b58bfc. Updated all four .adoc files (jdbc/sql source + catalog) to reflect both HikariCP and Agroal support, and updated the GraalVM native reflection NOTE to include Agroal classes.
The Secret Rotation docs in all four .adoc files (jdbc/sql source + catalog) still stated that only HikariCP was supported for active eviction and that Agroal was not. This contradicts the implementation added in ae4aa49 which actively supports Agroal via flush(GRACEFUL). Updated docs to accurately describe both supported pools (HikariCP and Agroal) and updated the GraalVM native reflection NOTE to include Agroal classes.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: CAMEL-24638 — SecretRotationAware for camel-jdbc / camel-sql
The previous review (on ae4aa49) noted one outstanding issue: the .adoc documentation for both components still described only HikariCP support, despite the Agroal eviction code having been added. Commit 2b58bfc addresses this:
- All four doc files (jdbc/sql source + catalog copies) now correctly list both HikariCP and Agroal as supported pools
- The GraalVM native reflection registration note includes
AgroalDataSource.flush(FlushMode) - The wording is consistent across all copies
All prior findings from previous reviews (gnodet-bot ×4, apupier, davsclaus, Croway) remain addressed. No regressions.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
The approval still holds. The Agroal eviction via flush(FlushMode.GRACEFUL) is reasonable scope given Quarkus uses Agroal, and the docs are updated in all copies. One test-quality request inline, and one question:
DataSourceHelperresolvesflushviads.getClass().getMethods(). If the DataSource is a non-public wrapper or proxy class,invokecould fail withIllegalAccessException(caught and logged as WARN). Resolvingflushfrom the publicAgroalDataSourceinterface would be more robust. Not blocking.
Note that @apupier's change request is still open (a test that actually rotates credentials, and a Camel Quarkus ticket).
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| } | ||
|
|
||
| // Eviction must succeed — real AgroalDataSource.flush(GRACEFUL) via reflection | ||
| assertDoesNotThrow(() -> DataSourceHelper.evictDataSourceConnections(ds, "test-rotation")); |
There was a problem hiding this comment.
evictDataSourceConnections swallows every exception, so assertDoesNotThrow passes even if the flush detection broke and the helper fell through to the generic log (same for line 70). Same point as for the Hikari test earlier. Suggest maxSize(1) and comparing the physical connection before and after, or metricsEnabled(true) and asserting ds.getMetrics().flushCount() > 0.
There was a problem hiding this comment.
Fixed in beb6afe. The Agroal test now enables metricsEnabled() and asserts flushCount() > 0 after eviction, and uses maxSize(1) + assertNotSame on the physical connection (via unwrap(Connection.class)) to prove the pool replaced the evicted connection — same approach as the HikariCP test.
There was a problem hiding this comment.
Addressed in 673fa2b. flush(FlushMode) is now resolved through the public AgroalDataSource interface via Class.forName("io.agroal.api.AgroalDataSource") + isInstance(ds), instead of ds.getClass().getMethods(). This handles proxy/wrapper classes correctly.
Use metricsEnabled() + flushCount() to verify flush(GRACEFUL) was actually called via reflection, and compare physical connections before/after eviction (maxSize=1) to prove the pool replaced the evicted connection. Same approach as the HikariCP integration test.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: CAMEL-24638 — SecretRotationAware for camel-jdbc / camel-sql
Single new commit (beb6afe) since the last review: strengthens the Agroal integration test to match the HikariCP test's rigour — metricsEnabled() + flushCount() verification, maxSize(1) forced slot recycling, physical connection identity comparison via unwrap() + assertNotSame. Correct and consistent.
All previous findings (gnodet-bot ×4, apupier, davsclaus, Croway) remain fully addressed. No new issues.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, the Agroal test now really checks the eviction (maxSize(1), flushCount() and a different physical connection). My approval still holds, with one fix requested inline.
Still open from before (non-blocking): resolving flush through the public AgroalDataSource interface rather than ds.getClass().getMethods(). Note that @apupier's review still asks for a test that actually rotates credentials and a Camel Quarkus ticket.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
|
||
| // GRACEFUL flush marks idle connections for eviction asynchronously; | ||
| // allow a short window for Agroal's housekeeping to destroy them | ||
| Thread.sleep(500); |
There was a problem hiding this comment.
Please don't use Thread.sleep in tests (project rule). It's also a flakiness risk: flushPool hands a FlushTask to the housekeeping executor, so the flush is async, and if 500ms isn't enough the next getConnection() can still return the old physical connection. Suggest await().atMost(5, SECONDS).until(() -> metrics.flushCount() > flushCountBefore) instead (Awaitility is already used in camel-sql tests).
There was a problem hiding this comment.
Fixed in a54a9dc. Replaced Thread.sleep(500) with await().atMost(5, SECONDS).untilAsserted() — properly handles the async nature of Agroal's flush(GRACEFUL) and complies with the project's no-Thread.sleep rule.
Replace Thread.sleep(500) with Awaitility's await().atMost(5, SECONDS) .untilAsserted() to properly handle Agroal's async flush(GRACEFUL) eviction. Avoids flakiness and complies with the project's no-Thread.sleep-in-tests rule.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: CAMEL-24638 — SecretRotationAware for camel-jdbc / camel-sql
New commit a54a9dcf replaces Thread.sleep(500) with Awaitility.await().atMost(5, TimeUnit.SECONDS) in DataSourceHelperAgroalIntegrationTest. Correct fix — Agroal's flush(GRACEFUL) delegates to the housekeeping executor asynchronously, so a fixed sleep was both fragile and non-idiomatic. The Awaitility approach is more robust and consistent with the project's testing conventions.
No production code changes. All previous findings remain addressed.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Resolves flush(FlushMode) from the public AgroalDataSource interface instead of ds.getClass().getMethods(). This is more robust when the DataSource is a proxy or non-public wrapper class, since the method is always visible on the public interface. Also adds an explicit Class.forName check so we skip the iteration entirely when Agroal is not on the classpath.
|
@apupier Addressing your remaining review points:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: CAMEL-24638 — SecretRotationAware for camel-jdbc / camel-sql
One new commit since the previous APPROVE (673fa2b8): resolves Agroal flush(FlushMode) through the public AgroalDataSource interface instead of ds.getClass().getMethods().
Change analysis
The Agroal detection now uses Class.forName("io.agroal.api.AgroalDataSource") → isInstance(ds) → resolve flush from agroalDsClass.getMethods() rather than scanning the concrete class's methods directly. This is strictly better:
- Works correctly with proxied or wrapped
AgroalDataSourceinstances (e.g. Quarkus CDI proxies) - Adds proper
ClassNotFoundExceptionhandling for the Agroal-not-on-classpath case - No change to the HikariCP path or any other production code
All previous findings (gnodet-bot ×4, apupier, davsclaus, Croway) remain addressed. CI green. No issues.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, the Awaitility change and resolving flush through the AgroalDataSource interface both look good. Two small, non-blocking points inline.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| // Resolved through the public AgroalDataSource interface (not ds.getClass()) so that proxy | ||
| // or wrapper classes work correctly. No compile-time dependency on agroal-api. | ||
| try { | ||
| Class<?> agroalDsClass = Class.forName("io.agroal.api.AgroalDataSource"); |
There was a problem hiding this comment.
Class.forName(String) resolves against camel-support's own classloader. If Agroal lives in a different (child) classloader than camel-support, this throws ClassNotFoundException and silently falls through to the generic fallback. Resolving it via the DataSource's classloader would be sturdier:
Class<?> agroalDsClass = Class.forName("io.agroal.api.AgroalDataSource", false, ds.getClass().getClassLoader());There was a problem hiding this comment.
Fixed in 7ed1cba. Now using Class.forName("io.agroal.api.AgroalDataSource", false, ds.getClass().getClassLoader()) to resolve against the DataSource's own classloader.
|
|
||
| // GRACEFUL flush hands a FlushTask to the housekeeping executor, so the | ||
| // actual eviction is async. Use Awaitility instead of Thread.sleep to | ||
| // avoid flakiness and comply with the project's no-Thread.sleep rule. |
There was a problem hiding this comment.
Nit: the reference to the project's no-Thread.sleep rule reads a bit odd in source; the first sentence (flush is async) is enough on its own.
There was a problem hiding this comment.
Fixed in 7ed1cba. Trimmed the comment to just the technical reason (flush is async).
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review: CAMEL-24638 — SecretRotationAware for camel-jdbc / camel-sql
Delta since last review (673fa2b → 7ed1cba7): two micro-changes, both correct.
-
Agroal classloader fix —
Class.forName("io.agroal.api.AgroalDataSource", false, ds.getClass().getClassLoader())instead of the no-arg overload. Correct: in OSGi/modular environments (Karaf, Quarkus dev-mode class isolation), the DataSource's classloader is the right one to resolve Agroal against. Thefalse(no-initialize) parameter is also appropriate since we only need theClassobject forisInstance+ method reflection, not static initialization. -
Comment trim — removed verbose Awaitility justification comment in the Agroal integration test. No logic change.
All previous findings remain addressed. CI pending on this push but the changes are trivially safe.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
apupier
left a comment
apupier
left a comment
There was a problem hiding this comment.
Tests closer to real use case: the current tests use real HikariCP and Agroal pools against h2 and verify that eviction actually replaces physical connections (via unwrap(Connection.class) + assertNotSame). A full credential rotation integration test (start pool with password A → rotate to password B → verify new connections use B) would require either a PostgreSQL container in CI or a custom DataSource that reads credentials dynamically — both of which would significantly increase the test surface and CI time. This could be a follow-up, similar to the Agroal support which was also added incrementally. The current tests cover the reflection path and the eviction behavior, which is the core of this PR.
please create a specific ticket for that because for now we have no proof that it is working end to end and I guess you have not even tested it manually locally
|
@apupier Created CAMEL-25149 to track the end-to-end credential rotation integration test (with Testcontainers + PostgreSQL, covering both HikariCP and Agroal). The Camel Quarkus ticket for native reflection registration was already created: camel-quarkus#9252. Could you re-review or clear the changes-requested status? The documentation concern (JMX/registerMbeans) was addressed — the docs now clarify that |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Guillaume. Both points from my last round are addressed: Agroal is now resolved through the DataSource's own classloader, and the test comment is trimmed. With the earlier rounds (eviction scoped to component/endpoint DataSources, documented credential limitation, real HikariCP and Agroal integration tests) this looks good to me. The end-to-end rotation test is tracked in CAMEL-25149.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
6 participants
Summary
Implements
SecretRotationAwareinJdbcComponent(camel-jdbc) andSqlComponent(camel-sql) so that JDBC connection pools evict stale connections when vault-backed database credentials are rotated.Root Cause
JdbcComponentandSqlComponentboth use aDataSourcethat is typically a registry bean (HikariCP pool, DBCP2 pool, etc.). This bean is not recreated during a vault-triggered context reload:DefaultContextReloadStrategy.reloadAllRoutes()clears the endpoint registry and restarts routes, but the DataSource bean persists with connections authenticated against the old password. New connections checked out from the pool after rotation continue to use the old credentials until the pool discards them naturally (e.g. on expiry or validation failure).Fix
Both components now implement
SecretRotationAware. TheonSecretRotation()callback:dataSourcefield (if set) plus DataSources held by its active endpoints (e.g.jdbc:myDs,sql:...?dataSource=#myDs), with identity-based deduplication so each pool is evicted at most once.softEvictConnections()via theHikariPoolMXBean(retrieved through reflection) — this marks existing connections for eviction while allowing in-flight queries to complete.The shared eviction logic lives in
DataSourceHelper(camel-support) — a reflection-only utility with no vendor dependency. Components pass aFunction<Endpoint, DataSource>extractor so the helper stays generic.Important limitation: eviction only closes existing connections — it does not update the pool's credentials. For pools configured with a static password (e.g. Spring Boot
spring.datasource.password), the pool will re-open connections using the old credentials. This feature works out of the box only with pools that resolve credentials dynamically (e.g.HikariCredentialsProvider, the AWS JDBC wrapper secrets plugin). Quarkus uses Agroal by default, not HikariCP, so this eviction does not apply there.Tests
JdbcComponentSecretRotationAwareTest(4 tests) — component-level: interface assertion, component-owned DataSource eviction, endpoint DataSource eviction (jdbc:myDscase), no DataSource configured (no throw).SqlComponentSecretRotationAwareTest(6 tests) — same as above, plusDataSourceHelper-level stub tests.DataSourceHelperHikariIntegrationTest(2 tests) — real HikariCP + h2: started-pool eviction and not-started-pool (null MXBean) handling.Related
SecretRotationAwareSPI added to camel-coreHermes Agent (Claude Sonnet 4.6) on behalf of Guillaume Nodet