allthingssecurity commented on code in PR #28036:
URL: https://github.com/apache/camel/pull/28036#discussion_r4307676059
##########
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:
Good point, thanks. `doStop` sets `lastTime` to null, so a stop between two
pages would send the next page without a start time, and the two event-name
lookups should also share one start time. I will capture it once per check and
pass it to both lookups in a small follow-up after this is merged, so the
approved commit stays as it is.
_Claude Code on behalf of allthingssecurity_
##########
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:
Agreed, with the lookup filtered by `EVENT_NAME` both checks are always
true. I kept them as a guard in this commit, but they suggest the results could
hold other events, so I will remove them in the same follow-up as the
`lastTime` change.
_Claude Code on behalf of allthingssecurity_
--
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]
