Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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("/"));
}
Expand All @@ -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) {
Copy link
Contributor

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:

IPv6 addresses are permitted for the host component. An IPv6 address must be enclosed in square brackets ('[' and ']') as specified by RFC 2732 . The IPv6 address itself must parse according to RFC 2373 . IPv6 addresses are further constrained to describe no more than sixteen bytes of address information, a constraint implicit in RFC 2373 but not expressible in the grammar.

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:

public static @Nullable String getHostName(String address) {
	if (address.startsWith("unix:")) {
		throw new UnsupportedOperationException("Unix socket addresses not supported");
	}
	// Fake a schema in cases where the address is scheme-less
	if (!address.contains("://")) {
		address = "http://" + address;
	}
	URI parsedUri;
	try {
		parsedUri = new URI(address);
	}
	catch (URISyntaxException ex) {
		throw new IllegalArgumentException("Cannot parse address '%s' due to: %s".formatted(address, ex.getMessage()), ex);
	}

	return parsedUri.getHost();
}

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.

Copy link
Author

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.

Copy link
Contributor

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/ .

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving the whole concept of SocketAddress out of the utils and have the server factory worry about that may keep the utils lighter and more focused, something like:

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
Expand Up @@ -23,6 +23,8 @@

import org.jspecify.annotations.Nullable;

import org.springframework.grpc.internal.GrpcUtils;

import io.grpc.TlsServerCredentials.ClientAuth;
import io.grpc.netty.NettyServerBuilder;
import io.netty.channel.MultiThreadIoEventLoopGroup;
Expand Down Expand Up @@ -56,7 +58,7 @@ protected NettyServerBuilder newServerBuilder() {
.bossEventLoopGroup(new MultiThreadIoEventLoopGroup(1, EpollIoHandler.newFactory()))
.workerEventLoopGroup(new MultiThreadIoEventLoopGroup(EpollIoHandler.newFactory()));
}
return super.newServerBuilder();
return NettyServerBuilder.forAddress(GrpcUtils.getSocketAddress(address()), credentials());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@

import org.jspecify.annotations.Nullable;

import org.springframework.grpc.internal.GrpcUtils;

import io.grpc.TlsServerCredentials.ClientAuth;
import io.grpc.netty.shaded.io.grpc.netty.NettyServerBuilder;
import io.grpc.netty.shaded.io.netty.channel.epoll.EpollEventLoopGroup;
Expand Down Expand Up @@ -55,7 +57,7 @@ protected NettyServerBuilder newServerBuilder() {
.bossEventLoopGroup(new EpollEventLoopGroup(1))
.workerEventLoopGroup(new EpollEventLoopGroup());
}
return super.newServerBuilder();
return NettyServerBuilder.forAddress(GrpcUtils.getSocketAddress(address()), credentials());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -56,4 +63,48 @@ void testGetInvalidAddress() {
assertThat(GrpcUtils.getPort(address)).isEqualTo(9090); // -1?
}

@TestFactory
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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 [ ] then we can just concat the host and port w/o having to validate here.

}

private boolean isIpV6() {
return GrpcUtils.countColons(this.host) >= 2;
}

public String getHost() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@

import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

import org.springframework.boot.context.properties.bind.Binder;
import org.springframework.boot.context.properties.source.MapConfigurationPropertySource;
Expand Down Expand Up @@ -187,6 +189,28 @@ void addressTakesPrecedenceOverHostAndPort() {
assertThat(properties.getAddress()).isEqualTo("my-server-ip:3130");
}

@ParameterizedTest
@ValueSource(strings = { "dummy.springframework.org", "127.0.0.1", "0.0.0.0", "192.168.1.2" })
void hostnameOrIpv4HostAndPort(String hostName) {
Map<String, String> map = new HashMap<>();
map.put("spring.grpc.server.host", hostName);
map.put("spring.grpc.server.port", "1234");
GrpcServerProperties properties = bindProperties(map);
assertThat(properties.getAddress()).isNullOrEmpty();
assertThat(properties.determineAddress()).isEqualTo(hostName + ":1234");
}

@ParameterizedTest
@ValueSource(strings = { "::", "1:2:3:4:5:6:7:8", "::1" })
void ipv6HostAndPort(String ipv6Address) {
Map<String, String> map = new HashMap<>();
map.put("spring.grpc.server.host", ipv6Address);
map.put("spring.grpc.server.port", "1234");
GrpcServerProperties properties = bindProperties(map);
assertThat(properties.getAddress()).isNullOrEmpty();
assertThat(properties.determineAddress()).isEqualTo("[" + ipv6Address + "]:1234");
}

}

}
Loading