You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Magnus Edenhill <ma...@edenhill.se> on 2017/05/04 20:46:06 UTC
Re: [DISCUSS] KIP-140: Add administrative RPCs for adding, deleting,
and listing ACLs
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.
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" }.
2. "Each of the arguments to ListAclsRequest acts as a filter."
Specify if these filters are OR:ed or AND:ed.
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?
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)
Regards,
Magnus
2017-04-21 22:27 GMT+02:00 Colin McCabe <cm...@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
>
Re: [DISCUSS] KIP-140: Add administrative RPCs for adding, deleting,
and listing ACLs
Posted by Colin McCabe <cm...@apache.org>.
Hi all,
In the course of reviewing the AdminClient#apiVersions API, we found out
that it was using an internal class,
org.apache.kafka.clients.NodeApiVersions. This internal class
references more internal stuff, including things in
org.apache.kafka.common.protocol. So we filed KAFKA-5214 to create a
public, stable API for this.
The main changes in KAFKA-5214 are:
* Create a public NodeVersions class to be returned by AdminClient
* Split the private ApiKeys enum into a public ApiKey enum, and some
internal data in ApiKeys.java
The idea here is to keep a clean separation between the API and the
implementation, so that if we need to change internal details later, we
easily can. For example, we should be able to refactor the protocol
classes without breaking users of AdminClient.
best,
Colin
On Thu, May 4, 2017, at 16:13, Colin McCabe wrote:
> 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 <cm...@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 <cm...@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
> > > > >
> > >
Re: [DISCUSS] KIP-140: Add administrative RPCs for adding, deleting,
and listing ACLs
Posted by Colin McCabe <cm...@apache.org>.
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 <cm...@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 <cm...@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
> > > >
> >
Re: [DISCUSS] KIP-140: Add administrative RPCs for adding, deleting,
and listing ACLs
Posted by dan <da...@gmail.com>.
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 <cm...@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 <cm...@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
> > >
>
Re: [DISCUSS] KIP-140: Add administrative RPCs for adding, deleting,
and listing ACLs
Posted by Colin McCabe <cm...@apache.org>.
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 <cm...@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
> >