Hi Robert, Since I've added the same feature to the operator I'll take a look at it. Though it won't be lightning fast since I'm having several weeks off.
Your questions are valid especially considering the fact that this feature touches the hearth of the authentication so this must be rock solid in order to avoid grey hair :) (1) I would vote on a single service which is heavily unit tested with all the possible combinations including threading. Some standalone app could be added to really play with it (that would help review). I mean, create X files, start Y threads, and make assertions. The reason why I'm suggesting it is the fact that AFAIR the watch service is quite sensitive even in single thread. If we could do this in a finite time consuming unit test then it's even better. (2) +1 on that name to avoid confusion (3) I agree that some e2e is must, however this can be easily and deeply unit tested so that part is also essential. One key test here is when certificates are not changing then no action must be performed (not to break the whole system apart). Purely personal opinion but such feature developments are slow by nature because of edge case / stress testing. BR, G On Fri, Apr 18, 2025 at 4:53 PM Robert Metzger <rmetz...@apache.org> wrote: > Hi Nicolas, > > This looks like a nice improvement, thanks for the write up. > Are you in touch with any committer who's willing to review / merge this? > > Some random questions on the FLIP: > (1) "Each service that depends on TLS certificates will initialize a > FileSytemWatchService" > > It seems that there are 4 components using SSL, does this mean there will > be 4 additional threads running, watching the same set of files? > Wouldn't it be better to introduce a central file watching service, and SSL > users can subscribe to updates, to reduce the number of threads? > If this makes the whole effort 4x more complicated, I wouldn't consider it, > but if its roughly the same effort, we should :) > > (2) "FileSytemWatchService" > When I read this name, I was wondering, whether this is somehow related to > the Flink "FileSystem" classes. Which I think its' not. > Maybe a different name, that makes this separation more explicit, would > make sense. Maybe "LocalFSWatchService"? > (I'm sorry to bring up naming stuff -- its very subjective, and difficult) > > (3) For the test plan: There seem to be some SSL related e2e tests: > > https://github.com/apache/flink/blob/master/flink-end-to-end-tests/test-scripts/common_ssl.sh > It would be nice to extend them to cover this feature as well. I would hate > for this feature to slowly break by future changes, so good e2e test > coverage is key, in particular bc so many components are involved. > > Best, > Robert > > On Wed, Apr 16, 2025 at 11:55 AM Nicolas Fraison > <nicolas.frai...@datadoghq.com.invalid> wrote: > > > Hi All, > > > > I'd like to start a discussion to Handle TLS Certificate Renewal > > Please provide some feedback on this proposal: > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-523%3A+Handle+TLS+Certificate+Renewal > > > > Regards, > > > > Nicolas Fraison > > >
