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