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

Resolve/cluster allows querying for cluster info only (no index expression required) #119898

Merged
merged 19 commits into from
Jan 22, 2025

Conversation

quux00
Copy link
Contributor

@quux00 quux00 commented Jan 9, 2025

Resolve/cluster allows querying for cluster-info-only (no index expression required)

This enhancement provides users with the ability to query the _resolve/cluster API endpoint without specifying
an index expression to match against. This allows users to quickly test what remote clusters are configured on
a cluster and whether they are available for querying.

This new endpoint works with clusters from older versions by querying with the index expression dummy* on those older clusters.

The new endpoint takes no index expression:

GET _resolve/cluster

and returns the same information as before except for the "matching_indices" field. Example response:

{
  "remote1": {
    "connected": false,
    "skip_unavailable": true
  },
  "remote2": {
    "connected": true,
    "skip_unavailable": false,
    "version": {
      "number": "8.17.0",
      "build_flavor": "default",
      "minimum_wire_compatibility_version": "7.17.0",
      "minimum_index_compatibility_version": "7.0.0"
    }
  }
}

@quux00 quux00 added >enhancement auto-backport Automatically create backport pull requests when merged Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch :Search Foundations/CCS v9.0.0 v8.18.0 labels Jan 9, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

@elasticsearchmachine
Copy link
Collaborator

Hi @quux00, I've created a changelog YAML for you.

@quux00 quux00 force-pushed the resolve-cluster/cluster-info-only branch 2 times, most recently from 05284eb to 5386ee3 Compare January 14, 2025 20:12
…ssion required)

This enhancement provides users with the ability to query the _resolve/cluster API endpoint without specifying
an index expression to match against. This allows users to quickly test what remote clusters are configured on
a cluster and whether they are available for querying.

This new endpoint works with clusters from older versions.

The new endpoint takes no index expression:

```
GET _resolve/cluster
```

and returns the same information as before except for the "matching_indices" field. Example response:

```
{
  "remote1": {
    "connected": false,
    "skip_unavailable": true
  },
  "remote2": {
    "connected": true,
    "skip_unavailable": false,
    "version": {
      "number": "8.17.0",
      "build_flavor": "default",
      "minimum_wire_compatibility_version": "7.17.0",
      "minimum_index_compatibility_version": "7.0.0"
    }
  }
}
```
@quux00 quux00 force-pushed the resolve-cluster/cluster-info-only branch from 5386ee3 to 46a7f28 Compare January 14, 2025 20:14
// Whether this request is being processed on the primary ("local") cluster being queried or on a remote.
// This is needed when clusterInfoOnly=true since we need to know whether to list out all possible remotes
// on a node. (We don't want cross-cluster chaining on remotes that might be configured with their own remotes.)
private final boolean isQueryingCluster;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd name it as something like queryRemotes, since the ultimate goal of this variable is not to identify the querying cluster but to define whether or not remotes should be queried.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that's not correct. This variable is intended to specify where the request is being processed on the querying cluster (which should forward it to configured remotes) or remotes (where it should not forward to configured remotes, since we don't support cross-cluster chaining).

@@ -73,6 +82,13 @@ public ResolveClusterActionRequest(StreamInput in) throws IOException {
this.names = in.readStringArray();
this.indicesOptions = IndicesOptions.readIndicesOptions(in);
this.localIndicesRequested = localIndicesPresent(names);
if (in.getTransportVersion().onOrAfter(TransportVersions.RESOLVE_CLUSTER_NO_INDEX_EXPRESSION)) {
this.clusterInfoOnly = in.readBoolean();
this.isQueryingCluster = in.readBoolean();
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder though - is there any case where we get this over the wire and it's not false? I mean, it is true on querying cluster, but querying cluster does not get it over the wire, does it? And the only cluster which does get it over the wire is the remote cluster, which must always make this false. If so, is there any point in serializing this flag at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there any point in serializing this flag at all?
Yes, the cluster receiving this request needs to know whether it is the primary cluster or a remote cluster that should not do additional remote cluster lookups (cross-cluster chaining). See the first part of TransportResolveClusterAction.doExecuteForked.

is there any case where we get this over the wire and it's not false?
If there was ever a case where a node could forward the request to a coordinating node on the same cluster it would be true. Not sure that happens, but I'd rather not have to worry about edge cases and just follow the general pattern.

assertOK(response2);

Map<String, Object> responseMap2 = responseAsMap(response2);
System.err.println(">>> responseMap2: " + responseMap2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this still be here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably got left over after a debugging session. 😆

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. Fixed in next push.

assertNotNull(remoteClusterResponse.get("version"));
}
// {
// // TEST CASE 12: Resolution against wildcarded remote cluster expression that matches no remotes
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is commented out? If it's waiting for some other patch or development, it probably should be specified what it is waiting for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll add a code comment.

While my code change in this PR fixes the GET _resolve/cluster/*nosuchcluster:foo problem (where it no longer queries the local cluster instead, even though that is clearly NOT what was asked for) for unsecured clusters, the security layer resolution still hits this problem because it has it's own index resolution pathways :-(

So I plan to figure that out in a follow-on PR and left this test as a reminder that needs doing.

@smalyshev
Copy link
Contributor

LGTM in general, some nitpicks.

@slobodanadamovic slobodanadamovic removed the request for review from jakelandis January 16, 2025 12:55
@slobodanadamovic slobodanadamovic self-requested a review January 16, 2025 12:55
Copy link
Contributor

@slobodanadamovic slobodanadamovic left a comment

Choose a reason for hiding this comment

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

LGTM (from security perspective)

* ElasticsearchSecurityException you can't tell whether the remote cluster is available or not, so mark
* it as connected=false
*/
resolveClusterInfo = new ResolveClusterInfo(false, skipUnavailable, ese.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

++ on treating security exception as not connected.

* just "*" since that could be an expensive operation on clusters with thousands of indices/aliases/datastreams
*/
String[] dummyIndexExpr = new String[] { DUMMY_INDEX_FOR_OLDER_CLUSTERS };
remoteClusterIndices = remoteClusterService.groupIndices(request.indicesOptions(), dummyIndexExpr, false);
Copy link
Contributor

@slobodanadamovic slobodanadamovic Jan 21, 2025

Choose a reason for hiding this comment

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

Depending on the request.indicesOptions(), this may result in security exception or index not found.

  • allow_no_indices=false -> results in IndexNotFoundException
  • expand_wildcards=none-> will results in ElasticsearchSecurityException if user does not have privileges for dummy* index, because dummy* is now treated as an index name and not expression

Meaning, the response of these calls will differ:

  • GET _resolve/cluster?allow_no_indices=true&expand_wildcards=none
  • GET _resolve/cluster?allow_no_indices=false&expand_wildcards=all
  • GET _resolve/cluster

This is not new behaviour and I'm not sure if this is relevant for this API, but just wanted to raise it since I did not see integration tests that pass any of the indices options.

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 very much for flagging this. I think I'll adjust the impl based on that and add some tests.

if (request.hasParam("ignore_throttled")) {
indexOptions.add("ignore_throttled");
}
return indexOptions;
Copy link
Member

Choose a reason for hiding this comment

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

drive-by comment: do we need to add these to a set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll add that in the next push.

@quux00 quux00 merged commit b3a032c into elastic:main Jan 22, 2025
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

quux00 added a commit to quux00/elasticsearch that referenced this pull request Jan 22, 2025
…ssion required) (elastic#119898)

Resolve/cluster allows querying for cluster-info-only (no index expression required)

This enhancement provides users with the ability to query the _resolve/cluster API endpoint without specifying
an index expression to match against. This allows users to quickly test what remote clusters are configured on
a cluster and whether they are available for querying.

The new endpoint takes no index expression:

```
GET _resolve/cluster
```

and returns the same information as before except for the "matching_indices" field. Example response:

```
{
  "remote1": {
    "connected": false,
    "skip_unavailable": true
  },
  "remote2": {
    "connected": true,
    "skip_unavailable": false,
    "version": {
      "number": "8.17.0",
      "build_flavor": "default",
      "minimum_wire_compatibility_version": "7.17.0",
      "minimum_index_compatibility_version": "7.0.0"
    }
  }
}
```

For backwards compatibility, this new endpoint works with clusters from older versions by querying with the index expression `dummy*` on those older clusters and ignoring the matching_indices value in the response they return.
elasticsearchmachine pushed a commit that referenced this pull request Jan 22, 2025
…ssion required) (#119898) (#120650)

Resolve/cluster allows querying for cluster-info-only (no index expression required)

This enhancement provides users with the ability to query the _resolve/cluster API endpoint without specifying
an index expression to match against. This allows users to quickly test what remote clusters are configured on
a cluster and whether they are available for querying.

The new endpoint takes no index expression:

```
GET _resolve/cluster
```

and returns the same information as before except for the "matching_indices" field. Example response:

```
{
  "remote1": {
    "connected": false,
    "skip_unavailable": true
  },
  "remote2": {
    "connected": true,
    "skip_unavailable": false,
    "version": {
      "number": "8.17.0",
      "build_flavor": "default",
      "minimum_wire_compatibility_version": "7.17.0",
      "minimum_index_compatibility_version": "7.0.0"
    }
  }
}
```

For backwards compatibility, this new endpoint works with clusters from older versions by querying with the index expression `dummy*` on those older clusters and ignoring the matching_indices value in the response they return.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged >enhancement :Search Foundations/CCS Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants