Skip to content
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

Add Dialer.ProxyConnectHeader #605

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AllenX2018
Copy link

Fixes #479. Add a field ProxyConnectHeader in Dialer for user to costomize the proxy headers

@@ -274,7 +277,7 @@ func (d *Dialer) DialContext(ctx context.Context, urlStr string, requestHeader h
return nil, nil, err
}
if proxyURL != nil {
dialer, err := proxy_FromURL(proxyURL, netDialerFunc(netDial))
dialer, err := proxy_FromURL(proxyURL, &netDialer{d.ProxyConnectHeader, netDial})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should check if the header is set before using it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Custom header information, can be empty

proxy.go Outdated
}

func init() {
proxy_RegisterDialerType("http", func(proxyURL *url.URL, forwardDialer proxy_Dialer) (proxy_Dialer, error) {
return &httpProxyDialer{proxyURL: proxyURL, forwardDial: forwardDialer.Dial}, nil
p, _ := forwardDialer.(*netDialer)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really should not just discard errors here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i fix it

@IngCr3at1on
Copy link

LGTM but someone else must approve.

Just a quick comment: force pushing on a PR makes it difficult to review your new changes to the PR; I'd suggest just pushing followup commits and if you want to squash them after review that's a good idea. In this case it was easy enough to scroll through and find the change but it can be quite cumbersome in large PRs.

@AllenX2018
Copy link
Author

Yes, thanks for the kind reminder

@garyburd garyburd changed the title fix issue #479 Add Dialer.ProxyConnectHeader Dec 19, 2021
@jaitaiwan
Copy link
Member

LGTM - if the merge conflicts can be resolved then we can merge this PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Support custom HTTP headers for CONNECT proxy request
5 participants