Rajini, Thanks for the updated wiki. It looks good to me. Do you want to start a vote on this?
Jun On Fri, May 27, 2016 at 11:47 AM, Rajini Sivaram < rajinisiva...@googlemail.com> wrote: > Jun, > > Thank you for the review and the suggestions. I have updated the KIP with > the changes you suggested. > > > On Fri, May 27, 2016 at 5:24 PM, Jun Rao <j...@confluent.io> wrote: > > > Rajini, > > > > Thanks for the updated KIP. Looks good overall. Just a few minor > comments. > > > > 10. For quota related metric names, currently, they already have a tag > > "client-d". It seems that we can just replace it with a similar tag > "user" > > if quota.type is user. The sensor names only have the client-id value. We > > can generalize it to something like quota.type-type.value. Note that only > > the metric names affect the jmx names, not the sensors. > > > > 11. For the value that we store in ZK for a user level quota, would it be > > better to include the user name? Since the path is base64, this could > make > > it easier to see what the actual user is associated with the path. > > > > 12. For values for quota.type, perhaps we can use "client-id" and "user"? > > > > Jun > > > > > > > > On Wed, May 25, 2016 at 12:29 PM, Rajini Sivaram < > > rajinisiva...@googlemail.com> wrote: > > > > > Hi Aditya, > > > > > > Thank you for the review. When *quota.type=user*, quotas are based on > the > > > user principal which may be an authenticated or unauthenticated user. > For > > > PLAINTEXT, the principal would be "*anonymous*" by default, but can be > > > overridden by supplying a principal builder. Quotas can be applied to " > > > *anonymous*". For example, in a cluster that has both PLAINTEXT and > SSL, > > > you can limit the quota for PLAINTEXT by specifying a quota for > > > "*anonymous*". > > > With PLAINTEXT, you can limit the quota for traffic from an IP address > by > > > setting principal using a custom principal builder and specifying quota > > for > > > the principal. > > > > > > I used "*clients*" for *quota.type* since the term was already used as > > > ConfigType to set quotas for client-ids in kaka-configs.sh and in the > > > Zookeeper path. But I understand that clients/users can be confusing. > How > > > about client-id and 'user-principal'? > > > > > > Regards, > > > > > > Rajini > > > > > > On Wed, May 25, 2016 at 6:16 PM, Aditya Auradkar < > > > aaurad...@linkedin.com.invalid> wrote: > > > > > > > Hey Rajini - > > > > > > > > If the quota.type is set to 'user', what happens to unauthenticated > > > > clients? They don't supply a principal, so are they essentially > > > > unthrottled? > > > > > > > > This may be a nit, but I prefer 'quota.type' options to be > > > > 'authenticated-user' and 'client-id' as opposed to 'client' and > 'user'. > > > For > > > > a new user, the options 'client' and 'user' sound essentially the > same. > > > > > > > > Aditya > > > > > > > > On Tue, May 24, 2016 at 5:55 AM, Rajini Sivaram < > > > > rajinisiva...@googlemail.com> wrote: > > > > > > > > > Jun, > > > > > > > > > > I have updated the KIP based on your suggestion. Can you take a > look? > > > > > > > > > > Thank you, > > > > > > > > > > Rajini > > > > > > > > > > On Tue, May 24, 2016 at 11:20 AM, Rajini Sivaram < > > > > > rajinisiva...@googlemail.com> wrote: > > > > > > > > > > > Jun, > > > > > > > > > > > > Thank you for the review. I agree that a simple user principal > > based > > > > > quota > > > > > > is sufficient to allocate broker resources fairly in a multi-user > > > > system. > > > > > > Hierarchical quotas proposed in the KIP currently enables clients > > of > > > a > > > > > user > > > > > > to be rate-limited as well. This allows a user to run multiple > > > clients > > > > > > which don't interfere with each other's quotas. Since there is no > > > clear > > > > > > requirement to support this at the moment, I am happy to limit > the > > > > scope > > > > > of > > > > > > the KIP to a single-level user-based quota. Will update the KIP > > > today. > > > > > > > > > > > > Regards, > > > > > > > > > > > > Rajini > > > > > > > > > > > > On Mon, May 23, 2016 at 5:24 PM, Jun Rao <j...@confluent.io> > wrote: > > > > > > > > > > > >> Rajini, > > > > > >> > > > > > >> Thanks for the KIP. When we first added the quota support, the > > > > intention > > > > > >> was to be able to add a quota per application. Since at that > time, > > > we > > > > > >> don't > > > > > >> have security yet. We essentially simulated users with > client-ids. > > > Now > > > > > >> that > > > > > >> we do have security. It seems that we just need to have a way to > > set > > > > > quota > > > > > >> at the user level. Setting quota at the combination of users and > > > > > >> client-ids > > > > > >> seems more complicated and I am not sure if there is a good use > > > case. > > > > > >> > > > > > >> Also, the new config quota.secure.enable seems a bit weird. > Would > > it > > > > be > > > > > >> better to add a new config quota.type. It defaults to clientId > for > > > > > >> backward > > > > > >> compatibility. If one sets it to user, then the default broker > > level > > > > > quota > > > > > >> is for users w/o a customized quota. In this setting, brokers > will > > > > also > > > > > >> only take quota set at the user level (i.e., quota set at > clientId > > > > level > > > > > >> will be ignored). > > > > > >> > > > > > >> Thanks, > > > > > >> > > > > > >> Jun > > > > > >> > > > > > >> On Tue, May 3, 2016 at 4:32 AM, Rajini Sivaram < > > > > > >> rajinisiva...@googlemail.com > > > > > >> > wrote: > > > > > >> > > > > > >> > Ewen, > > > > > >> > > > > > > >> > Thank you for the review. I agree that ideally we would have > one > > > > > >> definition > > > > > >> > of quotas that handles all cases. But I couldn't quite fit all > > the > > > > > >> > combinations that are possible today with client-id-based > quotas > > > > into > > > > > >> the > > > > > >> > new configuration. I think upgrade path is not bad since > quotas > > > are > > > > > >> > per-broker. You can configure quotas based on the new > > > configuration, > > > > > set > > > > > >> > quota.secure.enable=true and restart the broker. Since there > is > > no > > > > > >> > requirement for both insecure client-id based quotas and > secure > > > > > >> user-based > > > > > >> > quotas to co-exist in a cluster, isn't that sufficient? The > > > > > >> implementation > > > > > >> > does use a unified approach, so if an alternative > configuration > > > can > > > > be > > > > > >> > defined (perhaps with some acceptable limitations?) which can > > > > express > > > > > >> both, > > > > > >> > it will be easy to implement. Suggestions welcome :-) > > > > > >> > > > > > > >> > The cases that the new configuration cannot express, but the > old > > > one > > > > > can > > > > > >> > are: > > > > > >> > > > > > > >> > 1. SSL/SASL with multiple users, same client ids used by > > > multiple > > > > > >> users, > > > > > >> > client-id based quotas where quotas are shared between > > multiple > > > > > users > > > > > >> > 2. Default quotas for client-ids. In the new configuration, > > > > default > > > > > >> > quotas are defined for users and clients with no configured > > > > > sub-quota > > > > > >> > share > > > > > >> > the user's quota. > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > On Sat, Apr 30, 2016 at 6:21 AM, Ewen Cheslack-Postava < > > > > > >> e...@confluent.io> > > > > > >> > wrote: > > > > > >> > > > > > > >> > > Rajini, > > > > > >> > > > > > > > >> > > I'm admittedly not very familiar with a lot of this code or > > > > > >> > implementation, > > > > > >> > > so correct me if I'm making any incorrect assumptions. > > > > > >> > > > > > > > >> > > I've only scanned the KIP, but my main concern is the > > rejection > > > of > > > > > the > > > > > >> > > alternative -- unifying client-id and principal quotas. In > > > > > particular, > > > > > >> > > doesn't this make an upgrade for brokers using those > different > > > > > >> approaches > > > > > >> > > difficult since you have to make a hard break between > > client-id > > > > and > > > > > >> > > principal quotas? If people adopt client-id quotas to begin > > > with, > > > > it > > > > > >> > seems > > > > > >> > > like we might not be providing a clean upgrade path. > > > > > >> > > > > > > > >> > > As I said, I haven't kept up to date with the details of the > > > > > security > > > > > >> and > > > > > >> > > quota features, but I'd want to make sure we didn't suggest > > one > > > > path > > > > > >> with > > > > > >> > > 0.9, then add another that we can't provide a clean upgrade > > path > > > > to. > > > > > >> > > > > > > > >> > > -Ewen > > > > > >> > > > > > > > >> > > On Fri, Apr 22, 2016 at 7:22 AM, Rajini Sivaram < > > > > > >> > > rajinisiva...@googlemail.com> wrote: > > > > > >> > > > > > > > >> > > > The PR for KAFKA-3492 ( > > > > https://github.com/apache/kafka/pull/1256) > > > > > >> > > contains > > > > > >> > > > the code associated with KIP-55. I will keep it updated > > during > > > > the > > > > > >> > review > > > > > >> > > > process. > > > > > >> > > > > > > > > >> > > > Thanks, > > > > > >> > > > > > > > > >> > > > Rajini > > > > > >> > > > > > > > > >> > > > On Mon, Apr 18, 2016 at 4:41 PM, Rajini Sivaram < > > > > > >> > > > rajinisiva...@googlemail.com> wrote: > > > > > >> > > > > > > > > >> > > > > Hi All, > > > > > >> > > > > > > > > > >> > > > > I have just created KIP-55 to support quotas based on > > > > > >> authenticated > > > > > >> > > user > > > > > >> > > > > principals. > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-55%3A+Secure+Quotas+for+Authenticated+Users > > > > > >> > > > > > > > > > >> > > > > Comments and feedback are appreciated. > > > > > >> > > > > > > > > > >> > > > > Thank you... > > > > > >> > > > > > > > > > >> > > > > Regards, > > > > > >> > > > > > > > > > >> > > > > Rajini > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > -- > > > > > >> > > > Regards, > > > > > >> > > > > > > > > >> > > > Rajini > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > > -- > > > > > >> > > Thanks, > > > > > >> > > Ewen > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > -- > > > > > >> > Regards, > > > > > >> > > > > > > >> > Rajini > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Regards, > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Regards, > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > -- > > > Regards, > > > > > > Rajini > > > > > > > > > -- > Regards, > > Rajini >
