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.

Sign up for GitHub

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

Jump to bottom

Refactor experimental ML CLI and code #1218

Merged
merged 17 commits into from
Jun 3, 2021
Merged

Refactor experimental ML CLI and code #1218

merged 17 commits into from
Jun 3, 2021

Conversation

brokensound77
Copy link
Contributor

Issues

related to https://github.com/elastic/security-team/issues/1204

Summary

  • removes all experimental ML release code to a dedicated repo
  • refactors the CLI commands to be less spaghetti and more modular
    • adds support for problem child
  • refactors the layout and files of ML and github code
  • updates documentation

@brokensound77 brokensound77 added python Internal python for the repository experimental features or rules which may be unsupported in their current state labels May 21, 2021
@brokensound77 brokensound77 self-assigned this May 21, 2021
@brokensound77 brokensound77 requested a review from rw-access as a code owner May 21, 2021 02:50
@brokensound77
Copy link
Contributor Author

Will need to discuss backporting.

This will break compatibility with existing releases. More to follow

ajosh0504
ajosh0504 approved these changes May 21, 2021 •
edited
Loading
View reviewed changes
ajosh0504
ajosh0504 reviewed May 21, 2021
View reviewed changes
docs/experimental-machine-learning/problem_child.md Outdated
You can also upload files locally using the `-d` option, so long as the naming convention of the files match the
expected pattern for the filenames.

#### 2.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there more to come here?

ajosh0504
ajosh0504 reviewed May 21, 2021
View reviewed changes
docs/experimental-machine-learning/experimental-detections.md Outdated

### Uploading rules

You can then individually upload these rules using the [kibana upload-rule](../CLI.md#uploading-rules-to-kibana) command
Copy link
Contributor

Choose a reason for hiding this comment

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

The link to kibana upload-rule does not seem to be working.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can then individually upload these rules using the [kibana upload-rule](../CLI.md#uploading-rules-to-kibana) command
You can then individually upload these rules using the [kibana upload-rule](../../CLI.md#uploading-rules-to-kibana) command

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated.

@ajosh0504 ajosh0504 self-requested a review May 21, 2021 15:13
rw-access
rw-access reviewed Jun 1, 2021
View reviewed changes
rw-access
rw-access reviewed Jun 1, 2021
View reviewed changes
docs/experimental-machine-learning/DGA.md Outdated

#### 2. Update packetbeat configuration

You will need to update your packebeat.yml config file to point to the enrichment pipeline
Copy link
Contributor

@rw-access rw-access Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What if I'm using Fleet?

Suggested change
You will need to update your packebeat.yml config file to point to the enrichment pipeline
You will need to update your packetbeat.yml config file to point to the enrichment pipeline

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated.

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 fleet, then you should be able to just configure the ingest pipeline in the elasticsearch index config, IINM

rw-access
rw-access reviewed Jun 1, 2021
View reviewed changes
docs/experimental-machine-learning/problem-child.md Outdated
You can also upload files locally using the `-d` option, so long as the naming convention of the files match the
expected pattern for the filenames.

#### 2.
Copy link
Contributor

Choose a reason for hiding this comment

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

🍿

rw-access
rw-access reviewed Jun 1, 2021
View reviewed changes
rw-access
rw-access reviewed Jun 1, 2021
View reviewed changes
rw-access
rw-access reviewed Jun 1, 2021
View reviewed changes
detection_rules/ml.py Outdated

@classmethod
def from_release(cls, es_client: Elasticsearch, release_tag: str, repo: str = 'elastic/detection-rules'
) -> 'MachineLearningClient':
Copy link
Contributor

@rw-access rw-access Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

this floating )... 😭

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 stared at this line for several minutes deciding if I wanted to break at str, or here - not a fan of either

rw-access
rw-access reviewed Jun 1, 2021
View reviewed changes
detection_rules/ml.py

def remove(self) -> dict:
"""Remove machine learning files from a stack."""
results = dict(script={}, pipeline={}, model={})
Copy link
Contributor

Choose a reason for hiding this comment

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

think a named tuple or data class would make more sense?

rw-access
rw-access reviewed Jun 1, 2021
View reviewed changes
rw-access
rw-access reviewed Jun 1, 2021
View reviewed changes
rw-access
rw-access approved these changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@rw-access rw-access left a comment

Choose a reason for hiding this comment

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

LGTM, pending the TODOs.
thanks for the changes @brokensound77 and @ajosh0504

@brokensound77 brokensound77 merged commit 0ec8d67 into elastic:main Jun 3, 2021
@brokensound77 brokensound77 deleted the refactor-ml-releases branch June 3, 2021 04:37
protectionsmachine pushed a commit that referenced this pull request Jun 3, 2021
@brokensound77 @github-actions
Refactor experimental ML CLI and code (#1218)
14349b3 
* move github and ml to their own files
* refactor release and ml commands
* update ML readmes
* add unzip_to_dict function
* prompt for model ID in remove-model
* update experimental rule upload process
* update remove-scripts-pipelines to take multiple options

Co-authored-by: Ross Wolf <31489089+rw-access@users.noreply.github.com>
Co-authored-by: Apoorva <appujo@gmail.com>
(cherry picked from commit 0ec8d67)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rw-access rw-access rw-access approved these changes

@threat-punter threat-punter threat-punter approved these changes

@dishadasgupta dishadasgupta Awaiting requested review from dishadasgupta

@ajosh0504 ajosh0504 Awaiting requested review from ajosh0504

Assignees

@brokensound77 brokensound77

Labels
backport: auto experimental features or rules which may be unsupported in their current state python Internal python for the repository
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brokensound77 @ajosh0504 @rw-access @threat-punter