[Security Solution] add package_policy_id to .fleet-policies inputs #137355

joeypoon · 2022-07-27T20:46:56Z

Summary

Add migration for enriching .fleet-policies.inputs with package_policy_id as discussed in https://github.com/elastic/security-team/issues/3918#issuecomment-1176815273.

Checklist

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

joeypoon · 2022-07-27T20:49:01Z

x-pack/plugins/fleet/server/services/agent_policy.ts

+    const policies = await agentPolicyService.getByIDsAsMap(soClient, agentPolicyIds);
+    const fullPolicies = await Promise.all(
+      agentPolicyIds.map((agentPolicyId) =>
+        // TODO: make this a bulk


making this a bulk seemed a bit more involved so wanted to get some thoughts on logic so far and whether this would even be needed.

Taking a bit deeper look at getFullAgentPolicy, using it this way will fetch a few objects that will be re-used across agent policies (outputs, settings, upgrade download source uri), so we could save some round trips to ES in doing these fetches once and re-using them here.

This will likely only be a problem for users that have many thousands of agent policies, which we do have some users who use that many.

That said, this migration is a one time event for users upgrading from <8.5 so I don't want to spend a whole of time refactoring this if we can avoid it. @kpollich what do you think about deferring on optimizing this?

I'm +1 on deferring those optimizations here with the caveat that we capture essentially what @joshdover has said above in a code comment replacing the current TODO

joeypoon · 2022-07-27T20:49:14Z

x-pack/plugins/fleet/server/services/agent_policy.ts

      refresh: 'wait_for',
    });
+
+    // TODO: make this a bulk


same as https://github.com/elastic/kibana/pull/137355/files#r931568428

joeypoon · 2022-07-27T20:50:13Z

x-pack/plugins/fleet/server/services/agents/setup.ts

-      }
-    })
-  );
+  await agentPolicyService.deployPolicies(soClient, outdatedAgentPolicyIds);


this change wasn't really necessary but since I took the time to make deployPolicies, I figured it was a quick change 🤷‍♂️ .

x-pack/plugins/fleet/server/services/agent_policy.ts

x-pack/plugins/fleet/server/services/setup/upgrade_agent_policy_schema_version.ts

elasticmachine · 2022-07-29T14:33:16Z

Pinging @elastic/fleet (Team:Fleet)

joshdover · 2022-08-01T11:36:42Z

x-pack/plugins/fleet/server/services/agent_policy.ts

