IPv6 support #1696

Merged
merged 5 commits into from Dec 19, 2016

Projects

None yet

6 participants

@kyrias
Contributor
kyrias commented Dec 12, 2016

I've been testing it out in a clean private testing environment with 2 IPv4 and one IPv6 homeservers running, and it seems to be working pretty well so far.

(Supersedes #1689 and #1690)

glyph added some commits Dec 11, 2016
@glyph @kyrias glyph IPv6 support for client.py
This is an (untested) general sketch of how to use wrapClientTLS to implement TLS over IPv6, as well as faster connections over IPv4.
6e18805
@glyph @kyrias glyph IPv6 support for endpoint.py
Similar to matrix-org#1689, but for endpoint.py
9f07f4c
@matrixbot

Can one of the admins verify this patch?

@matrixbot

Can one of the admins verify this patch?

@matrixbot

Can one of the admins verify this patch?

@kyrias
Contributor
kyrias commented Dec 12, 2016

One problem though is that the default bind_address is an empty string which twisted interprets as "listen to all IPv4 addresses", so we're still not IPv6 friendly by default. I'd recommend we switch to use :: as the default bind address, which will cause it to bind to all IPv4 and IPv6 addresses.

@erikjohnston
Member

@matrixbot test this please

@kyrias kyrias Fixup for #1689 and #1690
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
d3bd948
@kyrias
Contributor
kyrias commented Dec 12, 2016

Seems I missed to add the fixed imports originally, so the tests would need to be re-run.

@kyrias
Contributor
kyrias commented Dec 12, 2016

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

uh, is this a known issue?

@erikjohnston
Member

@matrixbot ok to test

@erikjohnston
Member

@kyrias no idea why that build stalled, but I restarted it and now it errors with:

synapse/http/client.py:409:1: E303 too many blank lines (3)

@kyrias kyrias Remove spurious newline
Apparently I just removed the spaces instead...

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
0648e76
@kyrias
Contributor
kyrias commented Dec 12, 2016
org.tap4j.parser.ParserException: Error parsing TAP Stream: Missing TAP Plan.

?

@glyph
Contributor
glyph commented Dec 12, 2016

It may be worth noting that the empty string may cause Twisted to listen on IPv6 addresses as well, depending on your platform; :: might cause you to listen on IPv4 as well, depending on vagaries of your IP stack. This is an ongoing discussion in Twisted-land and it's not really clear how we're going to fix it; given that different platforms have very different ideas about what :: and 0.0.0.0 mean, we might need to add our own special string that means "set IPv6_V6ONLY" or "don't set it".

@ara4n
Member
ara4n commented Dec 12, 2016

For the record: I had a brief IRL discussion with @erikjohnston and @NegativeMjark and others this morning about what we do about ipv6-only HSes. Obviously we risk a major split-brain problem if some servers can't talk to others 'cos they're either ipv6 or ipv4 only. For now, the suggestion is to mandate that servers expose ipv4 publicly (but if any given pair of servers happen to do ipv6 then they'll use it). I guess this is the same semantics as email. If anyone has any smarter solutions then we're all ears.

@kyrias
Contributor
kyrias commented Dec 13, 2016

It would also be a good idea to be able to specify multiple bind_addresses, and I have this implemented uncommitted locally already.

+ return None
+ return SpiderEndpoint(
+ reactor, uri.host, uri.port, self.blacklist, self.whitelist,
+ endpoint=endpoint_factory, endpoint_kw_args=dict(timeout=15),
@ara4n
ara4n Dec 18, 2016 Member

what's the rationale for using dict() rather than {} here? i think the existing code style is to use {}...

@kyrias
kyrias Dec 18, 2016 Contributor

I just didn't change that part, but it seems dict() is used in a bunch of places as well

λ git grep ' dict\(' | wc -l
61

I can certainly change it to {} instead though.

@ara4n
ara4n Dec 18, 2016 Member

looking through the results of that grep, none of those uses seem to be for constants like this, which is only why it jumped out as looking weird. it's trivial, but probably worth fixing. (disclaimer: i'm hardly a python expert, but trying to help fill in whilst @erikjohnston is out)

@glyph
glyph Dec 18, 2016 Contributor

The rationale is as follows:

  1. This dictionary is going to be used as **kw, meaning it needs to be native str mapped to some object.
  2. To facilitate python 3 portability, all modules should slowly be getting as many __future__ imports as possible, to make the py2 environment and the py3 environment consistent. One such import is from __future__ import unicode_literals.
  3. If you rely on "" to produce a native str, this assumption will be broken with unicode_literals imported.
  4. However, dict(**kw) will produce kw unaltered; meaning, it will be a native str even if a quoted string would produce a unicode.

So this is generally a habit I believe one should get into for py3 hygiene.

@ara4n
ara4n Dec 18, 2016 Member

okay, i'm happy to defer (and yield, for that matter) to python wisdom here ;)

@njsmith
njsmith Dec 20, 2016

One such import is from __future__ import unicode_literals

FWIW, Guido says you shouldn't use unicode_literals and they're going to add a warning about this to the official docs. (See recent thread on python-dev.)

@glyph
glyph Dec 20, 2016 Contributor

Hrm. I'm not sure I fully agree with that thread, but definitely unicode_literals is not without its pitfalls. Nevertheless, dict(**kw) is a clearer expression of the intent of "I intend to use this for keywords".

@ara4n
Member
ara4n commented Dec 18, 2016

the multiple bind_addresses PR has ended up at #1709 - thanks @kyrias!

Doing a quick review here, the only thing that screams out as being missing is to kick the dependency for https://github.com/matrix-org/synapse/blob/master/synapse/python_dependencies.py? Otherwise I think I'm happy to merge this as an experimental feature (modulo random stylistic niggle)

@kyrias kyrias was assigned by ara4n Dec 18, 2016
@kyrias kyrias Bump twisted dependency
At least 16.0.0 is needed for wrapClientTLS support.

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
a9c1b41
@ara4n
Member
ara4n commented Dec 19, 2016

going to merge this and see what happens :D thanks again @glyph and @kyrias!

@ara4n ara4n merged commit a58e4e0 into matrix-org:develop Dec 19, 2016

3 of 4 checks passed

Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Merged PR) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kyrias kyrias deleted the kyrias:ipv6 branch Dec 20, 2016
@glyph
Contributor
glyph commented Dec 20, 2016

Really glad to help. (Thanks to Rackspace for sponsoring my time to work on open source infrastructure like this in the Twisted ecosystem... :))

@kyrias kyrias added a commit to kyrias/synapse that referenced this pull request Jan 8, 2017
@kyrias kyrias Revert "Merge pull request #1696 from kyrias/ipv6"
This reverts commit a58e4e0, reversing
changes made to b2f8642.
dfdc3fb
@kyrias kyrias added a commit to kyrias/synapse that referenced this pull request Jan 8, 2017
@kyrias kyrias Revert "Merge pull request #1696 from kyrias/ipv6"
This reverts commit a58e4e0, reversing
changes made to b2f8642.
0cb377c
@kyrias kyrias added a commit to kyrias/synapse that referenced this pull request Jan 8, 2017
@kyrias kyrias Revert "Merge pull request #1696 from kyrias/ipv6"
This reverts commit a58e4e0, reversing
changes made to b2f8642.
3c51eb9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment