Hi Boyang, Thanks for the KIP. I am fine with it in general. I just have a few comments.
With the proposal, we don't have the guarantee that both the new keystore and the new truststore will be picked up together so we may end up with the new keystore and the old truststore for a short period of time, or permanently if the second one can't be reloaded for any reason. This could disallow clients to authenticate for a while if the new keystore and the new trustore are not crafted to work with their old versions. I wonder how this would work in practice. Do we already have guards in place to avoid this or could we add something to ensure that listeners are updated only if both the truststore and the keystore works with each other? We don't have this issue today as both the truststore and the keystore are reloaded when the AlterConfig RPC is received so the admin can control this process. It is all or nothing. I think that this is acceptable but it is worth clearly mentioning that there is no guarantee from that regard in the KIP, and later in the doc. Perhaps, we could also mention that updating them in place is not a best practice and that using new paths gives better control to the admin. Best, David On Wed, Jan 6, 2021 at 6:55 PM Jason Gustafson <ja...@confluent.io> wrote: > Thanks Boyang. Someone mentioned my email never showed up, but basically I > suggested tying the refresh configuration more directly to the > configurations it would affect. I'm happy with the updates. > > -Jason > > On Tue, Jan 5, 2021 at 8:34 PM Boyang Chen <reluctanthero...@gmail.com> > wrote: > > > Thanks Jason for the feedback. I separated the time configs for key store > > and trust store, and rename the configs as you proposed. > > > > Best, > > Boyang > > > > On Mon, Dec 14, 2020 at 3:47 PM Boyang Chen <reluctanthero...@gmail.com> > > wrote: > > > > > Hey there, > > > > > > bumping up this thread to see if there are further questions regarding > > the > > > updated proposal. > > > > > > Best, > > > Boyang > > > > > > On Thu, Dec 10, 2020 at 11:52 AM Boyang Chen < > reluctanthero...@gmail.com > > > > > > wrote: > > > > > >> After some offline discussions, we believe that it's the right > direction > > >> to go by doing a hybrid approach which includes both file-watch > trigger > > and > > >> interval based reloading. The former guarantees a swift change in 99% > > time, > > >> while the latter provides a time-based guarantee in the worst case > when > > the > > >> file-watch does not take effect. The current default reloading > interval > > is > > >> set to 5 min. I have updated the KIP and ticket, feel free to check > out > > and > > >> see if it makes sense. > > >> > > >> Best, > > >> Boyang > > >> > > >> On Tue, Dec 8, 2020 at 8:58 PM Boyang Chen < > reluctanthero...@gmail.com> > > >> wrote: > > >> > > >>> Hey Gwen, thanks for the feedback. > > >>> > > >>> On Sun, Dec 6, 2020 at 10:06 PM Gwen Shapira <g...@confluent.io> > > wrote: > > >>> > > >>>> Agree with Igor. IIRC, we also encountered cases where filewatch was > > >>>> not triggered as expected. An interval will give us a better > > >>>> worse-case scenario that is easily controlled by the Kafka admin. > > >>>> > > >>>> Are the cases you were referring to happening in the cloud > > environment? > > >>> Should we investigate instead of simply assuming the standard API > won't > > >>> work? I checked around and found a similar complaint here > > >>> <https://blog.arkey.fr/2019/09/13/watchservice-and-bind-mount/>. > > >>> > > >>> I would be partially agreeing that we want to have a reliable > approach > > >>> for all different operating systems in general, but would be great if > > we > > >>> could reach a quantitative measure of file-watch success rate if > > possible > > >>> for us to make the call. Eventually, the benefit of file-watch is > more > > >>> prompt reaction time and less configuration to the broker. > > >>> > > >>>> Gwen > > >>>> > > >>>> On Sun, Dec 6, 2020 at 8:17 AM Igor Soarez <i...@soarez.me> wrote: > > >>>> > > > >>>> > > > >>>> > > > The proposed change relies on a file watch, why not also have > a > > >>>> polling > > >>>> > > > interval to check the file for changes? > > >>>> > > > > > >>>> > > > The periodical check could work, the slight downside is that > we > > >>>> need > > >>>> > > additional configurations to schedule the interval. Do you think > > the > > >>>> > > file-watch approach has any extra overhead than the interval > based > > >>>> solution? > > >>>> > > > >>>> > I don't think so. The reason I'm asking this is the KIP currently > > >>>> includes: > > >>>> > > > >>>> > "When the file watch does not work for unknown reason, user > could > > >>>> still try to change the store path in an explicit AlterConfig call > in > > the > > >>>> worst case." > > >>>> > > > >>>> > Having the interval in addition to the file watch could result in > a > > >>>> better worst case scenario. > > >>>> > I understand it would require introducing at least one new > > >>>> configuration for the interval, so maybe this doesn't have to solved > > in > > >>>> this KIP. > > >>>> > > > >>>> > -- > > >>>> > Igor > > >>>> > > > >>>> > On Fri, Dec 4, 2020, at 5:14 PM, Boyang Chen wrote: > > >>>> > > Hey Igor, thanks for the feedback. > > >>>> > > > > >>>> > > On Fri, Dec 4, 2020 at 5:24 AM Igor Soarez <i...@soarez.me> wrote: > > >>>> > > > > >>>> > > > Hi Boyang, > > >>>> > > > > > >>>> > > > > >>>> > > > > >>>> > > > What happens if the file is changed into an invalid store? > Does > > >>>> the > > >>>> > > > previous store stay in use? > > >>>> > > > > > >>>> > > > If the reload fails, the previous store should be effective. I > > >>>> will state > > >>>> > > that in the KIP. > > >>>> > > > > >>>> > > > > >>>> > > > Thanks, > > >>>> > > > > > >>>> > > > -- > > >>>> > > > Igor > > >>>> > > > > > >>>> > > > On Fri, Dec 4, 2020, at 1:28 AM, Boyang Chen wrote: > > >>>> > > > > Hey there, > > >>>> > > > > > > >>>> > > > > I would like to start the discussion thread for KIP-687: > > >>>> > > > > > > >>>> > > > > > >>>> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-687%3A+Automatic+Reloading+of+Security+Store > > >>>> > > > > > > >>>> > > > > This KIP is trying to deprecate the AlterConfigs API support > > of > > >>>> updating > > >>>> > > > > the security store by reloading path in-place, and replace > > with > > >>>> a > > >>>> > > > > file-watch mechanism inside the broker. Let me know what you > > >>>> think. > > >>>> > > > > > > >>>> > > > > Best, > > >>>> > > > > Boyang > > >>>> > > > > > > >>>> > > > > > >>>> > > > > >>>> > > >>>> > > >>>> > > >>>> -- > > >>>> Gwen Shapira > > >>>> Engineering Manager | Confluent > > >>>> 650.450.2760 | @gwenshap > > >>>> Follow us: Twitter | blog > > >>>> > > >>> > > >
