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

Fix BWC for ES|QL cluster request #117865

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Dec 3, 2024

We identified a BWC bug in the cluster computer request. Specifically, the indices options were not properly selected for requests from an older querying cluster. This caused the search_shards API on the remote cluster to use restricted indices options, leading to failures when resolving wildcard index patterns.

Our tests didn't catch this issue because the current BWC tests for cross-cluster queries only cover one direction: the querying cluster on the current version and the remote cluster on a compatible version.

This PR fixes the issue and expands BWC tests to support both directions: the querying cluster on the current version with the remote cluster on a compatible version, and vice versa.

@dnhatn dnhatn added the test-full-bwc Trigger full BWC version matrix tests label Dec 3, 2024
@@ -24,7 +26,8 @@ static RemoteClusterPlan from(PlanStreamInput planIn) throws IOException {
if (planIn.getTransportVersion().onOrAfter(TransportVersions.ESQL_ORIGINAL_INDICES)) {
originalIndices = OriginalIndices.readOriginalIndices(planIn);
} else {
originalIndices = new OriginalIndices(planIn.readStringArray(), IndicesOptions.strictSingleIndexNoExpandForbidClosed());
// fallback to the previous behavior
originalIndices = new OriginalIndices(planIn.readStringArray(), SearchRequest.DEFAULT_INDICES_OPTIONS);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the primary change, where we fall back to using the default indices option.

@elasticsearchmachine
Copy link
Collaborator

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

@dnhatn dnhatn marked this pull request as ready for review December 3, 2024 00:49
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 3, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@dnhatn dnhatn added auto-backport Automatically create backport pull requests when merged and removed test-full-bwc Trigger full BWC version matrix tests labels Dec 3, 2024
Copy link
Contributor

@astefan astefan 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
Contributor

@n1v0lg n1v0lg left a comment

Choose a reason for hiding this comment

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

Prod-code change LGTM from a security perspective. Good catch!

@dnhatn
Copy link
Member Author

dnhatn commented Dec 3, 2024

@astefan @n1v0lg Thank you for quick review!

@dnhatn dnhatn merged commit 267dc1a into elastic:main Dec 3, 2024
16 checks passed
@dnhatn dnhatn deleted the bwc-cluster-request branch December 3, 2024 15:27
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.16 Commit could not be cherrypicked due to conflicts
8.17
8.x

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 117865

dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Dec 3, 2024
We identified a BWC bug in the cluster computer request. Specifically, 
the indices options were not properly selected for requests from an
older querying cluster. This caused the search_shards API on the remote
cluster to use restricted indices options, leading to failures when
resolving wildcard index patterns.

Our tests didn't catch this issue because the current BWC tests for 
cross-cluster queries only cover one direction: the querying cluster on
the current version and the remote cluster on a compatible version.

This PR fixes the issue and expands BWC tests to support both 
directions: the querying cluster on the current version with the remote
cluster on a compatible version, and vice versa.
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Dec 3, 2024
We identified a BWC bug in the cluster computer request. Specifically, 
the indices options were not properly selected for requests from an
older querying cluster. This caused the search_shards API on the remote
cluster to use restricted indices options, leading to failures when
resolving wildcard index patterns.

Our tests didn't catch this issue because the current BWC tests for 
cross-cluster queries only cover one direction: the querying cluster on
the current version and the remote cluster on a compatible version.

This PR fixes the issue and expands BWC tests to support both 
directions: the querying cluster on the current version with the remote
cluster on a compatible version, and vice versa.
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Dec 3, 2024
We identified a BWC bug in the cluster computer request. Specifically,
the indices options were not properly selected for requests from an
older querying cluster. This caused the search_shards API on the remote
cluster to use restricted indices options, leading to failures when
resolving wildcard index patterns.

Our tests didn't catch this issue because the current BWC tests for
cross-cluster queries only cover one direction: the querying cluster on
the current version and the remote cluster on a compatible version.

This PR fixes the issue and expands BWC tests to support both
directions: the querying cluster on the current version with the remote
cluster on a compatible version, and vice versa.
elasticsearchmachine pushed a commit that referenced this pull request Dec 3, 2024
We identified a BWC bug in the cluster computer request. Specifically, 
the indices options were not properly selected for requests from an
older querying cluster. This caused the search_shards API on the remote
cluster to use restricted indices options, leading to failures when
resolving wildcard index patterns.

Our tests didn't catch this issue because the current BWC tests for 
cross-cluster queries only cover one direction: the querying cluster on
the current version and the remote cluster on a compatible version.

This PR fixes the issue and expands BWC tests to support both 
directions: the querying cluster on the current version with the remote
cluster on a compatible version, and vice versa.
elasticsearchmachine pushed a commit that referenced this pull request Dec 3, 2024
We identified a BWC bug in the cluster computer request. Specifically, 
the indices options were not properly selected for requests from an
older querying cluster. This caused the search_shards API on the remote
cluster to use restricted indices options, leading to failures when
resolving wildcard index patterns.

Our tests didn't catch this issue because the current BWC tests for 
cross-cluster queries only cover one direction: the querying cluster on
the current version and the remote cluster on a compatible version.

This PR fixes the issue and expands BWC tests to support both 
directions: the querying cluster on the current version with the remote
cluster on a compatible version, and vice versa.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.16.2 v8.17.1 v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants