-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
WIP: Custom Webhooks #19307
base: main
Are you sure you want to change the base?
WIP: Custom Webhooks #19307
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Thats essentially what the custom http hooks do. While I am in favor of writing bridge software, it seems to have not caught on for one reason or another. Instead we keep adding more and more hard-coded webhooks. Running external commands was mostly a shim for people who didn't want to run extra services for a bridge. |
I have no objection to introduce a local command mechanism (although I think it's not the solution for cloud native, if there is a chance, we could use some other methods ....). (update: I misread some context, I only worry about the local command. Indeed HTTP is cool and fine!)
|
This is both local command and http. As for form, that was how I had imagined letting webhook makers define a limited UI to be passed to their webhook. Regarding pasting configs, while I suppose that's possible, I was thinking this would more admin controlled rather than user. Perhaps details should be discussed in Discord or the forum? I'm happy to figure out a good solution, as our webhook code continues to get more complex and messy as we add more hook types and even different hook scopes. |
Signed-off-by: jolheiser <[email protected]>
Signed-off-by: jolheiser <[email protected]>
Signed-off-by: jolheiser <[email protected]>
Signed-off-by: jolheiser <[email protected]>
Signed-off-by: jolheiser <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #19307 +/- ##
==========================================
- Coverage 47.39% 47.26% -0.14%
==========================================
Files 956 961 +5
Lines 133115 133894 +779
==========================================
+ Hits 63093 63282 +189
- Misses 62393 62947 +554
- Partials 7629 7665 +36
Continue to review full report at Codecov.
|
How about to store them into database? |
Signed-off-by: jolheiser <[email protected]>
Signed-off-by: jolheiser <[email protected]>
Store what in the database? |
The custom webhooks yaml content. |
Why? A custom webhook definition still requires an image and potentially an executable. Unless we want to get rid of the executable idea, but imo there's benefit to not needing to spin up another server for a webhook translation. |
Images could be svg or an embed one into |
Why not having the web hook specifications in the repo under .gitea? And isn't this like some kind of CI feature? |
I think we can implement HTTP only in this PR and discuss local command one in other PRs. Why I prefer store them into database, because I think puting the custom webhooks in files may make multiple Gitea instances work difficult. |
So we're putting a yaml config in the db. Pasting yaml and an image is arguably less friendly than being able to unzip (or git pull) to a directory, but that might depend on the person. Having things on disk is similar to how template customizations work. Is this going to be a new page on the admin dashboard, then? |
Since we have more and more configurations in ini file, I think we should move configurations out of ini file as possible. I think yes. We could move some webhook settings from ini to database(#18058) and have a configuration UI there including which webhooks should be enabled. There is a button to create a custom webhook type, then we can go to a new page. We can upload an image and have a yaml edit box, there are example and some template variables explanation there and yml syntax highlight? |
Sounds like a lot of work for the user to gain a few UI boxes imo Am HTTP-only version means there's very little difference between this and the existing generic gitea webhook. The main advantage is being able to define a limited UI, but if users need to click through pages and mannually copy/paste yaml to gain that UI, is there much advantage? |
Regarding the payload customization part, mentioned in
and in #19307 (comment)
Did you consider a system like bloblang ?
The URL, method and headers (authorization) have to be configured elsewhere. There are (at least) 2 security aspects to study before choosing to go this way (with this processor or any other document mapping tool):
|
Regarding the security aspects, they are only relevant if the processor is accessible by the end-user. If this is limited to the instance owner, it shouldn't be an issue. From a quick look, the current custom webhooks could be replaced by such a system (it also supports unit-testing ;) |
I've been playing a bit with this. Here is how it would look like for matrix.Create ( // Create implements PayloadConvertor Create method
func (m *MatrixPayloadUnsafe) Create(p *api.CreatePayload) (api.Payloader, error) {
exe, err := newBloblangExecutor(`
let repoLink = html_link(this.repository.html_url, this.repository.full_name)
let refName = this.ref.ref_end_name()
let refURL = this.repository.html_url + match {
this.ref.has_prefix("refs/heads/") => "/src/branch/"
this.ref.has_prefix("refs/tags/") => "/src/tag/"
_ => "/src/commit/"
} + $refName.path_escape_segments()
let refLink = html_link($refURL, $refName)
let text = "[%s:%s] %s created by %s".format($repoLink, $refLink, this.ref_type, this.sender.login)
root.body = $text.html_to_markdown()
root.msgtype = "TODO"
root.format = "org.matrix.custom.html"
root.formatted_body = $text
`)
if err != nil {
return nil, err
}
return exe.Query(p)
} Let me know if you think this is worth exploring further. I can make a draft PR to share the whole code. |
@oliverpool What would be the advantage over JS in this case? |
I tried to make a comparison here #1089 (comment) (so as to not pollute this pull-request) |
Signed-off-by: jolheiser <[email protected]>
@jolheiser looks very good! I did some local tests and I think jsonnet is a promising choice 👍 I played a bit with your latest commit and added a (basic) test: Following this experiment, I had the following thoughts:
local form = import 'form.libsonnet';
"https://" + form.host + "/api/post-message" I am very excited about this, thank for your hard work! |
Hey, thanks for giving my code a test! I hadn't quite finished up or tested my changes. Glad to see I'm on the right track, though!
I'm not tied to "custom" as the name, but I think "file-based" might not accurately convey that they are "programmable" in a sense. Perhaps calling them
I thought about this, but overall I like that
The only reason I had it as a dedicated column was as an admittedly premature optimization in case any queries/migrations/etc needed to happen in the future for specific webhooks.
Ah, I will need to look a bit further.
This makes sense, and I think in general the custom webhooks likely need a bit of refactoring anyways. I don't love having a global map, so I may swap them for an interface. That way they can live in-mem, on disk, or however someone else wants to add support in the future.
Ah, yes, this will be removed. It was there because previously this would have been running against a bridge service, but now it is simply transformative. |
_This is a different approach to #20267, I took the liberty of adapting some parts, see below_ ## Context In some cases, a weebhook endpoint requires some kind of authentication. The usual way is by sending a static `Authorization` header, with a given token. For instance: - Matrix expects a `Bearer <token>` (already implemented, by storing the header cleartext in the metadata - which is buggy on retry #19872) - TeamCity #18667 - Gitea instances #20267 - SourceHut https://man.sr.ht/graphql.md#authentication-strategies (this is my actual personal need :) ## Proposed solution Add a dedicated encrypt column to the webhook table (instead of storing it as meta as proposed in #20267), so that it gets available for all present and future hook types (especially the custom ones #19307). This would also solve the buggy matrix retry #19872. As a first step, I would recommend focusing on the backend logic and improve the frontend at a later stage. For now the UI is a simple `Authorization` field (which could be later customized with `Bearer` and `Basic` switches): ![2022-08-23-142911](https://user-images.githubusercontent.com/3864879/186162483-5b721504-eef5-4932-812e-eb96a68494cc.png) The header name is hard-coded, since I couldn't fine any usecase justifying otherwise. ## Questions - What do you think of this approach? @justusbunsi @Gusted @silverwind - ~~How are the migrations generated? Do I have to manually create a new file, or is there a command for that?~~ - ~~I started adding it to the API: should I complete it or should I drop it? (I don't know how much the API is actually used)~~ ## Done as well: - add a migration for the existing matrix webhooks and remove the `Authorization` logic there _Closes #19872_ Co-authored-by: Lunny Xiao <[email protected]> Co-authored-by: Gusted <[email protected]> Co-authored-by: delvh <[email protected]>
Refactor the webhook logic, to have the type-dependent processing happen only in one place. --- ## Current webhook flow 1. An event happens 2. It is pre-processed (depending on the webhook type) and its body is added to a task queue 3. When the task is processed, some more logic (depending on the webhook type as well) is applied to make an HTTP request This means that webhook-type dependant logic is needed in step 2 and 3. This is cumbersome and brittle to maintain. Updated webhook flow with this PR: 1. An event happens 2. It is stored as-is and added to a task queue 3. When the task is processed, the event is processed (depending on the webhook type) to make an HTTP request So the only webhook-type dependent logic happens in one place (step 3) which should be much more robust. ## Consequences of the refactor - the raw event must be stored in the hooktask (until now, the pre-processed body was stored) - to ensure that previous hooktasks are correctly sent, a `payload_version` is added (version 1: the body has already been pre-process / version 2: the body is the raw event) So future webhook additions will only have to deal with creating an http.Request based on the raw event (no need to adjust the code in multiple places, like currently). Moreover since this processing happens when fetching from the task queue, it ensures that the queuing of new events (upon a `git push` for instance) does not get slowed down by a slow webhook. As a concrete example, the PR #19307 for custom webhooks, should be substantially smaller: - no need to change `services/webhook/deliver.go` - minimal change in `services/webhook/webhook.go` (add the new webhook to the map) - no need to change all the individual webhook files (since with this refactor the `*webhook_model.Webhook` is provided as argument)
Since #29145 merged, this PR can merge and do some refactors. |
This is a draft of custom webhooks.
It will close #14693 as well, as it includes both HTTP and binary-based webhooks.
This should also resolve #1089, at least as closely as I can think of without a true plugin system.
A configuration looks like this:
Executable
Executables are executed relative to the path the custom webhook lives in, which is currently
CUSTOM_PATH/webhooks
HTTP
Each custom webhook requires 2-3 files, namely an
image.png
, aconfig.yml
, and possibly an executable if required.On startup, Gitea scans the custom webhook directory (again, currently set to
CUSTOM_PATH/webhooks
) and adds them to a map of webhooks.The payload sent to either type of webhook (via
stdin
for executables) isFor executables, any headers are set in the environment as the header name, ie
X-Gitea-Signature
Notes: