[Endpoint] Policy Details integration with Ingest APIs #61827
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
basic save of static data
…cy-details-api-integration
…cy-details-api-integration
…cy-details-api-integration # Conflicts: # x-pack/plugins/endpoint/public/applications/endpoint/store/policy_details/action.ts # x-pack/plugins/endpoint/public/applications/endpoint/store/policy_details/middleware.ts # x-pack/plugins/endpoint/public/applications/endpoint/store/policy_details/selectors.ts # x-pack/plugins/endpoint/public/applications/endpoint/view/policy/policy_details.tsx
…nd body header title
…cy-details-api-integration
paul-tavares
added
the
release_note:skip
|
Pinging @elastic/endpoint-management (Team:Endpoint Management) |
…cy-details-api-integration
paul-tavares
commented
Mar 31, 2020
endpoint_metatdata.json
Outdated
| @@ -0,0 +1,35 @@ | |||
| { | |||
There was a problem hiding this comment.
Ignore this. It sneaked in on my commit. Deleting it now
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/discover/_field_visualize·ts.discover app discover field visualize button should preserve app filters in visualizeStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
kevinlog
reviewed
Mar 31, 2020
| @@ -27,7 +27,11 @@ export const EventingCheckbox: React.FC<{ | |||
| (event: React.ChangeEvent<HTMLInputElement>) => { | |||
| if (policyDetailsConfig) { | |||
| const newPayload = clone(policyDetailsConfig); | |||
| newPayload[os].eventing[protectionField] = event.target.checked; | |||
| if (os === OS.linux || os === OS.mac) { | |||
| newPayload[os].events.process = event.target.checked; | |||
There was a problem hiding this comment.
there will eventually be more than just process - we can address in later PRs where we add those forms
There was a problem hiding this comment.
Yeah. This is where i had trouble earlier with the types and had to include the if branches here. This entire callback will need to be refactored when we add all the other types back to the policy model and start to work the UI for it.
kevinlog
reviewed
Mar 31, 2020
| * NOTE: in the near future, this will likely be removed and an API call to EPM will be used to retrieve | ||
| * the latest from the Endpoint package | ||
| */ | ||
| export const generatePolicy = (): PolicyConfig => { |
There was a problem hiding this comment.
this is essentially our defaults for now?
There was a problem hiding this comment.
@kevinlog Yes, but this will move to the package registry (as part of the Endpoint package) (here: https://github.com/elastic/package-registry/blob/master/dev/packages/example/endpoint-1.0.0/manifest.yml). Once there, it should not be necessary for us to have this generator here (ok - maybe for UT), and if needed, we can use EPM APIs to retrieve it from the package.
kevinlog
reviewed
Mar 31, 2020
| network: boolean; | ||
| }; | ||
| } | ||
| type WindowsPolicyConfig = Pick<PolicyConfig['windows'], 'events' | 'malware'>; |
There was a problem hiding this comment.
interesting, this allows us to define the Windows config in a more concise way. Would this help us in updating state in any way in terms of satisfying the type checker? For instance, I want to update linux eventing - OS selection could trip up the type checker, especially as we add more protections.
There was a problem hiding this comment.
The Linux (UI only) policy properties are defined further below on line 188.
Maybe the naming here should be changed to UI<os_type>Config.
kevinlog
approved these changes
Mar 31, 2020
charlie-pichette
approved these changes
Mar 31, 2020
paul-tavares
commented
Mar 31, 2020
| // FIXME: remove this code once the Default Policy is available in the endpoint package - see: https://github.com/elastic/endpoint-app-team/issues/295 | ||
| // Until we get the Default configuration into the Endpoint package so that the datasource has | ||
| // the expected data structure, we will add it here manually. | ||
| if (!policyItem.inputs.length) { |
There was a problem hiding this comment.
@kevinlog - this is where we are injecting the "default" policy into the returned data from the API if not already there (which right now will always be the case)
parkiino
reviewed
Mar 31, 2020
| .endpoint-page-content { | ||
| border-left: none; | ||
| border-right: none; | ||
| &.endpoint--isDetailsView { |
There was a problem hiding this comment.
i see this -- pattern a lot in other css stuffs, does it have a standard meaning? -- pliss teach me
There was a problem hiding this comment.
:)
Ohhh. I hope I get this right from memory.
In CSS there are a few patterns for managing styles, one is called BEM - Block-Element-Modifier. in that pattern, a Modifier classname is normally defined using the double dash -- (in a past project, the _ was used, but same concept) . Its purpose is to normally drive how other classnames below it will adjust based on its presence.
(there are a few other patterns out there - google BEM vs and you should see the others popup)
😃
parkiino
reviewed
Mar 31, 2020
| .endpoint-header { | ||
| padding: ${props => props.theme.eui.euiSizeL}; | ||
| .endpoint-header { | ||
| padding: ${props => props.theme.eui.euiSizeL}; |
There was a problem hiding this comment.
i think we also want to do margin: 0; here. from the mockies i believe the header words are evensteven in the middle
There was a problem hiding this comment.
GOOOD EYE 👏
I will make this change in a subsequent PR.
FYI: The extra white space here is noticeable because a bodyHeader is also being used with the PageView component.
parkiino
reviewed
Mar 31, 2020
| @@ -20,24 +20,65 @@ import React, { memo, ReactNode } from 'react'; | |||
| import styled from 'styled-components'; | |||
There was a problem hiding this comment.
also just general question on our code organization: do we need to have a separate components folder? or does it make sense to just have all view related stuff just in the view folder? maybe common components can just be top level in the view folder? 🤷♀
There was a problem hiding this comment.
Thats what I was thinking - top level components folder was reusable across all views.
my approach also was that if a component from one view needs to be re-used in another View, we would elevate that component here for reuse - so that we're not importing from a sibling, only from a parent or a child of the given view/component.
parkiino
reviewed
Mar 31, 2020
| @@ -14,10 +14,10 @@ const entries = <T extends object>(o: T): Array<[keyof T, T[keyof T]]> => | |||
| type DeepPartial<T> = { [K in keyof T]?: DeepPartial<T[K]> }; | |||
|
|
|||
| /** | |||
| * Returns a deep copy of PolicyDetailsConfig | |||
| * Returns a deep copy of `UIPolicyConfig` object | |||
There was a problem hiding this comment.
Yeah - just needed to get the types right
parkiino
approved these changes
Mar 31, 2020
paul-tavares
added a commit
to paul-tavares/kibana
that referenced
this pull request
Mar 31, 2020
* implement Policy Details header * use Ingest APIs to persist (save) policy data to the Datasource * implement UI behaviour for Save, Cancel * implement UI bahaviour for when Policy (datasource) can not be retrieved
paul-tavares
added a commit
that referenced
this pull request
Mar 31, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Integrate the Policy Details pages with Ingest APIs and persist the Endpoint policy settings to the
datasourceobject.Changes include:
Updated
PageViewcomponent to export components used for the header and body titles as well as new required propviewType. Component now supports both a List view layout as well as a Detail layout in sync with UXThe policy details view is only shown after successfully retrieving the Policy data. A loader is initially shown. If unable to load policy data, the API error received will be displayed (can be seen by using an invalid ID in the URL)
Show Fleet Agent summary stats for the given policy in the header
Display 1 of two confirmation prompts when clicking the "Save" button, and prior to actually making the API call:
Success or failures in the update will be presented to the user by a toaster notification on the lower-right hand corner of the browser
Checklist
Delete any items that are not applicable to this PR.