[JENKINS-36121] Webhook behaviour

[dominics]

This is a fix for JENKINS-36121 that takes an opinionated approach.

• It disables all instances of owner.onSCMSourceUpdated() on webhooks; leaving it there, even as a fallback, on a busy repo, makes it too easy to hit rate limits. I've seen branch indexing churn and basically never sit idle. We can easily re-enable it as a fallback, however.
• It removes the GitHubWebhookListenerImpl in favour of a new PushGHEventSubscriber (so that we get the webhook payload)
• It makes GBSP depend on the workflow-multibranch plugin, so that we can take more customized actions upon receiving webhooks
• It adds an AbstractGHEventSubscriber to hold common functionality between the PullRequest and Pull subscribers

In the PullRequestGHEventSubscriber, each PR event webhook triggers up to two builds (merged/unmerged), and takes about one Github API request to do so (I think, namely checking the collaborators for .isTrusted?). Pushes to a PR that is already open won't yet trigger a build, I think. But editing the PR, or adding/removing a label should.

In the PushGHEventSubscriber, each push event webhook triggers a build of the relevant branch build. Again, very few, if any, GitHub API requests used.

Altogether, the new behaviour is much better for large repositories than the linear-on-the-number-of-branches/PRs behaviour it used to have.

Problems for review

• Security: I couldn't find any way to tell from github-api whether the webhook is secure/verified/authenticated (via the request signing + secret configured on webhook settings)
  • I've found that if you set a secret on the webhook, and configure one in the global settings for github-api, it will correctly reject badly-signed webhook requests. So, another way of putting it: I can't seem to programatically ensure that a secret was used, and ignore unsigned requests myself.
  • This is okaay; the tools are there to secure the endpoint from my testing, but it's unsecured by default. Which is aggravating. IMHO the github-api plugin should always generate/use a default secret credential if one isn't provided.
  • Of course, that's the existing scheme anyway (it's just an unauthorized webhook can only trigger full branch reindexing; but for the large monolithic repo I work on, that alone would be a DoS via GitHub rate limit after very few triggers)
• I need to sign a CLA I assume
• Jobs are scheduled in a pretty weird way (this static ParameterizedBuildMixin.scheduleBuild2; is that right?). I'm sure I'm missing some much easier way to schedule a multibranch subjob, but I don't know enough about the internals to find it
• I've ignored branch eligibility to a certain extent. I'm not sure if checking for a Jenkinsfile is worth it for individual branches if it doubles the API request rate. Personally, I'm just going to require everyone to merge from master once and then all the incoming webhooks will be for Jenkinsfile branches anyway. I don't mind this trade-off. And you can still use branch exclusions from what I can tell
• No tests. Should I wait for wiremock?
• A good refactor would move more of the logic back into the GithubSCMSource; I've had to open a few too many internal methods on that class up. Probably just a matter of extracting the details of the task, observer, etc.? But this works for me for now. I think I did better in that respect for the Push subscriber

Comments welcome; my first Jenkins contribution, so sorry if there's anything unidiomatic. Consider reviewing commit by commit; I've tried to keep the changes logically grouped.

Edited 2016-09-03T09:16:00+00:00