Skip to content

Commit 596fbc3

Browse files
gbhat618Vlatombejglick
authored
Warn about duplicated events received from GitHub via Admin Monitor (#388)
* create an admin monitor for warning about duplicate events from github * update the list of interested events and minor changes in cleanup * consider tests sending stapler currentRequest null * use mocks instead of putting null check in execution code * remove the fixed comment * track the last seen duplicate for 24 hours * Update src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DuplicateEventsSubscriber.java Co-authored-by: Vincent Latombe <[email protected]> * remove few bits to further simplify * provide a way to extract last logged event via logging in duplicates admin monitor (checking if this is the best, if yes then will write a test for it) * add a comment for why tracking the prev event inside the logger eval * log message should be clear to the user * update the admin monitor test to include duplicate event logging logic * show the payload via hyperlink not logging * mark the method with GET * use variable for shorter line * Update src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorTest.java Co-authored-by: Jesse Glick <[email protected]> * Update src/main/resources/org/jenkinsci/plugins/github/Messages.properties Co-authored-by: Jesse Glick <[email protected]> * Update src/main/resources/org/jenkinsci/plugins/github/Messages.properties Co-authored-by: Jesse Glick <[email protected]> * Update src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorTest.java Co-authored-by: Jesse Glick <[email protected]> * Update src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java Co-authored-by: Jesse Glick <[email protected]> * Update src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java Co-authored-by: Jesse Glick <[email protected]> * update for comments * add a comment and fix the javadoc * Update src/main/java/org/jenkinsci/plugins/github/extension/GHSubscriberEvent.java Co-authored-by: Jesse Glick <[email protected]> * use caffeine for cache instead of ConcurrentHashMap * add a note about cache size * move the duplicate event subscriber as an inner class into the github duplicate monitor * Update src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java Co-authored-by: Jesse Glick <[email protected]> --------- Co-authored-by: Vincent Latombe <[email protected]> Co-authored-by: Jesse Glick <[email protected]>
1 parent d2e9e60 commit 596fbc3

File tree

10 files changed

+549
-14
lines changed

10 files changed

+549
-14
lines changed

pom.xml

+5
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,11 @@
121121
<artifactId>instance-identity</artifactId>
122122
</dependency>
123123

124+
<dependency>
125+
<groupId>io.jenkins.plugins</groupId>
126+
<artifactId>caffeine-api</artifactId>
127+
</dependency>
128+
124129
<!--TEST DEPS-->
125130

126131
<!-- 4.5.3 used by rest-assured -->

src/main/java/com/cloudbees/jenkins/GitHubWebHook.java

+9-1
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,12 @@ public class GitHubWebHook implements UnprotectedRootAction {
5252
// headers used for testing the endpoint configuration
5353
public static final String URL_VALIDATION_HEADER = "X-Jenkins-Validation";
5454
public static final String X_INSTANCE_IDENTITY = "X-Instance-Identity";
55+
/**
56+
* X-GitHub-Delivery: A globally unique identifier (GUID) to identify the event.
57+
* @see <a href="https://docs.github.com/en/webhooks/webhook-events-and-payloads#delivery-headers">Delivery
58+
* headers</a>
59+
*/
60+
public static final String X_GITHUB_DELIVERY = "X-GitHub-Delivery";
5561

5662
private final transient SequentialExecutionQueue queue = new SequentialExecutionQueue(threadPoolForRemoting);
5763

@@ -117,8 +123,10 @@ public List<Item> reRegisterAllHooks() {
117123
@SuppressWarnings("unused")
118124
@RequirePostWithGHHookPayload
119125
public void doIndex(@NonNull @GHEventHeader GHEvent event, @NonNull @GHEventPayload String payload) {
126+
var currentRequest = Stapler.getCurrentRequest2();
127+
String eventGuid = currentRequest.getHeader(X_GITHUB_DELIVERY);
120128
GHSubscriberEvent subscriberEvent =
121-
new GHSubscriberEvent(SCMEvent.originOf(Stapler.getCurrentRequest2()), event, payload);
129+
new GHSubscriberEvent(eventGuid, SCMEvent.originOf(currentRequest), event, payload);
122130
from(GHEventsSubscriber.all())
123131
.filter(isInterestedIn(event))
124132
.transform(processEvent(subscriberEvent)).toList();
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,215 @@
1+
package org.jenkinsci.plugins.github.admin;
2+
3+
import java.time.Duration;
4+
import java.time.Instant;
5+
import java.util.Set;
6+
import java.util.logging.Logger;
7+
import java.util.stream.Collectors;
8+
9+
import com.github.benmanes.caffeine.cache.Cache;
10+
import com.github.benmanes.caffeine.cache.Caffeine;
11+
import com.github.benmanes.caffeine.cache.Ticker;
12+
import com.google.common.annotations.VisibleForTesting;
13+
14+
import hudson.Extension;
15+
import hudson.model.AdministrativeMonitor;
16+
import hudson.model.Item;
17+
import jenkins.model.Jenkins;
18+
import org.jenkinsci.plugins.github.Messages;
19+
import org.jenkinsci.plugins.github.extension.GHEventsSubscriber;
20+
import org.jenkinsci.plugins.github.extension.GHSubscriberEvent;
21+
import org.kohsuke.accmod.Restricted;
22+
import org.kohsuke.accmod.restrictions.NoExternalUse;
23+
import org.kohsuke.github.GHEvent;
24+
import org.kohsuke.stapler.HttpResponse;
25+
import org.kohsuke.stapler.WebMethod;
26+
import org.kohsuke.stapler.json.JsonHttpResponse;
27+
import org.kohsuke.stapler.verb.GET;
28+
import edu.umd.cs.findbugs.annotations.Nullable;
29+
import net.sf.json.JSONObject;
30+
31+
@SuppressWarnings("unused")
32+
@Extension
33+
public class GitHubDuplicateEventsMonitor extends AdministrativeMonitor {
34+
35+
@VisibleForTesting
36+
static final String LAST_DUPLICATE_CLICK_HERE_ANCHOR_ID = GitHubDuplicateEventsMonitor.class.getName()
37+
+ ".last-duplicate";
38+
39+
@Override
40+
public String getDisplayName() {
41+
return Messages.duplicate_events_administrative_monitor_displayname();
42+
}
43+
44+
public String getDescription() {
45+
return Messages.duplicate_events_administrative_monitor_description();
46+
}
47+
48+
public String getBlurb() {
49+
return Messages.duplicate_events_administrative_monitor_blurb(
50+
LAST_DUPLICATE_CLICK_HERE_ANCHOR_ID, this.getLastDuplicateUrl());
51+
}
52+
53+
@VisibleForTesting
54+
String getLastDuplicateUrl() {
55+
return this.getUrl() + "/" + "last-duplicate.json";
56+
}
57+
58+
@Override
59+
public boolean isActivated() {
60+
return DuplicateEventsSubscriber.isDuplicateEventSeen();
61+
}
62+
63+
@Override
64+
public boolean hasRequiredPermission() {
65+
return Jenkins.get().hasPermission(Jenkins.SYSTEM_READ);
66+
}
67+
68+
@Override
69+
public void checkRequiredPermission() {
70+
Jenkins.get().checkPermission(Jenkins.SYSTEM_READ);
71+
}
72+
73+
@GET
74+
@WebMethod(name = "last-duplicate.json")
75+
public HttpResponse doGetLastDuplicatePayload() {
76+
Jenkins.get().checkPermission(Jenkins.SYSTEM_READ);
77+
JSONObject data;
78+
var lastDuplicate = DuplicateEventsSubscriber.getLastDuplicate();
79+
if (lastDuplicate != null) {
80+
data = JSONObject.fromObject(lastDuplicate.ghSubscriberEvent().getPayload());
81+
} else {
82+
data = getLastDuplicateNoEventPayload();
83+
}
84+
return new JsonHttpResponse(data, 200);
85+
}
86+
87+
@VisibleForTesting
88+
static JSONObject getLastDuplicateNoEventPayload() {
89+
return new JSONObject().accumulate("payload", "No duplicate events seen yet");
90+
}
91+
92+
/**
93+
* Tracks duplicate {@link GHEvent} triggering actions in Jenkins.
94+
* Events are tracked for 10 minutes, with the last detected duplicate reference retained for up to 24 hours
95+
* (see {@link #isDuplicateEventSeen}).
96+
* <p>
97+
* Duplicates are stored in-memory only, so a controller restart clears all entries as if none existed.
98+
* Persistent storage is omitted for simplicity, since webhook misconfigurations would likely cause new duplicates.
99+
*/
100+
@Extension
101+
public static final class DuplicateEventsSubscriber extends GHEventsSubscriber {
102+
103+
private static final Logger LOGGER = Logger.getLogger(DuplicateEventsSubscriber.class.getName());
104+
105+
private static Ticker ticker = Ticker.systemTicker();
106+
/**
107+
* Caches GitHub event GUIDs for 10 minutes to track recent events to detect duplicates.
108+
* <p>
109+
* Only the keys (event GUIDs) are relevant, as Caffeine automatically handles expiration based
110+
* on insertion time; the value is irrelevant, we put {@link #DUMMY}, as Caffeine doesn't provide any
111+
* Set structures.
112+
* <p>
113+
* Maximum cache size is set to 24k so it doesn't grow unbound (approx. 1MB). Each key takes 36 bytes, and
114+
* timestamp (assuming caffeine internally keeps long) takes 8 bytes; total of 44 bytes
115+
* per entry. So the maximum memory consumed by this cache is 24k * 44 = 1056k = 1.056 MB.
116+
*/
117+
private static final Cache<String, Object> EVENT_TRACKER = Caffeine.newBuilder()
118+
.maximumSize(24_000L)
119+
.expireAfterWrite(Duration.ofMinutes(10))
120+
.ticker(() -> ticker.read())
121+
.build();
122+
private static final Object DUMMY = new Object();
123+
124+
private static volatile TrackedDuplicateEvent lastDuplicate;
125+
public record TrackedDuplicateEvent(
126+
String eventGuid, Instant lastUpdated, GHSubscriberEvent ghSubscriberEvent) { }
127+
private static final Duration TWENTY_FOUR_HOURS = Duration.ofHours(24);
128+
129+
@VisibleForTesting
130+
@Restricted(NoExternalUse.class)
131+
static void setTicker(Ticker testTicker) {
132+
ticker = testTicker;
133+
}
134+
135+
/**
136+
* This subscriber is not applicable to any item
137+
*
138+
* @param item ignored
139+
* @return always false
140+
*/
141+
@Override
142+
protected boolean isApplicable(@Nullable Item item) {
143+
return false;
144+
}
145+
146+
/**
147+
* {@inheritDoc}
148+
* <p>
149+
* Subscribes to events that trigger actions in Jenkins, such as repository scans or builds.
150+
* <p>
151+
* The {@link GHEvent} enum defines about 63 events, but not all are relevant to Jenkins.
152+
* Tracking unnecessary events increases memory usage, and they occur more frequently than those triggering any
153+
* work.
154+
* <p>
155+
* <a href="https://docs.github.com/en/webhooks/webhook-events-and-payloads">
156+
* Documentation reference (also referenced in {@link GHEvent})</a>
157+
*/
158+
@Override
159+
protected Set<GHEvent> events() {
160+
return Set.of(
161+
GHEvent.CHECK_RUN, // associated with GitHub action Re-run button to trigger build
162+
GHEvent.CHECK_SUITE, // associated with GitHub action Re-run button to trigger build
163+
GHEvent.CREATE, // branch or tag creation
164+
GHEvent.DELETE, // branch or tag deletion
165+
GHEvent.PULL_REQUEST, // PR creation (also PR close or merge)
166+
GHEvent.PUSH // commit push
167+
);
168+
}
169+
170+
@Override
171+
protected void onEvent(final GHSubscriberEvent event) {
172+
String eventGuid = event.getEventGuid();
173+
LOGGER.fine(() -> "Received event with GUID: " + eventGuid);
174+
if (eventGuid == null) {
175+
return;
176+
}
177+
if (EVENT_TRACKER.getIfPresent(eventGuid) != null) {
178+
lastDuplicate = new TrackedDuplicateEvent(eventGuid, getNow(), event);
179+
}
180+
EVENT_TRACKER.put(eventGuid, DUMMY);
181+
}
182+
183+
/**
184+
* Checks if a duplicate event was recorded in the past 24 hours.
185+
* <p>
186+
* Events are not stored for 24 hours—only the most recent duplicate is checked within this timeframe.
187+
*
188+
* @return {@code true} if a duplicate was seen in the last 24 hours, {@code false} otherwise.
189+
*/
190+
public static boolean isDuplicateEventSeen() {
191+
return lastDuplicate != null
192+
&& Duration.between(lastDuplicate.lastUpdated(), getNow()).compareTo(TWENTY_FOUR_HOURS) < 0;
193+
}
194+
195+
private static Instant getNow() {
196+
return Instant.ofEpochSecond(0L, ticker.read());
197+
}
198+
199+
public static TrackedDuplicateEvent getLastDuplicate() {
200+
return lastDuplicate;
201+
}
202+
203+
/**
204+
* Caffeine expired keys are not removed immediately. Method returns the non-expired keys;
205+
* required for the tests.
206+
*/
207+
@VisibleForTesting
208+
@Restricted(NoExternalUse.class)
209+
static Set<String> getEventCountsTracker() {
210+
return EVENT_TRACKER.asMap().keySet().stream()
211+
.filter(key -> EVENT_TRACKER.getIfPresent(key) != null)
212+
.collect(Collectors.toSet());
213+
}
214+
}
215+
}

src/main/java/org/jenkinsci/plugins/github/extension/GHSubscriberEvent.java

+22-2
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,32 @@ public class GHSubscriberEvent extends SCMEvent<String> {
1818
*/
1919
private final GHEvent ghEvent;
2020

21+
private final String eventGuid;
22+
23+
/**
24+
* @deprecated use {@link #GHSubscriberEvent(String, String, GHEvent, String)} instead.
25+
*/
26+
@Deprecated
27+
public GHSubscriberEvent(@CheckForNull String origin, @NonNull GHEvent ghEvent, @NonNull String payload) {
28+
this(null, origin, ghEvent, payload);
29+
}
30+
2131
/**
2232
* Constructs a new {@link GHSubscriberEvent}.
23-
*
33+
* @param eventGuid the globally unique identifier (GUID) to identify the event; value of
34+
* request header {@link com.cloudbees.jenkins.GitHubWebHook#X_GITHUB_DELIVERY}.
2435
* @param origin the origin (see {@link SCMEvent#originOf(HttpServletRequest)}) or {@code null}.
2536
* @param ghEvent the type of event received from GitHub.
2637
* @param payload the event payload.
2738
*/
28-
public GHSubscriberEvent(@CheckForNull String origin, @NonNull GHEvent ghEvent, @NonNull String payload) {
39+
public GHSubscriberEvent(
40+
@CheckForNull String eventGuid,
41+
@CheckForNull String origin,
42+
@NonNull GHEvent ghEvent,
43+
@NonNull String payload) {
2944
super(Type.UPDATED, payload, origin);
3045
this.ghEvent = ghEvent;
46+
this.eventGuid = eventGuid;
3147
}
3248

3349
/**
@@ -39,4 +55,8 @@ public GHEvent getGHEvent() {
3955
return ghEvent;
4056
}
4157

58+
@CheckForNull
59+
public String getEventGuid() {
60+
return eventGuid;
61+
}
4262
}

src/main/resources/org/jenkinsci/plugins/github/Messages.properties

+6
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,9 @@ github.trigger.check.method.warning.details=The webhook for repo {0}/{1} on {2}
1111
More info can be found on the global configuration page. This message will be dismissed if Jenkins receives \
1212
a PING event from repo webhook or if you add the repo to the ignore list in the global configuration.
1313
unknown.error=Unknown error
14+
duplicate.events.administrative.monitor.displayname=GitHub Duplicate Events
15+
duplicate.events.administrative.monitor.description=Warns about duplicate events received from GitHub.
16+
duplicate.events.administrative.monitor.blurb=Duplicate events were received from GitHub, possibly due to \
17+
misconfiguration (e.g., multiple webhooks targeting the same Jenkins controller at the repository or organization \
18+
level), potentially causing redundant builds or at least wasted work. \
19+
<a id="{0}" href="{1}" target="_blank">Click here</a> to inspect the last tracked duplicate event payload.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<?jelly escape-by-default='true'?>
2+
<j:jelly xmlns:j="jelly:core">
3+
<j:out value="${it.description}"/>
4+
</j:jelly>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<?jelly escape-by-default='true'?>
2+
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
3+
<div class="alert alert-warning">
4+
<form method="post" action="${rootURL}/${it.url}/disable" name="${it.id}">
5+
<f:submit primary="false" clazz="jenkins-!-destructive-color" name="no" value="${%Dismiss}"/>
6+
</form>
7+
<j:out value="${it.blurb}"/>
8+
</div>
9+
</j:jelly>

src/test/java/com/cloudbees/jenkins/GitHubWebHookTest.java

+29-11
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
import com.google.inject.Inject;
44

55
import hudson.model.Item;
6-
import hudson.model.Job;
76

87
import org.jenkinsci.plugins.github.extension.GHEventsSubscriber;
98
import org.junit.Before;
@@ -12,6 +11,11 @@
1211
import org.jvnet.hudson.test.JenkinsRule;
1312
import org.jvnet.hudson.test.TestExtension;
1413
import org.kohsuke.github.GHEvent;
14+
import org.kohsuke.stapler.Stapler;
15+
import org.kohsuke.stapler.StaplerRequest2;
16+
import org.mockito.Mock;
17+
import org.mockito.Mockito;
18+
import org.mockito.MockitoAnnotations;
1519

1620
import java.util.Set;
1721

@@ -41,31 +45,45 @@ public class GitHubWebHookTest {
4145
@Inject
4246
private ThrowablePullRequestSubscriber throwablePullRequestSubscriber;
4347

48+
@Mock
49+
private StaplerRequest2 req2;
50+
4451
@Before
4552
public void setUp() throws Exception {
53+
MockitoAnnotations.openMocks(this);
4654
jenkins.getInstance().getInjector().injectMembers(this);
4755
}
4856

4957
@Test
5058
public void shouldCallExtensionInterestedInIssues() throws Exception {
51-
new GitHubWebHook().doIndex(GHEvent.ISSUES, PAYLOAD);
52-
assertThat("should get interested event", subscriber.lastEvent(), equalTo(GHEvent.ISSUES));
59+
try(var mockedStapler = Mockito.mockStatic(Stapler.class)) {
60+
mockedStapler.when(Stapler::getCurrentRequest2).thenReturn(req2);
61+
62+
new GitHubWebHook().doIndex(GHEvent.ISSUES, PAYLOAD);
63+
assertThat("should get interested event", subscriber.lastEvent(), equalTo(GHEvent.ISSUES));
64+
}
5365
}
5466

5567
@Test
5668
public void shouldNotCallAnyExtensionsWithPublicEventIfNotRegistered() throws Exception {
57-
new GitHubWebHook().doIndex(GHEvent.PUBLIC, PAYLOAD);
58-
assertThat("should not get not interested event", subscriber.lastEvent(), nullValue());
69+
try(var mockedStapler = Mockito.mockStatic(Stapler.class)) {
70+
mockedStapler.when(Stapler::getCurrentRequest2).thenReturn(req2);
71+
72+
new GitHubWebHook().doIndex(GHEvent.PUBLIC, PAYLOAD);
73+
assertThat("should not get not interested event", subscriber.lastEvent(), nullValue());
74+
}
5975
}
6076

6177
@Test
6278
public void shouldCatchThrowableOnFailedSubscriber() throws Exception {
63-
new GitHubWebHook().doIndex(GHEvent.PULL_REQUEST, PAYLOAD);
64-
assertThat("each extension should get event",
65-
asList(
66-
pullRequestSubscriber.lastEvent(),
67-
throwablePullRequestSubscriber.lastEvent()
68-
), everyItem(equalTo(GHEvent.PULL_REQUEST)));
79+
try(var mockedStapler = Mockito.mockStatic(Stapler.class)) {
80+
mockedStapler.when(Stapler::getCurrentRequest2).thenReturn(req2);
81+
82+
new GitHubWebHook().doIndex(GHEvent.PULL_REQUEST, PAYLOAD);
83+
assertThat("each extension should get event",
84+
asList(pullRequestSubscriber.lastEvent(), throwablePullRequestSubscriber.lastEvent()),
85+
everyItem(equalTo(GHEvent.PULL_REQUEST)));
86+
}
6987
}
7088

7189
@TestExtension

0 commit comments

Comments
 (0)