An example I had in mind was the ProduceResponse - the auditor might need access to the new end offset of the partitions. The event-based approach sounds good - new events and fields can be added on-demand. Do we need the same versioning strategy we use with the requests/responses?
Daniel Viktor Somogyi-Vass <viktorsomo...@gmail.com> ezt írta (időpont: 2020. szept. 21., H, 14:08): > Hi Daniel, > > > If the auditor needs access to the details of the action, one could argue > that even the response should be passed down to the auditor. > At this point I don't think we need to include responses into the interface > but if you have a use-case we can consider doing that. > > > Is it feasible to convert the Java requests and responses to public API? > Well I think that in this case we would need to actually transform a lot of > classes and that might be a bit too invasive. Although since the protocol > itself *is* a public API it might make sense to have some kind of Java > representation as a public API as well. > > > If not, do we have another option to access this info in the auditor? > I think one option would be to do what the original KIP-567 was > implemented. Basically we could have an AuditEvent interface that would > contain request specific data. Its obvious drawback is that it has to be > implemented for most of the 40 something protocols but on the upside these > classes shouldn't be complicated. I can try to do a PoC with this to see > how it looks like and whether it solves the problem. To be honest I think > it would be better than publishing the request classes as an API because > here we're restricting access to only what is necessary. > > Thanks, > Viktor > > > > On Fri, Sep 18, 2020 at 8:37 AM Dániel Urbán <urb.dani...@gmail.com> > wrote: > > > Hi, > > > > Thanks for the KIP. > > > > If the auditor needs access to the details of the action, one could argue > > that even the response should be passed down to the auditor. > > Is it feasible to convert the Java requests and responses to public API? > > If not, do we have another option to access this info in the auditor? > > I know that the auditor could just send proper requests through the API > to > > the brokers, but that seems like an awful lot of overhead, and could > > introduce timing issues as well. > > > > Daniel > > > > > > Viktor Somogyi-Vass <viktorsomo...@gmail.com> ezt írta (időpont: 2020. > > szept. 16., Sze, 17:17): > > > > > One more after-thought on your second point (AbstractRequest): the > > reason I > > > introduced it in the first place was that this way implementers can > > access > > > request data. A use case can be if they want to audit a change in > > > configuration or client quotas but not just acknowledge the fact that > > such > > > an event happened but also capture the change itself by peeking into > the > > > request. Sometimes it can be useful especially when people want to > trace > > > back the order of events and what happened when and not just > acknowledge > > > that there was an event of a certain kind. I also recognize that this > > might > > > be a very loose interpretation of auditing as it's not strictly related > > to > > > authorization but rather a way of tracing the admin actions within the > > > cluster. It even could be a different API therefore but because of the > > > variety of the Kafka APIs it's very hard to give a method that fits > all, > > so > > > it's easier to pass down the AbstractRequest and the implementation can > > do > > > the extraction of valuable info. So that's why I added this in the > first > > > place and I'm interested in your thoughts. > > > > > > On Wed, Sep 16, 2020 at 4:41 PM Viktor Somogyi-Vass < > > > viktorsomo...@gmail.com> > > > wrote: > > > > > > > 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 > > > >> > > > > > >> > > > > > > > > > >
