Add health check command and document health checks #390
Conversation
brandur
left a comment
brandur
left a comment
There was a problem hiding this comment.
Thanks!
I gotta say that the idea of using an HTTP client to query the health check endpoint internally is a little odd, but I do understand the rationalization. I recently ran into a situation at work where I had no tools available in our streamlined Docker container at all except our program's binaries, so whatever was bundled in there was extremely useful.
@bgentry My thinking is that it's a little unconventional, but probably okay to have something like this. Thoughts?
| subtle.ConstantTimeCompare([]byte(reqUsername), []byte(username)) == 1 && | ||
| subtle.ConstantTimeCompare([]byte(reqPassword), []byte(password)) == 1 | ||
|
|
||
| return isHealthCheck || isValidAuth |
There was a problem hiding this comment.
Doh, was going to ask for a test for this one, but I guess we don't have any existing auth middleware tests. @bgentry there isn't one I'm missing somewhere is there?
There was a problem hiding this comment.
Added tests to the auth middleware including the exemption of healthcheck endpoint
docs/README.md
Outdated
| } | ||
| ] | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Thanks for taking a stab at the docs! This is too much info for the regular README though (they get unwieldy and hard to scan when they're too long). Can we move it to a separate health check doc in docs/?
There was a problem hiding this comment.
Done. Can you check again?
There was a problem hiding this comment.
Yep, thanks for that.
Actually, sorry for the nit, but maybe call it health_checks.md just because we're using the underscore convention elsewhere like state_machine.md in the main project:
https://github.com/riverqueue/river/blob/master/docs/state_machine.md
sallam-dev
force-pushed
the
add-health-check
branch
from
439039b to
c545ec2
Compare
|
Cool, thanks for adding those tests! I'll merge this, but I may refactor things a bit — we should really have those tests running in parallel and with a few other niceties. |
Follows up #390 with a couple tweaks for the tests: * Make auth middleware tests more focused so they only test `authMiddleware` rather than the full server HTTP stack. * Remove use of `t.Setenv` so we can run all tests in parallel. * Use `pathPrefix` in middleware so that only the specific configured prefix is accepted rather than any prefix that might be present. * Use `CamelCaseTestName` convention rather than "test name like this". * Use `testBundle` convention for `setup`.
|
Added a few tweaks in #394. |
2 participants
Motivation
I wanted to use RiverUI container as a standalone and not embed it.
I'm running it on AWS ECS Fargate which sits behind an AWS ALB
Changes Summary
/api/health-checks)No. 2 is necessary since AWS ECS tasks detect the container health via commands like in docker compose. And the AWS ALB will need the health endpoint to determine if it should route requests to the target group. So both approaches are needed for proper health checking across the stack.
Why make the CLI request the health endpoint?
I want to avoid pulling in another "container" dependency like
curlor increase its size.I thought of making the CLI apply the health check logic directly but that doesn't check the actual server health. It will check that the binary can connect to the DB. Which is useless.
Happy to address any feedback to get this merged!