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