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

Ensure cluster string could be quoted #120355

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/120355.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 120355
summary: Ensure cluster string could be quoted
area: ES|QL
type: enhancement
issues: []
1 change: 1 addition & 0 deletions x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ indexPattern

clusterString
: UNQUOTED_SOURCE
| QUOTED_STRING
;
Comment on lines 144 to 147
Copy link
Member

Choose a reason for hiding this comment

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

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.


indexString
Expand Down

Large diffs are not rendered by default.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.elasticsearch.cluster.metadata.MetadataCreateIndexService;
import org.elasticsearch.common.Strings;
import org.elasticsearch.indices.InvalidIndexNameException;
import org.elasticsearch.transport.RemoteClusterService;
import org.elasticsearch.xpack.esql.core.util.Holder;
import org.elasticsearch.xpack.esql.parser.EsqlBaseParser.IdentifierContext;
import org.elasticsearch.xpack.esql.parser.EsqlBaseParser.IndexStringContext;
Expand Down Expand Up @@ -51,29 +52,51 @@ protected static String quoteIdString(String unquotedString) {
return "`" + unquotedString.replace("`", "``") + "`";
}

@Override
public String visitClusterString(EsqlBaseParser.ClusterStringContext ctx) {
if (ctx == null) {
return null;
} else if (ctx.UNQUOTED_SOURCE() != null) {
return ctx.UNQUOTED_SOURCE().getText();
} else {
return unquote(ctx.QUOTED_STRING().getText());
}
Comment on lines +57 to +63
Copy link
Member

Choose a reason for hiding this comment

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

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

}

@Override
public String visitIndexString(IndexStringContext ctx) {
TerminalNode n = ctx.UNQUOTED_SOURCE();
return n != null ? n.getText() : unquote(ctx.QUOTED_STRING().getText());
if (ctx.UNQUOTED_SOURCE() != null) {
return ctx.UNQUOTED_SOURCE().getText();
} else {
return unquote(ctx.QUOTED_STRING().getText());
}
}

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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

} else {
validateClusterString(clusterString, c);
}
patterns.add(clusterString != null ? clusterString + REMOTE_CLUSTER_INDEX_SEPARATOR + indexPattern : indexPattern);
});
return Strings.collectionToDelimitedString(patterns, ",");
}

protected static void validateClusterString(String clusterString, EsqlBaseParser.IndexPatternContext ctx) {
if (clusterString.indexOf(RemoteClusterService.REMOTE_CLUSTER_INDEX_SEPARATOR) != -1) {
throw new ParsingException(source(ctx), "cluster string [{}] must not contain ':'", clusterString);
}
}

private static void validateIndexPattern(String indexPattern, EsqlBaseParser.IndexPatternContext ctx, boolean hasSeenStar) {
// multiple index names can be in the same double quote, e.g. indexPattern = "idx1, *, -idx2"
String[] indices = indexPattern.split(",");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public static String randomIndexPattern(Feature... features) {

var pattern = maybeQuote(index.toString());
if (canAdd(Features.CROSS_CLUSTER, features)) {
var cluster = randomIdentifier();
var cluster = maybeQuote(randomIdentifier());
pattern = maybeQuote(cluster + ":" + pattern);
}

Expand Down