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