@Colin, Thank you for the feedback!
I have updated the KIP, added the explanation of why we use API version
rather than Kafka version.
I will start a vote for this KIP
On Fri, Mar 29, 2019 at 9:47 AM Colin McCabe <cmcc...@apache.org> wrote:
> Hi Ying,
>
> That's a fair point. Maybe using API keys directly is reasonable here.
>
> One thing that's probably worth calling out is that if we make the name
> part of the configuration, we can't rename APIs in the future. That's
> probably OK as long as it's documented.
>
> best,
> Colin
>
> On Thu, Mar 28, 2019, at 17:36, Ying Zheng wrote:
> > @Colin McCabe <cmcc...@confluent.io>
> >
> > I did think about that option. Yes, for most users, it's much easier to
> > understand Kafka version, rather than API version. However, the existing
> > Kafka clients only report API versions in the Kafka requests. So, the
> > brokers can only block clients by API versions rather than the real Kafka
> > versions. This can be very confusing for the users, if an API did not
> > change for a specified Kafka version.
> >
> > For example, a user sets the min Kafka version of produce request to
> Kafka
> > 1.1. She would expect the broker will reject Kafka 1.0 producers.
> However,
> > both Kafka 1.1 and Kafka 1.0 are using api version 5. The broker can't
> > distinguish the 2 version. So, Kafka 1.0 producers are still allowed.
> >
> > I think we can say this configuration is only for "advanced users". The
> > user has to know the concept of "api version", and know the function of
> > each API, to be able to use this feature. (Similarly, it's easier for the
> > users to say: "I want to block old consumers, or old admin clients.". But
> > there is no clear definition of which set of APIs are "consumer APIs".
> So,
> > we still have to use API names, rather than "client roles")
> >
> >
> >
> > On Wed, Mar 27, 2019 at 5:32 PM Colin McCabe <cmcc...@apache.org> wrote:
> >
> > > Thanks, Ying Zheng. Looks good overall.
> > >
> > > One question is, should the version be specified as a Kafka version
> rather
> > > than as a RPC API version? I don't think most users are aware of RPC
> > > versions, but something like "min kafka version" would be easier to
> > > understand. That is how we handle the minimum inter-broker protocol
> > > version and the minimum on-disk format version, after all.
> > >
> > > best,
> > > Colin
> > >
> > > On Tue, Mar 26, 2019, at 17:52, Ying Zheng wrote:
> > > > I have rewritten the KIP. The new proposal is adding a new
> configuration
> > > > min.api.version in Kafka broker.
> > > >
> > > > Please review the new KIP. Thank you!
> > > >
> > > > On Fri, Mar 1, 2019 at 11:06 AM Colin McCabe <cmcc...@apache.org>
> wrote:
> > > >
> > > > > On Wed, Feb 27, 2019, at 15:53, Harsha wrote:
> > > > > > HI Colin,
> > > > > > Overlooked the IDEMPOTENT_WRITE ACL. This along with
> > > > > > client.min.version should solve the cases proposed in the KIP.
> > > > > > Can we turn this KIP into adding min.client.version config to
> broker
> > > > > > and it could be part of the dynamic config .
> > > > >
> > > > > +1, sounds like a good idea.
> > > > >
> > > > > Colin
> > > > >
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Harsha
> > > > > >
> > > > > > On Wed, Feb 27, 2019, at 12:17 PM, Colin McCabe wrote:
> > > > > > > On Tue, Feb 26, 2019, at 16:33, Harsha wrote:
> > > > > > > > Hi Colin,
> > > > > > > >
> > > > > > > > "> I think Ismael and Gwen here bring up a good point. The
> > > version
> > > > > of the
> > > > > > > > > request is a technical detail that isn't really related to
> > > > > > > > > authorization. There are a lot of other technical details
> like
> > > > > this
> > > > > > > > > like the size of the request, the protocol it came in on,
> etc.
> > > > > None of
> > > > > > > > > them are passed to the authorizer-- they all have
> configuration
> > > > > knobs
> > > > > > > > > to control how we handle them. If we add this technical
> > > detail,
> > > > > > > > > logically we'll have to start adding all the others, and
> the
> > > > > authorizer
> > > > > > > > > API will get really bloated. It's better to keep it
> focused on
> > > > > > > > > authorization, I think."
> > > > > > > >
> > > > > > > > probably my previous email is not clear but I am agreeing
> with
> > > > > Gwen's point.
> > > > > > > > I am not in favor of extending authorizer to support this.
> > > > > > > >
> > > > > > > >
> > > > > > > > "> Another thing to consider is that if we add a new broker
> > > > > configuration
> > > > > > > > > that lets us set a minimum client version which is allowed,
> > > that
> > > > > could
> > > > > > > > > be useful to other users as well. On the other hand, most
> > > users
> > > > > are
> > > > > > > > > not likely to write a custom authorizer to try to take
> > > advantage
> > > > > of
> > > > > > > > > version information being passed to the authorizer. So, I
> > > think
> > > > > using> a configuration is clearly the better way to go here.
> Perhaps
> > > it
> > > > > can
> > > > > > > > > be a KIP-226 dynamic configuration to make this easier to
> > > deploy?"
> > > > > > > >
> > > > > > > > Although minimum client version might help to a certain
> extent
> > > there
> > > > > > > > are other cases where we want users to not start using
> > > transactions
> > > > > for
> > > > > > > > example. My proposal in the previous thread was to introduce
> > > another
> > > > > > > > module/interface, let's say
> > > > > > > > "SupportedAPIs" which will take in dynamic configuration to
> check
> > > > > which
> > > > > > > > APIs are allowed.
> > > > > > > > It can throw UnsupportedException just like we are throwing
> > > > > > > > Authorization Exception.
> > > > > > >
> > > > > > > Hi Harsha,
> > > > > > >
> > > > > > > We can already prevent people from using transactions using
> ACLs,
> > > > > > > right? That's what the IDEMPOTENT_WRITE ACL was added for.
> > > > > > >
> > > > > > > In general, I think users should be able to think of ACLs in
> terms
> > > of
> > > > > > > "what can I do" rather than "how is it implemented." For
> example,
> > > > > > > maybe some day we will replace FetchRequest with
> GetStuffRequest.
> > > But
> > > > > > > users who have READ permission on a topic shouldn't have to
> change
> > > > > > > anything. So I think the Authorizer interface should not be
> aware
> > > of
> > > > > > > individual RPC types or message versions.
> > > > > > >
> > > > > > > best,
> > > > > > > Colin
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Harsha
> > > > > > > >
> > > > > > > >
> > > > > > > > n Tue, Feb 26, 2019, at 10:04 AM, Colin McCabe wrote:
> > > > > > > > > Hi Harsha,
> > > > > > > > >
> > > > > > > > > I think Ismael and Gwen here bring up a good point. The
> > > version
> > > > > of the
> > > > > > > > > request is a technical detail that isn't really related to
> > > > > > > > > authorization. There are a lot of other technical details
> like
> > > > > this
> > > > > > > > > like the size of the request, the protocol it came in on,
> etc.
> > > > > None of
> > > > > > > > > them are passed to the authorizer-- they all have
> configuration
> > > > > knobs
> > > > > > > > > to control how we handle them. If we add this technical
> > > detail,
> > > > > > > > > logically we'll have to start adding all the others, and
> the
> > > > > authorizer
> > > > > > > > > API will get really bloated. It's better to keep it
> focused on
> > > > > > > > > authorization, I think.
> > > > > > > > >
> > > > > > > > > Another thing to consider is that if we add a new broker
> > > > > configuration
> > > > > > > > > that lets us set a minimum client version which is allowed,
> > > that
> > > > > could
> > > > > > > > > be useful to other users as well. On the other hand, most
> > > users
> > > > > are
> > > > > > > > > not likely to write a custom authorizer to try to take
> > > advantage
> > > > > of
> > > > > > > > > version information being passed to the authorizer. So, I
> > > think
> > > > > using
> > > > > > > > > a configuration is clearly the better way to go here.
> Perhaps
> > > it
> > > > > can
> > > > > > > > > be a KIP-226 dynamic configuration to make this easier to
> > > deploy?
> > > > > > > > >
> > > > > > > > > cheers,
> > > > > > > > > Colin
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Mon, Feb 25, 2019, at 15:43, Harsha wrote:
> > > > > > > > > > Hi Ying,
> > > > > > > > > > I think the question is can we add a module in
> the
> > > core
> > > > > which
> > > > > > > > > > can take up the dynamic config and does a block certain
> APIs.
> > > > > This
> > > > > > > > > > module will be called in each of the APIs like the
> authorizer
> > > > > does
> > > > > > > > > > today to check if the API is supported for the client.
> > > > > > > > > > Instead of throwing AuthorizationException like the
> > > authorizer
> > > > > does
> > > > > > > > > > today it can throw UnsupportedException.
> > > > > > > > > > Benefits are, we are keeping the authorizer interface
> as is
> > > and
> > > > > adding
> > > > > > > > > > the flexibility based on dynamic configs without the
> need for
> > > > > > > > > > categorizing broker APIs and it will be easy to extend
> to do
> > > > > additional
> > > > > > > > > > options, like turning off certain features which might
> be in
> > > > > interest
> > > > > > > > > > to the service providers.
> > > > > > > > > > One drawback, It will introduce another call to check
> > > instead
> > > > > of
> > > > > > > > > > centralizing everything around Authorizer.
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Harsha
> > > > > > > > > >
> > > > > > > > > > On Mon, Feb 25, 2019, at 2:43 PM, Ying Zheng wrote:
> > > > > > > > > > > If you guys don't like the extension of authorizer
> > > interface,
> > > > > I will just
> > > > > > > > > > > propose a single broker dynamic configuration:
> > > > > client.min.api.version, to
> > > > > > > > > > > keep things simple.
> > > > > > > > > > >
> > > > > > > > > > > What do you think?
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Feb 25, 2019 at 2:23 PM Ying Zheng <
> yi...@uber.com
> > > >
> > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > @Viktor Somogyi-Vass, @Harsha, It seems the biggest
> > > concern
> > > > > is the
> > > > > > > > > > > > backward-compatibility to the existing authorizers.
> We
> > > can
> > > > > put the new
> > > > > > > > > > > > method into a new trait / interface:
> > > > > > > > > > > > trait AuthorizerEx extends Authorizer {
> > > > > > > > > > > > def authorize(session: Session, operation:
> Operation,
> > > > > resource: Resource,
> > > > > > > > > > > > apiVersion: Short): Boolean
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > When loading an authorizer class, broker will check
> if
> > > the
> > > > > class
> > > > > > > > > > > > implemented AuthorizerEx interface. If not, broker
> will
> > > > > wrapper the
> > > > > > > > > > > > Authorizer object with an Adapter class, in which
> > > > > authorizer(...
> > > > > > > > > > > > apiVersion) call is translated to the old
> authorizer()
> > > call.
> > > > > So that, both
> > > > > > > > > > > > old and new Authorizer is supported and can be
> treated as
> > > > > AuthorizerEx in
> > > > > > > > > > > > the new broker code.
> > > > > > > > > > > >
> > > > > > > > > > > > As for the broker dynamic configuration approach,
> I'm not
> > > > > sure how to
> > > > > > > > > > > > correctly categorize the 40+ broker APIs into a few
> > > > > categories.
> > > > > > > > > > > > For example, describe is used by producer, consumer,
> and
> > > > > admin. Should it
> > > > > > > > > > > > be controlled by producer.min.api.version or
> > > > > consumer.min.api.version?
> > > > > > > > > > > > Should producer.min.api.version apply to transaction
> > > > > operations?
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, Feb 25, 2019 at 10:33 AM Harsha <
> ka...@harsha.io
> > > >
> > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > >> I think the motivation of the KIP is to configure
> which
> > > API
> > > > > we want to
> > > > > > > > > > > >> allow for a broker.
> > > > > > > > > > > >> This is challenging for a hosted service where you
> have
> > > > > customers with
> > > > > > > > > > > >> different versions of clients.
> > > > > > > > > > > >> It's not just about down conversion but for example
> > > > > transactions, there
> > > > > > > > > > > >> is a case where we do not want to allow users to
> start
> > > > > using transactions
> > > > > > > > > > > >> and there is no way to disable to this right now
> and as
> > > > > specified in the
> > > > > > > > > > > >> KIP, having a lock on which client versions we
> support.
> > > > > > > > > > > >> Authorizer's original purpose is to allow policies
> to be
> > > > > enforced for
> > > > > > > > > > > >> each of the Kafka APIs, specifically in the context
> of
> > > > > security.
> > > > > > > > > > > >> Extending this to a general purpose gatekeeper might
> > > not be
> > > > > suitable and
> > > > > > > > > > > >> as mentioned in the thread every implementation of
> > > > > authorizer needs to
> > > > > > > > > > > >> re-implement to provide the same set of
> functionality.
> > > > > > > > > > > >> I think it's better to add an implementation which
> will
> > > use
> > > > > a broker's
> > > > > > > > > > > >> dynamic config as mentioned in approach 1.
> > > > > > > > > > > >>
> > > > > > > > > > > >> Thanks,
> > > > > > > > > > > >> Harsha
> > > > > > > > > > > >>
> > > > > > > > > > > >> On Sat, Feb 23, 2019, at 6:21 AM, Ismael Juma wrote:
> > > > > > > > > > > >> > Thanks for the KIP. Have we considered the
> existing
> > > topic
> > > > > config that
> > > > > > > > > > > >> makes
> > > > > > > > > > > >> > it possible to disallow down conversions? That's
> the
> > > > > biggest downside in
> > > > > > > > > > > >> > allowing older clients.
> > > > > > > > > > > >> >
> > > > > > > > > > > >> > Ismael
> > > > > > > > > > > >> >
> > > > > > > > > > > >> > On Fri, Feb 22, 2019, 2:11 PM Ying Zheng
> > > > > <yi...@uber.com.invalid>
> > > > > > > > > > > >> wrote:
> > > > > > > > > > > >> >
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> >
> > > > > > > > > > > >>
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
