[Security Solution] Keep Endpoint policies up to date with license changes #83992

pzl · 2020-11-20T21:44:52Z

Summary

Upon license downgrade, any features enabled in endpoint policy that are no longer allowed by the change in license tier are reset and/or disabled

~~Depends on #83972~~ (merged)

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

elasticmachine · 2020-11-25T13:56:09Z

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

pzl · 2020-11-25T15:17:46Z

@elasticmachine merge upstream

x-pack/plugins/security_solution/common/license/license.ts

paul-tavares · 2020-11-25T16:21:50Z

x-pack/plugins/security_solution/server/endpoint/lib/policy/license_watch.ts

+} from '../../../../common/license/policy_config';
+import { licenseService } from '../../../lib/license/license';
+
+export class PolicyWatcher {


PolicyWatcher confused me a little because it did not mention that it's really a license watcher. Maybe LicenseWatcher would be more appropriate?

(ps. I can see this being used by other things in the future that might not be Policy related)

(two hard problems joke here)

This is in server/endpoint/lib/policy, and it's job is to watch license changes, in order to keep policy up to date. I'm open to better names. LicenseWatcher is a possibility, but note that our other class, LicenseService is copy-pasted from about 4 other plugins that have the same class. But some of them call that LicenseWatcher. Wanted to prevent that confusion.

Also I do agree we may want other side-effects to license changes that may not be policy related. But those should create their own subscription via LicenseService observable. Not use this. Observables are designed to have anyone subscribe who also needs it, not collectively boggarting off of one sub.

x-pack/plugins/security_solution/server/endpoint/lib/policy/license_watch.ts

paul-tavares · 2020-11-25T16:25:36Z

x-pack/plugins/security_solution/server/endpoint/lib/policy/license_watch.ts

+            policyConfig,
+            licenseService
+          );
+          this.policyService.update(this.soClient, policy.id, policy);


Hmm.... Should you be throttling here? You could be generating several calls in parallel/rapid succession.

Also - you need to account for failures here for two reasons:

to ensure that the remainder of the loop succeeds. You also don't want to cause the outer look (do{} while()) to throw, I don't think (not sure what that will do to the observable calling logic).

to account for update conflicts, which may be the case if an update is done right after you retrieve the list of items or by other Kibana instance that may also be doing this same thing. I think we said maybe you do 1 attempt after a failure and then just log it if it still fails.

Do we have an accepted practice for how to throttle? This is certainly could be firing off a lot of update calls.

If I wrap update in a try/catch, do I now need to await update to make sure it's actually caught? If I'm now awaiting, do I still need to throttle since they are no longer in parallel?

Re:

Do we have an accepted practice for how to throttle?

I did a little of this in Fleet using Bluebird - see here

kibana/x-pack/plugins/fleet/server/routes/agent_policy/handlers.ts

Lines 55 to 65 in 95861a0

await bluebird.map(

items,

(agentPolicy: GetAgentPoliciesResponseItem) =>

listAgents(soClient, {

showInactive: false,

perPage: 0,

page: 1,

kuery: `${AGENT_SAVED_OBJECT_TYPE}.policy_id:${agentPolicy.id}`,

}).then(({ total: agentTotal }) => (agentPolicy.agents = agentTotal)),

{ concurrency: 10 }

);

Being that you are paging through, you might able to use a variation of that approach (chain each page to do concurrent updates, then await at the end).

paul-tavares · 2020-11-25T16:28:14Z

x-pack/plugins/security_solution/server/endpoint/lib/policy/license_watch.ts

