OSquery fix issue with document rejection by upgrading osquery_manager package and rolling over indices on upgrade #148991

kevinlog · 2023-01-16T18:37:10Z

Summary

This PR helps address this bug: elastic/beats#34250

The issue is that with the Agent v2, osquery beat sends different values in the data_stream field, which are type constant_keyword, to the logs-osquery_manager.result-* indices. This causes document rejection if users upgraded to Agent v8.6.0 and already had data OR if they start fresh with Agent v8.6.0 and upgrade to the 8.6.1 Agent/osquery beat with the fix for sending this data incorrectly.

To make the fix for this problem seamless for users on upgrade, we need to do the following:

Install the latest 1.6.0 osquery_manager package which has a new pipeline which will correct the values that the 8.6.0 Agent/osquery beat send.
Check to see if there are "bad" values in the relevant data streams and roll the over if needed

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

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

elasticmachine · 2023-01-17T19:02:07Z

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

kevinlog · 2023-01-17T19:02:23Z

@elasticmachine merge upstream

kevinlog · 2023-01-17T20:18:06Z

@elasticmachine merge upstream

tomsonpl · 2023-01-19T19:50:40Z

@paul-tavares Hey, do you mind taking a look to check if the function above meets all the ''requirements'' of a good a async function within plugin as we discussed? It's quite late for me and my brain stopped working so would appreciate a second pair of eyes :P
Big thanks in advance!

…fix'

kevinlog · 2023-01-19T22:40:52Z

@elasticmachine merge upstream

kevinlog · 2023-01-20T00:15:12Z

@tomsonpl I checked out and tried your change and it looks to be working. thanks for pushing up the change!

I can get the data after the fix in OSQuery:

and I get the log output as expected to show that we update the package and rollover the indices:

tomsonpl · 2023-01-21T09:17:40Z

@elasticmachine merge upstream

tomsonpl · 2023-01-23T06:59:04Z

@elasticmachine merge upstream

tomsonpl

LGTM. I am not sure about the way (force) to upgrade for users' integration, but it seems to be the best idea we have for now :) Thanks @kevinlog !

kibana-ci · 2023-01-23T08:01:44Z

💚 Build Succeeded

Buildkite Build
Commit: 3b36267

Metrics [docs]

✅ unchanged

History

💚 Build #101626 succeeded 2ab7c4f
💔 Build #101566 failed 79900fb
💚 Build #101537 succeeded a2971b3
💔 Build #101531 failed b3794fd
💚 Build #100856 succeeded 2574f24
💔 Build #100838 failed 1bec2c6

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

dasansol92

Code looks great to me. I just wonder about the amount of async calls and awaits it will do during the plugin start process. Is this something could take too much time?

dasansol92 · 2023-01-23T09:38:05Z

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

@@ -135,6 +136,9 @@ export class OsqueryPlugin implements Plugin<OsqueryPluginSetup, OsqueryPluginSt
        await this.initialize(core, dataViewsService);
      }

+      // Upgrade integration into 1.6.0 and rollover if found 'generic' dataset - we do not want to wait for it
+      upgradeIntegration({ packageInfo, client, esClient, logger: this.logger });


Suggestion: as the upgrade is conditional, could this be renamed to something like upgradeIntegrationIfNeeded or checkAndUpgradeIntegrationIfNeeded

dasansol92 · 2023-01-23T09:44:06Z

x-pack/plugins/osquery/server/utils/upgrade_integration.ts

