Hi Clement, Thanks for pointing to AbstractConfig. Now I understand what you were saying. I'll respond by tonight with more thoughts.
Thanks Maulin On Thu, Sep 19, 2019 at 5:46 AM Pellerin, Clement <clement_pelle...@ibi.com> wrote: > I appreciate the effort you put into this. > > Lets do this in steps. You had a question on getConfiguredInstance(). > > The method getConfiguredInstance(key, Class) implemented in AbstractConfig > is how the MetricsReporter and other extension points are intantiated. > Creating the extension point this way calls the default constructor which > is good. Since the (Re)Configurable interface dictates the signature of the > configure() method, that forces the addition of a new init(...) method to > pass the other constructor arguments. > > Do we agree on that before we move on to other issues? > > -----Original Message----- > From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] > Sent: Wednesday, September 18, 2019 4:37 PM > To: dev@kafka.apache.org > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > extensible > > Hi Clement > > Here are my thoughts based on my latest re-write attempt and learnings, > > 1. I think that it will be a great value to keep both classes separate - > SslFactory and SslEngineFactory and having method reconfigurableConfigs() > in the SslEngineFactory. Here is the reasoning, > > a. It is kind of a Decorator pattern to me - even without named like one > SslFactory is acting as a decorator/surrogate to the SslEngineFactory and > helping it get created and re-created as needed based on the > terms/conditions specified by SslEngineFactory (via reconfigurableConfigs() > method) > > b. SslEngineFactory will be pluggable class. By keeping the SslFactory > reconfigurable with delegation of reconfigurableConfigs() to > SslEngineFactory it allows the implementation of SslEngineFactory to be > worry free of - How Kafka manages reconfigurations. The contract is - > Kafka's SslFactory will ask the implementation to provide which > configurations it is ready to be reconfigured for. Rest of the logic for > triggering and reconfiguring and validation is in SslFactory. > > c. The current validation in SslFactory about inter-broker-ssl handshake > AND verifying that certificate chain doesn't change via dynamic config > changes is rightly owned by SslFactory. We should not give flexibility to > SslEngineFactory to decide if they want that validation or not. > > d. If SslEngineFactory fails to be re-created with new dynamic config > changes the constructor will throw some exception and the SslFactory will > fail the validateReconfiguration() call resulting in no-change. Hence the > validation if the new config is right is still controlled by the > SslEngineFactory without necessarily having explicit validate method > (assuming if you had a point about - we should keep validation of changed > configs in the pluggable class) > > > 2. About the keystore validation in SslFactory - as I mentioned in above > points, > > a. I feel it is Kafka's policy that it wants to mandate that validation > regardless of the SslEngineFactory's implementation. I feel that regardless > of customized implementation it is doing a 'logical' enforcement. I don't > see many cases where you will end up changing certificate chain (I can't > say the same about SANs entries though. see my below points). Hence that > validation is reasonable to be generally enforced for dynamic config > changes. If you change something violating that validation, you can avoid > making such changes via dynamic configuration and do a rolling restarts of > the boxes. > > b. If the implementation doesn't use keystore then automatically no > validation will happen. Hence I don't see any issue with SslEngineFactory's > implementations not having requirement to use keystores. > > c. There could be an argument however about - what it validates currently > and is there a scope of change. Example: It validates SANs entries and that > to me is a challenge because I have had scenarios where I kept adding more > VIPs in my certs SANs entries without really changing any certificate > chain. The existing validation will fail that setup unnecessarily. Given > that - there could be change in SslFactory but that doesn't still make that > validation eligible to go to SslEngineFactory implementations. > > > 3. I am still in two minds about your point on - not using existing SSL > Reconfigurable configs to be used by SslFactory on top of > SslEngineFactory's reconfigurable configs. The reason for that is- > > a. I agree with you on that we should not worry about existing SSL > reconfigurable configs in new changed code for SslFactory. Why depend on > something you really don't need. However, Rajini's point is- if we decide > to add more configs in the SSL reconfigurable configs which may be common > across SslEngineFactory's implementations, it will make it easier. Again, > just to make it easier we should not do it upfront. So now you see why I am > double minded on it while more leaning toward your suggestion. > > 4. I think I totally miss what you refer by > config.getConfiguredInstance(key, Class). Which Kafka existing class you > are referring to when you do that? Do we have that in KafkaConfig? If you > can clarify me on that I can think more about your input on it. > > 5. Now above all means- > > a. We will have createEngine(), reconfigurableConfigs(), keystore(), > truststore() methods in the SslEngineFactory interface. However the return > type for keystore and truststore method can't be existing SecurityStore. > For that I already thought of the replacement with KeystoreHolder class > which only contains references to java's KeyStore object and Kafka's > Password object making it feasible for us to return non-implementation > specific return type. > > b. We didn't talk about shouldBeRebuilt() so far at all given other > important conflicts to resolve. We will get to it once we can hash out > other stuff. > > 6. On Rajini's point on 'push notifications' for client side code when the > key/trust store changes, > > " - For client-side, custom SslEngineFactory implementations could > reconfigure themselves, we don't really need SslFactory to be involved > at all." > > I think I am missing something. If we just have SslEngineFactory > reconfigure itself it will generate new SSLContext and in-turn new > SSLEgnine but how will it communicate that to the ChannelBuilders? Don't > they have to refresh the reference to the SslEngineFactory via SslFactory's > reconfigure() method in order to pick up that change? > > > Thanks > Maulin > > > > > > > > > > > > On Tue, Sep 17, 2019 at 4:49 AM Pellerin, Clement < > clement_pelle...@ibi.com> > wrote: > > > Good point about the two callers of SslFactory. We can move the SslEngine > > validation to a separate class and call it in both places. That SslEngine > > validation class would not be part of the public API and therefore we > don't > > need to fuss about its API. > > > > -----Original Message----- > > From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] > > Sent: Tuesday, September 17, 2019 2:28 AM > > To: dev@kafka.apache.org > > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > > extensible > > > > Hi Clement/Rajini > > > > When I read your responses - I swing between both of your suggestions :) > I > > see both of your points. Let me ponder little bit more and give me take > in > > a day or so. > > > > I tend to agree with Clement in a sense that we need to define clear > > responsibilities of classes. Right now I feel it's not clear. Also, I > tend > > to agree to both of you about keystore/truststore validation - the > conflict > > I've to propose a clean agreeable solution to. > > > > One clarification to Clement is - there are two classes using SslFactory > > today - SslChannelBuilder and SaslChannelBuilder so we have to keep that > in > > mind. However, once we have clear responsibilities of classes, that > should > > automatically clear what goes where. > > > > Thanks > > Maulin > > >
