CAMEL-24341: camel-google-secret-manager - fix GCP vault refresh task defects - #25326
Conversation
… defects The GCP secret refresh task read the AWS vault configuration, so camel.vault.gcp.secrets was never honoured. It also kept triggerReloading as receiver state, so every message following a matching secret event triggered another CamelContext reload, restarted the subscriber on every period (a Google ApiService can only be started while it is NEW), and dereferenced the secretId/eventType attributes without a null check, so a message published on the subscription by anything else failed and was redelivered forever. Also aligns the javadoc of the task and of the properties function with GCP and reuses a single ObjectMapper for secret sub-key lookups. Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
davsclaus
left a comment
There was a problem hiding this comment.
All four bug fixes are correct and well-diagnosed:
- GCP→AWS secrets read —
aws().getSecrets()was clearly wrong; confirmed by comparing with the AWSCloudTrailReloadTriggerTaskwhich correctly readsaws().getSecrets()for its own config. triggerReloadingas local variable — matches the pattern already used inCloudTrailReloadTriggerTask(line 271). The old field-level flag caused every subsequent message to trigger a reload after the first match.- Subscriber started once —
ApiService.State.NEWcheck is the correct guard; a GoogleApiServicethrowsIllegalStateExceptionif started again afterRUNNING. - Null-safe attributes — constant-first
equalsIgnoreCaseplus explicitsecretId != nullprevents NPE on foreign messages.
Static ObjectMapper in GoogleSecretManagerPropertiesFunction is thread-safe for readTree() — good cleanup.
Minor convention notes (non-blocking):
- Test class and methods use
public— JUnit 5 convention in this project is package-private (no modifier). - Tests use JUnit assertions (
assertEquals,assertTrue, etc.) — project prefers AssertJ (assertThat(...)).
These are small style items and don't block the PR.
This review was generated by an AI agent (Claude Code on behalf of davsclaus) and may contain inaccuracies. Please verify all suggestions before applying.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
…tions Per davsclaus's non-blocking note, PubsubReloadTriggerTaskTest now uses package-private class/@test visibility and AssertJ assertions (assertThat, assertThatThrownBy, assertThatCode(...).doesNotThrowAnyException()). The interface-override methods (onReload/ack/nack) stay public since they override public API. Adds a test-scoped assertj-core dependency, not previously on the module's test classpath. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 29 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
…x GCP vault refresh task defects (#25339) CAMEL-24341: camel-google-secret-manager - fix GCP vault refresh task defects (#25326) * CAMEL-24341: camel-google-secret-manager - fix GCP vault refresh task defects The GCP secret refresh task read the AWS vault configuration, so camel.vault.gcp.secrets was never honoured. It also kept triggerReloading as receiver state, so every message following a matching secret event triggered another CamelContext reload, restarted the subscriber on every period (a Google ApiService can only be started while it is NEW), and dereferenced the secretId/eventType attributes without a null check, so a message published on the subscription by anything else failed and was redelivered forever. Also aligns the javadoc of the task and of the properties function with GCP and reuses a single ObjectMapper for secret sub-key lookups. * CAMEL-24341: address review - AssertJ and package-private test conventions Per davsclaus's non-blocking note, PubsubReloadTriggerTaskTest now uses package-private class/@test visibility and AssertJ assertions (assertThat, assertThatThrownBy, assertThatCode(...).doesNotThrowAnyException()). The interface-override methods (onReload/ack/nack) stay public since they override public API. Adds a test-scoped assertj-core dependency, not previously on the module's test classpath. --------- Signed-off-by: Andrea Cosentino <ancosen@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…x GCP vault refresh task defects (#25340) CAMEL-24341: camel-google-secret-manager - fix GCP vault refresh task defects (#25326) * CAMEL-24341: camel-google-secret-manager - fix GCP vault refresh task defects The GCP secret refresh task read the AWS vault configuration, so camel.vault.gcp.secrets was never honoured. It also kept triggerReloading as receiver state, so every message following a matching secret event triggered another CamelContext reload, restarted the subscriber on every period (a Google ApiService can only be started while it is NEW), and dereferenced the secretId/eventType attributes without a null check, so a message published on the subscription by anything else failed and was redelivered forever. Also aligns the javadoc of the task and of the properties function with GCP and reuses a single ObjectMapper for secret sub-key lookups. * CAMEL-24341: address review - AssertJ and package-private test conventions Per davsclaus's non-blocking note, PubsubReloadTriggerTaskTest now uses package-private class/@test visibility and AssertJ assertions (assertThat, assertThatThrownBy, assertThatCode(...).doesNotThrowAnyException()). The interface-override methods (onReload/ack/nack) stay public since they override public API. Adds a test-scoped assertj-core dependency, not previously on the module's test classpath. --------- Signed-off-by: Andrea Cosentino <ancosen@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes the defects found in
PubsubReloadTriggerTask(thegcp-secret-refreshperiod task):secretswas populated fromgetVaultConfiguration().aws().getSecrets(), so the documentedcamel.vault.gcp.secretsoption wasnever honoured, and configuring only the GCP property made
doStartfail with"Secrets must be configured on GCP vault configuration". The vault lookup now reads the GCP
configuration, and was moved ahead of the properties-function auto-detection (both are plain reads,
the "secrets must be configured" check still runs after both).
triggerReloadingwas receiver state and never reset. After the first matching secret event everysubsequent message on the subscription triggered another
CamelContextreload. It is now a localvariable, like in the AWS equivalent (
CloudTrailReloadTriggerTask).run()is invoked everycamel.vault.gcp.refreshPeriod(default 30s) and a Google
ApiServicecan only be started while it isNEW, so every tick after thefirst failed with an
IllegalStateException. The subscriber is now started once.eventType/secretIdwere dereferenced without a null check,so any message on the subscription without those attributes threw and was redelivered indefinitely.
Also in this PR: the javadoc of
setReloadEnabledsaid "on AWS secret updated", the properties functionjavadoc documented
camel.vault.aws.*instead ofcamel.vault.gcp.*, and the secret sub-key lookupallocated an
ObjectMapperper call.PubsubReloadTriggerTaskhad no test coverage at all; this addsPubsubReloadTriggerTaskTest, with onetest per defect above.
🤖 Generated with Claude Code