Hey David, thanks for the feedback. On Thu, Jan 7, 2021 at 2:37 AM David Jacot <dja...@confluent.io> wrote:
> 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'm not sure how we achieve that today, if we are updating both key store and trust store in the same request, it is still possible that one update succeeded but another failed. Does users have awareness to make the guarantee by themselves? My point is that today there is no reliable way to achieve atomic update either, and hoping users to make the correct sequence of decisions would be hard. 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 > > > >>>> > > > >>> > > > > > >
