-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
add Transport override option for active health checks #6802
base: master
Are you sure you want to change the base?
Conversation
ab14-tech seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
// The transport to use for health checks. If not set, the handler's transport is used | ||
Transport http.RoundTripper `json:"transport,omitempty"` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accepting the PR is something to be discussed, but this isn't how to implement custom transport. See here for how it's done in the reverse proxy handler:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point, this could be better off as a plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the quick feedback / thoughts
@mohammed90 I can take a look at how the reverse proxy handler implements its custom transport and try to emulate here.
@mholt can you expand on what you mean by making this a plugin? (I see a few ways of achieving more custom, active health check so want to make sure i'm understanding)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An http.RoundTripper
can't really be directly configured from JSON. So you define a module field instead, e.g.:
caddy/modules/caddyhttp/reverseproxy/reverseproxy.go
Lines 75 to 78 in 99073ea
// Configures the method of transport for the proxy. A transport | |
// is what performs the actual "round trip" to the backend. | |
// The default transport is plaintext HTTP. | |
TransportRaw json.RawMessage `json:"transport,omitempty" caddy:"namespace=http.reverse_proxy.transport inline_key=protocol"` |
Then in Provision()
, the module is initialized and converted into an http.RoundTripper:
caddy/modules/caddyhttp/reverseproxy/reverseproxy.go
Lines 239 to 259 in 99073ea
// start by loading modules | |
if h.TransportRaw != nil { | |
mod, err := ctx.LoadModule(h, "TransportRaw") | |
if err != nil { | |
return fmt.Errorf("loading transport: %v", err) | |
} | |
h.Transport = mod.(http.RoundTripper) | |
// enable request buffering for fastcgi if not configured | |
// This is because most fastcgi servers are php-fpm that require the content length to be set to read the body, golang | |
// std has fastcgi implementation that doesn't need this value to process the body, but we can safely assume that's | |
// not used. | |
// http3 requests have a negative content length for GET and HEAD requests, if that header is not sent. | |
// see: https://github.com/caddyserver/caddy/issues/6678#issuecomment-2472224182 | |
// Though it appears even if CONTENT_LENGTH is invalid, php-fpm can handle just fine if the body is empty (no Stdin records sent). | |
// php-fpm will hang if there is any data in the body though, https://github.com/caddyserver/caddy/issues/5420#issuecomment-2415943516 | |
// TODO: better default buffering for fastcgi requests without content length, in theory a value of 1 should be enough, make it bigger anyway | |
if module, ok := h.Transport.(caddy.Module); ok && module.CaddyModule().ID.Name() == "fastcgi" && h.RequestBuffers == 0 { | |
h.RequestBuffers = 4096 | |
} | |
} |
And the resulting http.RoundTripper is stored here, for example:
Transport http.RoundTripper `json:"-"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this makes sense. I updated the PR to emulate , but removed the requestBuffer / fastcgi bit as I don't think it's relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the detailed reply. (when you said 'plugin' I was thinking of an entirely separate way to configure and run a custom active health checker, but that is a separate conversation probably)
http
while the actual upstream useshttps
.Related Discussion
Related Issues