+    }, [] as FleetServerPolicy[]);
+
+    const fleetServerPoliciesBulkBody = fleetServerPolicies.flatMap((fleetServerPolicy) => [
+      { index: { _id: uuid() } },


Since this migration could potentially be executed more than once by different Kibana nodes, I think we should use a deterministic ID here to avoid creating duplicate documents in .fleet-policies.

In other places in Kibana, we've used uuid.v5() with an appropriate seed for this. IMO the policy_id and revision_idx fields should be sufficient, something like this:

import uuidv5 from 'uuid/v5'; // ... { index: { _id: uuidv5(`${fleetServerPolicy.policy_id}:${fleetServerPolicy.revision_idx}`, uuidv5.DNS) } }

We'll then need to handle the error on the esClient.bulk correctly (and maybe log a debug message?)

makes sense, I'll get this updated.

kpollich

Awesome work so far, @joeypoon - I've just left a few comments here and there. I'll review further in depth when @joshdover's requested changes are made.

kpollich · 2022-08-01T18:03:06Z

x-pack/plugins/fleet/server/services/agent_policy.ts

+    soClient: SavedObjectsClientContract,
+    ids: string[],
+    options: { fields?: string[] } = {}
+  ): Promise<{ [id: string]: AgentPolicy }> {


I think this function could likely be shortened or possibly replaced by keyBy from Lodash, especially since we only consume this public method in one place. The service methods don't need to be overly concerned with the structure of the results if callers can use transformation utilities directly.

x-pack/plugins/fleet/server/services/setup/upgrade_agent_policy_schema_version.ts

kpollich · 2022-08-02T11:17:38Z

x-pack/plugins/fleet/server/services/agent_policy.ts

+    const policies = await agentPolicyService.getByIDsAsMap(soClient, agentPolicyIds);
+    const fullPolicies = await Promise.all(
+      agentPolicyIds.map((agentPolicyId) =>
+        // TODO: make this a bulk


Suggested change

// TODO: make this a bulk

// There are some potential performance concerns around using `getFullAgentPolicy` in this context, e.g.

// re-fetching outputs, settings, and upgrade download source URI data for each policy. This could potentially

// be a bottleneck in environments with several thousand agent policies being deployed here.

juliaElastic · 2022-08-08T07:27:03Z

@elasticmachine merge upstream

kpollich

Just a few tiny nitpicks on some minor things, but otherwise LGTM! Thanks for your contributions here @joeypoon! 🚀

kpollich · 2022-08-15T18:07:31Z

x-pack/plugins/fleet/server/services/agent_policy.ts


 import { DEFAULT_SPACE_ID } from '@kbn/spaces-plugin/common/constants';

+import { lt } from 'semver';


(nit) Can we group this dependency with the other "external package" imports above - e.g. below p-map?

kpollich · 2022-08-15T18:08:22Z

x-pack/plugins/fleet/server/services/agent_policy.ts

+      if (!policy) {
+        return acc;
+      }


Thanks for using a guard clause here - much appreciated 👍

kpollich · 2022-08-15T18:11:08Z

x-pack/plugins/fleet/server/services/agent_policy.ts

+        return [...acc, value];
+      }, [] as BulkResponseItem[]);
+
+      logger.debug(`deployPolicies failed to index: ${JSON.stringify(erroredDocuments)}`);


Hmm, super nitpicky here, but could we avoid having the method name in this debug log and lean towards "plain english" logging output? e.g.

logger.debug(`Failed to index documents during policy deployment: ${JSON.stringify(erroredDocuments)}`)

…-ref HEAD~1..HEAD --fix'

kibana-ci · 2022-08-15T21:16:26Z

💚 Build Succeeded

Buildkite Build
Commit: 99c01d4

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
`fleet`	860	862	+2

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id	before	after	diff
`ingest-agent-policies`	18	19	+1

Unknown metric groups

API count

id	before	after	diff
`fleet`	955	957	+2

History

💚 Build #63487 succeeded b5323fa1d5370209658082190b36a41ec95bafea
💛 Build #63356 was flaky 9fd330f5ed07da69820dd9f36729a3553ca62592
💚 Build #61637 succeeded 1050a3501e3177b9f32725aa62f1bacabe301005
💔 Build #61327 failed 3892256fb03452989ae32e855b3f2affaab5bbb2

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

…lastic#137355)

joeypoon added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.5.0 labels Jul 27, 2022

joeypoon requested review from joshdover and kpollich July 27, 2022 20:46

joeypoon commented Jul 27, 2022

View reviewed changes

x-pack/plugins/fleet/server/services/agent_policy.ts Show resolved Hide resolved

joeypoon commented Jul 28, 2022

View reviewed changes

x-pack/plugins/fleet/server/services/setup/upgrade_agent_policy_schema_version.ts Show resolved Hide resolved

joeypoon force-pushed the feature/package-policy-id branch from 3892256 to 2a77c2f Compare July 28, 2022 22:34

joeypoon marked this pull request as ready for review July 29, 2022 14:33

joeypoon requested a review from a team as a code owner July 29, 2022 14:33

joshdover reviewed Aug 1, 2022

View reviewed changes

kpollich reviewed Aug 1, 2022

View reviewed changes

kpollich reviewed Aug 2, 2022

View reviewed changes

joeypoon force-pushed the feature/package-policy-id branch from 1050a35 to cc71d01 Compare August 4, 2022 21:57

kpollich self-requested a review August 9, 2022 14:51

kpollich approved these changes Aug 15, 2022

View reviewed changes

[Security Solution] add package_policy_id to .fleet-policies inputs

a3a9992

joeypoon force-pushed the feature/package-policy-id branch from b5323fa to a3a9992 Compare August 15, 2022 20:03

joeypoon enabled auto-merge (squash) August 15, 2022 20:03

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

99c01d4

…-ref HEAD~1..HEAD --fix'

joeypoon merged commit 2c96643 into elastic:main Aug 15, 2022

kibanamachine added the backport:skip This commit does not require backporting label Aug 15, 2022

joeypoon deleted the feature/package-policy-id branch August 15, 2022 21:56

Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022

[Security Solution] add package_policy_id to .fleet-policies inputs (e…

c88e8e5

…lastic#137355)

kpollich mentioned this pull request Sep 20, 2022

[APM/Fleet] Upgrade to 8.5.0-SNAPSHOT is failing on cloud elastic/elastic-agent#1238

Closed

hop-dev mentioned this pull request Feb 8, 2023

[Fleet] Make agent policy schema upgrade resilient to high numbers of agent policies #150538

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] add package_policy_id to .fleet-policies inputs #137355

[Security Solution] add package_policy_id to .fleet-policies inputs #137355

joeypoon commented Jul 27, 2022

joeypoon Jul 27, 2022

joshdover Aug 2, 2022 •

edited

Loading

kpollich Aug 2, 2022

joeypoon Jul 27, 2022

joeypoon Jul 27, 2022

elasticmachine commented Jul 29, 2022

joshdover Aug 1, 2022

joeypoon Aug 1, 2022

kpollich left a comment

kpollich Aug 1, 2022

kpollich Aug 2, 2022

juliaElastic commented Aug 8, 2022

kpollich left a comment

kpollich Aug 15, 2022

kpollich Aug 15, 2022

kpollich Aug 15, 2022

kibana-ci commented Aug 15, 2022

API count

-        // TODO: make this a bulk
+        // There are some potential performance concerns around using `getFullAgentPolicy` in this context, e.g.
+        // re-fetching outputs, settings, and upgrade download source URI data for each policy. This could potentially
+        // be a bottleneck in environments with several thousand agent policies being deployed here.


		import { DEFAULT_SPACE_ID } from '@kbn/spaces-plugin/common/constants';

		import { lt } from 'semver';

[Security Solution] add package_policy_id to .fleet-policies inputs #137355

[Security Solution] add package_policy_id to .fleet-policies inputs #137355

Conversation

joeypoon commented Jul 27, 2022

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

joshdover Aug 2, 2022 • edited Loading

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

elasticmachine commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpollich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliaElastic commented Aug 8, 2022

kpollich left a 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

kibana-ci commented Aug 15, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Saved Objects .kibana field count

API count

History

joshdover Aug 2, 2022 •

edited

Loading