-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Conversation
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
Pinging @elastic/es-security |
Pinging @elastic/es-distributed |
@elastic/es-distributed I'm not sure who is the reindex from remote expert these days. Can someone volunteer to review? |
I have 2 follow up PRs after this one
|
@@ -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, ?>> { |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
modules/reindex/build.gradle
Outdated
@@ -56,6 +56,7 @@ unitTest { | |||
|
|||
dependencies { | |||
compile "org.elasticsearch.client:elasticsearch-rest-client:${version}" | |||
compile project(':libs:elasticsearch-ssl-config') |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ywelsch, will do.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
Ping @jkakavas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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
@elasticmachine test this please |
@elasticmachine run elasticsearch-ci/default-distro |
@elasticmachine run elasticsearch-ci/1 |
@elasticmachine |
Reindex from remote now supports configurable SSL/TLS (node level) settings. This change adds documentation relating to those settings Relates: elastic#37527
Reindex from remote now supports configurable SSL/TLS (node level) settings. This change adds documentation relating to those settings Relates: #37527
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
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
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
This is used by the reindex-client library which is published to maven Relates: elastic#37287, elastic#37527 Closes: elastic#38944
This is used by the reindex-client library which is published to maven Relates: elastic#37287, elastic#37527 Backport of: elastic#39019
This is used by the reindex-client library which is published to maven Relates: elastic#37287, elastic#37527 Backport of: elastic#39019
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
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