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 >
