Pull Sonarr commit 'New: /ping endpoint for verifying that Sonarr is running and able to access it's DB'
Pull https://github.com/Sonarr/Sonarr/commit/75fc550a3fb44e528ca3c311e83075f79f987a36
git cherry-pick -ex 75fc550a3fb44e528ca3c311e83075f79f987a36
This commit has 2 conflict(s)
You can ask me to close all, open pr here or open pr everywhere
- [ ] - include https://github.com/Radarr/Radarr/issues/6787
Would like to see this one added, thanks!
Not a fan of this one.
Not a fan of this one.
Why's that? The issue linked to Sonarr is a very valid reason to support an endpoint like this.
See: https://github.com/Sonarr/Sonarr/issues/4766
I'd personally like to have a ping endpoint without needing to use the API key. Happy to hear alternative views against such an endpoint though.
The current method is having to use something like /api/system/status which requires the API key, this API key needs to be stored securely in whatever client is making the request. This also has to be replicated for any other Arr app so that's multiple API keys needing to be stored securely. None of the information from this endpoint is really needed for basic health check purposes, it's really about getting a 200 OK status code from a reliable endpoint and that's it.
Monitoring the interface or TCP port might work, but it's not really testing the core app itself i.e. database connection OK, if you use third party authentication before something like Sonarr/Radarr you can't do anything like that anyway.
The ping endpoint is essentially designed for this but without the authentication layer. It is also more lightweight than querying /api/system/status given the information from this endpoint isn't really needed for automated test. This ping endpoint returns 200 "OK" for success and I assume a HTTP 500 error with "Error" given any further information i.e. stacktrace could reveal sensitive data, which I believe was originally included but since removed on these grounds.
Seems useful for monitoring without the hassle of needing API keys for this simple check.
I don't think we'd ever want expose something totally unauthenticated
Monitoring the interface or TCP port might work, but it's not really testing the core app itself i.e. database connection OK, if you use third party authentication before something like Sonarr/Radarr you can't do anything like that anyway.
In my SRE experience, application healthchecks should not test the database for connectivity, that is a another system that usually has healthchecks of its own but since sqlite is embedded here that could be argued either way.
I would suggest putting the healthcheck on another port, one that is not exposed anywhere outside the container if database connectivity checks was a requirement. This would probably not be the easiest solution...
Normal: http://radarr:7878
Unauthenticated healthcheck: http://radarr:3000/healthz
Obviously healthchecks work best for container deployment methods otherwise people might complain of another port being expose if deployed on their Windows OS, so it would need to be optional and disabled by default via CLI args or whatever.
I don't think we'd ever want expose something totally unauthenticated
I think a simple unauthenticated (non-database called) /healthz, /ping or whatever endpoint which returns anything if the app is up is more than sufficient and no different that the login page being exposed.
I have plans to stop the login page from using a db connection as well. not a fan of that either because it can also lead to someone looping db hits over and over and over just like this would
Any movement on this? Having a simple /ping endpoint is usefull for monitoring.
Any movement on this? Having a simple /ping endpoint is usefull for monitoring.
Negative.
Any movement on this? Having a simple /ping endpoint is usefull for monitoring.
I need to make a few modifications to cache responses from DB for this endpoint for a short period of time to prevent the ability to load the server by hammering this endpoint. Then it should be going in.
It's on the list.
Awesome, thank you for the update