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

[Endpoint] EMT-65: make endpoint data types common, restructure #54772

Conversation

nnamdifrankie
Copy link
Contributor

@nnamdifrankie nnamdifrankie commented Jan 14, 2020

Summary

elastic/endpoint-app-team#65

  • This PR moves the endpoint data structures to a common folder so it is accessible by public and common.
  • Also changes some name for consistency as per a PR comment.
  • Updated types to reflect current schema definition.
  • Move the integration test files to test specific folder.
  • Unzip api integration test files so that future changes can be done when schema changes.

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
- [ ] Documentation was added for features that require explanation or tutorials

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

id: string;
};
sensor: {
persistence: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we will only have persistent endpoints with the new product. Perhaps capturing version number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can discuss this further with the larger team. As per @kevinlog we are going to peer down the data and change some of the references. Will involve us changing our generator too, so may not want to tackle that here. We just want to show the list.

name: string;
id: string;
};
sensor: {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should change this to endpoint. sensor is an SMP term and will not be used in the Elastic security product.

full: string;
};
};
endpoint: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think endpoint is the correct term to use here. endpoint is the code that is running on the computer. It seems the keys in this endpoint should be moved to host.

@nnamdifrankie nnamdifrankie added v7.6.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 14, 2020
request_page_index: number;
}

