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

pawankartik-elastic · 2025-02-13T11:59:58Z

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

FROM remote:"foo:bar", and,
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:

~~Should the :: operator be accounted for (because the respective PR is yet to be merged)?~~ Changes have been pulled into my branch and accommodated.
~~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.
~~What's the right team to tag? SF/CCS or Analytics/ES|QL?~~

elasticsearchmachine · 2025-02-13T12:00:23Z

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

in cluster name

pawankartik-elastic · 2025-02-13T18:42:49Z

Things unclear to me at the moment:

Why are we skipping validation when remote is specified?
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.
Why do we even allow quoting in the patterns (specified in the grammar file)?

alex-spies · 2025-02-17T12:30:15Z

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 !

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.

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.

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 :) )

alex-spies · 2025-02-17T13:02:43Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

+        expectError("FROM remote:\"foo:bar\"", "Unexpected index separator in index patter");
+        expectError("FROM \"remote:foo:bar:baz\"", "Unexpected index separator in index patter");


Let's move these into ~~their own test method~~ testInvalidRemoteClusterPattern, the test case testInvalidQuotingAsFromIndexPattern is about invalid quoting not remotes.

alex-spies · 2025-02-17T13:11:14Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

If we decide to add more validation for the remote names, I think we need to add more tests for all kinds of illegal characters that (right now) can go into remote names, using all kinds of possible quoting.

For instance, all of the following currently don't cause a parse time exception, but instead raise no_such_remote_cluster_exception:

from "cluster|one:foo"

from "cluster<one:foo"

from "cluster🦄one:foo"

alex-spies · 2025-02-17T14:02:39Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

