ahuang98 commented on code in PR #16346:
URL: https://github.com/apache/kafka/pull/16346#discussion_r1643521884
##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -640,8 +640,8 @@ class DynamicBrokerConfig(private val kafkaConfig:
KafkaConfig) extends Logging
reconfigurable.validateReconfiguration(newConfigs)
} catch {
case e: ConfigException => throw e
- case _: Exception =>
- throw new ConfigException(s"Validation of dynamic config update of
$updatedConfigNames failed with class ${reconfigurable.getClass}")
+ case e: Exception =>
+ throw new ConfigException(s"Validation of dynamic config update of
$updatedConfigNames failed with class ${reconfigurable.getClass} due to: $e")
Review Comment:
This function already catches and re-throws any ConfigExceptions - so it
seems a bit unlikely re-throwing the other exceptions missed will return
config-related sensitive data.
However, to be safe
1. we could limit blast radius by additionally catching
IllegalStateException. I've filtered through all the impls of
`validateReconfiguration` quickly and this looks to be safe to do.
2. change impls of `validateReconfiguration(Map<String, ?> configs)` to
throw ConfigException where they might throw other exception types. e.g.
SaslChannelBuilder currently will throw `IllegalStateException` when the
SslFactory has not been configured yet - we could wrap this in a
ConfigException.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
