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