+            var maxSeparators = clusterString == null ? 1 : 0;
+            if (patternExceedsMaxIndexSeparators(indexPattern, maxSeparators)) {
+                throw new ParsingException(source(c), "Unexpected index separator in index pattern");
+            }
            // skip validating index on remote cluster, because the behavior of remote cluster is not consistent with local cluster
            // For example, invalid#index is an invalid index name, however FROM *:invalid#index does not return an error
            if (clusterString == null) {


This shouldn't be an if-else, the index pattern should always be validated.

CC @fang-xing-esql. Maybe you have more context behind this decision.

Currently, we don't validate index names in IdentifierBuilder for remote clusters, when I implemented the validation of index names, I found the behaviors of invalid index names between remote and local cluster are not quite consistent, so I decide to skip the validation of index names on remote cluster.

For example, these queries below all run to completion, they don't error out although the remote index names are invalid

+ curl -u elastic:password -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d { "query": "FROM existing_index, *:\"invalid:index\"" } ' `* | `*.keyword | author | author.keyword | b`a`r | bar | page_count | release_date ------------------+------------------+-----------------+-----------------+---------------+---------------+---------------+------------------------ Snow Crash |Snow Crash |Neal Stephenson |Neal Stephenson |5 |1 |470 |1992-06-01T00:00:00.000Z Revelation Space |Revelation Space |Alastair Reynolds|Alastair Reynolds|4 |2 |585 |2000-03-15T00:00:00.000Z 1984 |1984 |George Orwell |George Orwell |3 |3 |328 |1985-06-01T00:00:00.000Z Fahrenheit 451 |Fahrenheit 451 |Ray Bradbury |Ray Bradbury |2 |4 |227 |1953-10-15T00:00:00.000Z Brave New World |Brave New World |Aldous Huxley |Aldous Huxley |1 |5 |268 |1932-06-01T00:00:00.000Z The Handmaids Tale|The Handmaids Tale|Margaret Atwood |Margaret Atwood |0 |0 |311 |1985-06-01T00:00:00.000Z + curl -u elastic:password -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d { "query": "FROM existing_index, \"*:invalid:index\"" } ' `* | `*.keyword | author | author.keyword | b`a`r | bar | page_count | release_date ------------------+------------------+-----------------+-----------------+---------------+---------------+---------------+------------------------ Snow Crash |Snow Crash |Neal Stephenson |Neal Stephenson |5 |1 |470 |1992-06-01T00:00:00.000Z Revelation Space |Revelation Space |Alastair Reynolds|Alastair Reynolds|4 |2 |585 |2000-03-15T00:00:00.000Z 1984 |1984 |George Orwell |George Orwell |3 |3 |328 |1985-06-01T00:00:00.000Z Fahrenheit 451 |Fahrenheit 451 |Ray Bradbury |Ray Bradbury |2 |4 |227 |1953-10-15T00:00:00.000Z Brave New World |Brave New World |Aldous Huxley |Aldous Huxley |1 |5 |268 |1932-06-01T00:00:00.000Z The Handmaids Tale|The Handmaids Tale|Margaret Atwood |Margaret Atwood |0 |0 |311 |1985-06-01T00:00:00.000Z + curl -u elastic:password -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d { "query": "FROM existing_index, *:\"-invalid:index\"" } ' `* | `*.keyword | author | author.keyword | b`a`r | bar | page_count | release_date ------------------+------------------+-----------------+-----------------+---------------+---------------+---------------+------------------------ Snow Crash |Snow Crash |Neal Stephenson |Neal Stephenson |5 |1 |470 |1992-06-01T00:00:00.000Z Revelation Space |Revelation Space |Alastair Reynolds|Alastair Reynolds|4 |2 |585 |2000-03-15T00:00:00.000Z 1984 |1984 |George Orwell |George Orwell |3 |3 |328 |1985-06-01T00:00:00.000Z Fahrenheit 451 |Fahrenheit 451 |Ray Bradbury |Ray Bradbury |2 |4 |227 |1953-10-15T00:00:00.000Z Brave New World |Brave New World |Aldous Huxley |Aldous Huxley |1 |5 |268 |1932-06-01T00:00:00.000Z The Handmaids Tale|The Handmaids Tale|Margaret Atwood |Margaret Atwood |0 |0 |311 |1985-06-01T00:00:00.000Z + curl -u elastic:password -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d { "query": "FROM existing_index, \"*:-invalid:index\"" } ' `* | `*.keyword | author | author.keyword | b`a`r | bar | page_count | release_date ------------------+------------------+-----------------+-----------------+---------------+---------------+---------------+------------------------ Snow Crash |Snow Crash |Neal Stephenson |Neal Stephenson |5 |1 |470 |1992-06-01T00:00:00.000Z Revelation Space |Revelation Space |Alastair Reynolds|Alastair Reynolds|4 |2 |585 |2000-03-15T00:00:00.000Z 1984 |1984 |George Orwell |George Orwell |3 |3 |328 |1985-06-01T00:00:00.000Z Fahrenheit 451 |Fahrenheit 451 |Ray Bradbury |Ray Bradbury |2 |4 |227 |1953-10-15T00:00:00.000Z Brave New World |Brave New World |Aldous Huxley |Aldous Huxley |1 |5 |268 |1932-06-01T00:00:00.000Z The Handmaids Tale|The Handmaids Tale|Margaret Atwood |Margaret Atwood |0 |0 |311 |1985-06-01T00:00:00.000Z

If we decide to validate remote index names, it does require more tests so that we are clear about the scope of change. : is not the only invalid character, and also be careful about * and - in the (remote)index patterns IndentifierBuilder.validateIndexPattern calls some existing APIs to validate index names, and remote cluster index name validation can take advantage of it. It could change the current behaviors related to (remote) index patterns potentially, we need to justify if the behavior changes make sense.

alex-spies · 2025-02-17T14:06:57Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

I think we need a bunch more tests for invalid index pattern in all the cases where there is a remote prefix some_remote:.... Ideally, randomly generated invalid index patterns.

pawankartik-elastic · 2025-02-17T14:12:26Z

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

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,
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).

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

astefan · 2025-02-17T15:32:27Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

