That's a good point. It's worth mentioning that there is a KIP in
progress, KIP-133: List and Alter Configs Admin APIs, that should help
with those.
In the long term, it would be nice if we could deprecate
'allow.everyone.if.no.acl.found', along with topic auto-creation. It
should be possible to get the functionality of
'allow.everyone.if.no.acl.found' by just adding a few ALLOW * ACLs.
Maybe we could even do it in an upgrade script? It will take a while to
get there.
best,
Colin
On Thu, May 4, 2017, at 15:09, dan wrote:
> 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
> > > >
> >
