Skip to content

MINOR: Log formatting for exceptions during configuration related operations #10843

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dajac merged 2 commits into from
Jun 15, 2021
Merged

MINOR: Log formatting for exceptions during configuration related operations #10843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa66075
Minor: Log formatting for exceptions during alter/update config
YiDing-Duke Jun 4, 2021
3ea2e02
Minor: Log formatting for exception during reconfiguration step in dy…
YiDing-Duke Jun 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
@Override
public String toString() {
return "ConfigEntry(" +
"name=" + name +
", value=" + value +
", value=" + (isSensitive ? "Redacted" : value) +

@showuon showuon Jun 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix!

", source=" + source +
", isSensitive=" + isSensitive +
", isReadOnly=" + isReadOnly +
Expand Down
9 changes: 5 additions & 4 deletions core/src/main/scala/kafka/server/DynamicBrokerConfig.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ class DynamicBrokerConfig(private val kafkaConfig: KafkaConfig) extends Logging
dynamicBrokerConfigs ++= props.asScala
updateCurrentConfig()
} catch {
case e: Exception => error(s"Per-broker configs of $brokerId could not be applied: $persistentProps", e)
case e: Exception => error(s"Per-broker configs of $brokerId could not be applied: ${persistentProps.keys()}", e)
}
}

Expand All @@ -306,7 +306,7 @@ class DynamicBrokerConfig(private val kafkaConfig: KafkaConfig) extends Logging
dynamicDefaultConfigs ++= props.asScala
updateCurrentConfig()
} catch {
case e: Exception => error(s"Cluster default configs could not be applied: $persistentProps", e)
case e: Exception => error(s"Cluster default configs could not be applied: ${persistentProps.keys()}", e)
}
}

Expand Down Expand Up @@ -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)

@showuon showuon Jun 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@YiDing-Duke YiDing-Duke Jun 14, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@showuon showuon Jun 15, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM! Thanks.

}
}

Expand Down Expand Up @@ -555,7 +555,8 @@ class DynamicBrokerConfig(private val kafkaConfig: KafkaConfig) extends Logging
} catch {
case e: Exception =>
if (!validateOnly)
error(s"Failed to update broker configuration with configs : ${newConfig.originalsFromThisConfig}", e)
error(s"Failed to update broker configuration with configs : " +
s"${ConfigUtils.configMapToRedactedString(newConfig.originalsFromThisConfig, KafkaConfig.configDef)}", e)
throw new ConfigException("Invalid dynamic configuration", e)
}
}
Expand Down
6 changes: 5 additions & 1 deletion core/src/main/scala/kafka/server/ZkAdminManager.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,8 +415,12 @@ class ZkAdminManager(val config: KafkaConfig,
info(message)
resource -> ApiError.fromThrowable(new InvalidRequestException(message, e))
case e: Throwable =>
val configProps = new Properties
config.entries.asScala.filter(_.value != null).foreach { configEntry =>
configProps.setProperty(configEntry.name, configEntry.value)
}
// Log client errors at a lower level than unexpected exceptions
val message = s"Error processing alter configs request for resource $resource, config $config"
val message = s"Error processing alter configs request for resource $resource, config ${toLoggableProps(resource, configProps).mkString(",")}"
if (e.isInstanceOf[ApiException])
info(message, e)
else
Expand Down