Browse Source

EQL: Fix sequences with conditions involving keys and non-keys (#133134) (#133384)

Luigi Dell'Aquila 1 month ago
parent
commit
ec7e5e8cf3

+ 5 - 0
docs/changelog/133134.yaml

@@ -0,0 +1,5 @@
+pr: 133134
+summary: Fix sequences with conditions involving keys and non-keys
+area: EQL
+type: bug
+issues: []

+ 1 - 1
x-pack/plugin/eql/qa/rest/build.gradle

@@ -12,7 +12,7 @@ apply plugin: 'elasticsearch.internal-test-artifact'
 
 restResources {
   restApi {
-    include '_common', 'bulk', 'indices', 'eql'
+    include '_common', 'bulk', 'indices', 'eql', 'capabilities'
   }
 }
 

+ 119 - 0
x-pack/plugin/eql/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/eql/70_functions_on_keys.yml

@@ -0,0 +1,119 @@
+---
+setup:
+  - requires:
+      test_runner_features: [ capabilities ]
+      capabilities:
+        - method: POST
+          path: /{index}/_eql/search
+          parameters: [ ]
+          capabilities: [ filters_on_keys_fix ]
+      reason: "Testing a recent fix"
+  - do:
+      indices.create:
+          index:  eql_test
+          body:
+            {
+              "mappings": {
+                "properties": {
+                  "@timestamp": {
+                    "type": "date"
+                  },
+                  "event": {
+                    "properties": {
+                      "type": {
+                        "type": "keyword",
+                        "ignore_above": 1024
+                      }
+                    }
+                  },
+                  "user": {
+                    "properties": {
+                      "domain": {
+                        "type": "keyword",
+                        "ignore_above": 1024
+                      },
+                      "name": {
+                        "type": "keyword",
+                        "fields": {
+                          "text": {
+                            "type": "match_only_text"
+                          }
+                        }
+                      }
+                    }
+                  },
+                  "winlog": {
+                    "dynamic": "true",
+                    "properties": {
+                      "computer_name": {
+                        "type": "keyword",
+                        "ignore_above": 1024
+                      }
+                    }
+                  },
+                  "source": {
+                    "properties": {
+                      "ip": {
+                        "type": "ip"
+                      }
+                    }
+                  }
+                }
+              }
+            }
+  - do:
+      bulk:
+        refresh: true
+        body:
+          - index:
+              _index: eql_test
+              _id:    "1"
+          - "winlog":
+              "computer_name": "foo.bar.baz"
+            "@timestamp": "2025-06-18T12:21:37.018Z"
+            "event":
+              "category": "authentication"
+              "code": "5145"
+            "user":
+              "domain": "bar.baz"
+              "name": "something"
+            "source":
+              "ip": "192.168.56.200"
+          - index:
+              _index: eql_test
+              _id:    "2"
+          - "winlog":
+              "computer_name": "foo.bar.baz"
+            "@timestamp": "2025-06-18T12:21:37.093Z"
+            "event":
+              "category": "authentication"
+            "user":
+              "domain": "BAR.BAZ"
+              "name": "foo$"
+            "source":
+              "ip": "192.168.56.200"
+
+---
+"Test one join key":
+  - do:
+      eql.search:
+        index: eql_test
+        body:
+          query: 'sequence by source.ip with maxspan=5s [any where event.code : "5145" and winlog.computer_name == "foo.bar.baz" ] [any where winlog.computer_name == "foo.bar.baz" and startswith(winlog.computer_name, substring(user.name, 0, -1)) ]'
+
+  - match: {timed_out: false}
+  - match: {hits.total.value: 1}
+  - match: {hits.total.relation: "eq"}
+
+---
+"Test two join keys ":
+  - do:
+      eql.search:
+        index: eql_test
+        body:
+          query: 'sequence by source.ip, winlog.computer_name with maxspan=5s [any where event.code : "5145" and winlog.computer_name == "foo.bar.baz" ] [any where winlog.computer_name == "foo.bar.baz" and startswith(winlog.computer_name, substring(user.name, 0, -1)) ]'
+
+  - match: {timed_out: false}
+  - match: {hits.total.value: 1}
+  - match: {hits.total.relation: "eq"}
+

+ 22 - 15
x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java

@@ -23,6 +23,7 @@ import org.elasticsearch.xpack.eql.plan.physical.LocalRelation;
 import org.elasticsearch.xpack.eql.session.Payload.Type;
 import org.elasticsearch.xpack.eql.util.MathUtils;
 import org.elasticsearch.xpack.eql.util.StringUtils;
+import org.elasticsearch.xpack.ql.expression.Attribute;
 import org.elasticsearch.xpack.ql.expression.Expression;
 import org.elasticsearch.xpack.ql.expression.FieldAttribute;
 import org.elasticsearch.xpack.ql.expression.Literal;
@@ -416,23 +417,29 @@ public class Optimizer extends RuleExecutor<LogicalPlan> {
 
             List<Expression> and = Predicates.splitAnd(condition);
             for (Expression exp : and) {
-                // if there are no conjunction and at least one key matches, save the expression along with the key
-                // and its ordinal so it can be replaced
-                if (exp.anyMatch(Or.class::isInstance) == false) {
-                    // comparisons against variables are not done
-                    // hence why on the first key match, the expression is picked up
-                    exp.anyMatch(e -> {
-                        for (int i = 0; i < keys.size(); i++) {
-                            Expression key = keys.get(i);
-                            if (e.semanticEquals(key)) {
-                                constraints.add(new Constraint(exp, filter, i));
-                                return true;
-                            }
-                        }
-                        return false;
-                    });
+                // if the expression only involves filter keys, it's simple enough (eg. there are no conjunction), and at least one key
+                // matches, save the expression along with the key and its ordinal so it can be replaced
+                if (exp.anyMatch(Or.class::isInstance)) {
+                    continue;
+                }
+
+                // expressions that involve attributes other than the keys have to be discarded
+                if (exp.anyMatch(x -> x instanceof Attribute && keys.stream().noneMatch(k -> x.semanticEquals(k)))) {
+                    continue;
                 }
+
+                exp.anyMatch(e -> {
+                    for (int i = 0; i < keys.size(); i++) {
+                        Expression key = keys.get(i);
+                        if (e.semanticEquals(key)) {
+                            constraints.add(new Constraint(exp, filter, i));
+                            return true;
+                        }
+                    }
+                    return false;
+                });
             }
+
             return constraints;
         }
 

+ 26 - 0
x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/EqlCapabilities.java

@@ -0,0 +1,26 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+package org.elasticsearch.xpack.eql.plugin;
+
+import java.util.HashSet;
+import java.util.Set;
+
+public final class EqlCapabilities {
+
+    private EqlCapabilities() {}
+
+    /** Fix bug on filters that include join keys https://github.com/elastic/elasticsearch/issues/133065 */
+    private static final String FILTERS_ON_KEYS_FIX = "filters_on_keys_fix";
+
+    public static final Set<String> CAPABILITIES;
+    static {
+        HashSet<String> capabilities = new HashSet<>();
+        capabilities.add(FILTERS_ON_KEYS_FIX);
+        CAPABILITIES = Set.copyOf(capabilities);
+    }
+}

+ 6 - 0
x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/RestEqlSearchAction.java

@@ -29,6 +29,7 @@ import org.elasticsearch.xpack.eql.action.EqlSearchResponse;
 
 import java.io.IOException;
 import java.util.List;
+import java.util.Set;
 
 import static org.elasticsearch.rest.RestRequest.Method.GET;
 import static org.elasticsearch.rest.RestRequest.Method.POST;
@@ -116,4 +117,9 @@ public class RestEqlSearchAction extends BaseRestHandler {
     public String getName() {
         return "eql_search";
     }
+
+    @Override
+    public Set<String> supportedCapabilities() {
+        return EqlCapabilities.CAPABILITIES;
+    }
 }

+ 112 - 0
x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java

@@ -12,6 +12,7 @@ import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.xpack.eql.analysis.Analyzer;
 import org.elasticsearch.xpack.eql.analysis.PostAnalyzer;
 import org.elasticsearch.xpack.eql.analysis.PreAnalyzer;
+import org.elasticsearch.xpack.eql.expression.function.scalar.string.StartsWith;
 import org.elasticsearch.xpack.eql.expression.function.scalar.string.ToString;
 import org.elasticsearch.xpack.eql.parser.EqlParser;
 import org.elasticsearch.xpack.eql.plan.logical.AbstractJoin;
@@ -576,6 +577,113 @@ public class OptimizerTests extends ESTestCase {
         assertEquals(ruleBCondition, filterCondition(query2.child().children().get(0)));
     }
 
+    /**
+     * sequence
+     * 1. filter startsWith(a, "foo") by a
+     * 2. filter X by a
+     * ==
+     * 1. filter startsWith(a, "foo") by a
+     * 2. filter startsWith(a, "foo") by a
+     *    \filter X
+     */
+    public void testKeyConstraintWithFunction() {
+        Attribute a = key("a");
+
+        Expression keyCondition = startsWithExp(a, new Literal(EMPTY, "foo", DataTypes.KEYWORD));
+        Expression filter = equalsExpression();
+
+        KeyedFilter rule1 = keyedFilter(basicFilter(keyCondition), a);
+        KeyedFilter rule2 = keyedFilter(basicFilter(filter), a);
+
+        AbstractJoin j = randomSequenceOrSample(rule1, rule2);
+
+        List<KeyedFilter> queries = j.queries();
+        assertEquals(rule1, queries.get(0));
+        assertEquals(keyCondition, filterCondition(queries.get(1).child()));
+        assertEquals(filterCondition(rule2.child()), filterCondition(queries.get(1).child().children().get(0)));
+    }
+
+    /**
+     * sequence
+     * 1. filter startsWith(a, b) by a
+     * 2. filter X by a
+     * ==
+     * same
+     */
+    public void testKeyConstraintWithNonKey() {
+        Attribute a = key("a");
+
+        Expression keyCondition = startsWithExp(a, key("b"));
+        Expression filter = equalsExpression();
+
+        KeyedFilter rule1 = keyedFilter(basicFilter(keyCondition), a);
+        KeyedFilter rule2 = keyedFilter(basicFilter(filter), a);
+
+        AbstractJoin j = randomSequenceOrSample(rule1, rule2);
+
+        List<KeyedFilter> queries = j.queries();
+        assertEquals(rule1, queries.get(0));
+        assertEquals(rule2, queries.get(1));
+    }
+
+    /**
+     * sequence
+     * 1. filter startsWith(a, b) and c > 10 by a, c
+     * 2. filter X by a, c
+     * ==
+     * 1. filter startsWith(a, b) and c > 10 by a, c
+     * 2. filter c > 10 by a, c
+     *    \filter X
+     */
+    public void testKeyConstraintWithNonKeyPartialPropagation() {
+        Attribute a = key("a");
+        Attribute b = key("b");
+        Attribute c = key("c");
+
+        GreaterThan gtExp = gtExpression(c);
+        Expression keyCondition = new And(EMPTY, startsWithExp(a, b), gtExp);
+        Expression filter = equalsExpression();
+
+        KeyedFilter rule1 = keyedFilter(basicFilter(keyCondition), a, c);
+        KeyedFilter rule2 = keyedFilter(basicFilter(filter), a, c);
+
+        AbstractJoin j = randomSequenceOrSample(rule1, rule2);
+
+        List<KeyedFilter> queries = j.queries();
+        assertEquals(rule1, queries.get(0));
+        assertEquals(gtExp, filterCondition(queries.get(1).child()));
+        assertEquals(filterCondition(rule2.child()), filterCondition(queries.get(1).child().children().get(0)));
+    }
+
+    /**
+     * sequence
+     * 1. filter startsWith(a, b)  by a, c
+     * 2. filter X and c > 10 by a, c
+     * ==
+     * 1. filter c > 10 by a, c
+     *    \filter startsWith(a, b)
+     * 2. filter X and c > 10 by a, c
+     */
+    public void testKeyConstraintWithNonKeyPartialReversePropagation() {
+        Attribute a = key("a");
+        Attribute b = key("b");
+        Attribute c = key("c");
+
+        GreaterThan gtExp = gtExpression(c);
+        Expression keyCondition = startsWithExp(a, b);
+        Expression filter = new And(EMPTY, equalsExpression(), gtExp);
+
+        KeyedFilter rule1 = keyedFilter(basicFilter(keyCondition), a, c);
+        KeyedFilter rule2 = keyedFilter(basicFilter(filter), a, c);
+
+        AbstractJoin j = randomSequenceOrSample(rule1, rule2);
+
+        List<KeyedFilter> queries = j.queries();
+        assertEquals(gtExp, filterCondition(queries.get(0).child()));
+        assertEquals(filterCondition(rule1.child()), filterCondition(queries.get(0).child().children().get(0)));
+        assertEquals(rule2, queries.get(1));
+    }
+
     /**
      * Key conditions inside a disjunction (OR) are ignored
      * <p>
@@ -817,4 +925,8 @@ public class OptimizerTests extends ESTestCase {
     private static GreaterThan gtExpression(Attribute b) {
         return new GreaterThan(EMPTY, b, new Literal(EMPTY, 1, INTEGER), UTC);
     }
+
+    private static StartsWith startsWithExp(Expression a, Expression b) {
+        return new StartsWith(EMPTY, a, b, randomBoolean());
+    }
 }