Reject port ranges in `discovery.seed_hosts` by Hohol · Pull Request #41404 · elastic/elasticsearch

Hohol · 2019-04-22T08:54:13Z

Today Elasticsearch accepts, but silently ignores, port ranges in the
discovery.seed_hosts setting:

discovery.seed_hosts: 10.1.2.3:9300-9400

Silently ignoring part of a setting like this is trappy. With this change we
reject seed host addresses of this form.

Closes #40786

elasticmachine · 2019-04-22T14:52:58Z

Pinging @elastic/es-distributed

dliappis · 2019-04-22T14:55:17Z

@elasticmachine test this please

Hohol · 2019-04-22T17:15:34Z

Got an error
Forbidden annotation use: org.junit.Test [defaultMessage Just name your test method testFooBar]
I was using @Test annotation to check if a method throws an exception.
Should I just use try/catch in the test instead?

dnhatn · 2019-04-22T18:05:07Z

Should I just use try/catch in the test instead?

@Hohol You can use expectThrows.

Hohol · 2019-04-25T10:31:25Z

@DaveCTurner can you review it please?

DaveCTurner

Please don't force-push to PR branches. It loses the history of the changes, including any comments, and generally makes it harder to review.

I think the fix can be made much more deeply. TcpTransport#parse() has a perAddressLimit that is always 1 except when discovery.seed_hosts is unset, and as far as I can tell this method is only used to read the discovery.seed_hosts setting. I think we could reasonably ditch this parameter and, instead, compute the local addresses in SettingsBasedSeedHostsProvider.

Hohol · 2019-04-26T12:00:28Z

I noticed an inconsistency with documentation.

Out of the box, without any network configuration, Elasticsearch will bind to
the available loopback addresses and will scan local ports 9300 to 9305 to try
to connect to other nodes running on the same server.

It sounds to me like ports 9300, 9301, 9302, 9303, 9304, 9305 should be scanned.

But actually LOCAL_PORTS_COUNT = 5 and the code scans for ports 9300, 9301, 9302, 9303, 9304 only.

Should I do something with it?

DaveCTurner · 2019-04-26T12:19:37Z

But actually LOCAL_PORTS_COUNT = 5 and the code scans for ports 9300, 9301, 9302, 9303, 9304 only.

Good catch, yes. Let's treat the docs as the desired behaviour and consider it as a bug that we don't scan port 9305.

…L_PORTS_COUNT

Hohol · 2019-04-26T13:01:30Z

PR was updated.
I'm sorry, but I used force-push again. I believe it was justified because changes of the old commit were completely discarded.

DaveCTurner · 2019-04-26T13:14:58Z

It's still better to avoid a force-push, even if you start again, because now the conversation above doesn't make much sense.

@elasticmachine ok to test.

Hohol · 2019-04-26T14:22:08Z

elasticsearch-ci/1 build failed.
Errors I found in log don't seem to be caused by my changes.
https://scans.gradle.com/s/ebfqezvw2jgco/console-log?task=:modules:percolator:integTestRunner#L24
Could it be some noisy failure?

DaveCTurner · 2019-04-26T15:16:27Z

Yes, that looks weird and totally unrelated. @elasticmachine please run elasticsearch/ci-1

Hohol · 2019-04-26T16:01:25Z

Looks like build hasn't started.
BTW can I give commands to @elasticmachine by myself?

DaveCTurner · 2019-04-27T05:19:54Z

@elasticmachine please run elasticsearch/ci-1

@Hohol no, I think @elasticmachine only pays attention to members of the Elastic organisation.

DaveCTurner · 2019-04-27T05:20:38Z

Bah, it's not running the build because I'm spelling it wrong.

@elasticmachine please run elasticsearch-ci/1 😁

Hohol · 2019-04-29T09:46:55Z

@DaveCTurner please review it.

server/src/main/java/org/elasticsearch/transport/Transport.java

