Hi Clement/Rajini I've gone through the code to understand how reconfigruation work currently. I sent both of you a note separately also. Let us reconvene next week and proceed.
Thanks Maulin On Thu, Sep 12, 2019 at 12:25 PM Pellerin, Clement <clement_pelle...@ibi.com> wrote: > You are proposing a different design pattern for the SSL reconfigurable > extension point than the one already implemented for MetricsReporter. You > need a good reason to justify this decision. It is as if you consider > SslFactory the extension point. This is indeed something I proposed > multiple times and was always shut down. > > Going back to your latest proposal, notice you cannot call > reconfigurableConfigs() until configure() is called because you need to > instantiate SslEngineFactory() first. There is no way to enforce this in > the API. > > You did not react to my observation that ConfigDef does a better job > validating/casting configs based on the ConfigKey declarations compared to > ad hoc code you force people to write in their extension point > implementations. > > You suggest not to augment ConfigDef with custom configs, so that takes > care of the recursive dependency. > I just noticed reconfigurableConfigs() returns Set<String> and that does > not force the creation of a ConfigKey for custom configs. > > >> SslFactory.reconfigurableConfigs() returns all standard reconfigurable > SSL configs as well as MySslEngineFactory.reconfigurableConfigs(). > That's no good because that implementation does not use the standard > property for the keystore location. For that particular use case, it would > probably be better to reuse the standard keystore location config and > change its semantics to a URL. Regardless, my point is the custom > implementation should decide all the reconfigurable properties. By the way, > my original use case for KIP-383 was to replace all SSL configs with a > single name. > > It is still not clear in your email if the keystore/truststore exception > is handled locally in SslFactory or by the initiator of the whole > AlterConfig. That determines whether "AlterConfig without config changes" > always goes through or is usually blocked early by the initiator. > > > -----Original Message----- > From: Rajini Sivaram [mailto:rajinisiva...@gmail.com] > Sent: Thursday, September 12, 2019 2:05 PM > To: dev > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > extensible > > Sorry about the confusion, my notes were conflicting! > > Let me give an example to clarify. Let's say KIP-519 adds a new interface > SslEngineFactory and I have a custom implementation MySslEngineFactory that > gets keystore material from a web service using a custom config ` > my.ssl.keystore.url`. MySslEngineFactory.reconfigurableConfigs() returns {` > my.ssl.keystore.url`}. We don't want to add this custom config to Kafka. My > SslEngineFactory will be managed by the (non-pluggable) Reconfigurable > SslFactory that invokes appropriate methods on MySslEngineFactory to create > a new SslEngine when required. SslFactory.reconfigurableConfigs() returns > all standard reconfigurable SSL configs as well as > MySslEngineFactory.reconfigurableConfigs(). > > The existing code in the broker is sufficient for both validation and > reconfiguration. We can support custom configs as well as reconfiguration > without config change. The only non-SSL change we require is a fix for > KAFKA-7588. > > 1) *Initial validation*: SslFactory.configure() invokes > MySslEngineFactory#configure() which validates the custom config ` > my.ssl.keystore.url`. Broker will fail to start if the URL is invalid even > though broker knows nothing about this config because the custom > implementation fails its `configure()`. > 2) *Validation during reconfiguration*: AdminClient sends an AlterConfig > request to update `my.ssl.keystore.url` Broker invokes. > SslFactory.validateReconfiguration() which will invoke > MySslEngineFactory.validateReconfiguration() and that can fail > reconfiguration if the provided URL is invalid. And AdminClient sees this > validation failure in its response. This validation is more flexible than > that provided by ConfigDef. During reconfiguration, we are not just > validating that the value is of the right type, we may want to verify that > you can connect to the remote web service for example. This validation path > for custom configs is already supported. > 3) *Reconfiguration without config change*: This is supported specifically > for two configs `ssl.keystore.location` and `ssl.truststore.location`. > SslFactory is given the opportunity to reconfigure whenever there is an > AlterConfig request from AdminClient since it uses these configs. This is > regardless of whether there is a config change. SslFactory will invoke > MySslEngineFactory.shouldBeRebuilt(). This gives MySslEngineFactory the > opportunity to provide a new SslEngine whenever there is an AdminClient > request to alter configs, regardless of whether any config changed or not. > > Hope that helps, > > Rajini > > On Thu, Sep 12, 2019 at 5:26 PM Pellerin, Clement < > clement_pelle...@ibi.com> > wrote: > > > I'm confused. Can you launch a reconfiguration without a config change or > > not? > > > > If I understand the test case correctly, the design pattern to implement > a > > reconfigurable extension point in Kafka is to implement the > Reconfigurable > > interface. This means SslEngineFactory would be created once and mutate > > with reconfigure. There is no ConfigKey created for the custom config and > > therefore there is no validation by ConfigDef. > > > > Optionally, to expose the built-in validation, it might be worthwhile to > > consider making ConfigKey a public API and move an individual config > parse > > from ConfigDef to ConfigKey. It would be more object oriented anyway. > > > > One of the use case of custom configs in SslEngineFactory is to remove > the > > need to specify the keystore and truststore locations. The special > handling > > to detect changes in keystore/truststore should be pushed to > > DefaultSslEngineFactory and all calls to reconfigure should reach the > > SslEngineFactory instance. Am I missing something? > > > > -----Original Message----- > > From: Rajini Sivaram [mailto:rajinisiva...@gmail.com] > > Sent: Thursday, September 12, 2019 12:01 PM > > To: dev > > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > > extensible > > > > Correction on my previous email. Custom implementations can use > AlterConfig > > request without a config change by including `ssl.keystore.location` or > > `ssl.truststore.location` in their `reconfigurabkeConfigs()`. This will > > trigger the same codepath as we use for keystore updates when the file > has > > changed. > > > > On Thu, Sep 12, 2019 at 4:43 PM Rajini Sivaram <rajinisiva...@gmail.com> > > wrote: > > > > > Hi Clement, > > > > > > Kafka does special handling for keystore/truststore file changes when > an > > > AlterConfig request is processed, but that is not easy to extend to > > custom > > > configs. I was thinking we could just add a custom config to trigger > > > reconfiguration. For example, a config like > `my.custom.keystore.version` > > that > > > is incremented when the custom implementation detects a change in > > keystore. > > > And the custom implementation would invoke admin client to update > > `my.custom.keystore.version`. > > > Kafka would do the rest to reconfigure SslFactory. And the custom > > > implementation can then create the new builder. > > > > > > For an example of custom config reconfiguration, this test may be > useful: > > > > > > https://github.com/apache/kafka/blob/d3559f628b2ccb23a9faf531796675376ac06abb/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala#L792 > > > > > > > > > > > > On Thu, Sep 12, 2019 at 3:24 PM Pellerin, Clement < > > > clement_pelle...@ibi.com> wrote: > > > > > >> For the push notification, Rajini prefers if the trigger to > reconfigure > > >> comes from an admin client. He says the admin client can reconfigure > > even > > >> though none of the config values have changed. This allows your custom > > impl > > >> to discover other conditions that have changed, for example the > > contents of > > >> the keystore. > > >> > > >> @Rajini, can you point us to an existing example of a Kafka extension > > >> point that implements reconfigurable custom configs. This way we could > > >> study it and do the same thing. Consistency is good. It would be nice > if > > >> there was a KIP that describes that design pattern. > > >> > > >> -----Original Message----- > > >> From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] > > >> Sent: Thursday, September 12, 2019 2:24 AM > > >> To: dev@kafka.apache.org > > >> Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > > >> extensible > > >> > > >> Thanks Clement and Rajini. Let me digest what both of you said. > Clearly > > I > > >> need to understand more about the configurations - dynamic, custom > etc. > > >> > > >> On the 'push notification' question Clement asked, > > >> The way I see is simple - Kafka defines the interface for > > >> SslEngineFactory. > > >> Implementation of that interface is owned by the Kafka operator who > > >> customized the implementation. Let us say, my SslEngineFactoryImpl > ONLY > > >> customizes loading of keys/certs where it knows when they are updated. > > How > > >> is Kafka going to know that? You said 'next time it loads' but if > there > > is > > >> NO additional configuration that was needed by my > SslEngineFactoryImpl, > > >> there won't be any trigger coming from Kafka to reconfigure and > > SslFactory > > >> will never re-create my SslEngineFactoryImpl code which will help > Kafka > > >> use > > >> new keys/certs in the next calls. Please let me know if this makes my > > >> 'push > > >> notification' needs clearer. > > >> > > >> Thanks > > >> Maulin > > >> > > >> > > >> > > >> > > >> > > >> > > >> On Wed, Sep 11, 2019 at 2:31 PM Pellerin, Clement < > > >> clement_pelle...@ibi.com> > > >> wrote: > > >> > > >> > Indeed, this is a general problem requiring a more general solution > > than > > >> > KIP-519. I'm glad there was work done on this already. > > >> > > > >> > So config.originals() still contains unknown configs but nothing has > > >> been > > >> > validated and cast to the proper type. > > >> > How does validation work for an extension point that receives > > >> > config.originals()? Is there a public validator helper to handle > this? > > >> > Do we need to create ConfigKeys in the ConfigDef for custom configs > > only > > >> > known to a custom SslEngineFactory implementation? > > >> > Do we need to declare the standard SSL configs in ConfigDef if > > >> SslFactory > > >> > needs to revalidate them anyway because it receives > > config.originals()? > > >> > I assume there is such a thing as config.originals() also for a > > >> > reconfigure()? > > >> > > > >> > If we implement KIP-519 and later change from config.values() to > > >> > config.originals(), this will affect the contract for the > constructor > > of > > >> > the SslEngineFactory. We might need to add custom configs support to > > >> > KIP-519 or delay KIP-519 until the change to config.originals(). > > >> > > > >> > > > >> > -----Original Message----- > > >> > From: Rajini Sivaram [mailto:rajinisiva...@gmail.com] > > >> > Sent: Wednesday, September 11, 2019 4:25 PM > > >> > To: dev > > >> > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine > configuration > > >> > extensible > > >> > > > >> > Kafka already has the notion of custom configs. And we support > > >> > reconfigurable custom configs for some interfaces e.g. > > MetricsReporter. > > >> We > > >> > also recently added custom reconfigurable configs for Authorizer > under > > >> > KIP-504. > > >> > > > >> > The issue with custom configs for SSL is described in > > >> > https://issues.apache.org/jira/browse/KAFKA-7588. We currently > don't > > >> pass > > >> > in custom configs to ChannelBuilders. We need to fix this, not just > > for > > >> SSL > > >> > but for other security plugins as well. So it needs to be a generic > > >> > solution, not specific to KIP-519. > > >> > > > >> > Once KAFKA-7588 is fixed, the existing dynamic reconfiguration > > >> mechanism in > > >> > brokers would simply work. Dynamic configs works exactly in the same > > way > > >> > for custom configs as it does for other configs. The list of > > >> reconfigurable > > >> > configs is returned by the implementation class and the class gets > > >> notified > > >> > when any of those configs changes. This includes > > >> validateReconfiguration() > > >> > as well the actual reconfigure(). > > >> > > > >> > For SSL alone, we have special handling of dynamic configs to enable > > >> > reloading of keystores/truststores when the file changes, even > though > > >> none > > >> > of the config values have changed. Reconfiguration is triggered by > an > > >> admin > > >> > client request to alter configs. In this case, none of the actual > > >> configs > > >> > may have changed, but we check if the file has changed. This is > > >> currently > > >> > done only for the standard keystore/truststore configs. With > KIP-519, > > I > > >> > guess we want the custom SslEngineFactory to be able to decide > whether > > >> > reconfiguration is required. The simplest way to achieve this would > be > > >> to > > >> > have a custom config that is updated when reconfiguration is > required. > > >> I am > > >> > not sure we need a separate mechanism to trigger reconfiguration > that > > >> > doesn't rely on admin clients since admin clients provide useful > > logging > > >> > and auditability. > > >> > > > >> > Regards, > > >> > > > >> > Rajini > > >> > > > >> > On Wed, Sep 11, 2019 at 4:13 PM Pellerin, Clement < > > >> > clement_pelle...@ibi.com> > > >> > wrote: > > >> > > > >> > > I'm sorry if I divert the discussion, but without this issue, it > > would > > >> > > have been pretty trivial to update KIP-383 to go as far as you > did. > > I > > >> am > > >> > > also happy to get a discussion going, the KIP-383 thread was a > > >> desolate > > >> > > place. > > >> > > > > >> > > Kafka needs to know about custom configs because it validates the > > >> configs > > >> > > before it passes them to (re)configure. Unknown configs are > silently > > >> > > removed by ConfigDef. We could keep unknown configs as strings > > without > > >> > > validating them in ConfigDef, but I don't know if the Kafka > > community > > >> > would > > >> > > accept this weaker validation. > > >> > > > > >> > > It appears we are trying to invent the notion of a meta config. > > >> Anyway, I > > >> > > think we have shown asking an instance of SslEngineFactory to > > >> contribute > > >> > to > > >> > > ConfigDef is way too late. > > >> > > > > >> > > For your push notification, would it be simpler to just let your > > >> > > SslEngineFactory notice the change and make it effective the next > > >> time it > > >> > > is called. SslFactory would not cache the SslEngine and always ask > > >> > > SslEngineFactory for it. You don't even need an inner thread if > > >> > > SslEngineFactory checks for a change when it is called. > > >> SslEngineFactory > > >> > > would no longer be immutable, so maybe it is worth reconsidering > how > > >> > > reconfigure works for it. > > >> > > > > >> > > -----Original Message----- > > >> > > From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] > > >> > > Sent: Wednesday, September 11, 2019 3:29 AM > > >> > > To: dev@kafka.apache.org > > >> > > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine > > configuration > > >> > > extensible > > >> > > > > >> > > Hi all, > > >> > > > > >> > > Since the "custom config" seems the main topic of interest let us > > talk > > >> > > about it. > > >> > > > > >> > > 1. I want to confirm that I interpret the definition of 'custom > > >> config of > > >> > > SslEngineFactory' the same way Clement is suggesting - "a config > > that > > >> > does > > >> > > not exist in Kafka but is specified by a custom implementation of > > >> > > SslEngineFactory". If there is a disagreement to that we have to > > >> bring > > >> > it > > >> > > up here sooner. > > >> > > > > >> > > 2. I've been thinking about it and I question why we are trying to > > >> make a > > >> > > custom config a first class citizen in standard config? > > >> > > The reasoning for that question is- > > >> > > Kafka wants to delegate creation of SSLEngine to a class which is > > >> "not" > > >> > > part of Kafka's distribution. Since the interface for SSLEngine > > >> creator > > >> > > will be defined by the public method of createSSLEngine(), why > would > > >> > Kafka > > >> > > care what does the implementation do other than fulfilling the > > >> contract > > >> > of > > >> > > creation of SSLEngine. The implementation can use any special > > configs > > >> > i.e. > > >> > > configs coming from input Map OR configs defined in a new file > only > > >> known > > >> > > to itself. Making the configs part of the interface contract in > any > > >> way > > >> > is > > >> > > not necessary. This way we keep it simple and straightforward. > > >> > > > > >> > > 3. Now, 2nd point raises a question - if we follow that > suggestion - > > >> how > > >> > > can we ever re-create the SSLEngineFactory object and allow new > > >> object to > > >> > > be created when something changes in the implementation. That is a > > >> valid > > >> > > question. If you noticed in the KIP section titled "Other > challenge" > > >> - we > > >> > > do have scenario where the SslEngineFactory implementation ONLY > > knows > > >> > that > > >> > > something changed - example: keystore got updated by a local > daemon > > >> > process > > >> > > only known to the specific implementation. This means we have a > need > > >> of > > >> > > "push notification" from the SslEngineFactory's implementation to > > the > > >> > > SslFactory actually. I feel if we build the "push notification" > via > > >> > adding > > >> > > a method in the interface as "public void > > >> > > registerReconfigurableListener(Reconfigurable r)" and make > > SslFactory > > >> > > register itself with the SslEngineFactory's impl class, we can > > trigger > > >> > the > > >> > > reconfiguration of SslEngineFactory implementation based on its > own > > >> terms > > >> > > and conditions without getting into making custom configs > > complicated. > > >> > > > > >> > > I am just thinking out loud here and expressing my opinion not to > > >> avoid > > >> > > addressing custom configs BUT what I genuinely believe might be a > > >> better > > >> > > approach. > > >> > > > > >> > > Thanks > > >> > > Maulin > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > > On Tue, Sep 10, 2019 at 9:06 PM Pellerin, Clement < > > >> > > clement_pelle...@ibi.com> > > >> > > wrote: > > >> > > > > >> > > > Regarding what I labeled the simplest solution below, SslConfigs > > >> could > > >> > > > instantiate the custom interface only if the yet to be validated > > >> > configs > > >> > > > were passed in to the call to get the list of known SSL configs. > > >> > > > > > >> > > > -----Original Message----- > > >> > > > From: Pellerin, Clement > > >> > > > Sent: Tuesday, September 10, 2019 11:36 AM > > >> > > > To: dev@kafka.apache.org > > >> > > > Subject: [EXTERNAL]RE: [DISCUSS] KIP-519: Make SSL > context/engine > > >> > > > configuration extensible > > >> > > > > > >> > > > Another solution could be a new standard ssl config that holds a > > >> list > > >> > of > > >> > > > extra custom configs to accept. > > >> > > > Using a custom SslEngineFactory with custom configs would > require > > >> > setting > > >> > > > two configs, one for the class name and another for the list of > > >> custom > > >> > > > configs. > > >> > > > In SslConfigs, we see that declaring a single config takes 5 > > >> values, so > > >> > > > I'm not sure how it would work exactly. > > >> > > > > > >> > > > We could also declare a new interface to return the list of > custom > > >> ssl > > >> > > > configs and the extra standard ssl config I'm proposing holds > the > > >> name > > >> > of > > >> > > > the implementation class instead. The reason a different > interface > > >> is > > >> > > > needed is because it would be instantiated by SslConfigs, not > > >> > SslFactory. > > >> > > > This seems the simplest solution. > > >> > > > > > >> > > > Anyway, the point of this exercise is to prove an acceptable > > >> solution > > >> > for > > >> > > > custom configs is not affecting the public API in KIP-519. > > >> > > > > > >> > > > > > >> > > > -----Original Message----- > > >> > > > From: Pellerin, Clement > > >> > > > Sent: Tuesday, September 10, 2019 9:35 AM > > >> > > > To: dev@kafka.apache.org > > >> > > > Subject: [EXTERNAL]RE: [DISCUSS] KIP-519: Make SSL > context/engine > > >> > > > configuration extensible > > >> > > > > > >> > > > Custom config is a term I invented to mean a config that does > not > > >> exist > > >> > > in > > >> > > > Kafka but is specified by a custom implementation of > > >> SslEngineFactory. > > >> > > > The problem with custom configs (as I remember it) is that the > > list > > >> of > > >> > > > configs is static in SslConfigs and config names are checked > > before > > >> > > > SslFactory is created. > > >> > > > ==> You must solve this problem because that is what killed > > KIP-383 > > >> and > > >> > > > therefore is the sole reason why KIP-519 exists. > > >> > > > ==> Your KIP does not have to implement the solution since it > can > > be > > >> > done > > >> > > > in a future KIP, but your KIP must be compatible with the > solution > > >> > found. > > >> > > > ==> A method to return the list of configs would help. This > cannot > > >> be a > > >> > > > static method in the interface since it cannot be overridden. > > >> > > > ==> You could require a static method in the implementation > class > > by > > >> > > > convention, just like the constructor you require. > > >> > > > > > >> > > > This email did not originate from inside Information Builders. > > >> > > > > > >> > > > > >> > > > >> > > > > > >
