Bind to configured IP address on netty servers

[speevy]

Replace call to super on the NettyGrpcServer and ShaddedNettyGrpcServer newServerBuilder() method, with a call to the NettyServerBuilder.forAddress that allows defining the IP and port to be bound to using a SocketAddress.

The glue logic for getting the SocketAddress from the configured address string is in the DefaultGrpcServerFactory as is common to both inheritor classes.

Signed-off-by: Ivan Lorca [email protected]

[resolves #319]

[onobc]

Thanks for the bug find and contribution/fix @speevy !

I have a couple of comments / suggestions. I look forward to hearing back.

Thanks,
Chris

[onobc]

👋🏻 @speevy ,

Thanks for the quick turnaround! I almost forgot how much "fun" working w/ IPV6 is 😼 . I have some ideas that will simplify things I think. Please see comments.

Also, can you adjust the DCO signed-by as it was expecting "Ivan Lorca [email protected]" but got "Ivan Lorca [email protected]".

[onobc]

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.

[onobc]

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.

[speevy]

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.

[onobc]

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?

[onobc]

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.

[speevy]

👋🏻 @speevy ,

Also, can you adjust the DCO signed-by as it was expecting "Ivan Lorca [email protected]" but got "Ivan Lorca [email protected]".

The previous email is my company's, and it was inserted by the git commit -s, and I didn't notice. I'd prefer to use my personal email, that also is the one I have linked to my github account, if you don't mind.