-
Notifications
You must be signed in to change notification settings - Fork 7
✨ [http/proxy] Add helpers for proxying requests and responses
#743
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| :sparkles: `[http/proxy]` Add helpers for proxying requests and responses |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| package proxy | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "io" | ||
| "net/http" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/go-http-utils/headers" | ||
|
|
||
| "github.com/ARM-software/golang-utils/utils/commonerrors" | ||
| httpheaders "github.com/ARM-software/golang-utils/utils/http/headers" | ||
| "github.com/ARM-software/golang-utils/utils/reflection" | ||
| "github.com/ARM-software/golang-utils/utils/safecast" | ||
| "github.com/ARM-software/golang-utils/utils/safeio" | ||
| ) | ||
|
|
||
| // ProxyDisallowList describes headers which are not proxied back. | ||
| var ProxyDisallowList = []string{ | ||
| headers.AccessControlAllowOrigin, | ||
| headers.AccessControlAllowMethods, | ||
| headers.AccessControlAllowHeaders, | ||
| headers.AccessControlExposeHeaders, | ||
| headers.AccessControlMaxAge, | ||
| headers.AccessControlAllowCredentials, | ||
| } | ||
|
|
||
| // 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) { | ||
| if reflection.IsEmpty(r) { | ||
| err = commonerrors.UndefinedVariable("request to proxy") | ||
| return | ||
| } | ||
| ctx := r.Context() | ||
| contentLength := determineRequestContentLength(r) | ||
| h := httpheaders.FromRequest(r).AllowList(headers.Authorization) | ||
| if reflection.IsEmpty(proxyMethod) { | ||
| proxyMethod = http.MethodGet | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why does it change it to get if it is not set?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is usually what happens in HTTP. the default method is
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
| proxiedRequest, err = http.NewRequestWithContext(ctx, proxyMethod, endpoint, r.Body) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this does not make the call this is just creating the request object (the configuration object you were suggesting)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not ideal and fairly unnecessary complex but was the only way to cover all the corner-cases I encountered
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is left to the client to follow redirect |
||
| if err != nil { | ||
| err = commonerrors.WrapError(commonerrors.ErrUnexpected, err, "could not create a proxied request") | ||
| return | ||
| } | ||
|
|
||
| if proxiedRequest.ContentLength <= 0 { | ||
| if proxiedRequest.Body == nil || proxiedRequest.Body == http.NoBody { | ||
| if contentLength > 0 { | ||
| proxiedRequest.Body = r.Body | ||
| proxiedRequest.GetBody = r.GetBody | ||
| } else { | ||
| proxiedRequest, err = http.NewRequestWithContext(ctx, proxyMethod, endpoint, convertBody(ctx, r.Body)) | ||
joshjennings98 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if err != nil { | ||
| err = commonerrors.WrapError(commonerrors.ErrUnexpected, err, "could not create a proxied request") | ||
| return | ||
| } | ||
|
Comment on lines
51
to
62
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 contentLength <= 0 { | ||
| 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 | ||
| } | ||
| } | ||
|
|
||
| if contentLength > 0 && proxiedRequest.ContentLength <= 0 { | ||
| proxiedRequest.ContentLength = contentLength | ||
| h.AppendHeader(headers.ContentLength, strconv.FormatInt(contentLength, 10)) | ||
| } | ||
| } | ||
| if contentLength > 0 && contentLength != proxiedRequest.ContentLength { | ||
| err = commonerrors.Newf(commonerrors.ErrUnexpected, "proxied request does not have the same content length `%v` as original request `%v`", proxiedRequest.ContentLength, contentLength) | ||
| return | ||
| } | ||
| h.AppendToRequest(proxiedRequest) | ||
| return | ||
| } | ||
|
|
||
| func determineRequestContentLength(r *http.Request) int64 { | ||
| if reflection.IsEmpty(r) { | ||
| return -1 | ||
| } | ||
| if r.ContentLength > 0 { | ||
| return r.ContentLength | ||
| } | ||
| // Following what was done in https://github.com/luraproject/lura/blob/b9ad9ab654dd6149aeb58a5d6ffe731aba41717e/proxy/http.go#L99C1-L105C4 | ||
| v := r.Header.Values(headers.ContentLength) | ||
| if len(v) == 1 && v[0] != "chunked" { | ||
| if size, err := strconv.Atoi(v[0]); err == nil { | ||
| return safecast.ToInt64(size) | ||
| } | ||
| } | ||
| return -1 | ||
| } | ||
|
|
||
| func convertBody(_ context.Context, body io.Reader) io.Reader { | ||
| if body == nil || body == http.NoBody { | ||
| return http.NoBody | ||
| } | ||
| switch v := body.(type) { | ||
| case *bytes.Buffer: | ||
| return body | ||
| case *bytes.Reader: | ||
| return body | ||
| case *strings.Reader: | ||
| return body | ||
| default: | ||
| // see example https://github.com/luraproject/lura/blob/b9ad9ab654dd6149aeb58a5d6ffe731aba41717e/proxy/http.go#L73 | ||
| buf := new(bytes.Buffer) | ||
| _, err := buf.ReadFrom(v) | ||
| if err != nil { | ||
| return http.NoBody | ||
| } | ||
| if b, ok := body.(io.ReadCloser); ok { | ||
| _ = b.Close() | ||
| } | ||
| return buf | ||
| } | ||
| } | ||
|
|
||
| // ProxyResponse proxies a response to a writer. Headers are sanitised and some headers such as CORS headers will be removed from the response. | ||
| func ProxyResponse(ctx context.Context, resp *http.Response, w http.ResponseWriter) (err error) { | ||
| if w == nil { | ||
| err = commonerrors.UndefinedVariable("response writer") | ||
| return | ||
| } | ||
| if reflection.IsEmpty(resp) { | ||
| err = commonerrors.UndefinedVariable("response") | ||
| return | ||
| } | ||
| h := httpheaders.FromResponse(resp) | ||
| h.Sanitise() | ||
|
|
||
| var written int64 | ||
| _, err = safeio.CopyDataWithContext(ctx, resp.Body, w) | ||
| if resp.Body != nil && resp.Body != http.NoBody { | ||
| written, err = safeio.CopyDataWithContext(ctx, resp.Body, w) | ||
| if err != nil { | ||
| err = commonerrors.DescribeCircumstance(err, "failed copying response body") | ||
| } | ||
| } | ||
| if written >= 0 { | ||
| h.AppendHeader(headers.ContentLength, strconv.FormatInt(written, 10)) | ||
| } | ||
| h.RemoveHeaders(ProxyDisallowList...) | ||
| h.AppendToResponse(w) | ||
| w.WriteHeader(resp.StatusCode) | ||
| return | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,164 @@ | ||
| package proxy | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "io" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "strconv" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/go-faker/faker/v4" | ||
| "github.com/go-http-utils/headers" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/ARM-software/golang-utils/utils/commonerrors" | ||
| "github.com/ARM-software/golang-utils/utils/commonerrors/errortest" | ||
| "github.com/ARM-software/golang-utils/utils/safecast" | ||
| "github.com/ARM-software/golang-utils/utils/safeio" | ||
| ) | ||
|
|
||
| func TestProxy(t *testing.T) { | ||
| content := faker.Paragraph() | ||
| path := faker.URL() | ||
| password := faker.Password() | ||
| tests := []struct { | ||
| request *http.Request | ||
| }{ | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), io.NopCloser(strings.NewReader(content))), | ||
| }, | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), strings.NewReader(content)), | ||
| }, | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), io.NopCloser(bytes.NewReader([]byte(content)))), | ||
| }, | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), bytes.NewReader([]byte(content))), | ||
| }, | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), io.NopCloser(bytes.NewBuffer([]byte(content)))), | ||
| }, | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), bytes.NewBuffer([]byte(content))), | ||
| }, | ||
| } | ||
| for i := range tests { | ||
| test := tests[i] | ||
| t.Run(strconv.Itoa(i), func(t *testing.T) { | ||
| req := test.request | ||
| req.Header.Set(headers.AccessControlAllowOrigin, faker.Word()) | ||
| req.Header.Set(headers.XHTTPMethodOverride, http.MethodPut) | ||
| req.Header.Set(headers.Authorization, password) | ||
| assert.NotEqual(t, req.URL.String(), path) | ||
| _, err := ProxyRequest(nil, http.MethodPost, "/") | ||
| errortest.AssertError(t, err, commonerrors.ErrUndefined) | ||
| preq, err := ProxyRequest(req, " ", path) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, preq) | ||
| assert.Equal(t, path, preq.URL.String()) | ||
| assert.Equal(t, http.MethodGet, preq.Method) | ||
| assert.NotEmpty(t, preq.Header.Get(headers.AccessControlAllowOrigin)) | ||
| assert.NotEmpty(t, preq.Header.Get(headers.Authorization)) | ||
| assert.NotZero(t, preq.ContentLength) | ||
| resp := generateTestResponseBasedOnRequest(t, preq) | ||
| defer func() { | ||
| if resp != nil { | ||
| _ = resp.Body.Close() | ||
| } | ||
| }() | ||
| w := httptest.NewRecorder() | ||
| require.NoError(t, ProxyResponse(context.Background(), resp, w)) | ||
| proxiedResp := w.Result() | ||
| defer func() { _ = proxiedResp.Body.Close() }() | ||
| assert.Empty(t, w.Header().Get(headers.AccessControlAllowOrigin)) | ||
| assert.Equal(t, http.MethodPut, w.Header().Get(headers.XHTTPMethodOverride)) | ||
| assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
| responseContent, err := safeio.ReadAll(context.Background(), proxiedResp.Body) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, content, string(responseContent)) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestEmptyResponse(t *testing.T) { | ||
| path := faker.URL() | ||
| tests := []struct { | ||
| request *http.Request | ||
| }{ | ||
| { | ||
| httptest.NewRequest(http.MethodGet, faker.URL(), nil), | ||
| }, | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), http.NoBody), | ||
| }, | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), io.NopCloser(http.NoBody)), | ||
| }, | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), bytes.NewReader(nil)), | ||
| }, | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), io.NopCloser(bytes.NewBuffer(nil))), | ||
| }, | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), strings.NewReader("")), | ||
| }, | ||
| { | ||
| request: httptest.NewRequest(http.MethodGet, faker.URL(), io.NopCloser(strings.NewReader(""))), | ||
| }, | ||
| } | ||
| for i := range tests { | ||
| test := tests[i] | ||
| t.Run(strconv.Itoa(i), func(t *testing.T) { | ||
| req := test.request | ||
| assert.NotEqual(t, req.URL.String(), path) | ||
| preq, err := ProxyRequest(req, http.MethodPost, path) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, preq) | ||
| assert.Equal(t, path, preq.URL.String()) | ||
| assert.Equal(t, http.MethodPost, preq.Method) | ||
| assert.Zero(t, preq.ContentLength) | ||
|
|
||
| resp := generateTestResponseBasedOnRequest(t, preq) | ||
| defer func() { | ||
| if resp != nil { | ||
| _ = resp.Body.Close() | ||
| } | ||
| }() | ||
| w := httptest.NewRecorder() | ||
| require.NoError(t, ProxyResponse(context.Background(), resp, w)) | ||
| require.NoError(t, err) | ||
| returnedResp := w.Result() | ||
| assert.LessOrEqual(t, returnedResp.ContentLength, safecast.ToInt64(0)) | ||
| assert.Equal(t, http.StatusOK, returnedResp.StatusCode) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func loopTestHandler(t *testing.T, w http.ResponseWriter, r *http.Request) { | ||
| t.Helper() | ||
| require.NotNil(t, r) | ||
| require.NotNil(t, w) | ||
| for k, v := range r.Header { | ||
| for h := range v { | ||
| w.Header().Add(k, v[h]) | ||
| } | ||
| } | ||
| written, err := safeio.CopyDataWithContext(r.Context(), r.Body, w) | ||
| require.NoError(t, err) | ||
| w.Header().Add(headers.ContentLength, strconv.FormatInt(written, 10)) | ||
| w.WriteHeader(http.StatusOK) | ||
| } | ||
|
|
||
| func generateTestResponseBasedOnRequest(t *testing.T, r *http.Request) *http.Response { | ||
| t.Helper() | ||
| require.NotNil(t, r) | ||
| w := httptest.NewRecorder() | ||
| loopTestHandler(t, w, r) | ||
| return w.Result() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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