Re: [PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
cmccabe merged PR #16346: URL: https://github.com/apache/kafka/pull/16346 -- 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
Re: [PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
ahuang98 commented on PR #16346: URL: https://github.com/apache/kafka/pull/16346#issuecomment-2229397339 7 test failures, they look unrelated https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-16346/5/tests -- 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
Re: [PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
ahuang98 commented on code in PR #16346:
URL: https://github.com/apache/kafka/pull/16346#discussion_r1672981769
##
clients/src/main/java/org/apache/kafka/common/network/SaslChannelBuilder.java:
##
@@ -190,9 +191,13 @@ public Set reconfigurableConfigs() {
}
@Override
-public void validateReconfiguration(Map configs) {
+public void validateReconfiguration(Map configs) throws
ConfigException {
if (this.securityProtocol == SecurityProtocol.SASL_SSL)
-sslFactory.validateReconfiguration(configs);
+try {
+sslFactory.validateReconfiguration(configs);
+} catch (IllegalStateException e) {
+throw new ConfigException("SASL reconfiguration failed due to
" + e);
Review Comment:
I'm concatenating the "SASL reconfiguration failed" message with the
underlying exception string now since it looks like `ConfigException` will
print errors in a very specific way
--
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
Re: [PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
rajinisivaram commented on code in PR #16346:
URL: https://github.com/apache/kafka/pull/16346#discussion_r1670076722
##
clients/src/main/java/org/apache/kafka/common/security/ssl/SslFactory.java:
##
@@ -113,8 +113,12 @@ public Set reconfigurableConfigs() {
}
@Override
-public void validateReconfiguration(Map newConfigs) {
-createNewSslEngineFactory(newConfigs);
+public void validateReconfiguration(Map newConfigs) throws
ConfigException {
+try {
+createNewSslEngineFactory(newConfigs);
+} catch (IllegalStateException e) {
+throw new ConfigException("", e);
Review Comment:
As above, should add an exception message?
##
clients/src/main/java/org/apache/kafka/common/network/SaslChannelBuilder.java:
##
@@ -190,9 +191,13 @@ public Set reconfigurableConfigs() {
}
@Override
-public void validateReconfiguration(Map configs) {
+public void validateReconfiguration(Map configs) throws
ConfigException {
if (this.securityProtocol == SecurityProtocol.SASL_SSL)
-sslFactory.validateReconfiguration(configs);
+try {
+sslFactory.validateReconfiguration(configs);
+} catch (IllegalStateException e) {
+throw new ConfigException("", e);
Review Comment:
Should we add some exception message to say SASL reconfiguration failed?
--
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
Re: [PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
ahuang98 commented on code in PR #16346:
URL: https://github.com/apache/kafka/pull/16346#discussion_r1644791643
##
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:
@cmccabe @rajinisivaram what do you think about what I suggested in my
earlier comment?
https://github.com/apache/kafka/pull/16346#discussion_r1643521884
i.e. additionally catching IllegalStateExceptions or wrapping some error
cases in ConfigException so the stacktrace is preserved properly?
--
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
Re: [PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
rajinisivaram commented on code in PR #16346:
URL: https://github.com/apache/kafka/pull/16346#discussion_r1644085534
##
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:
We have fixed all known issues with leaked credentials. But we should still
be careful about including exception strings related to configuration, where
the exception could be from libraries and we are not sure of what they contain,
until we have verified that callers have already sanitized these.
--
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
Re: [PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
cmccabe commented on code in PR #16346:
URL: https://github.com/apache/kafka/pull/16346#discussion_r1643559385
##
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:
Putting a password string into an exception is certainly a bug. If we're
doing that, let's file a JIRA for it and fix ASAP.
@rajinisivaram : Do you have an example of code doing this?
There are a limited number of reconfigurables that deal with SASL / SSL so
we should be able to look at them all and fix them if needed.
--
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
Re: [PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
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 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
Re: [PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
ahuang98 commented on code in PR #16346:
URL: https://github.com/apache/kafka/pull/16346#discussion_r1643505085
##
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:
Thanks for pointing this out, I'll make this more specific to the error case
I'm trying to catch or change the error case to throw ConfigException instead
##
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:
Thanks for pointing this out, I'll make this more specific to the error case
I'm trying to catch or change the error case to throw ConfigException instead
--
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
Re: [PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
ahuang98 commented on code in PR #16346:
URL: https://github.com/apache/kafka/pull/16346#discussion_r1643505085
##
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:
Thanks for pointing this out, perhaps I'll make this more specific to the
error I'm trying to catch
--
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
Re: [PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
rajinisivaram commented on code in PR #16346:
URL: https://github.com/apache/kafka/pull/16346#discussion_r1642352229
##
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:
Some config exceptions may contain sensitive data like SASL passwords or SSL
info. We need to make sure the exception string doesn't include any sensitive
information, especially if it could be sent to clients.
--
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
[PR] MINOR: Don't swallow validateReconfiguration exceptions [kafka]
ahuang98 opened a new pull request, #16346: URL: https://github.com/apache/kafka/pull/16346 We should print exception information ### Committer Checklist (excluded from commit message) - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes) -- 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
