Hi Rajini Will accommodate your comments.
Celement, while SSLContext factories are common, in this particular case, we need SSLEngine object because Kafka is using SSLEngine (as mentioned much before in this email thread, the SSLContext acts as factory for getting SSLEngine, SSLSocketFactory or SSLServerSocketFactory and Kafka chooses SSLEngine to be used for SSL Connections). The 'Mode' challenge doesn't go away easily since Kafka is using the "same" classes for Client side and Server side. Other stacks where you don't see this challenge is because either libraries are client centric or server centric and not both at the same time. I would suggest you do a deeper dive into the sample Pull request, build the code to get better idea about it. I don't find it strange to have 'Mode' argument in this context of Kafka. Kafka is not doing anything out of norm here since ultimately it is using JSSE spec for SSL Connection. I will try to respond to code comments in couple of weeks since I am out for few weeks. Will keep you guys posted. Thanks Maulin On Wed, Feb 5, 2020 at 9:50 PM Pellerin, Clement <clement_pelle...@ibi.com> wrote: > Many of these points came up before. > > I had great hope when Maulin suggested the custom factory could > return an SSLContext instead of SSLEngine. SSLContext factories are > common, > whereas I have never seen an SSLEngine factory being used before. > He must have hit the same problem I had with the Mode. > > If the Mode can be removed, can we find a way to return an SSLContext now? > What is so special about Kafka that it needs to hardcode the Mode when > everyone > else works with the SSLContext and ignores the other mode they don't use. > > -----Original Message----- > From: Rajini Sivaram [mailto:rajinisiva...@gmail.com] > Sent: Wednesday, February 5, 2020 10:03 AM > To: dev > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > extensible > > One more point: > 5) We should also add a method to SslEngineFactory that returns > `Set<String> > reconfigurableConfigs()` > > On Wed, Feb 5, 2020 at 1:50 PM Rajini Sivaram <rajinisiva...@gmail.com> > wrote: > > > Hi Maulin, > > > > Thanks for the updates. A few comments below: > > > > 1) SslEngineFactory is currently in the internal package > > org.apache.kafka.common.security.ssl. We should move it to the public > > package org.apache.kafka.common.security.auth. > > 2) SslEngineFactory should extend Configurable and Closeable. We should > > expect the implementation class to have a default constructor and invoke > configure() > > to be consistent with otSslher pluggable classes. > > 3) SslEngineFactory.createSslEngine uses the internal enum `Mode`. It > > would be better to add two methods instead: > > > > > > - SSLEngine createClientSslEngine(String peerHost, int peerPort, > String endpointIdentification); > > - SSLEngine createServerSslEngine(String peerHost, int peerPort); > > > > 4) Don't think we need a method on SslEngineFactory to return configs. > It seems like an odd thing to do in a pubic Configurable API and is > inconsistent with other APIs. We can track configs in the internal > SslFactory class instead. >
