search

RestComm / jdiameter

RestComm Diameter Stack and Services
http://www.restcomm.com/
GNU Affero General Public License v3.0
111 stars 153 forks source link

Migrate jDiameter to Netty #35

Open deruelle opened 8 years ago

deruelle commented 8 years ago

Move all the IO layer to Netty.

jehanzebqayyum commented 8 years ago

TCPClientConnection & TCPTransportClient are used by both diameter server and client. To move to netty this should be refactored into server and client specific transport. Since server will have both boss and worker group to handle multiple clients. So server side NetworkGuard functionality will move into TCPServerConnection.

jehanzebqayyum commented 8 years ago

I went through the code and seems like changing to netty requires a bit of refactoring or may be i need more understanding of the code.

The problems i am facing specifically are:

1- Client and Server both are using same TCPClientConnection class which I want to separate. I will need some assistance how to do that. In netty, Server will have 2 thread groups i.e. boss group which accepts clients and worker group which will serve each client. Whereas client will require only worker group.

2- I am trying to understand the purpose of NetworkGuard and when it is used? Because TCPServerConnection (which i am planning to create) should have the functionality to handle multiple clients by default for server logic.

I am attaching a client side NettyTransportClient that i created for review.

There are other parts of transport which will benefit from refactoring e.g.

Connection Listener and Event logic is duplicated in all transports TCP, TLS etc.

Connection Listeners are blocking i.e. when an event e.g. message received is raised, all the connection listeners are called in blocking which blocks the worker thread instead it should be asynchronous and we can use e.g. guava event bus for handling events.

What are your thoughts?

NettyTransportClient.java.txt

brainslog commented 8 years ago

Hi @jehanzebqayyum, thanks for your interest in this contribution.

As per your queries, I will try to explain as I can.

1- Client and Server both are using same TCPClientConnection class which I want to separate.

In jDiameter it's a common pattern to have the basic/common functionality in the client side and the server extending it with additional features. In this case it's pretty much the same, except the naming might not be the most clear.

So, on the client side we have:

These are the basics for a Diameter peer to operate, be it a server or a client, receive the bytes, transform them into a Diameter message and deliver it to the listeners. Also, always keep in mind that in Diameter both client or server might initiate the session with a message and even any of them may initiate the connection.

2- I am trying to understand the purpose of NetworkGuard and when it is used? Because TCPServerConnection (which i am planning to create) should have the functionality to handle multiple clients by default for server logic.

So, in case of the server, we need to open a port for incoming connections. We have the following:

So, I think this should make it clear that there's no need for distinct TCPTransportClient or TCPTransportClient as the tasks they perform are quite delimited and common to any type of peer. As for the TCPServerConnection, the functionality you seem to pretend to implement is partially what is actually being done at NetworkGuard.

Also should be taken into account the modularity of jDiameter, where all these components are changeable by custom implementations, thus the need of these separation of concerns.

I will review the NettyTransportClient.java you have attached, at first look it seems correct, but haven't yet had the time to fully review and test it, but wanted to try to answer your questions ASAP. Feel free to make any changes to it meanwhile, if you feel necessary and if this explanation might have caused any difference to it.

jehanzebqayyum commented 8 years ago

Thanks for the guidance. Base on the input i updated TCPClientConnection, TCPClientTransport (NettyTransportClient) and NetworkGuard. Attached updated.

A couple of more questions, hope you do not mind: 1- Should we be reusing the thread pool from IConcurrentFactory or instantiate new ones as defaults in netty's NioEventLoopGroups. 2- In NettyTransportClient orig/source address seems redundant since same channel cannot be used for bind and connect, i believe.

Meanwhile i will be testing the changes.

NettyTransportClient.java.txt NetworkGuard.java.txt TCPClientConnection.java.txt

deruelle commented 8 years ago

@jehanzebqayyum it may be easier for review if you fork the project, commit your changes in your own branch and push to your fork and do a pull request for review as described in our Open Source Playbook

jehanzebqayyum commented 8 years ago

Done. Please review the pull request and provide your comments. Meanwhile i continue to work on my fork.

Best Regards, Jehanzeb Qayyum +971 52 898 2285 +92 3311337064

On Thu, May 12, 2016 at 12:57 AM, Jean Deruelle notifications@github.com wrote:

@jehanzebqayyum https://github.com/jehanzebqayyum it may be easier for review to fork the project, commit your changes in your own branch and do a pull request for review as described in our Open Source Playbook https://docs.google.com/document/d/1RZz2nd2ivCK_rg1vKX9ansgNF6NpK_PZl81GxZ2MSnM/edit#heading=h.mcjitemt6ng1

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/RestComm/jdiameter/issues/35#issuecomment-218587827