+            var maxSeparators = clusterString == null ? 1 : 0;
+            if (patternExceedsMaxIndexSeparators(indexPattern, maxSeparators)) {
+                throw new ParsingException(source(c), "Unexpected index separator in index pattern");
+            }
            // skip validating index on remote cluster, because the behavior of remote cluster is not consistent with local cluster
            // For example, invalid#index is an invalid index name, however FROM *:invalid#index does not return an error
            if (clusterString == null) {


CC @fang-xing-esql. Maybe you have more context behind this decision.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

pawankartik-elastic · 2025-02-17T15:39:20Z

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 · 2025-04-11T15:55:57Z

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

pawankartik-elastic · 2025-04-11T15:56:33Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

@@ -784,6 +781,8 @@ public void testInvalidQuotingAsFromIndexPattern() {

        expectError("FROM \"\"\"foo\"\"\"bar\"\"\"", ": mismatched input 'bar' expecting {<EOF>, '|', ',', 'metadata'}");
        expectError("FROM \"\"\"foo\"\"\"\"\"\"bar\"\"\"", ": mismatched input '\"bar\"' expecting {<EOF>, '|', ',', 'metadata'}");
+        expectError("FROM remote:\"foo:bar\"", "Index pattern [foo:bar] contains a cluster alias despite specifying one [remote]");


Ignore these 2 test cases. They're already tested below; will drop them soon.

pawankartik-elastic · 2025-04-11T16:03:47Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

+                if (selectorString != null) {
+                    throwOnMixingSelectorWithCluster(reassembleIndexName(clusterString, indexPattern, selectorString), ctx);
+                }
+                validateClusterString(clusterString, ctx);


Do we allow unicode chars in the cluster name? Are the naming rules documented anywhere?

pattern

fang-xing-esql

Thank you @pawankartik-elastic! I added some thoughts.

fang-xing-esql · 2025-04-14T20:23:45Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

-                }
-            }
+
+            validateClusterAndIndexPatterns(indexPattern, c, clusterString, selectorString);


indexPattern, clusterString and selectorString can be extracted from an IndexPatternContext, providing all four to validateClusterAndIndexPatterns seems redundant.

validateClusterAndIndexPatterns() is a static method currently. If I were to pass the IndexPatternContext, I'd have to make it non-static since the visitor methods (such as visitIndexString()) are non-static and are overridden. Would that be alright with you?

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

fang-xing-esql · 2025-04-14T21:03:21Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

