doc: net/http/httputil: add example for reuse of Director

[syndbg]

Not a bug, it's a API change proposal documentation enhancement:

Rationale

Since go1.12 enhanced further the ReverseProxy implementation a closed source library (and many more open-source ones) that provide a reverse proxy for HTTP and WS can be deprecated.

The only difference between the closed source library was that I had a more convenient hook to modify a request that allowed me to focus only on the VHOST logic.

The change here would noticeably reduce the boilerplate and make developers avoid re-implementing logic and private function(s) that Directory already implements/uses.

Update: As per discussion, documentation is going to be enhanced. Ref: https://github.com/golang/go/issues/31406#issuecomment-482354227

So, in terms of Golang code

Current state:

forwardProxy := httputil.NewSingleHostReverseProxy(target)
forwardProxy.Director = func(req *http.Request) {
    // NOTE: Just a copy of `ReverseProxy`'s default `Director`
    req.URL.Scheme = target.Scheme
    req.URL.Host = target.Host
    // NOTE: `singleJoiningSlash` must also be defined again by developer
    req.URL.Path = singleJoiningSlash(target.Path, req.URL.Path)
    if targetQuery == "" || req.URL.RawQuery == "" {
        req.URL.RawQuery = targetQuery + req.URL.RawQuery
    } else {
        req.URL.RawQuery = targetQuery + "&" + req.URL.RawQuery
    }
    if _, ok := req.Header["User-Agent"]; !ok {
        // explicitly disable User-Agent so it's not set to default value
        req.Header.Set("User-Agent", "")
    }
    // NOTE: End of copied

    // NOTE: Custom developer logic just begins to start here
    ...
}
forwardProxy.Transport = &http.Transport{
    TLSClientConfig: &tls.Config{
        InsecureSkipVerify: true,
    },
}

(Not needed) With an addition of ModifyRequest hook

forwardProxy := httputil.NewSingleHostReverseProxy(target)
forwardProxy.ModifyRequest = func(req *http.Request) {
    // NOTE: Custom developer logic just begins to start here
    ...
}
forwardProxy.Transport = &http.Transport{
    TLSClientConfig: &tls.Config{
        InsecureSkipVerify: true,
    },
}

Documentation to be updated with

forwardProxy := httputil.NewSingleHostReverseProxy(target)
originalDirector := forwardProxy.Director
forwardProxy.Director = func(req *http.Request) {
    originalDirector(req)
    // NOTE: Custom developer logic just begins to start here
    ...
}
...

In pros/cons based on what I see (with respect to the fact that I'm just a Golang-using developer):

Pros

• Less boilerplate
• No need to re-implement logic and re-define private functions used in reverse_proxy.
• Less error-prone since the developer only needs to think what logic to add instead of checking the existing implementation of Director.
• Not a breaking-change in API.

Cons

* More than one way to do things. * Needs a bit better documentation to signify the difference between Director and ModifyRequest.

ref: #31393

cc: @agnivade

[gopherbot]

Change https://golang.org/cl/171577 mentions this issue: httputil: add ModifyRequest hook

[bcmills]

CC @bradfitz

[bradfitz]

This doesn't enable anything new. It only seems to add another way to do things and requires a bunch more docs. Maybe you could just add an example to the docs showing how to wrap a Director func to do the modifications of the request before or after an existing Director.

[syndbg]

@bradfitz

Maybe you could just add an example to the docs showing how to wrap a Director func to do the modifications of the request before or after an existing Director.

Sounds, interesting.

If I understand your point correctly,

forwardProxy := httputil.NewSingleHostReverseProxy(target)
originalDirector := forwardProxy.Director
forwardProxy.Director = func(req *http.Request) {
    originalDirector(req)
    // NOTE: Custom developer logic just begins to start here
    ...
}
...

Certainly not the prettiest thing out there, but it sure gets the job done and I think everyone can be happy about it.

From what I see it solves the:

Cons:
* More than one way to do things.
* Needs a bit better documentation to signify the difference between Director and ModifyRequest.

@bradfitz Just to be sure that I don't misunderstand the contribution flow here, I'll:

1. Rename the issue and content
2. Update the referenced PR's title and patch to only update the docs

[bradfitz]

Just to be sure that I don't misunderstand the contribution flow here, I'll:

1. Rename the issue and content
2. Update the referenced PR's title and patch to only update the docs

Sounds good to me.

[gopherbot]

Change https://golang.org/cl/172020 mentions this issue: net/http/httputil: add example for reuse of Director

[syndbg]

@bradfitz ping https://go-review.googlesource.com/c/go/+/171577

[odeke-em]

Thank you @syndbg, I've added some feedback to your CL but we are almost there, please take a look.