[Security Solution][Endpoint][Exceptions] Only write manifest to policy when there are changes #72000

madirey · 2020-07-16T01:43:11Z

Summary

This PR changes the semantics of the user-artifact-manifest SO from "last dispatched" to "last computed".

There is no need to keep track of the last dispatched. We give each manifest a version, which is essentially just a sha256 hash of the artifacts that it contains. Then, when the current policy does not match our hash, we know to update.

In the course of testing this PR, some additional improvements were made:

Extended mocks and unit tests extensively
Removed some lingering @ts-ignore instances and cleaned up types
Removed the confusing getSnapshot terminology in ManifestManager. Updates are now more explicit.
The callback on policy creation was greatly simplified. We now use the first policy creation to initialize the user manifest, and subsequent policy creations will just grab the last computed manifest value and return relatively quickly. @paul-tavares @XavierM
Cleaned up logs.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…on-callbacks

elasticmachine · 2020-07-17T02:31:52Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

elasticmachine · 2020-07-17T02:31:54Z

Pinging @elastic/endpoint-response (Team:Endpoint Response)

elasticmachine · 2020-07-17T02:31:54Z

Pinging @elastic/ingest-management (Team:Ingest Management)

…on-callbacks

alexk307 · 2020-07-17T14:03:42Z

x-pack/plugins/security_solution/server/endpoint/ingest_integration.ts

