EQL: Fix cidrMatch function fails to match when used in scripts by aleksmaus · Pull Request #56246 · elastic/elasticsearch

aleksmaus · 2020-05-05T21:04:16Z

Replaced the previous implementation with script function.
Addresses #55709

@astefan I think this will conflict with your changes on query folding with equal operator optimization, it generates a different script with function now.

elasticmachine · 2020-05-05T21:04:18Z

Pinging @elastic/es-ql (:Query Languages/EQL)

rw-access · 2020-05-05T21:16:13Z

+        Object o = field.fold();
+        ArrayList<Object> arr = new ArrayList<>(addresses.size());
        for (Expression address : addresses) {
-            final Equals eq = new Equals(source(), field, address);
-            func = (func == null) ? eq : new Or(source(), func, eq);
+            arr.add(address.fold());
+        }
+        return doProcess(o, arr);


Suggested change

Object o = field.fold();

ArrayList<Object> arr = new ArrayList<>(addresses.size());

for (Expression address : addresses) {

final Equals eq = new Equals(source(), field, address);

func = (func == null) ? eq : new Or(source(), func, eq);

arr.add(address.fold());

}

return doProcess(o, arr);

return doProcess(field.fold(), Expressions.fold(addresses));

rw-access · 2020-05-05T21:17:30Z

