Security Telemetry Refactor #109875

pjhampton · 2021-08-24T15:50:12Z

Summary

Maturing the security solutions use of event and batch-based telemetry to improve user protections on the endpoint agent.

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

…ion into a queries module

donaherc · 2021-08-25T02:42:58Z

x-pack/plugins/security_solution/server/lib/telemetry/queries.ts

@@ -0,0 +1,274 @@
+import { ElasticsearchClient, Logger, SavedObjectsClient } from 'src/core/server';


The idea, IMO, is that this module is going to capture ES queries inside the telemetry library. Let me know if this is a bad idea -- generally seemed good to abstract this portion from the actual transmission.

I think this is a good idea in theory, but the execution is problematic and doesn't set us up to adequately test this.
A more effective way of writing this code would be to invert control and pass the TelemetryQuerier as a dependency to the TelemetrySender class.

const telemetryQuerier = new TelemetryQuerier(...deps) const telemetrySender = new TelemetrySender(...deps, telemetryQuerier)

By doing this the sender no longer needs to reference an SO Client, ES Client, Fleet Interface, etc and will get us closer to a Single Responsibility telemetry sender.

const telemetryQuerier = new TelemetryQuerier(...deps) const telemetrySender = new TelemetrySender(...deps) const telemetryOrchestrator = new TelemetryOrchestrator(telemetryQuerier, telemetrySender, ...deps)

I also consider the term Querier is overloaded here - Receiver would be a better noun to describe the interface interactions.
Let's pair on this bit.

Yeah ok! That'll be a good starting point. I'm good with the inversion of control, and it's better as it removes coupling between the sender and the receiver. Where do you think the Queue should reside?

x-pack/plugins/security_solution/server/lib/telemetry/filters.ts

donaherc · 2021-08-25T02:47:26Z

It seems to me that the primary modules to abstract from the sender are into:

Query
Filter
Submission (sender)

I started to split these pieces out accordingly.

x-pack/plugins/security_solution/server/lib/telemetry/filters.ts

x-pack/plugins/security_solution/server/lib/telemetry/types.ts

pjhampton · 2021-08-26T09:58:33Z

x-pack/plugins/security_solution/server/lib/telemetry/receiver.ts

+import { exceptionListItemToEndpointEntry } from './helpers';
+import { TelemetryEvent, ESLicense, ESClusterInfo, GetEndpointListResponse } from './types';
+
+export class TelemetryReceiver {


@donaherc I cleaned this class up a little and got the tests / type checks working again. Make sure to g pull

On working on this code, I'm not sure "Receiver" is the correct name. "Retriever" might be better. I think this because the way the data is being requested.

entity -> push -> receiver -> sender -> ESTC entity <- pull <- retriever -> sender -> ESTC

What do you reckon?

On meditation, I think that this might a why not both situation.
We have a receiver in the form of:

*Detection Engine* -> push -> receiver -> sender -> ESTC

and a retreiver in:

* ES / SO / Fleet * <- pull <- retriever -> sender -> ESTC

Yeah it's a little confusing. I think I'm comfortable renaming in the PR we discussed as a follow-on (Telemetry Orchestrator). In my first set of commits I was definitely perceiving the behavior in the second example as primary, and by lines/functions I think the majority of the complexity in that module is the "pull from ES" functionality. Since we've decided that the Orchestrator will own the Queue, it seems that it'd serve as the receiver for now, but it we determine that there's enough functionality there, we can break out orchestrator functionality under the paradigm that you've described above, and both Tasks and the Detection Engine (streaming) approach can use the portions of the Orchestrator they need.

pjhampton · 2021-08-26T09:59:05Z

x-pack/plugins/security_solution/server/lib/telemetry/receiver.ts

+  private exceptionListClient?: ExceptionListClient;
+  private soClient?: SavedObjectsClientContract;
+  private readonly max_records = 10_000;
+  private maxQueueSize = 100;


We've duplicated this value in this class and the sender. We should move this into a constant reference.

pjhampton · 2021-08-26T10:00:40Z

x-pack/plugins/security_solution/server/lib/telemetry/receiver.ts

+      throw Error('elasticsearch client is unavailable: cannot retrieve cluster infomation');
+    }
+
+    return this.getClusterInfo(this.esClient);


I think we can / should collapse the private func getClusterInfo into this function as it is only called from here and nowhere else.

Sounds good.

pjhampton · 2021-08-26T10:01:36Z

x-pack/plugins/security_solution/server/lib/telemetry/receiver.ts