deruelle commented 8 years ago

Thanks @jehanzebqayyum !

@brainslog will review.

jehanzebqayyum commented 8 years ago

Should TLS negotiation happen in connection establishment or CER/CEA?

Below is what i found out: For RFC3588 compliant peers TLS (Transport Layer Security) may optionally be negotiated. For RFC6733 compliant peers TLS negotiation may optionally happen before the CER/CEA.

Secondly this is what i found for inband security id in RFC6733:

Inband-Security-Id AVP

The Inband-Security-Id AVP (AVP Code 299) is of type Unsigned32 and is used in order to advertise support of the security portion of the application. The use of this AVP in CER and CEA messages is NOT RECOMMENDED. Instead, discovery of a Diameter entity's security capabilities can be done either through static configuration or via Diameter Peer Discovery.

Any comments?

Best Regards, Jehanzeb Qayyum +971 52 898 2285 +92 3311337064

On Mon, May 16, 2016 at 1:49 PM, Jean Deruelle notifications@github.com wrote:

Thanks @jehanzebqayyum https://github.com/jehanzebqayyum !

@brainslog https://github.com/brainslog will review.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/RestComm/jdiameter/issues/35#issuecomment-219388930

brainslog commented 8 years ago

@jehanzebqayyum, regarding TLS we are working in parallel on issue #14 . The idea will be to have both ways working. We surely want RFC 6733 behavior to be the default, but most implementations seem to still be using RFC 3588, so we also need to have it for interop.

Implementing RFC 3588 is likely (still) the most useful and the most challenging one, so that'd be a good start :)

jehanzebqayyum commented 8 years ago

@brainslog quick questions why post sending CEA there is startTLS? i expect it only after post receiving CEA. what is the purpose of checking EXPERIMENTAL_RESULT in TLS transport?

brainslog commented 8 years ago

@jehanzebqayyum, both server and client need to prepare for TLS after sending/receiving the CEA, and setup the HandshakeCompletedListener.

As for the check of EXPERIMENTAL_RESULT, it's likely just a "fallback" mechanism when no Result-Code is present. It can probably be safely discarded.

deruelle commented 8 years ago

@jehanzebqayyum where you able to move forward thanks to @brainslog comments ? Anything blocking ?

jehanzebqayyum commented 8 years ago

Yes i almost finished TLS implementation. Since there are no test case for TLS. Can you point out for TLS sample config?

Thanks On Jun 1, 2016 12:32 PM, "Jean Deruelle" notifications@github.com wrote:

@jehanzebqayyum https://github.com/jehanzebqayyum where you able to move forward thanks to @brainslog https://github.com/brainslog comments ? Anything blocking ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RestComm/jdiameter/issues/35#issuecomment-222927479, or mute the thread https://github.com/notifications/unsubscribe/AFNfSl8xXtsxykxl7C2xjzDT7I-rHHFDks5qHUOsgaJpZM4IVOfw .

jehanzebqayyum commented 8 years ago

@brainslog https://github.com/brainslog where can i find sample TLS client/server config?

Best Regards, Jehanzeb Qayyum +971 52 898 2285 +92 3311337064

On Wed, Jun 1, 2016 at 12:36 PM, jz jehanzeb.qayyum@gmail.com wrote:

Yes i almost finished TLS implementation. Since there are no test case for TLS. Can you point out for TLS sample config?

Thanks On Jun 1, 2016 12:32 PM, "Jean Deruelle" notifications@github.com wrote:

@jehanzebqayyum https://github.com/jehanzebqayyum where you able to move forward thanks to @brainslog https://github.com/brainslog comments ? Anything blocking ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RestComm/jdiameter/issues/35#issuecomment-222927479, or mute the thread https://github.com/notifications/unsubscribe/AFNfSl8xXtsxykxl7C2xjzDT7I-rHHFDks5qHUOsgaJpZM4IVOfw .

brainslog commented 8 years ago

@jehanzebqayyum, I don't think we have any ready, will need to prepare it. Or maybe @coolface88 who's working on #14 might have some already ? If not, I'll try to provide you with it by next week.

BTW, regarding TCP, I've made some comments to the PR #39 as it seems it has some functional issues.

deruelle commented 8 years ago

@jehanzebqayyum just checking if you have been able to move forward on that ?

jehanzebqayyum commented 8 years ago

​Well i am trying to resolve following issue with TLS implementation​.

