Skip to content

refactor listener config loading#565

Merged
slingamn merged 6 commits intoergochat:masterfrom
slingamn:listener_refactor.3
Jul 12, 2019
Merged

refactor listener config loading#565
slingamn merged 6 commits intoergochat:masterfrom
slingamn:listener_refactor.3

Conversation

@slingamn
Copy link
Copy Markdown
Member

Not for merge until the 1.2.0 window opens.

This is prep work for #561, but it makes sense on its own, and the full change for #561 is messy enough that we may not want to do it unless discussions with the hashbang people proceed further. This change will also be helpful if we do #448 ("sts-only listener" will become a flag on listenerConfig, analogous to "is this listener TLS?" and "is this listener Tor?").

Anyway, another benefit of this change is that reading the certificates now takes place as part of the config load, so it solves the case where you get part of the way through the rehash and then it fails because the certs are missing or their permissions are bad.

@slingamn
Copy link
Copy Markdown
Member Author

Just a heads-up, the public plaintext listener is disabled in this rewrite of the default config, with instructions on how to reenable it. I can reverse that decision if necessary.

irc/server.go Outdated
}
}
if 0 < len(tlsListeners) && !usesStandardTLSPort {
if config.Server.trueListeners[":6697"].TLSConfig == nil {
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.

Is it just me or would this incorrectly say the "pls expose 6697" message if the admin for example has the listener "192.168.0.1:6697" or otherwise specifies a particular IP rather than just the port on all interfaces? If it would, may go back to the method used before this change.

@DanielOaks
Copy link
Copy Markdown
Member

Let's keep exposing the plaintext listener on 6667 for now. My opinion on it is: We need to keep exposing a listener on 6667 by default, being one of the most well-used ports for IRC. We don't, however, need to expose a real listener on it. Once #448 is done we can have it do that by default (and make it an easy flick of a switch to turn it back into a normal listener).

@DanielOaks
Copy link
Copy Markdown
Member

After the 6667 listener is re-enabled and the :6697 message issue raised above are addressed, this is fine to merge in. Thanks for the awesome fixes :)

@slingamn slingamn merged commit ecf9450 into ergochat:master Jul 12, 2019
@slingamn slingamn deleted the listener_refactor.3 branch July 14, 2019 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants