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 > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > >
