It's not uncommon for people to do large numbers of admin operations per
second. We've seen it many times in production. It's often a bug, but the
system must cope with it without affecting other users of the same Kafka
cluster.
Ismael
On Fri, Sep 6, 2019, 9:32 AM Don Bosco Durai <bo...@apache.org> wrote:
> +1
>
> I might be wrong here, but here are few of my assumptions..
>
> 1. Unlike transactional services, in Kafka most of the users are
> application users, so once the connection is established, then they are
> going to be long running sessions. So the caching is more predictable and
> heavy lifting can be done during initial session setup and cache can be
> frequently updated using background thread. KSQL might have a different
> flow, but I don't have enough datapoints for it.
> 2. For publish/consume authorization calls, providing async option will be
> misleading for Kafka like load
> 3. For admin calls, hopefully no one does hundreds of call in multiple
> thread within few seconds. This would be a bad design.
>
> So I feel, we should be okay with sync calls for now. And consider async
> in the future if needed. At least in Apache Ranger implementation, we would
> be okay using sync operation. I am not sure if any other plugin
> implementation would benefit from async implementation.
>
> Thanks
>
> Bosco
>
> On 9/6/19, 8:45 AM, "Ismael Juma" <ism...@juma.me.uk> wrote:
>
> One more thing: if people have strong opinions that authorize has to be
> sync, we could leave it like that for now. If needed, we can later add
> an
> authorizeAsync with another method returning a Boolean indicating that
> it's
> supported. That is, there is a path to evolve the interface (even if a
> bit
> ugly).
>
> What about the other methods, is there consensus that they should be
> updated to return CompletionStage?
>
> Ismael
>
> On Fri, Sep 6, 2019, 8:32 AM Ismael Juma <ism...@juma.me.uk> wrote:
>
> > I'm on the move, but what Rajini said seems reasonable. I don't think
> > using SSDs solves the issue. They can still hang for seconds when
> they
> > fail. Also, many people may not have local SSDs available (remote
> SSDs like
> > EBS hang for tens of seconds when there are issues).
> >
> > We are currently vulnerable to all of these in the normal read/write
> path,
> > but it seems suboptimal to continue assuming non blocking behavior
> for
> > things that actually do block.
> >
> > Ismael
> >
> > On Fri, Sep 6, 2019, 8:24 AM Rajini Sivaram <rajinisiva...@gmail.com
> >
> > wrote:
> >
> >> Hi Jun,
> >>
> >> For ACLs, the size is also dependent on the number of users. So in
> >> deployments with large number of users where some users are not
> active, an
> >> LRU cache may be useful. Agree that there may be other solutions
> that help
> >> avoid the requirement for an async API even for this case.
> >>
> >> I wasn't expecting our threading model to change very much. Having
> said
> >> that, I haven't figured out how the purgatory can be adapted to work
> >> efficiently for this. I was thinking:
> >> 1) If authorization future is complete when returning from
> authorize()
> >> call, we invoke the method immediately on the request thread
> >> 2) If future is not complete, the request goes into a purgatory
> >> 3) When future completes, we mark the request ready to be run. This
> can be
> >> done on ForkJoinPool.commonPool or an executor. But we don't handle
> the
> >> request on that thread
> >> 4) We handle the request on the request thread after 3).
> >>
> >> I totally agree that this adds complexity to the code. Will wait for
> >> Ismael
> >> to comment on whether he had a different model in mind.
> >>
> >> Thanks,
> >>
> >> Rajini
> >>
> >>
> >> On Thu, Sep 5, 2019 at 10:29 PM Jun Rao <j...@confluent.io> wrote:
> >>
> >> > Hi, Ismael,
> >> >
> >> > Yes, I agree that there are 2 separate discussion points, one on
> the API
> >> > and the other on the implementation.
> >> >
> >> > First on the API, in general, my feeling is that the authorize()
> call is
> >> > expected to be fast to reduce request latency. Adding an async API
> >> could be
> >> > mis-leading since it might give users the impression that it
> would be
> >> > arbitrarily long. In the short term, currently, we cache all
> partition
> >> > level metadata on every broker. The authorization metadata is
> >> proportional
> >> > to the number of topics, which typically is less in size. So it
> we can
> >> > cache all partition level metadata, we should be able to cache all
> >> > authorization metadata. Longer term, if we want to support more
> metadata
> >> > beyond memory, another option is to put all metadata in an SSD
> backed
> >> > key/value store. The lookup time could still be under 1ms, which
> we
> >> could
> >> > potentially absorb in the request threads. So, I am not sure async
> >> > authorize() api is strictly needed even in the long term.
> >> >
> >> > Second on the implementation. I was more concerned about the
> impact on
> >> our
> >> > threading model. If we make authorize() async, some threads have
> to
> >> > complete the future when it's ready. It seems to me that those
> threads
> >> have
> >> > to be inside the Authorizer implementation since only it knows
> when to
> >> > complete a future? If so, it feels weird that all the logic of
> request
> >> > handling post authorize() is now handled by external threads
> inside a
> >> > plugin. It's not clear how we can configure and monitor them, and
> how
> >> > throttling would work. Perhaps we can provide a bit more detail
> on the
> >> new
> >> > threading model.
> >> >
> >> > Thanks,
> >> >
> >> > Jun
> >> >
> >> > On Wed, Sep 4, 2019 at 9:56 PM Ismael Juma <ism...@juma.me.uk>
> wrote:
> >> >
> >> > > Hi Jun,
> >> > >
> >> > > I think it's important to discuss the pluggable API versus the
> calling
> >> > > implementation as two separate points.
> >> > >
> >> > > From an API perspective, are you suggesting that we should tell
> users
> >> > that
> >> > > they cannot block in authorize? Or are you saying that it's OK
> to
> >> block
> >> > on
> >> > > authorize on occasion and the recommendation would be for
> people to
> >> > > increase the number of threads in the request thread pool?
> >> > Architecturally,
> >> > > this feels wrong in my opinion.
> >> > >
> >> > > From the calling implementation perspective, you don't need a
> >> callback.
> >> > You
> >> > > can basically say:
> >> > >
> >> > > authorize(...).map { result =>
> >> > > ...
> >> > > }
> >> > >
> >> > > And then have a common method take that Future and handle it
> >> > synchronously
> >> > > if it's already complete or submit it to the purgatory if not.
> This is
> >> > > similar to how many modern async web libraries work.
> >> > >
> >> > > As Rajini said, this could be done later. The initial
> implementation
> >> > could
> >> > > simply do `toCompletableFuture().get()` to do it synchronously.
> But it
> >> > > would mean that we could add this functionality without
> changing the
> >> > > pluggable interface.
> >> > >
> >> > > Ismael
> >> > >
> >> > > On Wed, Sep 4, 2019 at 5:29 AM Jun Rao <j...@confluent.io>
> wrote:
> >> > >
> >> > > > Hi, Rajini,
> >> > > >
> >> > > > Thanks for the KIP. I was concerned about #4 too. If we
> change the
> >> > > handling
> >> > > > of all requests to use an async authorize() api, will that
> cause the
> >> > code
> >> > > > much harder to understand? There are quite a few callbacks
> already.
> >> I
> >> > am
> >> > > > not sure that we want to introduce more of those. The benefit
> from
> >> > async
> >> > > > authorize() api seems limited.
> >> > > >
> >> > > > Jun
> >> > > >
> >> > > > On Wed, Sep 4, 2019 at 5:38 PM Rajini Sivaram <
> >> rajinisiva...@gmail.com
> >> > >
> >> > > > wrote:
> >> > > >
> >> > > > > Hi Don,
> >> > > > >
> >> > > > > Thanks for your note.
> >> > > > >
> >> > > > > 1) The intention is to avoid blocking in the calling
> thread. We
> >> > already
> >> > > > > have several requests that are put into a purgatory when
> waiting
> >> for
> >> > > > remote
> >> > > > > communication, for example produce request waiting for
> >> replication.
> >> > > Since
> >> > > > > we have a limited number of request threads in the broker,
> we
> >> want to
> >> > > > make
> >> > > > > progress with other requests, while one is waiting on any
> form of
> >> > > remote
> >> > > > > communication.
> >> > > > >
> >> > > > > 2) Async management calls will be useful with the default
> >> authorizer
> >> > > when
> >> > > > > KIP-500 removes ZK and we rely on Kafka instead. Our current
> >> ZK-based
> >> > > > > implementation as well as any custom implementations that
> don't
> >> want
> >> > to
> >> > > > be
> >> > > > > async will just need to return a sync'ed value. So instead
> of
> >> > > returning `
> >> > > > > value`, the code would just return
> >> > > > > `CompletableFuture.completedFuture(value)
> >> > > > > `. So it would be just a single line change in the
> implementation
> >> > with
> >> > > > the
> >> > > > > new API. The caller would treat completedFuture exactly as
> it does
> >> > > today,
> >> > > > > processing the request synchronously without using a
> purgatory.
> >> > > > >
> >> > > > > 3) For implementations that return a completedFuture as
> described
> >> in
> >> > > 2),
> >> > > > > the behaviour would remain exactly the same. No additional
> >> threads or
> >> > > > > purgatory will be used for this case. So there would be no
> >> > performance
> >> > > > > penalty. For implementations that return a future that is
> not
> >> > complete,
> >> > > > we
> >> > > > > prioritise running more requests concurrently. So in a
> deployment
> >> > with
> >> > > a
> >> > > > > large number of clients, we would improve performance by
> allowing
> >> > other
> >> > > > > requests to be processed on the request threads while some
> are
> >> > waiting
> >> > > > for
> >> > > > > authorization metadata.
> >> > > > >
> >> > > > > 4) I was concerned about this too. The goal is to make the
> API
> >> > flexible
> >> > > > > enough to handle large scale deployments in future when
> caching
> >> all
> >> > > > > authorization metadata in each broker is not viable. Using
> an
> >> async
> >> > API
> >> > > > > that returns CompletionStage, the caller has the option to
> handle
> >> the
> >> > > > > result synchronously or asynchronously, so we don't
> necessarily
> >> need
> >> > to
> >> > > > > update the calling code right away. Custom authorizers
> using the
> >> > async
> >> > > > API
> >> > > > > have full control over whether authorization is performed
> in-line
> >> > since
> >> > > > > completedFuture will always be handled synchronously. We do
> need
> >> to
> >> > > > update
> >> > > > > KafkaApis to take advantage of the asynchronous API to
> improve
> >> scale.
> >> > > > Even
> >> > > > > though this is a big change, since we will be doing the
> same for
> >> all
> >> > > > > requests, it shouldn't be too hard to maintain since the
> same
> >> pattern
> >> > > > will
> >> > > > > be used for all requests.
> >> > > > >
> >> > > > > Regards,
> >> > > > >
> >> > > > > Rajini
> >> > > > >
> >> > > > > On Tue, Sep 3, 2019 at 11:48 PM Don Bosco Durai <
> bo...@apache.org
> >> >
> >> > > > wrote:
> >> > > > >
> >> > > > > > Hi Rajini
> >> > > > > >
> >> > > > > > Help me understand this a bit more.
> >> > > > > >
> >> > > > > > 1. For all practical purpose, without authorization you
> can't
> >> go to
> >> > > the
> >> > > > > > next step. The calling code needs to block anyway. So
> should we
> >> > just
> >> > > > let
> >> > > > > > the implementation code do the async part?
> >> > > > > > 2. If you feel management calls need to be async, then we
> should
> >> > > > consider
> >> > > > > > another set of async APIs. I don't feel we should
> complicate it
> >> > > > further (
> >> > > > > > 3. Another concern I have is wrt performance. Kafka has
> been
> >> built
> >> > to
> >> > > > > > handle 1000s per second requests. Not sure whether making
> it
> >> async
> >> > > will
> >> > > > > add
> >> > > > > > any unnecessary overhead.
> >> > > > > > 4. How much complication would this add on the calling
> side?
> >> And is
> >> > > it
> >> > > > > > worth it?
> >> > > > > >
> >> > > > > > Thanks
> >> > > > > >
> >> > > > > > Bosco
> >> > > > > >
> >> > > > > >
> >> > > > > > On 9/3/19, 8:50 AM, "Rajini Sivaram" <
> rajinisiva...@gmail.com>
> >> > > wrote:
> >> > > > > >
> >> > > > > > Hi all,
> >> > > > > >
> >> > > > > > Ismael brought up a point that it will be good to
> make the
> >> > > > Authorizer
> >> > > > > > interface asynchronous to avoid blocking request
> threads
> >> during
> >> > > > > remote
> >> > > > > > operations.
> >> > > > > >
> >> > > > > > 1) Since we want to support different backends for
> >> > authorization
> >> > > > > > metadata,
> >> > > > > > making createAcls() and deleteAcls() asynchronous
> makes
> >> sense
> >> > > since
> >> > > > > > these
> >> > > > > > always involve remote operations. When KIP-500 removes
> >> > ZooKeeper,
> >> > > > we
> >> > > > > > would
> >> > > > > > want to move ACLs to Kafka and async updates will
> avoid
> >> > > unnecessary
> >> > > > > > blocking.
> >> > > > > > 2) For authorize() method, we currently use cached
> ACLs in
> >> the
> >> > > > > built-in
> >> > > > > > authorizers, so synchronous authorise operations work
> well
> >> now.
> >> > > But
> >> > > > > > async
> >> > > > > > authorize() would support this scenario as well as
> >> authorizers
> >> > in
> >> > > > > large
> >> > > > > > organisations where an LRU cache would enable a
> smaller
> >> cache
> >> > > even
> >> > > > > > when the
> >> > > > > > backend holds a large amount of ACLs for infrequently
> used
> >> > > > resources
> >> > > > > or
> >> > > > > > users who don't use the system frequently.
> >> > > > > >
> >> > > > > > For both cases, the built-in authorizer will continue
> to be
> >> > > > > > synchronous,
> >> > > > > > using CompletableFuture.completedFuture() to return
> the
> >> actual
> >> > > > > result.
> >> > > > > > But
> >> > > > > > async API will make custom authorizer implementations
> more
> >> > > > flexible.
> >> > > > > I
> >> > > > > > would like to know if there are any concerns with
> these
> >> changes
> >> > > > > before
> >> > > > > > updating the KIP.
> >> > > > > >
> >> > > > > > *Proposed API:*
> >> > > > > > public interface Authorizer extends Configurable,
> Closeable
> >> {
> >> > > > > >
> >> > > > > > Map<Endpoint, CompletionStage<Void>>
> >> > > start(AuthorizerServerInfo
> >> > > > > > serverInfo);
> >> > > > > > List<CompletionStage<AuthorizationResult>>
> >> > > > > > authorize(AuthorizableRequestContext requestContext,
> >> > List<Action>
> >> > > > > > actions);
> >> > > > > > List<CompletionStage<AclCreateResult>>
> >> > > > > > createAcls(AuthorizableRequestContext requestContext,
> >> > > > > List<AclBinding>
> >> > > > > > aclBindings);
> >> > > > > > List<CompletionStage<AclDeleteResult>>
> >> > > > > > deleteAcls(AuthorizableRequestContext requestContext,
> >> > > > > > List<AclBindingFilter> aclBindingFilters);
> >> > > > > > CompletionStage<Collection<AclBinding>>
> >> > acls(AclBindingFilter
> >> > > > > > filter);
> >> > > > > > }
> >> > > > > >
> >> > > > > >
> >> > > > > > Thank you,
> >> > > > > >
> >> > > > > > Rajini
> >> > > > > >
> >> > > > > > On Sun, Aug 18, 2019 at 6:25 PM Don Bosco Durai <
> >> > > bo...@apache.org>
> >> > > > > > wrote:
> >> > > > > >
> >> > > > > > > Hi Rajini
> >> > > > > > >
> >> > > > > > > Thanks for clarifying. I am good for now.
> >> > > > > > >
> >> > > > > > > Regards
> >> > > > > > >
> >> > > > > > > Bosco
> >> > > > > > >
> >> > > > > > >
> >> > > > > > > On 8/16/19, 11:30 AM, "Rajini Sivaram" <
> >> > > rajinisiva...@gmail.com
> >> > > > >
> >> > > > > > wrote:
> >> > > > > > >
> >> > > > > > > Hi Don,
> >> > > > > > >
> >> > > > > > > That should be fine. I guess Ranger loads
> policies
> >> from
> >> > the
> >> > > > > > database
> >> > > > > > > synchronously when the authorizer is configured,
> >> similar
> >> > to
> >> > > > > > > SimpleAclAuthorizer loading from ZooKeeper.
> Ranger can
> >> > > > continue
> >> > > > > > to load
> >> > > > > > > synchronously from `configure()` or `start()`
> and
> >> return
> >> > an
> >> > > > > > empty map
> >> > > > > > > from
> >> > > > > > > `start()`. That would retain the existing
> behaviour..
> >> > When
> >> > > > the
> >> > > > > > same
> >> > > > > > > database stores policies for all listeners and
> the
> >> > policies
> >> > > > are
> >> > > > > > not
> >> > > > > > > stored
> >> > > > > > > in Kafka, there is no value in making the load
> >> > > asynchronous.
> >> > > > > > >
> >> > > > > > > Regards,
> >> > > > > > >
> >> > > > > > > Rajini
> >> > > > > > >
> >> > > > > > >
> >> > > > > > > On Fri, Aug 16, 2019 at 6:43 PM Don Bosco Durai
> <
> >> > > > > > bo...@apache.org>
> >> > > > > > > wrote:
> >> > > > > > >
> >> > > > > > > > Hi Rajini
> >> > > > > > > >
> >> > > > > > > > Assuming this doesn't affect custom plugins,
> I don't
> >> > have
> >> > > > any
> >> > > > > > > concerns
> >> > > > > > > > regarding this.
> >> > > > > > > >
> >> > > > > > > > I do have one question regarding:
> >> > > > > > > >
> >> > > > > > > > "For authorizers that don’t store metadata in
> >> > ZooKeeper,
> >> > > > > > ensure that
> >> > > > > > > > authorizer metadata for each listener is
> available
> >> > before
> >> > > > > > starting
> >> > > > > > > up the
> >> > > > > > > > listener. This enables different authorization
> >> metadata
> >> > > > > stores
> >> > > > > > for
> >> > > > > > > > different listeners."
> >> > > > > > > >
> >> > > > > > > > Since Ranger uses its own database for storing
> >> > policies,
> >> > > do
> >> > > > > you
> >> > > > > > > anticipate
> >> > > > > > > > any issues?
> >> > > > > > > >
> >> > > > > > > > Thanks
> >> > > > > > > >
> >> > > > > > > > Bosco
> >> > > > > > > >
> >> > > > > > > >
> >> > > > > > > > On 8/16/19, 6:49 AM, "Rajini Sivaram" <
> >> > > > > > rajinisiva...@gmail.com>
> >> > > > > > > wrote:
> >> > > > > > > >
> >> > > > > > > > Hi all,
> >> > > > > > > >
> >> > > > > > > > I made another change to the KIP. The KIP
> was
> >> > > > originally
> >> > > > > > > proposing to
> >> > > > > > > > extend SimpleAclAuthorizer to also
> implement the
> >> > new
> >> > > > API
> >> > > > > > (in
> >> > > > > > > addition
> >> > > > > > > > to
> >> > > > > > > > the existing API). But since we use the
> new API
> >> > when
> >> > > > > > available,
> >> > > > > > > this
> >> > > > > > > > can
> >> > > > > > > > break custom authorizers that extend this
> class
> >> and
> >> > > > > > override
> >> > > > > > > specific
> >> > > > > > > > methods of the old API. To avoid breaking
> any
> >> > > existing
> >> > > > > > custom
> >> > > > > > > > implementations that extend this class,
> >> > particularly
> >> > > > > > because it
> >> > > > > > > is in
> >> > > > > > > > the
> >> > > > > > > > public package kafka.security.auth, the
> KIP now
> >> > > > proposes
> >> > > > > to
> >> > > > > > > retain the
> >> > > > > > > > old
> >> > > > > > > > authorizer as-is. SimpleAclAuthorizer
> will
> >> > continue
> >> > > to
> >> > > > > > > implement the
> >> > > > > > > > old
> >> > > > > > > > API, but will be deprecated. A new
> authorizer
> >> > > > > > implementation
> >> > > > > > > > kafka.security.authorizer.AclAuthorizer
> will be
> >> > added
> >> > > > for
> >> > > > > > the
> >> > > > > > > new API
> >> > > > > > > > (this
> >> > > > > > > > will not be in the public package).
> >> > > > > > > >
> >> > > > > > > > Please let me know if you have any
> concerns.
> >> > > > > > > >
> >> > > > > > > > Regards,
> >> > > > > > > >
> >> > > > > > > > Rajini
> >> > > > > > > >
> >> > > > > > > >
> >> > > > > > > > On Fri, Aug 16, 2019 at 8:48 AM Rajini
> Sivaram <
> >> > > > > > > > rajinisiva...@gmail.com>
> >> > > > > > > > wrote:
> >> > > > > > > >
> >> > > > > > > > > Thanks Colin.
> >> > > > > > > > >
> >> > > > > > > > > If there are no other concerns, I will
> start
> >> vote
> >> > > > later
> >> > > > > > today.
> >> > > > > > > Many
> >> > > > > > > > thanks
> >> > > > > > > > > to every one for the feedback.
> >> > > > > > > > >
> >> > > > > > > > > Regards,
> >> > > > > > > > >
> >> > > > > > > > > Rajini
> >> > > > > > > > >
> >> > > > > > > > >
> >> > > > > > > > > On Thu, Aug 15, 2019 at 11:57 PM Colin
> McCabe
> >> <
> >> > > > > > > cmcc...@apache.org>
> >> > > > > > > > wrote:
> >> > > > > > > > >
> >> > > > > > > > >> Thanks, Rajini. It looks good to me.
> >> > > > > > > > >>
> >> > > > > > > > >> best,
> >> > > > > > > > >> Colin
> >> > > > > > > > >>
> >> > > > > > > > >>
> >> > > > > > > > >> On Thu, Aug 15, 2019, at 11:37, Rajini
> >> Sivaram
> >> > > > wrote:
> >> > > > > > > > >> > Hi Colin,
> >> > > > > > > > >> >
> >> > > > > > > > >> > Thanks for the review. I have
> updated the
> >> KIP
> >> > to
> >> > > > > move
> >> > > > > > the
> >> > > > > > > > interfaces for
> >> > > > > > > > >> > request context and server info to
> the
> >> > > authorizer
> >> > > > > > package.
> >> > > > > > > These
> >> > > > > > > > are now
> >> > > > > > > > >> > called AuthorizableRequestContext and
> >> > > > > > AuthorizerServerInfo.
> >> > > > > > > > Endpoint is
> >> > > > > > > > >> now
> >> > > > > > > > >> > a class in org.apache.kafka.common
> to make
> >> it
> >> > > > > reusable
> >> > > > > > > since we
> >> > > > > > > > already
> >> > > > > > > > >> > have multiple implementations of it.
> I have
> >> > > > removed
> >> > > > > > > requestName
> >> > > > > > > > from the
> >> > > > > > > > >> > request context interface since
> authorizers
> >> > can
> >> > > > > > distinguish
> >> > > > > > > > follower
> >> > > > > > > > >> fetch
> >> > > > > > > > >> > and consumer fetch from the
> operation being
> >> > > > > > authorized. So
> >> > > > > > > 16-bit
> >> > > > > > > > >> request
> >> > > > > > > > >> > type should be sufficient for audit
> >> logging.
> >> > > Also
> >> > > > > > replaced
> >> > > > > > > > AuditFlag
> >> > > > > > > > >> with
> >> > > > > > > > >> > two booleans as you suggested.
> >> > > > > > > > >> >
> >> > > > > > > > >> > Can you take another look and see if
> the
> >> KIP
> >> > is
> >> > > > > ready
> >> > > > > > for
> >> > > > > > > voting?
> >> > > > > > > > >> >
> >> > > > > > > > >> > Thanks for all your help!
> >> > > > > > > > >> >
> >> > > > > > > > >> > Regards,
> >> > > > > > > > >> >
> >> > > > > > > > >> > Rajini
> >> > > > > > > > >> >
> >> > > > > > > > >> > On Wed, Aug 14, 2019 at 8:59 PM Colin
> >> McCabe <
> >> > > > > > > cmcc...@apache.org>
> >> > > > > > > > >> wrote:
> >> > > > > > > > >> >
> >> > > > > > > > >> > > Hi Rajini,
> >> > > > > > > > >> > >
> >> > > > > > > > >> > > I think it would be good to rename
> >> > > > > > KafkaRequestContext to
> >> > > > > > > > something
> >> > > > > > > > >> like
> >> > > > > > > > >> > > AuthorizableRequestContext, and
> put it in
> >> > the
> >> > > > > > > > >> > > org.apache.kafka.server.authorizer
> >> > namespace.
> >> > > > If
> >> > > > > > we put
> >> > > > > > > it in
> >> > > > > > > > the
> >> > > > > > > > >> > > org.apache.kafka.common namespace,
> then
> >> it's
> >> > > not
> >> > > > > > really
> >> > > > > > > clear
> >> > > > > > > > that
> >> > > > > > > > >> it's
> >> > > > > > > > >> > > part of the Authorizer API. Since
> this
> >> > class
> >> > > is
> >> > > > > > purely an
> >> > > > > > > > interface,
> >> > > > > > > > >> with
> >> > > > > > > > >> > > no concrete implementation of
> anything,
> >> > > there's
> >> > > > > > nothing
> >> > > > > > > common
> >> > > > > > > > to
> >> > > > > > > > >> really
> >> > > > > > > > >> > > reuse in any case. We definitely
> don't
> >> want
> >> > > > > > someone to
> >> > > > > > > > accidentally
> >> > > > > > > > >> add or
> >> > > > > > > > >> > > remove methods because they think
> this is
> >> > just
> >> > > > > > another
> >> > > > > > > internal
> >> > > > > > > > class
> >> > > > > > > > >> used
> >> > > > > > > > >> > > for requests.
> >> > > > > > > > >> > >
> >> > > > > > > > >> > > The BrokerInfo class is a nice
> >> improvement.
> >> > > It
> >> > > > > > looks
> >> > > > > > > like it
> >> > > > > > > > will be
> >> > > > > > > > >> > > useful for passing in information
> about
> >> the
> >> > > > > context
> >> > > > > > we're
> >> > > > > > > > running
> >> > > > > > > > >> in. It
> >> > > > > > > > >> > > would be nice to call this class
> >> ServerInfo
> >> > > > rather
> >> > > > > > than
> >> > > > > > > > BrokerInfo,
> >> > > > > > > > >> since
> >> > > > > > > > >> > > we will want to run the authorizer
> on
> >> > > > controllers
> >> > > > > > as well
> >> > > > > > > as on
> >> > > > > > > > >> brokers,
> >> > > > > > > > >> > > and the controller may run as a
> separate
> >> > > process
> >> > > > > > post
> >> > > > > > > KIP-500.
> >> > > > > > > > I also
> >> > > > > > > > >> > > think that this class should be in
> the
> >> > > > > > > > >> org.apache.kafka.server.authorizer
> >> > > > > > > > >> > > namespace. Again, it is an
> interface,
> >> not a
> >> > > > > > concrete
> >> > > > > > > > implementation,
> >> > > > > > > > >> and
> >> > > > > > > > >> > > it's an interface that is very
> >> specifically
> >> > > for
> >> > > > > the
> >> > > > > > > authorizer.
> >> > > > > > > > >> > >
> >> > > > > > > > >> > > I agree that we probably don't have
> >> enough
> >> > > > > > information
> >> > > > > > > > preserved for
> >> > > > > > > > >> > > requests currently to always know
> what
> >> > entity
> >> > > > made
> >> > > > > > them.
> >> > > > > > > So we
> >> > > > > > > > can
> >> > > > > > > > >> leave
> >> > > > > > > > >> > > that out for now (except in the
> special
> >> case
> >> > > of
> >> > > > > > Fetch).
> >> > > > > > > > Perhaps we
> >> > > > > > > > >> can add
> >> > > > > > > > >> > > this later if it's needed.
> >> > > > > > > > >> > >
> >> > > > > > > > >> > > I understand the intention behind
> >> > > > > AuthorizationMode
> >> > > > > > > (which is
> >> > > > > > > > now
> >> > > > > > > > >> called
> >> > > > > > > > >> > > AuditFlag in the latest
> revision). But
> >> it
> >> > > still
> >> > > > > > feels
> >> > > > > > > > complex. What
> >> > > > > > > > >> if we
> >> > > > > > > > >> > > just had two booleans in Action:
> >> > logSuccesses
> >> > > > and
> >> > > > > > > logFailures?
> >> > > > > > > > That
> >> > > > > > > > >> seems
> >> > > > > > > > >> > > to cover all the cases here.
> >> > > > MANDATORY_AUTHORIZE
> >> > > > > =
> >> > > > > > true,
> >> > > > > > > true.
> >> > > > > > > > >> > > OPTIONAL_AUTHORIZE = true, false.
> >> FILTER =
> >> > > > true,
> >> > > > > > false.
> >> > > > > > > > >> LIST_AUTHORIZED =
> >> > > > > > > > >> > > false, false. Would there be
> anything
> >> lost
> >> > > > versus
> >> > > > > > having
> >> > > > > > > the
> >> > > > > > > > enum?
> >> > > > > > > > >> > >
> >> > > > > > > > >> > > best,
> >> > > > > > > > >> > > Colin
> >> > > > > > > > >> > >
> >> > > > > > > > >> > >
> >> > > > > > > > >> > > On Wed, Aug 14, 2019, at 06:29,
> Mickael
> >> > Maison
> >> > > > > > wrote:
> >> > > > > > > > >> > > > Hi Rajini,
> >> > > > > > > > >> > > >
> >> > > > > > > > >> > > > Thanks for the KIP!
> >> > > > > > > > >> > > > I really like that authorize()
> will be
> >> > able
> >> > > to
> >> > > > > > take a
> >> > > > > > > batch of
> >> > > > > > > > >> > > > requests, this will speed up many
> >> > > > > implementations!
> >> > > > > > > > >> > > >
> >> > > > > > > > >> > > > On Tue, Aug 13, 2019 at 5:57 PM
> Rajini
> >> > > > Sivaram <
> >> > > > > > > > >> rajinisiva...@gmail.com>
> >> > > > > > > > >> > > wrote:
> >> > > > > > > > >> > > > >
> >> > > > > > > > >> > > > > Thanks David! I have fixed the
> typo.
> >> > > > > > > > >> > > > >
> >> > > > > > > > >> > > > > Also made a couple of changes
> to make
> >> > the
> >> > > > > > context
> >> > > > > > > > interfaces more
> >> > > > > > > > >> > > generic.
> >> > > > > > > > >> > > > > KafkaRequestContext now
> returns the
> >> > 16-bit
> >> > > > API
> >> > > > > > key as
> >> > > > > > > Colin
> >> > > > > > > > >> suggested
> >> > > > > > > > >> > > as
> >> > > > > > > > >> > > > > well as the friendly name used
> in
> >> > metrics
> >> > > > > which
> >> > > > > > are
> >> > > > > > > useful
> >> > > > > > > > in
> >> > > > > > > > >> audit
> >> > > > > > > > >> > > logs.
> >> > > > > > > > >> > > > > `Authorizer#start` is now
> provided a
> >> > > generic
> >> > > > > > > `BrokerInfo`
> >> > > > > > > > >> interface
> >> > > > > > > > >> > > that
> >> > > > > > > > >> > > > > gives cluster id, broker id and
> >> endpoint
> >> > > > > > information.
> >> > > > > > > The
> >> > > > > > > > generic
> >> > > > > > > > >> > > interface
> >> > > > > > > > >> > > > > can potentially be used in
> other
> >> broker
> >> > > > > plugins
> >> > > > > > in
> >> > > > > > > future
> >> > > > > > > > and
> >> > > > > > > > >> provides
> >> > > > > > > > >> > > > > dynamically generated configs
> like
> >> > broker
> >> > > id
> >> > > > > > and ports
> >> > > > > > > > which are
> >> > > > > > > > >> > > currently
> >> > > > > > > > >> > > > > not available to plugins
> unless these
> >> > > > configs
> >> > > > > > are
> >> > > > > > > statically
> >> > > > > > > > >> > > configured.
> >> > > > > > > > >> > > > > Please let me know if there
> are any
> >> > > > concerns.
> >> > > > > > > > >> > > > >
> >> > > > > > > > >> > > > > Regards,
> >> > > > > > > > >> > > > >
> >> > > > > > > > >> > > > > Rajini
> >> > > > > > > > >> > > > >
> >> > > > > > > > >> > > > > On Tue, Aug 13, 2019 at 4:30
> PM David
> >> > > Jacot
> >> > > > <
> >> > > > > > > > dja...@confluent.io>
> >> > > > > > > > >> > > wrote:
> >> > > > > > > > >> > > > >
> >> > > > > > > > >> > > > > > Hi Rajini,
> >> > > > > > > > >> > > > > >
> >> > > > > > > > >> > > > > > Thank you for the update! It
> looks
> >> > good
> >> > > to
> >> > > > > me.
> >> > > > > > > There is a
> >> > > > > > > > typo
> >> > > > > > > > >> in the
> >> > > > > > > > >> > > > > > `AuditFlag` enum:
> >> > `MANDATORY_AUTHOEIZE`
> >> > > ->
> >> > > > > > > > >> `MANDATORY_AUTHORIZE`.
> >> > > > > > > > >> > > > > >
> >> > > > > > > > >> > > > > > Regards,
> >> > > > > > > > >> > > > > > David
> >> > > > > > > > >> > > > > >
> >> > > > > > > > >> > > > > > On Mon, Aug 12, 2019 at 2:54
> PM
> >> Rajini
> >> > > > > > Sivaram <
> >> > > > > > > > >> > > rajinisiva...@gmail.com>
> >> > > > > > > > >> > > > > > wrote:
> >> > > > > > > > >> > > > > >
> >> > > > > > > > >> > > > > > > Hi David,
> >> > > > > > > > >> > > > > > >
> >> > > > > > > > >> > > > > > > Thanks for reviewing the
> KIP!
> >> Since
> >> > > > > > questions
> >> > > > > > > about
> >> > > > > > > > >> `authorization
> >> > > > > > > > >> > > mode`
> >> > > > > > > > >> > > > > > > and `count` have come up
> multiple
> >> > > > times, I
> >> > > > > > have
> >> > > > > > > renamed
> >> > > > > > > > both.
> >> > > > > > > > >> > > > > > >
> >> > > > > > > > >> > > > > > > 1) Renamed `count` to
> >> > > > > > `resourceReferenceCount`.
> >> > > > > > > It is
> >> > > > > > > > the
> >> > > > > > > > >> number
> >> > > > > > > > >> > > of times
> >> > > > > > > > >> > > > > > > the resource being
> authorized is
> >> > > > > referenced
> >> > > > > > > within the
> >> > > > > > > > >> request.
> >> > > > > > > > >> > > > > > >
> >> > > > > > > > >> > > > > > > 2) Renamed
> `AuthorizationMode` to
> >> > > > > > `AuditFlag`. It
> >> > > > > > > is
> >> > > > > > > > provided
> >> > > > > > > > >> to
> >> > > > > > > > >> > > improve
> >> > > > > > > > >> > > > > > > audit logging in the
> authorizer.
> >> The
> >> > > > enum
> >> > > > > > values
> >> > > > > > > have
> >> > > > > > > > javadoc
> >> > > > > > > > >> which
> >> > > > > > > > >> > > > > > > indicate how the
> authorization
> >> > result
> >> > > is
> >> > > > > > used in
> >> > > > > > > each
> >> > > > > > > > of the
> >> > > > > > > > >> modes
> >> > > > > > > > >> > > to
> >> > > > > > > > >> > > > > > > enable authorizers to log
> audit
> >> > > messages
> >> > > > > at
> >> > > > > > the
> >> > > > > > > right
> >> > > > > > > > severity
> >> > > > > > > > >> > > level.
> >> > > > > > > > >> > > > > > >
> >> > > > > > > > >> > > > > > > Regards,
> >> > > > > > > > >> > > > > > >
> >> > > > > > > > >> > > > > > > Rajini
> >> > > > > > > > >> > > > > > >
> >> > > > > > > > >> > > > > > > On Mon, Aug 12, 2019 at
> 5:54 PM
> >> > David
> >> > > > > Jacot
> >> > > > > > <
> >> > > > > > > > >> dja...@confluent.io>
> >> > > > > > > > >> > > wrote:
> >> > > > > > > > >> > > > > > >
> >> > > > > > > > >> > > > > > > > Hi Rajini,
> >> > > > > > > > >> > > > > > > >
> >> > > > > > > > >> > > > > > > > Thank you for the KIP.
> >> Overall, it
> >> > > > looks
> >> > > > > > good
> >> > > > > > > to me.
> >> > > > > > > > I have
> >> > > > > > > > >> few
> >> > > > > > > > >> > > > > > > > questions/suggestions:
> >> > > > > > > > >> > > > > > > >
> >> > > > > > > > >> > > > > > > > 1. It is hard to grasp
> what
> >> > > > > > `Action#count` is
> >> > > > > > > for. I
> >> > > > > > > > guess I
> >> > > > > > > > >> > > understand
> >> > > > > > > > >> > > > > > > > where you want to go
> with it
> >> but
> >> > it
> >> > > > took
> >> > > > > > me a
> >> > > > > > > while to
> >> > > > > > > > >> figure it
> >> > > > > > > > >> > > out.
> >> > > > > > > > >> > > > > > > > Perhaps, we could come
> up with
> >> a
> >> > > > better
> >> > > > > > name
> >> > > > > > > than
> >> > > > > > > > `count`?
> >> > > > > > > > >> > > > > > > >
> >> > > > > > > > >> > > > > > > > 2. I had a hard time
> trying to
> >> > > > > understand
> >> > > > > > the
> >> > > > > > > > >> `AuthorizationMode`
> >> > > > > > > > >> > > > > > > concept,
> >> > > > > > > > >> > > > > > > > especially wrt. the
> OPTIONAL
> >> one.
> >> > My
> >> > > > > > > understanding is
> >> > > > > > > > that
> >> > > > > > > > >> an
> >> > > > > > > > >> > > ACL is
> >> > > > > > > > >> > > > > > > either
> >> > > > > > > > >> > > > > > > > defined or not. Could you
> >> > elaborate
> >> > > a
> >> > > > > bit
> >> > > > > > more
> >> > > > > > > on
> >> > > > > > > > that?
> >> > > > > > > > >> > > > > > > >
> >> > > > > > > > >> > > > > > > > Thanks,
> >> > > > > > > > >> > > > > > > > David
> >> > > > > > > > >> > > > > > > >
> >> > > > > > > > >> > > > > > > > On Fri, Aug 9, 2019 at
> 10:26 PM
> >> > Don
> >> > > > > Bosco
> >> > > > > > Durai
> >> > > > > > > <
> >> > > > > > > > >> > > bo...@apache.org>
> >> > > > > > > > >> > > > > > > wrote:
> >> > > > > > > > >> > > > > > > >
> >> > > > > > > > >> > > > > > > > > Hi Rajini
> >> > > > > > > > >> > > > > > > > >
> >> > > > > > > > >> > > > > > > > > 3.2 - This makes sense.
> >> Thanks
> >> > for
> >> > > > > > clarifying.
> >> > > > > > > > >> > > > > > > > >
> >> > > > > > > > >> > > > > > > > > Rest looks fine. Once
> the
> >> > > > > > implementations are
> >> > > > > > > done,
> >> > > > > > > > it
> >> > > > > > > > >> will be
> >> > > > > > > > >> > > more
> >> > > > > > > > >> > > > > > > clear
> >> > > > > > > > >> > > > > > > > > on the different values
> >> > > RequestType
> >> > > > > and
> >> > > > > > Mode.
> >> > > > > > > > >> > > > > > > > >
> >> > > > > > > > >> > > > > > > > > Thanks
> >> > > > > > > > >> > > > > > > > >
> >> > > > > > > > >> > > > > > > > > Bosco
> >> > > > > > > > >> > > > > > > > >
> >> > > > > > > > >> > > > > > > > >
> >> > > > > > > > >> > > > > > > > > On 8/9/19, 5:08 AM,
> "Rajini
> >> > > > Sivaram"
> >> > > > > <
> >> > > > > > > > >> rajinisiva...@gmail.com
> >> > > > > > > > >> > > >
> >> > > > > > > > >> > > > > > wrote:
> >> > > > > > > > >> > > > > > > > >
> >> > > > > > > > >> > > > > > > > > Hi Don,
> >> > > > > > > > >> > > > > > > > >
> >> > > > > > > > >> > > > > > > > > Thanks for the
> >> suggestions.
> >> > A
> >> > > > few
> >> > > > > > > responses
> >> > > > > > > > below:
> >> > > > > > > > >> > > > > > > > >
> >> > > > > > > > >> > > > > > > > > 3.1 Can rename and
> >> improve
> >> > > docs
> >> > > > if
> >> > > > > > we keep
> >> > > > > > > > this. Let's
> >> > > > > > > > >> > > finish the
> >> > > > > > > > >> > > > > > > > > discussion on
> Colin's
> >> > > > suggestions
> >> > > > > > > regarding this
> >> > > > > > > > >> first.
> >> > > > > > > > >> > > > > > > > > 3.2 No, I was
> thinking of
> >> > some
> >> > > > > > requests
> >> > > > > > > that
> >> > > > > > > > have an
> >> > > > > > > > >> old
> >> > > > > > > > >> > > way of
> >> > > > > > > > >> > > > > > > > > authorizing
> >> > > > > > > > >> > > > > > > > > and a new way
> where we
> >> have
> >> > > > > > retained the
> >> > > > > > > old
> >> > > > > > > > way for
> >> > > > > > > > >> > > backward
> >> > > > > > > > >> > > > > > > > > compatibility. One
> >> example
> >> > is
> >> > > > > > > Cluster:Create
> >> > > > > > > > >> permission to
> >> > > > > > > > >> > > create
> >> > > > > > > > >> > > > > > > > > topics.
> >> > > > > > > > >> > > > > > > > > We have replaced
> this
> >> with
> >> > > > > > fine-grained
> >> > > > > > > topic
> >> > > > > > > > create
> >> > > > > > > > >> > > access using
> >> > > >