io.netty.handler.codec.DecoderException: javax.net.ssl.SSLHandshakeException: no cipher suites in common at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:418) at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:245) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:292) at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:278)

Client config looks like this:

​Server config has following:

​ ​SSL context creation:

public static SslContext getSslContextForClient(Configuration config) throws SSLException, Exception { SslContext sslContext = SslContextBuilder.forClient()

.keyManager(getKeyManagerFactory(config)).trustManager(getTrustManagerFactory(config)).build(); return sslContext; }

public static SslContext getSslContextForServer(Configuration config) throws SSLException, Exception { SslContext sslContext = SslContextBuilder.forServer(getKeyManagerFactory(config)) .trustManager(getTrustManagerFactory(config)).build(); return sslContext; } ​

On Fri, Jun 10, 2016 at 8:40 PM, Jean Deruelle notifications@github.com wrote:

@jehanzebqayyum https://github.com/jehanzebqayyum just checking if you have been able to move forward on that ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RestComm/jdiameter/issues/35#issuecomment-225233307, or mute the thread https://github.com/notifications/unsubscribe/AFNfSvuvVy7m0NahDgSCN7Y39ngG3qHMks5qKZN0gaJpZM4IVOfw .

deruelle commented 8 years ago

@jehanzebqayyum were you able to debug it ? did you see http://stackoverflow.com/questions/9534118/ssl-tls-error-no-cipher-suites-in-common-with-netty-when-trying-to-establish#comment12130198_9534118 ?

try to make sure the key manager is not null http://stackoverflow.com/a/15144731/118052

jehanzebqayyum commented 8 years ago

Yes in fact the ssl debug log showed that the key i was using did not match the TLS specs/supported cipher suites.

I used default jdk keytool option to generate key pair which resulted in DSA algo.

I resolved it by specifying RSA as the keyalg.

And finally i was able to run a functional test (AccSessionStatefulBasicFlowTest) successfully.

I will push my changes soon.

On Mon, Jun 13, 2016 at 5:23 PM, Jean Deruelle notifications@github.com wrote:

@jehanzebqayyum https://github.com/jehanzebqayyum were you able to debug it ? did you see http://stackoverflow.com/questions/9534118/ssl-tls-error-no-cipher-suites-in-common-with-netty-when-trying-to-establish#comment12130198_9534118 ?

try to make sure the key manager is not null http://stackoverflow.com/a/15144731/118052

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RestComm/jdiameter/issues/35#issuecomment-225578969, or mute the thread https://github.com/notifications/unsubscribe/AFNfSnwvLi6GP-Qtka28eANGmB93gMV6ks5qLVnGgaJpZM4IVOfw .

jehanzebqayyum commented 8 years ago

@brainslog did you check?

On Jun 13, 2016 10:58 PM, "jz" jehanzeb.qayyum@gmail.com wrote:

Yes in fact the ssl debug log showed that the key i was using did not match the TLS specs/supported cipher suites.

I used default jdk keytool option to generate key pair which resulted in DSA algo.

I resolved it by specifying RSA as the keyalg.

And finally i was able to run a functional test (AccSessionStatefulBasicFlowTest) successfully.

I will push my changes soon.

On Mon, Jun 13, 2016 at 5:23 PM, Jean Deruelle notifications@github.com wrote:

@jehanzebqayyum were you able to debug it ? did you see http://stackoverflow.com/questions/9534118/ssl-tls-error-no-cipher-suites-in-common-with-netty-when-trying-to-establish#comment12130198_9534118 ?

try to make sure the key manager is not null http://stackoverflow.com/a/15144731/118052

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jehanzebqayyum commented 8 years ago

hi @deruelle, are you referring Restcomm/sctp netty branch? I do not see netty-2. The restcomm sctp netty branch is using older version of netty lib. Considering that there is default support for SctpChannel in netty should we be using the restcomm sctp netty lib? Since we will not be using features such as fall back to TCP, Management etc from the lib. I feel going along the lines of existing implementations of netty tcp/tls in jdiameter will be much straightforward.

deruelle commented 8 years ago

@jehanzebqayyum since my comment the master branch is now netty enabled so you can use https://github.com/RestComm/sctp/tree/master. Let's go with the pure netty approach then for now. I would like @vetss to comment on that too to see if we are missing on any restcomm sctp features if we go with the pure netty based approach but he is on vacations so please proceed with pure netty approach for now.

vetss commented 7 years ago

I have created an issue that explain details for switching to new SCTP lib versions https://github.com/RestComm/jdiameter/issues/60