+    public ScriptTemplate asScript() {
+        ScriptTemplate leftScript = asScript(field);
+
+        List<Object> values = new ArrayList<>(new LinkedHashSet<>(foldListOfValues(addresses, field.dataType())));


Suggested change

List<Object> values = new ArrayList<>(new LinkedHashSet<>(foldListOfValues(addresses, field.dataType())));

List<Object> values = Expressions.fold(addresses)

this will not eliminate duplicated values as far as I see

rw-access · 2020-05-05T21:22:12Z

 process where cidrMatch(source_address, "10.0.0.0/8")
 ;
-"term":{"source_address":{"value":"10.0.0.0/8"
+"script":{"source":"InternalQlScriptUtils.nullSafeFilter(InternalEqlScriptUtils.cidrMatch(


Should we add an optimizer rule to handle this so that we don't lose the more performant term query?

Will probably have to do another pass for that after @astefan merges his PR that also will affect the query planning optimization for simple equals. Can adjust this PR if that change merges sooner, sometimes this week.

…o script

aleksmaus · 2020-05-12T22:01:07Z

Updated PR to use term query for cidrMatch when possible otherwise fallback to script.
Thanks @astefan for pointers!

rw-access · 2020-05-12T22:08:19Z

+                if (cm.field() instanceof FieldAttribute && Expressions.foldable(cm.addresses())) {
+                    String targetFieldName = handler.nameOf(((FieldAttribute) cm.field()).exactAttribute());
+
+                    Set<Object> set = new LinkedHashSet<>(CollectionUtils.mapSize(cm.addresses().size()));


Is this worthwhile to add as a method? It's also called within CidrMatch.asScript

Not exactly, needed Set here for TermsQuery constructor and List for the scripting func

aleksmaus · 2020-05-12T22:08:44Z

+            new ExpressionTranslators.StringQueries(),
+            new ExpressionTranslators.Matches(),
+            new ExpressionTranslators.MultiMatches(),
+            new Scalars()


preferred without static importing everything here, more visible QL translators vs EQL.

A relevant aspect here: you might want to update your branch from master, to incorporate the StartsWith changes from #56404. In that PR, the Scalars from QL was updated to have startsWith use a PrefixQuery whenever possible. And for EQL and its QueryTranslator that you added, it will probably need the Scalars from QL, as well.

Updated, and adjusted Scalar translation in EQ to be able to reuse it for startsWith function

astefan

Looks good in general, but I have some comments as well.

astefan · 2020-05-13T06:59:40Z

+
+    public static boolean isInRange(String address, String... cidrAddresses) {
+        // Check if address is parsable first
+        byte[] addr = InetAddresses.forString(address).getAddress();


Since this code is in EQL, maybe we should handle the IllegalArgumentException here and wrap it in EqlIllegalArgumentException?

astefan · 2020-05-13T07:32:07Z

+            new ExpressionTranslators.StringQueries(),
+            new ExpressionTranslators.Matches(),
+            new ExpressionTranslators.MultiMatches(),
+            new Scalars()


A relevant aspect here: you might want to update your branch from master, to incorporate the StartsWith changes from #56404. In that PR, the Scalars from QL was updated to have startsWith use a PrefixQuery whenever possible. And for EQL and its QueryTranslator that you added, it will probably need the Scalars from QL, as well.

astefan · 2020-05-13T07:39:19Z

+import org.elasticsearch.xpack.ql.type.DataType;
+import org.elasticsearch.xpack.ql.type.DataTypeConverter;
+
+public class EqlTranslatorHandler implements TranslatorHandler {


This class is the same as QlTranslatorHandler except asQuery method. Maybe you could reuse most of the code from QlTranslatorHandler and only change the relevant bit in EqlTranslatorHandler. For example, why not extending QlTranslatorHandler and override asQuery only?

astefan · 2020-05-13T07:47:24Z

+{"terms":{"source_address":["10.0.0.0/8"]
 ;

 cidrMatchFunctionTwo


Please, add one more test where there are two cidrMatch function calls in the same query: where cidrMatch(whatever) or cidrMatch(another_whatever).

astefan · 2020-05-13T20:10:56Z

+process where cidrMatch(source_address, "10.0.0.0/8") or cidrMatch(source_address, "192.168.0.0/16")
+;
+{"bool":{"must":[{"term":{"event.category":{"value":"process"
+{"terms":{"source_address":["10.0.0.0/8"],"boost":1.0}},{"terms":{"source_address":["192.168.0.0/16"]


Indeed, it is relevant to have both terms queries in there and the must for event.category, but there has to be a bool should as well.
The query should translate as a bool with two must statements:

one is term on event.category

the other is a bool with two should statements where terms on the source_address is used.

astefan

LGTM

…tic#56246) Addresses elastic#55709

…) (#56735) EQL: Fix cidrMatch function fails to match when used in scripts (#56246) Addresses #55709

EQL: Fix cidrMatch function fails to match when used in scripts

eaf7bb7

aleksmaus added >bug :Analytics/EQL EQL querying Team:QL (Deprecated) Meta label for query languages team labels May 5, 2020

aleksmaus requested review from astefan, costin, imotov and rw-access May 5, 2020 21:04

rw-access reviewed May 5, 2020

View reviewed changes

Comment thread ...l/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/CIDRMatch.java Outdated

rw-access reviewed May 5, 2020

View reviewed changes

Comment thread ...l/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/CIDRUtils.java

aleksmaus added 4 commits May 6, 2020 16:48

Merge branch 'master' into fix/cidrmatch

8b0ea23

Address code review

f84ec05

Merge branch 'master' into fix/cidrmatch

087273c

Translate cidrMatch to term query when possible, otherwise fallback t…

236a1d6

…o script

Add one more test case for cidrMatch fallback to script

e00c92a

rw-access reviewed May 12, 2020

View reviewed changes

aleksmaus commented May 12, 2020

View reviewed changes

rw-access approved these changes May 12, 2020 •

edited

Loading

View reviewed changes

astefan reviewed May 13, 2020

View reviewed changes

aleksmaus added 2 commits May 13, 2020 09:21

Merge branch 'master' into fix/cidrmatch

dcf6645

Address code review comments from Andrei

c84485b

astefan reviewed May 13, 2020

View reviewed changes

Update test per code review feedback

97df132

astefan approved these changes May 13, 2020

View reviewed changes

Merge branch 'master' into fix/cidrmatch

70d7d77

aleksmaus merged commit 4b47e84 into elastic:master May 14, 2020

aleksmaus added a commit to aleksmaus/elasticsearch that referenced this pull request May 14, 2020

EQL: Fix cidrMatch function fails to match when used in scripts (elas…

f42a57a

…tic#56246) Addresses elastic#55709

aleksmaus added a commit that referenced this pull request May 14, 2020

EQL: Fix cidrMatch function fails to match when used in scripts (#56246…

87a1080

…) (#56735) EQL: Fix cidrMatch function fails to match when used in scripts (#56246) Addresses #55709

	List<Object> values = new ArrayList<>(new LinkedHashSet<>(foldListOfValues(addresses, field.dataType())));
	List<Object> values = Expressions.fold(addresses)

Conversation

aleksmaus commented May 5, 2020

Uh oh!

elasticmachine commented May 5, 2020

Uh oh!

Uh oh!

rw-access May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rw-access May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aleksmaus commented May 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rw-access May 5, 2020 •

edited

Loading

rw-access May 5, 2020 •

edited

Loading