Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn about duplicated events received from GitHub via Admin Monitor #388

Merged
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
fc6e29c
create an admin monitor for warning about duplicate events from github
gbhat618 Jan 31, 2025
a8bcb47
update the list of interested events and minor changes in cleanup
gbhat618 Feb 3, 2025
3d9c96b
consider tests sending stapler currentRequest null
gbhat618 Feb 4, 2025
0a3820c
use mocks instead of putting null check in execution code
gbhat618 Feb 4, 2025
c8f5a01
remove the fixed comment
gbhat618 Feb 4, 2025
978b8b6
track the last seen duplicate for 24 hours
gbhat618 Feb 5, 2025
bfd9df8
Update src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/…
gbhat618 Feb 5, 2025
e77068d
remove few bits to further simplify
gbhat618 Feb 5, 2025
188be21
provide a way to extract last logged event via logging in duplicates …
gbhat618 Feb 6, 2025
89ef2bd
add a comment for why tracking the prev event inside the logger eval
gbhat618 Feb 6, 2025
e3d9f80
log message should be clear to the user
gbhat618 Feb 6, 2025
a5dd3bf
update the admin monitor test to include duplicate event logging logic
gbhat618 Feb 6, 2025
b5ab212
show the payload via hyperlink not logging
gbhat618 Feb 7, 2025
57f050f
mark the method with GET
gbhat618 Feb 7, 2025
c0fe850
use variable for shorter line
gbhat618 Feb 8, 2025
aa61458
Update src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplica…
gbhat618 Feb 11, 2025
f311a6b
Update src/main/resources/org/jenkinsci/plugins/github/Messages.prope…
gbhat618 Feb 11, 2025
3e75413
Update src/main/resources/org/jenkinsci/plugins/github/Messages.prope…
gbhat618 Feb 11, 2025
e8fbb20
Update src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplica…
gbhat618 Feb 11, 2025
39350c7
Update src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplica…
gbhat618 Feb 11, 2025
5577148
Update src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplica…
gbhat618 Feb 11, 2025
8473753
update for comments
gbhat618 Feb 11, 2025
e46168d
add a comment and fix the javadoc
gbhat618 Feb 11, 2025
626e368
Update src/main/java/org/jenkinsci/plugins/github/extension/GHSubscri…
gbhat618 Feb 11, 2025
c65fc31
use caffeine for cache instead of ConcurrentHashMap
gbhat618 Feb 11, 2025
8334bda
add a note about cache size
gbhat618 Feb 11, 2025
82c5ab6
move the duplicate event subscriber as an inner class into the github…
gbhat618 Feb 14, 2025
50108ef
Update src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplica…
gbhat618 Feb 20, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/main/java/com/cloudbees/jenkins/GitHubWebHook.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ public class GitHubWebHook implements UnprotectedRootAction {
// headers used for testing the endpoint configuration
public static final String URL_VALIDATION_HEADER = "X-Jenkins-Validation";
public static final String X_INSTANCE_IDENTITY = "X-Instance-Identity";
/**
* X-GitHub-Delivery: A globally unique identifier (GUID) to identify the event.
* @see <a href="https://docs.github.com/en/webhooks/webhook-events-and-payloads#delivery-headers">Delivery
* headers</a>
*/
public static final String X_GITHUB_DELIVERY = "X-GitHub-Delivery";

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

Expand Down Expand Up @@ -117,8 +123,10 @@ public List<Item> reRegisterAllHooks() {
@SuppressWarnings("unused")
@RequirePostWithGHHookPayload
public void doIndex(@NonNull @GHEventHeader GHEvent event, @NonNull @GHEventPayload String payload) {
var currentRequest = Stapler.getCurrentRequest2();
String eventGuid = currentRequest.getHeader(X_GITHUB_DELIVERY);
GHSubscriberEvent subscriberEvent =
new GHSubscriberEvent(SCMEvent.originOf(Stapler.getCurrentRequest2()), event, payload);
new GHSubscriberEvent(eventGuid, SCMEvent.originOf(currentRequest), event, payload);
from(GHEventsSubscriber.all())
.filter(isInterestedIn(event))
.transform(processEvent(subscriberEvent)).toList();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package org.jenkinsci.plugins.github.admin;

import com.google.common.annotations.VisibleForTesting;

import hudson.Extension;
import hudson.model.AdministrativeMonitor;
import jenkins.model.Jenkins;
import org.jenkinsci.plugins.github.Messages;
import org.jenkinsci.plugins.github.webhook.subscriber.DuplicateEventsSubscriber;
import org.kohsuke.stapler.HttpResponse;
import org.kohsuke.stapler.WebMethod;
import org.kohsuke.stapler.json.JsonHttpResponse;
import net.sf.json.JSONObject;

@SuppressWarnings("unused")
@Extension
public class GitHubDuplicateEventsMonitor extends AdministrativeMonitor {

@VisibleForTesting
static final String LAST_DUPLICATE_CLICK_HERE_ANCHOR_ID = GitHubDuplicateEventsMonitor.class.getName()
+ "#last-duplicate";

@Override
public String getDisplayName() {
return Messages.duplicate_events_administrative_monitor_displayname();
}

public String getDescription() {
return Messages.duplicate_events_administrative_monitor_description();
}

public String getBlurb() {
return Messages.duplicate_events_administrative_monitor_blurb(
LAST_DUPLICATE_CLICK_HERE_ANCHOR_ID, this.getLastDuplicateUrl());
}

@VisibleForTesting
String getLastDuplicateUrl() {
return this.getUrl() + "/" + "last-duplicate.json";
}

@Override
public boolean isActivated() {
return DuplicateEventsSubscriber.isDuplicateEventSeen();
}

@Override
public boolean hasRequiredPermission() {
return Jenkins.get().hasPermission(Jenkins.SYSTEM_READ);
}

@Override
public void checkRequiredPermission() {
Jenkins.get().checkPermission(Jenkins.SYSTEM_READ);
}

@WebMethod(name = "last-duplicate.json")
public HttpResponse doGetLastDuplicatePayload() {
Jenkins.get().checkPermission(Jenkins.SYSTEM_READ);
JSONObject data = getLastDuplicateNoEventPayload();
if (DuplicateEventsSubscriber.getLastDuplicate() != null) {
data = JSONObject.fromObject(DuplicateEventsSubscriber.getLastDuplicate().ghSubscriberEvent().getPayload());
}
return new JsonHttpResponse(data, 200);
}

@VisibleForTesting
static JSONObject getLastDuplicateNoEventPayload() {
return JSONObject.fromObject("{\"payload\": \"No duplicate events seen yet.\"}");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,34 @@ public class GHSubscriberEvent extends SCMEvent<String> {
*/
private final GHEvent ghEvent;

private final String eventGuid;

/**
* @deprecated use {@link #GHSubscriberEvent(String, String, GHEvent, String)} instead.
*/
@Deprecated
public GHSubscriberEvent(@CheckForNull String origin, @NonNull GHEvent ghEvent, @NonNull String payload) {
super(Type.UPDATED, payload, origin);
this.ghEvent = ghEvent;
this.eventGuid = null;
}

/**
* Constructs a new {@link GHSubscriberEvent}.
*
* @param eventGuid the globally unique identifier (GUID) to identify the event; value of
* request header {@link com.cloudbees.jenkins.GitHubWebHook#X_GITHUB_DELIVERY}.
* @param origin the origin (see {@link SCMEvent#originOf(HttpServletRequest)}) or {@code null}.
* @param ghEvent the type of event received from GitHub.
* @param payload the event payload.
*/
public GHSubscriberEvent(@CheckForNull String origin, @NonNull GHEvent ghEvent, @NonNull String payload) {
public GHSubscriberEvent(
@CheckForNull String eventGuid,
@CheckForNull String origin,
@NonNull GHEvent ghEvent,
@NonNull String payload) {
super(Type.UPDATED, payload, origin);
this.ghEvent = ghEvent;
this.eventGuid = eventGuid;
}

/**
Expand All @@ -39,4 +57,8 @@ public GHEvent getGHEvent() {
return ghEvent;
}

@CheckForNull
public String getEventGuid() {
return eventGuid;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
package org.jenkinsci.plugins.github.webhook.subscriber;

import static com.google.common.collect.Sets.immutableEnumSet;

import com.google.common.annotations.VisibleForTesting;
import edu.umd.cs.findbugs.annotations.Nullable;
import hudson.Extension;
import hudson.model.Item;
import hudson.model.PeriodicWork;

import java.time.Instant;
import java.util.Collections;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
import java.util.logging.Logger;
import org.jenkinsci.plugins.github.extension.GHEventsSubscriber;
import org.jenkinsci.plugins.github.extension.GHSubscriberEvent;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.github.GHEvent;

/**
* Tracks duplicate {@link GHEvent} triggering actions in Jenkins.
* Events are tracked for 10 minutes, with the last detected duplicate reference retained for up to 24 hours
* (see {@link #isDuplicateEventSeen}).
* <p>
* Duplicates are stored in-memory only, so a controller restart clears all entries as if none existed.
* Persistent storage is omitted for simplicity, since webhook misconfigurations would likely cause new duplicates.
*/
@Extension
public final class DuplicateEventsSubscriber extends GHEventsSubscriber {

private static final Logger LOGGER = Logger.getLogger(DuplicateEventsSubscriber.class.getName());
private static final long TTL_MILLIS = TimeUnit.MINUTES.toMillis(10);
private static final long TWENTY_FOUR_HOURS_MILLIS = TimeUnit.HOURS.toMillis(24);
private static final Map<String, Long> EVENT_TRACKER = new ConcurrentHashMap<>();
private static volatile TrackedDuplicateEvent lastDuplicate;

public record TrackedDuplicateEvent(String eventGuid, long lastUpdated, GHSubscriberEvent ghSubscriberEvent) { }

/**
* This subscriber is not applicable to any item
*
* @param item ignored
* @return always false
*/
@Override
protected boolean isApplicable(@Nullable Item item) {
return false;
}

/**
* Subscribes to events that trigger actions in Jenkins, such as repository scans or builds.
* <p>
* The {@link GHEvent} enum defines about 63 events, but not all are relevant to Jenkins.
* Tracking unnecessary events increases memory usage, and they occur more frequently than those triggering any
* work.
* <p>
* <a href="https://docs.github.com/en/webhooks/webhook-events-and-payloads">
* Documentation reference (also referenced in {@link GHEvent})</a>
*/
@Override
protected Set<GHEvent> events() {
return immutableEnumSet(
GHEvent.CHECK_RUN, // associated with GitHub action Re-run button to trigger build
GHEvent.CHECK_SUITE, // associated with GitHub action Re-run button to trigger build
GHEvent.CREATE, // branch or tag creation
GHEvent.DELETE, // branch or tag deletion
GHEvent.PULL_REQUEST, // PR creation (also PR close or merge)
GHEvent.PUSH // commit push
);
}

@Override
protected void onEvent(final GHSubscriberEvent event) {
String eventGuid = event.getEventGuid();
LOGGER.fine(() -> "Received event with GUID: " + eventGuid);
if (eventGuid == null) {
return;
}
if (EVENT_TRACKER.containsKey(eventGuid)) {
lastDuplicate = new TrackedDuplicateEvent(eventGuid, Instant.now().toEpochMilli(), event);
}
EVENT_TRACKER.put(eventGuid, Instant.now().toEpochMilli());
}

/**
* Checks if a duplicate event was recorded in the past 24 hours.
* <p>
* Events are not stored for 24 hours—only the most recent duplicate is checked within this timeframe.
*
* @return {@code true} if a duplicate was seen in the last 24 hours, {@code false} otherwise.
*/
public static boolean isDuplicateEventSeen() {
return lastDuplicate != null
&& (Instant.now().toEpochMilli() - lastDuplicate.lastUpdated()) < TWENTY_FOUR_HOURS_MILLIS;
}

public static TrackedDuplicateEvent getLastDuplicate() {
return lastDuplicate;
}

@VisibleForTesting
@Restricted(NoExternalUse.class)
static void cleanUpOldEntries() {
var nowMillis = Instant.now().toEpochMilli();
EVENT_TRACKER.entrySet().removeIf(entry -> nowMillis - entry.getValue() > TTL_MILLIS);
LOGGER.fine(() -> "Entries remaining after cleanup " + EVENT_TRACKER.size());
}

@VisibleForTesting
@Restricted(NoExternalUse.class)
static Map<String, Long> getEventCountsTracker() {
return Collections.unmodifiableMap(EVENT_TRACKER);
}

/**
* Periodically runs every 5 minutes, and remove old entries from {@link #EVENT_TRACKER}.
*/
@SuppressWarnings("unused")
@Extension
public static class EventCountBackgroundWork extends PeriodicWork {

@Override
public long getRecurrencePeriod() {
return TTL_MILLIS / 2;
}

@Override
protected void doRun() {
LOGGER.fine(() -> "Cleaning up old entries from duplicate events tracker");
cleanUpOldEntries();
}

Check warning on line 135 in src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DuplicateEventsSubscriber.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 133-135 are not covered by tests
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,9 @@ github.trigger.check.method.warning.details=The webhook for repo {0}/{1} on {2}
More info can be found on the global configuration page. This message will be dismissed if Jenkins receives \
a PING event from repo webhook or if you add the repo to the ignore list in the global configuration.
unknown.error=Unknown error
duplicate.events.administrative.monitor.displayname=GitHub Duplicate Events
duplicate.events.administrative.monitor.description=Warns about duplicate events received from GitHub.
duplicate.events.administrative.monitor.blurb=Duplicate events were received from GitHub, possibly due to \
misconfiguration (e.g., multiple webhooks targeting the same Jenkins controller at the repository or organization \
level), potentially causing redundant job executions. \
<a id="{0}" href=./{1} target="_blank">Click here</a> to inspect the last tracked duplicate event payload.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core">
<j:out value="${it.description}"/>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<div class="alert alert-warning">
<form method="post" action="${rootURL}/${it.url}/disable" name="${it.id}">
<f:submit primary="false" clazz="jenkins-!-destructive-color" name="no" value="${%Dismiss}"/>
</form>
<j:out value="${it.blurb}"/>
</div>
</j:jelly>
40 changes: 29 additions & 11 deletions src/test/java/com/cloudbees/jenkins/GitHubWebHookTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import com.google.inject.Inject;

import hudson.model.Item;
import hudson.model.Job;

import org.jenkinsci.plugins.github.extension.GHEventsSubscriber;
import org.junit.Before;
Expand All @@ -12,6 +11,11 @@
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.TestExtension;
import org.kohsuke.github.GHEvent;
import org.kohsuke.stapler.Stapler;
import org.kohsuke.stapler.StaplerRequest2;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.MockitoAnnotations;

import java.util.Set;

Expand Down Expand Up @@ -41,31 +45,45 @@ public class GitHubWebHookTest {
@Inject
private ThrowablePullRequestSubscriber throwablePullRequestSubscriber;

@Mock
private StaplerRequest2 req2;

@Before
public void setUp() throws Exception {
MockitoAnnotations.openMocks(this);
jenkins.getInstance().getInjector().injectMembers(this);
}

@Test
public void shouldCallExtensionInterestedInIssues() throws Exception {
new GitHubWebHook().doIndex(GHEvent.ISSUES, PAYLOAD);
assertThat("should get interested event", subscriber.lastEvent(), equalTo(GHEvent.ISSUES));
try(var mockedStapler = Mockito.mockStatic(Stapler.class)) {
mockedStapler.when(Stapler::getCurrentRequest2).thenReturn(req2);

new GitHubWebHook().doIndex(GHEvent.ISSUES, PAYLOAD);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ignore WS)

assertThat("should get interested event", subscriber.lastEvent(), equalTo(GHEvent.ISSUES));
}
}

@Test
public void shouldNotCallAnyExtensionsWithPublicEventIfNotRegistered() throws Exception {
new GitHubWebHook().doIndex(GHEvent.PUBLIC, PAYLOAD);
assertThat("should not get not interested event", subscriber.lastEvent(), nullValue());
try(var mockedStapler = Mockito.mockStatic(Stapler.class)) {
mockedStapler.when(Stapler::getCurrentRequest2).thenReturn(req2);

new GitHubWebHook().doIndex(GHEvent.PUBLIC, PAYLOAD);
assertThat("should not get not interested event", subscriber.lastEvent(), nullValue());
}
}

@Test
public void shouldCatchThrowableOnFailedSubscriber() throws Exception {
new GitHubWebHook().doIndex(GHEvent.PULL_REQUEST, PAYLOAD);
assertThat("each extension should get event",
asList(
pullRequestSubscriber.lastEvent(),
throwablePullRequestSubscriber.lastEvent()
), everyItem(equalTo(GHEvent.PULL_REQUEST)));
try(var mockedStapler = Mockito.mockStatic(Stapler.class)) {
mockedStapler.when(Stapler::getCurrentRequest2).thenReturn(req2);

new GitHubWebHook().doIndex(GHEvent.PULL_REQUEST, PAYLOAD);
assertThat("each extension should get event",
asList(pullRequestSubscriber.lastEvent(), throwablePullRequestSubscriber.lastEvent()),
everyItem(equalTo(GHEvent.PULL_REQUEST)));
}
}

@TestExtension
Expand Down
Loading