dajac commented on a change in pull request #9628:
URL: https://github.com/apache/kafka/pull/9628#discussion_r538477244
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -864,13 +885,20 @@ object ConfigCommand extends Config {
}
}
+ if (hasEntityName && entityTypeVals.contains(ConfigType.Ip)) {
+ Seq(entityName,
ip).filter(options.has(_)).map(options.valueOf(_)).foreach(DynamicConfig.Ip.validateIpOrHost)
Review comment:
ditto here. It is a bit weird to have an `InvalidRequestException`
thrown here.
##########
File path: core/src/main/scala/kafka/server/DynamicConfig.scala
##########
@@ -141,6 +144,18 @@ object DynamicConfig {
def names = ipConfigs.names
def validate(props: Properties) = DynamicConfig.validate(ipConfigs, props,
customPropsAllowed = false)
+
+ def validateIpOrHost(ip: String): Unit = {
+ if (ip != ConfigEntityName.Default) {
+ if (!Utils.validHostPattern(ip))
+ throw new InvalidRequestException(s"$ip is not a valid hostname")
Review comment:
I wonder if this step is really necessary as `InetAddress.getByName`
also ensures that the hostname is valid. What do you think?
##########
File path: core/src/main/scala/kafka/server/DynamicConfig.scala
##########
@@ -141,6 +144,18 @@ object DynamicConfig {
def names = ipConfigs.names
def validate(props: Properties) = DynamicConfig.validate(ipConfigs, props,
customPropsAllowed = false)
+
+ def validateIpOrHost(ip: String): Unit = {
+ if (ip != ConfigEntityName.Default) {
+ if (!Utils.validHostPattern(ip))
+ throw new InvalidRequestException(s"$ip is not a valid hostname")
+ try {
+ InetAddress.getByName(ip)
+ } catch {
+ case _ :UnknownHostException => throw new
InvalidRequestException(s"$ip is not a valid IP or resolvable hostname")
Review comment:
nit: `_ :UnknownHostException` => `_: UnknownHostException`
##########
File path: core/src/main/scala/kafka/zk/AdminZkClient.scala
##########
@@ -394,8 +393,7 @@ class AdminZkClient(zkClient: KafkaZkClient) extends
Logging {
* @param configs properties to validate for the IP
*/
def validateIpConfig(ip: String, configs: Properties): Unit = {
- if (ip != ConfigEntityName.Default && !Utils.validHostPattern(ip))
- throw new AdminOperationException(s"IP $ip is not a valid address.")
+ DynamicConfig.Ip.validateIpOrHost(ip)
Review comment:
It is a little weird to throw `InvalidRequestException` here because
there is not request involved. Users of `AdminZkClient` may not expect this. I
think that it would be better to throw a `IllegalArgumentException`. Then in
the `AdminManager#alterClientQuotas`, we can catch it and transform it into
`InvalidRequestException` by copying the internal message.
----------------------------------------------------------------
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
