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.

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

Adapt es-archiver to call _migrate endpoint instead of creating a migrator #56971

Merged
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
1943e3a
es-archiver call _migrate endpoint instead of creating a migrator
pgayvallet Feb 6, 2020
b3d6019
fix crlf....
pgayvallet Feb 6, 2020
b8820a0
use promise instead of callback accessor
pgayvallet Feb 6, 2020
110187e
attempt with disabled authent
pgayvallet Feb 6, 2020
ec1a4ab
enable security again
pgayvallet Feb 6, 2020
d72d774
set mapping to dynamic before calling migration endpoint
pgayvallet Feb 7, 2020
41f56b3
rename space data from non-spaced tests
pgayvallet Feb 7, 2020
250ff7c
Merge remote-tracking branch 'upstream/master' into kbn-55860-es-arch…
pgayvallet Feb 10, 2020
72cdb3d
Merge remote-tracking branch 'upstream/master' into kbn-55860-es-arch…
pgayvallet Feb 11, 2020
6f02890
add documentation on the `rerun` flag
pgayvallet Feb 11, 2020
1f8b861
create router with the `/api/saved_objects` prefix
pgayvallet Feb 11, 2020
39b07f7
add unit test about strict mapping
pgayvallet Feb 11, 2020
a793695
add integration test on migrate endpoint
pgayvallet Feb 11, 2020
06f2138
wrap route handler with handleLegacyErrors
pgayvallet Feb 12, 2020
4038539
Merge remote-tracking branch 'upstream/master' into kbn-55860-es-arch…
pgayvallet Feb 13, 2020
c354dc7
add remark about limitations of the rerun flag
pgayvallet Feb 13, 2020
f62f9ae
Merge remote-tracking branch 'upstream/master' into kbn-55860-es-arch…
pgayvallet Feb 13, 2020
b467cfa
Merge remote-tracking branch 'upstream/master' into kbn-55860-es-arch…
pgayvallet Feb 14, 2020
2217872
Merge branch 'master' into kbn-55860-es-archiver-call-migrate-endpoint
elasticmachine Feb 17, 2020
89ab46e
Apply suggestions from code review
joshdover Feb 18, 2020
0ea85e0
Merge remote-tracking branch 'upstream/master' into kbn-55860-es-arch…
pgayvallet Feb 24, 2020
801189a
do not return detailed index result
pgayvallet Feb 24, 2020
10b8fac
Merge remote-tracking branch 'refs/remotes/origin/kbn-55860-es-archiv…
pgayvallet Feb 24, 2020
96b1315
add access tag to migrate route
pgayvallet Feb 24, 2020
26a15e5
Merge remote-tracking branch 'upstream/master' into kbn-55860-es-arch…
pgayvallet Feb 24, 2020
a79485d
Merge remote-tracking branch 'upstream/master' into kbn-55860-es-arch…
pgayvallet Feb 25, 2020
318fa14
use /internal prefix for migrate endpoint
pgayvallet Feb 25, 2020
4fdb39d
fix integ tests
pgayvallet Feb 25, 2020
502c042
Merge remote-tracking branch 'upstream/master' into kbn-55860-es-arch…
pgayvallet Feb 26, 2020
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
19 changes: 19 additions & 0 deletions packages/kbn-dev-utils/src/kbn_client/kbn_client_saved_objects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,28 @@ interface UpdateOptions<Attributes> extends IndexOptions<Attributes> {
id: string;
}

interface MigrateResponse {
success: boolean;
result: Array<{ status: string }>;
}

export class KbnClientSavedObjects {
constructor(private readonly log: ToolingLog, private readonly requester: KbnClientRequester) {}

/**
* Run the saved objects migration
*/
public async migrate() {
this.log.debug('Migrating saved objects');

return await this.requester.request<MigrateResponse>({
description: 'migrate saved objects',
path: uriencode`/api/saved_objects/_migrate`,
joshdover marked this conversation as resolved.
Show resolved Hide resolved
method: 'POST',
body: {},
});
}

/**
* Get an object
*/
Expand Down
12 changes: 12 additions & 0 deletions packages/kbn-pm/dist/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -43777,6 +43777,18 @@ class KbnClientSavedObjects {
this.log = log;
this.requester = requester;
}
/**
* Run the saved objects migration
*/
async migrate() {
this.log.debug('Migrating saved objects');
return await this.requester.request({
description: 'migrate saved objects',
path: kbn_client_requester_1.uriencode `/api/saved_objects/_migrate`,
method: 'POST',
body: {},
});
}
/**
* Get an object
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ import { IndexMapping } from './../../mappings';
import { buildActiveMappings, diffMappings } from './build_active_mappings';

describe('buildActiveMappings', () => {
test('creates a strict mapping', () => {
const mappings = buildActiveMappings({});
expect(mappings.dynamic).toEqual('strict');
});

test('combines all mappings and includes core mappings', () => {
const properties = {
aaa: { type: 'text' },
Expand Down
18 changes: 15 additions & 3 deletions src/core/server/saved_objects/migrations/kibana/kibana_migrator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,15 +88,27 @@ export class KibanaMigrator {
}

/**
* Migrates the mappings and documents in the Kibana index. This will run only
* Migrates the mappings and documents in the Kibana index. By default, this will run only
* once and subsequent calls will return the result of the original call.
*
* @param rerun - If true, method will run a new migration when called again instead of
* returning the result of the initial migration. This should only be used when factors external
* to Kibana itself alter the kibana index causing the saved objects mappings or data to change
* after the Kibana server performed the initial migration.
*
* @remarks When the `rerun` parameter is set to true, no checks are performed to ensure that no migration
* is currently running. Chained or concurrent calls to `runMigrations({ rerun: true })` can lead to
* multiple migrations running at the same time. When calling with this parameter, it's expected that the calling
* code should ensure that the initial call resolves before calling the function again.
*
* @returns - A promise which resolves once all migrations have been applied.
* The promise resolves with an array of migration statuses, one for each
* elasticsearch index which was migrated.
*/
public runMigrations(): Promise<Array<{ status: string }>> {
if (this.migrationResult === undefined) {
public runMigrations({ rerun = false }: { rerun?: boolean } = {}): Promise<
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a @param to the comment to explain what this option does / how / when it should be used?

Array<{ status: string }>
> {
if (this.migrationResult === undefined || rerun) {
this.migrationResult = this.runMigrationsInternal();
}
Comment on lines -98 to 113
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is very naive: if a migration is already running and we call runMigration again with return=true, multiple migrations can occur at the same time.

We should maybe wait for the pending migration to finish before triggering the next one. But that adds some complex logic to handle edge cases such as multiple rerun=true calls when a migration is already running. We should wait for the migration to finish but only re-execute ONCE and return the same promise for every calls to rerun that occurred during the initial migration.

As this API is internal I though current implementation was acceptable. Tell me if we should fireproof it more.

Copy link
Contributor

Choose a reason for hiding this comment

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

so, in theory, it could lead to a race condition if 2 migrations run over the same data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory it could yes, even if I don't think it could leads to data corruption, as in worse case, both 'threads' will access the data before any actually migrated it, and the document would be migrated then updated 'twice' from the same original data, which would only impact performances.

I can try to improve that by 'pooling' all the calls to runMigration(rerun=true) while one migration is running and only execute the rerun once, as explained in previous comment, if we think current implementation is too fragile.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's okay as long as this API is used only for tests internally. Could you add a comment about this known limitation?


Expand Down
33 changes: 33 additions & 0 deletions src/core/server/saved_objects/routes/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { InternalHttpServiceSetup } from '../../http';
import { IKibanaMigrator } from '../migrations';
import { registerMigrateRoute } from './migrate';

interface RegisterRouteOptions {
http: InternalHttpServiceSetup;
migratorPromise: Promise<IKibanaMigrator>;
}

export function registerRoutes({ http, migratorPromise }: RegisterRouteOptions) {
const router = http.createRouter('/api/saved_objects');

registerMigrateRoute(router, migratorPromise);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { mockKibanaMigrator } from '../../migrations/kibana/kibana_migrator.mock';

export const migratorInstanceMock = mockKibanaMigrator.create();
export const KibanaMigratorMock = jest.fn().mockImplementation(() => migratorInstanceMock);
jest.doMock('../../migrations/kibana/kibana_migrator', () => ({
KibanaMigrator: KibanaMigratorMock,
}));
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { migratorInstanceMock } from './migrate.test.mocks';
import * as kbnTestServer from '../../../../../test_utils/kbn_server';

describe('SavedObjects /_migrate endpoint', () => {
let root: ReturnType<typeof kbnTestServer.createRoot>;

beforeEach(async () => {
root = kbnTestServer.createRoot({ migrations: { skip: true } });
await root.setup();
await root.start();
migratorInstanceMock.runMigrations.mockClear();
}, 30000);
Copy link
Member

Choose a reason for hiding this comment

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

question: do we really need this explicit timeout? AFAIK we set large enough global timeout for all integration jest tests unless something has changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably it was added not to wait for 10 minutes to fail the test.

Copy link
Member

@azasypkin azasypkin Feb 17, 2020

Choose a reason for hiding this comment

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

🤷‍♂️ if we think Kibana startup and CI are supposed to be faster now we can decrease global timeout and set it to something more reasonable once and for all (out of scope of this PR, just thinking aloud).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably it was added not to wait for 10 minutes to fail the test.

Yea, that was it.

if we think Kibana startup and CI are supposed to be faster now we can decrease global timeout and set it to something more reasonable once and for all

+1, but changing the global timeout is not always a pleasant surprise in term of results, this is way outside of the scope of this PR.


afterEach(async () => {
await root.shutdown();
});

it('calls runMigrations on the migrator with rerun=true when accessed', async () => {
await kbnTestServer.request
.post(root, '/api/saved_objects/_migrate')
.send({})
.expect(200);

expect(migratorInstanceMock.runMigrations).toHaveBeenCalledTimes(1);
expect(migratorInstanceMock.runMigrations).toHaveBeenCalledWith({ rerun: true });
});

it('calls runMigrations multiple time when multiple access', async () => {
await kbnTestServer.request
.post(root, '/api/saved_objects/_migrate')
.send({})
.expect(200);

expect(migratorInstanceMock.runMigrations).toHaveBeenCalledTimes(1);

await kbnTestServer.request
.post(root, '/api/saved_objects/_migrate')
.send({})
.expect(200);

expect(migratorInstanceMock.runMigrations).toHaveBeenCalledTimes(2);
});
});
43 changes: 43 additions & 0 deletions src/core/server/saved_objects/routes/migrate.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { IRouter } from '../../http';
import { IKibanaMigrator } from '../migrations';

export const registerMigrateRoute = (
router: IRouter,
migratorPromise: Promise<IKibanaMigrator>
) => {
router.post(
{
path: '/_migrate',
Copy link
Member

Choose a reason for hiding this comment

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

question: I'm wondering if it'd make sense to at least tie this to savedObjectsManagement:all privilege and use something like access:migrateSavedObjects tag here.

The only thing is that I can't recall a case when we did something like this for internal APIs. @kobelb what do you think? Am I missing something and there is a better way to manage access to internal APIs that perform actions on behalf of internal Kibana user (SO migration)?

Copy link
Contributor

@kobelb kobelb Feb 19, 2020

Choose a reason for hiding this comment

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

If we're only using this endpoint internally for the functional test runner and don't intend for end-users in production to use this endpoint, I wouldn't tie this to the savedObjectsManagement:all privilege. End-users who get all access to the saved-object management feature would then be able to use this.

This route is rather different than our other "internal" routes. Our other internal routes are intended to be used from the browser, but end-users are authorized to access them. We don't really want this route to be used in production at all. We definitely can should use the access: tag on the route to restrict which users should have access to this route. However, we really only want it to be used by the functional test runner, which I don't believe has a real user to represent. If we used an access:migrateSavedObjects tag here, and then didn't add it to any privileges only a superuser would be able to invoke this API, as they're authorized to do everything.

Copy link
Contributor

@kobelb kobelb Feb 20, 2020

Choose a reason for hiding this comment

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

The other stipulation is that relying on the security plugin's authorization won't apply when the user hasn't enabled security, which is the default. What are the repercussions of any user being able to start migrations in these scenarios?

Copy link
Contributor

@joshdover joshdover Feb 20, 2020

Choose a reason for hiding this comment

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

I'm trying to come up with a bad scenario and coming up empty. Migrations only get run on objects from older versions, which the only way you could create objects from older versions is manually via direct Elasticsearch API calls, which only superusers and kibana_system should have access to do.

Adding access:migrateSavedObjects tag makes sense to me as an extra layer of precaution, but I don't think we need to do anything extra for the no-security scenario. Does that work for y'all?

Copy link
Member

@azasypkin azasypkin Feb 21, 2020

Choose a reason for hiding this comment

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

Having just access:migrateSavedObjects sounds like a reasonable trade-off to me assuming it works with esArchiver and there is no reliable way no detect "FTR" scenario using env.packageInfo.dist/buildNum (to not register this endpoint at all otherwise or register it via custom test plugin or something along these lines).

Copy link
Contributor

Choose a reason for hiding this comment

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

@kobelb was that intentional decision for some reason or we just didn't have any /internal routes that would require authorization check at that time.

There were no /internal routes when this was written, and we decided to only run that route middleware when the url began with /api. We can likely remove that restriction entirely, as we're only performing the authorization logic on route which have the access: tag. I created #58351 to see the impact this has. @legrego do you recall any reason why we included the restriction that the route must start with /api originally?

Copy link
Member

Choose a reason for hiding this comment

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

@kobelb nope I don’t think we had a great reason, beyond what you already mentioned that we didn’t have any /internal routes at that time. I don’t see a problem removing this restriction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed the commit adding the access tag to the route, to see if that doesn't introduce another 'surprise' from the FTR call. Waiting for #58351 to change the endpoint from api to internal.

Copy link
Contributor

Choose a reason for hiding this comment

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

CI just passed on #58351, so I think we should be good, just waiting on a review at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see #58351 has been merged. Thanks a lot to the whole security team 😄 Will move the endpoint to the internal prefix tomorrow, and we should be good to go.

validate: false,
},
router.handleLegacyErrors(async (context, req, res) => {
Copy link
Member

Choose a reason for hiding this comment

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

question: how unexpected migration errors look like (5xx)? May they include something that we don't want consumer to see? If so, we may need to try/catch errors in the handler, log them and return something more opaque.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mostly here to catch the elasticsearch service errors, that are currently thrown as boom errors. These errors are already thrown as-is from other SO endpoints, so I think there is no risk here, but I can try/catch and return a generic error instead if we think this is better.

Copy link
Member

Choose a reason for hiding this comment

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

These errors are already thrown as-is from other SO endpoints, so I think there is no risk here

If so, then yeah, it's fine as it's right now.

const migrator = await migratorPromise;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we log a warning if this endpoint is used in production env? (and maybe if the CI env var isn't set?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also there aren't any tests on this endpoint / logic. I know this is an internal API, but it still feels like we should test that we don't break this by accident. Though CI would probably fail if this broke, it would be much better if we had more insight into what broke to cause the failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we log a warning if this endpoint is used in production env? (and maybe if the CI env var isn't set?)

We are running the FTR suites against production cloud, so I'm not sure we should be logging a warning? At least we should not check the CI var env?

Also there aren't any tests on this endpoint / logic

Yea, I know. I can add basic unit test to check that 1/ the route is registered and 2/ calling the route properly calls the migrator. However I have no idea how to go further than that, as the API call is actually testing in the FTR. We can't really functional test a component used by the FTR framework itself, can we? Any insight on how to test that on a 'higher' level that unit is welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added integration tests on the endpoint to ensure it properly calls migrator.runMigrations with the correct parameters

Copy link
Member

Choose a reason for hiding this comment

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

I know this is an internal API

Is Platform going to promote any naming convention to clearly distinguish public (with BWC guarantees and documentation) and internal (no BWC guarantees, implicit use at your own risk warning and optional documentation) API endpoints? In Spaces/Security we started to define internal routes with /internal/ prefix instead of /api/ as an experiment that has been working well enough so far.

I'm a bit concerned that users will quickly discover this endpoint and may start using it in any imaginable context without realizing all the consequences leading to hard-to-handle/diagnose support cases....

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyone against /internal/saved_objects/_migrate?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to introduce another convention without a broad discussion. as of me, internal could work, yes.

Copy link
Member

Choose a reason for hiding this comment

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

We are running the FTR suites against production cloud

By the way, does it mean that env.packageInfo (dist, buildNum etc.) in this case is completely indistinguishable from the one we have when Kibana is run in production outside of functional tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/internal/saved_objects/_migrate kinda breaks the SO route prefix. This is very minor, but forces to use a / based router when declaring routes instead of a /saved_objects one. Would /saved_objects/internal/_migrate be acceptable, or do we really want the route to starts with /internal ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nevermind, did not understand the internal prefix was a replacement for the api one. Please forget my previous question

const result = await migrator.runMigrations({ rerun: true });
Copy link
Member

Choose a reason for hiding this comment

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

question: is result going to be consumed by the client somehow? If not, maybe it'd be safer to not return it at all for now (ideal option, we can log though) or add type annotation here (so that TS complain if signature changes) or just pick specific fields in case migrator starts returning something more sensitive? I know it sounds a bit paranoid, but you never know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not atm. I can just return a {status: 'ok'} dummy response instead

return res.ok({
body: {
success: true,
result,
},
});
})
);
};
2 changes: 2 additions & 0 deletions src/core/server/saved_objects/saved_objects_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { Env } from '../config';
import { configServiceMock } from '../mocks';
import { elasticsearchServiceMock } from '../elasticsearch/elasticsearch_service.mock';
import { legacyServiceMock } from '../legacy/legacy_service.mock';
import { httpServiceMock } from '../http/http_service.mock';
import { SavedObjectsClientFactoryProvider } from './service/lib';
import { BehaviorSubject } from 'rxjs';
import { NodesVersionCompatibility } from '../elasticsearch/version_check/ensure_es_version';
Expand All @@ -39,6 +40,7 @@ describe('SavedObjectsService', () => {
const createSetupDeps = () => {
const elasticsearchMock = elasticsearchServiceMock.createInternalSetup();
return {
http: httpServiceMock.createSetupContract(),
elasticsearch: elasticsearchMock,
legacyPlugins: legacyServiceMock.createDiscoverPlugins(),
};
Expand Down
15 changes: 13 additions & 2 deletions src/core/server/saved_objects/saved_objects_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@
* under the License.
*/

import { CoreService } from 'src/core/types';
import { Subject } from 'rxjs';
import { first, filter, take } from 'rxjs/operators';
import { CoreService } from '../../types';
import {
SavedObjectsClient,
SavedObjectsClientProvider,
Expand All @@ -32,7 +33,7 @@ import { InternalElasticsearchServiceSetup, APICaller } from '../elasticsearch';
import { KibanaConfigType } from '../kibana_config';
import { migrationsRetryCallCluster } from '../elasticsearch/retry_call_cluster';
import { SavedObjectsConfigType } from './saved_objects_config';
import { KibanaRequest } from '../http';
import { InternalHttpServiceSetup, KibanaRequest } from '../http';
import { SavedObjectsClientContract, SavedObjectsType } from './types';
import { ISavedObjectsRepository, SavedObjectsRepository } from './service/lib/repository';
import {
Expand All @@ -44,6 +45,7 @@ import { convertLegacyTypes } from './utils';
import { SavedObjectTypeRegistry, ISavedObjectTypeRegistry } from './saved_objects_type_registry';
import { PropertyValidators } from './validation';
import { SavedObjectsSerializer } from './serialization';
import { registerRoutes } from './routes';

/**
* Saved Objects is Kibana's data persistence mechanism allowing plugins to
Expand Down Expand Up @@ -196,6 +198,7 @@ export interface SavedObjectsRepositoryFactory {

/** @internal */
export interface SavedObjectsSetupDeps {
http: InternalHttpServiceSetup;
legacyPlugins: LegacyServiceDiscoverPlugins;
elasticsearch: InternalElasticsearchServiceSetup;
}
Expand All @@ -218,6 +221,7 @@ export class SavedObjectsService
private clientFactoryProvider?: SavedObjectsClientFactoryProvider;
private clientFactoryWrappers: WrappedClientFactoryWrapper[] = [];

private migrator$ = new Subject<KibanaMigrator>();
private typeRegistry = new SavedObjectTypeRegistry();
private validations: PropertyValidators = {};

Expand All @@ -237,6 +241,11 @@ export class SavedObjectsService
legacyTypes.forEach(type => this.typeRegistry.registerType(type));
this.validations = setupDeps.legacyPlugins.uiExports.savedObjectValidations || {};

registerRoutes({
http: setupDeps.http,
migratorPromise: this.migrator$.pipe(first()).toPromise(),
});
Copy link
Contributor Author

@pgayvallet pgayvallet Feb 7, 2020

Choose a reason for hiding this comment

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

Registration is done during setup, the migrator is not yet created. Had to use a subject/promise as workaround.


return {
setClientFactoryProvider: provider => {
if (this.clientFactoryProvider) {
Expand Down Expand Up @@ -278,6 +287,8 @@ export class SavedObjectsService
const adminClient = this.setupDeps!.elasticsearch.adminClient;
const migrator = this.createMigrator(kibanaConfig, savedObjectsConfig, migrationsRetryDelay);

this.migrator$.next(migrator);

/**
* Note: We want to ensure that migrations have completed before
* continuing with further Core start steps that might use SavedObjects
Expand Down
1 change: 1 addition & 0 deletions src/core/server/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ export class Server {
});

const savedObjectsSetup = await this.savedObjects.setup({
http: httpSetup,
elasticsearch: elasticsearchServiceSetup,
legacyPlugins,
});
Expand Down
3 changes: 1 addition & 2 deletions src/es_archiver/actions/empty_kibana_index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,8 @@ export async function emptyKibanaIndexAction({
kbnClient: KbnClient;
}) {
const stats = createStats('emptyKibanaIndex', log);
const kibanaPluginIds = await kbnClient.plugins.getEnabledIds();

await deleteKibanaIndices({ client, stats, log });
await migrateKibanaIndex({ client, log, kibanaPluginIds });
await migrateKibanaIndex({ client, kbnClient });
return stats;
}
2 changes: 1 addition & 1 deletion src/es_archiver/actions/load.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export async function loadAction({

// If we affected the Kibana index, we need to ensure it's migrated...
if (Object.keys(result).some(k => k.startsWith('.kibana'))) {
await migrateKibanaIndex({ client, log, kibanaPluginIds });
await migrateKibanaIndex({ client, kbnClient });

if (kibanaPluginIds.includes('spaces')) {
await createDefaultSpace({ client, index: '.kibana' });
Expand Down
Loading