You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/06/08 17:45:48 UTC
[GitHub] [kafka] YiDing-Duke opened a new pull request #10843: Minor: Log formatting for exceptions during configuration related operations
YiDing-Duke opened a new pull request #10843:
URL: https://github.com/apache/kafka/pull/10843
Format configuration logging during exceptions or errors.
Also make sure it redacts sensitive information or unknown values.
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#discussion_r651354059
##########
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:
SGTM! Thanks.
--
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:
users@infra.apache.org
[GitHub] [kafka] dajac merged pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
dajac merged pull request #10843:
URL: https://github.com/apache/kafka/pull/10843
--
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:
users@infra.apache.org
[GitHub] [kafka] YiDing-Duke commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [kafka] YiDing-Duke commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#discussion_r651354743
##########
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:
Nit: `isSensitive`
--
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:
users@infra.apache.org
[GitHub] [kafka] YiDing-Duke commented on pull request #10843: Minor: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
YiDing-Duke commented on pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#issuecomment-856973287
@dajac @kowshik @showuon could you help review this PR? thank you!
--
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:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#discussion_r650888452
##########
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:
Why should we log only `invalidProps.keys` here? I think we should log `key:value` pair to users to know what invalid key and value they provided. What do you think?
Also same questions to above 2 changes. Thanks.
##########
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:
**sensitive** is typo. Also, maybe we can change this sentense:
user should be responsible to set correct "senstive" field for each config entry.
-> user should be responsible to set **the** correct **"isSensitive"** field for each config entry.
##########
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.
+ */
@Override
public String toString() {
return "ConfigEntry(" +
"name=" + name +
- ", value=" + value +
+ ", value=" + (isSensitive ? "Redacted" : value) +
Review comment:
Nice fix!
--
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:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#issuecomment-861202533
@YiDing-Duke Thanks for the patch! Merging to trunk and 2.8.
--
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:
users@infra.apache.org
[GitHub] [kafka] YiDing-Duke commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [kafka] YiDing-Duke commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#issuecomment-860910847
@YiDing-Duke Thanks for the update.
@showuon Does the PR look good to you now?
--
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:
users@infra.apache.org
[GitHub] [kafka] YiDing-Duke commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [kafka] YiDing-Duke commented on pull request #10843: MINOR: Log formatting for exceptions during configuration related operations
Posted by GitBox <gi...@apache.org>.
YiDing-Duke commented on pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#issuecomment-861078081
@dajac we are good to go.
--
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:
users@infra.apache.org