On Tue, May 9, 2017, at 08:00, Ismael Juma wrote: > Hi Colin, > > I took another look and have a couple of comments: > > 1. DescribeAclsResponse only has an error code at the top level. In the > DescribeConfigs KIP, you suggested that we should have an error code at > the > batch level. Doesn't that apply here too?
Good question. I guess the answer is that I couldn't come up with any per-element error codes in DescribeAclsResponse. In DescribeConfigs, we have at least two scenarios where we need per-element error codes: 1. We sent to multiple brokers, and some sends fail. 2. We try to describe the configuration / acls of something that doesn't exist. 3. We try to describe the configuration / acls of something we don't have permissions to describe. Scenario #1 can't happen for DescribeAclsResponse, since all brokers have access to the Authorizer. We never make multiple RPCs. Scenario #2 just results in silently returning the empty set in DescribeAclsResponse. This comes directly out of the Authorizer semantics... the Authorizer doesn't know whether the Foo topic exists and doesn't care. It just knows there are no ACLs that are associated with (resourceType=topic, resourceName=Foo). (The same could be said for any other resource ACLs are potentially attached to.) I think these are reasonable semantics for the Authorizer, since it may be part of some larger authorization system that is not too tightly coupled to Kafka. In Scenario #3, we return a top-level response error currently. Partly, this is because the ability to list ACLs depends on having Cluster:Describe, which you either have or don't. We have not added fine-grained authorizations for listing specific ACLs. If we ever add more fine-grained authorizations, we'll have to choose between silently omitting results, and throwing an error that effectively says "some stuff was omitted, but we can't tell you exactly what." Neither case seems to call for a per-element error code. > > 2. CreateAclsResponse don't include the resource type and resource name > (the order of the responses has to be used to match against the request). > It seems like this would make it harder to debug and it's inconsistent > with other responses. Is it really worth the space savings? I found it helpful to prototype this, and the code is up on the KAFKA-3266 pull request. I didn't find it harder to debug due to the dependency on request ordering. It was a little bit harder to write since I had to preserve the request ordering, but not that bad. As to whether this is inconsistent with other responses... I think that's debatable. We don't send back the text of the messages which producers send to us when acknowledging it. CreateTopicsResponse doesn't include the full topic creation request-- it just includes the topic names. We couldn't do the same thing with CreateAclsResponse or DeleteAclsResponse simply because there is no "name" by which an individual ACL can be identified. Including the entire request in the response feels like a very heavy hammer to me. best, Colin > > Ismael > > On Tue, May 9, 2017 at 12:15 AM, Colin McCabe <cmcc...@apache.org> wrote: > > > Just a quick note: earlier Magnus and I talked about what happens if you > > attempt to add a duplicate ACL. It appears that SimpleAuthorizer > > silently allows re-adding an ACL that already exists. There is no error > > raised. However, the ACLs are deduplicated on the back end (only one > > ACL will show up in describeAcls). Since modifying authorizer semantics > > is out of scope for this KIP, those are the semantics we'll have for > > now. We can revisit this in KIP-50. > > > > cheers, > > Colin > > > > > > On Mon, May 8, 2017, at 10:52, Colin McCabe wrote: > > > Quick update: I renamed ListAcls to DescribeAcls, for consistency with > > > DescribeTopics, DescribeGroups, DescribeConfigs, etc. > > > > > > cheers, > > > Colin > > > > > > > > > On Fri, May 5, 2017, at 11:54, Colin McCabe wrote: > > > > Hi all, > > > > > > > > Thanks for all the discussion. I made some changes to the KIP. > > > > > > > > * After some discussion about permissions, CreateAcls/DeleteAcls now > > > > require Cluster:Alter rather than Cluster:All. > > > > > > > > * The resource type, operation type, etc. enums now start at 0 rather > > > > than at -2, for simplicity's sake > > > > > > > > * Added a new exception, SecurityDisabledException, which is returned > > > > when we attempt to list, add, or remove ACLs, but security is disabled > > > > (i.e. there is no authorizer configured to handle the request). > > > > > > > > * Added a note to listAclsRequest specifying that the arguments are > > > > ANDed together to act as a filter. > > > > > > > > * Added some clarification about the compatibility rules for ACL > > > > deletion when the client is newer than the broker > > > > > > > > cheers, > > > > Colin > > > > > > > > > > > > On Thu, May 4, 2017, at 13:46, Colin McCabe wrote: > > > > > Hi Guozhang, > > > > > > > > > > Thanks for taking a look. > > > > > > > > > > On Thu, May 4, 2017, at 00:43, Guozhang Wang wrote: > > > > > > Colin, > > > > > > > > > > > > Thanks for the KIP. A general question I have is whether we can > > support > > > > > > "wildcard" resource names as well? For example, if we want to > > create / > > > > > > delete ACLs for all topic names following a wildcard regex? I read > > though > > > > > > the principle wildcards description but I am not sure if it is > > designed > > > > > > for > > > > > > this purposes. > > > > > > > > > > If you want to delete all ACLs that apply to topics, you could use a > > > > > deletion filter with resourceType=AclResourceType.TOPIC, > > > > > resourceName=null. I think that answers the question, unless I > > > > > misunderstood. > > > > > > > > > > Note that it's important to distinguish between wildcards (the "*" > > > > > value) and "any". For example, when your deletionFilter has > > > > > principal="User:*" you are not deleting ACLs for all users. Just > > ACLs > > > > > that have the principal field set to "User:*" If you want to delete > > > > > ACLs for all users, you can use principal=null. Similarly, > > > > > principal=null is only valid in a filter, not in an actual ACL that > > is > > > > > applied to a resource. > > > > > > > > > > best, > > > > > Colin > > > > > > > > > > > > > > > > > Otherwise, lgtm! > > > > > > > > > > > > > > > > > > Guozhang > > > > > > > > > > > > > > > > > > On Fri, Apr 28, 2017 at 10:37 PM, Dongjin Lee <dong...@apache.org> > > wrote: > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > On 29 Apr 2017, 9:51 AM +0900, Michael Pearce < > > michael.pea...@ig.com>, > > > > > > > wrote: > > > > > > > > +1 > > > > > > > > ________________________________________ > > > > > > > > From: Colin McCabe <cmcc...@apache.org > > > > > > > > Sent: Saturday, April 29, 2017 1:09:25 AM > > > > > > > > To: dev@kafka.apache.org > > > > > > > > Subject: [VOTE] KIP-140: Add administrative RPCs for adding, > > deleting, > > > > > > > and listing ACLs > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > I'd like to start the voting for KIP-140: Add administrative > > RPCs for > > > > > > > > adding, deleting, and listing ACLs. This provides an API for > > adding, > > > > > > > > deleting, and listing the access control lists (ACLs) which > > are used to > > > > > > > > control access on Kafka topics and brokers. > > > > > > > > > > > > > > > > The wiki page is here: > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > > > > 140%3A+Add+administrative+RPCs+for+adding%2C+deleting% > > 2C+and+listing+ACLs > > > > > > > > > > > > > > > > The previous [DISCUSS] thread: > > > > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg70858. > > html > > > > > > > > > > > > > > > > cheers, > > > > > > > > Colin > > > > > > > > The information contained in this email is strictly > > confidential and for > > > > > > > the use of the addressee only, unless otherwise indicated. If > > you are not > > > > > > > the intended recipient, please do not read, copy, use or > > disclose to others > > > > > > > this message or any attachment. Please also notify the sender by > > replying > > > > > > > to this email or by telephone (+44(020 7896 0011 > > (tel:020%207896%200011)) > > > > > > > and then delete the email and any copies of it. Opinions, > > conclusion (etc) > > > > > > > that do not relate to the official business of this company > > shall be > > > > > > > understood as neither given nor endorsed by it. IG is a trading > > name of IG > > > > > > > Markets Limited (a company registered in England and Wales, > > company number > > > > > > > 04008957 (tel:04008957)) and IG Index Limited (a company > > registered in > > > > > > > England and Wales, company number 01190902 (tel:01190902)). > > Registered > > > > > > > address at Cannon Bridge House, 25 Dowgate Hill, London EC4R > > 2YA. Both IG > > > > > > > Markets Limited (register number 195355) and IG Index Limited > > (register > > > > > > > number 114059) are authorised and regulated > > > > > > > by the Financial Conduct Authority. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > -- Guozhang > >
