Hi Mickael, Thanks for reviewing the KIP.
1.) I just wanted to follow the conventions used with the Authorizer as it is built in a similar fashion, although it's true that in KafkaServer we call the configure() method and the start() in the next line. This would be the same in Auditor and even simpler as there aren't any parameters to start(), so I can remove it. If it turns out there is a need for it, we can add it later. 2.) Yes, this is a very good point, I will remove it, however in this case I don't think we need to add the ApiKey as it is already available in AuthorizableRequestContext.requestType(). One less parameter :). 3.) I'll add it. It will simply log important changes in the cluster like topic events (create, update, delete, partition or replication factor change), ACL events, config changes, reassignment, altering log dirs, offset delete, group delete with the authorization info like who initiated the call, was it authorized, were there any errors. Let me know if you think there are other APIs I should include. 4.) The builder is there mostly for easier usability but actually thinking of it it doesn't help much so I removed it. The AuditInfo is also a helper class so I don't see any value in transforming it into an interface but if I simplify it (by removing the builder) it will be cleaner. Would that work? I'll update the KIP to reflect my answers. Viktor On Mon, Sep 14, 2020 at 6:02 PM Mickael Maison <mickael.mai...@gmail.com> wrote: > Hi Viktor, > > Thanks for restarting the discussion on this KIP. Being able to easily > audit usage of a Kafka cluster is a very valuable feature. > > Regarding the API, I have a few of questions: > 1) You introduced a start() method. I don't think any other interfaces > have such a method. Users can do any setup they want in configure() > > 2) The first argument of audit is an AbstractRequest. Unfortunately > this type is not part of the public API. But actually I'm not sure > having the full request is really needed here. Maybe just passing the > Apikey would be enough as we already have all the resources from the > auditInfos field. > > 3) The KIP mentions a "LoggingAuditor" default implementation. What is > it doing? Can you add more details about it? > > 4) Can fields of AuditInfo be null? I can see there's a constructor > without an Errors and that sets the error field to None. However, with > the builder pattern, if error is not set it's null. > > 5) Should AuditInfo be an interface? > > On Mon, Sep 14, 2020 at 3:26 PM Viktor Somogyi-Vass > <viktorsomo...@gmail.com> wrote: > > > > Hi everyone, > > > > Changed the interface a little bit to accommodate methods better where > > authorization happens for multiple operations so the implementer of the > > audit interface will receive all authorizations together. > > I'll wait a few more days to allow people to react or give feedback but > if > > there are no objections until then, I'll start a vote. > > > > Viktor > > > > On Tue, Sep 8, 2020 at 9:49 AM Viktor Somogyi-Vass < > viktorsomo...@gmail.com> > > wrote: > > > > > Hi Everyone, > > > > > > I'd like to restart the discussion on this. Since the KIP has been > > > revamped I thought I'd start a new discussion thread. > > > > > > Link: > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-567%3A+Kafka+Cluster+Audit > > > > > > Short summary: > > > - Would like to introduce a new interface similar to the Authorizer > called > > > Auditor as follows: > > > public interface Auditor { > > > audit(Request r, AuthorizableRequestContext c, AclOperation > > > o, Map<ResourcePattern, Boolean> isAllowed, Map<ResourcePattern, > Errors> > > > errors); > > > } > > > - Basically it would pass down the request and the authorization > > > information to the auditor implementation where various kind of > reporting > > > can be done based on the request. > > > - A new config would be added called "auditor" which is similar to the > > > "authorizer" config, but users can pass a list of auditor class names. > > > - The implementation is expected to be low latency similarly to the > > > Authorizer. > > > - A default implementation will be added that logs into a file. > > > > > > I appreciate any feedback on this. > > > > > > Best, > > > Viktor > > > >
