Initial EQL rest API implementation #49768

aleksmaus · 2019-12-02T15:23:33Z

It's my first PR, need a bit of feedback on the code shape and direction.
Specifically had to poke through RBACEngine.java and “mark” the request with CompositeIndicesRequest interface, not sure if this is the right approach.

aleksmaus · 2019-12-02T15:25:09Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java

@@ -212,6 +212,7 @@ private static boolean shouldAuthorizeIndexActionNameOnly(String action, Transpo
            case "indices:data/write/reindex":
            case "indices:data/read/sql":
            case "indices:data/read/sql/translate":
+            case "indices:data/read/eql":


was not sure if there is a better approach instead of this ^

I am not sure we need this here. I don't see a scenario when we are going to resolve the index name based on the content of the request itself.

Removing this change, replacing with more suitable solution after discussion with Igor.

aleksmaus · 2019-12-18T03:05:19Z

server/src/main/java/org/elasticsearch/search/SearchHit.java

@@ -887,7 +887,7 @@ public boolean equals(Object obj) {
                && Objects.equals(seqNo, other.seqNo)
                && Objects.equals(primaryTerm, other.primaryTerm)
                && Objects.equals(source, other.source)
-                && Objects.equals(fields, other.fields)
+                && Objects.equals(getFields(), other.getFields())


A possible fix for equals as a separate commit here, can remove if not appropriate or should be it's own separate PR.
Sharing the same SearchHit structures in the eql resful API implementation, and the equality checks would fail without this change in place.
Following the same thing that was done a line below with highlightFields accessors.

It might be a bit cleaner to make this a separate PR to master and 7.x and then merge it in. In any case, I think this requires a modification to SearchHitTests that would trigger the issue that you are trying to fix here.

Sounds good, will update this PR once this change goes through the master.

This file change is removed now from PR.

aleksmaus · 2019-12-18T17:51:28Z

Removed the SearchHit.java change. It will go through master branch.
Updated the unit test for now to always create SearchHit with fields.
Squashed and force pushed.

aleksmaus · 2019-12-18T21:41:07Z

Pulled the latest master to feature/eql and rebased my PR against it, cause of build failures due to java version
21:07:03 Caused by: org.gradle.api.GradleException: JAVA13_HOME required to run task: 21:07:03 task ':distribution:bwc:minor:buildBwcOssLinuxTar'

master branch was on Java 13 already

elasticmachine · 2019-12-19T08:40:56Z

Pinging @elastic/es-search (:Search/EQL)

imotov

Should we also add a rest client and add some rest level tests to make sure it works end-to-end?

imotov · 2019-12-30T17:12:36Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/EqlPlugin.java

+public class EqlPlugin extends Plugin implements ActionPlugin {
+    @Override
+    public List<ActionHandler<? extends ActionRequest, ? extends ActionResponse>> getActions() {
+        return Arrays.asList(


We can probably replace it with Collections.singletonList() here and below since we don't anticipate more actions here.

Thought there was a possibility of one more "translate" action down the road.

imotov · 2019-12-30T18:07:11Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/action/EqlSearchRequest.java

+
+    @Override
+    public ActionRequestValidationException validate() {
+        return null;


Since we rely on the fact that index, timestampField, eventTypeField, etc cannot be null and fetchSize cannot be negative, we should validate that it's indeed the case. We could also make serialization more forgiving and avoid serializing the same default values on every request.

Will add validation code

imotov · 2019-12-30T18:17:48Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java

@@ -212,6 +212,7 @@ private static boolean shouldAuthorizeIndexActionNameOnly(String action, Transpo
            case "indices:data/write/reindex":
            case "indices:data/read/sql":
            case "indices:data/read/sql/translate":
+            case "indices:data/read/eql":


I am not sure we need this here. I don't see a scenario when we are going to resolve the index name based on the content of the request itself.

imotov · 2019-12-30T18:20:15Z

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

+        // Not sure yet if we will always have index in the path or not
+        // TODO: finalize the endpoints
+        controller.registerHandler(GET, "/{index}/_eql/search", this);
+        controller.registerHandler(GET, "/_eql/search", this);


In most cases, if the index name is not specified on the URL we search all indices or use index specified inside the request. In this case I don't think we will ever search all indices and I don't see any provisions from specifying the index name inside the request. So, I think we can remove this version.

Yeah, there was like going back-n-forth on this, so included both versions. Will remove the latter.

imotov · 2019-12-30T18:21:53Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java

@@ -49,15 +49,15 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
         * {@link XContent} outputs we can't use {@link RestToXContentListener}
         * like everything else. We want to stick as closely as possible to
         * Elasticsearch's defaults though, while still layering in ways to
-         * control the output more easilly.
+         * control the output more easily.


I would move this file into a separate trivial PR.

Will revert this file change from this PR.

aleksmaus · 2020-01-03T13:37:45Z

@elasticmachine run elasticsearch-ci/1

imotov

LGTM. Thanks!

aleksmaus requested a review from costin December 2, 2019 15:23

aleksmaus commented Dec 2, 2019

View reviewed changes

aleksmaus added the WIP label Dec 2, 2019

aleksmaus force-pushed the feature/eql_rest branch 2 times, most recently from a32e29a to bcf861b Compare December 3, 2019 21:25

aleksmaus removed the WIP label Dec 18, 2019

aleksmaus requested a review from imotov December 18, 2019 02:55

aleksmaus marked this pull request as ready for review December 18, 2019 02:55

aleksmaus commented Dec 18, 2019

View reviewed changes

aleksmaus added the WIP label Dec 18, 2019

aleksmaus force-pushed the feature/eql_rest branch from b883229 to 350ad30 Compare December 18, 2019 17:48

aleksmaus changed the title ~~Initial EQL rest API stub~~ Initial EQL rest API implementation Dec 18, 2019

Initial EQL rest API implementation

b80ef63

aleksmaus force-pushed the feature/eql_rest branch from 4bf928c to 4fef4a2 Compare December 18, 2019 21:38

Fix linter complains

9376e86

aleksmaus force-pushed the feature/eql_rest branch from 4fef4a2 to 9376e86 Compare December 18, 2019 21:58

aleksmaus removed the WIP label Dec 18, 2019

danielmitterdorfer added the :Analytics/EQL EQL querying label Dec 19, 2019

imotov reviewed Dec 30, 2019

View reviewed changes

Address Igor's code review feedback

fbd078f

imotov approved these changes Jan 3, 2020

View reviewed changes

aleksmaus merged commit c0f048a into elastic:feature/eql Jan 3, 2020

aleksmaus added a commit to aleksmaus/elasticsearch that referenced this pull request Jan 27, 2020

Initial EQL rest API implementation (elastic#49768)

79875ce

aleksmaus mentioned this pull request Jan 27, 2020

EQL: Backport to 7.x #51502

Merged

jrodewig mentioned this pull request Feb 14, 2020

[DOCS] Document EQL search REST API #52384

Merged

Initial EQL rest API implementation #49768

Initial EQL rest API implementation #49768

Uh oh!

Conversation

aleksmaus commented Dec 2, 2019

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!

aleksmaus Dec 18, 2019 • 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!

aleksmaus Dec 18, 2019 • 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!

aleksmaus commented Dec 18, 2019

Uh oh!

aleksmaus commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 19, 2019

Uh oh!

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

aleksmaus commented Jan 3, 2020

Uh oh!

imotov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aleksmaus Dec 18, 2019 •

edited

Loading

aleksmaus Dec 18, 2019 •

edited

Loading

aleksmaus commented Dec 18, 2019 •

edited

Loading