Hohol · 2019-04-29T13:10:29Z

Builds failed with some weird errors again.

Hohol · 2019-04-30T08:34:26Z

@DaveCTurner sorry for bothering you again, but I'd like to finish it sooner. Could you please rerun tests and review?

Hohol · 2019-05-06T06:01:25Z

@DaveCTurner done.

DaveCTurner

Nice work. I went through the whole PR including the tests and left a few more small suggestions.

server/src/main/java/org/elasticsearch/transport/TcpTransport.java

server/src/test/java/org/elasticsearch/transport/TcpTransportTests.java

server/src/main/java/org/elasticsearch/transport/TcpTransport.java

DaveCTurner

LGTM

Hohol · 2019-05-07T08:30:43Z

@DaveCTurner are we waiting for something else?

DaveCTurner · 2019-05-07T08:58:42Z

Hi @Hohol, as I've indicated, this PR looks good to me. I will merge and backport it when I have the availability, but the backport will take time since it requires documentation changes and I am not available to do it now. It is by no means urgent. Please be more patient in future.

Hohol · 2019-05-07T09:17:33Z

I really wanted to know what's going on. Thanks for reply. I'll try to be less annoying next time.

Today Elasticsearch accepts, but silently ignores, port ranges in the `discovery.seed_hosts` setting: ``` discovery.seed_hosts: 10.1.2.3:9300-9400 ``` Silently ignoring part of a setting like this is trappy. With this change we reject seed host addresses of this form. Closes elastic#40786 Backport of elastic#41404

