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