The defaultIndex advanced setting breaks after 8.0 upgrade

[jportner]

Originally described in #46124 (comment)

Kibana version: 8.0, 8.1, 8.2, 8.3

Describe the bug:

Advanced settings (aka UI settings) are persisted in the config saved object, and there is one of these per space.

In #100489 we migrated most isolated saved object types (including index patterns AKA data views) to be share-capable in the 8.0 release.
This means that, for any data view that was created in a custom space, its ID would be changed upon upgrading to 8.0.

The problem is that we never added a corresponding migration for the defaultIndex advanced setting, that is technically an object link and it does not use the references field so it was never migrated the way it should have been.

The defaultIndex is sort of self-healing (it will automatically be set if the user has appropriate privileges), but this is frustrating for clusters that have mostly read-only users with lots of spaces, and the if there are multiple data views then the correct one will be lost.

Steps to reproduce:

1. Start with Kibana 7.17
2. Create "Another" space and switch to it
3. Add some data
4. Create an index pattern
5. Set the defaultIndex in advanced settings to your index pattern's ID
6. Upgrade to Kibana 8.0
7. Observe that your defaultIndex advanced setting has the old index pattern data view ID

Expected behavior:

The defaultIndex should be migrated properly.

Screenshots (if relevant):

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[elasticmachine]

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

[jportner]

This could be addressed by modifying the 8.0 migration in the config object here:

kibana/src/core/server/ui_settings/saved_objects/migrations.ts

Lines 78 to 106 in 3730dd0

	'8.0.0': (doc: SavedObjectUnsanitizedDoc<any>): SavedObjectSanitizedDoc<any> => ({
	...doc,
	...(doc.attributes && {
	attributes: Object.keys(doc.attributes).reduce(
	(acc, key) =>
	[
	// owner: Team:Geo
	'visualization:regionmap:showWarnings',
	'visualization:tileMap:WMSdefaults',
	'visualization:tileMap:maxPrecision',
	// owner: Team:Core
	'telemetry:optIn',
	'xPackMonitoring:allowReport',
	'theme:version',
	// owner: Team:AppServices
	'courier:batchSearches',
	].includes(key)
	? {
	...acc,
	}
	: {
	...acc,
	[key]: doc.attributes[key],
	},
	{}
	),
	}),
	references: doc.references || [],
	}),

We can calculate if the defaultIndex ID should be changed:

let newDefaultIndex: string | undefined;
if (doc.namespace !== undefined) {
  newDefaultIndex = deterministicallyRegenerateObjectId(doc.namespace, 'index-pattern', doc.attributes.defaultIndex);
}

Here is the algorithm for generating the new object ID:

kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts

Lines 838 to 848 in a044026

	/**
	* Deterministically regenerates a saved object's ID based upon it's current namespace, type, and ID. This ensures that we can regenerate
	* any existing object IDs without worrying about collisions if two objects that exist in different namespaces share an ID. It also ensures
	* that we can later regenerate any inbound object references to match.
	*
	* @note This is only intended to be used when single-namespace object types are converted into multi-namespace object types.
	* @internal
	*/
	export function deterministicallyRegenerateObjectId(namespace: string, type: string, id: string) {
	return uuidv5(`${namespace}:${type}:${id}`, uuidv5.DNS); // the uuidv5 namespace constant (uuidv5.DNS) is arbitrary
	}

It's not great practice to do this, but we could change the existing 8.0 migration in 8.3.1/8.4.0 so that it also fixes the default index ID.
That will help people who are upgrading from 7.17 to 8.3.1, for example, but anyone who upgraded from 7.17 to 8.3.0 will have to manually fix the defaultIndex.

We can't add a new migration for 8.3.1 because we have no way of determining if a defaultIndex has already been changed at that point.

WDYT @elastic/kibana-core ?

[lukeelmers]

It's not great practice to do this, but we could change the existing 8.0 migration in 8.3.1/8.4.0 so that it also fixes the default index ID.

Yeah, I don't love this idea, but I also don't see any other alternatives if this is something we want to fix.

I guess I'd be comfortable with it as this technically represents a sort of data loss, and making a change to an existing migration like this is not without precedent. But I think we'd need to clearly document it in the code so there's an explanation for why the migration's implementation is different in the 8.3 branch.

Will let others chime in though, and try to poke holes in this.

[pgayvallet]

The suggested setting migration looks fine to me, however I don't think we can realistically 'patch' the existing 8.0 migration to apply it. First obvious consequence would be that customers already in a 8.x version would not benefit from the fix. Also, as we can't backport lower than 8.3.x anyway, I don't really see any benefit on breaking the rule here.

I think our only option would be to add it as a 8.3.1 migration instead on main and 8.3 branches. Of course, we would then need a way to detect if the current defaultIndex setting value is a migrated index or not to decide if the ID should be migrated, in case the value was potentially manually modified / fixed somehow for some customers already in 8.x.

WDYT?

[jportner]

