Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Adapt the default config to bind on IPv6.#2232

Closed
14mRh4X0r wants to merge 1 commit into
matrix-org:developfrom
14mRh4X0r:listen-ipv6-default
Closed

Adapt the default config to bind on IPv6.#2232
14mRh4X0r wants to merge 1 commit into
matrix-org:developfrom
14mRh4X0r:listen-ipv6-default

Conversation

@14mRh4X0r
Copy link
Copy Markdown
Contributor

Most deployments are on Linux, so this would actually bind on both IPv4 and IPv6. Resolves #1886.

This change would additionally prevent timeouts for domain names with both A and AAAA records, and allows IPv6-only homeservers to connect to servers with an AAAA record.

@matrixbot
Copy link
Copy Markdown
Member

Can one of the admins verify this patch?

2 similar comments
@matrixbot
Copy link
Copy Markdown
Member

Can one of the admins verify this patch?

@matrixbot
Copy link
Copy Markdown
Member

Can one of the admins verify this patch?

@silkeh
Copy link
Copy Markdown
Contributor

silkeh commented May 18, 2017

Side note: on BSD-variants (including macOS) this may bind on IPv6 only, depending on the net.inet6.ip6.v6only sysctl setting. By default this is false on macOS, and true on FreeBSD/OpenBSD. The latter need to uncomment the 0.0.0.0 line as indicated.

@ara4n
Copy link
Copy Markdown
Member

ara4n commented May 18, 2017

yup, the v6only thing is why we haven't dones this before. that said, i'm unsure why we can't just bind on 0.0.0.0 as well as :: (ie do two binds...)

@ara4n
Copy link
Copy Markdown
Member

ara4n commented May 18, 2017

i suspect the right fix for this is to listen twice, but fix the bind code so that a failing bind isn't considered a terminal error

@14mRh4X0r
Copy link
Copy Markdown
Contributor Author

I think it's safe to say that almost all Synapse installations are on Linux (where V6ONLY is disabled by default, so :: binds on 0.0.0.0 as well), and for other operating systems there are clear instructions on how to make it work. To me this seems like a more sensible default than only listening on IPv4.

I have looked into forcing V6ONLY to false, but as far as I can tell that requires meddling with Twisted internals.

@14mRh4X0r
Copy link
Copy Markdown
Contributor Author

14mRh4X0r commented May 18, 2017

By the way, according to RFC 3493, IPV6_V6ONLY should default to false.

@ara4n
Copy link
Copy Markdown
Member

ara4n commented May 18, 2017

A very common pattern (eg the one that half the matrix core team use) is to get up and running locally on OSX before then shifting to run on linux in production...

@14mRh4X0r 14mRh4X0r force-pushed the listen-ipv6-default branch from eed4157 to 60079d5 Compare May 18, 2017 19:35
@silkeh
Copy link
Copy Markdown
Contributor

silkeh commented May 18, 2017

@ara4n: This works the same on macOS as on Linux. The main exception in functionality is Windows, which does require a separate bind, but the 0.0.0.0 bind can easily be uncommented in the configuration.

@14mRh4X0r
Copy link
Copy Markdown
Contributor Author

I see I overlooked some IPv4 addresses, so changed them to IPv6 as well.

@ghost
Copy link
Copy Markdown

ghost commented May 22, 2017

I got an error with archlinux after commenting in both 0.0.0.0 and :: that the port is already used. So I am not sure if this would be a safe alternative.

@silkeh
Copy link
Copy Markdown
Contributor

silkeh commented May 23, 2017

@saram-kon: on Linux :: listens on both IPv4 and IPv6, so you only need ::.

@ghost
Copy link
Copy Markdown

ghost commented May 23, 2017

I know 😄 I just wanted to point out that enabling both as the default, as suggested by @ara4n

i'm unsure why we can't just bind on 0.0.0.0 as well as :: (ie do two binds...)

might not work on all platforms

Comment thread synapse/config/server.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Apparently binding to ::1 does not bind to 127.0.0.1. I’ve tried above (I use nginx as a reverse proxy), and bind_addresses: ['::1'] did not bind 127.0.0.1, but ['::1', '127.0.0.1'] worked.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're correct. I'll fix that.

Most deployments are on Linux (or Mac OS), so this would actually bind
on both IPv4 and IPv6.

Resolves matrix-org#1886.

Signed-off-by: Willem Mulder <willemmaster@hotmail.com>
@14mRh4X0r 14mRh4X0r force-pushed the listen-ipv6-default branch from 60079d5 to 4029f5a Compare May 31, 2017 17:00
@uhoreg
Copy link
Copy Markdown
Member

uhoreg commented Jun 11, 2017

@kyrias, can you comment on this PR? I seem to remember you looking into something like this when you first did the IPv6 support.

@kyrias
Copy link
Copy Markdown
Contributor

kyrias commented Jun 11, 2017

So yeah, twisted currently has no support for setting IPV6_V6ONLY which is why I left the default config with just IPv4 listeners enabled with just the note about IPv6 support. Seems I even found a twisted PR and commented on it back in December, but nothing has really changed since then.

I do think it make sense to default to ::, since Linux and MacOS apparently follows the default set forth in the RFC, and then BSD or Windows users could just change it themselves. This would give us IPv6 support by default and we're following standards so the default config not working on non-conforming operating systems is okay, I think. I mostly just left it as it is now because it would have made sense to discuss separately from the general IPv6 support PR, but then I forgot to actually open an issue about it!

On the other hand, it might also make sense to have the generated config be conditional on the platform, so on Windows and the BSDs where it's enabled by default we could generate a config file with listeners for both IPv4 and IPv6 enabled.

@kyrias
Copy link
Copy Markdown
Contributor

kyrias commented Jun 11, 2017

X shower thoughts later, I rather dislike the idea of automatically changing the default config based on platform for various reasons. I think what I'd prefer instead would be to have :: be the default, and then have a command line option to select between 0.0.0.0, ::, and both when generating the config. And then if you're generate a config on a platform where IPV6_V6ONLY is enabled by default without passing said command line option we would print a warning about it, preferably with a link that explains what's up more in-depth.

@ara4n
Copy link
Copy Markdown
Member

ara4n commented Jun 11, 2017

having just discussed this briefly with @kyrias again in #matrix-dev:matrix.org, I'm still unclear why my suggestion of:

i suspect the right fix for this is to listen twice, but fix the bind code so that a failing bind isn't considered a terminal error

doesn't work. It just feels clumsy to have to tell people to comment/uncomment chunks of config depending on platform (or special-casing '::' based on platform). Can we not just try to bind to all the given addresses, but not consider bind failures fatal?

@ArchangeGabriel
Copy link
Copy Markdown
Contributor

That seems like a good idea, and is what MPD does for instance: the default setting for bind_address is any, i.e. 0.0.0.0+::, but on my system this gives exception: bind to '0.0.0.0:6600' failed (continuing anyway, because binding to '[::]:6600' succeeded): Failed to bind socket: Address already in use, which is expected since :: implies 0.0.0.0. But note the continuing anyway. ;) (And indeed setting it to :: instead also binds 0.0.0.0:6660 and remove this warning)

silkeh added a commit to silkeh/synapse that referenced this pull request Dec 17, 2017
Binding on 0.0.0.0 when :: is specified in the bind_addresses is now allowed.
This causes a warning explaining the behaviour.
Configuration changed to match.

See matrix-org#2232

Signed-off-by: Silke Hofstra <silke@slxh.eu>
@richvdh
Copy link
Copy Markdown
Member

richvdh commented Dec 18, 2017

superceded by #2435

@richvdh richvdh closed this Dec 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants