Change https://golang.org/cl/295390 mentions this issue: net/http/httputil: add a ModifyRequest method to ReverseProxy

I do not support this change, literally Director is not the same as ModifyRequest, I want ModifyRequest to be like an optional handler, used before or after Director.

How would that work? What would ModifyRequest's role be? Disregarding the name, what would it be doing if it was called before the director? How would it help being called after it?

Director is currently there to modify the request in such a way that it is sent to a different location than the original one, which is something the new method can also do, since it is basically the same thing, called in the same place, just having a return value now. So in the spirit of having only one way of doing things, I believe Director would need to be deprecated if my proposal is accepted.

I was initially thinking of a different approach, adding a ValidateRequest method that would be called before the director, that would return a bool or error to indicate if the proxying should occur, and leave the director untouched. But then thought that having the two decoupled might mean some state can theoretically change between ValidateRequest ending and Director starting execution, that could invalidate some assumptions, and that at least some of the data required in one would also be required in the other, causing duplicate processing and code. So I ended up with merging the two into the proposed ModifyRequest.

Since the director's role is to modify the request, and there is already a ModifyResponse method, this is the best name I could come up with for the new method (well, this and DirectorEx), though I do agree Director conveys the method's purpose far better than my proposal. But preserving that wasn't an option since the function's signature has changed and it would break backwards compatibility.

I am in no way saying this is the best way to do it, just the best way I could come up with. If there is a more fitting approach, I'm certainly open to suggestions and would like to help make it happen, since not being able to interrupt the director without doing some gymnastics is an itch I want to scratch.

I understand that ModifyRequest can achieve the same purpose because it is not appropriate to add a method name, and it is not appropriate to mark Director as deprecated, and it is easier to modify the request to reach the destination by wrapping it before passing it to the reverse proxy than to modify the response of the reverse proxy.

And that is what this proposal is about, making it easier to use ReverseProxy without wrappers/middleware. For anything more complex than changing the destination host and/or basic path rewrites, one requires a wrapper that first of all validates the request can be proxied (e.g. if routing decisions are made by checking some headers or cookies, first step would be to make sure the required headers/cookies are present, or that the URL follows a certain format/matches certain regexps etc). Then the wrapper could modify the request directly, leaving nothing for the director to do, or if the wrapper does validation only, the director would then have to extract the same information again from path/headers/cookies, or be passed that information by the wrapper, e.g. via context. But that would be just to give the director something to do, because it obviously makes sense to have the middleware prepare the request.

But that would mean a case could be made the other way around, to remove the Director completely, because it's only useful in the most basic scenarios, and even then a wrapper can be used to modify the request. I believe replacing it with a method that can interrupt the flow adds more flexibility to ReverseProxy, as it could be used without wrappers in more advanced situations as well.

CC @neild @bradfitz

You can effectively cancel a request in a Director hook by modifying it to be invalid. For example, req.Method = "".

That said, explicitly returning an error from the request-modification hook is cleaner and more obvious. Also, I think ModifyRequest is a better name than Director (clearly states what the hook does, parallels ModifyResponse).

I think adding ModifyRequest is reasonable.

ModifyRequest supersedes and is redundant with Director, but I don't think we should mark Director as deprecated, since code which must work with older versions of Go will still need to use Director.

If we do add ModifyRequest, then my question is how it should interact with Director: What do we do if both are set? Neither? (Historically, not setting Director has been a panic.)

I don't see what this adds. The caller can look at the request and reject it or modify it before calling the ReverseProxy.ServeHTTP func.

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

This proposal doesn't add new functionality, indeed, it just makes working with ReverseProxy easier (usecases that previously required a wrapper could be handled directly by/in the new method). If the desire is for it to live side-by-side with the current Director, then it adds even less, I agree, as there would now be two ways of doing the same thing, one of which could interrupt the flow and return an error, while the other would need a bit of logic around it to achieve the same purpose.

The more I think about it, the more convinced I am that the two changes should come as a whole - a new method that can stop the flow and marking the old one as deprecated, in effect replacing it. Though I'll admit I'm saying that under the assumption that nothing marked as deprecated actually gets removed in Go 1, to preserve backwards compatibility (and the proposed patch actually accounts for that, still calling the Director as a second option).

I don't think we should deprecate an API that has been around for as long as ReverseProxy has existed to make some cases easier.

Given that adding a ModifyRequest at best makes some cases slightly easier (rejecting a request before passing it to ReverseProxy.ServeHTTP doesn't seem difficult), I don't think we should do this.

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

No change in consensus, so declined.
— rsc for the proposal review group