chore: create proxy instance once per provider#267
Conversation
ssncferreira
left a comment
ssncferreira
left a comment
There was a problem hiding this comment.
Overall LGTM, just some questions to make sure I understand this change 🙂
| reqPath: "http://client-host/chat", | ||
| reqRemoteAddr: "not-a-socket-address", | ||
| reqHeaders: http.Header{ | ||
| "X-Forwarded-For": {"1.1.1.1"}, |
There was a problem hiding this comment.
Why isn't this header in the expectHeader set?
There was a problem hiding this comment.
As test case name suggest: omits_forwarded_for_when_remote_addr_is_not_parseable
reqRemoteAddr is set to not proper value. This is weird edge case that probably will never happen but I think it is better to point it out.
There was a problem hiding this comment.
Right, but 1.1.1.1 is a valid address; it should still be kept in the header, no?
There was a problem hiding this comment.
I'm not exactly sure why library decides to drop the header but maybe due to a security concerns?
If RemoteAddr / last hop is unknown it is better to drop whole header then forward it with missing hop.
Maybe I should remove this test as it tests library rather then our code.
There was a problem hiding this comment.
Yeah, I think this is a very edge case and not sure it can even occur in production. I would suggest either removing the test, or if we decide to keep it add a comment explaining this for future reference
pawbana
force-pushed
the
pb/passthough-proxy-per-route
branch
from
b81eeff to
1334e6b
Compare
ssncferreira
left a comment
ssncferreira
left a comment
There was a problem hiding this comment.
LGTM 💪 thank you for addressing the comments!
| reqPath: "http://client-host/chat", | ||
| reqRemoteAddr: "not-a-socket-address", | ||
| reqHeaders: http.Header{ | ||
| "X-Forwarded-For": {"1.1.1.1"}, |
There was a problem hiding this comment.
Yeah, I think this is a very edge case and not sure it can even occur in production. I would suggest either removing the test, or if we decide to keep it add a comment explaining this for future reference
2 participants
Changes
newPassthroughRouterso it created proxy per provider not on each request.Replaced
Directorinhttputil.ReverseProxywithRewriteas Director is being deprecated in next go version.Fixes: #187