+      const adds = diffs.filter((diff) => diff.type === 'add').map((diff) => diff.id);
+      for (const artifactId of adds) {
+        const compressError = await newManifest.compressArtifact(artifactId);
+        if (compressError) {


Should this call the reportError function?

@alexk307 Each case is a little different and it's sometimes not entirely clear. Generally, I'm just using reportErrors when there are multiple errors that I can't just log directly. In this case, there's just 1 error that gets rethrown, and it will be caught in the catch clause and logged on line 63. So I don't think we need to log it again.

alexk307 · 2020-07-17T14:05:38Z

x-pack/plugins/security_solution/server/endpoint/ingest_integration.ts

+    if (manifest == null) {
+      // New computed manifest based on current state of exception list
+      const newManifest = await manifestManager.buildNewManifest(ManifestConstants.SCHEMA_VERSION);
+      const diffs = newManifest.diff(Manifest.getDefault(ManifestConstants.SCHEMA_VERSION));


Would this always be empty since this is the first manifest? Or are diffs created since they are going from nothing to something?

@alexk307 There will be a diff that just contains all 3 of the artifacts.

alexk307 · 2020-07-17T14:06:42Z

x-pack/plugins/security_solution/server/endpoint/lib/artifacts/lists.test.ts

@@ -340,4 +341,95 @@ describe('buildEventTypeSignal', () => {
    const resp = await getFullEndpointExceptionList(mockExceptionClient, 'linux', 'v1');
    expect(resp.entries.length).toEqual(0);
  });
+
+  test('it should return a stable hash regardless of order of entries', async () => {


alexk307 · 2020-07-17T14:09:50Z

x-pack/plugins/security_solution/server/endpoint/lib/artifacts/manifest.ts

+  public getSha256(): string {
+    let sha256 = createHash('sha256');
+    Object.keys(this.entries)
+      .sort()


If we are sorting before generating the hash, won't we need to make sure that the sort is persisted since the Endpoint will have to hash as well to verify the package?

@alexk307 The endpoint doesn't verify this hash. This is just for us to know whether or not a policy needs to be updated (if the manifest hashes don't match, we need to update the policy).

x-pack/plugins/security_solution/server/endpoint/ingest_integration.ts

peluja1012 · 2020-07-17T16:55:39Z

x-pack/plugins/security_solution/server/endpoint/ingest_integration.ts

@@ -31,85 +85,37 @@ export const getPackageConfigCreateCallback = (
    // follow the types/schema expected
    let updatedPackageConfig = newPackageConfig as NewPolicyData;


Would let updatedPackageConfig: NewPolicyData = newPackageConfig; work here?

@paul-tavares ^^

No, we can't use it that way because our type enforces the inputs[] to have 1 member, but inputs[] in the NewPackageConfig can be empty.

TS error:

x-pack/plugins/security_solution/server/endpoint/lib/artifacts/task.ts

x-pack/plugins/security_solution/server/endpoint/services/artifacts/artifact_client.test.ts

nchaulet · 2020-07-17T20:12:46Z

x-pack/plugins/ingest_manager/common/mocks.ts

@@ -44,3 +44,90 @@ export const createPackageConfigMock = (): PackageConfig => {
    ],
  };
 };
+
+export const createPackageConfigWithInitialManifestMock = (): PackageConfig => {


maybe me more specific in the naming, any reason to no keep this in the security_solution plugin? it will make future change easier no?

Suggested change

export const createPackageConfigWithInitialManifestMock = (): PackageConfig => {

export const createEndpointPackageConfigWithInitialManifestMock = (): PackageConfig => {

@nchaulet These are mocks related to the policy, so IMO they belong here. The manifest is part of the policy. I'm happy to rename or move though if that's the consensus. Can we address in a quick follow-up to this one?

We discussed and in our next PR, I'll see if I can make the names more descriptive. Leaving them here for now, as they may be useful for future tests. We can move them to security_solution if they don't end up being useful here.

@madirey sorry for the late response.
I agree with @nchaulet on this here - these are endpoint specific mocks that Ingest knows nothing about. We only use their data structure to store our Policy data in (the area where we store it accepts any json blob).

We should really move them to our Plugin (probably under the generator we have in /common/endpoint.

kibanamachine · 2020-07-17T20:36:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 807f80f

Build metrics

✅ unchanged

History

💚 Build #62722 succeeded 87f8ee4
💚 Build #62522 succeeded 5bf5655
💚 Build #62516 succeeded e5296ce
💔 Build #62500 failed 8e2cd14
💔 Build #62472 failed a2fc28a

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

…cy when there are changes (elastic#72000) * Refactor security_solution policy creation callback - part 1 * Fix manifest dispatch * Change how dispatches are performed * simplify manifest types * Remove unused mock * Fix tests * one place to construct artifact ids * fixing linter exceptions * Add tests for stable hashes * Additional testing and type cleanup * Remove unnecessary log * Minor fixup * jsdoc * type fixup * Additional type adjustments

…cy when there are changes (#72000) (#72345) * Refactor security_solution policy creation callback - part 1 * Fix manifest dispatch * Change how dispatches are performed * simplify manifest types * Remove unused mock * Fix tests * one place to construct artifact ids * fixing linter exceptions * Add tests for stable hashes * Additional testing and type cleanup * Remove unnecessary log * Minor fixup * jsdoc * type fixup * Additional type adjustments

…cy when there are changes (#72000) (#72344) * Refactor security_solution policy creation callback - part 1 * Fix manifest dispatch * Change how dispatches are performed * simplify manifest types * Remove unused mock * Fix tests * one place to construct artifact ids * fixing linter exceptions * Add tests for stable hashes * Additional testing and type cleanup * Remove unnecessary log * Minor fixup * jsdoc * type fixup * Additional type adjustments

* master: [Observability] Remove app logos (elastic#72259) Fix float percentiles line chart (elastic#71902) update chromedriver to 84 (elastic#72228) [esArchiver] actually re-delete the .kibana index if we lose recreate race (elastic#72354) [Maps] convert SavedGisMap to TS (elastic#72286) [DOCS] Removes occurrences of X-Pack Security and Reporting (elastic#72302) use WORKSPACE env var for stack_functional_integration tests, fix navigate path (elastic#71908) [Monitoring] Fix issue with ES node detail status (elastic#72298) [SIEM] Updates consumer in export_rule archive (elastic#72324) [kbn/dev-utils] add RunWithCommands utility (elastic#72311) [Security Solution][Endpoint][Exceptions] Only write manifest to policy when there are changes (elastic#72000) skip flaky suite (elastic#72339) [ML] Fix annotations pagination & change labels from letters to numbers (elastic#72204) [Lens] Fix switching with layers (elastic#71982) [Maps] 7.9 documenation updates (elastic#71893) docs: ✏️ add "Explore underlying data" user docs (elastic#70807) [Security Solution][Exceptions] - Remove initial add exception item button in builder (elastic#72215) Fix indentation level in code exploration doc (elastic#72274) register graph usage (elastic#72041) [Monitoring] Added a case for Alerting if security/ssl is disabled (elastic#71846)

madirey added 3 commits July 15, 2020 13:25

Refactor security_solution policy creation callback - part 1

936b498

Fix manifest dispatch

8392372

Change how dispatches are performed

17316b4

madirey added v8.0.0 Team:Endpoint Response Endpoint Response Team Feature:Endpoint Elastic Endpoint feature v7.9.0 labels Jul 16, 2020

madirey added 5 commits July 15, 2020 23:33

simplify manifest types

ac6a453

Merge branch 'master' of github.com:elastic/kibana into policy-creati…

a8cc143

…on-callbacks

Remove unused mock

c8516fb

Fix tests

045c470

Merge branch 'master' of github.com:elastic/kibana into policy-creati…

eaf1cfa

…on-callbacks

madirey added the bug Fixes for quality problems that affect the customer experience label Jul 16, 2020

madirey added 4 commits July 16, 2020 17:15

one place to construct artifact ids

4ec51cb

fixing linter exceptions

a2fc28a

Add tests for stable hashes

8e2cd14

Additional testing and type cleanup

85f873d

madirey marked this pull request as ready for review July 17, 2020 02:31

madirey requested a review from a team as a code owner July 17, 2020 02:31

madirey requested a review from a team July 17, 2020 02:31

madirey requested a review from a team as a code owner July 17, 2020 02:31

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Jul 17, 2020

madirey changed the title ~~[DRAFT] Changes to manifest dispatch / policy update~~ [Security Solution][Endpoint][Exceptions] Only write manifest to policy when there are changes Jul 17, 2020

madirey added release_note:fix release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix release_note:skip Skip the PR/issue when compiling release notes labels Jul 17, 2020

madirey added the release_note:enhancement label Jul 17, 2020

madirey added 4 commits July 16, 2020 22:41

Merge branch 'master' of github.com:elastic/kibana into policy-creati…

e5296ce

…on-callbacks

Remove unnecessary log

e5731e5

Minor fixup

08afa76

jsdoc

5bf5655

alexk307 reviewed Jul 17, 2020

View reviewed changes

alexk307 approved these changes Jul 17, 2020

View reviewed changes

peluja1012 reviewed Jul 17, 2020

View reviewed changes

x-pack/plugins/security_solution/server/endpoint/ingest_integration.ts Outdated Show resolved Hide resolved

peluja1012 reviewed Jul 17, 2020

View reviewed changes

x-pack/plugins/security_solution/server/endpoint/lib/artifacts/task.ts Outdated Show resolved Hide resolved

peluja1012 reviewed Jul 17, 2020

View reviewed changes

x-pack/plugins/security_solution/server/endpoint/services/artifacts/artifact_client.test.ts Outdated Show resolved Hide resolved

madirey added 2 commits July 17, 2020 14:27

type fixup

87f8ee4

Additional type adjustments

807f80f

peluja1012 approved these changes Jul 17, 2020

View reviewed changes

nchaulet reviewed Jul 17, 2020

View reviewed changes

nchaulet approved these changes Jul 17, 2020

View reviewed changes

madirey merged commit 5356941 into elastic:master Jul 17, 2020

madirey deleted the policy-creation-callbacks branch July 17, 2020 20:51

madirey mentioned this pull request Jul 17, 2020

[7.x] [Security Solution][Endpoint][Exceptions] Only write manifest to policy when there are changes (#72000) #72344

Merged

madirey mentioned this pull request Jul 17, 2020

[7.9] [Security Solution][Endpoint][Exceptions] Only write manifest to policy when there are changes (#72000) #72345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Endpoint][Exceptions] Only write manifest to policy when there are changes #72000

[Security Solution][Endpoint][Exceptions] Only write manifest to policy when there are changes #72000

madirey commented Jul 16, 2020 •

edited

Loading

elasticmachine commented Jul 17, 2020

elasticmachine commented Jul 17, 2020

elasticmachine commented Jul 17, 2020

alexk307 Jul 17, 2020

madirey Jul 17, 2020

alexk307 Jul 17, 2020

madirey Jul 17, 2020

alexk307 Jul 17, 2020

alexk307 Jul 17, 2020

madirey Jul 17, 2020

peluja1012 Jul 17, 2020

madirey Jul 17, 2020

paul-tavares Jul 17, 2020

nchaulet Jul 17, 2020

madirey Jul 17, 2020 •

edited

Loading

madirey Jul 17, 2020

paul-tavares Jul 20, 2020

kibanamachine commented Jul 17, 2020

		@@ -31,85 +85,37 @@ export const getPackageConfigCreateCallback = (
		// follow the types/schema expected
		let updatedPackageConfig = newPackageConfig as NewPolicyData;

	export const createPackageConfigWithInitialManifestMock = (): PackageConfig => {
	export const createEndpointPackageConfigWithInitialManifestMock = (): PackageConfig => {

[Security Solution][Endpoint][Exceptions] Only write manifest to policy when there are changes #72000

[Security Solution][Endpoint][Exceptions] Only write manifest to policy when there are changes #72000

Conversation

madirey commented Jul 16, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jul 17, 2020

elasticmachine commented Jul 17, 2020

elasticmachine commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madirey Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jul 17, 2020

💚 Build Succeeded

Build metrics

History

madirey commented Jul 16, 2020 •

edited

Loading

madirey Jul 17, 2020 •

edited

Loading