+    }
+  }
+
+  public async watch(license: ILicense) {


At one point we had talked about possibly doing the "migration" slightly delayed from when the observable triggers in order to try and account for edge case where requests might be in flight that would override our changes here. Delaying the migration would allow for existing one to complete ++ any future ones would be auto-rejected by the API handles, so migrating should be able to account for all records.

Also - do you need to protect against the license changing while a migration is in flight?

I don't think I was part of any of these discussions. We want to add a sleep here?

Do we expect other things changing at the same moment the license changes? What's this about migrations?

Re:

Do we expect other things changing at the same moment the license changes? What's this about migrations?

My understanding is that changing of license can happen while the system is running - there is downtime when it happens. It just happens

re:

What's this about migrations?

Sorry for the confusion. In my mind, what this service here is doing is a migration - thats what I mean.

x-pack/plugins/security_solution/server/plugin.ts

little cleanups

paul-tavares · 2020-11-25T20:39:08Z

x-pack/plugins/security_solution/server/endpoint/lib/policy/license_watch.ts

+    }
+  }
+
+  public async watch(license: ILicense) {


Re:

Do we expect other things changing at the same moment the license changes? What's this about migrations?

My understanding is that changing of license can happen while the system is running - there is downtime when it happens. It just happens

re:

What's this about migrations?

Sorry for the confusion. In my mind, what this service here is doing is a migration - thats what I mean.

x-pack/plugins/security_solution/server/plugin.ts

paul-tavares · 2020-11-25T20:43:47Z

x-pack/plugins/security_solution/server/endpoint/lib/policy/license_watch.ts

+          } catch (e) {
+            // try again for transient issues
+            try {
+              await this.policyService.update(this.soClient, policy.id, policy);


If the problem encountered as a version conflict error, then you can't just "try" again with the same payload. You will need to retrieve the record again and redo updates, and then use that to update record for the update.

One way to test this with a running kibana:

above line 102, update policy.version to something like 123. That update should fail for the version missmatch.

Given our most recent conversation, will you leave this "retry" here or remove it and address it in the subsequent issue?

I'm ok either way

pzl · 2020-12-01T14:40:46Z

@elasticmachine merge upstream

paul-tavares

Given our recent off-PR conversation, I'm 👍

Thanks @pzl

kibanamachine · 2020-12-02T22:13:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ceb2f40

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	43205	43206	+1

History

💚 Build #91409 succeeded e6fa3e9
💔 Build #91035 failed d3bf649
💚 Build #90333 succeeded 6113014
💚 Build #90301 succeeded b9ade22
💚 Build #90265 succeeded 178752e

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

…anges (elastic#83992)

* master: (40 commits) fix: 🐛 don't add separator befor group on no main items (elastic#83166) [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323) [APM] Add APM agent config options (elastic#84678) Fixed a11y issue on rollup jobs table selection (elastic#84567) [Discover] Refactor getContextUrl to separate file (elastic#84503) [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654) [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749) [Lens] Migrate legacy es client and remove total hits as int (elastic#84340) Improve logging pipeline in @kbn/legacy-logging (elastic#84629) Catch @hapi/podium errors (elastic#84575) [Discover] Unskip date histogram test (elastic#84727) Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791) [Enterprise Search] Fix schema errors button (elastic#84842) [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589) [Maps] Always initialize routes on server-startup (elastic#84806) [Fleet] EPM support to handle uploaded file paths (elastic#84708) [Snapshot Restore] Fix initial policy form state (elastic#83928) Upgrade Node.js to version 14 (elastic#83425) [Security Solution] Keep Endpoint policies up to date with license changes (elastic#83992) [Security Solution][Exceptions] Implement exceptions for ML rules (elastic#84006) ...

* master: (236 commits) fix: 🐛 don't add separator befor group on no main items (elastic#83166) [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323) [APM] Add APM agent config options (elastic#84678) Fixed a11y issue on rollup jobs table selection (elastic#84567) [Discover] Refactor getContextUrl to separate file (elastic#84503) [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654) [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749) [Lens] Migrate legacy es client and remove total hits as int (elastic#84340) Improve logging pipeline in @kbn/legacy-logging (elastic#84629) Catch @hapi/podium errors (elastic#84575) [Discover] Unskip date histogram test (elastic#84727) Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791) [Enterprise Search] Fix schema errors button (elastic#84842) [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589) [Maps] Always initialize routes on server-startup (elastic#84806) [Fleet] EPM support to handle uploaded file paths (elastic#84708) [Snapshot Restore] Fix initial policy form state (elastic#83928) Upgrade Node.js to version 14 (elastic#83425) [Security Solution] Keep Endpoint policies up to date with license changes (elastic#83992) [Security Solution][Exceptions] Implement exceptions for ML rules (elastic#84006) ...

…nse changes (#83992) (#84844)

pzl added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Endpoint Elastic Endpoint feature v7.11.0 Team:Defend Workflows “EDR Workflows” sub-team of Security Solution labels Nov 20, 2020

pzl force-pushed the policy-change-check-ep-features branch 2 times, most recently from 4d4a524 to e56e436 Compare November 25, 2020 02:08

pzl marked this pull request as ready for review November 25, 2020 13:56

pzl requested review from a team as code owners November 25, 2020 13:56

pzl force-pushed the policy-change-check-ep-features branch 2 times, most recently from 5933185 to 178752e Compare November 25, 2020 15:52

paul-tavares reviewed Nov 25, 2020

View reviewed changes

pzl force-pushed the policy-change-check-ep-features branch from 178752e to b9ade22 Compare November 25, 2020 18:32

pzl added 5 commits November 25, 2020 15:33

begin with a license watcher

0e2bfd2

create internal SOClient, correct loop

a8376be

start/stop subscription mgmt

30a0516

little cleanups

explain soClient in comment, perhaps cleanup watch fn?

ef99277

retry policy update && use incoming license object for checks

6113014

pzl force-pushed the policy-change-check-ep-features branch from b9ade22 to 6113014 Compare November 25, 2020 20:33

paul-tavares reviewed Nov 25, 2020

View reviewed changes

kibanamachine and others added 2 commits December 1, 2020 09:40

Merge branch 'master' into policy-change-check-ep-features

d3bf649

Merge branch 'master' into policy-change-check-ep-features

e6fa3e9

paul-tavares approved these changes Dec 2, 2020

View reviewed changes

pzl added 2 commits December 2, 2020 15:24

add unit tests for license-watching policy service

0b0698e

The Road to Hell is Paved with Unused Imports

ceb2f40

pzl merged commit e194434 into elastic:master Dec 2, 2020

pzl added a commit to pzl/kibana that referenced this pull request Dec 2, 2020

[Security Solution] Keep Endpoint policies up to date with license ch…

0a07413

…anges (elastic#83992)

pzl mentioned this pull request Dec 2, 2020

[7.x] [Security Solution] Keep Endpoint policies up to date with license changes (#83992) #84844

Merged

pzl added a commit that referenced this pull request Dec 3, 2020

[7.x] [Security Solution] Keep Endpoint policies up to date with lice…

1a9bd78

…nse changes (#83992) (#84844)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Keep Endpoint policies up to date with license changes #83992

[Security Solution] Keep Endpoint policies up to date with license changes #83992

pzl commented Nov 20, 2020 •

edited

Loading

elasticmachine commented Nov 25, 2020

pzl commented Nov 25, 2020

paul-tavares Nov 25, 2020

pzl Nov 25, 2020

paul-tavares Nov 25, 2020

pzl Nov 25, 2020

paul-tavares Nov 25, 2020

paul-tavares Nov 25, 2020

pzl Nov 25, 2020

paul-tavares Nov 25, 2020

paul-tavares Nov 25, 2020

paul-tavares Nov 25, 2020

paul-tavares Dec 2, 2020

pzl commented Dec 1, 2020

paul-tavares left a comment

kibanamachine commented Dec 2, 2020

	await bluebird.map(
	items,
	(agentPolicy: GetAgentPoliciesResponseItem) =>
	listAgents(soClient, {
	showInactive: false,
	perPage: 0,
	page: 1,
	kuery: `${AGENT_SAVED_OBJECT_TYPE}.policy_id:${agentPolicy.id}`,
	}).then(({ total: agentTotal }) => (agentPolicy.agents = agentTotal)),
	{ concurrency: 10 }
	);

[Security Solution] Keep Endpoint policies up to date with license changes #83992

[Security Solution] Keep Endpoint policies up to date with license changes #83992

Conversation

pzl commented Nov 20, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Nov 25, 2020

pzl commented Nov 25, 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

pzl commented Dec 1, 2020

paul-tavares left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 2, 2020

💚 Build Succeeded

Metrics [docs]

Distributable file count

History

pzl commented Nov 20, 2020 •

edited

Loading