-
Notifications
You must be signed in to change notification settings - Fork 65
Bind to configured IP address on netty servers #320
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,12 @@ | |
|
|
||
| package org.springframework.grpc.internal; | ||
|
|
||
| import java.net.InetSocketAddress; | ||
| import java.net.SocketAddress; | ||
| import java.util.Objects; | ||
|
|
||
| import org.springframework.util.StringUtils; | ||
|
|
||
| /** | ||
| * Provides convenience methods for various gRPC functions. | ||
| * | ||
|
|
@@ -36,9 +42,21 @@ private GrpcUtils() { | |
| */ | ||
| public static int getPort(String address) { | ||
| String value = address; | ||
| if (value.contains(":")) { | ||
| long numberOfColons = countColons(address); | ||
| if (numberOfColons == 1) { | ||
| value = value.substring(value.lastIndexOf(":") + 1); | ||
| } | ||
| else if (numberOfColons > 1) { | ||
| if (address.startsWith("[")) { | ||
| var index = address.lastIndexOf("]:"); | ||
| if (index >= 0) { | ||
| value = address.substring(index + 2); | ||
| } | ||
| else { | ||
| return DEFAULT_PORT; | ||
| } | ||
| } | ||
| } | ||
| if (value.contains("/")) { | ||
| value = value.substring(0, value.indexOf("/")); | ||
| } | ||
|
|
@@ -51,4 +69,68 @@ public static int getPort(String address) { | |
| return DEFAULT_PORT; | ||
| } | ||
|
|
||
| public static long countColons(String address) { | ||
| return address.chars().filter(ch -> ch == ':').count(); | ||
| } | ||
|
|
||
| /** | ||
| * Gets the hostname from a given address. | ||
| * @param address a hostname/IPv4/IPv6/empty/* optionally with a port specification | ||
| * @return the hostname or an empty string | ||
| * @see <a href= | ||
| * "https://en.wikipedia.org/wiki/IPv6#Address_representation">https://en.wikipedia.org/wiki/IPv6#Address_representation</a> | ||
| */ | ||
| public static String getHostName(String address) { | ||
| String trimmedAddress = address.trim(); | ||
| long numberOfColons = countColons(trimmedAddress); | ||
|
|
||
| if (numberOfColons == 0) { | ||
| return trimmedAddress; | ||
| } | ||
|
|
||
| if (numberOfColons == 1) { // An IPv6 address mush have at least 2 colons, so is | ||
| // {IPv4 or hostname}:{port} | ||
| return trimmedAddress.split(":")[0].trim(); | ||
| } | ||
|
|
||
| if (numberOfColons > 8 || numberOfColons == 8 && !trimmedAddress.startsWith("[")) { | ||
| // On an IPv6 address a maximum of 7 colons are allowed + 1 for the port | ||
| // IPv6 addresses with port should have the format [{address}]:port | ||
| throw new IllegalArgumentException("Cannot parse address: " + trimmedAddress); | ||
| } | ||
|
|
||
| if (trimmedAddress.startsWith("[")) { | ||
| var index = trimmedAddress.lastIndexOf("]"); | ||
| if (index < 0) { | ||
| throw new IllegalArgumentException("Cannot parse address: " + trimmedAddress); | ||
| } | ||
| return trimmedAddress.substring(1, index); | ||
| } | ||
|
|
||
| return trimmedAddress; // IPv6 Address with no port specified | ||
| } | ||
|
|
||
| /** | ||
| * Gets a SocketAddress for the given address. | ||
| * | ||
| * If the address part is empty, * or :: a SocketAddress with wildcard address will be | ||
| * returned. If the port part is empty or missing, a SocketAddress for the gRPC | ||
| * default port (9090) will be returned. | ||
| * @param address a hostname/IPv4/IPv6/empty/* optionally with a port specification | ||
| * @return a SocketAddress representation for the given address | ||
| */ | ||
| public static SocketAddress getSocketAddress(String address) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving the whole concept of SocketAddress socketAddress = new InetSocketAddress(GrpcUtils.getHostName(address), GrpcUtils.getPort(address));
return NettyServerBuilder.forAddress(socketAddress, credentials());Wdyt? |
||
| if (address.startsWith("unix:")) { | ||
| throw new UnsupportedOperationException("Unix socket addresses not supported"); | ||
| } | ||
|
|
||
| var host = getHostName(address); | ||
| if (StringUtils.hasText(host) && !Objects.equals(host, "*") && !Objects.equals(host, "::")) { | ||
| return new InetSocketAddress(host, getPort(address)); | ||
| } | ||
| else { | ||
| return new InetSocketAddress(getPort(address)); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,8 +17,15 @@ | |
| package org.springframework.grpc.internal; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatExceptionOfType; | ||
|
|
||
| import java.net.InetSocketAddress; | ||
| import java.net.SocketAddress; | ||
| import java.util.List; | ||
|
|
||
| import org.junit.jupiter.api.DynamicTest; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.TestFactory; | ||
|
|
||
| class GrpcUtilsTests { | ||
|
|
||
|
|
@@ -56,4 +63,48 @@ void testGetInvalidAddress() { | |
| assertThat(GrpcUtils.getPort(address)).isEqualTo(9090); // -1? | ||
| } | ||
|
|
||
| @TestFactory | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming, we go w/ the URI for the parsing, here are some sample tests that I was playing around w/ using the new impl based on URI: @TestFactory
List<DynamicTest> getHostname() {
return List.of(
testGetHostname("localhost:9999", "localhost"),
testGetHostname("tcp://foo:9090", "foo"),
testGetHostname("zookeeper://zk.example.com:9900/example_service", "zk.example.com"),
testGetHostname("static://foo:9090", "foo"),
testGetHostname("dns://8.8.8.8/foo.googleapis.com", "8.8.8.8"),
testGetHostname("http://localhost", "localhost"),
testGetHostname("localhost", "localhost"),
testGetHostname("127.0.0.1", "127.0.0.1"),
testGetHostname("foo.googleapis.com:8080", "foo.googleapis.com"),
testGetHostname("127.0.0.1:8080", "127.0.0.1"),
testGetHostname("[::1]:8080", "[::1]"),
testGetHostname("[2001:db8:85a3:8d3:1319:8a2e:370:7348]", "[2001:db8:85a3:8d3:1319:8a2e:370:7348]"),
testGetHostname("[2001:db8:85a3:8d3:1319:8a2e:370:7348]:443", "[2001:db8:85a3:8d3:1319:8a2e:370:7348]"));
}
private DynamicTest testGetHostname(String address, String expectedHostname) {
return DynamicTest.dynamicTest("Address: " + address, () -> {
assertThat(GrpcUtils.getHostName(address)).isEqualTo(expectedHostname);
});
}You can see that eventhough the address may contain a scheme - we (via java.net.URI) can still parse the hostname out of it. I'm not sure that we should even allow passing an address that contains a scheme but it is flexible enough to support it. FWIW. |
||
| List<DynamicTest> ipAddress() { | ||
| return List.of(testIpAddress(":9999", new InetSocketAddress(9999)), | ||
| testIpAddress("localhost:9999", new InetSocketAddress("localhost", 9999)), | ||
| testIpAddress("localhost", new InetSocketAddress("localhost", 9090)), | ||
| testIpAddress("127.0.0.1", new InetSocketAddress("127.0.0.1", 9090)), | ||
| testIpAddress("127.0.0.1:8888", new InetSocketAddress("127.0.0.1", 8888)), | ||
| testIpAddress("*", new InetSocketAddress(9090)), testIpAddress("*:8888", new InetSocketAddress(8888)), | ||
| testIpAddress("", new InetSocketAddress(9090)), | ||
| // IPv6 cases. See | ||
| // https://en.wikipedia.org/wiki/IPv6#Address_representation | ||
| testIpAddress("[::]:8888", new InetSocketAddress(8888)), | ||
| testIpAddress("::", new InetSocketAddress(9090)), testIpAddress("[::]", new InetSocketAddress(9090)), | ||
| testIpAddress("::1", new InetSocketAddress("::1", 9090)), | ||
| testIpAddress("[::1]", new InetSocketAddress("::1", 9090)), | ||
| testIpAddress("[::1]:9999", new InetSocketAddress("::1", 9999)), | ||
| testIpAddress("2001:db8::ff00:42:8329", new InetSocketAddress("2001:db8::ff00:42:8329", 9090)), | ||
| testIpAddress("[2001:db8::ff00:42:8329]", new InetSocketAddress("2001:db8::ff00:42:8329", 9090)), | ||
| testIpAddress("[2001:db8::ff00:42:8329]:9999", new InetSocketAddress("2001:db8::ff00:42:8329", 9999)), | ||
| testIpAddress("::ffff:192.0.2.128", new InetSocketAddress("::ffff:192.0.2.128", 9090)), | ||
| testIpAddress("[::ffff:192.0.2.128]", new InetSocketAddress("::ffff:192.0.2.128", 9090)), | ||
| testIpAddress("[::ffff:192.0.2.128]:9999", new InetSocketAddress("::ffff:192.0.2.128", 9999))); | ||
| } | ||
|
|
||
| private DynamicTest testIpAddress(String address, SocketAddress expected) { | ||
| return DynamicTest.dynamicTest("Socket address: " + address, () -> { | ||
| assertThat(GrpcUtils.getSocketAddress(address)).isEqualTo(expected); | ||
| }); | ||
| } | ||
|
|
||
| @TestFactory | ||
| List<DynamicTest> unsupportedAddress() { | ||
| return List.of(testThrows("unix:dummy", UnsupportedOperationException.class), | ||
| testThrows("0:1:2:3:4:5:6:7:8:9", IllegalArgumentException.class), | ||
| testThrows("[0:1:2:3:4:5:6:7:8:9]", IllegalArgumentException.class), | ||
| testThrows("[0:1:2:3:4:5:6:7:8]:9", IllegalArgumentException.class), | ||
| testThrows("[0:1:2:3:4:5:6:7]:8:9", IllegalArgumentException.class)); | ||
| } | ||
|
|
||
| private DynamicTest testThrows(String address, Class<? extends Exception> expectedException) { | ||
| return DynamicTest.dynamicTest("Socket address: " + address, () -> assertThatExceptionOfType(expectedException) | ||
| .isThrownBy(() -> GrpcUtils.getSocketAddress(address))); | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,7 +99,15 @@ public void setAddress(@Nullable String address) { | |
| * @return the address to bind to | ||
| */ | ||
| public String determineAddress() { | ||
| return (this.address != null) ? this.address : this.host + ":" + this.port; | ||
| return (this.address != null) ? this.address : getAddressFromHostAndPort(); | ||
| } | ||
|
|
||
| private String getAddressFromHostAndPort() { | ||
| return isIpV6() ? "[%s]:%d".formatted(this.host, this.port) : this.host + ":" + this.port; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we go w/ the tightened requirements that IPV6 host must be enclosed in |
||
| } | ||
|
|
||
| private boolean isIpV6() { | ||
| return GrpcUtils.countColons(this.host) >= 2; | ||
| } | ||
|
|
||
| public String getHost() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In looking into the updates, it reminded me of how crazy parsing IPV6 addresses can be. It also made me look for something already available that will handle this for us. I found my way to java.net.URI#new(String) which can do this parsing for us. It does tighten the requirements for IPV6 addresses as follows:
I am ok going w/ this tighter form of IPV6 host/address because really we do not need to support every variation and I expect most users to go w/ the default
*like gRPC Java does by default (only uses port).If we are ok w/ the above, then I think we can replace the current getHostname w/ something super simple like the following:
I apologize for pointing you in the direction of parsing the addresses - I know you spent time w/ the current updates. I think this is just one of those cases where you have to go down one path to understand it and seek alternatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a great idea, but in a second thought I'm afraid it may cause some problems when the defaults should be applied for either host or port or both.
I guess that for most cases neither address, not host not port will be defined, an in this case it should bind to *:9090.
I haven't done any tests, and I'm afraid I'll not have spare time in the few next days to deep dive into that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries @speevy , you have given me enough inspiration and already laid the ground work that I can take this from here and run w/ it. Thank you. I will be sure that we are co-authors on the final result though. I will also ping you for a code review of whatever I come up w/ .