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.

Sign up for GitHub

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

Jump to bottom

web: Add IPv6 support #1176

Merged
merged 1 commit into from
Jan 17, 2020
Merged

web: Add IPv6 support #1176

merged 1 commit into from
Jan 17, 2020

Conversation

CRTified
Copy link
Collaborator

Motivation

This PR closes #1145. It allows hledger-web to be reachable over IPv6, e.g. by calling it like

hledger-web --host ::1

Changes

  • Modify the --host validation in hledger-web/Hledger/Web/WebOptions.hs to allow IPv6 addresses
  • Modifies the default base URL to surround the host in [ and ] if an IPv6 address is used (required for proper IPv6-URLs in the browser)

Problems

@CRTified
Copy link
Collaborator Author

As soon as yesodweb/wai-handlers#2 merges and is available, we can switch to Network.Wai.Handler.Launch.runHostPortFullUrl and solve this problem.
With a local copy of my changes, it already works successful.

@simonmichael I'm not used to the tooling. Do I need to add a constraint for wai-handler-launch to require at least version 3.0.2.5 (the one I PR'd)?

@simonmichael
Copy link
Owner

simonmichael commented Jan 15, 2020 •
edited
Loading

@Amarandus yes, you'll change wai-handler-launch >=1.3 in hledger-web/package.yaml to wai-handler-launch >=3.0.3 or whatever. Your next stack build hledger-web will update hledger-web.cabal accordingly. I'll have to double check that won't cause a problem for the older GHC versions/package sets we support [with make incr-buildtest-all], but it looks like it'll be fine.

@simonmichael simonmichael added web The hledger-web tool. A-WISH Some kind of improvement request, hare-brained proposal, or plea. labels Jan 15, 2020
@CRTified
Copy link
Collaborator Author

As yesodweb/wai-handlers#2 is now merged, I tried updating the package.yaml accordingly.
If I understand it correctly, we have to wait until the next LTS snapshot on stackage is available (which should be anytime soon).

In the meantime, I checked whether IPv4-mapped addresses work, and this is the case:

$ stack exec -- hledger-web --host ::ffff:127.0.0.1
Stack has not been tested with GHC versions above 8.6, and using 8.8.1, this may fail
Stack has not been tested with Cabal versions above 2.4, but version 3.0.0.0 was found, this may fail
17/Jan/2020:20:43:53 +0100 [Info#yesod-core] Application launched @(yesod-core-1.6.17-8khiymix0izHkaDEW7detY:Yesod.Core.Dispatch src/Yesod/Core/Dispatch.hs:163:11)
Serving web UI and API on ::ffff:127.0.0.1:5000 with base url http://[::ffff:127.0.0.1]:5000
This server will exit after 2m with no browser windows open (or press ctrl-c)

@simonmichael
Copy link
Owner

we have to wait until the next LTS snapshot on stackage

Hopefully not. stack will pull newer versions of deps from hackage when it needs to. If you're seeing an error while testing stack install in master, maybe I can help.

@CRTified
web: Add IPv6 and hostname support
4525caf 
This commit introduces IPv6 support (and thus closes simonmichael#1145).
It also allows using local hostnames as a parameter for --host.
For this, multiple things needed to be changed:

 - checkWebOpts is dropped, as the supplied parameter is checked later
 by Network.Socket.getAddrInfo
 - defbaseurl needs to check if : is used in the host, as this indicates
 the usage of an IPv6 address. In this case, the host needs to be
 wrapped in [] for the base URL
 - To allow opening such a modified base URL, runHostPortFullUrl is used
 instead of runhostPortUrl, as it allows opening arbitrary URLs instead
 of a path prefixed with http://127.0.0.1

As checking the host for validity is postponed until the webserver tries
to start, an invalid hostname leads to an exception caused by
Network.Socket.getAddrInfo.
This is still fine, as hledger-web won't start in an undefined state, but
will terminate with a nonzero exit code.
@CRTified CRTified marked this pull request as ready for review January 17, 2020 20:39
@simonmichael simonmichael merged commit 76ffaca into simonmichael:master Jan 17, 2020
@simonmichael
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@CRTified CRTified

Labels
A-WISH Some kind of improvement request, hare-brained proposal, or plea. web The hledger-web tool.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[hledger-web] Lacking IPv6 support
2 participants
@CRTified @simonmichael