Hi, This remaining part of the doc has been updated
On Thu, Jun 19, 2025 at 9:37 AM Gabor Somogyi <gabor.g.somo...@gmail.com> wrote: > I think in this SSLContextLoader class call() function is not correct and > must be onFileOrDirectoryModified(...), right? > > [image: Screenshot 2025-06-19 at 9.35.14.png] > > On Thu, Jun 19, 2025 at 8:14 AM Nicolas Fraison < > nicolas.frai...@datadoghq.com> wrote: > >> Hi, >> >> Which part of the doc are you referring to? >> >> On Wed, Jun 18, 2025 at 7:08 PM Gabor Somogyi <gabor.g.somo...@gmail.com> >> wrote: >> >>> There is still a call function there which is left there from the >>> previous >>> design... >>> >>> On Tue, Jun 17, 2025 at 9:48 AM Nicolas Fraison >>> <nicolas.frai...@datadoghq.com.invalid> wrote: >>> >>> > Done >>> > >>> > On Mon, Jun 16, 2025 at 2:46 PM Gabor Somogyi < >>> gabor.g.somo...@gmail.com> >>> > wrote: >>> > >>> > > I still see 3 Callable occurrence in the FLIP... >>> > > >>> > > On Thu, Jun 12, 2025 at 2:16 PM Nicolas Fraison >>> > > <nicolas.frai...@datadoghq.com.invalid> wrote: >>> > > >>> > > > Hi, FLIP has been updated with last comment >>> > > > >>> > > > On Thu, Jun 5, 2025 at 6:35 PM Gabor Somogyi < >>> > gabor.g.somo...@gmail.com> >>> > > > wrote: >>> > > > >>> > > > > The interface has been added. Do we intend to pass an >>> > > > > instance registerWatchedPath function as we discussed? >>> > > > > If yes then the FLIP needs further adjustments all places where >>> now >>> > > > > Callable provided. >>> > > > > >>> > > > > G >>> > > > > >>> > > > > >>> > > > > On Thu, Jun 5, 2025 at 2:27 PM Nicolas Fraison >>> > > > > <nicolas.frai...@datadoghq.com.invalid> wrote: >>> > > > > >>> > > > > > Hi, >>> > > > > > >>> > > > > > It indeed make sense, FLIP has been updated >>> > > > > > >>> > > > > > Nicolas >>> > > > > > >>> > > > > > On Wed, Jun 4, 2025 at 12:10 PM Gabor Somogyi < >>> > > > gabor.g.somo...@gmail.com >>> > > > > > >>> > > > > > wrote: >>> > > > > > >>> > > > > > > Hi, >>> > > > > > > >>> > > > > > > I've read it through. Basically looks good with one comment. >>> > > > > > > *registerWatchedPath function* has a *Callable* as callback. >>> For >>> > > the >>> > > > > > > current implementation that would be enough, >>> > > > > > > but when somebody would like to use that for any other >>> use-case >>> > > then >>> > > > it >>> > > > > > > would be hard. Examples: >>> > > > > > > * A single *call* function tells the user nothing what kind >>> of >>> > > event >>> > > > is >>> > > > > > > this >>> > > > > > > * the watch service supports 3 events (create, modify, >>> delete), >>> > now >>> > > > > when >>> > > > > > I >>> > > > > > > register I can get only updates (I presume) >>> > > > > > > * 1+ dir watch with the same callback is not possible >>> > > > > > > >>> > > > > > > My suggestion would be to create a proper callback with all >>> the >>> > > event >>> > > > > > type >>> > > > > > > functions and no-op default behavior with the following >>> names[1]. >>> > > > > > > This is ~30 lines addition but will increase the readability >>> > > heavily >>> > > > + >>> > > > > no >>> > > > > > > need to touch this code later. >>> > > > > > > >>> > > > > > > BR, >>> > > > > > > G >>> > > > > > > >>> > > > > > > [1] >>> > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> https://github.com/apache/flink-kubernetes-operator/commit/e5a325c48965a50d61d0aa29e61ba79e97f27082#diff-a30b3ed9b8c53e998b15d7da7ad2e54374c98ffc3c920f76a70bce3fb37a9b2eR87-R93 >>> > > > > > > >>> > > > > > > >>> > > > > > > On Tue, Jun 3, 2025 at 8:53 AM Nicolas Fraison >>> > > > > > > <nicolas.frai...@datadoghq.com.invalid> wrote: >>> > > > > > > >>> > > > > > > > Hi, >>> > > > > > > > >>> > > > > > > > The FLIP has been updated. >>> > > > > > > > Let me know if you have some other comments. >>> > > > > > > > >>> > > > > > > > Nicolas >>> > > > > > > > >>> > > > > > > > On Tue, May 20, 2025 at 12:01 PM Nicolas Fraison < >>> > > > > > > > nicolas.frai...@datadoghq.com> wrote: >>> > > > > > > > >>> > > > > > > > > Hi Gabor, >>> > > > > > > > > >>> > > > > > > > > Thanks for the feedback. >>> > > > > > > > > The overall proposal with atomic dirty flag being set by >>> the >>> > > > > callback >>> > > > > > > > will >>> > > > > > > > > indeed work with any kind of implementation. >>> > > > > > > > > >>> > > > > > > > > Will see to update the FLIP in a week or 2 if there are >>> no >>> > > other >>> > > > > > > > comments >>> > > > > > > > > on it >>> > > > > > > > > >>> > > > > > > > > Nicolas >>> > > > > > > > > >>> > > > > > > > > >>> > > > > > > > > On Tue, May 20, 2025 at 11:08 AM Gabor Somogyi < >>> > > > > > > > gabor.g.somo...@gmail.com> >>> > > > > > > > > wrote: >>> > > > > > > > > >>> > > > > > > > >> Hi Nicolas, >>> > > > > > > > >> >>> > > > > > > > >> Related SSLContext I've not gone through all the cases >>> where >>> > > we >>> > > > > need >>> > > > > > > to >>> > > > > > > > >> reload so instead I'm sharing the concept. >>> > > > > > > > >> The main intention is that we must control all execution >>> > paths >>> > > > > which >>> > > > > > > > >> decide >>> > > > > > > > >> which certificate used for authentication. >>> > > > > > > > >> Creating an SSLContext decorator which checks reload >>> first >>> > and >>> > > > > then >>> > > > > > > > >> forwards all calls to the original (wrapped) context >>> > > > > > > > >> is one way to achieve that. If there are different >>> > > > implementations >>> > > > > > > which >>> > > > > > > > >> end up in similar behavior then it's fine. >>> > > > > > > > >> >>> > > > > > > > >> BR, >>> > > > > > > > >> G >>> > > > > > > > >> >>> > > > > > > > >> >>> > > > > > > > >> On Tue, May 20, 2025 at 10:15 AM Nicolas Fraison >>> > > > > > > > >> <nicolas.frai...@datadoghq.com.invalid> wrote: >>> > > > > > > > >> >>> > > > > > > > >> > Hi, >>> > > > > > > > >> > >>> > > > > > > > >> > Overall your proposal looks great. >>> > > > > > > > >> > The event handling must indeed be super fast and we >>> must >>> > > also >>> > > > > not >>> > > > > > > > change >>> > > > > > > > >> > original code path if reload not needed >>> > > > > > > > >> > >>> > > > > > > > >> > I have some concerns around the ReloadableSSLContext >>> > > > > implementing >>> > > > > > > all >>> > > > > > > > >> > SSLContext >>> > > > > > > > >> > Do you really mean SSLContext (java SSLContext one) >>> or do >>> > > you >>> > > > > > refer >>> > > > > > > to >>> > > > > > > > >> the >>> > > > > > > > >> > SslContext from netty? >>> > > > > > > > >> > >>> > > > > > > > >> > If java SSLContext >>> > > > > > > > >> > - I'm not sure how this will manage reload from the >>> > > BlobServer >>> > > > > > > > >> > BlobServer relies on creation of an >>> SSLServerSocketFactory >>> > > > from >>> > > > > > the >>> > > > > > > > >> > SSLContext. >>> > > > > > > > >> > But from my current understanding the >>> > SSLServerSocketFactory >>> > > > > does >>> > > > > > > not >>> > > > > > > > >> have >>> > > > > > > > >> > any connection with the SSLContext. >>> > > > > > > > >> > It only has some with an SSLContextImpl extends >>> > > SSLContextSpi. >>> > > > > > > > >> > I think we will need a callback here to enforce >>> recreation >>> > > of >>> > > > > the >>> > > > > > > > >> > BlobServer socket. >>> > > > > > > > >> > - I'm also don't see how to attach this to the >>> SslContext >>> > > from >>> > > > > > netty >>> > > > > > > > >> > >>> > > > > > > > >> > If netty SslContext >>> > > > > > > > >> > - we would still need to have the callback to >>> recreate the >>> > > > > > > BlobServer >>> > > > > > > > >> > socket >>> > > > > > > > >> > - for netty and pekko we should be able to rely on it >>> > > > > > > > >> > >>> > > > > > > > >> > Nicolas >>> > > > > > > > >> > >>> > > > > > > > >> > On Wed, May 7, 2025 at 12:40 PM Gabor Somogyi < >>> > > > > > > > >> gabor.g.somo...@gmail.com> >>> > > > > > > > >> > wrote: >>> > > > > > > > >> > >>> > > > > > > > >> > > Hi All, >>> > > > > > > > >> > > >>> > > > > > > > >> > > I've read through the concerns/proposals related the >>> > watch >>> > > > > > service >>> > > > > > > > and >>> > > > > > > > >> > here >>> > > > > > > > >> > > are my conclusions: >>> > > > > > > > >> > > * Watch service can watch local file systems only: >>> It's >>> > > fair >>> > > > > to >>> > > > > > > say >>> > > > > > > > >> that >>> > > > > > > > >> > > certificates must be copied to local FS in order to >>> work >>> > > > (init >>> > > > > > > > >> container >>> > > > > > > > >> > cp >>> > > > > > > > >> > > command or something) >>> > > > > > > > >> > > * Watch service can send multiple events even for a >>> > single >>> > > > > > > directory >>> > > > > > > > >> > > change: this can be mitigated with a single atomic >>> dirty >>> > > > flag >>> > > > > > (see >>> > > > > > > > my >>> > > > > > > > >> > > design suggestion) >>> > > > > > > > >> > > * Polling file modification time: When I hear any >>> kind >>> > of >>> > > > > > polling >>> > > > > > > > >> > > implemented by us is just something I'm mostly >>> opposing >>> > > > > > > > >> > > * security.ssl.internal.keystore.reload.duration: >>> > Security >>> > > > > > > features >>> > > > > > > > >> must >>> > > > > > > > >> > be >>> > > > > > > > >> > > executed nearly immediately no matter what >>> > > > > > > > >> > > >>> > > > > > > > >> > > As a general remark, not sure how many users are >>> using >>> > the >>> > > > > watch >>> > > > > > > > >> service >>> > > > > > > > >> > > based approach in the operator but until now I've >>> not >>> > seen >>> > > > any >>> > > > > > > issue >>> > > > > > > > >> > > related to that. >>> > > > > > > > >> > > If somebody is having some specifics then please >>> share. >>> > > > > > > > >> > > >>> > > > > > > > >> > > For now I would shoot for keystore not to have >>> feature >>> > > > creep. >>> > > > > > When >>> > > > > > > > >> there >>> > > > > > > > >> > is >>> > > > > > > > >> > > a valid use-case, community interest and the >>> keystore >>> > > story >>> > > > is >>> > > > > > > > already >>> > > > > > > > >> > rock >>> > > > > > > > >> > > stable >>> > > > > > > > >> > > then we can consider to involve truststore later. >>> > > > > > > > >> > > >>> > > > > > > > >> > > Having the mentioned assumption that the operator >>> > approach >>> > > > > > works, >>> > > > > > > > >> here is >>> > > > > > > > >> > > my high level proposal: >>> > > > > > > > >> > > * Let's have enable flag for all such watch >>> > functionality. >>> > > > If >>> > > > > > it's >>> > > > > > > > >> false >>> > > > > > > > >> > > then the currently existing functionality must >>> remain >>> > > as-is >>> > > > > > > > >> > > * Let's have a LocalFSWatchService which is a >>> singleton >>> > > > which >>> > > > > > has >>> > > > > > > no >>> > > > > > > > >> path >>> > > > > > > > >> > > registrations by default >>> > > > > > > > >> > > * Add path registration functionality which is >>> > > synchronised >>> > > > > > where >>> > > > > > > a >>> > > > > > > > >> > > callback can be registered >>> > > > > > > > >> > > * Let's have a ReloadableSSLContext which >>> implements the >>> > > > > > mentioned >>> > > > > > > > >> > callback >>> > > > > > > > >> > > * Inside the callback set an atomic dirty flag only >>> > (this >>> > > > can >>> > > > > > > handle >>> > > > > > > > >> > > multiple events for the same directory change + >>> event >>> > > > handling >>> > > > > > in >>> > > > > > > > the >>> > > > > > > > >> > watch >>> > > > > > > > >> > > service must be extreme fast) >>> > > > > > > > >> > > * Inside ReloadableSSLContext all SSLContext actions >>> > must >>> > > be >>> > > > > > > > >> overridden. >>> > > > > > > > >> > At >>> > > > > > > > >> > > the beginning of each function dirty flag must be >>> > checked >>> > > > > > > > >> > > and if dirty then certificates must be reloaded, >>> flag >>> > can >>> > > be >>> > > > > set >>> > > > > > > > back >>> > > > > > > > >> to >>> > > > > > > > >> > > false (then original functionality call). It's >>> extremely >>> > > > > > important >>> > > > > > > > >> that >>> > > > > > > > >> > > context reload must be synchronised. >>> > > > > > > > >> > > A synchronised boolean check + possible context >>> reload >>> > can >>> > > > > > consume >>> > > > > > > > >> some >>> > > > > > > > >> > > time but I wouldn't expect any significant >>> performance >>> > > drop. >>> > > > > > > > >> > > >>> > > > > > > > >> > > My proposal main drivers: >>> > > > > > > > >> > > * Original code path must run when no watch service >>> > asked >>> > > > > > > > >> > > * Super fast event handling because million events >>> may >>> > > come >>> > > > in >>> > > > > > > (not >>> > > > > > > > >> > > expecting but we should be prepared) >>> > > > > > > > >> > > * Clean separation between dir/file watch and >>> file/dir >>> > > usage >>> > > > > > > > >> > > * Well considered synchronisation model >>> > > > > > > > >> > > * Extensive unit testing because we're intended to >>> touch >>> > > the >>> > > > > > heart >>> > > > > > > > of >>> > > > > > > > >> > Flink >>> > > > > > > > >> > > >>> > > > > > > > >> > > Happy to hear other opinions. >>> > > > > > > > >> > > >>> > > > > > > > >> > > BR, >>> > > > > > > > >> > > G >>> > > > > > > > >> > > >>> > > > > > > > >> > > >>> > > > > > > > >> > > On Mon, Apr 28, 2025 at 1:54 PM Nicolas Fraison >>> > > > > > > > >> > > <nicolas.frai...@datadoghq.com.invalid> wrote: >>> > > > > > > > >> > > >>> > > > > > > > >> > > > Thanks all for your feedback and sorry for the >>> late >>> > > answer >>> > > > > (I >>> > > > > > > was >>> > > > > > > > on >>> > > > > > > > >> > > > holiday). >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > 1. Indeed it would add 4 threads. >>> > > > > > > > >> > > > For the non pekko component we can indeed have one >>> > > watcher >>> > > > > > > service >>> > > > > > > > >> used >>> > > > > > > > >> > > to >>> > > > > > > > >> > > > reload SSLContext for those components >>> > > > > > > > >> > > > For pekko this is a little more challenging as the >>> > > > creation >>> > > > > of >>> > > > > > > the >>> > > > > > > > >> > pekko >>> > > > > > > > >> > > > ssl engine is managed by pekko himself. >>> > > > > > > > >> > > > Flink only generates appropriate config with >>> class to >>> > > > > execute >>> > > > > > to >>> > > > > > > > >> > initiate >>> > > > > > > > >> > > > the pekko ssl engine [1]. This means that I will >>> not >>> > be >>> > > > able >>> > > > > > to >>> > > > > > > > >> provide >>> > > > > > > > >> > > the >>> > > > > > > > >> > > > watcher service to this ssl engine. >>> > > > > > > > >> > > > One solution would be to rely on a singleton >>> instead >>> > of >>> > > a >>> > > > > > > service >>> > > > > > > > >> > > injected >>> > > > > > > > >> > > > in each component but I'm not sure this is fine >>> to use >>> > > > such >>> > > > > in >>> > > > > > > > >> flink. >>> > > > > > > > >> > > > WDYT? >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > We can also add a specific flink configuration >>> > > > > > > > >> > > > (security.ssl.internal.keystore.reload.enable) to >>> only >>> > > add >>> > > > > > this >>> > > > > > > > >> watcher >>> > > > > > > > >> > > > mechanism if the config is enabled to avoid adding >>> > those >>> > > > > > threads >>> > > > > > > > if >>> > > > > > > > >> > this >>> > > > > > > > >> > > is >>> > > > > > > > >> > > > not needed. >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > 2. I'm fine with the LocalFSWatchService naming. >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > 3. Also agree that some e2e tests must be added. >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > @doguscan namal, Thanks for challenging the >>> proposal. >>> > > > > > > > >> > > > FYI, we are planning to rely on certificates with >>> > really >>> > > > > short >>> > > > > > > > >> > validity. >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > D1. It seems that the proposal to rely on a reload >>> > > period >>> > > > > will >>> > > > > > > > still >>> > > > > > > > >> > face >>> > > > > > > > >> > > > potential issues: >>> > > > > > > > >> > > > - receiving events while file content updates are >>> > still >>> > > in >>> > > > > > > > progress: >>> > > > > > > > >> > > > There is no guarantee that we will not load the >>> > > > certificate >>> > > > > > > while >>> > > > > > > > >> file >>> > > > > > > > >> > > > content updates are still in progress >>> > > > > > > > >> > > > - while it will not be affected by multiple >>> > > notifications, >>> > > > > we >>> > > > > > > can >>> > > > > > > > >> > reach a >>> > > > > > > > >> > > > point where only the truststore is updated when >>> the >>> > > reload >>> > > > > > > > happens. >>> > > > > > > > >> > > > Which means that if the keystore is updated just >>> > after, >>> > > it >>> > > > > > will >>> > > > > > > > not >>> > > > > > > > >> be >>> > > > > > > > >> > > > taken in account before next run of the reload >>> > mechanism >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > I think that with WatchService and appropriate >>> reload >>> > > > grace >>> > > > > > > period >>> > > > > > > > >> > > > mechanism we should be able to mitigate those 2 >>> issues >>> > > > > > (ensuring >>> > > > > > > > >> > minimum >>> > > > > > > > >> > > > reload even with multiple notify) >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > From KIP-1119 [2] it looks like the same kind of >>> > > > > requirements >>> > > > > > is >>> > > > > > > > >> under >>> > > > > > > > >> > > > discussion for Kafka to also rely on the >>> WatchService >>> > > Java >>> > > > > API >>> > > > > > > > >> > > (SpringBoot >>> > > > > > > > >> > > > seems to also rely on this API to manage ssl >>> reload >>> > > [3]). >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > D3. Do we have a real use case for this? >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > [1] >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > >>> > > > > > > > >> > > >>> > > > > > > > >> > >>> > > > > > > > >> >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> https://github.com/apache/flink/blob/b523264ab45d37cd9584a0e8c06f1ef6bd1aaed7/flink-rpc/flink-rpc-akka/src/main/java/org/apache/flink/runtime/rpc/pekko/PekkoUtils.java#L372 >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > [2] >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > >>> > > > > > > > >> > > >>> > > > > > > > >> > >>> > > > > > > > >> >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1119%3A+Add+support+for+SSL+auto+reload >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > [3] >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > >>> > > > > > > > >> > > >>> > > > > > > > >> > >>> > > > > > > > >> >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> https://docs.spring.io/spring-boot/reference/features/ssl.html#features.ssl.reloading >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > Regards, >>> > > > > > > > >> > > > Nicolas >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > On Wed, Apr 23, 2025 at 12:14 PM Doğuşcan Namal < >>> > > > > > > > >> > > namal.dogus...@gmail.com> >>> > > > > > > > >> > > > wrote: >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > > Hi Nicolas, thanks for the FLIP. >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > I am fully supportive of the motivation and we >>> > should >>> > > be >>> > > > > > > > >> supporting >>> > > > > > > > >> > > this >>> > > > > > > > >> > > > > feature. Here are couple of comments from my >>> side: >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > D1) >>> > > > > > > > >> > > > > Since you shared the implementation details on >>> the >>> > > FLIP >>> > > > as >>> > > > > > > > well, I >>> > > > > > > > >> > > would >>> > > > > > > > >> > > > > like to discuss whether using Java's >>> WatchService is >>> > > the >>> > > > > > best >>> > > > > > > > >> choice >>> > > > > > > > >> > > > here. >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > I believe that the certificate renewal is not a >>> > > frequent >>> > > > > > > > >> operation. >>> > > > > > > > >> > > Even >>> > > > > > > > >> > > > > once a day certificate renewals are not >>> > > realistic(except >>> > > > > for >>> > > > > > > > test >>> > > > > > > > >> > cases >>> > > > > > > > >> > > > > maybe) but let's assume that this covers up the >>> > p99.99 >>> > > > of >>> > > > > > the >>> > > > > > > > use >>> > > > > > > > >> > > cases. >>> > > > > > > > >> > > > I >>> > > > > > > > >> > > > > am confident on this estimation since there >>> hasn't >>> > > been >>> > > > a >>> > > > > > > > request >>> > > > > > > > >> > from >>> > > > > > > > >> > > > the >>> > > > > > > > >> > > > > community for this feature so far, which >>> confirms >>> > that >>> > > > > > people >>> > > > > > > > were >>> > > > > > > > >> > okay >>> > > > > > > > >> > > > > with infrequent cluster restarts. Following >>> that it >>> > is >>> > > > > > > > >> infrequent, I >>> > > > > > > > >> > > > > believe that spawning up a thread that watches >>> the >>> > > file >>> > > > > > > > >> modification >>> > > > > > > > >> > > > > operations is not the best use of the limited >>> > > resources >>> > > > > on a >>> > > > > > > > >> cluster. >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > There are some known limitations of the >>> WatchService >>> > > as >>> > > > > well >>> > > > > > > > such >>> > > > > > > > >> as >>> > > > > > > > >> > > > > receiving multiple modification events for the >>> same >>> > > > > > occurence >>> > > > > > > > [1], >>> > > > > > > > >> > > > > inotify's (WatchService's underlying mechanism >>> in >>> > > Linux >>> > > > > > > > >> environments) >>> > > > > > > > >> > > > > problems on containerized environments due to >>> remote >>> > > > file >>> > > > > > > > systems >>> > > > > > > > >> [2] >>> > > > > > > > >> > > or >>> > > > > > > > >> > > > > receiving events while file content updates are >>> > still >>> > > in >>> > > > > > > > progress. >>> > > > > > > > >> > > [3]. I >>> > > > > > > > >> > > > > do not know if these limitations are addressed >>> in >>> > the >>> > > > > newer >>> > > > > > > > >> versions >>> > > > > > > > >> > > but >>> > > > > > > > >> > > > > regardless of that it is clear that we may face >>> with >>> > > > some >>> > > > > > ugly >>> > > > > > > > >> edge >>> > > > > > > > >> > > cases >>> > > > > > > > >> > > > > due to that. >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > Given these complications, I would recommend >>> just >>> > > > > creating a >>> > > > > > > new >>> > > > > > > > >> > > > SSLContext >>> > > > > > > > >> > > > > after a configured duration is expired. We could >>> > > record >>> > > > > the >>> > > > > > > > >> timestamp >>> > > > > > > > >> > > > when >>> > > > > > > > >> > > > > the previous SSLContext is created and update it >>> > > after a >>> > > > > > > > >> configured >>> > > > > > > > >> > > > > duration is passed. This will be much easier to >>> test >>> > > and >>> > > > > > > reason >>> > > > > > > > >> about >>> > > > > > > > >> > > > when >>> > > > > > > > >> > > > > it is running on production. This will >>> eliminate the >>> > > > > > necessity >>> > > > > > > > to >>> > > > > > > > >> > > reason >>> > > > > > > > >> > > > > about the file modification operations as well. >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > I briefly skimmed through the classes that need >>> to >>> > be >>> > > > > > modified >>> > > > > > > > >> and it >>> > > > > > > > >> > > > > looked feasible for me. Let me know what are >>> your >>> > > > comments >>> > > > > > on >>> > > > > > > > >> these. >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > Note that this is already used in the Kafka >>> world >>> > > where >>> > > > a >>> > > > > > new >>> > > > > > > > >> > > SSLContext >>> > > > > > > > >> > > > is >>> > > > > > > > >> > > > > created after 12 hours. >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > D2) We could provide a configuration to the >>> user, >>> > such >>> > > > as >>> > > > > > > > >> > > > > >>> "security.ssl.internal.keystore.reload.duration" so >>> > > they >>> > > > > > could >>> > > > > > > > >> decide >>> > > > > > > > >> > > how >>> > > > > > > > >> > > > > often the new certificates should be loaded. >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > D3) >>> > > > > > > > >> > > > > On the other hand, I wonder whether we should >>> also >>> > > > handle >>> > > > > > > > >> supporting >>> > > > > > > > >> > > > > updating the file paths of the truststores and >>> > > keystores >>> > > > > > under >>> > > > > > > > >> this >>> > > > > > > > >> > > FLIP >>> > > > > > > > >> > > > as >>> > > > > > > > >> > > > > well. Since the name of the FLIP is "Handle TLS >>> > > > > Certificate >>> > > > > > > > >> Renewal" >>> > > > > > > > >> > I >>> > > > > > > > >> > > > > think we could bring that into scope too :) >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > Thanks >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > [1] >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > >>> > > > > > > > >> > > >>> > > > > > > > >> > >>> > > > > > > > >> >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> https://stackoverflow.com/questions/16777869/java-7-watchservice-ignoring-multiple-occurrences-of-the-same-event >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > [2] >>> > > > > > > > >>> https://blog.arkey.fr/2019/09/13/watchservice-and-bind-mount/ >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > [3] >>> > > > > > > > >> > > > >>> > > > > > > > >> >>> > > > > > >>> > > >>> https://surajatreyac.github.io/2014-07-29/reactive_file_handling.html >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > [4] See also Platform Dependencies - >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > >>> > > > > > > > >> > > >>> > > > > > > > >> > >>> > > > > > > > >> >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> https://docs.oracle.com/javase/8/docs/api/index.html?java/nio/file/WatchService.html >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > On Fri, 18 Apr 2025 at 18:25, Gabor Somogyi < >>> > > > > > > > >> > gabor.g.somo...@gmail.com >>> > > > > > > > >> > > > >>> > > > > > > > >> > > > > wrote: >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > > > 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 >>> > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > >>> > > > > > > > >> > > > > > >>> > > > > > > > >> > > > > >>> > > > > > > > >> > > > >>> > > > > > > > >> > > >>> > > > > > > > >> > >>> > > > > > > > >> >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> >>
