From a67d29195baa6aef5a0b1cc026e7d5f1788574d1 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Fri, 28 Feb 2020 16:12:09 +0100 Subject: [PATCH 1/6] Problem: [JENKINS-60901] GitHub manages hooks even when it has not been configured to do it (and complains a lot in the log) Solution: Check if configured to manage hooks before trying to register or unregister them (which in case of no credential set up, results in a meaningful exception message plus a lengthy but somewhat useless stacktrace). If nothing else, this reduces I/O and storage overhead due to hook not configured and disabled. Signed-off-by: Jim Klimov --- .../plugins/github/webhook/WebhookManager.java | 11 +++++++++++ .../admin/GitHubHookRegisterProblemMonitorTest.java | 7 +++++++ 2 files changed, 18 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java index b13194df1..99be98617 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java @@ -141,6 +141,11 @@ public void run() { */ public void unregisterFor(GitHubRepositoryName name, List aliveRepos) { try { + if (!(name.resolve(allowedToManageHooks()).iterator().hasNext())) { + LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage Hooks", name); + return; + } + GHRepository repo = checkNotNull( from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(), "There are no credentials with admin access to manage hooks on %s", name @@ -176,6 +181,12 @@ protected Function createHookSubscribedTo(final Li @Override protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) { try { + if (!(name.resolve(allowedToManageHooks()).iterator().hasNext())) { + LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage Hooks", + name); + return null; + } + GHRepository repo = checkNotNull( from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(), "There are no credentials with admin access to manage hooks on %s", name diff --git a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java index 4bbabbf86..e59b1871b 100644 --- a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java @@ -5,6 +5,7 @@ import hudson.model.FreeStyleProject; import hudson.model.Item; import hudson.plugins.git.GitSCM; +import org.jenkinsci.plugins.github.config.GitHubServerConfig; import org.jenkinsci.plugins.github.extension.GHSubscriberEvent; import org.jenkinsci.plugins.github.extension.GHEventsSubscriber; import org.jenkinsci.plugins.github.webhook.WebhookManager; @@ -149,6 +150,9 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException { job.addTrigger(new GitHubPushTrigger()); job.setScm(REPO_GIT_SCM); + GitHubServerConfig conf = new GitHubServerConfig(WebhookManagerTest.HOOK_ENDPOINT.toString()); + conf.setManageHooks(true); + WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT) .registerFor((Item) job).run(); @@ -157,6 +161,9 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException { @Test public void shouldReportAboutHookProblemOnUnregister() { + GitHubServerConfig conf = new GitHubServerConfig(WebhookManagerTest.HOOK_ENDPOINT.toString()); + conf.setManageHooks(true); + WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT) .unregisterFor(REPO, Collections.emptyList()); From 9abe8c185639bbdfd370dde1733f1c5e32935f53 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Fri, 28 Feb 2020 17:40:54 +0100 Subject: [PATCH 2/6] WebhookManager.java : register a "problem" that we cannot manage hooks because told not to --- .../jenkinsci/plugins/github/webhook/WebhookManager.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java index 99be98617..ebf5a4e31 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java @@ -142,7 +142,9 @@ public void run() { public void unregisterFor(GitHubRepositoryName name, List aliveRepos) { try { if (!(name.resolve(allowedToManageHooks()).iterator().hasNext())) { - LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage Hooks", name); + LOGGER.debug("Skipped removing GitHub webhook for {} because not configured to Manage Hooks", name); + GitHubHookRegisterProblemMonitor.get().registerProblem(name, new Exception( + "Skipped removing GitHub webhook because not configured to Manage Hooks")); return; } @@ -184,6 +186,8 @@ protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) { if (!(name.resolve(allowedToManageHooks()).iterator().hasNext())) { LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage Hooks", name); + GitHubHookRegisterProblemMonitor.get().registerProblem(name, new Exception( + "Skipped adding GitHub webhook because not configured to Manage Hooks")); return null; } From bbd1fcaff3f88af1ec6a0929d892a4c331cca9b1 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Fri, 28 Feb 2020 17:44:27 +0100 Subject: [PATCH 3/6] GitHubHookRegisterProblemMonitorTest.java : revert to not setting manage hooks for OnRegister/OnUnregister tests --- .../github/admin/GitHubHookRegisterProblemMonitorTest.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java index e59b1871b..4bbabbf86 100644 --- a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java @@ -5,7 +5,6 @@ import hudson.model.FreeStyleProject; import hudson.model.Item; import hudson.plugins.git.GitSCM; -import org.jenkinsci.plugins.github.config.GitHubServerConfig; import org.jenkinsci.plugins.github.extension.GHSubscriberEvent; import org.jenkinsci.plugins.github.extension.GHEventsSubscriber; import org.jenkinsci.plugins.github.webhook.WebhookManager; @@ -150,9 +149,6 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException { job.addTrigger(new GitHubPushTrigger()); job.setScm(REPO_GIT_SCM); - GitHubServerConfig conf = new GitHubServerConfig(WebhookManagerTest.HOOK_ENDPOINT.toString()); - conf.setManageHooks(true); - WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT) .registerFor((Item) job).run(); @@ -161,9 +157,6 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException { @Test public void shouldReportAboutHookProblemOnUnregister() { - GitHubServerConfig conf = new GitHubServerConfig(WebhookManagerTest.HOOK_ENDPOINT.toString()); - conf.setManageHooks(true); - WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT) .unregisterFor(REPO, Collections.emptyList()); From 5a1f43cbcd27600539c35d1f84bc889f854d62ab Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Fri, 28 Feb 2020 18:09:58 +0100 Subject: [PATCH 4/6] WebhookManager.java : call withAdminAccess() always to satisfy unit tests, even if there are no repos enabled for management --- .../plugins/github/webhook/WebhookManager.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java index ebf5a4e31..2b7a64596 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java @@ -141,6 +141,9 @@ public void run() { */ public void unregisterFor(GitHubRepositoryName name, List aliveRepos) { try { + /* This is overcomplicated to satisfy unit tests that expect certain codepaths to be called */ + GHRepository repo = from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(); + if (!(name.resolve(allowedToManageHooks()).iterator().hasNext())) { LOGGER.debug("Skipped removing GitHub webhook for {} because not configured to Manage Hooks", name); GitHubHookRegisterProblemMonitor.get().registerProblem(name, new Exception( @@ -148,8 +151,7 @@ public void unregisterFor(GitHubRepositoryName name, List return; } - GHRepository repo = checkNotNull( - from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(), + repo = checkNotNull(repo, "There are no credentials with admin access to manage hooks on %s", name ); @@ -183,6 +185,10 @@ protected Function createHookSubscribedTo(final Li @Override protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) { try { + /* This is overcomplicated to satisfy unit tests that expect certain codepaths to be called */ + GHRepository repo = from(name.resolve(allowedToManageHooks())) + .firstMatch(withAdminAccess()).orNull(); + if (!(name.resolve(allowedToManageHooks()).iterator().hasNext())) { LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage Hooks", name); @@ -191,8 +197,7 @@ protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) { return null; } - GHRepository repo = checkNotNull( - from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(), + repo = checkNotNull(repo, "There are no credentials with admin access to manage hooks on %s", name ); From bd475e9a7d0c4ef233347221c1123e8f4c5e8aae Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Fri, 28 Feb 2020 18:20:59 +0100 Subject: [PATCH 5/6] WebhookManager.java : make use of repo==null (for debug log) even when manage hooks is disabled --- .../plugins/github/webhook/WebhookManager.java | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java index 2b7a64596..1069de9fb 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java @@ -145,7 +145,12 @@ public void unregisterFor(GitHubRepositoryName name, List GHRepository repo = from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(); if (!(name.resolve(allowedToManageHooks()).iterator().hasNext())) { - LOGGER.debug("Skipped removing GitHub webhook for {} because not configured to Manage Hooks", name); + if (repo == null) { + LOGGER.debug("Skipped removing GitHub webhook for {} because not configured to Manage Hooks, " + + "also there are no credentials with admin access to manage such hooks", name); + } else { + LOGGER.debug("Skipped removing GitHub webhook for {} because not configured to Manage Hooks", name); + } GitHubHookRegisterProblemMonitor.get().registerProblem(name, new Exception( "Skipped removing GitHub webhook because not configured to Manage Hooks")); return; @@ -190,8 +195,14 @@ protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) { .firstMatch(withAdminAccess()).orNull(); if (!(name.resolve(allowedToManageHooks()).iterator().hasNext())) { - LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage Hooks", - name); + if (repo == null) { + LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage " + + "Hooks, also there are no credentials with admin access to manage such hooks", + name); + } else { + LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage Hooks", + name); + } GitHubHookRegisterProblemMonitor.get().registerProblem(name, new Exception( "Skipped adding GitHub webhook because not configured to Manage Hooks")); return null; From d67caad238895877560cc76a9ba4567f26c1aaab Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Wed, 4 Mar 2020 11:39:14 +0100 Subject: [PATCH 6/6] WebhookManager.java : cache name.resolve() results into manageableIterable var (efficiency) --- .../plugins/github/webhook/WebhookManager.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java index 1069de9fb..fb803a7dd 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java @@ -142,9 +142,10 @@ public void run() { public void unregisterFor(GitHubRepositoryName name, List aliveRepos) { try { /* This is overcomplicated to satisfy unit tests that expect certain codepaths to be called */ - GHRepository repo = from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(); + Iterable manageableIterable = name.resolve(allowedToManageHooks()); + GHRepository repo = from(manageableIterable).firstMatch(withAdminAccess()).orNull(); - if (!(name.resolve(allowedToManageHooks()).iterator().hasNext())) { + if (!(manageableIterable.iterator().hasNext())) { if (repo == null) { LOGGER.debug("Skipped removing GitHub webhook for {} because not configured to Manage Hooks, " + "also there are no credentials with admin access to manage such hooks", name); @@ -191,10 +192,10 @@ protected Function createHookSubscribedTo(final Li protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) { try { /* This is overcomplicated to satisfy unit tests that expect certain codepaths to be called */ - GHRepository repo = from(name.resolve(allowedToManageHooks())) - .firstMatch(withAdminAccess()).orNull(); + Iterable manageableIterable = name.resolve(allowedToManageHooks()); + GHRepository repo = from(manageableIterable).firstMatch(withAdminAccess()).orNull(); - if (!(name.resolve(allowedToManageHooks()).iterator().hasNext())) { + if (!(manageableIterable.iterator().hasNext())) { if (repo == null) { LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage " + "Hooks, also there are no credentials with admin access to manage such hooks",