Skip to content
This repository has been archived by the owner on Jan 24, 2019. It is now read-only.

Fix url parse error #365

Merged
merged 1 commit into from
Apr 20, 2017
Merged

Fix url parse error #365

merged 1 commit into from
Apr 20, 2017

Conversation

travisofthenorth
Copy link
Contributor

@travisofthenorth travisofthenorth commented Apr 1, 2017

Fixes this error which I was running into locally:
2017/04/01 14:59:25 http.go:29: FATAL: could not parse "127.0.0.1:4180": parse 127.0.0.1:4180: first path segment in URL cannot contain colon

Update: the above error happens as of go 1.8. This PR removes parsing via url.Parse and parses the scheme manually.

@ploxiln
Copy link
Contributor

ploxiln commented Apr 1, 2017

The square brackets in the help text suggest that the "http://" is supposed to be optional. And, if you test the v2.1 release binary, it works without "http://" specified. So, this is a bug elsewhere in oauth2_proxy, maybe due to a library update.

@travisofthenorth
Copy link
Contributor Author

@ploxiln I'm guessing the release binary was built with go 1.7? Looks like the behavior changed slightly in 1.8 (golang/go#19297). Thoughts? I'm happy to update this PR or close it if 1.8 isn't meant to be supported.

@ploxiln
Copy link
Contributor

ploxiln commented Apr 2, 2017

The previous release was built with go-1.6 (it's in the tarball name). We do intend to support go-1.8 but were not aware of this change to url parsing yet. Thanks for the report and the link to an explanation.

I think that ServeHTTP() should be changed to parse the optional scheme without url.Parse(). All it really does is remove the scheme from the option string if it is present, and specify "tcp" or "unix" as appropriate to net.Listen() (whereas ServeHTTPS() just passes the option string directly to net.Listen(), always "tcp").

@travisofthenorth
Copy link
Contributor Author

@ploxiln how does this look? Tested it manually with:

  • tcp://127.0.0.1:4180
  • http://127.0.0.1:4180
  • //127.0.0.1:4180
  • 127.0.0.1:4180
  • unix:///tmp/foo.sock

@travisofthenorth travisofthenorth changed the title Fix default http address setting Fix url parse error Apr 2, 2017
@ploxiln
Copy link
Contributor

ploxiln commented Apr 3, 2017

That looks OK. I think it could be simplified a bit more, since oauth2_proxy has never accepted //127.0.0.1:4180 ("panic: unknown upstream protocol"). (I tested 2.1 release built with go-1.6 and current master built with go-1.8.)

Anyway, we'll have to wait for a review from a maintainer, I'm not really one of them :)

@travisofthenorth
Copy link
Contributor Author

Sounds good. Thanks for taking a look!

@ploxiln
Copy link
Contributor

ploxiln commented Apr 20, 2017

ping @jehiah
recap: default value for flag --http-address no longer works in go-1.8

@jehiah jehiah added the bug label Apr 20, 2017
@jehiah jehiah merged commit f511cac into bitly:master Apr 20, 2017
@travisofthenorth travisofthenorth deleted the fix/default-http-address branch April 20, 2017 19:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

3 participants