Hi Rajini, Ok, I think I got you. I wasn't calculating with the fact that the parent might not be set, therefore it could be a default user as well or even the default client if nothing else is set (supposing we're talking about the <user, client> example). So if I'm correct, the quota will be applied in the order of the above points. In this case your argument is absolutely valid. I'll modify the QuotaSource.
About your last point: yes, I was hesitating a lot. I thought the interface would be simpler but after removing the helpers it's not that scary afterall :). I'll start the vote. Viktor On Thu, Jan 18, 2018 at 7:59 PM, Rajini Sivaram <rajinisiva...@gmail.com> wrote: > Hi Viktor, > > Thanks for the updates. > > *QuotaSource* currently has *Self/Default/Parent*. Not sure if that is > sufficient. > For the entity <user, client-id>, quota could be used from any of these > configs: > > 1. /config/users/<user>/clients/<client-id> > 2. /config/users/<user>/clients/<default> > 3. /config/users/<user> > 4. /config/users/<default>/clients/<client-id> > 5. /config/users/<default>/clients/<default> > 6. /config/users/<default> > 7. /config/clients/<client-id> > 8. /config/clients/<default> > > So perhaps we should have a *QuotaSource* entry for each of these eight? > > A couple of minor points: > > - *Help Message* still uses --config.properties > - The other AdminClient APIs don't use aliases for various collections. > So not sure if we need the aliases here. I think you can leave it as-is > and > see what others think. > > Yes, please do start the voting thread to make it in time for the KIP > freeze. > > Thank you, > > Rajini > > > On Thu, Jan 18, 2018 at 6:15 PM, Viktor Somogyi <viktorsomo...@gmail.com> > wrote: > > > Rajini, I have updated the KIP as agreed. Could you please have a second > > look at it? > > I have also added a section about SCRAM: > > "To enable describing and altering SCRAM credentials we will use the > > DescribeConfigs and AlterConfigs protocols. There are no changes in the > > protocol's structure but we will allow the USER resource type to be > passed > > in the protocol. When this happens, the server will know that SCRAM > configs > > are asked and will send them in the response. In case of AlterConfigs > if a > > USER resource type is passed it will validate if there are only SCRAM > > credentials are changed. If not, then will fail with > > InvalidRequestException > > ." > > > > If you don't have any comments, we might start voting as we're close to > KIP > > freeze. > > > > On Thu, Jan 18, 2018 at 12:12 PM, Viktor Somogyi < > viktorsomo...@gmail.com> > > wrote: > > > > > 3. Ok, I'll remove this from the KIP for now and perhaps add a future > > > considerations section with the idea. > > > > > > 9. Ok, I can do that. > > > > > > On Thu, Jan 18, 2018 at 11:18 AM, Rajini Sivaram < > > rajinisiva...@gmail.com> > > > wrote: > > > > > >> Hi Viktor, > > >> > > >> 3. Agree that it would be better to use something like > ConfigEntityList > > >> rather than ListQuotas. But I would leave it out for now since we are > so > > >> close to KIP freeze. We can introduce it later if required. Earlier, I > > was > > >> thinking that if we just wanted to get a list of entities without > their > > >> actual quota values, you could have an option in DescribeQuotas to > > return > > >> the entities without the quota values. But actually that doesn't make > > >> sense > > >> since you will need to read all the ZK entries and find the ones with > > >> quotas in the first place. So let's just leave DescribeQuotas as-is. > > >> > > >> 7. Yes, with client-id quotas at the lowest level. The full list in > the > > >> order of precedence is here: > > >> https://kafka.apache.org/documentation/#design_quotasconfig > > >> > > >> 9. One more suggestion. Since DescribeQuotas and AlterQuotas are > > specific > > >> to quotas, we could use *quota* instead of *config* in the protocol > (and > > >> AdminClient API). Instead of *config_name*, we could use a > *quota_type* > > >> enum (we have three types). And *config_value *could be *quota_value > > *that > > >> is a double rather than a string*,* > > >> > > >> On Thu, Jan 18, 2018 at 9:38 AM, Viktor Somogyi < > > viktorsomo...@gmail.com> > > >> wrote: > > >> > > >> > Hi Rajini, > > >> > > > >> > 1. Yes, --adminclient.config it is. I missed that, sorry :) > > >> > > > >> > 3. Indeed SCRAM in this case can raise complications. Since we'd > like > > to > > >> > handle altering SCRAM credentials via AlterConfigs, I think we > should > > >> use > > >> > DescribeConfigs to describe them. That is, to describe all the > > entities > > >> of > > >> > a given type we might need to introduce some kind of generic way of > > >> getting > > >> > metadata of config entities instead of ListQuotas which is very > > >> specific. > > >> > Therefore instead of ListQuotas we could have a ConfigMetadata (or > > >> > ConfigEntityList) protocol tailored to the needs of the admin > client. > > >> This > > >> > protocol would accept a list of config entities for the request and > > >> produce > > >> > a list of entities as a response. For instance requesting (type:USER > > >> > name:user1, type:CLIENT name:) resources would return all the > clients > > of > > >> > user1. This I think would better fit the use case and potentially > > future > > >> > use case too (like 'group' that you mentioned). > > >> > What do you think? Should we introduce a protocol like this or shall > > we > > >> > solve the problem without it? > > >> > Also, in a previous email you mentioned that we could use options. > > Could > > >> > you please elaborate on this idea? > > >> > > > >> > 7. Yea, that is a good idea. How are quotas applied? Does it first > > fall > > >> > back to the default on the same level and if there is no default > then > > >> > applies the parent's config? > > >> > > > >> > Regards, > > >> > Viktor > > >> > > > >> > > > >> > On Wed, Jan 17, 2018 at 12:56 PM, Rajini Sivaram < > > >> rajinisiva...@gmail.com> > > >> > wrote: > > >> > > > >> > > Hi Viktor, > > >> > > > > >> > > Thank you for the responses. > > >> > > > > >> > > 1. ConsoleProducer uses *--producer.config <file> > > --producer-property > > >> > > key=value*, ConsoleConsumer uses* --consumer.config <file> > > >> > > --consumer-property key=value*, so perhaps we should use > > >> > > *--adminclient.config > > >> > > *rather than *--config.properties*? > > >> > > > > >> > > 3. The one difference is that with ListGroups, ListTopics etc. you > > are > > >> > > listing the entities (groups/topics). With ConfigCommand, you can > > list > > >> > > entities and that makes sense. But with ListQuotas, quota is an > > >> > attribute, > > >> > > the entity is user/clientid/(user, clientId). This is significant > > >> since > > >> > we > > >> > > can store other attributes for those entities. For instance, we > > store > > >> > SCRAM > > >> > > credentials along with quotas for 'user'. So to ListQuotas for > user, > > >> you > > >> > > need to actually get the entries and check if quotas are defined > or > > >> just > > >> > > credentials. > > >> > > > > >> > > 7. > > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > >> > > 226+-+Dynamic+Broker+Configuration > > >> > > is > > >> > > replacing* is_default *flag in the config entry for > DescribeConfigs > > >> with > > >> > a* > > >> > > config_source* enum which indicates where the config came from. > > >> Perhaps > > >> > we > > >> > > could do something similar here? > > >> > > > > >> > > 8. Yes, that is correct. > > >> > > > > >> > > Regards, > > >> > > > > >> > > Rajini > > >> > > > > >> > > On Tue, Jan 16, 2018 at 2:59 PM, Viktor Somogyi < > > >> viktorsomo...@gmail.com > > >> > > > > >> > > wrote: > > >> > > > > >> > > > Hi Rajini, > > >> > > > > > >> > > > 1 and 2: corrected it in my code. So there will be 3 properties > in > > >> this > > >> > > > group: --bootstrap-server, --config.properties and > > >> > --adminclient-property > > >> > > > (following the conventions established elsewhere, like the > > >> > > > console-producer). > > >> > > > > > >> > > > 3: Let me explain the reason for ListQuotas. In the current > > version > > >> of > > >> > > > kafka-configs you can do this: > > >> > > > bin/kafka-configs.sh --zookeeper localhost:2181 --describe > > >> > --entity-type > > >> > > > users > > >> > > > And this will return you all the configs for users under > > >> /config/users > > >> > > > znode. In that command you have direct access to zookeeper, so > you > > >> can > > >> > > > instantly do an iteration through the znode. Therefore I looked > at > > >> > other > > >> > > > protocols (ListAcls ListGroups, ListTopics) and thought it would > > be > > >> > > aligned > > >> > > > with those if I separated off listing. This has the pros of > being > > >> able > > >> > to > > >> > > > return a list of entities or fine tuning permissions (you might > > have > > >> > > users > > >> > > > who don't have to know other users' quota settings). Once the > list > > >> of > > >> > > > resources returned, the user can initiate a bulk describe. > > >> > > > Of course the cons of having ListQuotas as a separate protocol > is > > >> that > > >> > it > > >> > > > might do something too simple for a protocol and actually as you > > >> say it > > >> > > > might be implemented with DescribeQuotasOptions perhaps by only > > >> using > > >> > an > > >> > > > extra flag in the DescribeQuotas protocol (like "describe_all"). > > >> > > > Do you think it would be better to add an option to "describe > > all"? > > >> > Also > > >> > > of > > >> > > > course the response would be "asymmetric" to the request in this > > >> case > > >> > > > meaning that I send one resource and might get back more. One of > > my > > >> > > reasons > > >> > > > of implementing this "list then describe" way of doing things > was > > >> to be > > >> > > > aligned with DescribeConfigs (as that is also symmetric > > similarly). > > >> > > > > > >> > > > 4. OK, I think I can do that, it makes sense. > > >> > > > > > >> > > > 5. Sure, I can do that. In fact I started with this but then > > >> reverted > > >> > as > > >> > > I > > >> > > > didn't know if it's really planned to have more levels. > > >> > > > > > >> > > > 6. OK, will remove those. > > >> > > > > > >> > > > 7. Well, at this point if you specify (userA, client1) it will > > >> simply > > >> > > get's > > >> > > > the znode's data at /config/users/userA/clients/client1 . If > > there > > >> is > > >> > no > > >> > > > such client it returns empty. This now functionally compatible > > with > > >> the > > >> > > > current ConfigCommand. However what you're saying I think makes > > >> sense, > > >> > > > meaning that we want to return what will actually be applied if > > the > > >> > exact > > >> > > > mapping doesn't exist (and may tell the user that for instance > > >> "there > > >> > is > > >> > > no > > >> > > > client1 for userA so I returned that will be applied"). > > >> > > > > > >> > > > 8. Certainly, I can include that as well. Just to confirm, are > you > > >> > > thinking > > >> > > > about something like this? > > >> > > > bin/kafka-configs.sh --zookeeper localhost:2181 --alter > > --add-config > > >> > > > 'SCRAM-SHA-256=[iterations=8192,password=alice-secret],SCRAM > > >> > > > -SHA-512=[password=alice-secret]' > > >> > > > --entity-type users --entity-name alice > > >> > > > Which will result in: > > >> > > > get /config/users/alice > > >> > > > {"version":1,"config":{"SCRAM-SHA-512":"salt=some_salt,store > > >> > > > d_key=some_key,server_key=some_server_key,iterations= > > >> > > > 4096","SCRAM-SHA-256":"salt=some_salt,stored_key=some_key_ > > >> > > > 2,server_key=some_server_key_2,iterations=8192"}} > > >> > > > > > >> > > > Regards, > > >> > > > Viktor > > >> > > > > > >> > > > > > >> > > > On Tue, Jan 16, 2018 at 12:33 PM, Rajini Sivaram < > > >> > > rajinisiva...@gmail.com> > > >> > > > wrote: > > >> > > > > > >> > > > > Hi Viktor, > > >> > > > > > > >> > > > > Thank you for the KIP. It is looking good. A few comments: > > >> > > > > > > >> > > > > 1. --bootstrap-server option: "*Help Message*" uses > > >> > > > --bootstrap-servers. I > > >> > > > > think other tools use the singular form even though it should > > >> > probably > > >> > > > have > > >> > > > > been plural to start with. Can we use* --bootstrap-server* for > > >> > > > consistency? > > >> > > > > > > >> > > > > 2. ConsoleProducer/ConsoleConsumer etc. have two ways of > > >> providing > > >> > > > client > > >> > > > > properties: *--producer.config <configFile>* and > > >> *--producer.property > > >> > > > > key=value*. Can we do the same here since it is easier for > > >> scripts to > > >> > > > pass > > >> > > > > in property on the command line sometimes rather than a file. > > >> Perhaps > > >> > > > > *--adminclient.config > > >> > > > > *and *--adminclient.property* or something along those lines? > > >> > > > > > > >> > > > > 3. Not sure if ListQuotas is useful. For various other > entities, > > >> we > > >> > > just > > >> > > > > have a Describe request. Wouldn't that be sufficient? If we > did > > >> want > > >> > a > > >> > > > less > > >> > > > > detailed version, we could have options for the describe > > request. > > >> > > > > > > >> > > > > 4. We use "<default>" internally to store default quotas and > > >> other > > >> > > > > defaults. But I don't think we should externalise that string. > > We > > >> use > > >> > > > empty > > >> > > > > string elsewhere for indicating default, we can do the same > > here. > > >> And > > >> > > we > > >> > > > > use STRING rather than NULLABLE_STRING in describe configs > etc. > > >> So we > > >> > > > > should probably do the same for quotas. > > >> > > > > > > >> > > > > 5. At the moment, we have two levels of quotas and you can > > define > > >> > > <user, > > >> > > > > clientId> quotas. We may want to add more levels in the > future, > > >> eg. > > >> > > > <group, > > >> > > > > user, clientId>. It would be more flexible to specify an array > > of > > >> > > > entities > > >> > > > > rather than a single child entity. (e.g resource => > > >> [quota_entity], > > >> > > > > quota_entity => entity_type entity_name). > > >> > > > > > > >> > > > > 6. In DescribeQuotasResponse, we don't need the flags > read_only > > or > > >> > > > > is_sensitve since they will always be false for quotas. > > >> > > > > > > >> > > > > 7. What will DescribeQuotas(userA, client1) actually return? > > Will > > >> it > > >> > > > return > > >> > > > > the quota defined for (userA, client1)? Or will it return the > > >> quota > > >> > > that > > >> > > > > will be applied to (userA, client1) - i.e. if there is no > quota > > >> > defined > > >> > > > for > > >> > > > > (userA, client1), but there is a quota defined for userA, will > > it > > >> > > return > > >> > > > > userA quota since that will be the quota applied to (userA, > > >> client1)? > > >> > > > > > > >> > > > > 8. I think at the moment, AdminClient doesn't have an API for > > >> > creating > > >> > > > > SCRAM credentials (ConfigCommand is used for this). I think > the > > >> > > existing > > >> > > > > describe/alterConfigs API can be used if we add a resource > type > > >> USER. > > >> > > If > > >> > > > > this will be done in the same JIRA, can we include it in the > > KIP? > > >> > > > > > > >> > > > > Regards, > > >> > > > > > > >> > > > > Rajini > > >> > > > > > > >> > > > > > > >> > > > > On Fri, Jan 12, 2018 at 11:11 AM, Viktor Somogyi < > > >> > > > viktorsomo...@gmail.com> > > >> > > > > wrote: > > >> > > > > > > >> > > > > > Hi all, > > >> > > > > > > > >> > > > > > I have submitted KIP-248 that details how can > kafka-configs.sh > > >> > could > > >> > > > > > utilize KafkaAdminClient through a new ConfigCommand class > in > > >> the > > >> > > tools > > >> > > > > > module: > > >> > > > > > > > >> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > 248+-+ > > >> > > > > > Create+New+ConfigCommand+That+Uses+The+New+AdminClient > > >> > > > > > > > >> > > > > > This KIP proposes to add 3 new wire protocols (listing quota > > >> > configs, > > >> > > > > > describing quota configs and altering quota configs), a new > > >> class > > >> > > > called > > >> > > > > > ConfigCommand in tools which would be the main entry point > for > > >> > > > > > kafka-configs.sh and a way to deprecate the current Scala > > >> > > > ConfigCommand. > > >> > > > > > > > >> > > > > > I'd be happy to receive some feedback about the proposal. > > >> > > > > > > > >> > > > > > Thank you & regards, > > >> > > > > > Viktor > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > >
