what about the configs for: `allow.everyone.if.no.acl.found` and `
super.users`?
i understand they are an implementation detail of
`SimpleAclAuthorizer` configs,
but without these its difficult to make sense of what permissions a
`ListAclsResponse`
actually represents.
maybe something for another kip.
dan
On Thu, May 4, 2017 at 2:36 PM, Colin McCabe <cmcc...@apache.org> wrote:
> 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
> > >
>
