Ensure cluster string could be quoted #120355

idegtiarenko · 2025-01-17T09:19:56Z

Currently we accept "remote:index", remote:"index" but not "remote":"index" as a valid index pattern. This change fixes this.

elasticsearchmachine · 2025-01-17T09:21:20Z

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

elasticsearchmachine · 2025-01-17T09:21:22Z

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

fang-xing-esql · 2025-01-17T19:22:48Z

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

@@ -615,6 +615,55 @@ private void clustersAndIndices(String command, String indexString1, String inde
        );
    }

+    public void testValidQuotingFromIndexPattern() {


The coverage looks pretty good to me, there is one negative case that I can think of. According to RemoteClusterAware.isRemoteIndexName, : is a valid character in the index pattern used to separate cluster and index name, it is not a valid character for index name, I wonder if it is a valid character for the cluster name?

The following queries can pass the grammar and parser, they errors out, which look correct, however the error message does not look quite clear.

+ curl -u elastic:password -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d { "query": "FROM \"remot:e\":existing_index" } ' { "error" : { "root_cause" : [ { "type" : "no_such_remote_cluster_exception", "reason" : "no such remote cluster: [remot]" } ], "type" : "no_such_remote_cluster_exception", "reason" : "no such remote cluster: [remot]" }, "status" : 404 } + curl -u elastic:password -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d { "query": "FROM \"remote:\":existing_index" } ' { "error" : { "root_cause" : [ { "type" : "invalid_index_name_exception", "reason" : "Invalid index name [remote::existing_index], Invalid usage of :: separator, [existing_index] is not a recognized selector", "index_uuid" : "_na_", "index" : "remote::existing_index" } ], "type" : "invalid_index_name_exception", "reason" : "Invalid index name [remote::existing_index], Invalid usage of :: separator, [existing_index] is not a recognized selector", "index_uuid" : "_na_", "index" : "remote::existing_index" }, "status" : 400 }

RemoteClusterAware.isRemoteIndexName, : is a valid character in the index pattern used to separate cluster and index name, it is not a valid character for index name, I wonder if it is a valid character for the cluster name?

I believe : can not be used as a character in a cluster name.

According to

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

Lines 91 to 114 in c3839e1

/**

* Split the index name into remote cluster alias and index name.

* The index expression is assumed to be individual index (no commas) but can contain `-`, wildcards,

* datemath, remote cluster name and any other syntax permissible in index expression component.

* There's no guarantee the components actually represent existing remote cluster or index, only

* rudimentary checks are done on the syntax.

*/

public static String[] splitIndexName(String indexExpression) {

if (indexExpression.isEmpty() || indexExpression.charAt(0) == '<' || indexExpression.startsWith("-<")) {

// This is date math, but even if it is not, the remote can't start with '<'.

// Thus, whatever it is, this is definitely not a remote index.

return new String[] { null, indexExpression };

}

int i = indexExpression.indexOf(RemoteClusterService.REMOTE_CLUSTER_INDEX_SEPARATOR);

if (i == 0) {

throw new IllegalArgumentException("index name [" + indexExpression + "] is invalid because the remote part is empty");

}

if (i < 0 || indexExpression.startsWith(SelectorResolver.SELECTOR_SEPARATOR, i)) {

// Either no colon present, or the colon was a part of a selector separator (::)

return new String[] { null, indexExpression };

} else {

return new String[] { indexExpression.substring(0, i), indexExpression.substring(i + 1) };

}

}

we rely on finding the first : when splitting cluster name and index pattern in indexExpression. This would lead to an indexPattern with : when multiple : used that is not permitted.

Also when registering a remote with : I am getting the following:

PUT http://localhost:9200/_cluster/settings Content-Type: application/json { "persistent" : { "cluster.remote.remote:1.seeds" : ["127.0.0.1:9301"] } }

{ "error": { "root_cause": [ { "type": "illegal_argument_exception", "reason": "persistent setting [cluster.remote.remote:1.seeds], not recognized" } ], "type": "illegal_argument_exception", "reason": "persistent setting [cluster.remote.remote:1.seeds], not recognized" }, "status": 400 }

I believe above indicates that we do not support : in cluster names.

I added basic cluster string validation in d58db78.

Please note below indexPatterns are not checked:

FROM remote:invalid:index FROM remote:"invalid:index"

as we skip all validation when detecting remote:

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

Lines 83 to 86 in d58db78

if (clusterString == null) {

hasSeenStar.set(indexPattern.contains(WILDCARD) || hasSeenStar.get());

validateIndexPattern(indexPattern, c, hasSeenStar.get());

} else {

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

Lines 105 to 107 in d58db78

if (isRemoteIndexName(index)) { // skip the validation if there is remote cluster

continue;

}

fang-xing-esql

LGTM, thank you!

costin

LGTM

costin · 2025-01-22T03:46:07Z

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

 clusterString
    : UNQUOTED_SOURCE
+    | QUOTED_STRING
    ;


Since they are the same, point clusterString to indexString:

clusterString : indexString ;

We could fully remove it but it's worth keeping the element in for future changes.

costin · 2025-01-22T03:47:05Z

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

+        if (ctx == null) {
+            return null;
+        } else if (ctx.UNQUOTED_SOURCE() != null) {
+            return ctx.UNQUOTED_SOURCE().getText();
+        } else {
+            return unquote(ctx.QUOTED_STRING().getText());
+        }


See my comment in the grammar - this method can then be either remove or delegate to visitIndexString.

alex-spies

Heya, I agree this looks good and consistent.

However, do you know of an example where the added syntax enables us to specify a remote-index-pattern combination that was either not possible before, or was less ergonomic to express?

If we can construct one, it'd be great to have this in the PR's description and/or as a test.

Otherwise, I have only some minor remarks - and the added tests have some overlap with existing tests. Maybe it'd be nicer if we could at least move them closer together.

alex-spies · 2025-01-22T09:44:41Z

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

+        expectError("FROM \"remote:\":index", "line 1:6: cluster string [remote:] must not contain ':'");
+        expectError("FROM \"remote:invalid\":index", "line 1:6: cluster string [remote:invalid] must not contain ':'");
+    }
+
    public void testInvalidQuotingAsFromIndexPattern() {


Maybe we should also add tests for invalid quoting of the remote name itself for good measure.

alex-spies · 2025-01-22T09:47:24Z

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

+        return str.charAt(randomInt(str.length() - 1));
+    }
+
+    public void testInvalidFromIndexPattern() {


This is related to testInvalidCharacterInIndexPattern in the same file.

alex-spies · 2025-01-22T09:47:38Z

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

@@ -615,6 +615,60 @@ private void clustersAndIndices(String command, String indexString1, String inde
        );
    }

+    public void testValidFromIndexPattern() {


This has some overlap with testStringAsIndexPattern in the same file.

I think this test has the following drawbacks:

Only one kind of quoting (", but there's also still """).

None of the index patterns actually require quoting. Looking at testStringAsIndexPattern, there's e.g. date math stuff that can make quoting required.

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.interp # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

alex-spies · 2025-01-30T10:15:49Z

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

    }

    public String visitIndexPattern(List<EsqlBaseParser.IndexPatternContext> ctx) {
        List<String> patterns = new ArrayList<>(ctx.size());
        Holder<Boolean> hasSeenStar = new Holder<>(false);
        ctx.forEach(c -> {
            String indexPattern = visitIndexString(c.indexString());
-            String clusterString = c.clusterString() != null ? c.clusterString().getText() : null;
+            String clusterString = visitClusterString(c.clusterString());
            // 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) {
                hasSeenStar.set(indexPattern.contains(WILDCARD) || hasSeenStar.get());
                validateIndexPattern(indexPattern, c, hasSeenStar.get());


Hm, that's slightly out of scope but I just realized that the validation for the index pattern is lacking. For instance, you can use FROM "foo:bar:baz" and you'll not even get an error message.

What is a bit more in scope: at least when the cluster string is not null, we should probably validate that the index pattern is not a remote pattern. This applies to cases like FROM "remote":"index:pattern".

I just tried locally, with your branch:

$ curl -u elastic:password -H "Content-Type: application/json" "127.0.0.1:9200/_query?format=txt" -d ' { "query": "from cluster_one:\"remote:index\"" }' <no-fields> --------------- $ curl -u elastic:password -H "Content-Type: application/json" "127.0.0.1:9200/_query?format=txt" -d ' { "query": "from cluster_one:remote:index" }' {"error":{"root_cause":[{"type":"parsing_exception","reason":"line 1:24: mismatched input ':' expecting {<EOF>, '|', ',', 'metadata'}"}],"type":"parsing_exception","reason":"line 1:24: mismatched input ':' expecting {<EOF>, '|', ',', 'metadata'}","caused_by":{"type":"input_mismatch_exception","reason":null}},"status":400}% $ curl -u elastic:password -H "Content-Type: application/json" "127.0.0.1:9200/_query?format=txt" -d ' { "query": "from \"cluster_one:remote:index\"" }' <no-fields> ---------------

@idegtiarenko if you feel like it, I think we could add some validation for this as we're just touching this anyway. Otherwise, let's put what we found into an issue because that'll need to be fixed one day, anyway.

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.interp # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.java

Ensure cluster string could be quoted

d4d26c5

Currently we accept "remote:index", remote:"index" but not "remote":"index" as a valid index pattern. This change fixes this.

idegtiarenko added >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.0 labels Jan 17, 2025

idegtiarenko requested a review from alex-spies January 17, 2025 09:19

Update docs/changelog/120355.yaml

1989c3d

idegtiarenko added 2 commits January 17, 2025 10:23

upd

a944a60

upd

29187af

astefan requested review from costin and fang-xing-esql and removed request for alex-spies January 17, 2025 09:31

fang-xing-esql reviewed Jan 17, 2025

View reviewed changes

idegtiarenko requested a review from fang-xing-esql January 20, 2025 08:03

idegtiarenko added 3 commits January 20, 2025 11:25

remove unnecessary throws

c184382

add basic cluster string validation

d58db78

Merge branch 'main' into quoted_cluster_string

7b906c8

fang-xing-esql approved these changes Jan 21, 2025

View reviewed changes

costin approved these changes Jan 22, 2025

View reviewed changes

alex-spies approved these changes Jan 22, 2025

View reviewed changes

alex-spies mentioned this pull request Jan 24, 2025

Disallow CCS with lookup join #120277

Merged

alex-spies reviewed Jan 30, 2025

View reviewed changes

elasticsearchmachine added v8.19.0 and removed v8.18.0 labels Jan 30, 2025

Merge branch 'main' into quoted_cluster_string

2570e8d

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.interp # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.java

elasticsearchmachine added v9.1.0 and removed v9.0.0 labels Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure cluster string could be quoted #120355

Ensure cluster string could be quoted #120355

idegtiarenko commented Jan 17, 2025

elasticsearchmachine commented Jan 17, 2025

elasticsearchmachine commented Jan 17, 2025

fang-xing-esql Jan 17, 2025 •

edited

Loading

idegtiarenko Jan 20, 2025 •

edited

Loading

idegtiarenko Jan 20, 2025

fang-xing-esql left a comment

costin left a comment

costin Jan 22, 2025

costin Jan 22, 2025

alex-spies left a comment

alex-spies Jan 22, 2025

alex-spies Jan 22, 2025

alex-spies Jan 22, 2025

alex-spies Jan 30, 2025

alex-spies Jan 30, 2025

alex-spies Jan 30, 2025

	/**
	* Split the index name into remote cluster alias and index name.
	* The index expression is assumed to be individual index (no commas) but can contain `-`, wildcards,
	* datemath, remote cluster name and any other syntax permissible in index expression component.
	* There's no guarantee the components actually represent existing remote cluster or index, only
	* rudimentary checks are done on the syntax.
	*/
	public static String[] splitIndexName(String indexExpression) {
	if (indexExpression.isEmpty() \|\| indexExpression.charAt(0) == '<' \|\| indexExpression.startsWith("-<")) {
	// This is date math, but even if it is not, the remote can't start with '<'.
	// Thus, whatever it is, this is definitely not a remote index.
	return new String[] { null, indexExpression };
	}
	int i = indexExpression.indexOf(RemoteClusterService.REMOTE_CLUSTER_INDEX_SEPARATOR);
	if (i == 0) {
	throw new IllegalArgumentException("index name [" + indexExpression + "] is invalid because the remote part is empty");
	}
	if (i < 0 \|\| indexExpression.startsWith(SelectorResolver.SELECTOR_SEPARATOR, i)) {
	// Either no colon present, or the colon was a part of a selector separator (::)
	return new String[] { null, indexExpression };
	} else {
	return new String[] { indexExpression.substring(0, i), indexExpression.substring(i + 1) };
	}
	}

	if (clusterString == null) {
	hasSeenStar.set(indexPattern.contains(WILDCARD) \|\| hasSeenStar.get());
	validateIndexPattern(indexPattern, c, hasSeenStar.get());
	} else {

	if (isRemoteIndexName(index)) { // skip the validation if there is remote cluster
	continue;
	}

Ensure cluster string could be quoted #120355

Are you sure you want to change the base?

Ensure cluster string could be quoted #120355

Conversation

idegtiarenko commented Jan 17, 2025

elasticsearchmachine commented Jan 17, 2025

elasticsearchmachine commented Jan 17, 2025

fang-xing-esql Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

idegtiarenko Jan 20, 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

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

fang-xing-esql Jan 17, 2025 •

edited

Loading

idegtiarenko Jan 20, 2025 •

edited

Loading