I think our only option would be to add it as a 8.3.1 migration instead on main and 8.3 branches. Of course, we would then need a way to detect if the current defaultIndex setting value is a migrated index or not to decide if the ID should be migrated, in case the value was potentially manually modified / fixed somehow for some customers already in 8.x.

The "need a way to detect if the current defaultIndex setting value is a migrated index or not to decide if the ID should be migrated" part is impossible, I think. If you have a suggestion on how to do that, I'd be interested to hear it 😅

That's the only reason I suggested patching the 8.0 migration -- it won't help folks who've already upgraded to 8.x, sure, but the vast majority of our users haven't upgraded to 8.x yet, so this would help prevent the problem for them in the future (assuming they upgrade straight from 7.17 to 8.3.1+)

[lukeelmers]

Also, as we can't backport lower than 8.3.x anyway, I don't really see any benefit on breaking the rule here.

I think what Joe was suggesting here was not backporting to 8.0, but rather changing the 8.0 migration on the main and 8.3 branches only. So the 8.0 migration that shipped with 8.0 would have a different implementation than the 8.0 migration that ships with 8.3.1

[jportner]

I think what Joe was suggesting here was not backporting to 8.0, but rather changing the 8.0 migration on the main and 8.3 branches only. So the 8.0 migration that shipped with 8.0 would have a different implementation than the 8.0 migration that ships with 8.3.1

Yes, exactly, sorry I tried to make that clear 😅

[pgayvallet]

think what Joe was suggesting here was not backporting to 8.0, but rather changing the 8.0 migration on the main and 8.3 branches only

Yea, I get that. And as I said, I don't like it, for the reasons I enumerated in my previous comment. We won't fix the problem for users already in a 8.x version by patching the 8.0.0 migration (as it will not be applied again). And given a common way to upgrade from X.last is to go to X+1.first first, We may not be solving the problem for most of our users by doing what was suggested (e.g 7.17->8.0->8.3 scenario).

and making a change to an existing migration like this is not without precedent

TIL. Any example coming to mind? Not for the past 2.5 years, at least that I'm aware of.

The "need a way to detect if the current defaultIndex setting value is a migrated index or not to decide if the ID should be migrated" part is impossible, I think. If you have a suggestion on how to do that, I'd be interested to hear it

You mean we don't have any way to detect/guess if an ID is pre or post shareable-migration rewrite? If that's the case, that's plain terrible... But then I don't have a better idea than your suggestion.

[jportner]

You mean we don't have any way to detect/guess if an ID is pre or post shareable-migration rewrite? If that's the case, that's plain terrible... But then I don't have a better idea than your suggestion.

Well, we regenerate the ID using a hash function (uuidv5) so there's no way to reverse the hash.

We could attempt to resolve the index pattern -- but migrations must be done in isolation, we can't query any other documents.
The only relevant information we have is the attributes.defaultIndex and the namespace from the current config object.

Another option:
Add a post-start "custom migration" that queries for all config objects, checks to see if their defaultIndex values are intact, attempts to fix any non-intact ones using the aforementioned deterministicallyRegenerateObjectId, and adds a canary attribute to the object attributes to mark that it doesn't need to be checked again.
It's a bit more work but it would solve the problem for all users upon upgrading to 8.3.1+.

[jportner]

Add a post-start "custom migration"

FWIW we have some precedent here, we had to do this for APM in #126176 and #126378 to convert a space-agnostic saved object type to a space-isolated one

[pgayvallet]

Not sure which option I prefer to be honest. @lukeelmers, what do you think about this?

[TinaHeiligers]

My gut tells me adding a post-start custom migration is the way to go here to avoid the code discrepancies we'd have in different branches. Since that approach seemed to work out fine for the APM case, it looks like the best of the choices we have, certainly winning against hacking code for specific branches.

[jportner]

After chatting with Pierre and Tina offline, I decided to take a different approach in #133339 --

The config saved objects are versioned and isolated to each space.
When a client attempts to fetch or create a config saved object, Kibana first checks if there's already an old one that needs to be upgraded. This PR takes advantage of the existing functionality to simply call any necessary transform functions during that process. This way, we can ensure that transforms will be applied to each config after upgrading, but we don't need to add a complicated "on start" loop to search and update saved objects.

[lukeelmers]

Sorry not to respond here earlier.

and making a change to an existing migration like this is not without precedent

TIL. Any example coming to mind? Not for the past 2.5 years, at least that I'm aware of.

FWIW this was the example I was thinking of, where a visualizations migration was retroactively added for an older version, and then later fixed in the same migration fn when a bug was found. I think there have been a few similar vis-related migrations that got fixed this way. I don't think it's necessarily something we should be doing, just saying that this wouldn't be a first :)

After chatting with Pierre and Tina offline, I decided to take a different approach in #133339

Took a look at the PR, and I think that approach is a good middle ground. It feels less hacky than the other 2 options discussed in this thread since we are mostly plugging into existing functionality 👍

[afharo]

Bad bot. Closing...