> 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 >>
