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 >
