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

Resolver in timeline #69728

Merged
merged 11 commits into from
Jun 29, 2020
Merged

Resolver in timeline #69728

merged 11 commits into from
Jun 29, 2020

Conversation

oatkiller
Copy link
Contributor

@oatkiller oatkiller commented Jun 23, 2020

Summary

based on #70111

This displays resolver in the security solution timeline.

Screenshots

Panning between nodes using the panel, in the timeline
panning_using_panel_in_timeline mov

Browsing resolver's via event panel
browsing_resolvers_in_events_table mov

Loading resolver w/ artificially added latency (to show the loading dialog)
high_latency_loading mov

Artificially causing the request for entity_id to fail (the request made with _id)
when_the_entity_request_fails mov

Artificially causing the main request for resolver data to fail (the request made w/ entity_id)
when_the_main_data_request_fails mov

Some data from the generator, in the timeline (not animated)
image

To fix before merge

  • Remove TODO.md
  • fix any TODOs in code or github comments
  • screenshot failure state
  • screenshot loading state
  • proofread selector tests
  • add screenshots and gif

Follow up work

  • use it more and do exhaustive manual validation (it's barely been used.)
  • consider removing service composition in the client code
  • Fix styling.
  • Make redux dev-tools work w/ multiple resolvers
  • test multiple resolvers
  • Address the SVG symbols when multiple resolvers show
  • Address the query string implementation when multiple resolvers show
  • Remove existing Endpoint Alerts route
  • functional tests (not sure who will handle that)
  • finish data selector tests.
  • test 'terminatedProcesses' selector
  • finish refactoring reducer
  • re-evaluate 'ResolverTree' type.
  • test middleware.
  • finish refactoring middleware
  • api integration tests.
  • consider moving 'related event stats' to the server.
  • use useStateSyncingActions to model query string.

API integration test plan

  • what happens if the indices aren't found
  • happy path

Checklist

For maintainers

@oatkiller

This comment has been minimized.

* you may not use this file except in compliance with the Elastic License.
*/

/* eslint-disable no-duplicate-imports */
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ what's this for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

editor automatically fixing superfluous eslint rules

const entityIDToFetch = selectors.entityIDToFetch(state);

if (selectors.shouldAbortRequest(state) && lastRequest) {
lastRequest.abortController.abort();
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ Should check https://devdocs.io/dom/abortsignal/aborted before .abort?

Copy link
Contributor

Choose a reason for hiding this comment

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

The signal only seems to raise an abort event once regardless of how many times you call abort, so it's probably fine.

var ac = new AbortController();
var signal = ac.signal;
signal.addEventListener('abort', ()=>{ console.log('aborted') });
ac.abort();
ac.abort();//only logs `abort` once

});
} catch {
api.dispatch({
type: 'serverFailedToReturnResolverData',
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ This runs when a signal aborts right? Seems like we should have a distinct catch for AbortError and an action like appReceivedAbortSignalDuringResolverDataFetch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like a good idea. i'm going to try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like:

      } catch (error) {
        // https://developer.mozilla.org/en-US/docs/Web/API/DOMException#exception-AbortError
        if (error instanceof DOMException && error.name === 'AbortError') {
          api.dispatch({
            type: 'appAbortedResolverDataRequest',
            payload: databaseDocumentIDToFetch,
          });
        } else {
          api.dispatch({
            type: 'serverFailedToReturnResolverData',
            payload: databaseDocumentIDToFetch,
          });
        }
      }

?

@oatkiller oatkiller force-pushed the resolver-in-timeline branch 4 times, most recently from c6d911d to 08bfddb Compare June 25, 2020 13:13
@oatkiller oatkiller force-pushed the resolver-in-timeline branch 7 times, most recently from b9756e8 to 627d5f8 Compare June 26, 2020 19:40
Copy link
Contributor Author

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

Need to do some cleanup before merging

/**
* Return each node of the tree.
* used to calculate related event stas
* TODO, this is used twice, by two separate things? probably wrong

This comment was marked as resolved.

This comment was marked as resolved.

/**
* Test the data reducer and selector.
*/
describe('Resolver Data Middleware', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bkimmel i moved your tests here

@@ -27,7 +28,7 @@ describe('useCamera on an unpainted element', () => {
let simulator: SideEffectSimulator;

beforeEach(async () => {
({ store } = storeFactory());
store = storeFactory();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was returning an object with the store in it.

@@ -29,7 +29,7 @@ export const TimelineBody = styled.div.attrs(({ className = '' }) => ({
overflow: auto;
scrollbar-width: thin;
flex: 1;
visibility: ${({ visible }) => (visible ? 'visible' : 'hidden')};
display: ${({ visible }) => (visible ? 'block' : 'none')};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use 'visibility' here, it still occupies space. The parent is 'flex' and Resolver is a sibling of this component. We need Resolver to grow. Seems like display: none did the trick. Anyone know of any reason why this would be bad?

@XavierM seems to know many things

* what happens if there are more matches than can be returned from es?
* do we even need more than one match?
*/
export function handleEntities(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonathan-buttner if ya get a chance, could ya give it a quick look?

@oatkiller oatkiller force-pushed the resolver-in-timeline branch 4 times, most recently from e22f482 to 1d88a9e Compare June 26, 2020 21:23
@oatkiller oatkiller marked this pull request as ready for review June 26, 2020 21:44
@oatkiller oatkiller requested review from a team as code owners June 26, 2020 21:44
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

@oatkiller
Copy link
Contributor Author

@elasticmachine merge upstream

@oatkiller oatkiller added the Team:Endpoint Data Visibility Team managing the endpoint resolver label Jun 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility)

endgame: {
process_name: '',
event_type_full: 'process_event',
event_subtype_full: 'creation_event',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to mock the new style endpoint events in ecs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it should matter here. The code operates on the ResolverEvent interface which supports either old or new events. As long as the methods works correctly on old and new style events, this code should work. We could also test those methods in isolation.

At some point, old events and new ECS events will probably stop being equivalent to each other. At that point we'd probably want a version of mockProcessEvent that returns the new style as well. At that point, if we wanted to be exhaustive, we could make a version of the mockProcessEvent that returned new ECS events and then run these tests w/ those objects as well.

Does that all add up?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since most of these tests are for code paths that occur after we normalize unique_pid/entity_id I don't think it's super important now

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, just wanted to double check 👍

beforeEach(() => {
const generator = new EndpointDocGenerator('seed');
const tree = mockResolverTree({
events: generator.generateTree({ ancestors: 1, generations: 2, children: 2 }).allEvents,
Copy link
Contributor

Choose a reason for hiding this comment

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

@bkimmel don't know if it matters but the generated tree will have parents with up to 2 children (randomly generated). If you always want the nodes to have exactly 2 children you can pass in alwaysGenMaxChildrenPerNode: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should do that

@@ -58,3 +89,5 @@ export const dataReducer: Reducer<DataState, ResolverAction> = (state = initialS
return state;
}
};

// TODO, handle abort scenario
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to handle this scenario in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

abort is handled. TODO comment should be removed. ty

result = await context.services.http.get(`/api/endpoint/resolver/${entityIDToFetch}`, {
signal: lastRequestAbortController.signal,
query: {
children: 5,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully we can bump these number up after I do some performance testing.

bool: {
filter: [
{
match: {
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 we can switch this to an IDs search: https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-ids-query.html

@andrew-goldstein the _id that we're passing should be the unique ES document ID (i.e. _id field right) right?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll give this a shot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

worked just fine.

};
}

const queryResponse: ExpectedQueryResponse = await context.core.elasticsearch.legacy.client.callAsCurrentUser(
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 try/catch around this? If ES fails for some reason maybe the server should log and return a 500?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me look into this.

Copy link
Contributor Author

@oatkiller oatkiller Jun 29, 2020

Choose a reason for hiding this comment

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

As the code is written now:

Logs in Kibana:
image

And the response was: {"statusCode":500,"error":"Internal Server Error","message":"An internal server error occurred."}

This seems fine to me. The internal details are logged, while the client gets a generic 500.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Sounds good, thanks for looking.

* ResolverTree type is returned by the server. It organizes events into a complex structure. The
* organization of events in the tree is done to associate metadata with the events. The client does not
* use this metadata. Intsead, the client flattens the tree into an array. Therefore we can safely
* make a malformed RelsoverTree for the purposes of the tests, so long as it is flattened in a predicatable way.
Copy link
Contributor

Choose a reason for hiding this comment

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

🤷 Resolver spelled wrong in the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intsead and predicatable too. (i turned on spellcheck lol)

* This prints out all of the properties of the data state.
* This way we can see the overall behavior of the selector easily.
*/
const viewAsAString = (dataState: DataState) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@oatkiller oatkiller merged commit 8ffdd45 into elastic:master Jun 29, 2020
@oatkiller oatkiller deleted the resolver-in-timeline branch June 29, 2020 17:10
oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Jun 29, 2020
Display Resolver in Security Solution's Timeline.
oatkiller pushed a commit that referenced this pull request Jun 29, 2020
Display Resolver in Security Solution's Timeline.
Bamieh pushed a commit to Bamieh/kibana that referenced this pull request Jul 1, 2020
Display Resolver in Security Solution's Timeline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants