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

cli: use proxy from environment #2316

Merged
merged 1 commit into from
Jul 8, 2021
Merged

cli: use proxy from environment #2316

merged 1 commit into from
Jul 8, 2021

Conversation

tskinn
Copy link
Contributor

@tskinn tskinn commented Jun 28, 2021

Summary

The existing implementation of pomerium-cli does not respect the http proxy env vars so here we add the default http.Transport behavior back with ProxyFromEnvironment.

Related issues

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@tskinn tskinn requested a review from a team as a code owner June 28, 2021 16:22
@tskinn tskinn requested a review from calebdoxsey June 28, 2021 16:22
@CLAassistant
Copy link

CLAassistant commented Jun 28, 2021

CLA assistant check
All committers have signed the CLA.

@codeclimate
Copy link

codeclimate bot commented Jun 28, 2021

Code Climate has analyzed commit 8235da4 and detected 0 issues on this pull request.

View more on Code Climate.

@tskinn
Copy link
Contributor Author

tskinn commented Jun 30, 2021

Hey @calebdoxsey what needs to happen to get this PR approved? Pomerium is an awesome project that we would love to use but not being able to use a proxy is a deal breaker for us.

@@ -120,6 +120,7 @@ func (client *AuthClient) runOpenBrowser(ctx context.Context, li net.Listener, s
}

transport := &http.Transport{
Proxy: http.ProxyFromEnvironment,
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense to me. I wonder if a broader fix would be to use a Clone'd version of the default http transport http.DefaultTransport which also has some likely sane default timeouts and settings beyond Proxy?

https://golang.org/pkg/net/http/#Transport.Clone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that sounds good to me. You mean something like this right?

transport := http.DefaultTransport.Clone()
transport.TLSClientConfig = client.cfg.tlsConfig

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep!

@desimone
Copy link
Contributor

@tskinn -- This makes sense to me. Had one suggestion, but otherwise I think we can move forward.

@desimone desimone added the enhancement New feature or request label Jun 30, 2021
@coveralls
Copy link

coveralls commented Jun 30, 2021

Coverage Status

Coverage decreased (-0.01%) to 64.596% when pulling 8235da4 on tskinn:master into 163e538 on pomerium:master.

@desimone desimone requested a review from calebdoxsey July 8, 2021 04:30
@desimone desimone changed the title authclient - use proxy from environment cli: use proxy from environment Jul 8, 2021
@desimone desimone merged commit 93e7358 into pomerium:master Jul 8, 2021
@desimone
Copy link
Contributor

desimone commented Jul 8, 2021

Thanks @tskinn !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants