search

snipe / snipe-it

A free open source IT asset/license management system
https://snipeitapp.com
GNU Affero General Public License v3.0
11.1k stars 3.18k forks source link

Add a health route #8931

Open phenixdotnet opened 3 years ago

phenixdotnet commented 3 years ago

Server (please complete the following information):

Is your feature request related to a problem? Please describe. We have deployed snipeit on a k8s cluster with an health check to /login page which is used by our load balancers to test if the application is up and running. My issue is: each health check done by the load balancers (3 LB check per 5sc :D ) create a new session which fill the session storage and ultimately bring down the application because there is no more free space.

Describe the solution you'd like I suggest to add a new route (may be /healthz) which the only purpose is to response 200 ok with a minimal json payload: 

{ "status": "ok" }

and which doesn't start a new session

If you're ok with this approach, I can submit a PR for this :)

Thanks !

welcome[bot] commented 3 years ago

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

rcmcdonald91 commented 3 years ago

Second. HAProxy is DoS'ing my SnipeIT install as well. I had to turn off health checking.

I say submit the PR request and see what @snipe says 👍

uberbrady commented 3 years ago

I can't see why we wouldn't take a PR for a health-check that lived at /health (doesn't need to have the z imho). I would try and have it check that Laravel is up and running, but I think I wouldn't want it to check the database (you don't want to have a database reboot mark all of your nodes as 'unhealthy').

How many days does it take before the session store fills up? There's a built-in "session lottery" system in Laravel that should clean out old sessions; it has a 1/50 chance of firing - but probably wouldn't delete a sufficiently new session. You could certainly enlarge the storage you're using for sessions, but that seems like quite a lot to do for such a small problem.

Another option would be that you could configure a Redis container for the session store, but that's a whole other box to have to administer...

I'm looking a little further into the 'lottery' system right now, hoping to report something further.

uberbrady commented 3 years ago

You can probably also update the SESSION_LIFETIME value to something smaller than our default (12000, a little more than 8 days). The session lottery looks like it only deletes stuff older than that.

I guess another option might be to mount a specialized volume onto the sessions directory, but that sounds silly to me too.

phenixdotnet commented 3 years ago

I can't see why we wouldn't take a PR for a health-check that lived at /health (doesn't need to have the z imho). I would try and have it check that Laravel is up and running, but I think I wouldn't want it to check the database (you don't want to have a database reboot mark all of your nodes as 'unhealthy').

Yes totally agree, the health check should be very lite and should not check external dependencies

You can probably also update the SESSION_LIFETIME value to something smaller than our default

Yes that what we done as a workaround (setting it to 2h does the trick)

PR is WIP, I will finish it ASAP.