Skip to content
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

Introduce DynamicAddressConnectHandler which can be used to dynamically change remoteAddress / localAddress when a connect is issued #8982

Merged
merged 1 commit into from Apr 30, 2019

Conversation

Projects
None yet
5 participants
@normanmaurer
Copy link
Member

commented Mar 26, 2019

Motivation:

Bootstrap allows you to set a localAddress for outbound TCP connections, either via the Bootstrap.localAddress(localAddress) or Bootstrap.connect(remoteAddress, localAddress) methods. This works well if you want to bind to just one IP address on an interface. Sometimes you want to bind to a specific address based on the resolved remote address which should be possible.

Modifications:

Add DynamicAddressConnectHandler and tests

Result:

Fixes #8940.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

@loganrosen PTAL...

@trustin @carl-mastrangelo @ejona86 I still need to add some more tests but I just wanted to show the idea first.

@loganrosen
Copy link

left a comment

This is awesome, thank you!

@ejona86

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

@normanmaurer, I'm a bit confused by this, since remoteAddress has to be resolved (once it arrives at the Channel, so while a handler could swap in a resolved address that doesn't seem like the use-case).

@normanmaurer normanmaurer force-pushed the dynamic_local_address branch from f9f0581 to 88864e5 Apr 8, 2019

@normanmaurer normanmaurer requested a review from trustin Apr 8, 2019

@normanmaurer normanmaurer force-pushed the dynamic_local_address branch 2 times, most recently from 333652f to d26fb8d Apr 8, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@loganrosen PTAL again as well

@ejona86

ejona86 approved these changes Apr 9, 2019

Copy link
Member

left a comment

This looks a lot better than DynamicLocalSocketAddress. I have some reservation about this since the function is called from a "random" thread and it could be achieved via a ChannelHandler. But I do agree it makes a case easier and it isn't that large of an API.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

@ejona86 using another handler is actually not a bad Idea at all. I just did not think of this solution.

@loganrosen what do you think about just doing what @ejona86 suggested here and close this PR ?

The handler would look like this:

public class DynamicLocalAddressHandler extends ChannelOutboundHandlerAdapter {

    @Override
    public void connect(ChannelHandlerContext ctx, SocketAddress remoteAddress, SocketAddress localAddress, ChannelPromise promise) throws Exception {
        // localAddress should be null if you not specify it when calling Bootstrap.connect(...)
        assert localAddress == null;
        try {
            ctx.connect(remoteAddress, selectLocalAddress(remoteAddress), promise);
        } catch (Throwable cause) {
            promise.setFailure(cause);
        }
    }
    
    private SocketAddress selectLocalAddress(SocketAddress remoteAddress) {
        // TODO: Implement me!
        return null;
    }
}
@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

@trustin I start to wonder now if we should just not merge this and let the user handle it in the ChannelOutboundHandler as I did show above. WDYT ?

All in all this seems a bit redundant

@trustin

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Should we provide such a handler out of the box?

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

@trustin sure ... I can add it to the handlers package.

@carl-mastrangelo
Copy link
Member

left a comment

LGTM (with one comment)

@loganrosen

This comment has been minimized.

Copy link

commented Apr 16, 2019

@normanmaurer Sorry for the delay in getting back — I was on vacation. I definitely think the handler implementation is cleaner, and it would be great to have it provided out of the box.

@normanmaurer normanmaurer force-pushed the dynamic_local_address branch from d26fb8d to 03fc89a Apr 16, 2019

@normanmaurer normanmaurer changed the title Allow to dynamically choose local address during bootstrap based on r… Introduce DynamicAddressConnectHandler which can be used to dynamically change remoteAddress / localAddress when a connect is issued Apr 16, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

@normanmaurer normanmaurer requested review from ejona86 and trustin and removed request for ejona86 Apr 16, 2019

@normanmaurer normanmaurer requested review from ejona86, carl-mastrangelo and trustin and removed request for trustin Apr 16, 2019

@normanmaurer normanmaurer force-pushed the dynamic_local_address branch from 03fc89a to bce5a04 Apr 16, 2019

Introduce DynamicAddressConnectHandler which can be used to dynamical…
…ly change remoteAddress / localAddress when a connect is issued

Motivation:

Bootstrap allows you to set a localAddress for outbound TCP connections, either via the Bootstrap.localAddress(localAddress) or Bootstrap.connect(remoteAddress, localAddress) methods. This works well if you want to bind to just one IP address on an interface. Sometimes you want to bind to a specific address based on the resolved remote address which should be possible.

Modifications:

Add DynamicAddressConnectHandler and tests

Result:

Fixes #8940.

@normanmaurer normanmaurer force-pushed the dynamic_local_address branch from bce5a04 to 6408a8b Apr 18, 2019

@trustin
Copy link
Member

left a comment

LGTM. Thanks!

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

@netty-bot test this please

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

@loganrosen WDYT ?

@loganrosen

This comment has been minimized.

Copy link

commented Apr 30, 2019

This looks great. We can just extend this class and add our interface binding logic there. Thanks @normanmaurer!

@normanmaurer normanmaurer merged commit 0c114da into 4.1 Apr 30, 2019

2 of 3 checks passed

pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details

@normanmaurer normanmaurer deleted the dynamic_local_address branch Apr 30, 2019

normanmaurer added a commit that referenced this pull request Apr 30, 2019

Introduce DynamicAddressConnectHandler which can be used to dynamical…
…ly change remoteAddress / localAddress when a connect is issued (#8982)

Motivation:

Bootstrap allows you to set a localAddress for outbound TCP connections, either via the Bootstrap.localAddress(localAddress) or Bootstrap.connect(remoteAddress, localAddress) methods. This works well if you want to bind to just one IP address on an interface. Sometimes you want to bind to a specific address based on the resolved remote address which should be possible.

Modifications:

Add DynamicAddressConnectHandler and tests

Result:

Fixes #8940.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.