ES|QL: Check if cluster aliases and index patterns are valid before executing query

[pawankartik-elastic]

Consider the queries with more than 1 index seperator (not valid as per our current spec):

1. FROM remote:"foo:bar", and,
2. FROM "remote:foo:bar:baz"

As of now, both the queries return empty results when POSTing to /_query?format=txt. This happens because they're syntactically valid (but not semantically) and the subsequent validation makes no attempt to discard them or error out. This PR fixes this behaviour by failing such queries at the parsing stage. There was an initial concern surrounding the licensing and security aspects but since we're failing fast at the parsing stage and aren't allowing the queries "through", we should be fine. Once malformed queries are discarded, subsequent queries should go through the validation steps fine.

Addresses #121901.

Considerations:

1. Should the :: operator be accounted for (because the respective PR is yet to be merged)? Changes have been pulled into my branch and accommodated.
2. This is a bug fix. How far should it be backported (keeping in mind the :: operator)? Looking good to go back as far as 8.19.
3. What's the right team to tag? SF/CCS or Analytics/ES|QL?

[elasticsearchmachine]

Hi @pawankartik-elastic, I've created a changelog YAML for you.

[pawankartik-elastic]

Things unclear to me at the moment:

1. Why are we skipping validation when remote is specified?
2. Is there a single API we could use everywhere to parse and validate remote and index names? At the moment, the parsing and validation is kind of fragmented and different parts of the ES' codebase make different attempts at grasping this information (ref. RemoteClusterAware, which unfortunately, does not do anything to invalidate the above invalid formats). Personally, I'd like to see any format-adherence checks and errors close to or within the parsing stage rather than deferring them.
3. Why do we even allow quoting in the patterns (specified in the grammar file)?

[alex-spies]

I only very briefly looked at the solution, but the approach appears correct to me.

Maybe it's not so nice to validate in multiple places for CCQ-purposes, as CCQ needs to take into consideration more than just lexikographic correctness. However, I believe that any validation that is possible based on the parser, only, should happen in the IdentifierBuilder (or otherwise close to the parsing step), like in this PR.

This allows early rejections without risks, that e.g. we start big fat field_caps calls to try and validate queries that the parser could rightfully reject. And it means that whenever we have a index pattern inside ESQL, we know that it's at least sane from a lexikographic point of view.

The possibility of index patterns containing : slipping through is not ideal at the moment. These are clearly invalid and shouldn't make it into the rest of the system.

Why are we skipping validation when remote is specified

I think we're not going through the same code path. This may or may not be related to how we treat partial results from remotes. However, with the setup from the original issue (#121901), a missing index leads to no validation failure at all:

curl -u elastic:password -H "Content-Type: application/json" "127.0.0.1:9200/_query?format=txt" -d '
{
  "query": "from cluster_one:foo"
}'
  <no-fields>  
---------------

In contrast, the non-CCS code path for FROM foo for a non-existing index foo goes through EsqlSession.preAnalyzeIndices into IndexResolver.mergedMappings, which performs a field caps call and returns IndexResolution.notFound(indexPattern), which is then picked up in the analysis or verification step and fails the query.

Update: Please ignore, there's a much simpler explanation, see below.

Is there a single API we could use everywhere to parse and validate remote and index names?

I didn't analyze the code path for CCQ, but going by the fact that from cluster_one:foo doesn't enter the same code path (I performed a quick check by setting break points), it seems like we should probably carefully double check if we can refactor and consolidate the two code paths, so that CCQ-code and local cluster code remain in sync where it makes sense for them. I don't think there's presently any API that is ready to be used for both cases without at least some refactoring.

Update: It's much simpler than that. For local FROM local_idx we use MetadataCreateIndexService.validateIndexOrAliasName. In case of patterns, we do our own validation. But that can be re-used in CCQ. I don't know of any centralized validation API for remote names.

Why do we even allow quoting in the patterns (specified in the grammar file)?

Quoting is required due to index patterns that can contain funny characters. Like FROM whatever:"foo|*" This is valid! Ugly but valid. We cannot lose the quotes due to the pipe | character.

Allowed quoting:

• FROM "cluster":"remote"
• FROM cluster:"remote"
• FROM "cluster":remote
• FROM cluster:remote
• FROM "cluster:remote"

[alex-spies]

Heya, thanks @pawankartik-elastic !

1. This approach is unfortunately insufficient, but not by much.

This PR improves the situation for the buggy queries that initially triggered the issue. But it doesn't solve the problem that the index pattern may be otherwise invalid, like in FROM valid_remote:"invalid<index_name" - which still just returns

  <no-fields>  
---------------

The reason is that in IdentifierBuilder.visitIndexPattern, we just never call validateIndexPattern in case that clusterString is not null. Additionally, inside validateIndexPattern, we explicitly skip the validation in case that the pattern contains a :.

