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

Send clear session as routable remote request #36805

Merged
merged 11 commits into from
Dec 21, 2018

Conversation

Tim-Brooks
Copy link
Contributor

@Tim-Brooks Tim-Brooks commented Dec 19, 2018

This commit adds a RemoteClusterAwareRequest interface that allows a
request to specify which remote node it should be routed to. The remote
cluster aware client will attempt to route the request directly to this
node. Otherwise it will send it as a proxy action to eventually end up
on the requested node.

It implements the ccr clean_session action with this client.

@Tim-Brooks Tim-Brooks added >non-issue v7.0.0 :Distributed/CCR Issues around the Cross Cluster State Replication features v6.7.0 labels Dec 19, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@Tim-Brooks
Copy link
Contributor Author

This includes some of the changes that we discussed related to routing requests to a specific node. If we like this work, it can also be used to implement the ccr file chunk requests.

@ywelsch
Copy link
Contributor

ywelsch commented Dec 19, 2018

@tbrooks8 Thanks for exploring this solution. What I don't like about this solution is that we're now introducing the concept of a target node in the Client interface (even though it's only in the RemoteClient sub-interface) whereas that interface has previously been completely agnostic of that.

An alternative would be to introduce a new RemoteClusterAwareRequest interface with a method DiscoveryNode getPreferredTargetNode() that returns a preferred target node. That interface can then be implemented by specific ActionRequests, and we then change RemoteClusterAwareClient to be aware of this interface and select the connection based on the preferred target node. @tbrooks8 WDYT?

@Tim-Brooks
Copy link
Contributor Author

An alternative would be to introduce a new RemoteClusterAwareRequest interface with a method DiscoveryNode getPreferredTargetNode()

So do you:

  1. Still want to have the remote client sub-interface that takes this request type
    or
  2. Internally in the remote client do an instanceof check

@ywelsch
Copy link
Contributor

ywelsch commented Dec 19, 2018

I would be fine with option 2 (instanceof check). Option 1 would require adding two more execute methods to the RemoteClient interface.

@Tim-Brooks Tim-Brooks changed the title Send clear session using routable remote client Send clear session as routable remote request Dec 19, 2018
@Tim-Brooks
Copy link
Contributor Author

@ywelsch I have implemented your suggestion.

Unfortunately, I think that there is another issue we will need to work through. The remote cluster service will send an action that is not directly to the remote node as a proxy action internal:transport/proxy/. The system user is not authorized for this type of action so I think that the action will be rejected with security enabled.

The client (I think) will be using the system user because the client is passed in at repository creation time. And the actions will be executed in the restoreShard method in the ccr repository. This will happen on the generic thread pool so we will have lost the headers (and original user) from the put follow action request.

I'll mention @tvernum @jaymode here as they might know the best way I should confront this issue. I don't see a clear way to work the headers through into the repository since it is hooked up through the normal restore mechanism.

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. Let's get @jaymode or @tvernum's input first before merging.

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

I think we should introduce a CCR origin instead of relying on the system user for these actions (proxied or non proxied). The client can be wrapped with one that always uses the origin IF everything should be executed as a user specific to CCR. In #35667, the OriginSettingClient was introduced that would do the trick.

@Tim-Brooks Tim-Brooks merged commit d9b2ed6 into elastic:master Dec 21, 2018
jasontedor added a commit to ywelsch/elasticsearch that referenced this pull request Dec 21, 2018
* elastic/master: (539 commits)
  SQL: documentation improvements and updates (elastic#36918)
  [DOCS] Merges list of discovery and cluster formation settings (elastic#36909)
  Only compress responses if request was compressed (elastic#36867)
  Remove duplicate paragraph (elastic#36942)
  Fix URI to cluster stats endpoint on specific nodes (elastic#36784)
  Fix typo in unitTest task (elastic#36930)
  RecoveryMonitor#lastSeenAccessTime should be volatile (elastic#36781)
  [CCR] Add `ccr.auto_follow_coordinator.wait_for_timeout` setting (elastic#36714)
  Scripting: Remove deprecated params.ctx (elastic#36848)
  Refactor the REST actions to clarify what endpoints are deprecated. (elastic#36869)
  Add JDK 12 to CI rotation (elastic#36915)
  Improve error message for 6.x style realm settings (elastic#36876)
  Send clear session as routable remote request (elastic#36805)
  [DOCS] Remove redundant ILM attributes (elastic#36808)
  SQL: Fix bug regarding histograms usage in scripting (elastic#36866)
  Update index mappings when ccr restore complete (elastic#36879)
  Docs: Bump version to alpha2 after release
  Enable IPv6 URIs in reindex from remote (elastic#36874)
  Watcher: Remove unused local variable in doExecute (elastic#36655)
  [DOCS] Synchs titles of X-Pack APIs
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Dec 21, 2018
* master: (31 commits)
  Move ingest-geoip default databases out of config (elastic#36949)
  [ILM][DOCS] add extra scenario to policy update docs (elastic#36871)
  [Painless] Add String Casting Tests (elastic#36945)
  SQL: documentation improvements and updates (elastic#36918)
  [DOCS] Merges list of discovery and cluster formation settings (elastic#36909)
  Only compress responses if request was compressed (elastic#36867)
  Remove duplicate paragraph (elastic#36942)
  Fix URI to cluster stats endpoint on specific nodes (elastic#36784)
  Fix typo in unitTest task (elastic#36930)
  RecoveryMonitor#lastSeenAccessTime should be volatile (elastic#36781)
  [CCR] Add `ccr.auto_follow_coordinator.wait_for_timeout` setting (elastic#36714)
  Scripting: Remove deprecated params.ctx (elastic#36848)
  Refactor the REST actions to clarify what endpoints are deprecated. (elastic#36869)
  Add JDK 12 to CI rotation (elastic#36915)
  Improve error message for 6.x style realm settings (elastic#36876)
  Send clear session as routable remote request (elastic#36805)
  [DOCS] Remove redundant ILM attributes (elastic#36808)
  SQL: Fix bug regarding histograms usage in scripting (elastic#36866)
  Update index mappings when ccr restore complete (elastic#36879)
  Docs: Bump version to alpha2 after release
  ...
Tim-Brooks added a commit to Tim-Brooks/elasticsearch that referenced this pull request Jan 15, 2019
This commit adds a RemoteClusterAwareRequest interface that allows a
request to specify which remote node it should be routed to. The remote
cluster aware client will attempt to route the request directly to this
node. Otherwise it will send it as a proxy action to eventually end up
on the requested node.

It implements the ccr clean_session action with this client.
Tim-Brooks added a commit that referenced this pull request Jan 21, 2019
This commit adds a RemoteClusterAwareRequest interface that allows a
request to specify which remote node it should be routed to. The remote
cluster aware client will attempt to route the request directly to this
node. Otherwise it will send it as a proxy action to eventually end up
on the requested node.

It implements the ccr clear_session action with this client.
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
@Tim-Brooks Tim-Brooks deleted the remote_cluster_client branch December 18, 2019 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/CCR Issues around the Cross Cluster State Replication features >non-issue v6.7.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants