[GitHub] [kafka] YiDing-Duke commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
YiDing-Duke commented on a change in pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#discussion_r651361653
##
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##
@@ -174,11 +174,15 @@ public int hashCode() {
return result;
}
+/**
+ * Override toString to redact sensitive value.
+ * WARNING, user should be responsible to set the correct "IsSensitive"
field for each config entry.
Review comment:
Fixed.
--
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
[GitHub] [kafka] YiDing-Duke commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
YiDing-Duke commented on a change in pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#discussion_r651094578
##
File path: core/src/main/scala/kafka/server/DynamicBrokerConfig.scala
##
@@ -469,7 +469,7 @@ class DynamicBrokerConfig(private val kafkaConfig:
KafkaConfig) extends Logging
}
invalidProps.keys.foreach(props.remove)
val configSource = if (perBrokerConfig) "broker" else "default cluster"
-error(s"Dynamic $configSource config contains invalid values:
$invalidProps, these configs will be ignored", e)
+error(s"Dynamic $configSource config contains invalid values in:
${invalidProps.keys}, these configs will be ignored", e)
Review comment:
Hi @showuon, the concern here is Props are key value pairs passed in
from user. It's possible user puts sensitive or secret content in "value" that
we should not log onto disk. There is no single Config here to tell which key
may contain sensitive or secrets, so current solution is for invalid input, we
only print keys to give user hints and user can refer their "origins" to figure
out the reason.
--
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
[GitHub] [kafka] YiDing-Duke commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
YiDing-Duke commented on a change in pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#discussion_r651094578
##
File path: core/src/main/scala/kafka/server/DynamicBrokerConfig.scala
##
@@ -469,7 +469,7 @@ class DynamicBrokerConfig(private val kafkaConfig:
KafkaConfig) extends Logging
}
invalidProps.keys.foreach(props.remove)
val configSource = if (perBrokerConfig) "broker" else "default cluster"
-error(s"Dynamic $configSource config contains invalid values:
$invalidProps, these configs will be ignored", e)
+error(s"Dynamic $configSource config contains invalid values in:
${invalidProps.keys}, these configs will be ignored", e)
Review comment:
Props are key value pairs passed in from user. It's possible user puts
sensitive or secret content in "value" that we should not log onto disk. There
is no single Config here to tell which key may contain sensitive or secrets, so
current solution is for invalid input, we only print keys to give user hints
and user can refer their "origins" to figure out the reason.
--
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
[GitHub] [kafka] YiDing-Duke commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
YiDing-Duke commented on a change in pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#discussion_r651094904
##
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##
@@ -174,11 +174,15 @@ public int hashCode() {
return result;
}
+/**
+ * Override toString to redact sensitive value.
+ * WARNING, user should be responsible to set correct "senstive" field for
each config entry.
Review comment:
Good catch! Fixed as suggested.
--
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
[GitHub] [kafka] YiDing-Duke commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
YiDing-Duke commented on a change in pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#discussion_r651094578
##
File path: core/src/main/scala/kafka/server/DynamicBrokerConfig.scala
##
@@ -469,7 +469,7 @@ class DynamicBrokerConfig(private val kafkaConfig:
KafkaConfig) extends Logging
}
invalidProps.keys.foreach(props.remove)
val configSource = if (perBrokerConfig) "broker" else "default cluster"
-error(s"Dynamic $configSource config contains invalid values:
$invalidProps, these configs will be ignored", e)
+error(s"Dynamic $configSource config contains invalid values in:
${invalidProps.keys}, these configs will be ignored", e)
Review comment:
Props are key value pairs passed in from user. It's possible user put
sensitive or secret content in "values" that we should not log onto disk. There
is no a single Config here to tell which key may contain sensitive or secrets,
so current solution is for invalid input, we only print keys to give user hints
and user can refer their "origins" to figure out the reason.
--
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
