Hi Viktor, While implementing KAFKA-6494, I realised that there is a mismatch between the --alter command of ConfigCommand and AlterConfigs request. ConfigCommand uses --add-config and --delete-config to make incremental updates. --add-config reads all the configs from ZooKeeper and adds the delta provided on top of that. AlterConfigs request currently sets the whole properties object, so you need to know the full set of properties of an entity to use AlterConfigs request through the AdminClient. We don't allow sensitive configs to be read using AdminClient, so we can't read and add configs as we do with ZooKeeper. So we need a protocol change to make this work. I didn't want to make this change after KIP freeze, so perhaps we could include this in your KIP? We could perhaps add a mode (SET/ADD/DELETE) for AlterConfigs request where SET matches the existing behaviour for backward compatibility and ConfigCommand uses ADD/DELETE.
Thoughts? Regards, Rajini On Fri, Jan 19, 2018 at 12:57 PM, Viktor Somogyi <viktorsomo...@gmail.com> wrote: > 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 > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > > > > > > >
