caddyserver / caddy

Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS
https://caddyserver.com
Apache License 2.0
55.45k stars 3.91k forks source link

Add Placeholders for forward_proxy_url #6395

Open ImpostorKeanu opened 2 weeks ago

ImpostorKeanu commented 2 weeks ago

Hi,

I worked with y'all to have the reverse_proxy.transport.forward_proxy_url field added to Caddy a few months ago and it's working just fine, but I failed apply placeholder values.

To make it more useful, I've branched and applied a patch here.

Happy to make a PR, but it seems like I made things more complicated than necessary last time around. Thoughts?

Here's how the change looks:

    // negotiate any HTTP/SOCKS proxy for the HTTP transport
    var proxy func(*http.Request) (*url.URL, error)
    if h.ForwardProxyURL != "" {
        logger := caddyCtx.Logger()
        proxy = func(r *http.Request) (*url.URL, error) {
            // retrieve the replacer from the context.
            repl, ok := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
            if !ok {
                err := errors.New("failed to obtain replacer from request")
                logger.Error(err.Error())
                return nil, err
            }

            // apply placeholders to the value
            // note: h.ForwardProxyURL should never be empty at this point
            s := repl.ReplaceAll(h.ForwardProxyURL, "")
            if s == "" {
                logger.Error("forward_proxy_url was empty after applying placeholders",
                    zap.String("initial_value", h.ForwardProxyURL),
                    zap.String("final_value", s),
                    zap.String("hint", "check for invalid placeholders"))
                return nil, errors.New("empty value for forward_proxy_url")
            } else if pUrl, err := url.Parse(s); err != nil {
                logger.Warn("failed to derive transport proxy from forward_proxy_url")
                return nil, err
            } else {
                logger.Debug("setting transport proxy url", zap.String("url", s))
                return pUrl, nil
            }
        }
    } else {
        proxy = http.ProxyFromEnvironment
    }

    rt := &http.Transport{
        Proxy:                  proxy,
        DialContext:            dialContext,
        MaxConnsPerHost:        h.MaxConnsPerHost,
        ResponseHeaderTimeout:  time.Duration(h.ResponseHeaderTimeout),
        ExpectContinueTimeout:  time.Duration(h.ExpectContinueTimeout),
        MaxResponseHeaderBytes: h.MaxResponseHeaderSize,
        WriteBufferSize:        h.WriteBufferSize,
        ReadBufferSize:         h.ReadBufferSize,
    }
francislavoie commented 2 weeks ago

A PR is certainly welcome, it's the easiest way to review and iterate on the change.

Makes sense to me, just one nitpick about the if chain - when each condition returns, there's no need for else, just do two ifs in a row and then the final case can be unwrapped from the else (and un-indented).