+      });
+
+      // Then for each of those datastreams, we need to see if they need to rollover.
+      await asyncForEach(dataStreams.data_streams, async (dataStream) => {


Doesn't need to be changed, but if you would like to avoid asyncForEach import, you could just use a for loop:

for (const dataStream of dataStreams.data_streams) { [....] }

Could this be done in parallel? Wondering if you could use pMap here and speed up the process.

tomsonpl · 2023-01-23T10:51:56Z

We discussed this offline with @dasansol92 , and given the short time we have for the 8.6.1 FF, we agreed that it's gonna be merged in the current state and the performance improvements can be added in a next iteration.
Big thanks @dasansol92 :)

dasansol92

Code changes LGTM, suggestions can be done later.

tomsonpl · 2023-01-23T10:54:39Z

Merged, @kevinlog FYI.

kibanamachine · 2023-01-23T10:57:41Z

💔 All backports failed

Status	Branch	Result
❌	8.6	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 148991

Questions ?

Please refer to the Backport tool documentation

tomsonpl · 2023-01-23T11:19:10Z

💚 All backports created successfully

Status	Branch	Result
✅	8.6

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…r package and rolling over indices on upgrade (elastic#148991) (cherry picked from commit 192c739) # Conflicts: # x-pack/plugins/osquery/server/plugin.ts # x-pack/plugins/osquery/tsconfig.json

paul-tavares

Sorry for the delay. I left a comment regarding the pinning of the package version to 1.6.0

paul-tavares · 2023-01-23T14:24:11Z

x-pack/plugins/osquery/server/utils/upgrade_integration.ts

+        savedObjectsClient: client,
+        pkgkey: pkgToPkgKey({
+          name: packageInfo.name,
+          version: '1.6.0', // This package upgrade is specific to a bug fix, so keeping the upgrade focused on 1.6.0


You seem to be pinning the version to 1.6.0 here, is that correct? What if the newest version is higher than 1.6.0?

The reason this looks strange to me is because this PR is being merged to main which means it applies to v8.7++.

Thank @paul-tavares! Thanks for raising the issue :) I haven’t really think about it, but the goal is to force upgrade older version than 1.6.0 to 1.6.0 which fixed the issue. The rest - update to any other versions are not crucial to us.
I think we can still discuss how to handle this 8.7 onwards, the most crucial part is to fix 8.6.1 and actually @kevinlog suggested another way to fix it: #149325

@paul-tavares - osquery doesn't normally upgrade it's package automatically. We wanted to make an exception in this case since 1.6.0 contains a critical fix. We'll only try to upgrade if the package version is below 1.6.0. We will make sure the package is at 1.6.0 and after that, users can check and upgrade the package as normal with newer changes pushed out.

Ok. As long as you realize that the forced upgrade here might be to a version (1.6.0) that is not the latest and thats ok condition, then 👍

Thanks for the responses @tomsonpl , @kevinlog

patrykkopycinski and others added 4 commits January 15, 2023 00:46

[Osquery] Fix 8.6.0 agent data_stream values

190dd47

add try catch to avoid rollover breaking the package policy update

0d108bf

OSquery datastream rollover check moved to plugin start

788a1c0

sort indicies, perform rollover, package update code

6a6641d

kevinlog force-pushed the task/move-datastream-check-to-plugin-start branch from c63b8f6 to 6a6641d Compare January 16, 2023 21:38

get package update to work

7915504

kevinlog force-pushed the task/move-datastream-check-to-plugin-start branch from 1f9025e to 7915504 Compare January 17, 2023 16:50

kevinlog and others added 6 commits January 17, 2023 11:52

revert fleet integration change

7d9cd2b

revert uneeded change

02d3c3f

revert uneeded change

6df1c42

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

86e6998

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

ignore ts error to access mapping values

197d573

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

3a12288

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

kevinlog added Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.7.0 v8.6.0 v8.6.1 release_note:fix labels Jan 17, 2023

kevinlog marked this pull request as ready for review January 17, 2023 19:02

kevinlog requested a review from a team as a code owner January 17, 2023 19:02

kevinlog requested review from patrykkopycinski and dasansol92 January 17, 2023 19:02

Merge branch 'main' into task/move-datastream-check-to-plugin-start

1bec2c6

kevinlog requested review from tomsonpl and removed request for patrykkopycinski January 17, 2023 19:02

kevinlog changed the title ~~OSquery datastream rollover check moved to plugin start~~ OSquery fix issue with document rejection by upgrading osquery_manager package and rolling over indices on upgrade Jan 17, 2023

Merge branch 'main' into task/move-datastream-check-to-plugin-start

2574f24

tomsonpl requested a review from paul-tavares January 19, 2023 19:47

tomsonpl and others added 2 commits January 19, 2023 20:52

fix file name

67e44ac

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

a2971b3

…fix'

Merge branch 'main' into task/move-datastream-check-to-plugin-start

79900fb

Merge branch 'main' into task/move-datastream-check-to-plugin-start

2ab7c4f

Merge branch 'main' into task/move-datastream-check-to-plugin-start

3b36267

tomsonpl self-requested a review January 23, 2023 07:50

tomsonpl approved these changes Jan 23, 2023

View reviewed changes

dasansol92 reviewed Jan 23, 2023

View reviewed changes

dasansol92 approved these changes Jan 23, 2023

View reviewed changes

tomsonpl added the bug Fixes for quality problems that affect the customer experience label Jan 23, 2023

tomsonpl merged commit 192c739 into elastic:main Jan 23, 2023

tomsonpl mentioned this pull request Jan 23, 2023

[8.6] OSquery fix issue with document rejection by upgrading osquery_manager package and rolling over indices on upgrade (#148991) #149310

Closed

kevinlog deleted the task/move-datastream-check-to-plugin-start branch January 23, 2023 11:36

paul-tavares reviewed Jan 23, 2023

View reviewed changes

kevinlog removed v8.6.0 v8.6.1 labels Jan 23, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jan 23, 2023

KOTungseth added the Feature:Osquery Security Solution Osquery feature label Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSquery fix issue with document rejection by upgrading osquery_manager package and rolling over indices on upgrade #148991

OSquery fix issue with document rejection by upgrading osquery_manager package and rolling over indices on upgrade #148991

kevinlog commented Jan 16, 2023 •

edited by kibanamachine

Loading

elasticmachine commented Jan 17, 2023

kevinlog commented Jan 17, 2023

kevinlog commented Jan 17, 2023

tomsonpl commented Jan 19, 2023

kevinlog commented Jan 19, 2023

kevinlog commented Jan 20, 2023

tomsonpl commented Jan 21, 2023

tomsonpl commented Jan 23, 2023

tomsonpl left a comment

kibana-ci commented Jan 23, 2023

dasansol92 left a comment

dasansol92 Jan 23, 2023

dasansol92 Jan 23, 2023 •

edited

Loading

dasansol92 Jan 23, 2023

tomsonpl commented Jan 23, 2023

dasansol92 left a comment

tomsonpl commented Jan 23, 2023

kibanamachine commented Jan 23, 2023

tomsonpl commented Jan 23, 2023

paul-tavares left a comment

paul-tavares Jan 23, 2023

tomsonpl Jan 23, 2023

kevinlog Jan 23, 2023

paul-tavares Jan 23, 2023

OSquery fix issue with document rejection by upgrading osquery_manager package and rolling over indices on upgrade #148991

OSquery fix issue with document rejection by upgrading osquery_manager package and rolling over indices on upgrade #148991

Conversation

kevinlog commented Jan 16, 2023 • edited by kibanamachine Loading

Summary

Checklist

Risk Matrix

For maintainers

elasticmachine commented Jan 17, 2023

kevinlog commented Jan 17, 2023

kevinlog commented Jan 17, 2023

tomsonpl commented Jan 19, 2023

kevinlog commented Jan 19, 2023

kevinlog commented Jan 20, 2023

tomsonpl commented Jan 21, 2023

tomsonpl commented Jan 23, 2023

tomsonpl left a comment

Choose a reason for hiding this comment

kibana-ci commented Jan 23, 2023

💚 Build Succeeded

Metrics [docs]

History

dasansol92 left a comment

Choose a reason for hiding this comment

dasansol92 Jan 23, 2023

Choose a reason for hiding this comment

dasansol92 Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

dasansol92 Jan 23, 2023

Choose a reason for hiding this comment

tomsonpl commented Jan 23, 2023

dasansol92 left a comment

Choose a reason for hiding this comment

tomsonpl commented Jan 23, 2023

kibanamachine commented Jan 23, 2023

💔 All backports failed

Manual backport

Questions ?

tomsonpl commented Jan 23, 2023

💚 All backports created successfully

Questions ?

paul-tavares left a comment

Choose a reason for hiding this comment

paul-tavares Jan 23, 2023

Choose a reason for hiding this comment

tomsonpl Jan 23, 2023

Choose a reason for hiding this comment

kevinlog Jan 23, 2023

Choose a reason for hiding this comment

paul-tavares Jan 23, 2023

Choose a reason for hiding this comment

kevinlog commented Jan 16, 2023 •

edited by kibanamachine

Loading

dasansol92 Jan 23, 2023 •

edited

Loading