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.

Sign up for GitHub

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

Jump to bottom

Add ability to resolve the cluster alias of remote cluster connections #91724

Merged
merged 2 commits into from
Nov 23, 2022
Merged

Add ability to resolve the cluster alias of remote cluster connections #91724

merged 2 commits into from
Nov 23, 2022

Conversation

slobodanadamovic
Copy link
Contributor

As part of Remote Cluster Security 2.0, we need to be able to determine
the remote cluster alias for outbound remote cluster connections in the
SecurityServerTransportInterceptor.

This commit adds this ability to RemoteConnectionManager
by wrapping all created connection objects with cluster alias.

@slobodanadamovic slobodanadamovic added >enhancement >non-issue :Distributed Coordination/Network Http and internode communication implementations :Security/Security Security issues without another label Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. Team:Security Meta label for security team labels Nov 18, 2022
@slobodanadamovic slobodanadamovic self-assigned this Nov 18, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

n1v0lg
n1v0lg approved these changes Nov 21, 2022
View reviewed changes
Copy link
Contributor

@n1v0lg n1v0lg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! This is indeed simpler than the "reverse-lookup" approach we discussed. Deferring to @Tim-Brooks for final approval.

server/src/main/java/org/elasticsearch/transport/RemoteConnectionManager.java Show resolved Hide resolved
server/src/test/java/org/elasticsearch/transport/RemoteConnectionManagerTests.java Show resolved Hide resolved
@n1v0lg n1v0lg mentioned this pull request Nov 22, 2022
Closed
Tim-Brooks
Tim-Brooks requested changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@Tim-Brooks Tim-Brooks 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 there is an issue unwrapping proxy connections.

server/src/main/java/org/elasticsearch/transport/RemoteConnectionManager.java
@@ -87,7 +89,7 @@ public void openConnection(DiscoveryNode node, ConnectionProfile profile, Action
@Override
public Transport.Connection getConnection(DiscoveryNode node) {
try {
return delegate.getConnection(node);
return getConnectionInternal(node);
} catch (NodeNotConnectedException e) {
return new ProxyConnection(getAnyRemoteConnection(), node);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the functionality that you want is broken? We do not have a "direct" connection to the target node, the ProxyConnection type will be returned. But your resolveRemoteClusterAlias method does not unwrap proxy types.

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 thought I covered this at line 165 by calling unwrap before checking remote connection type:

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

Line 165 in c404382

Transport.Connection unwrapped = TransportService.unwrapConnection(connection);

Is this okay or do you have something else in mind?

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 is also a test specifically for resolving the cluster alias of a proxy connections:

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

Lines 104 to 107 in c404382

DiscoveryNode remoteNode2 = new DiscoveryNode("remote-node-2", address, Version.CURRENT);
Transport.Connection proxyConnection = remoteConnectionManager.getConnection(remoteNode2);
assertThat(proxyConnection, instanceOf(RemoteConnectionManager.ProxyConnection.class));
assertThat(RemoteConnectionManager.resolveRemoteClusterAlias(proxyConnection).get(), equalTo("remote-cluster"));

Tim-Brooks
Tim-Brooks approved these changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@Tim-Brooks Tim-Brooks left a comment

Choose a reason for hiding this comment

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

No that's fine. I had just missed that line since it referenced unmodified code.

@slobodanadamovic slobodanadamovic merged commit e064b0d into elastic:main Nov 23, 2022
@n1v0lg n1v0lg mentioned this pull request Dec 28, 2022
Merged
n1v0lg added a commit that referenced this pull request Jan 24, 2023
@n1v0lg
Resolve cluster alias for opened connections (#92584)
ffabc7e 
We introduced the ability to resolve the cluster alias in #91724, given a
connection towards a remote cluster. This works for connections
obtained via RemoteConnectionManager::getConnection. However,
connections can also be initiated via openConnection (e.g., in
SniffConnectionStrategy). This PR adds support to correctly resolve the
cluster alias for such connections, by wrapping them in an
InternalRemoteConnection instance. This is necessary to support the new
remote cluster security model.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@n1v0lg n1v0lg n1v0lg approved these changes

@Tim-Brooks Tim-Brooks Tim-Brooks approved these changes

Assignees

@slobodanadamovic slobodanadamovic

Labels
:Distributed Coordination/Network Http and internode communication implementations >non-issue :Security/Security Security issues without another label Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. Team:Security Meta label for security team v8.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@slobodanadamovic @elasticsearchmachine @Tim-Brooks @n1v0lg