CAMEL-24531: extract a shared early-resolution properties parser - #1917
CAMEL-24531: extract a shared early-resolution properties parser#1917ammachado wants to merge 12 commits into
Conversation
davsclaus
left a comment
There was a problem hiding this comment.
Nice extraction. I diffed the migrated SpringBootAwsSecretsManagerPropertiesParser, SpringBootGoogleSecretManagerPropertiesParser, SpringBootHashicorpVaultPropertiesParser, and IBMSecretsManagerVaultPropertiesParser line-by-line against their pre-refactor versions: the shared AbstractEarlyResolutionPropertiesParser faithfully reproduces the original per-parser control flow, and the three called-out behavior changes (exception normalization, blank-value rejection for the Hashicorp settings, exact delimiter stripping instead of a global replace) are each covered by a dedicated test in AbstractEarlyResolutionPropertiesParserTest. The asymmetric overridden-ibm-secrets-manager-properties name is preserved exactly as claimed. CI is green across all jobs.
One thing worth flagging before merge, not a defect in this PR by itself: PR #1907 (CAMEL-24532) is open concurrently and touches the same 7 parser files to fix the property-source-precedence bug (duplicate keys across sources currently resolve last-write-wins instead of highest-precedence-wins). This PR is built on the pre-#1907 version of those files and, per its own description, intentionally keeps the old last-write-wins semantics (props.put(key, resolved) in the new shared base class) so that CAMEL-24532 can be handled separately — that's a sensible division per the linked JIRA tickets, but it means whichever of the two PRs merges second will need real rework rather than a routine rebase: if this PR merges first, #1907's fix has no per-parser loop left to attach its helper calls to and will need to move its precedence check into AbstractEarlyResolutionPropertiesParser's shared loop; if #1907 merges first, this branch would silently reintroduce the last-write-wins bug into the new shared class unless the precedence fix is carried forward manually. Worth deciding merge order and making sure whichever merges second reconciles with the other explicitly.
This review does not replace CodeRabbit/Sourcery/SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
…IOException handling
…uration exceptions
06b9c7d to
0bdcb78
Compare
What this does
Seven vault and secrets starters each carried a near-identical
ApplicationListener<ApplicationEnvironmentPreparedEvent>that resolved{{<prefix>:...}}placeholders before theApplicationContextexists. The bodies were roughly 108 lines each and differed only in the guard property, the override property source name, and how thePropertiesFunctionis constructed.This extracts the shared lifecycle into
AbstractEarlyResolutionPropertiesParserincore/camel-spring-bootand reduces each starter to three small overrides.SpringBootAwsSecretsManagerPropertiesParsercamel.component.aws-secrets-manager.early-resolve-propertiesSpringBootAzureKeyVaultPropertiesParsercamel.component.azure-key-vault.early-resolve-propertiesSpringBootCyberArkVaultPropertiesParsercamel.component.cyberark-vault.early-resolve-propertiesSpringBootGoogleSecretManagerPropertiesParsercamel.component.google-secret-manager.early-resolve-propertiesSpringBootHashicorpVaultPropertiesParsercamel.component.hashicorp-vault.early-resolve-propertiesIBMSecretsManagerVaultPropertiesParsercamel.component.ibm-secrets-manager.early-resolve-propertiesSpringBootCloudConfigPropertiesParsercamel.component.spring-cloud-config.early-resolve-propertiesThe base class declares three abstract methods (
getEarlyResolutionProperty(),getOverridePropertySourceName(),createPropertiesFunction(ConfigurableEnvironment)), one overridablegetSourceDescription(), and afinalonApplicationEvent.Every override property source name is preserved byte for byte, including
camel-ibm-secrets-manager-starter's asymmetricoverridden-ibm-secrets-manager-properties, which lacks thecamel-segment the other six use. That asymmetry is pre-existing and renaming it would break anyone looking that source up by name.Behaviour changes
Most of this is a pure extraction, but three things do change. They are called out here so they can be reviewed as decisions rather than discovered as surprises.
1. Exception normalization. Two starters previously threw exception types that signal a programming defect rather than an operator configuration error, and that slip past any
catch (RuntimeCamelException)handler.RuntimeExceptionwrappingIOExceptionRuntimeCamelException, cause chainedNullPointerExceptionviaObjects.requireNonNullRuntimeCamelException, original message plus the property keyNumberFormatExceptionnaming no propertyRuntimeCamelExceptionnamingcamel.vault.hashicorp.port, cause chainedNo message became less specific.
Objects.requireNonNull's messages were already good (for example "Hashicorp Vault token is required") and they survive verbatim with the property key appended. The port case is the one that was genuinely unhelpful before, sinceInteger.parseIntreportsFor input string: "..."without ever naming the property the operator got wrong.2. Blank values are now rejected for the four Hashicorp settings. The new
required()helper usesObjectHelper.isEmpty, socamel.vault.hashicorp.port=now fails with "port is required (set camel.vault.hashicorp.port)" instead of reachingInteger.parseIntand dying on an empty string. An empty scheme now fails in the parser rather than deeper insideVaultEndpoint.3. Placeholder unwrapping is now exact. The old code did
value.replace("{{aws:", "").replace("}}", ""), a global replace that would corrupt a secret path legitimately containing}}. The shared version usessubstringagainst the known delimiter lengths. There is a test pinning this:{{test:a}}b}}must yield the remaindera}}b, which the old code turned into the empty string.A related simplification: the placeholder prefix is no longer hardcoded per starter.
PropertiesFunction.getName()already returns exactly the prefix token (aws,azure,gcp, and so on), so the base class derives"{{" + fn.getName() + ":"itself. The prefix can no longer drift out of sync with the function that resolves it.Deliberately out of scope
uri={{aws:user}}:{{aws:pass}}satisfies bothstartsWithandendsWith, so it is unwrapped into a garbage remainder and fails. This behaviour is unchanged from before this PR and from before CAMEL-24508: vault and secrets starters - fail closed when early property resolution fails #1900, so it is not a regression, but the class javadoc now documents the limitation instead of claiming such values are left alone. Worth its own ticket.SystemEnvironmentPropertySourceextendsMapPropertySource, so a placeholder inMY_SECRETis resolved and stored under the literal keyMY_SECRET, while Spring's relaxed binding then resolvesmy.secretfrom the environment source rather than the exact-match override source. The unresolved placeholder stays effective with no error. This arrives from CAMEL-24508: vault and secrets starters - fail closed when early property resolution fails #1900 unchanged by this refactor and fixing it means changing property source semantics. Flagging it because it is security relevant and deserves its own ticket.Testing
mvn -o verifypasses in all 8 affected modules with zero failures and zero errors.core/camel-spring-boot: 156 tests plus 2 integration tests. 11 of those are new, covering the shared class: the guard, flag-read ordering, whole-value matching,OriginTrackedValueunwrapping, exact delimiter stripping, failure aggregation with causes attached viaaddSuppressed, no override source registered on failure, tolerant mode, and a null resolution result.camel-hashicorp-vault-starter'sEarlyResolvedPropertiesTestran end to end against a real Testcontainers Vault and passed.Two coverage gaps worth stating plainly rather than leaving to be found:
IOExceptiontoRuntimeCamelExceptionchange has no test. ForcingSecretManagerServiceClient.createto throw requires credential environment manipulation, and the alternatives drag live GCP into a unit test. Verified by inspection.EarlyResolvedPropertiesTestclasses are guarded by@EnabledIfSystemPropertyand@EnabledIfEnvironmentVariableand skip without live cloud credentials. That is pre-existing. The behavioural safety net for this refactor is the shared class's own test suite incore/camel-spring-boot, which runs everywhere.No new dependencies, no POM changes, and therefore no regenerated code. 16 files changed.
Note on sequencing
This originally stacked on #1900. That PR has since merged, and this branch has been rebased directly onto
main, so it no longer depends on anything unmerged.Claude Code on behalf of Adriano Machado.