[GitHub] [kafka] rondagostino commented on pull request #9032: KAFKA-10259: KIP-554 Broker-side SCRAM Config API

[GitBox <gi...@apache.org>]

rondagostino commented on pull request #9032:
URL: https://github.com/apache/kafka/pull/9032#issuecomment-672110976


   @cmccabe and @rajinisivaram Thanks for the reviews.  I think this is where we stand:
   
   1. I removed `-1` as a valid `iterations` value in the code.  I will need to announce this in the DISCUSS email thread and adjust the KIP.  I will do that once we have the full set of KIP changes based on all these points.
   2. We decided that we will not allow users who authenticated using delegation tokens to alter user SRAM credentials.  I will add a mention of this to the KIP once we have the full set of KIP changes based on all these points.
   2. The KIP says that Describe `will be sent to the controller, and will return NOT_CONTROLLER if the receiving broker is not the controller.`. This constraint is perhaps not necessary for Describe requests.  If we are all in agreement then I can change the code, and I will adjust the KIP once we have the full set of KIP changes based on all these points.
   3. We have to decide what to do if a client asks to Describe specific users that don't exist.  The KIP does not explicitly say what to do in this situation, but the Describe Response only has a top-level error; unlike the Alterations response, the Describe response doesn't provide an error per user.  So the entire Describe request currently has to succeed or fail as a whole.  Therefore the code currently just silently ignores users that don't exist and only describes users that do exist (i.e. if you request to describe `user1`, `user2`, and `user3`, but `user2` doesn't exist (or it does exist but doesn't have any SCRAM credentials), the response indicates success and describes just `user1` and `user3`.  We need to decide if the current behavior is acceptable, and if it is acceptable, I would add a line to the KIP making this behavior explicit (since I just implied it based on the single error design).  If this behavior is not acceptable, then I think we would need to adjust the fo
 rmat of the response, the code, and the KIP.  Thoughts?
   4. I updated existing both the `integration tests` and `system tests` to use the admin client when creating User SCRAM credentials after the brokers start instead of going directly to ZooKeeper; ZooKeeper is still used for bootstrapping broker credentials prior to starting Kafka, of course.  These changes haven't been reviewed yet, I don't think, and we should probably kick off test runs to exercise everything (tests pass locally for me).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org