Added proxy from env support #885
Conversation
|
@valyala Hi, pls take a look |
| // c := &fasthttp.Client{ | ||
| // Dial: FasthttpProxyHTTPDialer(), | ||
| // } | ||
| func FasthttpProxyHTTPDialer() fasthttp.DialFunc { |
There was a problem hiding this comment.
hey! first of all, thanks for your contribution and welcome!
I think we can drop FasthttpProxy prefix from function names, as we already have that in the package name. Also, as we are trying to keep fasthttp as minimalistic as possible, this package looks like a good fit for an addon package. For example, we can create github.com/fasthttp/fasthttpproxy or github.com/fasthttp/httpproxy, and put it there. This would allow us to keep a smaller api footprint. What you think?
There was a problem hiding this comment.
Hi @kirillDanshin !
Added FasthttpProxy prefix to be consistent with current naming in the same package.
I'm ok to move the proxy to the addon. Could you pls create github.com/fasthttp/fasthttpproxy and i will open a merge request there?
I would choose the github.com/fasthttp/fasthttpproxy to distinguis between fasthttp proxy and x/net/httpproxy packets, WDYT?
There was a problem hiding this comment.
@kirillDanshin Could you pls create the repo for this addon? i will move MR there.
There was a problem hiding this comment.
We already have the other proxy code in this repo with similar naming. I don't see why we shouldn't add this one either.
And since it's in a different package it doesn't increase the API footprint of the fasthttp itself.
Moving this code and the other proxy code to github.com/fasthttp/fasthttpproxy sounds nice but I feel like it's a v2 thing when we can introduce breaking changes.
|
|
||
| return func(addr string) (net.Conn, error) { | ||
|
|
||
| proxyURL, err := proxier(&url.URL{Host: addr}) |
There was a problem hiding this comment.
I'm wondering if Scheme should be set to https here if the port is 443?
There was a problem hiding this comment.
Did you test this code? Not setting Scheme makes this always return nil.
See: https://github.com/golang/net/blob/08b38378de702b893ee869b94b32f833e2933bd2/http/httpproxy/proxy.go#L129-L138
There was a problem hiding this comment.
@erikdubbelboer Hi! thanks for the review. You are pointing to the outdated version of the x/net/httpproxy.
here is which one is currently used: https://github.com/golang/net/blob/627f9648deb96c27737b83199d44bb5c1010cbcf/http/httpproxy/proxy.go#L130-L138
| authBarriersLock := sync.RWMutex{} | ||
|
|
||
| return func(addr string) (net.Conn, error) { | ||
|
|
|
Thanks! |
|
@erikdubbelboer Hi! Why this MR was merged? There are still where open questions on the naming, etc? I agree with |
|
@mkorolyov the pull request was good as. This way the naming matches the other functions in that subpackage. When I commented about v2 I meant if we ever release a v2 version of fasthttp with backwards incompatible changes, not a v2 of the fasthttpproxy subpackage. |
Added support for default unix proxy config via envs: