[discussion] pool ssl/tls connections - alternative 2 #206
Conversation
There was a problem hiding this comment.
Ok cool, thanks for this. I also prefer this to the other, more intrusive option. I guess the downside being there's a fair bit of code repetition between this and the existing proxy connection code.
Otherwise, generally looks good to me. I've added some comments.
I would really like to have tests around this stuff, as well as the existing forwarding proxy connection code. This was contributed code and not a use case of mine, so I've always been a bit nervous about it. I've just got travis builds working. I'm wondering if we can do something with Docker Compose to test against a forwarding proxy?
Happy to merge this as is with my minor comments addressed though. Please rebase against master before pushing changes, as you'll pick up the working travis config.
You'll also want to add your new module to the rockspec and .luacov, and bump LUACOV_NUM_MODULES in the Makefile for test coverage reporting.
Thanks!
Tieske
force-pushed
the
connect-better
branch
6 times, most recently
from
de36967
to
fbe6d94
Compare
|
@pintsized all updates done, please have another look |
|
@pintsized any updates? PS. CI seems to be in a weird state. I can't click any of the links to check what's wrong? |
|
ping |
|
Hey, sorry (again) for being so slow. Things are pretty hectic for me at the moment. It's passing local tests fine, I think it's just because your PR was submitted before I got Travis working - Github isn't getting the notification or something. Of course the important tests are the ones you're hosting (thanks for sending the details over). My only concern with all of this is the amount of code duplication. Could / should I appreciate it's a bit of a mess at this stage, and that your contribution here is important for correctness. If we can push the old interface towards using this new code somehow, things will be much less likely to rot. Having said all of this, I really don't have any time to work on it at the moment, and so if we need to merge this and come back to it, I can live with that. I'll hopefully have some proper time in a month or so. At which point I'd be keen to get something like your integration tests setup for this. Thanks for your input, and sorry for not being more responsive! |
Tieske
force-pushed
the
connect-better
branch
2 times, most recently
from
bb81345
to
0021a71
Compare
This covers connecting on tcp, ssl and proxy level
Tieske
force-pushed
the
connect-better
branch
from
0021a71
to
4a45d32
Compare
these options must be global since the https auth is used when connecting, and the http one only when requesting. So these settings span the process of connecting and requesting, and hence we should not pass them into 'connect', because we would have to ensure that the user would pass the same ones to 'request'.
also renames other variables to be more descriptive
Tieske
force-pushed
the
connect-better
branch
from
50bbf8a
to
28c0d0f
Compare
the connect method cannot deal with the auth header for a http request, only for https. Hence we store the value and inject is when making the request. We need it on connect, to construct the proper pool-name. We honor the existing request header if it is there, but that is a bad idea, since the poolname is based on the one provided to connect. So users should set the proxy options, and make requests without that header to be safe.
required to be able to use aio_connect from the request_uri method, since that method allows to use the proxy auth header for https proxy requests.
This provides better backward compatibility.
|
@pintsized I updated the PR. I squashed the original PR into a single commit and added/changed step-by-step, to make the short hand method use the aio connect method (and fix some bugs that popped up). I made an effort to make commits atomic so for review it is probably easiest to review individual commits. Can you please have another look? |
|
Awesome man, thanks for the effort here. I'll go through it properly by the end of the week. |
Tieske
force-pushed
the
connect-better
branch
from
f9daf39
to
09503be
Compare
Tieske
force-pushed
the
connect-better
branch
from
09503be
to
af1ff9c
Compare
|
there was another flaky test, updated the commit and pushed again, CI is green now. |
There was a problem hiding this comment.
OK, looking good 👍
Aside from the minor comments that I added, my only real concerns are the test coverage and the increased API surface.
For the test coverage, the major difference seems to be that connect_proxy no longer has any coverage at all. Am I right in thinking this method is really only there for backwards compatibility with code using the generic interface? I personally have almost no experience with forward proxy semantics, and all of the use cases here have been contributed by others. I'm more than a little nervous about breaking things I don't understand ;)
But this leads me to wonder if we should be marking anything as deprecated whilst we're at it? I guess I'm thinking; if someone new comes to this module, are we making it clear enough what they should reach for first, since they now have even more options? Seems like we could deprecate the old connect signatures, and also the connect_proxy method?
I guess that includes ssh_handshake too. Originally I just tried to keep the interface similar to the other cosocket modules, but the reality is that HTTP semantics are just more complicated than that, and my sense is we should encourage everyone to use your "all in one" method, right? Can you think of use cases that aren't covered by that?
I think we're also missing test coverage for a non https proxy, but that appears to be true on the master branch too.
Again, thanks for all the effort here.
| @@ -151,14 +151,53 @@ Creates the http object. In case of failures, returns `nil` and a string describ | |||
|
|
|||
| ## connect | |||
|
|
|||
| `syntax: ok, err = httpc:connect(aio_options_table)` | |||
|
|
|||
| `syntax: ok, err = httpc:connect(host, port, options_table?)` | |||
|
|
|||
There was a problem hiding this comment.
Do you think perhaps we should deprecate the old interface? Seems like we could use your new interface, without ssl or proxy options, and get the same behaviour without much cost?
There was a problem hiding this comment.
yes.
These days everything is ssl mostly. Doing the steps manual is causing a lot of code duplication, and is very tedious because there are so many edge cases one needs to think of. A lot of parameters to each of the connect, handshake, and proxy calls share a lifetime over all the three methods.
That is exactly what caused me to rewrite the whole thing, such that it would be less error prone.
Yes
Yes probably all three should be deprecated, and examples updated to use the aio method
Nope
👍 |
|
Ok, I'll merge this and rework the docs, deprecating the old connection syntax. |
|
Nice! 🎉 |
|
@pintsized fyi: I just created an updated version of the Kong Lambda plugin which has integration tests for keepalive and proxy connections. The tests in the branch run against the master branch of the http client. Everything is green. Just extra confirmation all seems well, since proxy tests are lacking in the lib. |
|
Nice, good to know! I'll cut a new release soon... |
Here's the second option for safe keep-alive across ssl + proxy connections.
It implements a new connect method that does it all at once (connect, ssl-handshake, proxy)
This is (imo the better) alternative to #202 .
There are no tests since this is very hard to test without a proxy server. Yet tests are available in the originating code here: https://github.com/Kong/kong-plugin-aws-lambda/compare/better-connect?expand=1 , which includes a forward-proxy and full integration tests.
Input is appreciated @hamishforbes @pintsized @bungle .
replaces #201
fixes #161
replaces #205