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

[Security Solution] Adds support for testing of prerelease detection rules #148426

Merged
merged 29 commits into from
Jan 17, 2023

Conversation

spong
Copy link
Member

@spong spong commented Jan 5, 2023

Summary

Resolves #147466
Resolves #112910

  • Updates useUpgradeSecurityPackages hook to install the prerelease version of the endpoint and security_detection_engine packages if the current branch is main or build is -SNAPSHOT (to ensure PR's are testing against the latest to-be-released packages)
  • Adds new kibana.yml configuration xpack.securitySolution.prebuiltRulesPackageVersion for specifying the version of the security_detection_engine package to install within the client-side logic of the useUpgradeSecurityPackages hook
  • Adds FTR helpers for consuming the xpack.securitySolution.prebuiltRulesPackageVersion configuration from the kbnServerArgs and for installing a specific detection rules package version c467762.
  • Regenerated docs
  • Unskips useUpgradeSecurityPackages tests from #112910

Note: I added jest tests for the useUpgradeSecurityPackages changes, however didn't find a reasonable way to test the prebuiltRulesPackageVersion configuration addition via FTR's, so ended up testing that manually by running a local package-registry and serving up two different versions of the security_detection_engine package (8.3.1/8.4.1) and specifying

xpack.securitySolution.prebuiltRulesPackageVersion: '8.3.1'

in my kibana.dev.yml to try and install the previous version. This initially failed as fleet would say the package is out-of-date

Since there was a higher version with the same kibana.version requirement: kibana.version: ^8.4.0. Modifying this for the higher version to ^8.9.0 then allowed for the installation of the 8.3.1 as specified in the prebuiltRulesPackageVersion setting:

As mentioned by @xcrzx, I ended up adding force:true to the individual install request to get around this limitation and to have a better testing experience within Cypress.

Note II: When using the prebuiltRulesPackageVersion setting, since this is used for updates initiated from the client and not on kibana start like the fleet_package.json (added in #143839), you will have to uninstall the package that was installed on start-up for this to be successful.

Note III: When wanting to run the Cypress tests against a specific package version, be sure to update the cypress FTR configuration cf3a83f.

Checklist

Delete any items that are not applicable to this PR.

@spong spong added release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area v8.7.0 labels Jan 5, 2023
@spong spong self-assigned this Jan 5, 2023
@spong spong marked this pull request as ready for review January 12, 2023 00:54
@spong spong requested review from a team as code owners January 12, 2023 00:54
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@spong spong requested a review from a team as a code owner January 12, 2023 22:37
@spong
Copy link
Member Author

spong commented Jan 12, 2023

I have also tested the package version override setting. It works as expected locally; however, I didn't notice any effect of this setting on Cypress tests. @spong Is it possible to run Cypress tests with the version specified in kibana.yaml? Maybe I just missed something. 🤔

@xcrzx you didn't miss anything! Looks like this requires additional/explicit config for the cypress CI runner. I just tested by adding the config to the serverArgs over in x-pack/test/security_solution_cypress/config.ts and that worked like a charm when running cypress locally 🙂

image

Note though, since we're now sending force:true on the install request, this will result in the package installing on every full page load, even if the same version is already installed. This will undoubtedly slow down these test suites, but so long as there aren't any timeouts, it shouldn't be a big issue since we'll only be creating PR's with this config when releasing new detection-rule packages.

That said, I can check with @MadameSheema, and maybe we can put this setting in its own config like they're doing with ccs_config.ts and cases_cli_config.ts and only use that env when performing prebuilt rules tests? For discoverability, I did end up checking in this config commented out in cf3a83f.


As for using this within the FTR suites, we'd need to do the same and include the config in one of our group configs like x-pack/test/detection_engine_api_integration/security_and_spaces/group1/config.ts ala:

image

But since this config is used client-side only, it might just be best for me to include a util function for installing a specific fleet package manually before a test, and then we can use those explicitly where needed. I went down the path of setting this as above, and including a mocked out fleet bundled with two different package versions that would actually use the kibana.yml setting (similar to the fleet_api_integration tests), but was running into issues and seemed like too much overhead. Let me know if you agree a util function will suffice here (added in c467762), otherwise maybe we can pair when it comes time to write some additional FTR's and see if we can't get it working with a mocked out fleet instance.

@spong
Copy link
Member Author

spong commented Jan 13, 2023

@elasticmachine merge upstream

@xcrzx
Copy link
Contributor

xcrzx commented Jan 13, 2023

Let me know if you agree a util function will suffice here (added in c467762), otherwise maybe we can pair when it comes time to write some additional FTR's and see if we can't get it working with a mocked out fleet instance.

This should be fine for now, as we don't have integrational tests that require actual Fleet packages. I'll be adding some in #148176, so will see what would be the best way to install a required package version. Likely, calling the installDetectionRulesPackageFromFleet method directly from a test will be enough. So thank you for adding this util 👍

Copy link
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@MadameSheema MadameSheema left a comment

Choose a reason for hiding this comment

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

security-engineering-productivity LGTM!!

@terrancedejesus
Copy link
Contributor

While TRaDE is currently working on updating our release process and starting with prerelease packages, we are using -beta.x to follow SemVer for prerelease tags. Therefore a prerelease registry version will be v8.3.4-beta.1 as an example. We actually have a prerelease package already in EPR if need be for additional testing. Does this perhaps require adjustments to the string comparison logic that is used regarding this PR?

Reference: https://epr.elastic.co/package/security_detection_engine/8.3.4-beta.1/

@banderror
Copy link
Contributor

@elasticmachine merge upstream

@banderror banderror force-pushed the support-prerelease-detection-rules branch from cec368a to 363821b Compare January 16, 2023 12:09
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Reviewed the code changes and tested locally with the 8.3.4-beta.1 version mentioned by @terrancedejesus:

  • Run the app locally with xpack.securitySolution.prebuiltRulesPackageVersion: 8.3.4-beta.1 specified in config/kibana.dev.yml
  • Run Cypress tests locally with --xpack.securitySolution.prebuiltRulesPackageVersion=8.3.4-beta.1 specified in x-pack/test/security_solution_cypress/config.ts

Everything worked as expected! 🎉 And I did really appreciate all the comments added to the code, thank you! I left a bunch of nits, but nothing really important.

@spong What are our next steps to make sure everyone understands how that works and could be used for testing prerelease versions? I'm thinking about the following:

api_docs/security_solution.devdocs.json Show resolved Hide resolved
...plugins,
kibanaBranch: this.kibanaBranch,
kibanaVersion: this.kibanaVersion,
prebuiltRulesPackageVersion: this.prebuiltRulesPackageVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It looks a bit disjoined from the rest of the config passed to the FE. I think that we could probably combine ExperimentalFeaturesService with the rest of the config and simply pass the whole parsed config to KibanaServices.init. No more manual passing of new config settings would be needed when we add some new settings again. Just a thought for future modifications, not suggesting refactoring it within this PR!

Copy link
Member Author

Choose a reason for hiding this comment

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

++, this would be a nice QoL improvement here for managing these configs. Would be curious to revisit how other plugins are doing it as well these days (between client/server). cc @elastic/security-solution-platform

@spong
Copy link
Member Author

spong commented Jan 17, 2023

@spong What are our next steps to make sure everyone understands how that works and could be used for testing prerelease versions? I'm thinking about the following:

Let's update our dev docs with the corresponding info https://docs.elastic.dev/security-solution/analyst-experience/detections/rules/prebuilt-rules
Test it with a bunch of (fake?) prerelease versions for 8.7 and help @terrancedejesus open a bunch of PRs and see if it fits the release workflow of the TRADE team

That sounds good to me @banderror! Additionally I was going to update the source issues with instructions, and also create a sample Kibana PR (with the new config) for the TRADE team to use as example (as you mentioned). And as discussed yesterday, will look into if we can just have a templated Buildkite job that we can trigger with the config instead of having to open/close a Kibana PR each time.

edit:
Internal docs PR here: https://github.com/elastic/security-team/pull/5762
Sample Kibana PR for testing cypress against the 8.3.4-beta.1 package #149081

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
securitySolution 76 75 -1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 12.7MB 12.7MB +379.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 51.6KB 52.3KB +666.0B
Unknown metric groups

API count

id before after diff
securitySolution 113 115 +2

History

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

cc @spong

@spong
Copy link
Member Author

spong commented Jan 17, 2023

While TRaDE is currently working on updating our release process and starting with prerelease packages, we are using -beta.x to follow SemVer for prerelease tags. Therefore a prerelease registry version will be v8.3.4-beta.1 as an example. We actually have a prerelease package already in EPR if need be for additional testing. Does this perhaps require adjustments to the string comparison logic that is used regarding this PR?

Reference: https://epr.elastic.co/package/security_detection_engine/8.3.4-beta.1/

Nope, we should be all good here! 🙂 The explicit main branch and -SNAPSHOT build checks are what toggles the prerelease:true param, and will ensure we're installing whatever the latest prerelease package is. So this will cover CI builds and any tests from a SNAPSHOT kibana release artifact. If for some reason the logic is off for the branch/build checks (whether that's testing elsewhere or in buildkite), specifying the config xpack.securitySolution.prebuiltRulesPackageVersion: '8.3.4-beta.1' will ensure that specific version is installed for testing, ala:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.7.0
Projects
None yet
10 participants