Thanks for the feedback. Below are some responses:
> I don't have any problem with breaking things into 2 requests if it's
> necessary or optimal. But can you explain why separate requests "vastly
> simplifies the broker side implementation"? It doesn't seem like it should
> be particularly complex to process each ACL change in order.
You are right, it isn't super difficult to process ACL changes in order.
The simplicity I was thinking about comes from not needing to group and
re-order them like in the current implementation. It also removes the
"action" enumerator which also simplifies things a bit. I am open to both
ideas (single alter processing in order vs separate requests) as the trade
offs aren't drastic.
I am leaning towards separate requests for a few reasons:
- The Admin API will likely submit separate requests for delete and add
regardless
- I expect most admin apis will have a removeAcl/s and addAcl/s api
the fires a request immediately. I don't expect "batching" explicit or
implicit to be all that common.
- Independent concerns and capabilities
- Separating delete into its own request makes it easier to define
"delete all" type behavior.
- I think 2 simple requests may be easier to document and understand
by client developers than 1 more complicated one.
- Matches the Authorizer interface
- Separate requests matches authorizer interface more closely and
still allows for actions on collections of ACLs instead of one
call per ACL
via Authorizer.addAcls(acls: Set[Acl], resource: Resource) and
Authorizer.removeAcls(acls: Set[Acl], resource: Resource)
Hmm, even if all ACL requests are directed to the controller, concurrency
> is only guaranteed on the same connection. For requests coming from
> different connections, the order that they get processed on the broker is
> non-deterministic.
I was thinking less about making sure the exact order of requests is
handled correctly, since each client will likely get a response between
each request before sending another. Its more about ensuring local
state/cache is accurate and that there is no way 2 nodes can have different
ACLs which they think are correct. Good implementations will handle this,
but may take a performance hit.
Perhaps I am overthinking it and the SimpleAclAuthorizer is the only one
that would be affected by this (and likely rarely because volume of ACL
write requests should not be high). The SimpleAclAuthorizer is eventually
consistent between instances. It writes optimistically with the cached zk
node version while writing a complete list of ACLs (not just adding
removing single nodes for acls). Below is a concrete demonstration of the
impact I am thinking about with the SimpleAclAuthorizer:
If we force all writes to go through one instance then follow up (ignoring
first call to warm cache) writes for a resource would:
1. Call addAcls
2. Call updateResourceAcls combining the current cached acls and the new
acls
3. Write the result to Zookeeper via conditional write on the cached
version
4. Success 1 remote call
If requests can go through any instance then follow up writes for a
resource may:
1. Call addAcls
2. Call updateResourceAcls combining the current cached acls and the new
acls
1. If no cached acls read from Zookeeper
3. Write the result to Zookeeper via conditional write on the cached
version
1. If the cached version is wrong due to a write on another
instance read from Zookeeper
2. Rebuild the final ACLs list
3. Repeat until the write is successful
4. Success in 1 or 4 (or more) remote calls
It looks like the Sentry implementation would not have this issue and the
Ranger implementation doesn't support modifying ACLs anyway (must use the
Ranger UI/API).
I wanted to explain my original thoughts, but I am happy to remove the
controller constraint given the SimpleAclAuthorizer appears to be the only
(of those I know) implementation to be affected.
Thank you,
Grant
On Sat, Aug 13, 2016 at 5:41 PM, Ewen Cheslack-Postava <e...@confluent.io>
wrote:
> On Mon, Aug 8, 2016 at 2:44 PM, Grant Henke <ghe...@cloudera.com> wrote:
>
> > Thank you for the feedback everyone. Below I respond to the last batch of
> > emails:
> >
> > You mention that "delete" actions
> > > will get processed before "add" actions, which makes sense to me. An
> > > alternative to avoid the confusion in the first place would be to
> replace
> > > the AlterAcls APIs with separate AddAcls and DeleteAcls APIs. Was this
> > > option already rejected?
> >
> >
> > 4. There is no CreateAcls or DeleteAcls (unlike CreateTopics and
> >
> > DeleteTopics, for example). It would be good to explain the reasoning for
> >
> > this choice (Jason also asked this question).
> >
> >
> > Re: 4 and Create/Delete vs Alter, I'm a fan of being able to bundle a
> bunch
> > > of changes in one request. Seems like an ACL change could easily
> include
> > > additions + deletions and is nice to bundle in one request that can be
> > > processed as quickly as possible. I don't think its a requirement, but
> > the
> > > usage pattern for changing ACLs does seem like its different from how
> you
> > > manage topics where you probably usually are either only creating or
> only
> > > deleting topics.
> >
> >
> > I had chosen to handle everything (Add/Delete) in Alter mainly for the
> > reason Ewen mentioned. I though it would be fairly common for a single
> > change to Add and Remove permissions at the same time. That said the idea
> > of having separate requests has not been discussed and is not out of the
> > question. More on this below.
> >
> > I'm a bit concerned about the ordering -- the KIP mentions processing
> > > deletes before adds. Isn't that likely to lead to gaps in security? For
> > > example, lets say I have a very simple 1 rule ACL. To change it, I have
> > to
> > > delete it and add a new one. If we do the delete first, don't we end up
> > > with an (albeit small) window where the resource ends up unprotected? I
> > > would think processing them in the order they are requested gives the
> > > operator the control they need to correctly protect resources.
> Reordering
> > > the processing of requests is also just unintuitive to me and I think
> > > probably unintuitive to users as well.
> >
> >
> > I agree that this is a bit of a nuance that could lead to rare but
> > confusing behavior. This is another chip in the "separate requests"
> > bucket.
> >
> > The more I think about the protocol and its most common usage in clients,
> > the more I lean towards separate requests. Clients are most likely to
> have
> > separate add and remove apis. In order to ensure correct/controlled
> > ordering and expected access they will likely fire separate requests
> > instead of batching.
> >
> > Though breaking the request into 2 requests causes more protocol
> messages,
> > its vastly simplifies the broker side implementation and makes the
> > expectations very clear. It also follows the Authorizer API much more
> > closely.
> >
> >
> I don't have any problem with breaking things into 2 requests if it's
> necessary or optimal. But can you explain why separate requests "vastly
> simplifies the broker side implementation"? It doesn't seem like it should
> be particularly complex to process each ACL change in order.
>
> -Ewen
>
>
> > 1. My main concern is that we are skipping the discussion on the desired
> > > model for controlling ACL access and updates. I understand the desire
> to
> > > reduce the scope, but this seems to be a fundamental aspect of the
> design
> > > that we need to get right. Without a plan for that, it is difficult to
> > > evaluate if that part of the current proposal is fine.
> >
> >
> > ++1.
> >
> >
> > Do you have any specific thoughts on the approach you would like to see?
> I
> > may be missing the impact and how it affects the rest of the proposal.
> >
> > The current proposal suggests using a wide authorization requirement
> where
> > the principal must be authorized to the "All" Operation on the "Cluster"
> > resource to alter ACLs. I suggested this approach so we could maintain
> > backwards compatibility with existing Authorizer implementations while
> > still leaving the door open for a more fine grained approach later. If we
> > add new Operations or ResourceTypes the existing Authorizer
> implementations
> > will not expect them and could break.
> >
> > I think choosing to solve it now or later should have no bearing on the
> > approach or impact since the "All" Operation on the "Cluster" resource
> will
> > grant access to any model we choose. I could be missing something though.
> >
> > I had marked it in the follow up work section on the KIP-4 Wiki:
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 4+-+Command+line+and+centralized+administrative+operations#KIP-4-
> > Commandlineandcentralizedadministrativeoperations-ACLAdminSchema
> >
> >
> > > 2. Are the Java objects in "org.apache.kafka.common.security.auth"
> going
> > > to
> > > be public API? If so, we should explain why they should be public and
> > > describe them in the KIP. If not, we should mention that.
> >
> >
> > They are public API. They are leveraged and exposed in the
> Request/Response
> > objects and likely the Admin clients. I can describe them a bit more in
> the
> > KIP. They are basically 1-1 mapping from the Authorizer interface. The
> only
> > reason they are being moved/duplicated is because they are needed by the
> > clients package and in Java.
> >
> >
> > > 3. It would be nice to have a name for a (Resource, ACL) pair. The
> > current
> > > protocol uses `requests`/`responses` for the list of such pairs, but it
> > > would be nice to have something more descriptive, if possible. Any
> ideas?
> >
> >
> > The problem w/ being more descriptive is that its possible that
> >
> > it restricts potential use cases if people think that somehow
> >
> > their use case wouldn't fit.
> >
> >
> >
> > > In the database world (Resource, ACL) pair is typically called a
> > > "grant". (sorry)
> > >
> > > You "grant" permission on a resource to a user.
> >
> >
> > "grant" has a nice sound to it...I can't figure it out but I like it a
> lot.
> > Happy to use that as the term for the resource & ACLs pair. I don't think
> > it will restrict any potential use cases.
> >
> > I didn't use the term since its not used in the Authorizer API methods
> > (addAcls, removeAcls) or existing docs.
> >
> > 5. What is the plan for when we add standard exceptions to the Authorizer
> > > interface? Will we bump the protocol version?
> >
> >
> > Currently I sort of "punt" on this issue. I catch any throwable and log
> it
> > server side before letting the generic error handling take over. If the
> > error is not mapped in Errors.java it results in a Unknown error code
> back
> > to the client. This means that when we decide to set standard/descriptive
> > exceptions (KIP-50?) they can be communicated without any server side
> > changes.
> >
> > An alternative is to wrap all exceptions in some generic
> > AuthorizerException but that doesn't feel much more descriptive or
> helpful.
> >
> > I could also add the current exceptions laid out in KIP-50 here
> > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 50+-+Move+Authorizer+to+o.a.k.common+package#KIP-50-
> MoveAuthorizertoo.a.k.
> > commonpackage-AddexceptionsrelatedtoAuthorizer.>,
> > but they will not be thrown until new Authorizer implementations use
> them.
> >
> > Is there any benefit for sending the AlterAcls
> > > request to the controller? The controller is currently only designed
> for
> > > sending topic level metadata.
> > >
> >
> > Essentially I am calling the controller node the leader for ACL writes.
> The
> > Authorizer API exposes no details of delayed writes or consensus. If we
> > allow writes to go to any node, we will be crossing our fingers that any
> > Authorizer implementation handles this concurrency problem well today.
> Even
> > if the implementation is sound, directing the low volume writes to a
> single
> > node prevents delays due to retries and stale caches.
> >
> > As an example, "KAFKA-3328
> > <https://issues.apache.org/jira/browse/KAFKA-3328>: SimpleAclAuthorizer
> > can
> > lose ACLs with frequent add/remove calls" would have been a much bigger
> > issue if requests could go to all nodes. Even after the fix, it could
> cause
> > delayed writes because of retries.
> >
> > Its not absolutely required, but I think its a good simplification.
> >
> > Thanks,
> > Grant
> >
> > On Fri, Jul 29, 2016 at 12:03 AM, Gwen Shapira <g...@confluent.io>
> wrote:
> >
> > > In the database world (Resource, ACL) pair is typically called a
> > > "grant". (sorry)
> > >
> > > You "grant" permission on a resource to a user.
> > >
> > > http://dev.mysql.com/doc/refman/5.7/en/show-grants.html
> > >
> > > Gwen
> > >
> > >
> > >
> > >
> > > On Fri, Jul 22, 2016 at 4:13 AM, Jim Jagielski <j...@jagunet.com>
> wrote:
> > > >
> > > >> On Jul 21, 2016, at 10:57 PM, Ismael Juma <ism...@juma.me.uk>
> wrote:
> > > >>
> > > >> Hi Grant,
> > > >>
> > > >> Thanks for the KIP. A few questions and comments:
> > > >>
> > > >> 1. My main concern is that we are skipping the discussion on the
> > desired
> > > >> model for controlling ACL access and updates. I understand the
> desire
> > to
> > > >> reduce the scope, but this seems to be a fundamental aspect of the
> > > design
> > > >> that we need to get right. Without a plan for that, it is difficult
> to
> > > >> evaluate if that part of the current proposal is fine.
> > > >
> > > > ++1.
> > > >
> > > >> 2. Are the Java objects in "org.apache.kafka.common.security.auth"
> > > going to
> > > >> be public API? If so, we should explain why they should be public
> and
> > > >> describe them in the KIP. If not, we should mention that.
> > > >> 3. It would be nice to have a name for a (Resource, ACL) pair. The
> > > current
> > > >> protocol uses `requests`/`responses` for the list of such pairs, but
> > it
> > > >> would be nice to have something more descriptive, if possible. Any
> > > ideas?
> > > >
> > > > The problem w/ being more descriptive is that its possible that
> > > > it restricts potential use cases if people think that somehow
> > > > their use case wouldn't fit.
> > > >
> > > >> 4. There is no CreateAcls or DeleteAcls (unlike CreateTopics and
> > > >> DeleteTopics, for example). It would be good to explain the
> reasoning
> > > for
> > > >> this choice (Jason also asked this question).
> > > >> 5. What is the plan for when we add standard exceptions to the
> > > Authorizer
> > > >> interface? Will we bump the protocol version?
> > > >>
> > > >> Thanks,
> > > >> Ismael
> > > >>
> > > >> On Thu, Jul 14, 2016 at 5:09 PM, Grant Henke <ghe...@cloudera.com>
> > > wrote:
> > > >>
> > > >>> The KIP-4 Delete Topic Schema vote has passed and the patch
> > > >>> <https://github.com/apache/kafka/pull/1616> is available for
> review.
> > > Now I
> > > >>> would like to start the discussion for the Acls request/response
> and
> > > server
> > > >>> side implementations. This includes the ListAclsRequest/Response
> and
> > > the
> > > >>> AlterAclsRequest/Response.
> > > >>>
> > > >>> Details for this implementation can be read here:
> > > >>> *
> > > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 4+-+Command+line+and+centralized+administrative+operations#KIP-4-
> > > Commandlineandcentralizedadministrativeoperations-
> > > ACLAdminSchema(KAFKA-3266)
> > > >>> <
> > > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 4+-+Command+line+and+centralized+administrative+operations#KIP-4-
> > > Commandlineandcentralizedadministrativeoperations-
> > > ACLAdminSchema(KAFKA-3266)
> > > >>>> *
> > > >>>
> > > >>> I have included the exact content below for clarity:
> > > >>>
> > > >>>> ACL Admin Schema (KAFKA-3266
> > > >>>> <https://issues.apache.org/jira/browse/KAFKA-3266>)
> > > >>>>
> > > >>>> *Note*: Some of this work/code overlaps with "KIP-50 - Move
> > > Authorizer to
> > > >>>> o.a.k.common package
> > > >>>> <
> > > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 50+-+Move+Authorizer+to+o.a.k.common+package
> > > >>>> ".
> > > >>>> KIP-4 does not change the Authorizer interface at all, but does
> > > provide
> > > >>>> java objects in "org.apache.kafka.common.security.auth" to be
> used
> > > in the
> > > >>>> protocol request/response classes. It also provides translations
> > > between
> > > >>>> the Java and Scala versions for server side compatibility with
> > > >>>> the Authorizer interface.
> > > >>>>
> > > >>>> List ACLs Request
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> ListAcls Request (Version: 0) => principal resource
> > > >>>> principal => NULLABLE_STRING
> > > >>>> resource => resource_type resource_name
> > > >>>> resource_type => INT8
> > > >>>> resource_name => STRING
> > > >>>>
> > > >>>> Request semantics:
> > > >>>>
> > > >>>> 1. Can be sent to any broker
> > > >>>> 2. If a non-null principal is provided the returned ACLs will be
> > > >>>> filtered by that principle, otherwise ACLs for all principals
> will
> > > be
> > > >>>> listed.
> > > >>>> 3. If a resource with a resource_type != -1 is provided ACLs
> will
> > be
> > > >>>> filtered by that resource, otherwise ACLs for all resources will
> > be
> > > >>> listed.
> > > >>>> 4. Any principle can list their own ACLs where the permission
> type
> > > is
> > > >>>> "Allow", Otherwise the principle must be authorized to the "All"
> > > >>> Operation
> > > >>>> on the "Cluster" resource to list ACLs.
> > > >>>> - Unauthorized requests will receive a
> > ClusterAuthorizationException
> > > >>>> - This avoids adding a new operation that an existing
> > authorizer
> > > >>>> implementation may not be aware of.
> > > >>>> - This can be reviewed and further refined/restricted as a
> > follow
> > > >>>> up ACLs review after this KIP. See Follow Up Changes
> > > >>>> <
> > > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 4+-+Command+line+and+centralized+administrative+operations#KIP-4-
> > > Commandlineandcentralizedadministrativeoperations-follow-up-changes
> > > >>>>
> > > >>>> .
> > > >>>> 5. Requesting a resource or principle that does not have any
> ACLs
> > > will
> > > >>>> not result in an error, instead empty response list is returned
> > > >>>>
> > > >>>> List ACLs Response
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> ListAcls Response (Version: 0) => [responses] error_code
> > > >>>> responses => resource [acls]
> > > >>>> resource => resource_type resource_name
> > > >>>> resource_type => INT8
> > > >>>> resource_name => STRING
> > > >>>> acls => acl_principle acl_permission_type acl_host
> acl_operation
> > > >>>> acl_principle => STRING
> > > >>>> acl_permission_type => INT8
> > > >>>> acl_host => STRING
> > > >>>> acl_operation => INT8
> > > >>>> error_code => INT16
> > > >>>>
> > > >>>> Alter ACLs Request
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> AlterAcls Request (Version: 0) => [requests]
> > > >>>> requests => resource [actions]
> > > >>>> resource => resource_type resource_name
> > > >>>> resource_type => INT8
> > > >>>> resource_name => STRING
> > > >>>> actions => action acl
> > > >>>> action => INT8
> > > >>>> acl => acl_principle acl_permission_type acl_host
> acl_operation
> > > >>>> acl_principle => STRING
> > > >>>> acl_permission_type => INT8
> > > >>>> acl_host => STRING
> > > >>>> acl_operation => INT8
> > > >>>>
> > > >>>> Request semantics:
> > > >>>>
> > > >>>> 1. Must be sent to the controller broker
> > > >>>> 2. If there are multiple instructions for the same resource in
> one
> > > >>>> request an InvalidRequestException will be logged on the broker
> > and
> > > a
> > > >>>> single error code for that resource will be returned to the
> client
> > > >>>> - This is because the list of requests is modeled server side
> > as
> > > a
> > > >>>> map with resource as the key
> > > >>>> 3. ACLs with a delete action will be processed first and the add
> > > >>>> action second.
> > > >>>> 1. This is to prevent confusion about sort order and final state
> > > when
> > > >>>> a batch message is sent.
> > > >>>> 2. If an add request was processed first, it could be deleted
> > > right
> > > >>>> after.
> > > >>>> 3. Grouping ACLs by their action allows batching requests to
> > the
> > > >>>> authorizer via the Authorizer.addAcls and
> Authorizer.removeAcls
> > > >>> calls.
> > > >>>> 4. The request is not transactional. One failure wont stop
> others
> > > from
> > > >>>> running.
> > > >>>> 1. If an error occurs on one action, the others could still
> be
> > > run.
> > > >>>> 2. Errors are reported independently.
> > > >>>> 5. The principle must be authorized to the "All" Operation on
> > the
> > > >>>> "Cluster" resource to alter ACLs.
> > > >>>> - Unauthorized requests will receive a
> > > >>> ClusterAuthorizationException
> > > >>>> - This avoids adding a new operation that an existing
> > authorizer
> > > >>>> implementation may not be aware of.
> > > >>>> - This can be reviewed and further refined/restricted as a
> > follow
> > > >>>> up ACLs review after this KIP. See Follow Up Changes
> > > >>>> <
> > > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 4+-+Command+line+and+centralized+administrative+operations#KIP-4-
> > > Commandlineandcentralizedadministrativeoperations-follow-up-changes
> > > >>>>
> > > >>>> .
> > > >>>>
> > > >>>> QA:
> > > >>>>
> > > >>>> - Why doesn't this request have a timeout and implement any
> > blocking
> > > >>>> like the CreateTopicsRequest?
> > > >>>> - The Authorizer implementation is synchronous and exposes no
> > > >>>> details about propagating the ACLs to other nodes.
> > > >>>> - The best we can do in the existing implementation is
> > > >>>> call Authorizer.addAcls and Authorizer.removeAcls and hope
> the
> > > >>> underlying
> > > >>>> implementation handles the rest.
> > > >>>> - What happens if there is an error in the Authorizer?
> > > >>>> - Currently the best we can do is log the error broker side
> and
> > > >>>> return a generic exception because there are no "standard"
> > > >>> exceptions
> > > >>>> defined in the Authorizer interface to provide a more clear
> > code
> > > >>>> - KIP-50
> > > >>>> <
> > > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 50+-+Move+Authorizer+to+o.a.k.common+package#KIP-50-
> > MoveAuthorizertoo.a.k.
> > > commonpackage-AddexceptionsrelatedtoAuthorizer.>
> > > >>> is
> > > >>>> tracking adding the standard exceptions
> > > >>>> - The Authorizer interface also provides no feedback about
> > > >>>> individual ACLs when added or deleted in a group
> > > >>>> - Authorizer.addAcls is a void function, the best we can do
> is
> > > >>>> return an error for all ACLs and let the user check the
> > > current
> > > >>> state by
> > > >>>> listing the ACLs
> > > >>>> - Autohrizer.removeAcls is a boolean function, the best
> we
> > > can
> > > >>>> do is return an error for all ACLs and let the user check
> > the
> > > >>> current state
> > > >>>> by listing the ACLs
> > > >>>> - Behavior here could vary drastically between
> > implementations
> > > >>>> - I suggest this be addressed in KIP-50 as well, though it
> > has
> > > >>>> some compatibility concerns.
> > > >>>> - Why require the request to go to the controller?
> > > >>>> - The controller is responsible for the cluster metadata and
> > its
> > > >>>> propagation
> > > >>>> - This ensures one instance of the Authorizer sees all the
> > > changes
> > > >>>> and reduces concurrency issues, especially because the
> > Authorizer
> > > >>> interface
> > > >>>> exposes no details about propagating the ACLs to other nodes.
> > > >>>> - See Request Forwarding
> > > >>>> <
> > > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 4+-+Command+line+and+centralized+administrative+operations#KIP-4-
> > > Commandlineandcentralizedadministrativeoperations-request
> > > >>>>
> > > >>>> below
> > > >>>>
> > > >>>> Alter ACLs Response
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> AlterAcls Response (Version: 0) => [responses]
> > > >>>> responses => resource [results]
> > > >>>> resource => resource_type resource_name
> > > >>>> resource_type => INT8
> > > >>>> resource_name => STRING
> > > >>>> results => action acl error_code
> > > >>>> action => INT8
> > > >>>> acl => acl_principle acl_permission_type acl_host
> acl_operation
> > > >>>> acl_principle => STRING
> > > >>>> acl_permission_type => INT8
> > > >>>> acl_host => STRING
> > > >>>> acl_operation => INT8
> > > >>>> error_code => INT16
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>
> > > >>> Thank you,
> > > >>> Grant
> > > >>> --
> > > >>> Grant Henke
> > > >>> Software Engineer | Cloudera
> > > >>> gr...@cloudera.com | twitter.com/gchenke |
> > linkedin.com/in/granthenke
> > > >>>
> > > >
> > >
> >
> >
> >
> > --
> > Grant Henke
> > Software Engineer | Cloudera
> > gr...@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
> >
>
>
>
> --
> Thanks,
> Ewen
>
--
Grant Henke
Software Engineer | Cloudera
gr...@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke
