Hi Rajini When I configure the Default value for ssl.engine.factory.class as Type Class, it is resulting into lot of test cases failures since in many places - tests and actual classes/scala code it is converting the configuration maps value to value.toString(). I verified that was the issues by fixing some of it but still evaluating how many places we need to fix if we really want to support Class Type key for the configuration. Password and List Types are also of non-String types but it seems their defaults are Null and they are optional vs in my case the ssl.engine.factory.class is *not* optional - it needs a value by default.
Will keep you posted. I am thinking of reverting the config type to String and then load it as String and do Class loading in SslFactory. Thanks Maulin On Mon, Mar 23, 2020 at 1:38 AM Maulin Vasavada <maulin.vasav...@gmail.com> wrote: > still working on the pull request. hopefully will be done soon. > > On Wed, Mar 11, 2020 at 11:48 AM Maulin Vasavada < > maulin.vasav...@gmail.com> wrote: > >> Thanks Rajini. Sounds good. I'll make changes and update this thread. >> >> On Wed, Mar 11, 2020 at 6:41 AM Rajini Sivaram <rajinisiva...@gmail.com> >> wrote: >> >>> Hi Maulin, >>> >>> I have reviewed the PR and left some comments, can you turn it into a PR >>> for https://github.com/apache/kafka? It looks good overall. >>> >>> We pass all configs to other plugins, so we can do the same here. That >>> would be safer than assuming that all custom SSL-related configs start >>> with >>> `ssl.`. You can use principal builder as an example for what we do today. >>> >>> Regards, >>> >>> Rajini >>> >>> On Thu, Mar 5, 2020 at 12:03 AM Maulin Vasavada < >>> maulin.vasav...@gmail.com> >>> wrote: >>> >>> > 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. >>> > >> >>> > > >>> > >>> >>
