tombentley commented on a change in pull request #10530:
URL: https://github.com/apache/kafka/pull/10530#discussion_r616468886
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java
##########
@@ -369,9 +370,31 @@ else if (serverConnector != null &&
serverConnector.getHost() != null && serverC
else if (serverConnector != null && serverConnector.getPort() > 0)
builder.port(serverConnector.getPort());
- log.info("Advertised URI: {}", builder.build());
+ URI uri = builder.build();
+ validateUriHost(uri);
+ log.info("Advertised URI: {}", uri);
- return builder.build();
+ return uri;
+ }
+
+ /**
+ * Parses the uri and throws a more definitive error
+ * when the internal node to node communication can't happen due to an
invalid host name.
+ */
+ static void validateUriHost(URI uri) {
+ if (uri.getHost() == null) {
+ String host = Utils.getHost(uri.getAuthority());
+ String errorMsg = "Could not parse host from advertised URL=" +
uri.toString();
Review comment:
We don't tend to use `=` in exception messages like this, so perhaps
it's better to format the String something link `"Could not parse host '%s'
from advertised URL"`
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java
##########
@@ -369,9 +370,31 @@ else if (serverConnector != null &&
serverConnector.getHost() != null && serverC
else if (serverConnector != null && serverConnector.getPort() > 0)
builder.port(serverConnector.getPort());
- log.info("Advertised URI: {}", builder.build());
+ URI uri = builder.build();
+ validateUriHost(uri);
+ log.info("Advertised URI: {}", uri);
- return builder.build();
+ return uri;
+ }
+
+ /**
+ * Parses the uri and throws a more definitive error
+ * when the internal node to node communication can't happen due to an
invalid host name.
+ */
+ static void validateUriHost(URI uri) {
+ if (uri.getHost() == null) {
+ String host = Utils.getHost(uri.getAuthority());
+ String errorMsg = "Could not parse host from advertised URL=" +
uri.toString();
+ if (host != null) {
+ try {
+ IDN.toASCII(host, IDN.USE_STD3_ASCII_RULES);
+ } catch (IllegalArgumentException e) {
+ errorMsg += ", as it doesn't conform to RFC 1123
specification, reason=" + e.getMessage();
Review comment:
Same comment about `=`, here I'd write `", as it doesn't conform to RFC
1123 specification: " + e.getMessage();`
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/RestServerTest.java
##########
@@ -185,6 +189,25 @@ public void testAdvertisedUri() {
Assert.assertEquals("http://plaintext-localhost:4761/";,
server.advertisedUrl().toString());
}
+ @Test
+ public void testValidateUriHost() {
+ validateUriHost(URI.create("http://localhost:8080";));
+ validateUriHost(URI.create("http://172.217.2.110:80";));
+ validateUriHost(URI.create("http://[2607:f8b0:4006:818::2004]:80";));
Review comment:
Could we add some valid IDN hostnames here too?
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java
##########
@@ -369,9 +370,31 @@ else if (serverConnector != null &&
serverConnector.getHost() != null && serverC
else if (serverConnector != null && serverConnector.getPort() > 0)
builder.port(serverConnector.getPort());
- log.info("Advertised URI: {}", builder.build());
+ URI uri = builder.build();
+ validateUriHost(uri);
+ log.info("Advertised URI: {}", uri);
- return builder.build();
+ return uri;
+ }
+
+ /**
+ * Parses the uri and throws a more definitive error
+ * when the internal node to node communication can't happen due to an
invalid host name.
+ */
+ private void validateUriHost(URI uri) {
+ if (uri.getHost() == null) {
+ String host = Utils.getHost(uri.getAuthority());
Review comment:
I do wonder whether it would be simpler to just validate the given
`REST_ADVERTISED_HOST_NAME_CONFIG` around line 362. That way we don't need to
mess around with URIs and are just validating what's been supplied.
If we do need to keep this stuff with URIs, I think this is worth a comment
to explain that `uri.getHost()` will only be null if the host was undefined or
the URI couldn't be parsed as a URI containing a host. Also, it's possible that
`getAuthority` can return null, so best to check it.
--
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
