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 ssl settings to reindex from remote #37527

Merged
merged 13 commits into from
Jan 31, 2019

Conversation

tvernum
Copy link
Contributor

@tvernum tvernum commented Jan 16, 2019

Adds reindex.ssl.* settings for reindex from remote.

This uses the ssl-config/ internal library to parse and load SSL
configuration and files. This SSL configuration is applied to the
low level rest client when connecting to a remote ES node.

Relates: #37287
Resolves: #29755

This uses the ssl-config/ internal library to parse and load SSL
configuration and files. This is applied when using the low level
rest client to connect to a remove ES node
@tvernum tvernum added >enhancement :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v7.0.0 :Security/TLS SSL/TLS, Certificates v6.7.0 labels Jan 16, 2019
@tvernum tvernum requested a review from jkakavas January 16, 2019 11:38
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@tvernum
Copy link
Contributor Author

tvernum commented Jan 16, 2019

@elastic/es-distributed I'm not sure who is the reindex from remote expert these days. Can someone volunteer to review?

@tvernum
Copy link
Contributor Author

tvernum commented Jan 16, 2019

I have 2 follow up PRs after this one

  1. To switch the reindex-with-security QA tests to use SSL
  2. Another to update docs.

@@ -82,14 +83,16 @@
* Abstract base for scrolling across a search and executing bulk actions on all results. All package private methods are package private so
* their tests can use them. Most methods run in the listener thread pool because the are meant to be fast and don't expect to block.
*/
public abstract class AbstractAsyncBulkByScrollAction<Request extends AbstractBulkByScrollRequest<Request>> {
public abstract class AbstractAsyncBulkByScrollAction<Request extends AbstractBulkByScrollRequest<Request>,
Action extends TransportAction<Request, ?>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The introduction of the Action as a generic type parameter and field (mainAction below) is an attempt to solve the problems that come from the way AbstractAsyncBulkByScrollAction calls buildScrollableResultSource from its constructor.
Because the call is made from the superclass's constructor, it means that TransportReindexAction.AsyncIndexBySearchAction.buildScrollableResultSource (and by extension buildRestClient) can't make use of any fields in the subclass.
But buildRestClient needs access to the ReindexSslConfig instance, so we either need to push that up into the base class, or come up with a different approach.
By pushing a generic mainAction up into the base class, we can access fields from the Action without needing to duplicate them in AbstractAsyncBulkByScrollAction. This allows us to also remove scriptService from this class.

@ywelsch ywelsch self-requested a review January 16, 2019 12:44
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tvernum. I've left two initial comments. I'm not super familiar with the SSL work though.

@@ -56,6 +56,7 @@ unitTest {

dependencies {
compile "org.elasticsearch.client:elasticsearch-rest-client:${version}"
compile project(':libs:elasticsearch-ssl-config')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

register the ssl-config project in the top-level build.gradle under projectSubstitutions, and then you can use compile "org.elasticsearch:elasticsearch-ssl-config:${version}"
Also check if the ssl-config is properly published through the release manager

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ywelsch, will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build.gradle change has been pushed, and there is an open PR for release manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually @ywelsch Is there a reason you think this should be in release-manager? I don't think we publish any reindex jars that would need this.
As an example we don't publish the grok libs that are used in ingest modules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right. No need for that.

import static org.mockito.Mockito.mock;

/**
* Because core ES doesn't have SSL available, this test uses a mock webserver
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to this test here, can you also add a reindex test to x-pack? This will provide us a full end-to-end test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an existing reindex-with-security test in x-pack/qa.
I have a change prepared to switch it to use SSL. I left it out of this change so that this PR could be limited to the parts that needed reviews from the distributed team.
I'll open that PR now (the test will fail until this is PR is merged, but it will be available for review)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #37600 with that test change.

@tvernum
Copy link
Contributor Author

tvernum commented Jan 24, 2019

Ping @jkakavas

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Tim, sorry for the delay

@tvernum
Copy link
Contributor Author

tvernum commented Jan 29, 2019

@elasticmachine test this please

@tvernum
Copy link
Contributor Author

tvernum commented Jan 29, 2019

02:45:53   1> [2019-01-29T08:45:53,368][INFO ][o.e.u.FullClusterRestartIT] [testRecovery] after test
02:45:53 FAILURE 0.09s | FullClusterRestartIT.testRecovery <<< FAILURES!
02:45:53    > Throwable #1: java.lang.AssertionError: expected version to be one of [8.0.0,7.6.0] but was p 0 testrecovery 7.7.0
02:45:53    > 	at __randomizedtesting.SeedInfo.seed([AAC6A07A621C65D0:6B36D9D64F4CAF77]:0)
02:45:53    > 	at org.elasticsearch.upgrades.FullClusterRestartIT.testRecovery(FullClusterRestartIT.java:842)
02:45:53    > 	at java.lang.Thread.run(Thread.java:748)
02:45:53 Completed [3/3] in 5.04s, 13 tests, 1 failure, 1 skipped <<< FAILURES!

@elasticmachine run elasticsearch-ci/default-distro

@tvernum
Copy link
Contributor Author

tvernum commented Jan 29, 2019

@elasticmachine run elasticsearch-ci/1

@tvernum
Copy link
Contributor Author

tvernum commented Jan 29, 2019

@elasticmachine
run elasticsearch-ci/1
run elasticsearch-ci/default-distro

tvernum added a commit that referenced this pull request Feb 4, 2019
Update the x-pack/qa/reindex-tests-with-security integration tests to
run with TLS enabled on the Rest interface.

Backport of: #37600
Relates: #37527
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 6, 2019
Reindex from remote now supports configurable SSL/TLS (node level)
settings. This change adds documentation relating to those settings

Relates: elastic#37527
tvernum added a commit that referenced this pull request Feb 11, 2019
Reindex from remote now supports configurable SSL/TLS (node level)
settings. This change adds documentation relating to those settings

Relates: #37527
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 11, 2019
Reindex from remote now supports configurable SSL/TLS (node level)
settings. This change adds documentation relating to those settings

Relates: elastic#37527
Backport of: elastic#38486
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 11, 2019
Reindex from remote now supports configurable SSL/TLS (node level)
settings. This change adds documentation relating to those settings

Relates: elastic#37527
Backport of: elastic#38486
tvernum added a commit that referenced this pull request Feb 11, 2019
Reindex from remote now supports configurable SSL/TLS (node level)
settings. This change adds documentation relating to those settings

Relates: #37527
Backport of: #38486
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 11, 2019
Reindex from remote now supports configurable SSL/TLS (node level)
settings. This change adds documentation relating to those settings

Relates: elastic#37527
Backport of: elastic#38486
tvernum added a commit that referenced this pull request Feb 11, 2019
Reindex from remote now supports configurable SSL/TLS (node level)
settings. This change adds documentation relating to those settings

Relates: #37527
Backport of: #38486
tvernum added a commit that referenced this pull request Feb 11, 2019
Reindex from remote now supports configurable SSL/TLS (node level)
settings. This change adds documentation relating to those settings

Relates: #37527
Backport of: #38486
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: elastic#37287, elastic#37527
Closes: elastic#38944
tvernum added a commit that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: #37287, #37527
Closes: #38944
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: elastic#37287, elastic#37527
Backport of: elastic#39019
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: elastic#37287, elastic#37527
Backport of: elastic#39019
tvernum added a commit that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: #37287, #37527
Backport of: #39019
tvernum added a commit that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: #37287, #37527
Backport of: #39019
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: elastic#37287, elastic#37527
Closes: elastic#38944

Backport of: elastic#39019
tvernum added a commit that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: #37287, #37527
Backport of: #39019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >enhancement v6.7.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants