siem-detection-engine-rule-status migration could cause failed upgrade

[rudolf]

If alertId is not a string we don't remove the alertId

https://github.com/elastic/kibana/blob/7.16/x-pack/plugins/security_solution/server/lib/detection_engine/rules/legacy_rule_status/legacy_migrations.ts#L55

However, since there is no mapping for alertId this could theoretically cause the migration to fail with mapping set to strict, dynamic introduction of [alertId] within [siem-detection-engine-rule-status] is not allowed
Related #114585

A more defensive approach would be to always remove alertId:

index 72ab4a2237b..e4df3487202 100644
--- a/x-pack/plugins/security_solution/server/lib/detection_engine/rules/legacy_rule_status/legacy_migrations.ts
+++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rules/legacy_rule_status/legacy_migrations.ts
@@ -52,7 +52,11 @@ export const legacyMigrateRuleAlertIdSOReferences = (
 
   // early return if alertId is not a string as expected
   if (!isString(alertId)) {
-    return { ...doc, references: existingReferences };
+    return {
+      ...doc,
+      attributes: otherAttributes,
+      references: existingReferences,
+    };
   }
 
   const alertReferences = legacyMigrateAlertId({

(Sorry for missing this during our code review of the PR 😅 )

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)

[rudolf]

Note: we also have a test harness for testing migrations https://docs.elastic.dev/kibana-dev-docs/tutorials/testing-plugins#integration-testing

It's very similar to your existing e2e test except that it uses the import API instead of esArchiver, might not add much value to convert the existing tests but just an FYI

[banderror]

@rudolf, that's a great catch! Thank you so much 🙏

However, since there is no mapping for alertId this could theoretically cause the migration to fail with mapping set to strict, dynamic introduction of [alertId] within [siem-detection-engine-rule-status] is not allowed

Just curious - why theoretically? 🙂

test harness for testing migrations https://docs.elastic.dev/kibana-dev-docs/tutorials/testing-plugins#integration-testing

I didn't know about it! Super helpful, thank you. Will take a closer look.
Is it possible to catch these kind of bugs in this type of integration tests? What are the limitations of the test harness and what it does under the hood?

[rudolf]

Just curious - why theoretically? 🙂

This could only happen if there was a document with an alertId that wasn't a String but also not null so it would have to be a number or something like ["myvalue"]. This might only ever happen if a user meddles with their data in which case it's not so unusual if it causes a migration to fail.

[rudolf]

Is it possible to catch these kind of bugs in this type of integration tests? What are the limitations of the test harness and what it does under the hood?

Yeah the test harness should catch this specific bug.

The harness basically just spins up Elasticsearch and Kibana like FTR would. You then supply outdated saved objects which it will import into Kibana/Elasticsearch using the Saved Objects Import API. When saved objects are imported we apply migrations to them so during this step we will exercise the migration logic. Because the migrated objects are written to Elasticsearch, if there's a field left after the migration for which there's no mapping it will cause this bug to surface.

After importing, the test harness will use the Saved Objects export API to export the objects so that you test can assert that they have the right shape after the migration.

One limitation is that your saved object type should be importable/exportable but I believe after 8.0 most types should already be importable/exportable. Another thing to be aware of is import/export hooks will be applied so if a type has these hooks it could change the data in a way that's not directly related to the saved object migration.

[rudolf]

I had a look at #112869 which is very similar, but in this case the ruleAlertId field was never removed from the mappings so it couldn't cause a failed migration. However, if, in the future we even think "hey this field is unused, let's remove it from the mappings" we could introduce an upgrade failure.

I see comments that this saved object type is deprecated so maybe it's highly unlikely that we ever make any changes until we remove the whole thing but I just want to flag it.

[spong]

I also didn't know this test harness existed, so thanks for sharing @rudolf! 🙂

One limitation is that your saved object type should be importable/exportable but I believe after 8.0 most types should already be importable/exportable.

That said, this SO isn't exposed in the SOM UI, so we won't be able to leverage it here. We're planning to remove this SO entirely in early 8.x out in favor of leveraging the rule execution log, but good to know either way 👍.

I had a look at #112869 which is very similar, but in this case the ruleAlertId field was never removed from the mappings so it couldn't cause a failed migration. However, if, in the future we even think "hey this field is unused, let's remove it from the mappings" we could introduce an upgrade failure.

Yeah I spoke to @FrankHassanabad about this during implementation and he mentioned he ended up keeping it around for certain compatibility reasons iirc, but noted that there shouldn't be any reason it couldn't be removed. There were less usages of alertId within the scope of the siem-detection-engine-rule-status SO, so I ended up removing it in effort to prevent folks from using it at all instead of accessing via references. Either way, thanks for catching this corner case here, really appreciate it! 🙂

[elasticmachine]

Pinging @elastic/security-detections-response (Team:Detections and Resp)