Add support for SSL and TCP6 in app.Klein.run #113
Conversation
Updates app.Klein.run() so that user will be able to specify protocol, certificate, private key and dh_parameters. This way it will be possible to easily run HTTPS site with Klein. It will also mean HTTP2 support for Klein will work ok with Twisted > 16.3.0. TLS is not formally required for HTTP2 but most client implementations require it.
| server_string += ":dhParameters={}".format(dhParameters) | ||
|
|
||
| endpoint = endpoints.serverFromString(reactor, server_string) | ||
| endpoint.listen(Site(self.resource())) |
There was a problem hiding this comment.
It's not clear to me that using string formatting to build the endpoint is a great plan. It may be much better to simply take an optional endpoint string and use serverFromString on that string, or otherwise use the TCP4ServerEndpoint directly in the way the old code used listenTCP.
There was a problem hiding this comment.
t's not clear to me that using string formatting to build the endpoint is a great plan.
why do you think so? What kind of problems it can cause? One thing I was thinking of is that users can pass invalid values, but in this case things will simply fail to work.
It may be much better to simply take an optional endpoint string and use serverFromString on that string, or otherwise use the TCP4ServerEndpoint directly in the way the old code used listenTCP.
why do you think it's better?
There was a problem hiding this comment.
why do you think it's better?
Because it allows users to use the full grammar of endpoints. For example, it's inconvenient in this form to use txacme, because you end up having to smuggle it in by passing proto="lets:/srv/www/certs:tcp", port="443". That works, but it reveals the weaknesses of the string formatting approach: it gives you enough rope to totally break something, but not enough to achieve something really powerful.
So if we're going to use serverFromString, we should just let the user pass whatever endpoint string they want. That gives them the full flexibility to ask for whatever they want. For example, they can pass TLS options other than the ones you allowed them, or use txacme to auto-provision their certs, or any number of other good things.
There was a problem hiding this comment.
Because it allows users to use the full grammar of endpoints
hmm yes this sounds convincing.
So if we're going to use serverFromString, we should just let the user pass whatever endpoint string they want.
but this will create two different (possibly duplicated) forms of passing arguments to run(), one would accept host, port etc (as it does now), and other would accept server string specification. To pass string specification you'd have to call run like this run(host, port, server_string='tcp:port:host:something_extra_here") so this would be duplicated.
Maybe it would be better to just add another method to app.Klein object that would take optional server string and it would run serverFromString on that? E.g. add method run_from_string(self, server_string) or endpoint_listen(self, endpoint_spec)?
or otherwise use the TCP4ServerEndpoint directly in the way the old code used listenTCP.
given the amount of options in SSL it might be slightly inconvenient to pass all of them to Klein.run, run would have to take lots of optional keyword arguments defaulting to None and then pass them to appropriate ServerEndpoint.
I think I would vote for passing string specification directly in some new Klein method and doing serverFromString on that.
There was a problem hiding this comment.
To pass string specification you'd have to call run like this
run(host, port, server_string='tcp:port:host:something_extra_here")so this would be duplicated.
There's no reason for that to be true, we can just make host and port optional arguments.
However, I otherwise agree with your other option, which is to create a new method. That's also fine by me.
There was a problem hiding this comment.
However, I otherwise agree with your other option, which is to create a new method. That's also fine by me.
On second thought I realized that one drawback of new method is that it will create two ways of doing same thing, so this may add some confusion. Aside from that some server specifications will work but probably don't apply to Klein, e.g. "unix:address=/var/run/web.sock:lockfile=1" will be valid string that could perhaps possibly work for serverFromString but probably doesn't make much sense for Klein.
Using endpoints directly may allow to limit arguments to those relevant for TCP and SSL and keep only one way of doing things.
There was a problem hiding this comment.
Is there any reason to forbid the UNIX listening socket? HTTP over UNIX socket is well defined, and quite common when deploying a reverse proxy like nginx.
There was a problem hiding this comment.
On second thought I realized that one drawback of new method is that it will create two ways of doing same thing, so this may add some confusion.
I agree it would cause initial confusion, in the sense that TCP4ServerEndpoint or serverFromString causes general confusion in Twisted. It can become a bit difficult to determine which function to use but at the end of the day you just pick one and move on to bigger problems. With that being said, what would be the problem with having 2 methods? One is a "dummy-proof" method where you supply the interface/port and a connection is started using ipv4. The other is a method that allows devs to utilize what ever protocol they want.
A single method solution is feasible, but if it's not something that can be agreed upon, couldn't another method be added and then mark the Klein.run method for deprecation?
There was a problem hiding this comment.
A single method solution is feasible, but if it's not something that can be agreed upon, couldn't another method be added and then mark the Klein.run method for deprecation?
deprecating methods is always inconvenient for users so its better to avoid that if possible. It is certainly possible to avoid that here so I think this would be right thing to do.
With that being said, what would be the problem with having 2 methods?
if something can be done within one method why add two methods? It's better to keep things simple.
I added commit allowing to pass endpoint_description argument here: c90d181
app.Klein.run() gets new keyword argument 'endpoint_description' that may contain valid endpoint specification. Old way of calling Klein.run() will still work ok, only thing that will change is that TCP4ServerEndpoint by default gets backlog=50 keyword argument. Endpoint description should allow to run Klein in all forms allowed by endpoint spec (e.g. SSL, TCP6, unix socket etc).
|
Cool. Let's do it! :) |
Current coverage is 94.50% (diff: 100%)
|
Updates app.Klein.run() so that user will be able to specify protocol (ssl, tcp, tcp6), certificate, private key and dh_parameters. This way it will be possible to easily run HTTPS site with Klein. It will also mean HTTP2 support for Klein will work ok with Twisted > 16.3.0. TLS is not formally required for HTTP2 but most client implementations require it.
cc @glyph @hawkowl