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

Port of publishAddress should match port of corresponding boundAddress #14535

Merged
merged 1 commit into from Nov 6, 2015

Conversation

Projects
None yet
4 participants
@ywelsch
Copy link
Contributor

ywelsch commented Nov 4, 2015

Relates to #14503, #14514, #14513.

I refactored the code a bit to make it easier to understand.

@nik9000

View changes

core/src/test/java/org/elasticsearch/transport/netty/NettyTransportPublishAddressIT.java Outdated
}
}
assertTrue("Could not find a boundAddress matching publishAddress", foundMatch);
}

This comment has been minimized.

Copy link
@nik9000

nik9000 Nov 4, 2015

Contributor

This loses the check that the ports bind as commented above that I had in nik9000@b171c84

I get that that test wouldn't work on machines that only have ipv4 enabled - maybe we can just skip the test on those machines rather than not test that we successfully created the broken bindings?

This comment has been minimized.

Copy link
@nik9000

nik9000 Nov 4, 2015

Contributor

Don't get me wrong - I like the loop above - I just don't think is sufficient to prove to ourselves that we recreated the problem.

This comment has been minimized.

Copy link
@ywelsch

ywelsch Nov 5, 2015

Author Contributor

I changed the assertions in the test to make them closer to the original test you showed me. However, some assertions cannot be added without making the test flaky. For example, we cannot guarantee that port of ipv6 and ipv4 for bothNode is different (As there are other integration tests binding to ports as well).

@nik9000

This comment has been minimized.

Copy link
Contributor

nik9000 commented Nov 4, 2015

@rmuir knows this code better than I and should have a look as well.

Note that this isn't a big refactoring of publish_address, just an attempt to pick a sane one when we bind to different ports of different interfaces.

@ywelsch

This comment has been minimized.

Copy link
Contributor Author

ywelsch commented Nov 5, 2015

@rmuir Can you help reviewing this?

@rmuir

This comment has been minimized.

Copy link
Contributor

rmuir commented Nov 5, 2015

Yeah, give me a little time. I need time as I have to re-learn all the horrible transport profiles code: which is at the root cause of all this.

if (logger.isDebugEnabled()) {
logger.debug("Bound profile [{}] to address {{}}", name, NetworkAddress.format(boundSocket.get()));
// if port still not matches, just take port of first bound address
if (publishPort == null) {

This comment has been minimized.

Copy link
@rmuir

rmuir Nov 5, 2015

Contributor

If its unspecified, and we automatically created the publish address, then the only situation where we should not find a matching publish address should be wildcard bind addresses (0.0.0.0, ::). In this case we take the wildcard bind address and "expand" the wildcard to select a publish address (https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/common/network/NetworkService.java#L172-L176)

I'm not sure if its easy to add an assert (given transport profiles) but maybe a code comment for the future. It would be nice somewhere in the future to check this is actually the case and throw exception otherwise.

This comment has been minimized.

Copy link
@ywelsch

ywelsch Nov 5, 2015

Author Contributor

Would the following assertions work?

  • For default profile, we require that there is a bound address that matches the publish address (accounting for wildcards).
  • For transport profiles, we require that there is a bound address (of another profile or the default one which matches the current publish address). Can we make this one even more restrictive?

This comment has been minimized.

Copy link
@rmuir

rmuir Nov 5, 2015

Contributor

I was afraid to suggest specific asserts because its hard to think about. it might be easier to remove transport profiles, then it would be simple like the http case and not hurt our brains.

@rmuir

This comment has been minimized.

Copy link
Contributor

rmuir commented Nov 5, 2015

The http case must be also handled, but it should be significantly simpler. when looking at the list of bound addresses, find the first one where address.equals(publishAddress) || address.isAnyLocalAddress() and return that port. fail if we don't find it.

you can see this easily by doing e.g. nc -l 127.0.0.1 9200 and running /bin/elasticsearch on a dual stack system.

@rmuir

This comment has been minimized.

Copy link
Contributor

rmuir commented Nov 5, 2015

The only other idea i had to make this better, would be to remove NetworkService.resolvePublishHostAddresses() completely: https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/common/network/NetworkService.java#L149-L150

The idea being, the caller must deal with this themselves. We could still have the "sorting logic" as a shared method somewhere so that both http and transport can use it, but then it would not be NetworkUtils.sortAddresses(List<InetAddress> list) but instead something more useful/less trappy like NetworkUtils.sortAddresses(List<InetSocketAddress> list). Beware: other code may call this stuff today, or the sorting logic directly (multicast plugin), so I think its better as a followup issue after fixing the bug.

@ywelsch

This comment has been minimized.

Copy link
Contributor Author

ywelsch commented Nov 5, 2015

Pushed a set of changes to address the comments.

@rmuir I like the idea of inlining and adapting resolvePublishHostAddresses() at call sites. At the same time, settings should be cleaned up (at the moment it's so difficult to see where each of the different settings come into play...)

@rmuir

This comment has been minimized.

Copy link
Contributor

rmuir commented Nov 5, 2015

looks good, thanks! And I agree with your opinion about cleaning up settings first.

@ywelsch ywelsch force-pushed the ywelsch:fix/bound-vs-publish-port branch to 1902c66 Nov 6, 2015

ywelsch added a commit that referenced this pull request Nov 6, 2015

Merge pull request #14535 from ywelsch/fix/bound-vs-publish-port
Port of publishAddress should match port of corresponding boundAddress

@ywelsch ywelsch merged commit 1c31845 into elastic:master Nov 6, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

ywelsch added a commit that referenced this pull request Nov 6, 2015

@ywelsch ywelsch removed the v2.1.0 label Nov 6, 2015

@ywelsch

This comment has been minimized.

Copy link
Contributor Author

ywelsch commented Nov 6, 2015

Back porting this to 2.1 is a bit tricky as resolvePublishHostAddresses only takes a single host as parameter. I would prefer to keep this change to 2.2.0 and 3.0.0.
@nik9000 I removed the 2.1 label on #14503. If you don't agree, we can discuss it again.

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.