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 both IPv4 and IPv6 on all platforms#2435

Merged
richvdh merged 5 commits into
matrix-org:developfrom
silkeh:listen-ipv6-default
Dec 18, 2017
Merged

Adapt the default config to bind on both IPv4 and IPv6 on all platforms#2435
richvdh merged 5 commits into
matrix-org:developfrom
silkeh:listen-ipv6-default

Conversation

@silkeh
Copy link
Copy Markdown
Contributor

@silkeh silkeh commented Sep 2, 2017

This pull requests extends #2232 with the suggestions from @ara4n:

Listen on both :: and 0.0.0.0, but ignore bind errors for 0.0.0.0 when :: was specified.
This ensures that the default configuration supports all platforms.

@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?

Comment thread synapse/app/homeserver.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could you add a comment about why this is a sensible thing to do (ie, explaining some of the problems discussed in #2232 ) ?

@richvdh
Copy link
Copy Markdown
Member

richvdh commented Sep 5, 2017

matrixbot: ok to test

Comment thread synapse/app/homeserver.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure it makes sense to apply the same logic to the rabbithole or replication listeners, not least because if you're binding them to the wildcard addresses, you're probably doing it wrong.

Copy link
Copy Markdown
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I think if we're going to do this we should probably be consistent, so this needs updating in all the other synapse/app/*.py workers.

@richvdh
Copy link
Copy Markdown
Member

richvdh commented Sep 5, 2017

generally this looks like a good idea to me. @erikjohnston do you have any thoughts?

@silkeh
Copy link
Copy Markdown
Contributor Author

silkeh commented Sep 6, 2017

I think if we're going to do this we should probably be consistent, so this needs updating in all the other synapse/app/*.py workers.

I have committed an attempt to abstract this in _base. Depending on feedback I will do the same for all other synapse/app/*.py workers.

Comment thread synapse/app/homeserver.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is a PEP8 failure

Comment thread synapse/app/homeserver.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we need to do this for the replication listener

Comment thread synapse/app/_base.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it'd probably be better to create a new logger in this file than to pass around the logger from another file.

14mRh4X0r and others added 3 commits December 17, 2017 13:07
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>
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>
Add listen_tcp and listen_ssl which implement Twisted's reactor.listenTCP
and reactor.listenSSL for multiple addresses.

Signed-off-by: Silke Hofstra <silke@slxh.eu>
@silkeh silkeh force-pushed the listen-ipv6-default branch 2 times, most recently from 7ce6531 to 8ad120b Compare December 17, 2017 12:27
@silkeh
Copy link
Copy Markdown
Contributor Author

silkeh commented Dec 17, 2017

The requested changes have been applied and I have rebased on develop.
I still need to update the other synapse/app/*.py workers.

Edit: they have been updated.

@silkeh silkeh force-pushed the listen-ipv6-default branch from 8ad120b to 70b20f8 Compare December 17, 2017 12:32
Copy link
Copy Markdown
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

looks good otherwise

Comment thread synapse/app/_base.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is used outside the client reader. logging.getLogger(__name__), please.

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.

Indeed. Fixed.

Signed-off-by: Silke <silke@slxh.eu>
@silkeh silkeh force-pushed the listen-ipv6-default branch from 3311c6b to df0f602 Compare December 18, 2017 19:00
@richvdh richvdh merged commit 48fa4e1 into matrix-org:develop Dec 18, 2017
@richvdh
Copy link
Copy Markdown
Member

richvdh commented Dec 18, 2017

thanks!

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.

4 participants