Hi Rajini I made changes suggested by you on https://github.com/maulin-vasavada/kafka/pull/4. Please check.
In particular I had challenge in 'SslFactory#configure()' method the first time to know which configs I have to add without having actual SslEngineFactory impl. I think it is best to just copy all configs with "ssl." prefix. Can you please review https://github.com/maulin-vasavada/kafka/pull/4/files#diff-1e3432211fdbb7b2e2b44b5d8838a40bR90 particularly? Clement, I missed to see your point about Mode in previous post but even after I realized what you are suggesting, my response would be the same as before :) Thanks Maulin On Wed, Feb 5, 2020 at 8:40 PM Maulin Vasavada <maulin.vasav...@gmail.com> wrote: > 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. >> >