+        String[] patterns = indexPattern.split(",");
+        boolean isFirstPattern = true;
+
+        for (String pattern : patterns) {


I find this is hard for me to follow. The old logic that we check clusterString first and then check indexString and selectorString, the same sequence as the index patterns are coded in the query, is easier for me to follow. I wonder if we can restore to the previous sequence, does it look messier? It seems to me that the old way is easier to understand, maintain, and less error prone.

Isn't that exactly what we're doing in this loop? At this point, we loop over each pattern and then towards the end we validate the clusterString (if set), then call the validateIndicesForCluster() to validate the indices specified, and then we finally validate the selectorString via validateIndexSelectorString()—all within this loop, at the end of the loop.

I maintained the same order. Had I deviated from this order, wrong error messages would have taken precedence and caused the tests to fail due to incorrect error strings (which indeed happened when I was working on this PR—when I deviated from the previous validation ordering).

I agree that the new logic may be complex. It's due to whole new scenarios that are "unlocked" for validation because it applies to remotes too now.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

fang-xing-esql · 2025-04-14T22:57:04Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

@@ -488,15 +489,12 @@ public void testStringAsIndexPattern() {
            clusterAndIndexAsIndexPattern(command, "cluster:index");
            clusterAndIndexAsIndexPattern(command, "cluster:.index");
            clusterAndIndexAsIndexPattern(command, "cluster*:index*");
-            clusterAndIndexAsIndexPattern(command, "cluster*:<logstash-{now/D}>*");// this is not a valid pattern, * should be inside <>
-            clusterAndIndexAsIndexPattern(command, "cluster*:<logstash-{now/D}*>");


Why this test is removed?

Previously, this would not error out even if the logstash part was invalid because it's a remote index and we use to skip validation for remote indices. Now that we validate even remote indices, these cases should error out with a message along the lines of 'D' is an invalid unit. Such an exact test case is already present so these are duplicates.

@pawankartik-elastic , it's hard for me to validate that the case FROM + cluster*:<logstash-{now/D}>* and cluster*:<logstash-{now/D}*> is still tested. Could you point me to where exactly this is tested, or just add these back in with an appropriate expectError just so human readers can easily find this case covered? (Maybe it's covered below in testInvalidPatternsWithIntermittentQuotes, but that's harder to parse for humans.)

Actually, I just tested these two expressions and except for the fact that it should be a lower case d, these come through as valid just fine. So I think these cases can be left in place but need to be slightly fixed:

clusterAndIndexAsIndexPattern(command, "cluster*:<logstash-{now/d}>*");// this is not a valid pattern, * should be inside <> clusterAndIndexAsIndexPattern(command, "cluster*:<logstash-{now/d}*>");

fang-xing-esql · 2025-04-15T00:10:01Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

-            clustersAndIndices(command, "index*", "-index#pattern");
-            clustersAndIndices(command, "*", "-<--logstash-{now/M{yyyy.MM}}>");
-            clustersAndIndices(command, "index*", "-<--logstash#-{now/M{yyyy.MM}}>");
+            expectInvalidIndexNameErrorWithLineNumber(command, "*, index#pattern", lineNumber, "index#pattern", "must not contain '#'");


This changes the current behavior. Today, if a * exists in index patterns, invalid index names are ignored, they don't error out as far as the index patterns match at least one index(valid and existing). The same applies to the tests a few lines below that have selectors. However, it looks reasonable to me that as soon as there is an index pattern with invalid character found, we error out. In this situation we have a *, that means there is a high chance that this whole index pattern in the from command matches an existing index, and it doesn't look completely wrong to tolerate invalid character(could be a typo) either. There could be a reason that why this was allowed originally.

Understood. Should we have a slightly detailed discussion internally with the ES|QL and the CCS team regarding whether we should tolerate such erroneous patterns?

It changes the current behaviors, it worths bringing up to a broader group to discuss further in my opinion, and also add ES|QL-ui label to inform kibana team.

This behaviour is about excluding certain patterns as long as there's a preceding pattern(s) with a * (and does not require any valid and existing indices). Since this functionality existed as a way to exclude arbitrary patterns (but is unfortunately not documented anywhere?), I've reverted some of my changes based on our conversation earlier today to retain this behaviour and added back these test cases.

@fang-xing-esql , are you sure this is a behavior change? I just ran this against main and got an invalid index name exception with a 400:

curl -u elastic:password -X POST 'localhost:9200/_query?format=txt' -H 'Content-Type: application/json' '-d 16:36 { "query": "from *, index#pattern" } ' {"error":{"root_cause":[{"type":"invalid_index_name_exception","reason":"Invalid index name [index#pattern], must not contain '#'","index_uuid":"_na_","index":"index#pattern"}],"type":"parsing_exception","reason":"line 1:9: Invalid index name [index#pattern], must not contain '#'","caused_by":{"type":"invalid_index_name_exception","reason":"Invalid index name [index#pattern], must not contain '#'","index_uuid":"_na_","index":"index#pattern"}},"status":400}%

Both added tests here seem to behave the same way as on main in my short manual test.

The code is non-trivial so I may not have read this correctly, but it appears to me that the behavior of the validation for non-remote index patterns remains consistent with this change.

1. Wrapped `splitIndexName()` in try catch and ensured it throws parse exception. 2. Relaxed the validation to allow excluding garbage. 3. Validation no longer breaks patterns on pipe char as it's an invalid char.

pawankartik-elastic · 2025-04-16T14:55:52Z

Ignore the CI failures. It's mostly BWC failures because branch is out of date. I will update it later.

alex-spies

Thanks a lot @pawankartik-elastic ! This is looking much better already.

I reviewed the production code, but not yet the new test that was added. The reason is that there have been important discussions on what should and should not be allowed in FROM commands, especially the comments by @jbaiera here and here, which are not reflected in this PR.

If we go with the suggestions by @jbaiera , and I believe we should, then the present PR could be simplified.

Changing the validation of the FROM command is high risk, as any potentially introduced new bug that we don't happen to already have tests for can and will break customer queries immediately as this sees the light of day in Serverless.

Therefore, I'd like to double check if we can go with a simpler approach based on @jbaiera 's suggestion. That would IMO likely lead to simpler validation code and thus require less paranoid double checking during review to have good confidence in the change.

(I'm not saying the current code likely has bugs, just that I, as a human, am likely to overlook bugs in code that requires multiple nested loops handling quoted and unquoted index patterns and mutating the current cluster string and so on.)

alex-spies · 2025-04-30T12:51:52Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

-            }
+
+            hasSeenStar.set(hasSeenStar.get() || indexPattern.contains(WILDCARD));
+            validateClusterAndIndexPatterns(indexPattern, c, clusterString, selectorString, hasSeenStar.get());


validateClusterAndIndexPatterns gets handed indexPattern, c, clusterString, selectorString. That's a bit redundant, as c contains the other 3.

I think c is only really needed for source(ctx) inside validateClusterAndIndexPatterns, so I'd change the signature of validateClusterAndIndexPatterns to only require the Source, not the whole thing.

alex-spies · 2025-04-30T13:02:25Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

+             * Just because there was no clusterString before this index pattern does not mean that the indices
+             * are local indices. Patterns can be clubbed with remote names within quotes such as:
+             * "remote_one:remote_index,local_index". In this case, clusterString will be null.


Thank you for the nice comment!

alex-spies · 2025-04-30T13:22:43Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

+                 * Handle scenarios like remote_one:"index1,remote_two:index2". The clusterString here is
+                 * remote_one and is associated with index1 and not index2.


Heya, in a discussion with @jbaiera , we ended up suggesting that remote_one:"index1,remote_two:index2" should simply be disallowed; more generally, when there is a cluster string prefix, we should interpret the index pattern as being a single actual index pattern and not split by commas - commas should just be invalid.

That's not only simpler, but also less surprising to users. C.f. the table of suggested validations here.

To take this even further, I also brought up simply disallowing half quoted patterns like remote:"index" altogether; more specifically, quotes and the cluster string would be mutually exclusive. More specifically, this would mean that either

there is an unquoted cluster string and an unquoted index pattern and the index pattern is interpreted as a singular index pattern (no splitting at commas)

XOR

there is a single, quoted string and cluster string is null; then we first have to split at commas to get individual index patterns, resp. cluster string + index pattern pairs.
This more extreme suggestion would get rid of a bunch of edge cases that we need to cover here. E.g. remote":index, other_remote:other_index" would simply not occur, ever.

Both suggestions would avoid ambiguity from e.g. remote":index,other_index" as to whether the remote is supposed to apply to the first or both indices. They should also cut down on the finicky edge case handling that this PR currently has to implement. (Sorry about that, this looks super painful!)

I think we should settle whether we want to go with these suggestions or not before moving forward, as they would also affect the necessary testing and associated headaches. Since ESQL+CCS is not yet GA, we can only put these restrictions in place right now. Lifting them later is possible, but adding them later would be breaking.

alex-spies · 2025-04-30T13:37:15Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

-                    index = splitPattern.v1();
+
+            if (clusterString != null) {
+                if (selectorString != null) {


Heya, this is a bit out of this PR's scope, but the selectorString is treated as if it applied to every comma-split pattern.

Not a big deal - in #122651 (comment) James suggested that comma-split patterns in quotes just shouldn't mix selectors, like FROM "index1,index2"::data. This should be easy to guard against in a follow-up at a later time. /cc @jbaiera .

alex-spies · 2025-04-30T13:54:26Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

+            // throws exception if the selector expression is invalid. Selector resolution does not complain about exclusions
+            throw new ParsingException(e, source(ctx), e.getMessage());
+        }
+        hasSeenStar = hasSeenStar || index.contains(WILDCARD);


nit and out of scope: This is pre-existing, but I think this may be redundant, as we scan the whole index pattern for wildcards already up here.

alex-spies · 2025-04-30T14:11:57Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

            clusterAndIndexAsIndexPattern(command, "cluster*:*");
            clusterAndIndexAsIndexPattern(command, "*:index*");
            clusterAndIndexAsIndexPattern(command, "*:*");
+            expectError("FROM \"cluster:index|pattern\"", "Invalid index name [index|pattern], must not contain the following characters");


This should be

Suggested change

expectError("FROM \"cluster:index|pattern\"", "Invalid index name [index|pattern], must not contain the following characters");

expectError(command + " \"cluster:index|pattern\"", "Invalid index name [index|pattern], must not contain the following characters");

to also include TS in the tests.

Same below.

alex-spies · 2025-04-30T14:14:33Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

-            assertStringAsIndexPattern("cluster:index|pattern", command + " cluster:\"index|pattern\"");
-            assertStringAsIndexPattern("*:index|pattern", command + " \"*:index|pattern\"");


When you added these with an expectError below, I think you changed the quoting. Let's add these test cases back in with the original quoting, in addition to the ones you added.

Additionally, these cases are only invalid because of the | character in the index pattern. Let's add these test cases back in with valid index patterns, e.g.

assertStringAsIndexPattern("*:index", command + " \"*:index\""); assertStringAsIndexPattern("*:index*pattern", command + " \"*:index*pattern\""); assertStringAsIndexPattern("*:index", command + " \"*:index\""); assertStringAsIndexPattern("*:index*pattern", command + " \"*:index*pattern\"");

alex-spies · 2025-04-30T14:18:00Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

@@ -488,15 +489,12 @@ public void testStringAsIndexPattern() {
            clusterAndIndexAsIndexPattern(command, "cluster:index");
            clusterAndIndexAsIndexPattern(command, "cluster:.index");
            clusterAndIndexAsIndexPattern(command, "cluster*:index*");
-            clusterAndIndexAsIndexPattern(command, "cluster*:<logstash-{now/D}>*");// this is not a valid pattern, * should be inside <>
-            clusterAndIndexAsIndexPattern(command, "cluster*:<logstash-{now/D}*>");


@pawankartik-elastic , it's hard for me to validate that the case FROM + cluster*:<logstash-{now/D}>* and cluster*:<logstash-{now/D}*> is still tested. Could you point me to where exactly this is tested, or just add these back in with an appropriate expectError just so human readers can easily find this case covered? (Maybe it's covered below in testInvalidPatternsWithIntermittentQuotes, but that's harder to parse for humans.)

Actually, I just tested these two expressions and except for the fact that it should be a lower case d, these come through as valid just fine. So I think these cases can be left in place but need to be slightly fixed:

clusterAndIndexAsIndexPattern(command, "cluster*:<logstash-{now/d}>*");// this is not a valid pattern, * should be inside <> clusterAndIndexAsIndexPattern(command, "cluster*:<logstash-{now/d}*>");

alex-spies · 2025-04-30T14:38:52Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

-            clustersAndIndices(command, "index*", "-index#pattern");
-            clustersAndIndices(command, "*", "-<--logstash-{now/M{yyyy.MM}}>");
-            clustersAndIndices(command, "index*", "-<--logstash#-{now/M{yyyy.MM}}>");
+            expectInvalidIndexNameErrorWithLineNumber(command, "*, index#pattern", lineNumber, "index#pattern", "must not contain '#'");


@fang-xing-esql , are you sure this is a behavior change? I just ran this against main and got an invalid index name exception with a 400:

curl -u elastic:password -X POST 'localhost:9200/_query?format=txt' -H 'Content-Type: application/json' '-d 16:36 { "query": "from *, index#pattern" } ' {"error":{"root_cause":[{"type":"invalid_index_name_exception","reason":"Invalid index name [index#pattern], must not contain '#'","index_uuid":"_na_","index":"index#pattern"}],"type":"parsing_exception","reason":"line 1:9: Invalid index name [index#pattern], must not contain '#'","caused_by":{"type":"invalid_index_name_exception","reason":"Invalid index name [index#pattern], must not contain '#'","index_uuid":"_na_","index":"index#pattern"}},"status":400}%

Both added tests here seem to behave the same way as on main in my short manual test.

The code is non-trivial so I may not have read this correctly, but it appears to me that the behavior of the validation for non-remote index patterns remains consistent with this change.

alex-spies · 2025-04-30T14:50:59Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

-                clustersAndIndices(command, "*", "-index#pattern::data");
-                clustersAndIndices(command, "*", "-index#pattern::data");
-                clustersAndIndices(command, "index*", "-index#pattern::data");
-                clustersAndIndices(command, "*", "-<--logstash-{now/M{yyyy.MM}}>::data");
-                clustersAndIndices(command, "index*", "-<--logstash#-{now/M{yyyy.MM}}>::data");


Why is this being removed? It doesn't fail for me when adding it back in.

We should probably fail this due to the #, I believe, but I'd rather add a comment like "this should actually fail due to the #", so that @jbaiera can maybe come back to this as part of the selectors work.

Another good thing to do would be to add a correct positive version of these cases, e.g.

clustersAndIndices(command, "*", "-index::data"); clustersAndIndices(command, "*", "-index*pattern::data"); ...

Check if index patterns conform to valid format before validation

8f324ae

pawankartik-elastic added >bug Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch :Search Foundations/CCS v8.19.0 v9.1.0 labels Feb 13, 2025

Update docs/changelog/122497.yaml

1ddbce4

pawankartik-elastic added 2 commits February 13, 2025 13:47

Let validateClusterString() look for REMOTE_CLUSTER_INDEX_SEPARATOR

46758cb

in cluster name

Merge branch 'main' into pkar/index-pattern-check

2f2ee85

alex-spies self-requested a review February 13, 2025 17:31

alex-spies requested changes Feb 17, 2025

View reviewed changes

astefan requested a review from fang-xing-esql February 17, 2025 15:05

astefan added :Analytics/ES|QL AKA ESQL v9.0.0 and removed :Search Foundations/CCS labels Feb 17, 2025

astefan requested changes Feb 17, 2025

View reviewed changes

alex-spies mentioned this pull request Feb 19, 2025

ESQL: Quotations in index patterns allow for inconsistent pattern composition #122651

Open

jbaiera mentioned this pull request Mar 19, 2025

Support index pattern selector syntax in ES|QL #120660

Merged

Merge branch 'main' into pkar/index-pattern-check

7ca6d70

github-actions bot deployed to docs-preview April 7, 2025 14:54 View deployment

Mute 3 cases till further clarification and fix error string message

9dd96df

github-actions bot deployed to docs-preview April 7, 2025 14:58 View deployment

pawankartik-elastic removed the v9.0.0 label Apr 7, 2025

pawankartik-elastic changed the title ~~Check if index patterns conform to valid format before validation~~ ES|QL: Check if cluster aliases and index patterns are valid before executing query Apr 7, 2025

Fix tests

0686c8f

Fix bug in breaking down indices

7c55d6c

github-actions bot deployed to docs-preview April 8, 2025 08:57 View deployment

Merge branch 'main' into pkar/index-pattern-check

2653377

github-actions bot deployed to docs-preview April 10, 2025 10:42 View deployment

Tiny refactoring around how wildcard is processed and added tests

5832ff0

github-actions bot deployed to docs-preview April 10, 2025 10:46 View deployment

[CI] Auto commit changes from spotless

6cdbf5a

github-actions bot deployed to docs-preview April 10, 2025 10:55 View deployment

Drop duplicated test cases and fix flaky-ness caused by quoting

c2160bb

github-actions bot deployed to docs-preview April 10, 2025 12:25 View deployment

pawankartik-elastic added auto-backport Automatically create backport pull requests when merged v9.0.1 labels Apr 11, 2025

pawankartik-elastic marked this pull request as ready for review April 11, 2025 15:55

elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) and removed Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch labels Apr 11, 2025

pawankartik-elastic commented Apr 11, 2025

View reviewed changes

Set cluster string to null when it cannot be associated with an index

544171b

pattern

github-actions bot deployed to docs-preview April 11, 2025 16:20 View deployment

pawankartik-elastic added 2 commits April 11, 2025 18:00

Generate correct invalid patterns

133a7c7

Merge branch 'main' into pkar/index-pattern-check

7da29d8

github-actions bot deployed to docs-preview April 11, 2025 17:00 View deployment

fang-xing-esql reviewed Apr 15, 2025

View reviewed changes

Address review comments and don't break indices into its constituents

5c70d51

1. Wrapped `splitIndexName()` in try catch and ensured it throws parse exception. 2. Relaxed the validation to allow excluding garbage. 3. Validation no longer breaks patterns on pipe char as it's an invalid char.

github-actions bot deployed to docs-preview April 16, 2025 13:44 View deployment

alex-spies reviewed Apr 30, 2025

View reviewed changes

elasticsearchmachine added v9.0.2 and removed v9.0.1 labels Apr 30, 2025

		expectError("FROM remote:\"foo:bar\"", "Unexpected index separator in index patter");
		expectError("FROM \"remote:foo:bar:baz\"", "Unexpected index separator in index patter");

		* Handle scenarios like remote_one:"index1,remote_two:index2". The clusterString here is
		* remote_one and is associated with index1 and not index2.

	expectError("FROM \"cluster:index\|pattern\"", "Invalid index name [index\|pattern], must not contain the following characters");
	expectError(command + " \"cluster:index\|pattern\"", "Invalid index name [index\|pattern], must not contain the following characters");

		assertStringAsIndexPattern("cluster:index\|pattern", command + " cluster:\"index\|pattern\"");
		assertStringAsIndexPattern(":index\|pattern", command + " \":index\|pattern\"");

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

Are you sure you want to change the base?

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

Conversation

pawankartik-elastic commented Feb 13, 2025 • edited Loading

elasticsearchmachine commented Feb 13, 2025

pawankartik-elastic commented Feb 13, 2025 • edited Loading

alex-spies commented Feb 17, 2025 • edited Loading

alex-spies left a comment • edited Loading

Choose a reason for hiding this comment

alex-spies Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

alex-spies Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawankartik-elastic commented Feb 17, 2025

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawankartik-elastic commented Feb 17, 2025

elasticsearchmachine commented Apr 11, 2025

pawankartik-elastic Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fang-xing-esql left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawankartik-elastic Apr 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawankartik-elastic Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawankartik-elastic commented Apr 16, 2025 • edited Loading

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawankartik-elastic commented Feb 13, 2025 •

edited

Loading

pawankartik-elastic commented Feb 13, 2025 •

edited

Loading

alex-spies commented Feb 17, 2025 •

edited

Loading

alex-spies left a comment •

edited

Loading

alex-spies Feb 17, 2025 •

edited

Loading

alex-spies Feb 17, 2025 •

edited

Loading

pawankartik-elastic Apr 11, 2025 •

edited

Loading

pawankartik-elastic Apr 15, 2025 •

edited

Loading

pawankartik-elastic Apr 16, 2025 •

edited

Loading

pawankartik-elastic commented Apr 16, 2025 •

edited

Loading