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