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.

Sign up for GitHub

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

Jump to bottom

EQL: Change default indices options #63192

Merged
merged 4 commits into from
Oct 5, 2020
Merged

EQL: Change default indices options #63192

merged 4 commits into from
Oct 5, 2020

Conversation

costin
Copy link
Member

@costin costin commented Oct 2, 2020

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

@costin
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
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Oct 2, 2020
costin
costin commented Oct 2, 2020
View reviewed changes
...in/eql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/eql/AsyncEqlSecurityIT.java Outdated
Comment on lines 92 to 93
assertThat(exc.getResponse().getStatusLine().getStatusCode(), equalTo(404));
//assertThat(exc.getMessage(), containsString("unauthorized"));
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
matriv reviewed Oct 2, 2020
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

astefan
astefan reviewed Oct 2, 2020
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

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

imotov
imotov reviewed Oct 2, 2020
View reviewed changes
...in/eql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/eql/AsyncEqlSecurityIT.java Outdated
Comment on lines 92 to 93
assertThat(exc.getResponse().getStatusLine().getStatusCode(), equalTo(404));
//assertThat(exc.getMessage(), containsString("unauthorized"));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

x-pack/plugin/eql/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/eql/10_basic.yml Outdated
@@ -83,6 +83,20 @@ setup:
- match: {hits.sequences.1.events.0._id: "2"}
- match: {hits.sequences.1.events.1._id: "3"}

---
"Execute EQL sequence by default ignores unavailable indices"
Copy link
Contributor

Choose a reason for hiding this comment

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

missing : at the end

costin
costin commented Oct 2, 2020
View reviewed changes
client/rest-high-level/src/main/java/org/elasticsearch/client/eql/EqlSearchRequest.java
@@ -34,7 +34,7 @@
public class EqlSearchRequest implements Validatable, ToXContentObject {

private String[] indices;
private IndicesOptions indicesOptions = IndicesOptions.fromOptions(false, false, true, false);
private IndicesOptions indicesOptions = IndicesOptions.fromOptions(true, false, true, false);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

costin
costin commented Oct 2, 2020
View reviewed changes
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
Copy link
Member Author

Choose a reason for hiding this comment

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

@matriv the integration test against non-existing indices

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@jrodewig jrodewig mentioned this pull request Oct 2, 2020
Merged
bpintea
bpintea approved these changes Oct 2, 2020
View reviewed changes
Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

lgtm

matriv
matriv approved these changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

LGTM, left 2 comments.

...in/eql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/eql/AsyncEqlSecurityIT.java Outdated
assertThat(exc.getResponse().getStatusLine().getStatusCode(), equalTo(403));
assertThat(exc.getMessage(), containsString("unauthorized"));
assertThat(exc.getResponse().getStatusLine().getStatusCode(), equalTo(404));
//assertThat(exc.getMessage(), containsString("unauthorized"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove it then?

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

astefan
astefan approved these changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

@costin costin merged commit fd75ac2 into elastic:master Oct 5, 2020
@costin costin deleted the fix/62986 branch October 5, 2020 11:19
costin added a commit that referenced this pull request Oct 5, 2020
@costin
EQL: Change default indices options (#63192)
8c4503b 
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)
@rylnd rylnd mentioned this pull request Oct 12, 2020
Closed
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imotov imotov imotov left review comments

@matriv matriv matriv approved these changes

@bpintea bpintea bpintea approved these changes

@astefan astefan astefan approved these changes

@palesz palesz Awaiting requested review from palesz

Assignees
No one assigned
Labels
:Analytics/EQL EQL querying >refactoring Team:QL (Deprecated) Meta label for query languages team v7.10.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EQL: allow_no_indices behavior
7 participants
@costin @elasticmachine @imotov @astefan @bpintea @matriv @jakelandis