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

use twisted endpoint description strings to bind to ports and sockets #37

Merged
merged 10 commits into from
Nov 14, 2016

Conversation

mcallistersean
Copy link
Contributor

This implements the usage of twisted server endpoint description strings to connect to ports, sockets and such as discussed in #10
This allows daphne to support cases such as #17 and #36 without needing any extra code.
E.g.

daphne -e ssl:8443:privateKey=localhost.key:certKey=localhost.crt <channel_layer>

starts up a SSL enabled server on port 8443

@andrewgodwin
Copy link
Member

We're actually looking at rewriting Daphne itself to use more of the Twisted stuff natively that comes around this, so I'm going to leave this for now until we have that done, as it may clash with what's coming there (it's mostly around the pluggable TLS/Lets Encrypt stuff, and HTTP2 support)

@Lukasa
Copy link
Contributor

Lukasa commented Oct 21, 2016

@andrewgodwin In fact, pluggable LE and HTTP/2 would benefit from having this merged first. Pluggable LE is managed by Twisted endpoints (which this PR enables), and HTTP/2 requires TLS support (which is also enabled by this PR).

So my recommendation is that we hammer this into shape for merging and then I'll look at what code changes are required to get HTTP/2 support for Daphne. LE support should drop out in the form of txacme.

@andrewgodwin
Copy link
Member

Alright, good to know - I'll see if I can find some time to merge it in once the checks pass.

@mcallistersean
Copy link
Contributor Author

The builds are now passing.
But thinking about the changes a bit more it might be reasonable to move these lines to the cli.py file:
https://github.com/mcallistersean/daphne/blob/ticket_10/daphne/server.py#L161-L183

And then pass the endpoints to the servers init method. That would probably make for a cleaner interface. Is the Server class meant to be public, or would it be ok to change it so that it can only handle endpoint descriptions as opposed to host/port/unix sockets?

@andrewgodwin
Copy link
Member

The Server class is kind of public - it's used directly by the runserver command code for one - but if we get them both patched at the same time I don't see the problem with changing it. You could also introduce the ability to pass either an endpoint or host/port.

@mcallistersean
Copy link
Contributor Author

Ok. I'll work on that during the DUTH sprints.

@mcallistersean
Copy link
Contributor Author

@andrewgodwin: did you mean that the channels runserver command should be able to accept twisted endpoint description strings?
The daphne cli interface already supports this kind of mixed options (just added a test for that), and I'm not sure if runserver should because (1) that would be an extra command line option as opposed to django runserver and (2) is probably not useful for runserver driven development.
I now intend to keep the endpoint string creation where it is now because of the usage by channels runserver. That could easily be changed once 1.0 approaches for either channels or daphne.

Any ideas/comments? Otherwise I guess this could be merged.

@andrewgodwin
Copy link
Member

I meant that I'd like to see the Server class accept endpoint strings only and remove the host/port/unix socket arguments. I'd still want the runserver command to work the same as the main Django one, but translate internally into an endpoint string when it makes the Server.

@mcallistersean
Copy link
Contributor Author

I actually meant this comment:

You could also introduce the ability to pass either an endpoint or host/port.

Anyway, the latest commits deprecate the use of host/port/socket names/file_descriptor on the Server class and move the building of endpoint description strings to a separate function.
The corresponding pull request for channels is here: django/channels#434
It currently fails because it would need this pull request landed first.

@andrewgodwin
Copy link
Member

Lovely - thanks for your work on this!

@ertygiq
Copy link

ertygiq commented Sep 25, 2017

Hi,

I tried following call of daphne
daphne -e ssl:8443:privateKey=/etc/letsencrypt/live/my.domain.net/privkey.pem:certKey=/etc/letsencrypt/live/my.domain.net/fullchain.pem project.asgi:channel_layer -b <my_id>
And got this in log:

2017-09-25 19:16:30,283 INFO     Listening on endpoint ssl:8443:privateKey=/etc/letsencrypt/live/my.domain.net/privkey.pem:certKey=/etc/letsencrypt/live/my.domain.net/fullchain.pem
2017-09-25 19:16:30,289 INFO     Listening on endpoint tcp:port=8000:interface=<my_id>

Does it mean, that is should connect to <my_ip> with WSS protocol on 8443 port?
What is 8000 port left for?

@mcallistersean
Copy link
Contributor Author

You can simply remove the -b option from your command.
-b/-p are for the simple cases and for overriding defaults, -e gives you more control.

@ertygiq
Copy link

ertygiq commented Sep 25, 2017

@mcallistersean thank you. The log I provided is created by daphne call without -p argument.
If I don't provide -b arg how would daphne know which ip listen to?

Anyway, does my log mean that daphne listens to both 8443 and 8000 port?

@mcallistersean
Copy link
Contributor Author

mcallistersean commented Sep 25, 2017

-p was just another example of a command line flag that overrides the defaults. Actually daphne just uses -b and -p options (with fallback defaults) to construct the endpoint string you are seeing above.

The -e string is interpreted directly by twisted, so it does what twisted does by default (all interfaces). If you want to be on the safe side just add interface=<ip> to the endpoint (-e) string.

Anyway, does my log mean that daphne listens to both 8443 and 8000 port?

Yes, but the server listening on port 8000 is not secure

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.

4 participants