fixing erroneous direct construction of Netty3Listener without params in finagle-thrift

[danielpcox]

Greetings Finagle masters,

TL;DR In finagle-thrift (and none of the other protocol libraries), Netty3Listener was being constructed directly, without passing in the params.

I was originally trying to test the official solution to #175 and was able to get TLS over Thrift and extraction of the client certificate working using the old builder API, but not with the new Finagle 6 API. Eventually I discovered that apply on the Netty3Listener object was never being called, which led to this adjustment.

@mosesn and I discussed the removal of the hard-coded "thrift" label, and since it's only used as a backup scope for the stats receiver if the ServerRegistry doesn't have one for the listening address, and because it's manually configurable by the user, it seemed safe to remove it. I think it was probably only put there anyway because direct use of the constructor requires it.

Thanks to @mosesn @jeremyrsmith for helping me run this to ground.

[mosesn]

Have you tested this to make sure it solves the problem?

[danielpcox]

I will be today. Just have to figure out how to build Finagle first. :)

[danielpcox]

Yes. Hallelujah. I can confirm that this fix allows me to use the new Finagle 6 APIs to

a) do Thrift RPC over TLS and
b) extract the client certificate from the Transport to allow for PKI-based client authentication.

Examples of these miracles are here: https://github.com/DecipherNow/finagle-tls/tree/new-api
Obviously at the moment they require a manual publishM2 of this finagle-thrift fork to use.

[mosesn]

👍 shipit

[nshkrob]

Makes sense. Can you add a simple unit test, e.g. by setting the Stats param and checking for stats?

[danielpcox]

Sure, though it's crunch-time on another project so it'll have to wait until Monday.

[suls]

I verified this with https://groups.google.com/forum/#!topic/finaglers/g_4i1xpfAJo ..

Would adding a thrift/TLS test case be overkill?

[mosesn]

Nah, seems reasonable. Do you want to make a PR against Daniel's branch? The one caveat is that because we're going to squash this into one commit, only he would get the authorship credit for the change.

[nshkrob]

It would be definitely useful as a reference and as a test for this, thanks! @mosesn we can pull the test separately I think.

[mosesn]

👍