I believe the correct solution should try to split the indexPattern at the first : in case that there is no clusterString and this is not a date math index pattern. And then, the remaining, actual index pattern should be sent to validateIndexPattern, which needs additionally to consider the : character as invalid rather than just skipping validation.

2. I think there is a separate problem where cluster names can be invalid, which we don't reject at parse time, but in this case we just throw no_such_remote_cluster_exception.

This may be cheap enough to not be a problem - although we could improve validateIndexPattern too check for correct remote name syntax, too.

3. On a separate note: here https://github.com/elastic/elasticsearch/pull/120355/files#diff-ef7f022d55c8068ca9c7c26cc5bac2c4cc3cf5689a9bfc20e931cdf17541556eL83-L84 Ievgen added randomly generated cluster names to our parser tests.

I think it'd be nice to go and check if they use all possible characters that can be used in remote names? If there are more characters than that, we should go and make the identifier generator use that, so this is included in our tests. For instance, what about unicode? (I don't know :) )

[pawankartik-elastic]

Thank you @alex-spies. I agree that this fix doesn't fully address the issue. I was simply trying to:

1. Validate my approach (and kick-off some internal discussions with the CCS team; will document them briefly here before the PR merges for readers' context), and,
2. Discuss a few uncertainities (of why certain things were done certain way).

I'll now work on ensuring that we fully address the issue.

[astefan]

Apart from what @alex-spies awesomely mentioned already, please add many many more tests. For one, there is no test with date math expressions (something you explicitly consider in your code).

[pawankartik-elastic]

please add many many more tests.

Of course! I'd definitely do that. I did the bare minimums to kick start some discussions. Over the next few commits, I'll address all your concerns.

[elasticsearchmachine]

Hi @pawankartik-elastic, I've updated the changelog YAML for you.

[astefan]

I've left some comments.
This is a very sensitive and tricky area we are changing now and tests are essential to our confidence in pushing forward this change. Apart from the random tests, I would like to see many more concrete tests:

FROM  "*: ,*,"
FROM ",,, "
FROM ",,,"
FROM ", ,," 
FROM ",,,",*
FROM "",*
FROM ""
FROM "<logstash-{now/d{yyyy.MM.dd|+12:00}}>"
FROM "<a|b>"
FROM "*:<logstash-{now/M{yyyy.MM}}>"

[astefan]

There is a big disconnect here between the mechanism we use for validating these index patterns and where we actually use these index patterns. Meaning, the mechanism is the Elasticsearch one where, at indexing (index creation time), it's validating the name provided.

Let me give an example:
from "test<" gives Invalid index name [test<], must not contain the following characters [' ','\"','*',',','/','<','>','?','\\','|'] which is acceptable. The error message is not appropriate for ES|QL, it may be correct for ES. We do allow * in index names, so I can do from "test*" and ES|QL doesn't complain about *. We need to revisit this.

[astefan]

@pawankartik-elastic thank you for adding more tests.
What is our plan for this error message?

[pawankartik-elastic]

This error message comes from MetadataCreateIndexService.validateIndexOrAliasName(). One idea is to trap it specifically and re-throw with a different message. Wdyt?

[astefan]

This is the paradox: that validateIndexOrAliasName is made to check indices names at creation time, while we use for checks at runtime. And these two are different :-). Even _search allows * at search time. We need to provide a different error message indeed.

[pawankartik-elastic]

Yes, I understood that. In that case, should I push a fix to display a different error string by omitting * from the error string since that's the only misleading char (or did I miss anything else)?

[astefan]

Yes, please take out the wildcard from there. Thanks.

[alex-spies]

I only reviewed the productive changes as Andrei already made suggestions to the tests.

The production code looks right to me; it's also much simpler than the iteration before the grammar change and looks quite clean to me. Thank you!

I spotted minor issues with trailing commas and white spaces, but nothing dramatic. Please consider this PR unblocked and ready to merge once @astefan approves it.

[alex-spies]

Nice javadoc, thank you!

[alex-spies]

nit: inconsistent comment style, other comments use //.

[alex-spies]

This currently permits white spaces between separators and index name, like FROM "remote : idx" and FROM idx :: failures; it fails with less helpful error messages later down the line.

I think this is related to #129768, but not exactly the same. To avoid scope creep, we can tackle this in a follow-up or make it part of the other issue. I added a comment #129768 (comment) so we don't forget.

[astefan]

Apart from the leftover here, LGTM. Thank you!

[elasticsearchmachine]

💔 Backport failed

Status	Branch	Result
❌	8.19	Commit could not be cherrypicked due to conflicts

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

[pawankartik-elastic]

Ignore the 1 check flagged as failing. It passed on manual retry but the update did not reflect here in the UI.

[pawankartik-elastic]

💚 All backports created successfully

Status	Branch	Result
✅	8.19

Questions ?

Please refer to the Backport tool documentation