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

EMT-339: policy response schema, views and tests #65962

Closed

Conversation

nnamdifrankie
Copy link
Contributor

@nnamdifrankie nnamdifrankie commented May 11, 2020

Summary

  • update schema
  • update component and tests

Checklist

@nnamdifrankie nnamdifrankie added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels May 11, 2020
@nnamdifrankie
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Some initial feedback

*/
export interface HostPolicyResponseActionDetails {
export interface HostPolicyResponseAppliedAction {
name: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we could have a permissive list of possible actions that we could use here and everywhere else where we do string[]. Some like this:

type HostPolicyActionName = 
  | 'download_model'
  | 'ingest_events_config'
  | 'workflow'
  | 'configure_elasticsearch_connection'
  | 'configure_kernel'
  | 'configure_logging'
  | 'configure_malware'
  | 'connect_kernel'
  | 'detect_file_open_events'
  | 'detect_file_write_events'
  | 'detect_image_load_events'
  | 'detect_process_events'
  | 'download_global_artifacts'
  | 'load_config'
  | 'load_malware_model'
  | 'read_elasticsearch_config'
  | 'read_events_config'
  | 'read_kernel_config'
  | 'read_logging_config'
  | 'read_malware_config'
  | string;

Then on this line (and other areas below):

Suggested change
name: string;
name: HostPolicyActionName;

Tried it quickly on my IDE and seems to work ok even with custom unknown strings:

image

export type HostPolicyResponseConfiguration = HostPolicyResponse['endpoint']['policy']['applied']['response']['configurations'];

interface HostPolicyResponseConfigurationStatus {
status: HostPolicyResponseActionStatus;
concerned_actions: Array<keyof HostPolicyResponseActions>;
concerned_actions: string[];
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 implement the suggestion above, this would then be:

Suggested change
concerned_actions: string[];
concerned_actions: HostPolicyActionName[]

@@ -80,16 +80,18 @@ export const hostMiddlewareFactory: ImmutableMiddlewareFactory<HostState> = core
version: '1.0.0',
status: HostPolicyResponseActionStatus.success,
id: '17d4b81d-9940-4b64-9de5-3e03ef1fb5cf',
actions: {
download_model: {
actions: [
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably added this only because @parkiino has not yet merged her change that integrates with API, correct (and to suppress ts errors)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes just to get the test to pass.

@@ -81,7 +83,7 @@ export const policyResponseFailedOrWarningActionCount: (
*/
export const policyResponseActions: (
state: Immutable<HostState>
) => undefined | Partial<HostPolicyResponseActions> = createSelector(
) => undefined | ImmutableArray<HostPolicyResponseAppliedAction> = createSelector(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use Immutable<> instead of ImmutableArray<>. That type will take care of using the appropriate Immutable type in its implementation. I saw Rob provide this feedback recently when I saw him providing the same feedback - the reason being: if Typescript introduces a builtin Immutable Generic, then we will be in a better place to remove our implementation.

}: {
actions: Immutable<Array<keyof HostPolicyResponseActions>>;
actionStatus: Partial<HostPolicyResponseActions>;
actions: ImmutableArray<string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Immutable<> (same below in several places). Also, would this change based on my prior suggestion to type up known action names?

@nnamdifrankie
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@nnamdifrankie
Copy link
Contributor Author

Closed for #66264

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants