Thanks, Colin. The standard "about" message for ThrottleTimeMs seems
to be "The
duration in milliseconds for which the request was throttled due to a quota
violation, or zero if the request did not violate any quota." as
opposed to "The
time spent waiting for quota." Should we adjust to match the typical
definition?
I'm wondering if describing Scram credentials should require READ privilege
rather than ALTER on the cluster? Altering SCRAM credentials of course
requires ALTER privilege, and I can see the argument for requiring ALTER
privilege to describe them as well, but it did catch my eye as something
worth questioning/confirming.
I'm also now thinking that "UNKNOWN" shouldn't be listed in the
ScramMechanism enum. I thought maybe it was a catch-all so we will always
be able to deserialize something regardless of what key actually appears,
but I just realized that SCRAM credentials and Client Quotas are mixed
together in the same JSON, so it will be up to the corresponding API to
ignore what it doesn't recognize -- i.e. if both client quotas and SCRAM
credentials are defined for a user, then invoking DescribeClientQuotas must
only describe the quota configs and invoking DescribeScramUsers must only
describe the SCRAM configs. Also, note that invoking kafka-configs.sh
--bootstrap-server ... --entity-type user --describe will require the
invocation of two separate APIs -- one to describe quotas and one to
describe SCRAM credentials; I don't think this is a problem, but I did want
to call it out explicitly.
Finally, there is a lack of consistency regarding how we name things. For
example, we are calling these APIs {Describe,Alter}ScramUsers and we have
declared the classes {Describe,Alter}ScramUserOptions, which matches up
fine. We also have public class DescribeScramUsersResult, which also
matches. But then we have public class AlterScramCredentialsResult,
interface ScramCredentialAlteration, public class ScramCredentialAddition,
and public class ScramCredentialDeletion, none of which match up in terms
of consistency of naming. I wonder if we should always use
"UserScramCredential" everywhere since that is technically what these API's
are about: describing/altering Users' SCRAM credentials. So the APis would
be {Describe,Alter}UserScramCredentials, and everything else that is
publiuc that now refers inconsistently to either ScramUsers or
ScramCredential would instead refer to UserScramCredentials (sometimes
singular rather than plural if warranted). For example: public class {
Describe,Alter}UserScramCredentialsResult, interface User
ScramCredentialAlteration, public class UserScramCredentialAddition, and
public class UserScramCredentialDeletion
Ron
On Wed, Jul 15, 2020 at 5:53 PM Colin McCabe <cmcc...@apache.org> wrote:
> Hi all,
>
> Thanks, everyone, for reviewing.
>
> Since we made a few changes to the RPCs in the last few days, I'm going to
> extend the vote until Monday and close it out then if it looks good.
>
> best,
> Colin
>
>
> On Wed, Jul 15, 2020, at 14:47, Colin McCabe wrote:
> > On Tue, Jul 14, 2020, at 16:23, Ron Dagostino wrote:
> > > Thanks, Colin.
> > >
> > > DescribeScramUsersResponse returns a list of ScramUser instances, which
> > > makes sense, but then each of the ScramUser instances only has a single
> > > ScramUserMechanismInfo instance. I believe it needs a List of those?
> >
> > Hi Ron,
> >
> > Sorry, that was a typo in the response JSON. Fixed.
> >
> > >
> > > Also, ScramUserMechanismInfo probably needs a better "about" value (it
> > > currently says "The user name.")
> > >
> >
> > Also fixed :)
> >
> > >
> > > Should both responses include ThrottleTimeMs fields?
> > >
> >
> > Good call. I added this.
> >
> > best,
> > Colin
> >
> > >
> > > I haven't looked at the AlterScramUsers stuff yet; I'll look at that in
> > > detail tomorrow.
> > >
> > > Ron
> > >
> > >
> > > On Tue, Jul 14, 2020 at 4:11 PM Colin McCabe <cmcc...@apache.org>
> wrote:
> > >
> > > > On Tue, Jul 14, 2020, at 07:57, Ron Dagostino wrote:
> > > > > Hi again, Colin. I also just realized a couple of other
> > > > incompatibilities
> > > > > with the way kafka-configs works today that prevents
> --bootstrap-server
> > > > > from being a drop-in replacement. This may or may not be a hard
> > > > > requirement, but we should explicitly decide on these one way or
> the
> > > > other.
> > > > >
> > > > > One issue is that it is legal to list the SCRAM credentials for a
> single
> > > > > user with kafka-configs (e.g. bin/kafka-configs.sh --zookeeper
> > > > > localhost:2181 --describe --entity-type users --entity-name
> alice). The
> > > > > current ListScramUsersRequest API does not support specifying an
> > > > (optional)
> > > > > user name, so it always returns all users' SCRAM credentials. We
> could
> > > > > filter the lst on the client side, of course, but that seems
> inefficient.
> > > > >
> > > >
> > > > Hi Ron,
> > > >
> > > > Yes, I think we should allow listing just a particular scram user or
> > > > users. I will change this to "describe" and add a list of user
> names which
> > > > can be supplied, or null to list all.
> > > >
> > > > (more responses below)
> > > >
> > > > >
> > > > > The second issue is that the content of the data returned by the
> new API
> > > > > does not match the content that kafka-configs currently returns.
> Here is
> > > > > what the tool currently returns:
> > > > >
> > > > > # add a user with a pair of SCRAM credentials
> > > > > $ bin/kafka-configs.sh --zookeeper localhost:2181 --alter
> --add-config
> > > > > 'SCRAM-SHA-256=[iterations=8192,password=alice-secret],
> > > > > SCRAM-SHA-512=[password=alice-secret]' --entity-type users
> --entity-name
> > > > > alice
> > > > > # list that user's SCRAM credentials
> > > > > $ bin/kafka-configs.sh --zookeeper localhost:2181 --describe
> > > > > --entity-type
> > > > > users --entity-name alice
> > > > > Configs for user-principal 'alice' are
> > > > >
> > > >
> SCRAM-SHA-512=salt=MXNpcXViZmwxcmN4ZzhjeDkwam51c2I3Yw==,stored_key=V3IYnwwC5qjrzoRMyLrzgBstrJVDimQkFfAnJbVrG5mIEaJ/W6j5iV6xITF6HWvtsYrhGDIxsNSZbvVxAIck1w==,server_key=/BpX9JtxqzqyQS6NQcvyksN8Hs8XV65f2G7jx9PMy8Z9s22qCQrqCAtpvLPReYIMMDYRZ9/5x4aSlU/1rfLvVA==,iterations=4096,SCRAM-SHA-256=salt=N3g1eTB2aWUyeDlkYXZ5NDljM3h2aTd4dA==,stored_key=zzliLFJtNeQGY07lBAzzxM6jz0dEm5OkpaJUTfRrD+Y=,server_key=punFVKLCKcZymuRCqh6f6Gjp+VU8ZE3qd8iTboMqHbA=,iterations=8192
> > > > >
> > > > > The API as currently defined only returns the number of
> iterations. I
> > > > > would like to confirm that this particular lack of drop-in
> compatibility
> > > > is
> > > > > acceptable.
> > > > >
> > > >
> > > > Yes, this is expected. The argument was that returning the salted
> > > > password and hash was not secure, so we elected not to return this
> > > > information.
> > > >
> > > > >
> > > > > Even if the difference in content is acceptable, I think the
> inability to
> > > > > list a single user is probably something we should fix, and the
> previous
> > > > > issue I raised about kafka-configs being able to specify
> alterations and
> > > > > deletions simultaneously also still stands as something we need to
> decide
> > > > > about -- perhaps drop-in compatibility is not a requirement given
> the
> > > > > content difference, in which case we could make it an error to
> specify
> > > > both
> > > > > alterations and deletions when using --bootstrap-server.
> > > > > [...]
> > > > >
> > > >
> > > > I think you brought up some very good points. I got rid of the
> delete
> > > > operation and replaced it with an Alter that can remove individual
> > > > credentials as needed. We certainly need this, given what the
> command line
> > > > tool needs to be able to do.
> > > >
> > > > Thanks for the comments. Take a look and see if the latest changes
> fix
> > > > it...
> > > >
> > > > best,
> > > > Colin
> > > >
> > > > >
> > > > >
> > > > > Ron
> > > > >
> > > > >
> > > > > On Mon, Jul 13, 2020 at 4:21 PM Ron Dagostino <rndg...@gmail.com>
> wrote:
> > > > >
> > > > > > Hi Colin. I wanted to explicitly identify a side-effect that I
> think
> > > > > > derives from having deletions separated out from the
> > > > AlterScramUsersRequest
> > > > > > and put in their own DeleteScramUsersRequest. The command line
> > > > invocation
> > > > > > of kafka-configs can specify alterations and deletions
> simultaneously:
> > > > it
> > > > > > is entirely legal for that tool to accept and process both
> > > > --add-config and
> > > > > > --delete-config (the current code removes any keys from the added
> > > > configs
> > > > > > that are also supplied in the deleted configs, it grabs the
> > > > > > currently-defined keys, deletes the keys to be deleted, adds the
> ones
> > > > to be
> > > > > > added, and then sets the JSON for the user accordingly). If we
> split
> > > > these
> > > > > > two operations into separate APIs then an invocation of
> kafka-configs
> > > > that
> > > > > > specifies both operations can't complete atomically and could
> possibly
> > > > have
> > > > > > one of them succeed but the other fail. I am wondering if
> splitting
> > > > the
> > > > > > deletions out into a separate API is acceptable given this
> > > > possibility, and
> > > > > > if so, what the behavior should be. Maybe the kafka-configs
> command
> > > > would
> > > > > > have to prevent both from being specified simultaneously when
> > > > > > --bootstrap-server is used. That would create an inconsistency
> with
> > > > how
> > > > > > the tool works with --zookeeper, meaning it is conceivable that
> > > > switching
> > > > > > over to --bootstrap-server would not necessarily be a drop-in
> > > > replacement.
> > > > > > Am I missing/misunderstanding something? Thoughts?
> > > > > >
> > > > > > Also, separately, should the responses include a ThrottleTimeMs
> > > > field? I
> > > > > > believe so but would like to confirm.
> > > > > >
> > > > > > Ron
> > > > > >
> > > > > > On Mon, Jul 13, 2020 at 3:44 PM David Arthur <mum...@gmail.com>
> wrote:
> > > > > >
> > > > > >> Thanks for the clarification, Colin. +1 binding from me
> > > > > >>
> > > > > >> -David
> > > > > >>
> > > > > >> On Mon, Jul 13, 2020 at 3:40 PM Colin McCabe <
> cmcc...@apache.org>
> > > > wrote:
> > > > > >>
> > > > > >> > Thanks, Boyang. Fixed.
> > > > > >> >
> > > > > >> > best,
> > > > > >> > Colin
> > > > > >> >
> > > > > >> > On Mon, Jul 13, 2020, at 08:43, Boyang Chen wrote:
> > > > > >> > > Thanks for the update Colin. One nit comment to fix the RPC
> type
> > > > > >> > > for AlterScramUsersRequest as:
> > > > > >> > > "apiKey": 51,
> > > > > >> > > "type": "request",
> > > > > >> > > "name": "AlterScramUsersRequest",
> > > > > >> > > Other than that, +1 (binding) from me.
> > > > > >> > >
> > > > > >> > >
> > > > > >> > > On Mon, Jul 13, 2020 at 8:38 AM Colin McCabe <
> cmcc...@apache.org>
> > > > > >> wrote:
> > > > > >> > >
> > > > > >> > > > Hi David,
> > > > > >> > > >
> > > > > >> > > > The API is for clients. Brokers will still listen to
> ZooKeeper
> > > > to
> > > > > >> load
> > > > > >> > > > the SCRAM information.
> > > > > >> > > >
> > > > > >> > > > best,
> > > > > >> > > > Colin
> > > > > >> > > >
> > > > > >> > > >
> > > > > >> > > > On Mon, Jul 13, 2020, at 08:30, David Arthur wrote:
> > > > > >> > > > > Thanks for the KIP, Colin. The new RPCs look good to
> me, just
> > > > one
> > > > > >> > > > question:
> > > > > >> > > > > since we don't return the password info through the
> RPC, how
> > > > will
> > > > > >> > brokers
> > > > > >> > > > > load this info? (I'm presuming that they need it to
> configure
> > > > > >> > > > > authentication)
> > > > > >> > > > >
> > > > > >> > > > > -David
> > > > > >> > > > >
> > > > > >> > > > > On Mon, Jul 13, 2020 at 10:57 AM Colin McCabe <
> > > > cmcc...@apache.org
> > > > > >> >
> > > > > >> > > > wrote:
> > > > > >> > > > >
> > > > > >> > > > > > On Fri, Jul 10, 2020, at 10:55, Boyang Chen wrote:
> > > > > >> > > > > > > Hey Colin, thanks for the KIP. One question I have
> about
> > > > > >> > > > AlterScramUsers
> > > > > >> > > > > > > RPC is whether we could consolidate the deletion
> list and
> > > > > >> > alteration
> > > > > >> > > > > > list,
> > > > > >> > > > > > > since in response we only have a single list of
> results.
> > > > The
> > > > > >> > further
> > > > > >> > > > > > > benefit is to reduce unintentional duplicate
> entries for
> > > > both
> > > > > >> > > > deletion
> > > > > >> > > > > > and
> > > > > >> > > > > > > alteration, which makes the broker side handling
> logic
> > > > easier.
> > > > > >> > > > Another
> > > > > >> > > > > > > alternative is to add DeleteScramUsers RPC to align
> what
> > > > we
> > > > > >> > currently
> > > > > >> > > > > > have
> > > > > >> > > > > > > with other user provided data such as delegation
> tokens
> > > > > >> (create,
> > > > > >> > > > change,
> > > > > >> > > > > > > delete).
> > > > > >> > > > > > >
> > > > > >> > > > > >
> > > > > >> > > > > > Hi Boyang,
> > > > > >> > > > > >
> > > > > >> > > > > > It can't really be consolidated without some
> awkwardness.
> > > > It's
> > > > > >> > > > probably
> > > > > >> > > > > > better just to create a DeleteScramUsers function and
> RPC.
> > > > I've
> > > > > >> > > > changed
> > > > > >> > > > > > the KIP.
> > > > > >> > > > > >
> > > > > >> > > > > > >
> > > > > >> > > > > > > For my own education, the salt will be automatically
> > > > generated
> > > > > >> > by the
> > > > > >> > > > > > admin
> > > > > >> > > > > > > client when we send the SCRAM requests correct?
> > > > > >> > > > > > >
> > > > > >> > > > > >
> > > > > >> > > > > > Yes, the client generates the salt before sending the
> > > > request.
> > > > > >> > > > > >
> > > > > >> > > > > > best,
> > > > > >> > > > > > Colin
> > > > > >> > > > > >
> > > > > >> > > > > > > Best,
> > > > > >> > > > > > > Boyang
> > > > > >> > > > > > >
> > > > > >> > > > > > > On Fri, Jul 10, 2020 at 8:10 AM Rajini Sivaram <
> > > > > >> > > > rajinisiva...@gmail.com>
> > > > > >> > > > > > > wrote:
> > > > > >> > > > > > >
> > > > > >> > > > > > > > +1 (binding)
> > > > > >> > > > > > > >
> > > > > >> > > > > > > > Thanks for the KIP, Colin!
> > > > > >> > > > > > > >
> > > > > >> > > > > > > > Regards,
> > > > > >> > > > > > > >
> > > > > >> > > > > > > > Rajini
> > > > > >> > > > > > > >
> > > > > >> > > > > > > >
> > > > > >> > > > > > > > On Thu, Jul 9, 2020 at 8:49 PM Colin McCabe <
> > > > > >> > cmcc...@apache.org>
> > > > > >> > > > > > wrote:
> > > > > >> > > > > > > >
> > > > > >> > > > > > > > > Hi all,
> > > > > >> > > > > > > > >
> > > > > >> > > > > > > > > I'd like to call a vote for KIP-554: Add a
> broker-side
> > > > > >> SCRAM
> > > > > >> > > > > > > > configuration
> > > > > >> > > > > > > > > API. The KIP is here:
> > > > > >> > > > https://cwiki.apache.org/confluence/x/ihERCQ
> > > > > >> > > > > > > > >
> > > > > >> > > > > > > > > The previous discussion thread is here:
> > > > > >> > > > > > > > >
> > > > > >> > > > > > > > >
> > > > > >> > > > > > > >
> > > > > >> > > > > >
> > > > > >> > > >
> > > > > >> >
> > > > > >>
> > > >
> https://lists.apache.org/thread.html/r69bdc65bdf58f5576944a551ff249d759073ecbf5daa441cff680ab0%40%3Cdev.kafka.apache.org%3E
> > > > > >> > > > > > > > >
> > > > > >> > > > > > > > > best,
> > > > > >> > > > > > > > > Colin
> > > > > >> > > > > > > > >
> > > > > >> > > > > > > >
> > > > > >> > > > > > >
> > > > > >> > > > > >
> > > > > >> > > > >
> > > > > >> > > > >
> > > > > >> > > > > --
> > > > > >> > > > > David Arthur
> > > > > >> > > > >
> > > > > >> > > >
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > > >>
> > > > > >> --
> > > > > >> David Arthur
> > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
>
