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.

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

[SIEM] [Detection Engine] Search signals index #52661

Merged
merged 5 commits into from
Dec 11, 2019

Conversation

dhurley14
Copy link
Contributor

@dhurley14 dhurley14 commented Dec 10, 2019

Summary

Adds route for searching signals index and sample usage script. Also updates naming for signals status schemas.

There is a sample usage query in scripts/signals/query_signals.sh in addition to an aggs script in scripts/signals/aggs_signals.sh. Ensure you have a signal doc in the signals index. There is a sample doc that can be indexed via the signals/put_signal_doc.sh script.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

expect(statusCode).toBe(400);
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome tests! I should back fill my to have at least the basics like this. Really appreciate it!

query: { range: { '@timestamp': { gte: 'now-2M', lte: 'now/M' } } },
status: 'closed',
});

export const setStatusSignalMissingIdsAndQueryPayload = (): Partial<SignalsRestParams> => ({
export const typicalSignalsQuery = (): Partial<SignalsQueryRestParams> => ({
search_query: { query: { match_all: {} } },
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just drop the search_query and use { query since you already have the URL for search. Would feel more natural to the ES querying of things.


export const querySignalsSchema = Joi.object({
search_query: Joi.object().required(),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

If you dropped the search_query, you could change this to just be that query is required? Unless you were planning more than just query to be accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you wanted aggregations too you could then do a "at least one of these is required" with the top level keys being:

query
aggregations

Just kind of asking since I have to do these type of endpoints myself for rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah I forgot about aggregations. Good call.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, ++ on aggregations -- we'll want to use this for the Signals histogram.

./check_env_variables.sh

# Example: ./signals/query_signals.sh
curl -v -k \
Copy link
Contributor

Choose a reason for hiding this comment

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

Woa, I don't think you want the -v here for the example. That adds a lot of extra stuff! :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very true - was debugging and forgot to take it out. Thanks 👍

set -e
./check_env_variables.sh

# Example: ./signals/query_signals.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

You mean signals/aggs_signals.sh on this line

payload: querySignalsSchema,
},
},
async handler(request: SignalsQueryRequest, _headers) {
Copy link
Contributor

@FrankHassanabad FrankHassanabad Dec 10, 2019

Choose a reason for hiding this comment

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

Optional, if you're not using _headers you can just do: async handler(request: SignalsQueryRequest)

@@ -24,7 +24,7 @@ export const setSignalsStatusRouteDef = (server: ServerFacade): Hapi.ServerRoute
payload: setSignalsStatusSchema,
},
},
async handler(request: SignalsRequest, headers) {
async handler(request: SignalsStatusRequest, _headers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional, if you're not using headers you can just do: async handler(request: SignalsStatusRequest)

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

Found one comment in a sh file that was a copy-pasta and 2 optionals.

Outside of that, I checked this out, tested it, and everything looks great!

LGTM!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💔 Build #14696 failed 907115da91a1e74957fdc974e28d2027d3a2a1a9
  • 💚 Build #14594 succeeded 6153f6e6416b0eddf8c0731c6dc8f06f1b1bb39f

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@dhurley14 dhurley14 merged commit a12d855 into elastic:master Dec 11, 2019
@dhurley14 dhurley14 deleted the search-signals-api branch December 11, 2019 16:09
dhurley14 added a commit to dhurley14/kibana that referenced this pull request Dec 11, 2019
* adds route for querying signals index, also updates signal status type names

* first pass at happy path tests

* fixes stuff after rebase with master

* utilizes removes search_query from payload and replaces it with just query, adds aggs to signals search api, updates route and validation tests

* removes _headers parameter from route handler and updates comment for aggs script
dhurley14 added a commit that referenced this pull request Dec 11, 2019
* adds route for querying signals index, also updates signal status type names

* first pass at happy path tests

* fixes stuff after rebase with master

* utilizes removes search_query from payload and replaces it with just query, adds aggs to signals search api, updates route and validation tests

* removes _headers parameter from route handler and updates comment for aggs script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants