Use modelVersions instead of hashes to track changes in SO types by gsoldevila · Pull Request #176803 · elastic/kibana

gsoldevila · 2024-02-13T10:18:51Z

Summary

We're currently storing information about Saved Object types in the <index>.mapping._meta.
More specifically, we're storing hashes of the mappings of each type, under the migrationMappingPropertyHashes property.
This allows us to detect which types' mappings have changed, in order to update and pick up changes only for those types.

The problem is that md5 cannot be used if we want to be FIPS compliant.

Thus, the idea is to stop using hashes to track changes in the SO mappings.
Instead, we're going to use the same modelVersions introduced by ZDT:

Whenever mappings change for a given SO type, the corresponding modelVersion will change too, so modelVersions can be used to determine if mappings have changed.

jloleysens

Overall approach makes sense to me to facilitate the transition away from md5 hashes. I left a few comments!

jloleysens · 2024-02-15T10:32:46Z

nit: it's a little funky that migrationMappingPropertyHashes will no longer contain hashes, but doesn't seem worth the upheaval of changing it and seems like something we need to keep bwc support for anyway.

Don't have a good solution in mind right now other than tracking it for later and adding a comment on wherever this type is defined.

Thinking about it, I think I'll switch to the same property names used on ZDT algorithm: appVersions and docVersions. This way, I will not impact the existing hashes field (which will simply be deprecated).

I believe the advantages of doing this are worth it:

We will not be reusing a property with an outdated / misleading name.

We will align with the _meta stored by the ZDT algorithm, which could, in the future, facilitate transition from Cloud to Serverless.

If a migration fails or Kibana fails to start after a migration, and a rollback is performed, the previous version using hashes will still have the hashes field in place. This will prevent the migration logic from picking up ALL types (this could be a costly operation for large customers).

jloleysens · 2024-02-15T10:54:05Z

If I understand this correctly: we need to know for sure that this map correctly reflects hashes for the previous 8.x release in order to know we will have "hits" for this md5 hashes. For non-stack releases, to my knowledge this is only serverless, we accept that this static map may have misses? Am I following correctly 😄

If that's true I'd consider naming this file something like 8_13_0_hash_to_version_map.json since that is the last stack version where we will be storing hashes in migrationMappingPropertyHashes.

nit: might be worth adding a README.md explaining the purpose/reason of this file (transitioning off of md5)

Agree to both comments!

For non-stack releases, to my knowledge this is only serverless, we accept that this static map may have misses?

On serverless we use the ZDT algorithm, which uses the mappingVersions and docVersions instead of the migrationMappingPropertyHashes, so this change only impacts cloud and on prem.

nit: might be worth adding a README.md explaining the purpose/reason of this file (transitioning off of md5)

Added a description (now a const instead of a JSON file).

pgayvallet

A few NITs and questions, but the approach looks fine to me

pgayvallet · 2024-02-19T16:01:27Z

Asking as I'm not sure anymore: Could left.updatedHashes contain root fields (e.g namespace and so on) in addition to types previously, and did that change in that PR in left.updatedTypes?

The modelVersion logic is currently not including the root fields, so with the PR we're not storing model versions for them in the _meta.

I have an update on the pipes to preserve existing behavior, and what I propose is to deep compare the mappings objects for the root fields (which are pretty simple). This way, the updated types will handle comparison of SO types through their modelVersions and comparison of root fields through deep compare of their mappings.
It will emit 2 different Left responses depending on which of them has changed (root_fields_changed vs types_changed), which in turn will simplify the logic in model.ts.

WDYT?

Seems good to me

pgayvallet · 2024-02-20T14:27:28Z

NIT: I know this was there already, but can somehow use something more precise than any for those parameters?

pgayvallet · 2024-02-20T14:30:46Z

NIT: why is the import * as XXX required here compared to import XXX from ?

It was not apparently. I have moved it to constants.ts on @kbn/core-saved-objects-base-server-internal.

pgayvallet · 2024-02-20T14:33:07Z

NIT: I see it defaults to HASH_TO_VERSION_MAP, but I think I'd still rather have the new parameter not optional, wdyt?

Sounds good to me, we'll be more consistent with similar params that are being passed 👌🏼

rudolf · 2024-02-19T11:23:20Z

Is it possible to tighten the types and remove any?

(now part of diff_mappings.ts)
Fixed in the latest updates

rudolf · 2024-02-19T11:35:17Z

When a type's metadata isn't available in the index actual[type] will be undefined so indexHashOrVersion could also be undefined.

(Now part of compare_mappings.ts)

Correct, this has been made optional in the latest updates.

rudolf · 2024-02-19T14:49:30Z

${type}|${indexHashOrVersion} can type ever be undefined?

(Now part of compare_mappings.ts)

type cannot be undefined, as we loop through the different types from the registry.

On the other hand, indexHashOrVersion could be undefined (e.g. if it is a new type).
In this scenario, searching for type|undefined in the map would yield no results, and thus that type would be considered as updated.

pgayvallet

LGTM

…appings_addition`s

kibana-ci · 2024-03-25T13:43:41Z

💚 Build Succeeded

Buildkite Build
Commit: 26283c5

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
`@kbn/core-saved-objects-base-server-internal`	71	180	+109
`@kbn/core-saved-objects-migration-server-internal`	94	96	+2
total			+111

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	10	11	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	112	223	+111
`@kbn/core-saved-objects-migration-server-internal`	129	131	+2
total			+113

References to deprecated APIs

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	18	23	+5

History

💔 Build #199100 failed 257d4a1
💔 Build #199083 failed 05fa106
💔 Build #198260 failed 8ca46c76ac3bb819476275617487f14af845e7f2
💚 Build #197992 succeeded 2eaaa3dd926bfa5e7a7e6c35d5e06712161dab04
💔 Build #197911 failed cd0e33f637d8156430cf741f9601408a7fb236ea

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

cc @gsoldevila

rudolf

nice work

## Summary Follow-up of #176803 Now that `8.14` has been branched and it is about to be released, we can remove the test that ensures the `HASH_TO_VERSION_MAP` is aligned. This PR also rolls back a few changes on the map that shouldn't have been performed, as they belong to `8.15.0`.

## Summary Follow-up of elastic#176803 Now that `8.14` has been branched and it is about to be released, we can remove the test that ensures the `HASH_TO_VERSION_MAP` is aligned. This PR also rolls back a few changes on the map that shouldn't have been performed, as they belong to `8.15.0`. (cherry picked from commit ed2e59f)

# Backport This will backport the following commits from `main` to `8.14`: - [Kbn 176803 cleanup 8.14 md5 check (#183416)](#183416)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co>

## Summary Addresses elastic/elastic-entity-model#70 Fixes regression introduced in #176803

## Summary Addresses elastic/elastic-entity-model#70 Fixes regression introduced in elastic#176803 (cherry picked from commit 8de3636)

…197573) # Backport This will backport the following commits from `main` to `8.16`: - [Update mappings if/when new SO types are introduced (#197061)](#197061)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co>

## Summary Addresses elastic/elastic-entity-model#70 Fixes regression introduced in elastic#176803 (cherry picked from commit 8de3636) # Conflicts: # src/core/server/integration_tests/saved_objects/migrations/group1/v2_migration.test.ts # src/core/server/integration_tests/saved_objects/migrations/group2/multiple_kb_nodes.test.ts # src/core/server/integration_tests/saved_objects/migrations/kibana_migrator_test_kit.fixtures.ts

## Summary Addresses elastic/elastic-entity-model#70 Fixes regression introduced in elastic#176803 (cherry picked from commit 8de3636) # Conflicts: # src/core/server/integration_tests/saved_objects/migrations/group1/v2_migration.test.ts

…197598) # Backport This will backport the following commits from `main` to `8.15`: - [Update mappings if/when new SO types are introduced (#197061)](#197061)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) \n\n### Questions ?\nPlease refer to the [Backport tool\ndocumentation](https://github.com/sqren/backport)\n\n\n\nCo-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co>"}},{"branch":"8.15","label":"v8.15.3","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->

…197597) # Backport This will backport the following commits from `main` to `8.x`: - [Update mappings if/when new SO types are introduced (#197061)](#197061)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) \n\n### Questions ?\nPlease refer to the [Backport tool\ndocumentation](https://github.com/sqren/backport)\n\n\n\nCo-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co>"}},{"branch":"8.15","label":"v8.15.3","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->

gsoldevila added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting labels Feb 13, 2024

gsoldevila self-assigned this Feb 13, 2024

gsoldevila added 8.14 candidate v8.14.0 labels Feb 14, 2024

gsoldevila marked this pull request as ready for review February 14, 2024 18:29

gsoldevila requested a review from a team as a code owner February 14, 2024 18:29

jloleysens reviewed Feb 15, 2024

View reviewed changes

pgayvallet reviewed Feb 20, 2024

View reviewed changes

rudolf reviewed Feb 20, 2024

View reviewed changes

gsoldevila force-pushed the kbn-169734-fips-compliant-migrations branch 7 times, most recently from db3ff4c to 8cb7af1 Compare March 7, 2024 10:29

gsoldevila requested review from jloleysens, pgayvallet and rudolf March 7, 2024 13:21

gsoldevila force-pushed the kbn-169734-fips-compliant-migrations branch 2 times, most recently from 4223d6a to 99d8ea8 Compare March 8, 2024 09:09

pgayvallet approved these changes Mar 14, 2024

View reviewed changes

gsoldevila force-pushed the kbn-169734-fips-compliant-migrations branch 2 times, most recently from b4eebbf to cd0e33f Compare March 18, 2024 08:53

gsoldevila added 4 commits March 25, 2024 10:10

Use modelVersions instead of hashes to track changes in SO types

495ba1d

Remove unnecessary mocks

f491fda

Parametrize hashToVersionMap in KibanaMigrator

12078dc

Add integration tests for the md5 to modelVersion transition

fa538b8

gsoldevila added 8 commits March 25, 2024 10:10

Update zdt tests, as v2 state is v2-partially-migrated by default

4909084

Fix udpated typo

f6cf1dc

Update hashToVersionMap with latest changes

c099cc7

Fix remaining udpated typo

31cdd22

Update outdated hashToVersionMap

52f3e40

Rely on modelVersions that bring mappings_addition

53baf7c

Consider a type has changed only if the new model versions include `m…

4e91b51

…appings_addition`s

Update with latest mappings changes

05fa106

gsoldevila force-pushed the kbn-169734-fips-compliant-migrations branch from 8ca46c7 to 05fa106 Compare March 25, 2024 09:17

gsoldevila added 3 commits March 25, 2024 11:02

Update outdated tests

257d4a1

Store latest model versions in _meta

f6b6eb0

Fix outdated test

26283c5

rudolf approved these changes Mar 25, 2024

View reviewed changes

gsoldevila merged commit 2f5396c into elastic:main Mar 25, 2024

thomheymann mentioned this pull request Mar 26, 2024

[Observability Onboarding] Link all “Add data” buttons in observability to onboarding landing page #179459

Merged

legrego mentioned this pull request Mar 28, 2024

Change hashing algorithm in Saved Object Migrations to be FIPS compliant #169734

Closed

gsoldevila mentioned this pull request May 14, 2024

Kbn 176803 cleanup 8.14 md5 check #183416

Merged

gsoldevila mentioned this pull request Oct 21, 2024

Update mappings if/when new SO types are introduced #197061

Merged

gsoldevila added a commit that referenced this pull request Oct 24, 2024

Update mappings if/when new SO types are introduced (#197061)

8de3636

## Summary Addresses elastic/elastic-entity-model#70 Fixes regression introduced in #176803

Conversation

gsoldevila commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsoldevila Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibana-ci commented Mar 25, 2024

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

References to deprecated APIs

History

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

gsoldevila commented Feb 13, 2024 •

edited

Loading

gsoldevila Mar 6, 2024 •

edited

Loading