On Thu, May 4, 2017, at 13:46, Magnus Edenhill wrote:
> Hey Colin,
>
> good KIP!
>
> Some comments:
>
> 1a. For operation, permission_type and resource_type: is there any reason
> for having the any and unknown enums as negative values?
> Since neither of these fields has an integer significance (unlike for
> example offsets which use negative offsets for logical offsets) I dont
> really see a reason to do this. It might also trick client developers to
> make assumptions on future negative values (if resource_type < 0:
> treat_as_invalid()...), unless that's the reason :). This might be my
> personal preference but encoding extended meaning into types should be
> avoided unless needed, and I dont think it is needed for enums.
Hi Magnus,
That's a fair question. I don't have a strong preference either way.
If it is more convenient or consistent to start at 0, we can certainly
do that.
>
> but..
>
> 1b. Since most clients implementing the ACL requests probably wont make
> much use of this API themselves but rather treat it as a straight
> pass-through between the protocol and the client's public API to the
> application, could we save ourselves some work (current and future) to
> make
> the enums as nullable_strings instead of integer enums? This would cut
> down
> on the number of enum-to-str and vice versa conversions needed, and would
> also make the APIs more future proof since an added resource_type (et.al)
> would not need a client, or even tool, update, and the new type will not
> show up as UNKNOWN but of its real value.
> From a broker-side verification perspective there should really be no
> difference since the enum values will need to be interpreted anyway.
> So instead of int enum { unknown = -2, any = -1, deny, allow }, we have {
> null, "deny", "allow" }.
Strings use much, much more space, though. An INT8 is one byte, whereas
the string "clusterAction" is 13 bytes, plus a 2 byte length field (if I
remember our serialization correctly) A 10x or 15x RPC space penalty
starts to really hurt, especially when you have hundreds or thousands of
ACLs, and each one has 6 fields.
>
>
> 2. "Each of the arguments to ListAclsRequest acts as a filter."
> Specify if these filters are OR:ed or AND:ed.
Yeah, they are ANDed. I'll add a note.
>
> 3. "CreateAclsRequest and CreateAclsResponse"
> What happens if a client attempts to create an ACL entry which is
> identical
> to one already created in the cluster?
> Is this an error? silently ignored? resulting in duplicates?
Unfortunately, we are somewhat at the mercy of the
kafka.security.auth.Authorizer implementation here. The default
SimpleAclAuthorizer does not allow duplicates to be added.
If the Authorizer doesn't de-duplicate, we can't add de-duplication
without doing distributed locking of some sort. I think it makes sense
to add a new exception, DuplicateAcl, though, and specify that the
authorizer ought to throw that exception. Let me add a little more
detail here.
>
> 4. "Compatibility plan"
> "If we later add new resource types, operation types, and so forth, we
> would like to be able to interact with them with the old AdminClient.
> This
> is why the AclResourceType, AclOperation, and AclPermissionType enums
> have
> UNKNOWN values. If we get an INT8 which we don't know the name for, it
> gets mapped to an UNKNOWN object."
>
> I'm not sure who "we" are. Is it the broker or the client?
> (also see 1b for avoiding UNKNOWN altogether)
Both the broker and the client can get "unknown" values for those enums,
if someone sends them something they don't understand. The broker will
not add new ACLs that contain unknowns, nor will it delete using filters
that have unknowns. Similarly, attempting to list ACLs with unknowns is
an error. It is still possible for a client to delete an ACL that it
doesn't understand, by using ANY fields to match it. I guess I should
spell out all these cases in the wiki.
best,
Colin
>
>
> Regards,
> Magnus
>
> 2017-04-21 22:27 GMT+02:00 Colin McCabe <cmcc...@apache.org>:
>
> > Hi all,
> >
> > As part of the AdminClient work, we would like to add methods for
> > adding, deleting, and listing access control lists (ACLs). I wrote up a
> > KIP to discuss implementing requests for those operations, as well as
> > AdminClient APIs. Take a look at:
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 140%3A+Add+administrative+RPCs+for+adding%2C+deleting%2C+and+listing+ACLs
> >
> > regards,
> > Colin
> >
