Add peer host and port info for server SslHandler (4.x)

[ben1222]

Fixes #5290 , on 4.x branch.

[vietj]

looks good but I'd prefer use vertx type HostAndPort which is more suited for this internally

[ben1222]

@vietj Hmm... I'm not sure if it worth to do this extra conversion... it adds overhead...
What's the benefit? Holding minimal required data? What else?

Anyway, I'll try to use HostAndPort and update PR for you to review.

[ben1222]

@vietj I updated the PR to use HostAndPort, please review again.

[ben1222]

Hmm... looks like the vertx gen failed... sorry that I didn't notice it when testing in my IDE... seems it didn't do the vertx gen...
I added @GenIgnore(GenIgnore.PERMITTED_TYPE) on HostAndPort.fromSocketAddress to fix that problem.
@vietj please review again.

[ben1222]

@vietj Could you review? If it looks good to you, I'll update the PR for master branch to also use the HostAndPort.

[vietj]

this should use instead directly inet socket address and not assume we accept any socket address and instead return null

[ben1222]

@vietj This method is added to simplify the caller (no need to add if-else to check if the SocketAddress is an InetSocketAddress in caller side)
If making the parameter to be InetSocketAddress then this method is less useful and I'd rather move this method to somewhere else.
(I'm still thinking passing InetSocketAddress directly to SslChannelProvider is the simplest and having minimal overhead)
What do you think?

[vietj]

the issue is that we prefer to avoid methods that perform the null check, the caller should not rely on the method to return null when passing an inapropriate argument, instead it should check the argument and perform the work itself, eventually it can be in an internal util class, but we should not encourage nor promote this on a public API.

[ben1222]

@vietj You didn't object my previous comment #5346 (comment) about returning the nullable on public API so I assumed you agree on that:

If use HostAndPort, I may need to add a conversion method like @Nullable static HostAndPort socketHostAndPort(SocketAddress address); to simplify the conversion, is it OK to put it in HostAndPort interface (which will become a new external API)?

I don't understand your expectation and reason behind it now.
You asked to use HostAndPort instead of SocketAddress, while not allowing adding a method that returns nullable for public API.
If like you said, the caller should check the argument, then it will become:

// in HttpServerWorker.configurePipeline
SocketAddress remoteAddress = ch.remoteAddress();
HostAndPort remoteHostAndPort = (remoteAddress instanceof InetSocketAddress ? HostAndPort.fromSocketAddress(remoteAddress) : null);
pipeline.addLast("ssl", sslChannelProvider.createServerHandler(remoteHostAndPort));

// in NetServerImpl.NetServerWorker.configurePipeline
SocketAddress remoteAddress = ch.remoteAddress();
HostAndPort remoteHostAndPort = (remoteAddress instanceof InetSocketAddress ? HostAndPort.fromSocketAddress(remoteAddress) : null);
ch.pipeline().addLast("ssl", sslChannelProvider.createServerHandler(remoteHostAndPort));

// in NetSocketImpl.upgradeToSsl
SocketAddress remoteAddress = chctx.channel().remoteAddress();
HostAndPort remoteHostAndPort = (remoteAddress instanceof InetSocketAddress ? HostAndPort.fromSocketAddress(remoteAddress) : null);
sslHandler = sslChannelProvider.createServerHandler(remoteHostAndPort);

This will result in duplicated code and increase maintenance effort, so I don't think it is a good option.

Another option may be moving this method to an internal API, e.g., move to SslChannelProvider or other internal util class.
If moving to SslChannelProvider, then why do we need brother changing public ChannelHandler createServerHandler(java.net.SocketAddress remoteAddress) to public ChannelHandler createServerHandler(HostAndPort remoteAddress)?
If move to another internal util class, could you advice a proper place to put this SocketAddress to HostAndPort conversion method that may return null?