export interface EndpointData {

Choose a reason for hiding this comment

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

@nnamdifrankie Is there a PR that defines this struct in ECS that the Endpoint team agreed upon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@james-elastic

This our current working agreement. We will need another revision it would seem, now that we have more experience. Once we do this we can make the changes on all sides.

https://docs.google.com/spreadsheets/d/1ROweh2FrHxEROcXPKWV7bvGEjg2baSA4W5dNxI5yOw8/edit#gid=0

Choose a reason for hiding this comment

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

@nnamdifrankie Awesome! Can you condense that into a PR that's in the ECS yml format to https://github.com/elastic/endpoint-app-team/pulls - this is where we're tracking all the ECS data formats that the endpoint will be sending.

Choose a reason for hiding this comment

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

I think the format should be in source control and easily found by everyone on the teams to make comments about the structure and format.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

export class EndpointAppConstants {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to move a Class to this types.ts file? I don't see the FE needing it

Copy link
Contributor

Choose a reason for hiding this comment

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

@nnamdifrankie see my prior comment here. I don't think you should have a Class in this file (maybe you meant interface?

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 @nnamdifrankie is defining constants here, not types necessarily. So I would say that maybe we could move this somewhere else, if we're trying to have this file contain and export types for the app.

@kevinlog
Copy link
Contributor

kevinlog commented Jan 14, 2020

@james-elastic @charlie-pichette I created this ticket to get an agreement on the ECS format.
https://github.com/elastic/endpoint-app-team/issues/103

@nnamdifrankie after we create that format, we'll have a source of truth so that these structs are easier to reason about.

It seems like we should just wait until the ECS format is in and agreed upon and then adjust this one using the ECS as the source of truth

EDIT: PR is now merged for ECS format https://github.com/elastic/endpoint-app-team/pull/118


export interface EndpointData {
machine_id: string;
created_at: Date;
Copy link
Contributor

Choose a reason for hiding this comment

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

Q. I think this is a string, not a Date object ?? 🤷‍♂ (same question for other date type of properties below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you think it is a str, is it because of the format.

Copy link
Contributor

@paul-tavares paul-tavares Jan 16, 2020

Choose a reason for hiding this comment

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

@nnamdifrankie JSON does not have a way to serialize/de-serialize dates. They will come across as Strings if the value of attribute is an actual JS Date object (defaults to (new Date()).toString() output which is normally the localized date+time).
My guess is that you are getting a string from Elasticsearch's query response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paul-tavares

"event": {
        "created": "2020-01-24T21:01:56.183Z"
      },

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @nnamdifrankie . So back to the type - what will it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paul-tavares I think we can map it at time of use or parameter definition. Once the other PR is merged and the type definition is available. Remember that we use it here as output, so we can map it on output or the caller can. Also recall that this is an api route and can be called by any client curl, front end e.t.c so it is not critical here for us and makes no difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @nnamdifrankie , I'm confused. Are you leaving the created_at defined as a Date? or String?

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 decided to define it a date so that it is explicit that when data is deserialized into that EndpointMetadata object the value of that field should be a date type. Is this satisfactory?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine.
I'll follow up on the FE side as to how we want to handle this (ex. should we go through the known date fields we get from the API and convert them to Date object or just do the conversion when needed) (cc/ @oatkiller , @kevinlog )

@LeeDr
Copy link
Contributor

LeeDr commented Jan 22, 2020

Since we're past feature freeze for v7.6.0 this PR should be bumped to v7.7.0

@nnamdifrankie
Copy link
Contributor Author

@elasticmachine merge upstream

@nnamdifrankie
Copy link
Contributor Author

Since we're past feature freeze for v7.6.0 this PR should be bumped to v7.7.0

@LeeDr updated. Thanks.

@nnamdifrankie
Copy link
Contributor Author

@elasticmachine merge upstream

request_page_index: number;
}

export interface EndpointMetadata {
Copy link
Contributor

Choose a reason for hiding this comment

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

My last concern, @peluja1012 has a PR where types are being handled a bit differently: https://github.com/elastic/kibana/pull/55800/files#diff-a19f3a4b9567cb1b06760465a6a74d43R25

That PR is using type as opposed to interface.

Thoughts @paul-tavares @peluja1012 @nnamdifrankie @oatkiller

Copy link
Contributor

Choose a reason for hiding this comment

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

@kevinlog I could be wrong, but I think that when you define a type directly as an object, the (ESLint) formater automatically converts it to an Interface.
The PR you reference is actually assigning a type to another Type (Immutable), thus why I think its ok there. (maybe?? 🤷‍♂ 😄 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevinlog I do not think we should be mixing this two. I prefer the interface and it is advised not to mix both. I think type is good for shorthand or derived type definition.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it. still though, we discussed getting everything across the app into a common types file until we need to break things out. Would it still make sense to follow the same pattern for defining an endpoint metadata response and an alert response?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kevinlog like Paul said, the linter will automatically convert types to interfaces when possible, so you probably don't need to think about it.

Copy link
Contributor

@kevinlog kevinlog Jan 27, 2020

Choose a reason for hiding this comment

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

@paul-tavares @oatkiller thanks for the knowledge and responses, lgtm then

@kevinlog kevinlog added the Feature:Endpoint Elastic Endpoint feature label Jan 27, 2020
@kevinlog
Copy link
Contributor

@nnamdifrankie One more thing - Add [Endpoint] tag to the title and add labels: "Team:Endpoint Management" and "Feature:Endpoint"

@kevinlog
Copy link
Contributor

kevinlog commented Jan 27, 2020

Also, reference relevant github issues in the original description, Github will link up mentions.

In this case, https://github.com/elastic/endpoint-app-team/issues/65 works

Copy link
Contributor

@kevinlog kevinlog left a comment

Choose a reason for hiding this comment

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

@nnamdifrankie just rename the title to have an [Endpoint] tag, ex: "[Endpoint] EMT-65: make endpoint data types common, restructure". Also wanna get @paul-tavares approval before merging. Other than that, lgtm on my end.

@nnamdifrankie nnamdifrankie changed the title EMT-65: make endpoint data types common, restructure [Endpoint] EMT-65: make endpoint data types common, restructure Jan 27, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@nnamdifrankie nnamdifrankie merged commit 9301531 into elastic:master Jan 27, 2020
@nnamdifrankie nnamdifrankie deleted the EMT-65_naming_changes_are_restructuring branch January 27, 2020 19:24
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 28, 2020
…ve-out-legacy

* 'master' of github.com:elastic/kibana: (187 commits)
  [ML] Reseting categorization validation if category field is cleared (elastic#56029)
  [SIEM] Fields browser readable (elastic#56000)
  [docs] Remove unused callout (elastic#56032)
  Refactor saved object management registry usage (elastic#54155)
  [SIEM][Detection Engine] critical blocker, updates the pre-packaged rules, removes dead ones, adds license file (elastic#56090)
  Fix failing snapshot artifact tests when using env var (elastic#56063)
  Fix Github PR comment formatting (elastic#56078)
  [Maps] fix join metric field selection bugs (elastic#56044)
  Create a new menu for observability links (elastic#54847)
  [SIEM] [Detection Engine] Fixes histogram intervals  (elastic#55969)
  make test less flaky by retrying if list is re-rendered (elastic#55949)
  Remove matrix build support (elastic#54202)
  Add animation to service map layout (elastic#56042)
  [Canvas] Remove Angular and unnecessary reporting config from Canvas (elastic#54050)
  [Uptime] Simplify snapshot max to Infinity (elastic#55931)
  [Uptime] Reintroduce a column for url (elastic#55451)
  Cleanup action task params objects after successful execution (elastic#55227)
  [CI] Retry flaky tests (elastic#53961)
  Expose NP FieldFormats service to server side (elastic#55419)
  [Endpoint] EMT-65: make endpoint data types common, restructure (elastic#54772)
  ...

# Conflicts:
#	src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap
#	src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/containers/panel.tsx
#	src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/context.tsx
#	src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/index.ts
#	src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/split_panel.test.tsx
#	src/legacy/ui/public/vis/editors/default/default_editor.tsx
#	src/plugins/console/public/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap
#	src/plugins/console/public/application/components/split_panel/components/resizer.tsx
#	src/plugins/console/public/application/components/split_panel/containers/panel.tsx
#	src/plugins/console/public/application/components/split_panel/containers/panel_container.tsx
#	src/plugins/console/public/application/components/split_panel/context.tsx
#	src/plugins/console/public/application/components/split_panel/index.ts
#	src/plugins/console/public/application/components/split_panel/registry.ts
#	src/plugins/console/public/application/components/split_panel/split_panel.test.tsx
#	src/plugins/kibana_react/public/split_panel/__snapshots__/split_panel.test.tsx.snap
#	src/plugins/kibana_react/public/split_panel/containers/panel.tsx
#	src/plugins/kibana_react/public/split_panel/context.tsx
#	src/plugins/kibana_react/public/split_panel/index.ts
#	src/plugins/kibana_react/public/split_panel/split_panel.test.tsx
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 28, 2020
* master: (77 commits)
  [ML] Reseting categorization validation if category field is cleared (elastic#56029)
  [SIEM] Fields browser readable (elastic#56000)
  [docs] Remove unused callout (elastic#56032)
  Refactor saved object management registry usage (elastic#54155)
  [SIEM][Detection Engine] critical blocker, updates the pre-packaged rules, removes dead ones, adds license file (elastic#56090)
  Fix failing snapshot artifact tests when using env var (elastic#56063)
  Fix Github PR comment formatting (elastic#56078)
  [Maps] fix join metric field selection bugs (elastic#56044)
  Create a new menu for observability links (elastic#54847)
  [SIEM] [Detection Engine] Fixes histogram intervals  (elastic#55969)
  make test less flaky by retrying if list is re-rendered (elastic#55949)
  Remove matrix build support (elastic#54202)
  Add animation to service map layout (elastic#56042)
  [Canvas] Remove Angular and unnecessary reporting config from Canvas (elastic#54050)
  [Uptime] Simplify snapshot max to Infinity (elastic#55931)
  [Uptime] Reintroduce a column for url (elastic#55451)
  Cleanup action task params objects after successful execution (elastic#55227)
  [CI] Retry flaky tests (elastic#53961)
  Expose NP FieldFormats service to server side (elastic#55419)
  [Endpoint] EMT-65: make endpoint data types common, restructure (elastic#54772)
  ...
oatkiller pushed a commit that referenced this pull request Feb 18, 2020
* Add Endpoint plugin and Resolver embeddable (#51994)

* Add functional tests for plugins to x-pack (so we can do a functional test of the Resolver embeddable)
* Add Endpoint plugin
* Add Resolver embeddable
* Test that Resolver embeddable can be rendered

 Conflicts:
	x-pack/.i18nrc.json
	x-pack/test/api_integration/apis/index.js

* [Endpoint] Register endpoint app (#53527)

* register app, create functional test

* formatting

* update tests

* adjust test data for endpoint

* add endpoint tests for testing spaces, app enabled, disabled, etc

* linting

* add read privileges to endpoint

* rename variable since its used now

* remove deprecated context

* remove unused variable

* fix type check

* correct test suite message

Co-Authored-By: Larry Gregory <lgregorydev@gmail.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Larry Gregory <lgregorydev@gmail.com>

* [Endpoint] add react router to endpoint app (#53808)

* add react router to endpoint app

* linting

* linting

* linting

* correct tests

* change history from hash to browser, add new test util

* remove default values in helper functions

* fix type check, use FunctionComponent as oppsed to FC

* use BrowserRouter component

* use BrowserRouter component lin

* add comments to test framework, change function name to include browserHistory

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* EMT-issue-65: add endpoint list api (#53861)

add endpoint list api

* EMT-65:always return accurate endpoint count (#54423)

EMT-65:always return accurate endpoint count, independent of paging properties

* Resolver component w/ sample data (#53619)

Resolver is a map. It shows processes that ran on a computer. The processes are drawn as nodes and lines connect processes with their parents.

Resolver is not yet implemented in Kibana. This PR adds a 'map' type UX. The user can click and drag to pan the map and zoom using trackpad pinching (or ctrl and mousewheel.)

There is no code providing actual data. Sample data is included. The sample data is used to draw a map. The fundamental info needed is:

process names
the parent of a process
With this info we can topologically lay out the processes. The sample data isn't yet in a realistic format. We'll be fixing that soon.

Related issue: elastic/endpoint-app-team#30

* Resolver test plugin not using mount context. (#54933)

Mount context was deprecated. Use core.getStartServices() instead.

* Resolver nonlinear zoom (#54936)

* [Endpoint] add Redux saga Middleware and app Store (#53906)

* Added saga library
* Initialize endpoint app redux store

* Resolver is overflow: hidden to prevent obscured elements from showing up (#55076)

* [Endpoint] Fix saga to start only after store is created and stopped on app unmount (#55245)

- added `stop()`/`start()` methods to the Saga Middleware creator factory
- adjust tests based on changes
- changed application `renderApp` to stop sagas when react app is unmounted

* Resolver zoom, pan, and center controls (#55221)

* Resolver zoom, pan, and center controls

* add tests, fix north panning

* fix type issue

* update west and east panning to behave like google maps

* [Endpoint] FIX: Increase tests `sleep` default duration back to 100ms (#55492)

Revert `sleep()` default duration, in the saga tests, back to 100ms in order to prevent intermittent failures during CI runs.

Fixes #55464
Fixes #55465

* [Endpoint] EMT-65: make endpoint data types common, restructure (#54772)

[Endpoint] EMT-65: make endpoint data types common, use schema changes

* Basic Functionality Alert List (#55800)

* sets up initial grid and data type

* data feeds in from backend but doesnt update

* sample data feeding in correctly

* Fix combineReducers issue by importing Redux type from 'redux' package

* Add usePageId hook that fires action when user navigates to page

* Strict typing for middleware

* addresses comments and uses better types

* move types to common/types.ts

* Move types to endpoint/types.ts, address PR comments

blah 2

Co-authored-by: Pedro Jaramillo <peluja1012@gmail.com>

* [Endpoint] Add Endpoint Details route (#55746)

* Add Endpoint Details route

* add Endpoint Details tests

* sacrifices to the Type gods

* update to latest endpoint schema

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* [Endpoint] EMT-67: add kql support for endpoint list (#56328)

[Endpoint] EMT-67: add kql support for endpoint list

* [Endpoint] ERT-82 ERT-83 ERT-84: Alert list API with pagination (#56538)

* ERT-82 ERT-83 ERT-84 (partial): Add Alert List API with pagination

* Better type safety for alert list API

* Add Test to Verify Endpoint App Landing Page (#57129)

 Conflicts:
	x-pack/test/functional/page_objects/index.ts

* fixes render bug in alert list (#57152)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* Resolver: Animate camera, add sidebar (#55590)

This PR adds a sidebar navigation. clicking the icons in the nav will focus the camera on the different nodes. There is an animation effect when the camera moves.

 Conflicts:
	yarn.lock

* [Endpoint] Task/basic endpoint list (#55623)

* Adds host management list to endpoint security plugin

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* [Endpoint] Policy List UI route and initial view (#56918)

* Initial Policy List view

* Add `endpoint/policy` route and displays Policy List
* test cases (both unit and functional)

Does not yet interact with API (Ingest).

* Add ApplicationService app status management (#50223)

This was already backported, but changes to endpoint app could not be
backported, since endpoint app itself hadn't been backported. Now that
the endpoint app is backported, reapply the endpoint specific changes
from the original commit.

* Implements `getStartServices` on server-side (#55156)

This was already backported, but changes to endpoint app could not be
backported, since endpoint app itself hadn't been backported. Now that
the endpoint app is backported, reapply the endpoint specific changes
from the original commit.

* [ui/utils/query_string]: Remove unused methods & migrate apps to querystring lib (#56957)

This was already backported, but changes to endpoint app could not be
backported, since endpoint app itself hadn't been backported. Now that
the endpoint app is backported, reapply the endpoint specific changes
from the original commit.

Co-authored-by: Kevin Logan <56395104+kevinlog@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Larry Gregory <lgregorydev@gmail.com>
Co-authored-by: nnamdifrankie <56440728+nnamdifrankie@users.noreply.github.com>
Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>
Co-authored-by: Paul Tavares <56442535+paul-tavares@users.noreply.github.com>
Co-authored-by: Pedro Jaramillo <peluja1012@gmail.com>
Co-authored-by: Dan Panzarella <pzl@users.noreply.github.com>
Co-authored-by: Madison Caldwell <madison.rey.caldwell@gmail.com>
Co-authored-by: Charlie Pichette <56399229+charlie-pichette@users.noreply.github.com>
Co-authored-by: Candace Park <56409205+parkiino@users.noreply.github.com>
Co-authored-by: Pierre Gayvallet <pierre.gayvallet@gmail.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants