Skip to content
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

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ab14-tech
Copy link

  • Add Transport override option for active health checks.
  • This enables the active healthcheck to use http while the actual upstream uses https.
  • This enables more powerful active healthcheck capabilities

Related Discussion

Related Issues

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


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.

Comment on lines 93 to 95
// The transport to use for health checks. If not set, the handler's transport is used
Transport http.RoundTripper `json:"transport,omitempty"`

Copy link
Member

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:

https://github.com/caddyserver/caddy/blob/99073eaa33af62bff51c31305e3437c57d936284/modules/caddyhttp/reverseproxy/reverseproxy.go#L74-78

Copy link
Member

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.

Copy link
Author

@ab14-tech ab14-tech Jan 24, 2025

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)

Copy link
Member

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.:

// 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:

// 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:"-"`

Copy link
Author

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.

Copy link
Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants