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

feat: added HTTP method option for push monitor #1991

Merged

Conversation

0tt0sson
Copy link
Contributor

Description

Some services use POST as their preferred callback HTTP method. This PR adds HTTP method option for push monitor so it can handle all the HTTP methods.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots

image

@0tt0sson 0tt0sson marked this pull request as ready for review August 17, 2022 15:34
@louislam louislam modified the milestone: 1.18.1 Oct 2, 2022
@louislam louislam added the question Further information is requested label Oct 3, 2022
@louislam
Copy link
Owner

louislam commented Oct 3, 2022

Thanks for the pr. In my opinion, it should be standardized, so I am afraid I cannot merge this pr.

@Computroniks
Copy link
Contributor

Thanks for the pr. In my opinion, it should be standardized, so I am afraid I cannot merge this pr.

Do you mean standardized as in only GET requests? I don't really see a problem with giving the option of using different request types.

@louislam
Copy link
Owner

louislam commented Oct 3, 2022

Thanks for the pr. In my opinion, it should be standardized, so I am afraid I cannot merge this pr.

Do you mean standardized as in only GET requests? I don't really see a problem with giving the option of using different request types.

Yes, only GET request.

The first reason is that, the push url is using query string ?msg=123&ping=123 to pass values, which is very common and easy to use in GET request. While in POST request, it is not common. A lot of HTTP libraries will eventually convert their POST request data into x-www-form-urlencoded or multipart/form-data instead of query string. Developers who don't aware this must be confused.

The second reason is that, it should a part of API docs in the future, I think dynamic HTTP methods should be wired.

So in my opinion, drawbacks > benefits.

On the other hand, what exact services which does provide POST request only but not GET request?

@ErmakovDmitriy
Copy link

Just my humble opinion: if Uptime Kuma supports HTTP POST as "Liveness" probe, it will allow me to use it as an endpoint to Prometheus-Alertmanager Watchdog alert.

In the Prometheus-Alertmanager stack, the Watchdog can be used to monitor the monitoring pipeline itself (if the Watchdog alert is present, then we know that Prometheus can contact Alertmanager, Alertmanager can contact "outside world").

@Computroniks
Copy link
Contributor

I see where your coming from with the query string being used. I think that this is just an issue with this implementation as opposed to the actual idea of using a post request. It would be far better if the details were passed in the post body.

For the docs, openapi supports multiple methods for each path and it not uncommon to see such things, especially with a REST API.

If this PR were to be altered so that a separate post handler is used instead of the .all option and params are passed in the request body instead of query strings, would you be open to accepting it?

@0tt0sson
Copy link
Contributor Author

0tt0sson commented Oct 4, 2022

I use Duplicacy that can send a POST call on backup completion.

The PR was created with as few changes and reusing as much existing code as possible to keep it simple.
I'm open to change it to only accept GET or POST if it preferred.

@svengerber
Copy link

I came across this limitation for configuring an uptime kuma push monitor integration with the watchdog alert from alertmanager.
Alertmanager always sends POST request to webhook urls.

It would be great to have a push monitor in uptime kuma that accepts POST requests.

For the time beeing, I have written a small middleware converting the POST to GET requests, that can be run as a lightweight sidecar to alertmanager. We are currently using this internally as a workaround.
Feel free to try it out.
https://github.com/natrontech/alertmanager-uptime-kuma-push

@ErebusBat
Copy link

FWIW Healthchecks.io does have a standardized Push API: https://healthchecks.io/docs/http_api/

HEAD, GET, and POST requests methods. The HTTP POST requests can optionally include diagnostic information in the request body. If the request body looks like a UTF-8 string, Healthchecks.io stores the request body (limited to the first 100KB for each received ping).

I recently installed https://github.com/favonia/cloudflare-ddns which supports the healthchecks API and hence will not work with Uptime Kuma.

Perhaps a decent compromise would be to support all three HTTP methods, but if you want to submit data (other than success or failure) then you must use a GET?

@deefdragon
Copy link
Contributor

@louislam I very strongly believe this should be merged in (merge conflicts resolved of course) and ask you to reconsider. The fact that svengerber has gone and made a program (which others are have been using) to handle JUST this issue for alert-manager is indicative of how desired a feature it is.

