Hi Chris, Thank you for your comments above. I disagree with your recommendation for a new SslEngineFactory variant/hierarchy.
1. A superinterface could be more confusing to users. Since this is an interface in `clients`, the connect-specific interface would also need to be in clients, despite being superfluous for clients users and broker developers. 2. A user could implement the reduced interface, and then have issues loading their implementation in a broker, and need to find documentation/javadocs to explain to them why. 3. This interface has been in use since 2020, so I don't believe that the burden of implementing these methods has been excessive. I assume there's at least one "static" implementation out there that would have benefitted from the proposed super-interface, but they managed to adapt to the standardized interface. 4. Implementations that don't want to provide basic implementations can throw UnsupportedOperationException from the extra methods as a last resort. On the other hand, how much would it take to support the full suite of SslEngineFactory operations in Connect? Could part of this KIP be improving Connect to make use of these methods? This would help unify the experience for users that implement the interface specifically for the dynamic reconfiguration support, and rely on it on the broker side. Taras, are you interested in dynamic SSL reconfiguration in Connect? Would you be willing to investigate the details of that for the KIP? Thanks, Greg On Mon, Dec 4, 2023 at 1:17 PM Chris Egerton <chr...@aiven.io.invalid> wrote: > > Hi Taras, > > Regarding slimming down the interface: IMO, we should do this right the > first time, and that includes not requiring unnecessary methods from users. > I think BaseSslEngineFactory is good enough as a superinterface. > > > Regarding the parsing logic: I think the KIP needs to be more explicit. We > should add something like this to the proposed changes section: > > "If any properties are present in the worker config with a prefix of > "listeners.https.", then only properties with that prefix will be passed to > the SSL engine factory. Otherwise, all top-level worker properties will be > passed to the SSL engine factory. Note that this differs slightly from > existing logic in that the set of properties (prefixed or otherwise) will > not be filtered based on a predefined set of keys; this will enable custom > SSL engine factories to define and accept custom properties." > > I also took a quick look at the prototype (I usually try not to do this > since we vote on KIP documents, not PRs). I don't think we should populate > default values for SSL-related properties before sending properties to the > SSL engine factory, since it may confuse users who have written custom SSL > engine factories to see that properties not specified in their Connect > worker config are being passed to their factory. Instead, the default SSL > engine factory used by Connect can perform this logic, and we can let other > custom factories be responsible for their own default values. > > > Cheers, > > Chris > > On Wed, Nov 29, 2023 at 8:36 AM Taras Ledkov <tled...@apache.org> wrote: > > > Hi team, > > > > Ping for review / vote for KIP-967 [1]. > > Voting thread is here [2] > > > > [1]. > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer > > [2]. https://github.com/apache/kafka/pull/14203 > > [2]. https://lists.apache.org/thread/wc4v5v3pynl15g1q547m2croqsqgmzpw > > > > -- > > With best regards, > > Taras Ledkov > >
