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

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

Open
syndbg opened this issue Apr 11, 2019 · 8 comments
Open

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

syndbg opened this issue Apr 11, 2019 · 8 comments
Labels
Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@syndbg
Copy link

syndbg commented Apr 11, 2019

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: #31406 (comment)

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

syndbg added a commit to syndbg/go that referenced this issue Apr 11, 2019
Adds a ModifyRequest hook (field) to the ReverseProxy struct.

It aims to be a simplified and more recommended hook that you would want
to use if you plan to retain default reverse proxy behavior from Director,
but further extend it for specific cases a la vhost reverse proxying.

Fixes golang#31406
@gopherbot
Copy link

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

@syndbg syndbg changed the title httputil: addd ModifyRequest hook httputil: add ModifyRequest hook Apr 11, 2019
@agnivade agnivade changed the title httputil: add ModifyRequest hook proposal: net/httputil: add ModifyRequest hook Apr 11, 2019
@gopherbot gopherbot added this to the Proposal milestone Apr 11, 2019
@bcmills
Copy link
Contributor

bcmills commented Apr 11, 2019

CC @bradfitz

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 11, 2019
@bradfitz bradfitz changed the title proposal: net/httputil: add ModifyRequest hook proposal: net/http/httputil: add ModifyRequest hook Apr 11, 2019
@bradfitz
Copy link
Contributor

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
Copy link
Author

syndbg commented Apr 11, 2019

@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
Copy link
Contributor

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.

@syndbg syndbg changed the title proposal: net/http/httputil: add ModifyRequest hook documentation: net/http/httputil: add example for reuse of Director Apr 15, 2019
@gopherbot
Copy link

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

@bcmills bcmills changed the title documentation: net/http/httputil: add example for reuse of Director doc: net/http/httputil: add example for reuse of Director Apr 16, 2019
@syndbg
Copy link
Author

syndbg commented Apr 18, 2019

@andybons andybons removed the Proposal label Oct 7, 2019
@andybons andybons removed this from the Proposal milestone Oct 7, 2019
@odeke-em
Copy link
Member

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

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants