✨ [http/proxy] Add helpers for proxying requests and responses
#743
Conversation
| contentLength := determineRequestContentLength(r) | ||
| h := httpheaders.FromRequest(r).AllowList(headers.Authorization) | ||
| if reflection.IsEmpty(proxyMethod) { | ||
| proxyMethod = http.MethodGet |
There was a problem hiding this comment.
why does it change it to get if it is not set?
There was a problem hiding this comment.
this is usually what happens in HTTP. the default method is GET https://en.wikipedia.org/wiki/Post/Redirect/Get
There was a problem hiding this comment.
Fair enough, I didn't know about that.
It still seems like a proxy should not do that, that Post/Redirect/Get thing looks like it is more about allowing page refreshing without form resubmission which is different to proxying.
But I will leave the decision up to you
There was a problem hiding this comment.
And they introduced 307 and 308 because the original ones were causing so many issues for people.
I think to me, a proxy is different from a redirect. I see a proxy like a middleman so it is just part of the normal request therefore it shouldn't change behavior, whereas a redirect seems like a more explicit change to the destination.
There was a problem hiding this comment.
this Post/Redirect/Get was more to give an example. this should never happen but I preferred that the proxy would be attempted rather than a 500 being returned
joshjennings98
left a comment
joshjennings98
left a comment
There was a problem hiding this comment.
Just some minor comments
| } | ||
|
|
||
| // ProxyRequest proxies a request to a new endpoint. The method can also be changed. Headers are sanitised during the process. | ||
| func ProxyRequest(r *http.Request, proxyMethod, endpoint string) (proxiedRequest *http.Request, err error) { |
There was a problem hiding this comment.
I have typically built functions like this with an object (struct) input for the configuration because they've always grown over time to add headers, rewriting, redirect following, etc, although I appreciate that might not be the norm in Go.
There was a problem hiding this comment.
this is exactly what http.Request is : it contains all the information for making a request. It does not do the request, That is the role of the client
| if contentLength > 0 { | ||
| proxiedRequest.Body = r.Body | ||
| proxiedRequest.GetBody = r.GetBody | ||
| } else { | ||
| proxiedRequest, err = http.NewRequestWithContext(ctx, proxyMethod, endpoint, convertBody(ctx, r.Body)) | ||
| if err != nil { | ||
| err = commonerrors.WrapError(commonerrors.ErrUnexpected, err, "could not create a proxied request") | ||
| return | ||
| } |
There was a problem hiding this comment.
Behaviourally I don't really understand what is going on here (could be lack of experience). Why will it perform a re-request, also why is the proxied request body replaced with the original request body?
There was a problem hiding this comment.
Here, I am trying to consider all the weird edge cases which can happen because of go implementation and also trying to reduce the amount of copying so that there is no penalty in performance
| if reflection.IsEmpty(proxyMethod) { | ||
| proxyMethod = http.MethodGet | ||
| } | ||
| proxiedRequest, err = http.NewRequestWithContext(ctx, proxyMethod, endpoint, r.Body) |
There was a problem hiding this comment.
If the origin returns a redirect, say 301, will this natively follow the 301 to resolution or just return the 301 back to the client to follow?
There was a problem hiding this comment.
this does not make the call this is just creating the request object (the configuration object you were suggesting)
It is rewriting the endpoint path and method but it also needs to "clone" quite a few things manually such as headers and body
There was a problem hiding this comment.
you need to look into the implementation of the newRequest to see what is actually done. It is trying to do things a bit like https://cs.opensource.google/go/go/+/refs/tags/go1.25.3:src/net/http/request.go;l=386 but the request object does not expose all the fields and so, these are weird workarounds to make sure everything is correctly set
There was a problem hiding this comment.
this is not ideal and fairly unnecessary complex but was the only way to cover all the corner-cases I encountered
There was a problem hiding this comment.
It is left to the client to follow redirect
joshjennings98
left a comment
joshjennings98
left a comment
There was a problem hiding this comment.
I like the comments
Description
Those helpers should ease proxying requests and responses and are inspired from the lura project
Test Coverage