gnodet-bot commented on code in PR #28036:
URL: https://github.com/apache/camel/pull/28036#discussion_r4306527696
##########
components/camel-aws/camel-aws-secrets-manager/src/main/java/org/apache/camel/component/aws/secretsmanager/vault/CloudTrailReloadTriggerTask.java:
##########
@@ -273,26 +311,30 @@ public void run() {
if (!useSqsNotification) {
try {
- LookupEventsRequest.Builder eventsRequestBuilder =
LookupEventsRequest.builder()
- .maxResults(100)
-
.lookupAttributes(LookupAttribute.builder().attributeKey(LookupAttributeKey.EVENT_SOURCE)
-
.attributeValue(SECRETSMANAGER_AMAZONAWS_COM).build());
-
- if (lastTime != null) {
- eventsRequestBuilder.startTime(lastTime.plusMillis(1000));
- }
-
- LookupEventsRequest lookupEventsRequest =
eventsRequestBuilder.build();
-
- LookupEventsResponse response =
cloudTrailClient.lookupEvents(lookupEventsRequest);
- List<Event> events = response.events();
-
- if (!events.isEmpty()) {
- lastTime = events.get(0).eventTime();
+ // read every page of the update events since the last check.
LookupEvents takes a single lookup
+ // attribute, so there is one lookup per event name: filtering
on the event source instead would also
+ // return every read (GetSecretValue, DescribeSecret), and the
pages would grow with the read traffic
+ List<Event> events = new ArrayList<>();
+ for (String eventName : UPDATE_EVENT_NAMES) {
+ lookupEvents(eventName, events);
}
LOG.debug("Found {} events", events.size());
+ Instant newest = lastTime;
+ Set<String> newestEventIds = new HashSet<>();
for (Event event : events) {
+ if (lastTimeEventIds.contains(event.eventId())) {
+ // already seen by the previous check
+ continue;
+ }
+ Instant eventTime = event.eventTime();
+ if (newest == null || eventTime.isAfter(newest)) {
+ newest = eventTime;
+ newestEventIds.clear();
+ newestEventIds.add(event.eventId());
+ } else if (eventTime.equals(newest)) {
+ newestEventIds.add(event.eventId());
+ }
if
(event.eventSource().equalsIgnoreCase(SECRETSMANAGER_AMAZONAWS_COM)) {
if
(event.eventName().equalsIgnoreCase(SECRETSMANAGER_UPDATE_EVENT)
||
event.eventName().equalsIgnoreCase(SECRETSMANAGER_UPDATE_SECRET_EVENT)) {
Review Comment:
⚠️ **Dead code** — the `eventSource` and `eventName` guards here are always
true.
`lookupEvents()` already filters by `LookupAttributeKey.EVENT_NAME` with the
exact value from `UPDATE_EVENT_NAMES`, so every event that reaches this block
must already have the expected `eventName`. The `eventSource` check is the only
remaining gate — but `secretsmanager.amazonaws.com` is the only source that
emits `PutSecretValue` / `UpdateSecret`, so it is vacuously true as well.
These guards made sense when the old code filtered by `EVENT_SOURCE` and
iterated all Secrets Manager events. With the new per-event-name lookups they
are misleading — they imply the results might contain events with a different
name or source, which is no longer possible. Consider removing both conditions
and dedenting the body:
```suggestion
String secretName = event.resources().stream()
```
##########
components/camel-aws/camel-aws-secrets-manager/src/main/java/org/apache/camel/component/aws/secretsmanager/vault/CloudTrailReloadTriggerTask.java:
##########
@@ -263,6 +276,31 @@ protected void doShutdown() throws Exception {
}
updates.clear();
+ lastTime = null;
+ lastTimeEventIds.clear();
+ }
+
+ /**
+ * Adds every event with the given event name since the last check to the
list, reading all pages.
+ */
+ private void lookupEvents(String eventName, List<Event> events) {
+ String nextToken = null;
+ do {
+ LookupEventsRequest.Builder eventsRequestBuilder =
LookupEventsRequest.builder()
+ .maxResults(LOOKUP_MAX_RESULTS)
+
.lookupAttributes(LookupAttribute.builder().attributeKey(LookupAttributeKey.EVENT_NAME)
+ .attributeValue(eventName).build());
+ if (lastTime != null) {
+ // inclusive, so an event in the same second as the newest
event seen is not skipped
+ eventsRequestBuilder.startTime(lastTime);
Review Comment:
⚠️ **Fragile volatile read inside pagination loop** — `lastTime` is read as
a field on every loop iteration (line 295) rather than captured once before the
loop.
`lastTime` is declared `volatile`, which means another thread can change it
between pages of a multi-page result. If the field is ever written while a
lookup is mid-pagination (e.g. from a shutdown path that nulls it), the second
page's `startTime` will silently differ from the first page's, producing
inconsistent results or a `NullPointerException` on the builder call.
Capture it once before the loop:
```suggestion
Instant since = lastTime;
do {
```
and replace `lastTime` inside with `since`.
--
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]