+    }
+
+    try {
+      const ret = await this.getLicense(this.esClient, true);


Same here. I think we can get rid of getLicense and move the effect into this function. No point having these dispatches.

pjhampton · 2021-08-26T10:02:33Z

Killing it 💪 @donaherc

pjhampton · 2021-09-01T09:09:06Z

@elasticmachine merge upstream

pjhampton · 2021-09-01T12:09:27Z

@elasticmachine merge upstream

kibanamachine · 2021-09-01T14:32:17Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @pjhampton @donaherc

donaherc

This LGTM, as @pjhampton and I paired on it. Some added context for other reviewers... We're taking an incremental approach to break up some chunky modules into more intuitive smaller chunks.

To that end, this PR pulls purpose-specific behavior from the sender module and spreads it into other modules (filter,receiver), with some bonus information hiding. We'll then, in a subsequent PR, wrap these modules in a coordinating orchestrator module and then hide the vast majority of the implementation inside the library itself so that both the Detection Engine and Task portions need to know less about the underlying implementation. This will greatly improve our ability to test portions of this in the future, as well as allow other portions of the code to use the telemetry in the future.

@donaherc

* [@pjhampton/@donaherc] Move sec telem tasks into own package. * Split filter out into its own module, started abstracting ES interaction into a queries module * Implemented querier and fixed some types * Updated tests, moved receiver to plugin from sender to decouple them. * fixed integration in detection engine, misc fixes * [@pjhampton] Fix type ref problems. Update test defs. * Make url transformer a member func of the sender class. * [@pjhampton] clean up receiver commentary. * [@pjhampton] add null check consistency. * Fix bad formatting. Co-authored-by: cdonaher <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-09-01T23:48:45Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

@donaherc

* [@pjhampton/@donaherc] Move sec telem tasks into own package. * Split filter out into its own module, started abstracting ES interaction into a queries module * Implemented querier and fixed some types * Updated tests, moved receiver to plugin from sender to decouple them. * fixed integration in detection engine, misc fixes * [@pjhampton] Fix type ref problems. Update test defs. * Make url transformer a member func of the sender class. * [@pjhampton] clean up receiver commentary. * [@pjhampton] add null check consistency. * Fix bad formatting. Co-authored-by: cdonaher <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Pete Hampton <[email protected]> Co-authored-by: cdonaher <[email protected]>

* Resolve merge conflict. * Add lost functionality in merge commit.

[@pjhampton/@donaherc] Move sec telem tasks into own package.

44c1b41

pjhampton assigned pjhampton and donaherc Aug 24, 2021

donaherc added 2 commits August 24, 2021 20:03

Split filter out into its own module, started abstracting ES interact…

ac5ea05

…ion into a queries module

Implemented querier and fixed some types

a2e1ba8

donaherc reviewed Aug 25, 2021

View reviewed changes

x-pack/plugins/security_solution/server/lib/telemetry/filters.ts Show resolved Hide resolved

pjhampton commented Aug 25, 2021

View reviewed changes

x-pack/plugins/security_solution/server/lib/telemetry/filters.ts Show resolved Hide resolved

pjhampton commented Aug 25, 2021

View reviewed changes

x-pack/plugins/security_solution/server/lib/telemetry/types.ts Show resolved Hide resolved

donaherc and others added 5 commits August 25, 2021 17:48

Updated tests, moved receiver to plugin from sender to decouple them.

72f71b2

fixed integration in detection engine, misc fixes

cc276a7

[@pjhampton] Fix type ref problems. Update test defs.

8cc5618

Make url transformer a member func of the sender class.

c0c20c9

[@pjhampton] clean up receiver commentary.

a1b32bc

pjhampton commented Aug 26, 2021

View reviewed changes

[@pjhampton] add null check consistency.

46fd2db

kibanamachine and others added 2 commits September 1, 2021 05:09

Merge branch 'master' into pjhampton/telemetry-refactor

2e6ab24

Fix bad formatting.

048f533

pjhampton marked this pull request as ready for review September 1, 2021 10:03

pjhampton requested a review from a team as a code owner September 1, 2021 10:03

Merge branch 'master' into pjhampton/telemetry-refactor

39c69f5

elastic deleted a comment from elasticmachine Sep 1, 2021

pjhampton enabled auto-merge (squash) September 1, 2021 16:54

donaherc approved these changes Sep 1, 2021

View reviewed changes

pjhampton merged commit 4f0a63f into master Sep 1, 2021

kibanamachine mentioned this pull request Sep 1, 2021

[7.x] Security Telemetry Refactor (#109875) #110925

Merged

pjhampton mentioned this pull request Sep 22, 2021

[7.15] Security Telemetry Refactor (#109875) #112792

Merged

pjhampton added a commit that referenced this pull request Sep 22, 2021

[7.15] Security Telemetry Refactor (#109875) (#112792)

0828ecd

* Resolve merge conflict. * Add lost functionality in merge commit.

spalger deleted the pjhampton/telemetry-refactor branch May 8, 2022 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security Telemetry Refactor #109875

Security Telemetry Refactor #109875

pjhampton commented Aug 24, 2021 •

edited

Loading

donaherc Aug 25, 2021

pjhampton Aug 25, 2021

donaherc Aug 25, 2021

donaherc commented Aug 25, 2021

pjhampton Aug 26, 2021

pjhampton Aug 26, 2021

donaherc Sep 1, 2021

pjhampton Aug 26, 2021

pjhampton Aug 26, 2021

donaherc Aug 26, 2021

pjhampton Aug 26, 2021

donaherc Aug 26, 2021

pjhampton commented Aug 26, 2021

pjhampton commented Sep 1, 2021

pjhampton commented Sep 1, 2021

kibanamachine commented Sep 1, 2021

donaherc left a comment

kibanamachine commented Sep 1, 2021

		@@ -0,0 +1,274 @@
		import { ElasticsearchClient, Logger, SavedObjectsClient } from 'src/core/server';

Security Telemetry Refactor #109875

Security Telemetry Refactor #109875

Conversation

pjhampton commented Aug 24, 2021 • edited Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaherc commented Aug 25, 2021

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

pjhampton commented Aug 26, 2021

pjhampton commented Sep 1, 2021

pjhampton commented Sep 1, 2021

kibanamachine commented Sep 1, 2021

💚 Build Succeeded

Metrics [docs]

History

donaherc left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 1, 2021

💚 Backport successful

pjhampton commented Aug 24, 2021 •

edited

Loading