EQL: Change default indices options by costin · Pull Request #63192 · elastic/elasticsearch

costin · 2020-10-02T14:27:56Z

Ignore by default unavailable indices (same as ES) and verify that
allowNoIndices is set to false since at least one index is required
for validating the query.

Fix #62986

Ignore by default unavailable indices (same as ES) and verify that allowNoIndices is set to false since at least one index is required for validating the query. Fix elastic#62986

elasticmachine · 2020-10-02T14:27:58Z

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

costin · 2020-10-02T14:29:08Z

...in/eql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/eql/AsyncEqlSecurityIT.java

+        assertThat(exc.getResponse().getStatusLine().getStatusCode(), equalTo(404));
+        //assertThat(exc.getMessage(), containsString("unauthorized"));


@imotov not sure why this fails now... Essentially by changing the index option ignoreUnavailable from false to true the 403/unauthorized became a 404...

I think it is because we ignore unavailable indices and ended up with no indices at all, which leads to 404.

And previously we did not ignore them but because we couldn't get access to them to verify their existence we got the 403...Make sense.

matriv

So this means that if you only include non existing indices we don't fail?
Can we have a test for that?

astefan

I think you need to make the same change in rest high level client corresponding EqlSearchRequest class.

imotov · 2020-10-02T15:27:51Z

...in/eql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/eql/AsyncEqlSecurityIT.java

+        assertThat(exc.getResponse().getStatusLine().getStatusCode(), equalTo(404));
+        //assertThat(exc.getMessage(), containsString("unauthorized"));


I think it is because we ignore unavailable indices and ended up with no indices at all, which leads to 404.

imotov · 2020-10-02T15:30:16Z

x-pack/plugin/eql/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/eql/10_basic.yml

  - match: {hits.sequences.1.events.1._id: "3"}

+---
+"Execute EQL sequence by default ignores unavailable indices"


missing : at the end

costin · 2020-10-02T18:27:09Z

client/rest-high-level/src/main/java/org/elasticsearch/client/eql/EqlSearchRequest.java


    private String[] indices;
-    private IndicesOptions indicesOptions = IndicesOptions.fromOptions(false, false, true, false);
+    private IndicesOptions indicesOptions = IndicesOptions.fromOptions(true, false, true, false);


@astefan the change inside EQL high-level rest client.

costin · 2020-10-02T18:32:07Z

x-pack/plugin/eql/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/eql/10_basic.yml

+"Execute EQL sequence by default ignores unavailable indices"
+  - do:
+      eql.search:
+        index: eql_test,non_existing


@matriv the integration test against non-existing indices

I meant to query against only unavailable indices, just for completeness.

bpintea

lgtm

matriv

LGTM, left 2 comments.

matriv · 2020-10-05T06:11:01Z

...in/eql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/eql/AsyncEqlSecurityIT.java

-        assertThat(exc.getResponse().getStatusLine().getStatusCode(), equalTo(403));
-        assertThat(exc.getMessage(), containsString("unauthorized"));
+        assertThat(exc.getResponse().getStatusLine().getStatusCode(), equalTo(404));
+        //assertThat(exc.getMessage(), containsString("unauthorized"));


Should we remove it then?

matriv · 2020-10-05T06:12:11Z

x-pack/plugin/eql/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/eql/10_basic.yml

+"Execute EQL sequence by default ignores unavailable indices"
+  - do:
+      eql.search:
+        index: eql_test,non_existing


I meant to query against only unavailable indices, just for completeness.

astefan

LGTM

Ignore by default unavailable indices (same as ES) and verify that allowNoIndices is set to false since at least one index is required for validating the query. Fix #62986 (cherry picked from commit fd75ac2)

EQL: Change default indices options

2e4de89

Ignore by default unavailable indices (same as ES) and verify that allowNoIndices is set to false since at least one index is required for validating the query. Fix elastic#62986

costin added >refactoring v8.0.0 :Analytics/EQL EQL querying v7.10.0 labels Oct 2, 2020

costin requested review from astefan, bpintea, imotov, matriv and palesz October 2, 2020 14:27

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Oct 2, 2020

costin commented Oct 2, 2020

View reviewed changes

matriv reviewed Oct 2, 2020

View reviewed changes

astefan reviewed Oct 2, 2020

View reviewed changes

imotov reviewed Oct 2, 2020

View reviewed changes

Update yml file

280a4bf

costin commented Oct 2, 2020

View reviewed changes

jrodewig mentioned this pull request Oct 2, 2020

[DOCS] Update ignore_unavailable default for EQL search API #63210

Merged

bpintea approved these changes Oct 2, 2020

View reviewed changes

Add integration test for non-existing index pattern

3e843d6

matriv approved these changes Oct 5, 2020

View reviewed changes

astefan approved these changes Oct 5, 2020

View reviewed changes

Remove commented code

b1b352a

costin merged commit fd75ac2 into elastic:master Oct 5, 2020

costin deleted the fix/62986 branch October 5, 2020 11:19

rylnd mentioned this pull request Oct 12, 2020

[Security Solution] [Detections] EQL rule cannot be created elastic/kibana#80126

Closed

jakelandis removed the v8.0.0 label Jul 26, 2021

jakelandis added the v8.0.0-alpha1 label Jul 26, 2021

		assertThat(exc.getResponse().getStatusLine().getStatusCode(), equalTo(404));
		//assertThat(exc.getMessage(), containsString("unauthorized"));

Conversation

costin commented Oct 2, 2020

Uh oh!

elasticmachine commented Oct 2, 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!

matriv left a comment • 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!

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!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

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

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

matriv left a comment •

edited

Loading