reverseproxy: Active health checks request body option #6520
Conversation
| // if body is provided, create a reader for it, otherwise nil | ||
| var requestBody io.Reader | ||
| if h.HealthChecks.Active.Body != "" { | ||
| requestBody = strings.NewReader(h.HealthChecks.Active.Body) | ||
| } | ||
|
|
There was a problem hiding this comment.
It may be useful to run it through the replacer 🤔
There was a problem hiding this comment.
Agreed. Though in reality it would only let {env.*} and {file.*} work, not much else. There's no request context available here in active health checks because they happen in their own goroutines.
There was a problem hiding this comment.
Oh yeah I did think about doing that, but then I guess I forgot all about it again :-)
I'll give it go.
There was a problem hiding this comment.
OK, I've add a replacer to the body in my latest commit. I've moved the initialization of replacer used for the headers up, and reused that. Is that a good solution ?
992108c to
a2fd07a
Compare
There was a problem hiding this comment.
Thank you for working on this!
It feels a little weird to set a body without a Content-Type, but maybe we can see if anyone needs that feature and then add a way to customize Headers. (Honestly it's starting to feel a lot like the respond directive, i.e. the static_response middleware, and I wonder if we would benefit from just using that instead. But that's a matter for another discussion.)
|
@mholt we have |
Hmm that's a very good point, and you are probably right that in a weeks time I'll probably find out that I need to set a I just checked http.Request, and I don't think anything of value can't be set after this PR is merged. I mean the protocol version is not something I usually touch, and we probably don't want to explicitly set the content length either. The only maybe could be trailers? |
a2fd07a to
fdf435f
Compare
|
@francislavoie Ah you're right, I thought that was for headers to check for healthy status, but it seems we are using that to set them. |
|
Hi, just a heads-up, this is not in the current docs, nor is |
|
Ah thanks, @ngotchac -- been a bit busy lately; would either @jbro or you be available to send a quick PR to https://github.com/caddyserver/website? |
|
Yep! Thanks |
|
PR here caddyserver/website#470 |
|
Thanks for the prompt reply! You beat me to it @jbro , I was about to open the PR 😛 |
Changes
Added the field
Bodyto theActiveHealthChecksstruct, which optionally sets the body of the request used for the health check requests.Also added
Caddyfilesupport for the same option under the namehealth_request_body.Rational
After adding the option to setting the HTTP method used for active health checks it turns out it's also helpful to set the body. Eg when using the POST method on a JSON RPC service with an empty body may return a
400 Bad Request, instead of a200 OK. Being able to set the body to a valid NOOP RPC call enables a "deeper" health check, than just accepting the 400 status.