You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Colin McCabe <cm...@apache.org> on 2021/11/23 21:52:53 UTC
[DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Hi all,
I have written a new KIP describing an Authorizer that stores data in the __cluster_metadata topic. It is designed to be used in KRaft mode. Please take a look here: https://cwiki.apache.org/confluence/x/h5KqCw
best,
Colin
Re: [DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Posted by Colin McCabe <cm...@apache.org>.
On Tue, Dec 14, 2021, at 10:13, David Arthur wrote:
> Thanks for the KIP, Colin!
>
> 4) For the various "Type" fields in the AccessControlEntityRecord, is it
> worth explicitly enumerating the allowed types in the KIP? E.g.,
> PermissionType = {Any, Deny, Allow}. If these are listed out in another
> KIP, maybe we can just reference that.
>
Hi David,
Good question. The values should be the same as those used in the ACL RPCs (CreateAclsRequest, etc.) I will include this information in the KIP.
> 5) You mention "StandardAuthorizer must load all the records atomically as
> a group" when loading from a snapshot. I was under the impression that
> snapshot loads were already effectively atomic operations. That is, we
> recalculate the whole MetadataImage from the snapshot and publish it to
> components. Can you clarify what you mean here? Is this to do with how
> StandardAuthorizer handles the published metadata?
>
Snapshots are still replayed one record at a time, though. I was pointing out that we don't want to do this for ACLs since we want to encounter only valid states.
> 6) When we handle Create/Delete ACL RPCs on the controller, I think
> the operations should be written as atomic batches to the metadata log.
> Should we mention this here?
Hmm... I'm not sure if batching has an impact since these are single records. I hope I'm not issing something.
best,
Colin
>
> Thanks!
> David
>
>
>
>
>
>
>
> On Tue, Dec 14, 2021 at 11:27 AM José Armando García Sancio
> <js...@confluent.io.invalid> wrote:
>
>> Thanks for the additional information Colin.
>>
>> On Mon, Dec 13, 2021 at 4:43 PM Colin McCabe <cm...@apache.org> wrote:
>> >
>> > Hi José,
>> >
>> > I think these are good questions. We have a few situations like this
>> where there is something brokers have to know before they can contact the
>> controller quorum -- or something controllers have to know before they can
>> accept broker connections. Basically, the bootstrapping problem.
>> >
>> > Offhand, I can think of a few scenarios like this:
>> > 1. If you need certain ACLs to be present, you need a way of setting
>> those up on the controller before starting the controller quorum for the
>> first time.
>> > 2. If you are using SCRAM for the broker user, you need some way of
>> setting that up before starting up the cluster for the first time.
>> > 3. If you are using KIP-226 dynamic broker configurations to configure
>> the SSL settings for the connection to the controller, you need a way of
>> setting those up before starting the broker.
>>
>> It sounds to me like KIP-801 is assuming that this "bootstrapping KIP"
>> will at least generate a snapshot with this information in all of the
>> controllers. I would like to understand this a bit better. Do you
>> think that we need to write this "bootstrapping KIP" as soon as
>> possible?
>>
>> Thanks
>> -José
>>
>
>
> --
> -David
Re: [DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Posted by David Arthur <da...@confluent.io.INVALID>.
Thanks for the KIP, Colin!
4) For the various "Type" fields in the AccessControlEntityRecord, is it
worth explicitly enumerating the allowed types in the KIP? E.g.,
PermissionType = {Any, Deny, Allow}. If these are listed out in another
KIP, maybe we can just reference that.
5) You mention "StandardAuthorizer must load all the records atomically as
a group" when loading from a snapshot. I was under the impression that
snapshot loads were already effectively atomic operations. That is, we
recalculate the whole MetadataImage from the snapshot and publish it to
components. Can you clarify what you mean here? Is this to do with how
StandardAuthorizer handles the published metadata?
6) When we handle Create/Delete ACL RPCs on the controller, I think
the operations should be written as atomic batches to the metadata log.
Should we mention this here?
Thanks!
David
On Tue, Dec 14, 2021 at 11:27 AM José Armando García Sancio
<js...@confluent.io.invalid> wrote:
> Thanks for the additional information Colin.
>
> On Mon, Dec 13, 2021 at 4:43 PM Colin McCabe <cm...@apache.org> wrote:
> >
> > Hi José,
> >
> > I think these are good questions. We have a few situations like this
> where there is something brokers have to know before they can contact the
> controller quorum -- or something controllers have to know before they can
> accept broker connections. Basically, the bootstrapping problem.
> >
> > Offhand, I can think of a few scenarios like this:
> > 1. If you need certain ACLs to be present, you need a way of setting
> those up on the controller before starting the controller quorum for the
> first time.
> > 2. If you are using SCRAM for the broker user, you need some way of
> setting that up before starting up the cluster for the first time.
> > 3. If you are using KIP-226 dynamic broker configurations to configure
> the SSL settings for the connection to the controller, you need a way of
> setting those up before starting the broker.
>
> It sounds to me like KIP-801 is assuming that this "bootstrapping KIP"
> will at least generate a snapshot with this information in all of the
> controllers. I would like to understand this a bit better. Do you
> think that we need to write this "bootstrapping KIP" as soon as
> possible?
>
> Thanks
> -José
>
--
-David
Re: [DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Posted by Colin McCabe <cm...@apache.org>.
Thanks for looking at the KIP, Manikumar.
On Mon, Jan 31, 2022, at 11:21, Manikumar wrote:
> Thanks for the KIP. LGTM once we get consensus on bootstrapping logic.
>
The current plan for bootstrapping is to use super.users for now. Later, we'll have a separate KIP that will allow us to bootstrap a lot of KIP-500 / KRaft things by putting some records in the metadata log when we format the broker and controller nodes.
> 1. When do we say, ACL is created (or deleted)? Is it after we persist it
> durably by a majority of peers?.
Yes. From the perspective of the brokers, the ACL is created when the record is persisted durably by a majority of peers (controllers).
> 2. Can we add a metric for total acl count (This was missing in ZK
> Authoriser).
Good idea. I added a new metric to the KIP.
best,
Colin
>
> On Thu, Jan 13, 2022 at 2:26 AM David Arthur
> <da...@confluent.io.invalid> wrote:
>
>> Sounds good on the ordering, and yea I agree we can look at atomic ACL
>> modifications in the future. Thanks!
>>
>> On Wed, Jan 12, 2022 at 3:53 PM Colin McCabe <cm...@apache.org> wrote:
>>
>> > Hi David,
>> >
>> > On Wed, Dec 15, 2021, at 07:28, David Arthur wrote:
>> > > 5) Ok, gotcha. So will the StandardAuthorizer be replaying records
>> > > directly, or will it get an *Image like other metadata consumers on the
>> > > broker?
>> > >
>> >
>> > It reads the information out of the MetadataDelta, being careful to
>> > preserve the ordering of the changes.
>> >
>> > The current implementation uses a LinkedHashMap to preserve that
>> ordering.
>> > You can take a look at the PR here:
>> > https://github.com/apache/kafka/pull/11649/files
>> >
>> > > 6) I was thinking since the CreateAcl and DeleteAcl requests can modify
>> > > multiple ACL in one request, that we should reflect that by committing
>> > the
>> > > resulting records as an atomic batch. I think from an operators
>> > > perspective, they would expect the ACLs sent in their request to be
>> > > enacted together atomically.
>> > >
>> >
>> > That's never been guaranteed, though. Creating multiple ACLs in ZK
>> > requires changing multiple znodes, which is not atomic. Given that users
>> > haven't asked for this and it would add substantial complexity, can be
>> > discuss it later once we have feature parity with the ZK version?
>> >
>> > best,
>> > Colin
>> >
>> >
>> > >
>> > >
>> > > On Tue, Dec 14, 2021 at 4:20 PM Colin McCabe <cm...@apache.org>
>> wrote:
>> > >
>> > >> On Tue, Dec 14, 2021, at 08:27, José Armando García Sancio wrote:
>> > >> > Thanks for the additional information Colin.
>> > >> >
>> > >> ...
>> > >> >
>> > >> > It sounds to me like KIP-801 is assuming that this "bootstrapping
>> KIP"
>> > >> > will at least generate a snapshot with this information in all of
>> the
>> > >> > controllers. I would like to understand this a bit better. Do you
>> > >> > think that we need to write this "bootstrapping KIP" as soon as
>> > >> > possible?
>> > >> >
>> > >>
>> > >> Hi José,
>> > >>
>> > >> I don't know about "as soon as possible." The authorizer is useful
>> even
>> > >> without the bootstrapping KIP, as I mentioned (just using
>> super.users).
>> > But
>> > >> I do think we'll need the bootstrapping KIP before KRaft goes GA.
>> > >>
>> > >> best,
>> > >> Colin
>> > >>
>> > >
>> > >
>> > > --
>> > > -David
>> >
>>
>>
>> --
>> -David
>>
Re: [DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Posted by Manikumar <ma...@gmail.com>.
Thanks for the KIP. LGTM once we get consensus on bootstrapping logic.
1. When do we say, ACL is created (or deleted)? Is it after we persist it
durably by a majority of peers?.
2. Can we add a metric for total acl count (This was missing in ZK
Authoriser).
On Thu, Jan 13, 2022 at 2:26 AM David Arthur
<da...@confluent.io.invalid> wrote:
> Sounds good on the ordering, and yea I agree we can look at atomic ACL
> modifications in the future. Thanks!
>
> On Wed, Jan 12, 2022 at 3:53 PM Colin McCabe <cm...@apache.org> wrote:
>
> > Hi David,
> >
> > On Wed, Dec 15, 2021, at 07:28, David Arthur wrote:
> > > 5) Ok, gotcha. So will the StandardAuthorizer be replaying records
> > > directly, or will it get an *Image like other metadata consumers on the
> > > broker?
> > >
> >
> > It reads the information out of the MetadataDelta, being careful to
> > preserve the ordering of the changes.
> >
> > The current implementation uses a LinkedHashMap to preserve that
> ordering.
> > You can take a look at the PR here:
> > https://github.com/apache/kafka/pull/11649/files
> >
> > > 6) I was thinking since the CreateAcl and DeleteAcl requests can modify
> > > multiple ACL in one request, that we should reflect that by committing
> > the
> > > resulting records as an atomic batch. I think from an operators
> > > perspective, they would expect the ACLs sent in their request to be
> > > enacted together atomically.
> > >
> >
> > That's never been guaranteed, though. Creating multiple ACLs in ZK
> > requires changing multiple znodes, which is not atomic. Given that users
> > haven't asked for this and it would add substantial complexity, can be
> > discuss it later once we have feature parity with the ZK version?
> >
> > best,
> > Colin
> >
> >
> > >
> > >
> > > On Tue, Dec 14, 2021 at 4:20 PM Colin McCabe <cm...@apache.org>
> wrote:
> > >
> > >> On Tue, Dec 14, 2021, at 08:27, José Armando García Sancio wrote:
> > >> > Thanks for the additional information Colin.
> > >> >
> > >> ...
> > >> >
> > >> > It sounds to me like KIP-801 is assuming that this "bootstrapping
> KIP"
> > >> > will at least generate a snapshot with this information in all of
> the
> > >> > controllers. I would like to understand this a bit better. Do you
> > >> > think that we need to write this "bootstrapping KIP" as soon as
> > >> > possible?
> > >> >
> > >>
> > >> Hi José,
> > >>
> > >> I don't know about "as soon as possible." The authorizer is useful
> even
> > >> without the bootstrapping KIP, as I mentioned (just using
> super.users).
> > But
> > >> I do think we'll need the bootstrapping KIP before KRaft goes GA.
> > >>
> > >> best,
> > >> Colin
> > >>
> > >
> > >
> > > --
> > > -David
> >
>
>
> --
> -David
>
Re: [DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Posted by David Arthur <da...@confluent.io.INVALID>.
Sounds good on the ordering, and yea I agree we can look at atomic ACL
modifications in the future. Thanks!
On Wed, Jan 12, 2022 at 3:53 PM Colin McCabe <cm...@apache.org> wrote:
> Hi David,
>
> On Wed, Dec 15, 2021, at 07:28, David Arthur wrote:
> > 5) Ok, gotcha. So will the StandardAuthorizer be replaying records
> > directly, or will it get an *Image like other metadata consumers on the
> > broker?
> >
>
> It reads the information out of the MetadataDelta, being careful to
> preserve the ordering of the changes.
>
> The current implementation uses a LinkedHashMap to preserve that ordering.
> You can take a look at the PR here:
> https://github.com/apache/kafka/pull/11649/files
>
> > 6) I was thinking since the CreateAcl and DeleteAcl requests can modify
> > multiple ACL in one request, that we should reflect that by committing
> the
> > resulting records as an atomic batch. I think from an operators
> > perspective, they would expect the ACLs sent in their request to be
> > enacted together atomically.
> >
>
> That's never been guaranteed, though. Creating multiple ACLs in ZK
> requires changing multiple znodes, which is not atomic. Given that users
> haven't asked for this and it would add substantial complexity, can be
> discuss it later once we have feature parity with the ZK version?
>
> best,
> Colin
>
>
> >
> >
> > On Tue, Dec 14, 2021 at 4:20 PM Colin McCabe <cm...@apache.org> wrote:
> >
> >> On Tue, Dec 14, 2021, at 08:27, José Armando García Sancio wrote:
> >> > Thanks for the additional information Colin.
> >> >
> >> ...
> >> >
> >> > It sounds to me like KIP-801 is assuming that this "bootstrapping KIP"
> >> > will at least generate a snapshot with this information in all of the
> >> > controllers. I would like to understand this a bit better. Do you
> >> > think that we need to write this "bootstrapping KIP" as soon as
> >> > possible?
> >> >
> >>
> >> Hi José,
> >>
> >> I don't know about "as soon as possible." The authorizer is useful even
> >> without the bootstrapping KIP, as I mentioned (just using super.users).
> But
> >> I do think we'll need the bootstrapping KIP before KRaft goes GA.
> >>
> >> best,
> >> Colin
> >>
> >
> >
> > --
> > -David
>
--
-David
Re: [DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Posted by Colin McCabe <cm...@apache.org>.
Hi David,
On Wed, Dec 15, 2021, at 07:28, David Arthur wrote:
> 5) Ok, gotcha. So will the StandardAuthorizer be replaying records
> directly, or will it get an *Image like other metadata consumers on the
> broker?
>
It reads the information out of the MetadataDelta, being careful to preserve the ordering of the changes.
The current implementation uses a LinkedHashMap to preserve that ordering. You can take a look at the PR here: https://github.com/apache/kafka/pull/11649/files
> 6) I was thinking since the CreateAcl and DeleteAcl requests can modify
> multiple ACL in one request, that we should reflect that by committing the
> resulting records as an atomic batch. I think from an operators
> perspective, they would expect the ACLs sent in their request to be
> enacted together atomically.
>
That's never been guaranteed, though. Creating multiple ACLs in ZK requires changing multiple znodes, which is not atomic. Given that users haven't asked for this and it would add substantial complexity, can be discuss it later once we have feature parity with the ZK version?
best,
Colin
>
>
> On Tue, Dec 14, 2021 at 4:20 PM Colin McCabe <cm...@apache.org> wrote:
>
>> On Tue, Dec 14, 2021, at 08:27, José Armando García Sancio wrote:
>> > Thanks for the additional information Colin.
>> >
>> ...
>> >
>> > It sounds to me like KIP-801 is assuming that this "bootstrapping KIP"
>> > will at least generate a snapshot with this information in all of the
>> > controllers. I would like to understand this a bit better. Do you
>> > think that we need to write this "bootstrapping KIP" as soon as
>> > possible?
>> >
>>
>> Hi José,
>>
>> I don't know about "as soon as possible." The authorizer is useful even
>> without the bootstrapping KIP, as I mentioned (just using super.users). But
>> I do think we'll need the bootstrapping KIP before KRaft goes GA.
>>
>> best,
>> Colin
>>
>
>
> --
> -David
Re: [DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Posted by David Arthur <da...@confluent.io.INVALID>.
5) Ok, gotcha. So will the StandardAuthorizer be replaying records
directly, or will it get an *Image like other metadata consumers on the
broker?
6) I was thinking since the CreateAcl and DeleteAcl requests can modify
multiple ACL in one request, that we should reflect that by committing the
resulting records as an atomic batch. I think from an operators
perspective, they would expect the ACLs sent in their request to be
enacted together atomically.
On Tue, Dec 14, 2021 at 4:20 PM Colin McCabe <cm...@apache.org> wrote:
> On Tue, Dec 14, 2021, at 08:27, José Armando García Sancio wrote:
> > Thanks for the additional information Colin.
> >
> ...
> >
> > It sounds to me like KIP-801 is assuming that this "bootstrapping KIP"
> > will at least generate a snapshot with this information in all of the
> > controllers. I would like to understand this a bit better. Do you
> > think that we need to write this "bootstrapping KIP" as soon as
> > possible?
> >
>
> Hi José,
>
> I don't know about "as soon as possible." The authorizer is useful even
> without the bootstrapping KIP, as I mentioned (just using super.users). But
> I do think we'll need the bootstrapping KIP before KRaft goes GA.
>
> best,
> Colin
>
--
-David
Re: [DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Posted by Colin McCabe <cm...@apache.org>.
On Tue, Dec 14, 2021, at 08:27, José Armando García Sancio wrote:
> Thanks for the additional information Colin.
>
...
>
> It sounds to me like KIP-801 is assuming that this "bootstrapping KIP"
> will at least generate a snapshot with this information in all of the
> controllers. I would like to understand this a bit better. Do you
> think that we need to write this "bootstrapping KIP" as soon as
> possible?
>
Hi José,
I don't know about "as soon as possible." The authorizer is useful even without the bootstrapping KIP, as I mentioned (just using super.users). But I do think we'll need the bootstrapping KIP before KRaft goes GA.
best,
Colin
Re: [DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Posted by José Armando García Sancio <js...@confluent.io.INVALID>.
Thanks for the additional information Colin.
On Mon, Dec 13, 2021 at 4:43 PM Colin McCabe <cm...@apache.org> wrote:
>
> Hi José,
>
> I think these are good questions. We have a few situations like this where there is something brokers have to know before they can contact the controller quorum -- or something controllers have to know before they can accept broker connections. Basically, the bootstrapping problem.
>
> Offhand, I can think of a few scenarios like this:
> 1. If you need certain ACLs to be present, you need a way of setting those up on the controller before starting the controller quorum for the first time.
> 2. If you are using SCRAM for the broker user, you need some way of setting that up before starting up the cluster for the first time.
> 3. If you are using KIP-226 dynamic broker configurations to configure the SSL settings for the connection to the controller, you need a way of setting those up before starting the broker.
It sounds to me like KIP-801 is assuming that this "bootstrapping KIP"
will at least generate a snapshot with this information in all of the
controllers. I would like to understand this a bit better. Do you
think that we need to write this "bootstrapping KIP" as soon as
possible?
Thanks
-José
Re: [DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Posted by Colin McCabe <cm...@apache.org>.
Hi José,
I think these are good questions. We have a few situations like this where there is something brokers have to know before they can contact the controller quorum -- or something controllers have to know before they can accept broker connections. Basically, the bootstrapping problem.
Offhand, I can think of a few scenarios like this:
1. If you need certain ACLs to be present, you need a way of setting those up on the controller before starting the controller quorum for the first time.
2. If you are using SCRAM for the broker user, you need some way of setting that up before starting up the cluster for the first time.
3. If you are using KIP-226 dynamic broker configurations to configure the SSL settings for the connection to the controller, you need a way of setting those up before starting the broker.
Let's tackle these in a separate KIP since they all have some common features, and I think it's somewhat orthogonal to the main point here (authorizer in kraft)
If this KIP lands before the bootstrapping one, we can always set super.users to be the broker user as a workaround.
best,
Colin
On Wed, Dec 8, 2021, at 14:15, José Armando García Sancio wrote:
> Hi Colin,
>
> Thanks for the KIP.
>
> 1. Can you talk about how the set of ACLs needed to authorize
> controllers and brokers will get bootstrapped? I am particularly
> interested in how we are going to configure a new cluster so that the
> controllers nodes can authorize each other's requests to establish
> quorum. After a quorum is established, I am interested in how the user
> would make sure that new brokers will get authorize against the
> controllers for requests like "register broker" and "fetch".
>
> thanks,
> -Jose
Re: [DISCUSS] KIP-801: Implement an Authorizer that stores metadata in __cluster_metadata
Posted by José Armando García Sancio <js...@confluent.io.INVALID>.
Hi Colin,
Thanks for the KIP.
1. Can you talk about how the set of ACLs needed to authorize
controllers and brokers will get bootstrapped? I am particularly
interested in how we are going to configure a new cluster so that the
controllers nodes can authorize each other's requests to establish
quorum. After a quorum is established, I am interested in how the user
would make sure that new brokers will get authorize against the
controllers for requests like "register broker" and "fetch".
thanks,
-Jose