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

Allow 'bind' in config to specify bind address #38

Merged
merged 3 commits into from Nov 27, 2018

Conversation

mrvdb
Copy link
Collaborator

@mrvdb mrvdb commented Nov 26, 2018

Minimal changes, definitely WIP

To resolve:

  • how to support dualstack when not using localhost?
  • net/http package uses string, mentions IP address instead of bind,
    need info.

For issue #5

Minimal changes, definitely WIP, to resolve:

- how to support dualstack when not using localhost?
- net/http package uses string, mentions IP address instead of bind,
  need info.
@mrvdb mrvdb requested a review from thebaer November 26, 2018 15:54
app.go Outdated Show resolved Hide resolved
@thebaer
Copy link
Member

thebaer commented Nov 26, 2018

Besides the one mentioned change, this looks good. I'm not sure about the net/http stuff off the top of my head, and otherwise for testing I'd like to defer to you or anyone else with a dualstack config. I just don't have the setup to experiment and figure out the best way forward there.

@mrvdb
Copy link
Collaborator Author

mrvdb commented Nov 26, 2018

I've read up a bit on the methods. The net/http/Listen method is basically the same as net/Listen with the network set to tcp, meaning dualstack tcp.
In practice this means if you use localhost the bind will probably go to ipv6 instead of ipv4 (which is correct)
So, we're good to go I think. The only thing to be aware of is the single binding limitation. For example there is no way to bind it to localhost on ipv4 and ipv6 simultaneously. The library is a bit higher level than I thought.

@BenOvermyer
Copy link
Collaborator

This looks fine to me.

We should really get some integration tests (e.g., InSpec) set up to automatically test variations of this config, but that is probably outside the scope of this PR.

@thebaer thebaer added this to the 0.5 milestone Nov 27, 2018
@thebaer
Copy link
Member

thebaer commented Nov 27, 2018

Okay great, I'll merge this now.

@thebaer thebaer merged commit fb18b8f into writefreely:master Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants