Hi Taras, Thanks for the KIP! I have some feedback but ultimately I like this proposal:
1. The "ssl.engine.factory.class" property was originally added for Kafka brokers in KIP-519 [1]. It'd be nice to link to that KIP (possibly in a "Background" section?) so that reviewers who don't have that context can find it quickly without having to dig through commit histories in the code base. 2. Can we clarify that the new "listeners.https.ssl.engine.factory.class" property (and the way that the engine factory is configured with all properties prefixed with "listeners.https.") will also affect MirrorMaker 2 clusters with the internal REST server introduced by KIP-710 [2] enabled? 3. We don't need to specify in the KIP that the org.apache.kafka.connect.runtime.rest.util.SSLUtils class will be removed, since that class isn't part of public API (i.e., nobody should be using that class directly in external projects). If you're ever in doubt about which classes are part of the public API for the project, you can check the Javadocs [3]; if it's part of our public API, it should be included in them. The same applies for changes to the org.apache.kafka.common.security.ssl.SslFactory class. 4. The test plan includes an integration test for "Default SSL behavior and compatibility"--is this necessary? Doesn't the existing org.apache.kafka.connect.integration.RestForwardingIntegrationTest give us sufficient coverage already? Similarly, the test plan includes an integration test for "RestClient creation" and calls out the RestForwardingIntegrationTest--don't we already create RestClient instances in that test (like here [4])? It seems like this part of the KIP may implicitly include tests that are already covered by the existing code base, but if that's the case, it'd be nice to see this clarified as the assumption is usually that items in the test plan cover changes that will have to be implemented for the KIP. 5. There are several methods in the SslEngineFactory interface that don't seem applicable for Kafka Connect (or MM2): shouldBeRebuilt(Map<String, Object> nextConfigs), reconfigurableConfigs(), and possibly keystore() and truststore(). Does it make sense to require users to implement these? It seems like a new interface may make more sense here. [1] - https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=128650952 [2] - https://cwiki.apache.org/confluence/display/KAFKA/KIP-710%3A+Full+support+for+distributed+mode+in+dedicated+MirrorMaker+2.0+clusters [3] - https://kafka.apache.org/36/javadoc/index.html?overview-summary.html [4] - https://github.com/apache/kafka/blob/9dbee599f13997effd8f7e278fd7256b850c8813/connect/runtime/src/test/java/org/apache/kafka/connect/integration/RestForwardingIntegrationTest.java#L161 Cheers, Chris On Thu, Oct 12, 2023 at 7:40 AM Taras Ledkov <tled...@apache.org> wrote: > Hi Ashwin, > > > I was referring to (and did not understand) the removal of L141 in > clients/src/main/java/org/apache/kafka/common/security/ssl/SslFactory.java > This line is moved to "new" private method `instantiateSslEngineFactory0 > `. Please take a look at the `SslFactory:L132` at the patch. > Just dummy refactoring. > > > Yes, I think this class [SslEngineFactory] should be moved to something > like `server-common` module - but would like any of the committers to > comment on this. > Sorry, not catch an idea. > SslEngineFactory - public interface is placed at the 'clients' project. I > don't know a more common place >