While in POST request, [query parameters are] not common

Query parameters, the request method, and the request body are all fundamentally distinct parts in a request, and should not impact each other's usage. I have worked in many a project that uses query parameters in get and post requests (along with in patch, delete etc.

Developers who don't aware this must be confused.

I feel you are vastly underestimating the number of people who are using tooling that they can not really control (IE alertmanager or external services sending webhooks) in favor of potential developer confusion over what are standardized features of an http request.

A lot of HTTP libraries will eventually convert their POST request data into x-www-form-urlencoded or multipart/form-data instead of query string

While it may be better to allow for the data to be passed in the body when using a post etc. request, accepting using query parameters now, and potentially allowing both in the future would, I think, be good adherence to the robustness principal anyway.

At a fundamental level, this feature adds a lot of benefit for little added complexity, and (IMO) little confusion.

@favonia
Copy link

favonia commented Oct 27, 2023

I recently installed https://github.com/favonia/cloudflare-ddns which supports the healthchecks API and hence will not work with Uptime Kuma.

@ErebusBat While waiting for the resolution of this PR, I (the auther of the quoted DDNS tool) just implemented the specific support for Uptime Kuma, so it should be working now... (but you need to use the new environment variable; see the README).

However, in general I think it's a good idea to accept requests other than GET requests, at least the POST ones. One can even argue that a ping request here is not idempotent and the ping should be POST instead GET. That is, POST is making it absolutely clear to any proxy in the middle that (1) it can have important effects on the server (and it does) and (2) things should not be cached, ever. It also has the benefit that misclicking on the link (which is possible in a modern terminal emulator) will not accidentally ping the monitor. All in all I think the benefit of allowing POST requests is significant given the small code change. I also hope @louislam could reconsider the decision.

@chakflying chakflying added the area:monitor Everything related to monitors label Dec 2, 2023
@CommanderStorm CommanderStorm added the needs:review this PR needs a review by maintainers or other community members label May 19, 2024
@github-actions github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 19, 2024
@github-actions github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 21, 2024
@github-actions github-actions bot removed the needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again label May 23, 2024
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

First of all, I am fine with allowing more HTTP-Methods, but not fine with the rest of the changes, as they seem not fully thought out (or tested, see comment about the likely missing migration)

=> I would merge that line

I see problems with the others:

  • Why would a user want to only use a specific method for a push monitor? => I don't see a reason to restrict this
  • I also don't think that we need to change our examples for this

Note

I am going to disregart the api-docs argument previously in the issue as I don't think this route would need to be documented.
The examples we show after saving are likely enough..

extra/push-examples/bash-curl/index.sh Outdated Show resolved Hide resolved
extra/push-examples/bash-curl/index.sh Outdated Show resolved Hide resolved
extra/push-examples/javascript-fetch/index.js Outdated Show resolved Hide resolved
extra/push-examples/javascript-fetch/index.js Outdated Show resolved Hide resolved
server/routers/api-router.js Outdated Show resolved Hide resolved
@0tt0sson
Copy link
Contributor Author

First of all, I am fine with allowing more HTTP-Methods, but not fine with the rest of the changes, as they seem not fully thought out (or tested, see comment about the likely missing migration)

=> I would merge that line

I see problems with the others:

  • Why would a user want to only use a specific method for a push monitor? => I don't see a reason to restrict this
  • I also don't think that we need to change our examples for this

Note

I am going to disregart the api-docs argument previously in the issue as I don't think this route would need to be documented. The examples we show after saving are likely enough..

I reduced this PR to the minimal changes needed to get this feature working.

@CommanderStorm
Copy link
Collaborator

Thanks for the change to the monitor! 🎉

Note

This PR is part of the v2.0 merge window => see #4500 for the bugs that need to be addressed before we can ship this release ^^

All changes in this PR are small and uncontroversial ⇒ merging with junior maintainer approval

@CommanderStorm CommanderStorm merged commit 1100782 into louislam:master May 24, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:monitor Everything related to monitors needs:review this PR needs a review by maintainers or other community members question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants