The vote has passed with 4 binding votes (Gwen, Jun, Jason, me) and 2 non-binding votes (Ted You, Tom Bentley).
Many thanks for the reviews and votes. I will update the KIP page. Regards, Rajini On Tue, Jan 9, 2018 at 11:00 AM, Rajini Sivaram <rajinisiva...@gmail.com> wrote: > > Thank you, Jun! I have updated the KIP. > > If there are no other comments or concerns, I will close the vote later > today. > > Thanks, > > Rajini > > On Mon, Jan 8, 2018 at 10:57 PM, Jun Rao <j...@confluent.io> wrote: > >> Hi, Rajini, >> >> Thanks for the explanation. Then your suggestion sounds good to me. >> >> Jun >> >> On Mon, Jan 8, 2018 at 1:32 PM, Rajini Sivaram <rajinisiva...@gmail.com> >> wrote: >> >> > Hi Jun, >> > >> > No, password.encoder.secret cannot be updated dynamically at the >> moment. >> > Dynamic configs are stored in ZooKeeper and since ZK is not secure, all >> > password configs in ZK are encrypted using password.encoder.secret. We >> > cannot make password.encoder.secret dynamic since it would need another >> > secret to encrypt it for storing in ZK and that secret would need to be >> > static and cannot be rotated. >> > >> > On Mon, Jan 8, 2018 at 6:33 PM, Jun Rao <j...@confluent.io> wrote: >> > >> > > Hi, Rajini, >> > > >> > > Could password.encoder.secret be updated dynamically? If so, each >> broker >> > > will still have access to the old secret when password.encoder.secret >> is >> > > updated. Perhaps that's a simpler way to handle changing secret than >> > > introducing an extra config. >> > > >> > > Thanks, >> > > >> > > Jun >> > > >> > > On Fri, Jan 5, 2018 at 3:09 AM, Rajini Sivaram < >> rajinisiva...@gmail.com> >> > > wrote: >> > > >> > > > Hi Jun, >> > > > >> > > > We are using 2-way encryption. The password configs encoded are >> > > > keystore/truststore passwords and JAAS configuration. We need to be >> > able >> > > to >> > > > extract the actual values for these, so we cannot use 1-way hash. >> So if >> > > we >> > > > have the old secret, we can decrypt and get the original values. >> > > > >> > > > Thank you, >> > > > >> > > > Rajini >> > > > >> > > > On Fri, Jan 5, 2018 at 12:11 AM, Jun Rao <j...@confluent.io> wrote: >> > > > >> > > > > Hi, Rajin, >> > > > > >> > > > > Does providing the old-secret help? My understanding is that the >> > > encoded >> > > > > passwd is the result of a 1-way hash with the secret. So, one >> can't >> > > > decode >> > > > > the passwd with old-secret. If that's the case, one still needs to >> > > > provide >> > > > > the unencrypted paaswd to re-encode with the new secret? >> > > > > >> > > > > Thanks, >> > > > > >> > > > > Jun >> > > > > >> > > > > On Thu, Jan 4, 2018 at 1:28 AM, Rajini Sivaram < >> > > rajinisiva...@gmail.com> >> > > > > wrote: >> > > > > >> > > > > > Hi Jun/Jason, >> > > > > > >> > > > > > I was wondering whether it is worth adding a new property >> (static >> > > > config >> > > > > in >> > > > > > server.properties) to pass in the previous encoder password as >> well >> > > > when >> > > > > > changing encoder password. So you would set: >> > > > > > >> > > > > > - password.encoder.secret=new-password >> > > > > > - password.encoder.old.secret=old-password >> > > > > > >> > > > > > When the broker starts up and loads passwords from ZK, it would >> > check >> > > > if >> > > > > > old-password is being used. If so, it would re-encode all >> passwords >> > > in >> > > > ZK >> > > > > > using new-password and store them back in ZK. If the >> new-password >> > is >> > > > > > already in use in ZK, the old one will be ignored. This needs an >> > > extra >> > > > > > property, but makes it simpler for the user since all other >> > passwords >> > > > can >> > > > > > be used from ZK. >> > > > > > >> > > > > > What do you think? >> > > > > > >> > > > > > >> > > > > > >> > > > > > On Wed, Jan 3, 2018 at 6:01 PM, Rajini Sivaram < >> > > > rajinisiva...@gmail.com> >> > > > > > wrote: >> > > > > > >> > > > > > > Hi Jason, >> > > > > > > >> > > > > > > Thank you for reviewing and voting. >> > > > > > > >> > > > > > > Thanks, I had missed the rename. Have updated the KIP. >> > > > > > > >> > > > > > > The configs can be defined in the static server.properties or >> in >> > > > > > > ZooKeeper. If a ZK config cannot be decoded (or is not >> valid), we >> > > log >> > > > > an >> > > > > > > error and revert to the static config or default. When >> updating >> > the >> > > > > > secret >> > > > > > > used by the encode, we expect all password values to be >> specified >> > > in >> > > > > > > server.properties. And the decoding or sanity check of the >> > password >> > > > in >> > > > > ZK >> > > > > > > would fail with the new secret, so we would use the password >> > values >> > > > > from >> > > > > > > server.properties. Once the broker starts up, the values can >> be >> > > reset >> > > > > in >> > > > > > ZK >> > > > > > > using AdminClient and they will be encoded using the new >> secret. >> > > > > > > >> > > > > > > >> > > > > > > On Wed, Jan 3, 2018 at 5:34 PM, Jason Gustafson < >> > > ja...@confluent.io> >> > > > > > > wrote: >> > > > > > > >> > > > > > >> +1 Thanks for the KIP. One minor nit: I think we changed >> > > > > > >> ConfigSource.TOPIC_CONFIG to ConfigSource.DYNAMIC_TOPIC_CON >> FIG >> > in >> > > > the >> > > > > > PR. >> > > > > > >> >> > > > > > >> As far as updating secrets, I wasn't sure I understand how >> that >> > > will >> > > > > > work. >> > > > > > >> Do the password configs accept multiple values? >> > > > > > >> >> > > > > > >> On Wed, Jan 3, 2018 at 2:58 AM, Rajini Sivaram < >> > > > > rajinisiva...@gmail.com >> > > > > > > >> > > > > > >> wrote: >> > > > > > >> >> > > > > > >> > Hi Jun, >> > > > > > >> > >> > > > > > >> > Thank you for reviewing and voting. >> > > > > > >> > >> > > > > > >> > 50. I have updated the KIP to describe how the secret may >> be >> > > > > changed. >> > > > > > >> All >> > > > > > >> > dynamically configurable passwords and per-broker configs. >> So >> > > the >> > > > > > secret >> > > > > > >> > can be different across brokers and updated using rolling >> > > restart. >> > > > > In >> > > > > > >> order >> > > > > > >> > to update the secret, each broker needs to be restarted >> with >> > an >> > > > > > updated >> > > > > > >> > server.properties which contains the new secret as well as >> the >> > > > > current >> > > > > > >> > values of all the password configs. Admin client can then >> be >> > > used >> > > > to >> > > > > > >> update >> > > > > > >> > the passwords in ZooKeeper that are encrypted using the new >> > > > secret. >> > > > > > >> > >> > > > > > >> > 51. leader.replication.throttled.replicas and >> > > > > > >> > follower.replication.throttled.replicas >> > > > > > >> > are dynamically configurable at the topic level. But there >> are >> > > no >> > > > > > >> defaults >> > > > > > >> > for these at the broker level since they refer to >> partitions >> > of >> > > > the >> > > > > > >> topic. >> > > > > > >> > The rates used for throttling were already configurable at >> the >> > > > > broker >> > > > > > >> > level. >> > > > > > >> > >> > > > > > >> > I made a couple of other changes to the KIP: >> > > > > > >> > >> > > > > > >> > 1. The config names used for encoding passwords are now >> > prefixed >> > > > > with >> > > > > > >> > password.encoder. >> > > > > > >> > Also added key length as a config since this is >> constrained by >> > > the >> > > > > > >> > algorithm which is also configurable. >> > > > > > >> > 2. I moved the update of inter-broker security protocol and >> > > > > > >> > inter-broker sasl mechanism to the follow-on KIP under >> Future >> > > > Work. >> > > > > As >> > > > > > >> part >> > > > > > >> > of the new KIP, we need to add protocol changes to validate >> > that >> > > > all >> > > > > > >> > brokers in the cluster support the new >> > > protocol/mechanism/version >> > > > to >> > > > > > >> avoid >> > > > > > >> > accidental changes before all brokers are updated. >> > > > > > >> > >> > > > > > >> > >> > > > > > >> > On Tue, Jan 2, 2018 at 10:58 PM, Jun Rao <j...@confluent.io >> > >> > > > wrote: >> > > > > > >> > >> > > > > > >> > > Hi, Rajini, >> > > > > > >> > > >> > > > > > >> > > Thank for the KIP. +1. Just a couple of minor comments >> > below. >> > > > > > >> > > >> > > > > > >> > > >> > > > > > >> > > 50. config.secret.*: Could you document how the >> > > > > > encryption/decryption >> > > > > > >> of >> > > > > > >> > > passwd work? In particular, how do we support changing >> > > > > > config.secret? >> > > > > > >> > > >> > > > > > >> > > 51. At the topic level, we also have >> > > > leader.replication.throttled. >> > > > > > >> > replicas >> > > > > > >> > > and follower.replication.throttled.replicas. Should >> they be >> > > > > > >> dynamically >> > > > > > >> > > configurable? >> > > > > > >> > > >> > > > > > >> > > Jun >> > > > > > >> > > >> > > > > > >> > > >> > > > > > >> > > >> > > > > > >> > > >> > > > > > >> > > >> > > > > > >> > > >> > > > > > >> > > On Tue, Dec 12, 2017 at 9:24 AM, Gwen Shapira < >> > > > g...@confluent.io> >> > > > > > >> wrote: >> > > > > > >> > > >> > > > > > >> > > > +1 (binding). Thank you for leading this, Rajini. >> > > > > > >> > > > >> > > > > > >> > > > On Tue, Dec 12, 2017 at 8:35 AM Tom Bentley < >> > > > > > t.j.bent...@gmail.com> >> > > > > > >> > > wrote: >> > > > > > >> > > > >> > > > > > >> > > > > +1 (nonbinding) >> > > > > > >> > > > > >> > > > > > >> > > > > On 12 December 2017 at 15:34, Ted Yu < >> > yuzhih...@gmail.com >> > > > >> > > > > > wrote: >> > > > > > >> > > > > >> > > > > > >> > > > > > +1 >> > > > > > >> > > > > > >> > > > > > >> > > > > > On Tue, Dec 12, 2017 at 5:44 AM, Rajini Sivaram < >> > > > > > >> > > > rajinisiva...@gmail.com >> > > > > > >> > > > > > >> > > > > > >> > > > > > wrote: >> > > > > > >> > > > > > >> > > > > > >> > > > > > > Since there are no more outstanding comments, I >> > would >> > > > like >> > > > > > to >> > > > > > >> > start >> > > > > > >> > > > > vote >> > > > > > >> > > > > > > for KIP-226: >> > > > > > >> > > > > > > >> > > > > > >> > > > > > > https://cwiki.apache.org/ >> > > confluence/display/KAFKA/KIP- >> > > > > > >> > > > > > > 226+-+Dynamic+Broker+Configuration >> > > > > > >> > > > > > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > > The KIP enables dynamic update of commonly >> updated >> > > > broker >> > > > > > >> > > > configuration >> > > > > > >> > > > > > > options to avoid expensive restarts. >> > > > > > >> > > > > > > >> > > > > > >> > > > > > > Thank you, >> > > > > > >> > > > > > > >> > > > > > >> > > > > > > Rajini >> > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > >> > > > > > >> > > > >> > > > > > >> > > >> > > > > > >> > >> > > > > > >> >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> > >
