You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Colin McCabe <cm...@apache.org> on 2017/04/21 20:27:23 UTC
[DISCUSS] KIP-140: Add administrative RPCs for adding, deleting,
and listing ACLs
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
> >
Re: [DISCUSS] KIP-140: Add administrative RPCs for adding, deleting,
and listing ACLs
Posted by Magnus Edenhill <ma...@edenhill.se>.
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 Jun,
Thanks for the review.
On Thu, Apr 27, 2017, at 14:21, Jun Rao wrote:
> Hi, Colin,
>
> Thanks for the KIP. Looks good overall. Just a few minor comments.
>
> 1. The controller is responsible for managing the state of topics. So, it
> makes sense for create/delete topic requests to be sent to the
> controller.
> The controller is not responsible for managing ACLs right now. So, it's a
> bit weird to send acl requests only to the controller. Sending the acl
> request to any broker is more intuitive and easier to implement.
That's a fair point. I'll change it so that we can send these to any
broker.
>
> 10. Since we may add new operations in the future, perhaps use 0 for the
> ALL operation?
OK.
>
> 11. "That is, ListAclsRequest(principal=none) will return all ACLs". Do
> you
> mean principal == null?
Right, this meant principal == null. I'll make that a little more
explicit.
>
> 12. "Principals must possess Cluster:All permissions to call
> CreateAclsRequest". All just means all types of operations. So, we need a
> specific operation that allows CreateAclsRequest. Will that be
> ClusterAction or a new one?
Yeah, let's use ClusterAction here.
best,
Colin
>
> Jun
>
> On Mon, Apr 24, 2017 at 10:57 AM, Colin McCabe <cm...@apache.org>
> wrote:
>
> > Thanks for taking a look, Ismael.
> >
> > On Mon, Apr 24, 2017, at 04:36, Ismael Juma wrote:
> > > Thanks Colin. A few quick comments:
> > >
> > > 1. Is there a reason why AddAclsRequest must be sent to the controller
> > > broker? When this was discussed previously, Jun suggested that such a
> > > restriction may not be necessary for this request.
> >
> > Hmm. I guess my thinking here was that since the controller handles
> > AddTopics and DeleteTopics requests, it would be nice if it had the most
> > up-to-date ACL information. This was also in the original KIP-4
> > proposal. However, given that auth is ZK (or a pluggable system,
> > optionally) there is no inherent reason the controller broker has to be
> > the only one to make the change. What do you think?
> >
> > >
> > > 2. Other protocol APIs use the Delete prefix instead of Remove. Is there
> > > a
> > > reason to deviate here? If there's a good reason to do so, we have to fix
> > > the one mention of DeleteAcls in the proposal.
> >
> > Good point. Let's make it consistent by changing it to DeleteAcls. I
> > will also change AddAclsRequest to CreateAclsRequest to match
> > CreateTopicsRequest.
> >
> > >
> > > 3. Do you mean "null" in the following sentence? "Note that an argument
> > > of
> > > "none" is different than a wildcard argument."
> >
> > For the string types, NULL is considered "none"; for the INT8 types, -1
> > is considered "none".
> >
> > >
> > > 4. How will the non-zero top-level error code be computed? A bit more
> > > detail on this would be helpful. As a general rule, most batch protocol
> > > APIs don't have a top level error code because errors are usually at the
> > > batch element level. This has proved to be a bit of an issue in cases
> > > where
> > > we want to return a generic error code (e.g. InvalidProtocolVersion).
> > > Also,
> > > V2 of OffsetFetch has a top-level error code[1]. However, the OffsetFetch
> > > behaviour is that we either use the top-level error code or the partition
> > > level error codes, which is different than what is being suggested here.
> >
> > The idea behind the top-level error code is to implement error codes
> > that don't have to do with elements in the batch. For example, if we
> > implement backpressure, the server could send back an error code of
> > "slow down" telling the client to resend the request after a few
> > milliseconds have elapsed.
> >
> > As you mention, we ought to have a generic response header that would
> > let us intelligently handle situations like "server doesn't understand
> > this request version or type" Maybe this is something that needs to be
> > handled in another KIP, though, since it would require all the responses
> > to have this, and in the same format. I guess I will remove this for
> > now.
> >
> > >
> > > 5. Nit: In other requests we used `error_message` instead of
> > > `error_string`.
> >
> > OK.
> >
> > > 6. Regarding the migration plan, it's worth making it clear that the CLI
> > > transition is not part of this KIP.
> >
> > OK.
> >
> > >
> > > 7. Regarding the forward compatibility point, we could potentially use
> > > enums with UNKNOWN element? This pattern has been used elsewhere in Kafka
> > > and it would be good to compare it with the proposed solution. An
> > > important
> > > question is what can users do with UNKNOWN elements. If the assumption is
> > > that users ignore them, then it seems like the enum approach may be good
> > > enough.
> >
> > Hmm. It seemed straightforward to let callers see the numeric value of
> > the element. It makes the command-line tools more useful when
> > interacting with clusters that have newer versions of the software, for
> > example. I guess using UNKNOWN instead of UNKNOWN(<number>) has the
> > advantage of hiding the internal representation better.
> >
> > best,
> > Colin
> >
> >
> > >
> > > Thanks,
> > > Ismael
> > >
> > > [1]
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-88%3A+
> > OffsetFetch+Protocol+Update
> > >
> > >
> > > On Fri, Apr 21, 2017 at 9:27 PM, Colin McCabe <cm...@apache.org>
> > wrote:
> > >
> > > > 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 Jun Rao <ju...@confluent.io>.
Hi, Colin,
Thanks for the KIP. Looks good overall. Just a few minor comments.
1. The controller is responsible for managing the state of topics. So, it
makes sense for create/delete topic requests to be sent to the controller.
The controller is not responsible for managing ACLs right now. So, it's a
bit weird to send acl requests only to the controller. Sending the acl
request to any broker is more intuitive and easier to implement.
10. Since we may add new operations in the future, perhaps use 0 for the
ALL operation?
11. "That is, ListAclsRequest(principal=none) will return all ACLs". Do you
mean principal == null?
12. "Principals must possess Cluster:All permissions to call
CreateAclsRequest". All just means all types of operations. So, we need a
specific operation that allows CreateAclsRequest. Will that be
ClusterAction or a new one?
Jun
On Mon, Apr 24, 2017 at 10:57 AM, Colin McCabe <cm...@apache.org> wrote:
> Thanks for taking a look, Ismael.
>
> On Mon, Apr 24, 2017, at 04:36, Ismael Juma wrote:
> > Thanks Colin. A few quick comments:
> >
> > 1. Is there a reason why AddAclsRequest must be sent to the controller
> > broker? When this was discussed previously, Jun suggested that such a
> > restriction may not be necessary for this request.
>
> Hmm. I guess my thinking here was that since the controller handles
> AddTopics and DeleteTopics requests, it would be nice if it had the most
> up-to-date ACL information. This was also in the original KIP-4
> proposal. However, given that auth is ZK (or a pluggable system,
> optionally) there is no inherent reason the controller broker has to be
> the only one to make the change. What do you think?
>
> >
> > 2. Other protocol APIs use the Delete prefix instead of Remove. Is there
> > a
> > reason to deviate here? If there's a good reason to do so, we have to fix
> > the one mention of DeleteAcls in the proposal.
>
> Good point. Let's make it consistent by changing it to DeleteAcls. I
> will also change AddAclsRequest to CreateAclsRequest to match
> CreateTopicsRequest.
>
> >
> > 3. Do you mean "null" in the following sentence? "Note that an argument
> > of
> > "none" is different than a wildcard argument."
>
> For the string types, NULL is considered "none"; for the INT8 types, -1
> is considered "none".
>
> >
> > 4. How will the non-zero top-level error code be computed? A bit more
> > detail on this would be helpful. As a general rule, most batch protocol
> > APIs don't have a top level error code because errors are usually at the
> > batch element level. This has proved to be a bit of an issue in cases
> > where
> > we want to return a generic error code (e.g. InvalidProtocolVersion).
> > Also,
> > V2 of OffsetFetch has a top-level error code[1]. However, the OffsetFetch
> > behaviour is that we either use the top-level error code or the partition
> > level error codes, which is different than what is being suggested here.
>
> The idea behind the top-level error code is to implement error codes
> that don't have to do with elements in the batch. For example, if we
> implement backpressure, the server could send back an error code of
> "slow down" telling the client to resend the request after a few
> milliseconds have elapsed.
>
> As you mention, we ought to have a generic response header that would
> let us intelligently handle situations like "server doesn't understand
> this request version or type" Maybe this is something that needs to be
> handled in another KIP, though, since it would require all the responses
> to have this, and in the same format. I guess I will remove this for
> now.
>
> >
> > 5. Nit: In other requests we used `error_message` instead of
> > `error_string`.
>
> OK.
>
> > 6. Regarding the migration plan, it's worth making it clear that the CLI
> > transition is not part of this KIP.
>
> OK.
>
> >
> > 7. Regarding the forward compatibility point, we could potentially use
> > enums with UNKNOWN element? This pattern has been used elsewhere in Kafka
> > and it would be good to compare it with the proposed solution. An
> > important
> > question is what can users do with UNKNOWN elements. If the assumption is
> > that users ignore them, then it seems like the enum approach may be good
> > enough.
>
> Hmm. It seemed straightforward to let callers see the numeric value of
> the element. It makes the command-line tools more useful when
> interacting with clusters that have newer versions of the software, for
> example. I guess using UNKNOWN instead of UNKNOWN(<number>) has the
> advantage of hiding the internal representation better.
>
> best,
> Colin
>
>
> >
> > Thanks,
> > Ismael
> >
> > [1]
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-88%3A+
> OffsetFetch+Protocol+Update
> >
> >
> > On Fri, Apr 21, 2017 at 9:27 PM, Colin McCabe <cm...@apache.org>
> wrote:
> >
> > > 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>.
Thanks for taking a look, Ismael.
On Mon, Apr 24, 2017, at 04:36, Ismael Juma wrote:
> Thanks Colin. A few quick comments:
>
> 1. Is there a reason why AddAclsRequest must be sent to the controller
> broker? When this was discussed previously, Jun suggested that such a
> restriction may not be necessary for this request.
Hmm. I guess my thinking here was that since the controller handles
AddTopics and DeleteTopics requests, it would be nice if it had the most
up-to-date ACL information. This was also in the original KIP-4
proposal. However, given that auth is ZK (or a pluggable system,
optionally) there is no inherent reason the controller broker has to be
the only one to make the change. What do you think?
>
> 2. Other protocol APIs use the Delete prefix instead of Remove. Is there
> a
> reason to deviate here? If there's a good reason to do so, we have to fix
> the one mention of DeleteAcls in the proposal.
Good point. Let's make it consistent by changing it to DeleteAcls. I
will also change AddAclsRequest to CreateAclsRequest to match
CreateTopicsRequest.
>
> 3. Do you mean "null" in the following sentence? "Note that an argument
> of
> "none" is different than a wildcard argument."
For the string types, NULL is considered "none"; for the INT8 types, -1
is considered "none".
>
> 4. How will the non-zero top-level error code be computed? A bit more
> detail on this would be helpful. As a general rule, most batch protocol
> APIs don't have a top level error code because errors are usually at the
> batch element level. This has proved to be a bit of an issue in cases
> where
> we want to return a generic error code (e.g. InvalidProtocolVersion).
> Also,
> V2 of OffsetFetch has a top-level error code[1]. However, the OffsetFetch
> behaviour is that we either use the top-level error code or the partition
> level error codes, which is different than what is being suggested here.
The idea behind the top-level error code is to implement error codes
that don't have to do with elements in the batch. For example, if we
implement backpressure, the server could send back an error code of
"slow down" telling the client to resend the request after a few
milliseconds have elapsed.
As you mention, we ought to have a generic response header that would
let us intelligently handle situations like "server doesn't understand
this request version or type" Maybe this is something that needs to be
handled in another KIP, though, since it would require all the responses
to have this, and in the same format. I guess I will remove this for
now.
>
> 5. Nit: In other requests we used `error_message` instead of
> `error_string`.
OK.
> 6. Regarding the migration plan, it's worth making it clear that the CLI
> transition is not part of this KIP.
OK.
>
> 7. Regarding the forward compatibility point, we could potentially use
> enums with UNKNOWN element? This pattern has been used elsewhere in Kafka
> and it would be good to compare it with the proposed solution. An
> important
> question is what can users do with UNKNOWN elements. If the assumption is
> that users ignore them, then it seems like the enum approach may be good
> enough.
Hmm. It seemed straightforward to let callers see the numeric value of
the element. It makes the command-line tools more useful when
interacting with clusters that have newer versions of the software, for
example. I guess using UNKNOWN instead of UNKNOWN(<number>) has the
advantage of hiding the internal representation better.
best,
Colin
>
> Thanks,
> Ismael
>
> [1]
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-88%3A+OffsetFetch+Protocol+Update
>
>
> On Fri, Apr 21, 2017 at 9:27 PM, Colin McCabe <cm...@apache.org> wrote:
>
> > 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 Ismael Juma <is...@juma.me.uk>.
Thanks Colin. A few quick comments:
1. Is there a reason why AddAclsRequest must be sent to the controller
broker? When this was discussed previously, Jun suggested that such a
restriction may not be necessary for this request.
2. Other protocol APIs use the Delete prefix instead of Remove. Is there a
reason to deviate here? If there's a good reason to do so, we have to fix
the one mention of DeleteAcls in the proposal.
3. Do you mean "null" in the following sentence? "Note that an argument of
"none" is different than a wildcard argument."
4. How will the non-zero top-level error code be computed? A bit more
detail on this would be helpful. As a general rule, most batch protocol
APIs don't have a top level error code because errors are usually at the
batch element level. This has proved to be a bit of an issue in cases where
we want to return a generic error code (e.g. InvalidProtocolVersion). Also,
V2 of OffsetFetch has a top-level error code[1]. However, the OffsetFetch
behaviour is that we either use the top-level error code or the partition
level error codes, which is different than what is being suggested here.
5. Nit: In other requests we used `error_message` instead of `error_string`.
6. Regarding the migration plan, it's worth making it clear that the CLI
transition is not part of this KIP.
7. Regarding the forward compatibility point, we could potentially use
enums with UNKNOWN element? This pattern has been used elsewhere in Kafka
and it would be good to compare it with the proposed solution. An important
question is what can users do with UNKNOWN elements. If the assumption is
that users ignore them, then it seems like the enum approach may be good
enough.
Thanks,
Ismael
[1]
https://cwiki.apache.org/confluence/display/KAFKA/KIP-88%3A+OffsetFetch+Protocol+Update
On Fri, Apr 21, 2017 at 9:27 PM, Colin McCabe <cm...@apache.org> wrote:
> 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
>