This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

I am bike shedding, but httputil.Rewriter looks like it should be an interface.

I do not see discussion in #50580, so I will ask here: what is the benefit to the func(*struct{}) and (void?) methods, over having httputil.ReverseProxy.Rewrite be a struct with fields to be configured? It seems more complex, which I think is a bit of a double edged sword, but I have not seen this pattern elsewhere in the stdlib. My alternative would look like this in your example:

// This is the equivalent of httputil.NewSingleHostReverseProxy(someURL).
proxy := httputil.ReverseProxy{
        Rewrite: httputil.Rewrite{
                SetXForwarded: true,
                SetURL: someURL
        }
}

Rewriter definitely isn't an interface. There's only one implementation of it, and we want to be able to add methods to it in the future.

The proposed Rewrite hook, like the Director hook that it would supersede, is a user-provided function that can perform any amount of modification of a proxied request before it is forwarded. It can't be replaced by a fixed list of operations.

Rewriter definitely isn't an interface. There's only one implementation of it, and we want to be able to add methods to it in the future.

Should have been more clear: pkg.Doer, to me, looks like an interface because it ends in -er. We may want to consider a different name.

The proposed Rewrite hook, like the Director hook that it would supersede, is a user-provided function that can perform any amount of modification of a proxied request before it is forwarded. It can't be replaced by a fixed list of operations.

Yep, sorry the example pigenholed my thinking; that makes sense.

That said, why then would we not make it Rewrite: func(*http.Request) *http.Request? Having the methods for setURL or forward headers as utility functions could be useful outside this usecase.

You could also have two inputs if void is desirable: func(in, out *http.Request). It could also be worth making in an http.Request, no pointer, to suggest/enforce this immutability requirement.

func(*http.Request) *http.Request doesn't address #50580. If the input request contains hop-by-hop headers, we can't tell if a header in the output was added by the rewriter; if the input doesn't contain hop-by-hop headers, the rewriter has no way to look for ones it may be interested in. We need to pass both the inbound and outbound requests to the user hook.

Wrapping both requests up in a Rewriter type gives us a good place to hang additional functions such as SetURL. While the ones in this proposal could also be stand-alone functions, making them methods of a type passed to Rewrite is good for discoverability, especially given the overly-broad API surface in httputil. It also gives us the option of adding methods in the future that customize the proxy behavior in other ways.

func(*http.Request) *http.Request doesn't address #50580. If the input request contains hop-by-hop headers, we can't tell if a header in the output was added by the rewriter; if the input doesn't contain hop-by-hop headers, the rewriter has no way to look for ones it may be interested in. We need to pass both the inbound and outbound requests to the user hook.

No that makes sense, there has already been some preprocessing done to create a populated Rewriter.OutReq, thus we would need the two parameter form:

// This is the equivalent of httputil.NewSingleHostReverseProxy(someURL).
proxy := httputil.ReverseProxy{
        Rewrite: func(in, out *http.Request) { ...}
}

Wrapping both requests up in a Rewriter type gives us a good place to hang additional functions such as SetURL. While the ones in this proposal could also be stand-alone functions, making them methods of a type passed to Rewrite is good for discoverability, especially given the overly-broad API surface in httputil. It also gives us the option of adding methods in the future that customize the proxy behavior in other ways.

Yeah, if we see a clear need to add new struct fields or methods that include magic [recte internal apis], I can see this being useful; it just feels like a novel design pattern akin to grpc input structs, which feel decidedly non go-like. I just prefer the obvious nature of my explicit format.

Have we resolved the objections to Rewriter?
Should we tentatively try to land it early in Go 1.20?
What other open issues would we be able to close if we add Rewriter?

Perhaps there's a better name than Rewriter, but in the absence of a concrete idea for something I think it's fine. (RewriteState? RewriteContext?)

Landing this early in 1.20 would be nice.

Looking through issues that mention ReverseProxy:

Primary motivation for this proposal (and sufficient to justify it, I believe, since this is a security concern):

• net/http/httputil: ReverseProxy can remove headers added by Director #50580

Issues related to forwarding headers (X-Forwarded-For, etc.). Making the addition of X-Forwarded headers explicit makes it easier to adjust this behavior in a backwards-compatible fashion.

• net/http/httputil: add X-Forwarded-Proto and X-Forwarded-Host by default #50465
• net/http/httputil: Reverse Proxy X-Forwarded-For include Port #31095
• net/http/httputil: support RFC 7239 Forwarded header in ReverseProxy #30963

Issues related to NewSingleHostReverseProxy preserving the inbound request's Host header. We could make the Rewriter.SetURL method clear the inbound Host. Even if we preserve the current behavior of preserving the host, Rewrite makes it much simpler for the user to customize the proxy behavior, since it doesn't require wrapping the Director function created by NewSingleHostReverseProxy.

• net/http/httputil: ReverseProxy does not set host header correctly #28168
• net/http/httputil: ReverseProxy.ServeHTTP should empty the outgoing request's Host once the outgoing request is cloned #33861

Possibly better name than Rewriter: ProxyRequest.

Change https://go.dev/cl/407214 mentions this issue: net/http/httputil: add ReverseProxy.Rewrite

CL 407214 contains an implementation of this proposal, with a few changes from the original post:

• Renamed Rewriter to ProxyRequest.
• When using a Rewrite func,
  • The default behavior is to send no X-Forwarded-* headers (stripping ones from the inbound request).
  • ProxyRequest.SetXForwarded will set X-Forwarded-*.
  • To append to the client's X-Forwarded-For header, copy the inbound header to the outbound request and call ProxyRequest.SetXForwarded.
• ProxyRequest.SetURL sets the outbound request's Host header.

The X-Forwarded-* changes preference more secure behavior (not to trust the client's headers).

The SetURL change addresses #28168, while leaving it simple to get the current behavior of NewSingleHostReverseProxy back if desired.

If we're clearing X-Forwarded-*, then should Forwarded also be preemptively cleared?

Yes, I'm actually already clearing Forwarded in the CL but neglected to mention it specifically.

FWIW when I said "objections to Rewriter" I meant this proposal generally, not the name. We should use whatever name you think is clearest; Rewriter is fine if so.

Does anyone object to trying this in Go 1.20?

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Change https://go.dev/cl/450515 mentions this issue: doc/go1.20: add release notes for net/http and net/http/httputil