cmccabe commented on a change in pull request #9032:
URL: https://github.com/apache/kafka/pull/9032#discussion_r465377717



##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -365,45 +368,90 @@ object ConfigCommand extends Config {
         adminClient.incrementalAlterConfigs(Map(configResource -> 
alterLogLevelEntries).asJava, alterOptions).all().get(60, TimeUnit.SECONDS)
 
       case ConfigType.User | ConfigType.Client =>
-        val nonQuotaConfigsToAdd = 
configsToBeAdded.keys.filterNot(QuotaConfigs.isQuotaConfig)
-        if (nonQuotaConfigsToAdd.nonEmpty)
-          throw new IllegalArgumentException(s"Only quota configs can be added 
for '$entityTypeHead' using --bootstrap-server. Unexpected config names: 
$nonQuotaConfigsToAdd")
-        val nonQuotaConfigsToDelete = 
configsToBeDeleted.filterNot(QuotaConfigs.isQuotaConfig)
-        if (nonQuotaConfigsToDelete.nonEmpty)
-          throw new IllegalArgumentException(s"Only quota configs can be 
deleted for '$entityTypeHead' using --bootstrap-server. Unexpected config 
names: $nonQuotaConfigsToDelete")
-
-
-        val oldConfig = getClientQuotasConfig(adminClient, entityTypes, 
entityNames)
-
-        val invalidConfigs = configsToBeDeleted.filterNot(oldConfig.contains)
-        if (invalidConfigs.nonEmpty)
-          throw new InvalidConfigurationException(s"Invalid config(s): 
${invalidConfigs.mkString(",")}")
-
-        val alterEntityTypes = entityTypes.map { entType =>
-          entType match {
-            case ConfigType.User => ClientQuotaEntity.USER
-            case ConfigType.Client => ClientQuotaEntity.CLIENT_ID
-            case _ => throw new IllegalArgumentException(s"Unexpected entity 
type: ${entType}")
+        val hasQuotaConfigsToAdd = 
configsToBeAdded.keys.exists(QuotaConfigs.isQuotaConfig)
+        val scramConfigsToAddMap = configsToBeAdded.filter(entry => 
ScramMechanism.isScram(entry._1))
+        val unknownConfigsToAdd = configsToBeAdded.keys.filterNot(key => 
ScramMechanism.isScram(key) || QuotaConfigs.isQuotaConfig(key))
+        val hasQuotaConfigsToDelete = 
configsToBeDeleted.exists(QuotaConfigs.isQuotaConfig)
+        val scramConfigsToDelete = 
configsToBeDeleted.filter(ScramMechanism.isScram)
+        val unknownConfigsToDelete = configsToBeDeleted.filterNot(key => 
ScramMechanism.isScram(key) || QuotaConfigs.isQuotaConfig(key))
+        if (entityTypeHead == ConfigType.Client || entityTypes.size == 2) { // 
size==2 for case where users is specified first on the command line, before 
clients
+          // either just a client or both a user and a client
+          if (unknownConfigsToAdd.nonEmpty || scramConfigsToAddMap.nonEmpty)
+            throw new IllegalArgumentException(s"Only quota configs can be 
added for '${ConfigType.Client}' using --bootstrap-server. Unexpected config 
names: ${unknownConfigsToAdd ++ scramConfigsToAddMap.keys}")
+          if (unknownConfigsToDelete.nonEmpty || scramConfigsToDelete.nonEmpty)
+            throw new IllegalArgumentException(s"Only quota configs can be 
deleted for '${ConfigType.Client}' using --bootstrap-server. Unexpected config 
names: ${unknownConfigsToDelete ++ scramConfigsToDelete}")
+        } else { // ConfigType.User
+          if (unknownConfigsToAdd.nonEmpty)
+            throw new IllegalArgumentException(s"Only quota and SCRAM 
credential configs can be added for '${ConfigType.User}' using 
--bootstrap-server. Unexpected config names: $unknownConfigsToAdd")
+          if (unknownConfigsToDelete.nonEmpty)
+            throw new IllegalArgumentException(s"Only quota and SCRAM 
credential configs can be deleted for '${ConfigType.User}' using 
--bootstrap-server. Unexpected config names: $unknownConfigsToDelete")
+          if (scramConfigsToAddMap.nonEmpty || scramConfigsToDelete.nonEmpty) {
+            if (entityNames.exists(_.isEmpty)) // either --entity-type users 
--entity-default or --user-defaults
+              throw new IllegalArgumentException("The use of --entity-default 
or --user-defaults is not allowed with User SCRAM Credentials using 
--bootstrap-server.")
+            if (hasQuotaConfigsToAdd || hasQuotaConfigsToDelete)
+              throw new IllegalArgumentException(s"Cannot alter both quota and 
SCRAM credential configs simultaneously for '${ConfigType.User}' using 
--bootstrap-server.")
           }
         }
-        val alterEntityNames = entityNames.map(en => if (en.nonEmpty) en else 
null)
 
-        // Explicitly populate a HashMap to ensure nulls are recorded properly.
-        val alterEntityMap = new java.util.HashMap[String, String]
-        alterEntityTypes.zip(alterEntityNames).foreach { case (k, v) => 
alterEntityMap.put(k, v) }
-        val entity = new ClientQuotaEntity(alterEntityMap)
+        if (hasQuotaConfigsToAdd || hasQuotaConfigsToDelete) {
+          // handle altering client/user quota configs

Review comment:
       it would be good to split this off into its own function (after 
verifying that we weren't trying to do both SCRAM + quota)




----------------------------------------------------------------
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:
us...@infra.apache.org


Reply via email to