* elastic/master: (50 commits) Cleanup versioned deprecations in analysis (elastic#41560) Allow unknown task time in QueueResizingEsTPE (elastic#41810) SQL: Remove CircuitBreaker from parser (elastic#41835) [DOCS] Fix callouts for dataframe APIs (elastic#41904) Handle serialization exceptions during publication (elastic#41781) Correct spelling of MockLogAppender.PatternSeenEventExpectation (elastic#41893) Update TLS ciphers and protocols for JDK 11 (elastic#41808) Remove Harmful Exists Check from BlobStoreFormat (elastic#41898) Fix fractional seconds for strict_date_optional_time (elastic#41871) Remove op.name configuration setting (elastic#41445) Reject port ranges in `discovery.seed_hosts` (elastic#41404) [ML-DataFrame] migrate to PageParams for get and stats, move PageParams into core (elastic#41851) Reenable RareClusterStateIT Mapping Propagation Tests (elastic#41884) [DOCS] Rewrite `exists` query docs (elastic#41868) Revert "Mute MinimumMasterNodesIT.testThreeNodesNoMasterBlock()" [DOCS] Fix typo referring to multi search API Provide names for all artifact repositories (elastic#41857) Move InternalAggregations to Writeable (elastic#41841) Fix compilation after incorrect merge Unmute TestClustersPluginIT.testMultiNode (elastic#41340) ...

Today Elasticsearch accepts, but silently ignores, port ranges in the `discovery.seed_hosts` setting: ``` discovery.seed_hosts: 10.1.2.3:9300-9400 ``` Silently ignoring part of a setting like this is trappy. With this change we reject seed host addresses of this form. Closes #40786 Backport of #41404

This change makes the default seed address tests account for the lack of an IPv6 network. By default docker containers only run with IPv4 and these tests fail in a vanilla installation of elasticsearch-ci. To resolve this we only expect IPv6 seed addresses if IPv6 is available. Relates elastic#41404

This change makes the default seed address tests account for the lack of an IPv6 network. By default docker containers only run with IPv4 and these tests fail in a vanilla installation of elasticsearch-ci. To resolve this we only expect IPv6 seed addresses if IPv6 is available. Relates #41404

Today Elasticsearch accepts, but silently ignores, port ranges in the `discovery.seed_hosts` setting: ``` discovery.seed_hosts: 10.1.2.3:9300-9400 ``` Silently ignoring part of a setting like this is trappy. With this change we reject seed host addresses of this form. Closes elastic#40786

This change makes the default seed address tests account for the lack of an IPv6 network. By default docker containers only run with IPv4 and these tests fail in a vanilla installation of elasticsearch-ci. To resolve this we only expect IPv6 seed addresses if IPv6 is available. Relates elastic#41404

Hohol mentioned this pull request Apr 22, 2019

Reject port ranges in discovery.seed_hosts #40786

Closed

dliappis added the :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. label Apr 22, 2019

dliappis added the >bug label Apr 22, 2019

dliappis requested a review from DaveCTurner April 22, 2019 14:53

dliappis added v8.0.0 v7.2.0 labels Apr 22, 2019

DaveCTurner requested changes Apr 25, 2019

View reviewed changes

Reject port ranges in discovery.seed_hosts. Fix wrong value of LOCA…

51041b3

…L_PORTS_COUNT

remove redundant comment

12b1338

DaveCTurner reviewed Apr 29, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/transport/Transport.java Outdated Show resolved Hide resolved

Hohol added 3 commits April 29, 2019 16:31

move default seed addresses computation to TcpTransport

a48c045

remove unused imports

3262c39

avoid too long lines of code

58161e4

Merge remote-tracking branch 'upstream/master' into reject-port-ranges

8990ddd

DaveCTurner requested changes May 6, 2019

View reviewed changes

Hohol added 3 commits May 6, 2019 12:27

use Integer.parseInt instead of new PortRange

f90ca2c

test getDefaultSeedAddresses properly

7f7cd3c

avoid using hardcoded port setting name

a0d2917

DaveCTurner approved these changes May 6, 2019

View reviewed changes

DaveCTurner merged commit e6019b4 into elastic:master May 7, 2019

DaveCTurner added the >breaking label May 7, 2019

Hohol deleted the reject-port-ranges branch May 7, 2019 16:11

DaveCTurner mentioned this pull request May 7, 2019

Reject port ranges in discovery.seed_hosts #41905

Merged

DaveCTurner added the backport pending label May 7, 2019

DaveCTurner removed the backport pending label May 8, 2019

jaymode mentioned this pull request May 8, 2019

Default seed address tests account for no IPv6 #41971

Merged

ywelsch mentioned this pull request Jul 1, 2019

Rejection of Port Ranges in module discovery.seed_hosts #41425

Closed

jakelandis mentioned this pull request Feb 22, 2021

DRAFT [META] REST Compatible API V7 completeness #68905

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

Hohol commented Apr 22, 2019 • edited by DaveCTurner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 22, 2019

Uh oh!

dliappis commented Apr 22, 2019

Uh oh!

Hohol commented Apr 22, 2019

Uh oh!

dnhatn commented Apr 22, 2019

Uh oh!

Hohol commented Apr 25, 2019

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Hohol commented Apr 26, 2019

Uh oh!

DaveCTurner commented Apr 26, 2019

Uh oh!

Hohol commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner commented Apr 26, 2019

Uh oh!

Hohol commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner commented Apr 26, 2019

Uh oh!

Hohol commented Apr 26, 2019

Uh oh!

DaveCTurner commented Apr 27, 2019

Uh oh!

DaveCTurner commented Apr 27, 2019

Uh oh!

Hohol commented Apr 29, 2019

Uh oh!

Uh oh!

Hohol commented Apr 29, 2019

Uh oh!

Hohol commented Apr 30, 2019

Uh oh!

Hohol commented May 6, 2019

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Hohol commented May 7, 2019

Uh oh!

DaveCTurner commented May 7, 2019

Uh oh!

Hohol commented May 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Hohol commented Apr 22, 2019 •

edited by DaveCTurner

Loading

Hohol commented Apr 26, 2019 •

edited

Loading

Hohol commented Apr 26, 2019 •

edited

Loading