Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent future convertToMultiNamespaceType migrations #147369

Merged
merged 5 commits into from
Dec 21, 2022
Merged

Prevent future convertToMultiNamespaceType migrations #147369

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2ce5baa
Prevent future convertToMultiNamespaceType migrations
gsoldevila Dec 12, 2022
ec69b92
Address PR comments
gsoldevila Dec 21, 2022
4c6a0f3
Merge branch 'main' into kbn-147344-prevent-future-convert-to-multi-n…
gsoldevila Dec 21, 2022
5d0c374
Update docs
gsoldevila Dec 21, 2022
6fbdfdd
Update docs, take 2
gsoldevila Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/developer/advanced/sharing-saved-objects.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
This guide describes the "Sharing saved objects" effort, and the breaking changes that plugin developers need to be aware of for the planned
8.0 release of {kib}. It also describes how developers can take advantage of this feature.

`From *8.7.0*, as a step towards Zero-Down-Time upgrades, plugins are no longer allowed to switch to shareable saved objects.`
Copy link
Contributor Author

@gsoldevila gsoldevila Dec 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

[[sharing-saved-objects-overview]]
=== Overview

Expand Down
2 changes: 2 additions & 0 deletions ...objects/core-saved-objects-api-server-internal/src/test_helpers/repository.test.common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ export const expectErrorInvalidType = (obj: TypeIdTuple, overrides?: Record<stri
expectErrorResult(obj, createUnsupportedTypeErrorPayload(obj.type), overrides);

export const KIBANA_VERSION = '2.0.0';
export const ALLOWED_CONVERT_VERSION = '8.0.0';
export const CUSTOM_INDEX_TYPE = 'customIndex';
/** This type has namespaceType: 'agnostic'. */
export const NAMESPACE_AGNOSTIC_TYPE = 'globalType';
Expand Down Expand Up @@ -372,6 +373,7 @@ export const createDocumentMigrator = (registry: SavedObjectTypeRegistry) => {
return new DocumentMigrator({
typeRegistry: registry,
kibanaVersion: KIBANA_VERSION,
convertVersion: ALLOWED_CONVERT_VERSION,
log: loggerMock.create(),
});
};
Expand Down
12 changes: 4 additions & 8 deletions ...d-objects/core-saved-objects-migration-server-internal/src/core/document_migrator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@ describe('DocumentMigrator', () => {
return {
kibanaVersion,
typeRegistry: createRegistry(),
minimumConvertVersion: '0.0.0', // no minimum version unless we specify it for a test case
log: mockLogger,
};
}

describe('validation', () => {
const createDefinition = (migrations: any) => ({
kibanaVersion: '3.2.3',
convertVersion: '8.0.0',
typeRegistry: createRegistry({
name: 'foo',
migrations: migrations as any,
Expand Down Expand Up @@ -163,7 +163,6 @@ describe('DocumentMigrator', () => {
name: 'foo',
convertToMultiNamespaceTypeVersion: 'bar',
}),
minimumConvertVersion: '0.0.0',
log: mockLogger,
};
expect(() => new DocumentMigrator(invalidDefinition)).toThrow(
Expand All @@ -179,27 +178,26 @@ describe('DocumentMigrator', () => {
convertToMultiNamespaceTypeVersion: 'bar',
namespaceType: 'multiple',
}),
minimumConvertVersion: '0.0.0',
log: mockLogger,
};
expect(() => new DocumentMigrator(invalidDefinition)).toThrow(
`Invalid convertToMultiNamespaceTypeVersion for type foo. Expected value to be a semver, but got 'bar'.`
);
});

it('validates convertToMultiNamespaceTypeVersion is not less than the minimum allowed version', () => {
it('validates convertToMultiNamespaceTypeVersion matches the convertVersion, if specified', () => {
const invalidDefinition = {
kibanaVersion: '3.2.3',
typeRegistry: createRegistry({
name: 'foo',
convertToMultiNamespaceTypeVersion: '3.2.4',
namespaceType: 'multiple',
}),
// not using a minimumConvertVersion parameter, the default is 8.0.0
convertVersion: '3.2.3',
log: mockLogger,
};
expect(() => new DocumentMigrator(invalidDefinition)).toThrowError(
`Invalid convertToMultiNamespaceTypeVersion for type foo. Value '3.2.4' cannot be less than '8.0.0'.`
`Invalid convertToMultiNamespaceTypeVersion for type foo. Value '3.2.4' cannot be any other than '3.2.3'.`
);
});

Expand All @@ -211,7 +209,6 @@ describe('DocumentMigrator', () => {
convertToMultiNamespaceTypeVersion: '3.2.4',
namespaceType: 'multiple',
}),
minimumConvertVersion: '0.0.0',
log: mockLogger,
};
expect(() => new DocumentMigrator(invalidDefinition)).toThrowError(
Expand All @@ -227,7 +224,6 @@ describe('DocumentMigrator', () => {
convertToMultiNamespaceTypeVersion: '3.1.1',
namespaceType: 'multiple',
}),
minimumConvertVersion: '0.0.0',
log: mockLogger,
};
expect(() => new DocumentMigrator(invalidDefinition)).toThrowError(
Expand Down
24 changes: 9 additions & 15 deletions .../saved-objects/core-saved-objects-migration-server-internal/src/core/document_migrator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,6 @@ import {
import { MigrationLogger } from './migration_logger';
import { TransformSavedObjectDocumentError } from '.';

const DEFAULT_MINIMUM_CONVERT_VERSION = '8.0.0';

export type MigrateFn = (doc: SavedObjectUnsanitizedDoc) => SavedObjectUnsanitizedDoc;
export type MigrateAndConvertFn = (doc: SavedObjectUnsanitizedDoc) => SavedObjectUnsanitizedDoc[];

Expand Down Expand Up @@ -95,7 +93,7 @@ interface TransformOptions {
interface DocumentMigratorOptions {
kibanaVersion: string;
typeRegistry: ISavedObjectTypeRegistry;
minimumConvertVersion?: string;
convertVersion?: string;
log: Logger;
}

Expand Down Expand Up @@ -142,7 +140,7 @@ export interface VersionedTransformer {
* A concrete implementation of the VersionedTransformer interface.
*/
export class DocumentMigrator implements VersionedTransformer {
private documentMigratorOptions: Omit<DocumentMigratorOptions, 'minimumConvertVersion'>;
private documentMigratorOptions: Omit<DocumentMigratorOptions, 'convertVersion'>;
private migrations?: ActiveMigrations;
private transformDoc?: ApplyTransformsFn;

Expand All @@ -152,17 +150,12 @@ export class DocumentMigrator implements VersionedTransformer {
* @param {DocumentMigratorOptions} opts
* @prop {string} kibanaVersion - The current version of Kibana
* @prop {SavedObjectTypeRegistry} typeRegistry - The type registry to get type migrations from
* @prop {string} minimumConvertVersion - The minimum version of Kibana in which documents can be converted to multi-namespace types
* @prop {string} convertVersion - The version of Kibana in which documents can be converted to multi-namespace types
* @prop {Logger} log - The migration logger
* @memberof DocumentMigrator
*/
constructor({
typeRegistry,
kibanaVersion,
minimumConvertVersion = DEFAULT_MINIMUM_CONVERT_VERSION,
log,
}: DocumentMigratorOptions) {
validateMigrationDefinition(typeRegistry, kibanaVersion, minimumConvertVersion);
constructor({ typeRegistry, kibanaVersion, convertVersion, log }: DocumentMigratorOptions) {
validateMigrationDefinition(typeRegistry, kibanaVersion, convertVersion);

this.documentMigratorOptions = { typeRegistry, kibanaVersion, log };
}
Expand Down Expand Up @@ -300,7 +293,7 @@ function validateMigrationsMapObject(
function validateMigrationDefinition(
registry: ISavedObjectTypeRegistry,
kibanaVersion: string,
minimumConvertVersion: string
convertVersion?: string
) {
function assertObjectOrFunction(entity: any, prefix: string) {
if (!entity || (typeof entity !== 'function' && typeof entity !== 'object')) {
Expand All @@ -321,9 +314,9 @@ function validateMigrationDefinition(
throw new Error(
`Invalid convertToMultiNamespaceTypeVersion for type ${type}. Expected value to be a semver, but got '${convertToMultiNamespaceTypeVersion}'.`
);
} else if (Semver.lt(convertToMultiNamespaceTypeVersion, minimumConvertVersion)) {
} else if (convertVersion && Semver.neq(convertToMultiNamespaceTypeVersion, convertVersion)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut feeling would have been to keep 8.0.0 as the minimum version and introduces a new maximumConvertVersion that we would set either to 8.0.0 or the current version like 8.6.0. But I'm probably overthinking this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could we useful if in the future we wish to unfreeze the functionality.
But at that point, we will probably have to rollback future changes in migration logic as well, otherwise we would have unexpected results.

throw new Error(
`Invalid convertToMultiNamespaceTypeVersion for type ${type}. Value '${convertToMultiNamespaceTypeVersion}' cannot be less than '${minimumConvertVersion}'.`
`Invalid convertToMultiNamespaceTypeVersion for type ${type}. Value '${convertToMultiNamespaceTypeVersion}' cannot be any other than '${convertVersion}'.`
);
} else if (Semver.gt(convertToMultiNamespaceTypeVersion, kibanaVersion)) {
throw new Error(
Expand All @@ -345,6 +338,7 @@ function validateMigrationDefinition(
);
}
if (convertToMultiNamespaceTypeVersion) {
// CHECKPOINT 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Checkpoint!

assertValidConvertToMultiNamespaceType(
namespaceType,
convertToMultiNamespaceTypeVersion,
Expand Down
5 changes: 5 additions & 0 deletions ...es/core/saved-objects/core-saved-objects-migration-server-internal/src/kibana_migrator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ import { createIndexMap } from './core/build_index_map';
import { runResilientMigrator } from './run_resilient_migrator';
import { migrateRawDocsSafely } from './core/migrate_raw_docs';

// ensure plugins don't try to convert SO namespaceTypes after 8.0.0
// see https://github.com/elastic/kibana/issues/147344
const ALLOWED_CONVERT_VERSION = '8.0.0';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to @rudolf, but are we sure we don't want to allow type owners to migrate to shareable until some versions to come? This PR basically hard block any type from being converted as soon as merged. I want to make sure we're fine with the consequences.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I understood correctly, this is a pre-requisite to only migrate an index if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I just saw #147198, but it is converting from "agnostic".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2cents are: as claimed in #147344, if we are not reindexing when there are no mapping updates (or, at least, no breaking changes), we cannot support any further version unless we change the current logic to identify that we need reindex anyway because a conversion must happen.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah if someone REALLY REALLY REALLY needs this we'll need to re-enable and detect this to ensure we reindex (which again will introduce downtime).

Re #147198 this is sort of a unique case because it's a new saved object type, there aren't any files saved objects in users clusters.


export interface KibanaMigratorOptions {
client: ElasticsearchClient;
typeRegistry: ISavedObjectTypeRegistry;
Expand Down Expand Up @@ -92,6 +96,7 @@ export class KibanaMigrator implements IKibanaMigrator {
this.kibanaVersion = kibanaVersion;
this.documentMigrator = new DocumentMigrator({
kibanaVersion: this.kibanaVersion,
convertVersion: ALLOWED_CONVERT_VERSION,
typeRegistry,
log: this.log,
});
Expand Down
1 change: 1 addition & 0 deletions packages/core/saved-objects/core-saved-objects-server/src/migration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export interface SavedObjectMigrationContext {
readonly migrationVersion: string;
/**
* The version in which this object type is being converted to a multi-namespace type
* @deprecated Converting to multi-namespace clashes with the ZDT requirement for serverless
*/
readonly convertToMultiNamespaceTypeVersion?: string;
/**
Expand Down
1 change: 1 addition & 0 deletions packages/core/saved-objects/core-saved-objects-server/src/saved_objects_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export interface SavedObjectsType<Attributes = any> {
* ```
*
* Note: migration function(s) can be optionally specified for any of these versions and will not interfere with the conversion process.
* @deprecated Converting to multi-namespace clashes with the ZDT requirement for serverless
*/
convertToMultiNamespaceTypeVersion?: string;
/**
Expand Down