Repository navigation
CAMEL-25729: camel-aws-secrets-manager - read every CloudTrail event since the last check in the refresh task - #28036
Conversation
…since the last check in the refresh task The CloudTrail refresh task (used when refreshEnabled=true without SQS notification) sent one LookupEvents request per check, with maxResults=100 (the service accepts at most 50), never followed nextToken, and started the next lookup one second after the newest event it had seen. The task filters on the secretsmanager.amazonaws.com event source, which includes every GetSecretValue / DescribeSecret call in the account and Region, so an update of a tracked secret that was not on the first page, or that shares its second with the newest event of the previous check, was never seen: the context was not reloaded and kept the old secret values, with nothing logged. Each check now reads every page from the newest event time seen (inclusive), 50 events per page, and skips the events of that instant that the previous check already handled, by event id (the same approach as the aws-cloudtrail consumer, CAMEL-24082). The position starts when the task starts, instead of the first check reading the newest events of the last 90 days (which could reload the context for an old update). If a page fails, the position is not moved and the next check reads the same events again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @allthingssecurity.
The bug is real, and the fix matches what CAMEL-24082 did in the aws-cloudtrail consumer: read every page from the newest time seen, inclusive, skip the event ids already handled at that instant, start at task start, and leave the position alone when a page fails. I traced the boundary handling (no new events, only events at lastTime, a newer event) and it holds, and all three tests fail on main for the reasons stated.
One design point before this goes in. The lookup still filters on EventSource=secretsmanager.amazonaws.com, so every check now pages through every GetSecretValue/DescribeSecret call in the account and Region, only to keep PutSecretValue and UpdateSecret. LookupEvents has a 2 requests/s limit shared by the whole account and Region, so with several Camel instances using the refresh task a busy account will get throttled, and since a failed check keeps the old position, the next window is larger and needs more pages. Before this change it was always one request per check.
LookupEvents can filter on EventName instead: two lookups per check, one for PutSecretValue and one for UpdateSecret (the API takes only one lookup attribute), both from the same lastTime, with the merged list going through the same newest/event-id logic unchanged. Only update events are read then, so a second page should be rare and the cost does not grow with read traffic. The eventSource check in the loop can stay as a guard. If you go this way, please update the upgrade-guide sentence about one request per 50 events.
Claude Code on behalf of Claus Ibsen
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 8 of 706 tested, 26 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (2 direct + 7 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (26)
All tested modules (35 modules, 6m 8s total)Total reactor time: 6m 8s
Top 20 slowest modules:
|
…ts in the CloudTrail refresh task Filter LookupEvents on the event names PutSecretValue and UpdateSecret (one lookup each, the API takes a single lookup attribute) instead of the Secrets Manager event source, so reads of secrets no longer add pages and a check sends about two requests whatever the read traffic. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Solid fix — the pagination bug is real, the deduplication logic is sound, and the new tests are thorough and realistic. Two points worth addressing.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
Thanks for the careful review. I went the Claude Code on behalf of allthingssecurity |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @allthingssecurity.
The earlier point is fixed in 6be0d46. Each check now sends one LookupEvents per update event name (PutSecretValue, UpdateSecret) with LookupAttributeKey.EVENT_NAME, reads every page of each from the same inclusive lastTime, and runs the merged list through the same newest-time/event-id loop. That loop finds the maximum and does not need the merged list in order, so the boundary handling I traced on the previous commit still holds. Reads of secrets no longer cost requests, and readsOfSecretsDoNotAddLookupRequests pins the two requests per check. The fake LookupEvents in the test now applies the lookup attribute and rejects more than one, which is what the service does. The upgrade-guide note was updated to match. The test class uses JUnit assertions like the other tests in the module.
On the two gnodet-bot threads, neither blocks this PR:
- The
eventSourcecheck can stay as a guard, as suggested earlier. The lookup filters on event name only, and event names are not unique across AWS services, so it is not dead code. TheeventNamecheck is now redundant but harmless. - Reading
lastTimeonce at the start ofrun()and passing it to bothlookupEventscalls would be a small cleanup. Both lookups would then use the same start by construction, not only because nothing else writes the field during a check. Optional.
As the description says, an update that CloudTrail makes visible after a newer event was already seen is still missed here. If #28038 is merged, a follow-up could give the refresh task the same late-event lookup.
Good to merge once CI is green.
Claude Code on behalf of Claus Ibsen
4 participants
Description
CAMEL-25729
With
camel.vault.aws.refreshEnabled=trueand without SQS notification,CloudTrailReloadTriggerTask.runsent oneLookupEventsrequest per check withmaxResults(100)(the API accepts 1 to 50), never readnextToken, and started the next check atlastTime + 1 s. The lookup is by event source only, so it returns every Secrets Manager call of the account and Region (for exampleGetSecretValueof other applications), newest first. An update of a tracked secret that was not on the first page, or that was recorded in the same second as the newest event of the previous check, was never looked at: the context was not reloaded and kept the old secret values, without a log line. The first check, which had no start time, looked at the newest events of the last 90 days and could reload the context for an old update.Change (in
CloudTrailReloadTriggerTask), the same approach asCloudtrailConsumer.pollsince CAMEL-24082:PutSecretValueevents and theUpdateSecretevents (one lookup per event name, as LookupEvents takes a single lookup attribute; before: every event of thesecretsmanager.amazonaws.comevent source) and reads every page from the newest event time seen, inclusive, 50 events per page;What users can notice: an update is seen whatever the number of other Secrets Manager events in the check period; the first check no longer reloads the context for an update made before the start. Upgrade guide (4.24): a note under
camel-aws-secrets-manager. Cost: two requests per check, one per event name, and one more per further 50 updates since the previous check (before: always one request, for all Secrets Manager events). Reads of secrets (GetSecretValue,DescribeSecret) are not returned and add no requests. LookupEvents allows two requests per second per account and Region; a throttled request is retried by the SDK, and a check that still fails leaves the position where it was, so the next check reads the same events again.Tests:
CloudTrailReloadTriggerTaskLookupTest(4 tests, mockedCloudTrailClientthat answers like the service: only the events that match the single lookup attribute and are at or afterstartTime, newest first, at most 50 per page, a next token while events are left). The first three fail on main, 3 of 3 runs: an update behind 60 other updates, an update in the same second as the newest event seen (also checks that a third check does not reload again), and the first check looking at an update made a day before the start.readsOfSecretsDoNotAddLookupRequests(200 reads and oneUpdateSecret) checks that a check sends exactly twoEVENT_NAMElookups; it fails with the event-source lookup (5 requests).What is not changed: an event that CloudTrail makes visible after a newer one was seen by an earlier check (delivery delay) is still missed, as in the camel-aws-cloudtrail consumer. The SQS mode is unchanged.
What was not run: the full root build; no test against AWS (what the service does with
maxResults=100was not checked; the change is right whether it rejects it or answers with 50 events).Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.(I built and tested camel-aws-secrets-manager, including the formatter and import-sort plugins. No option or generated file changed. I did not run the full root build.)
AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a
Co-Authored-Bytrailer.Claude Code on behalf of allthingssecurity
🤖 Generated with Claude Code