-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Changes from all commits
1943e3a
b3d6019
b8820a0
110187e
ec1a4ab
d72d774
41f56b3
250ff7c
72cdb3d
6f02890
1f8b861
39b07f7
a793695
06f2138
4038539
c354dc7
f62f9ae
b467cfa
2217872
89ab46e
0ea85e0
801189a
10b8fac
96b1315
26a15e5
a79485d
318fa14
4fdb39d
502c042
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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< | ||
Array<{ status: string }> | ||
> { | ||
if (this.migrationResult === undefined || rerun) { | ||
this.migrationResult = this.runMigrationsInternal(); | ||
} | ||
Comment on lines
-98
to
113
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 As this API is internal I though current implementation was acceptable. Tell me if we should fireproof it more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
|
||
|
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yea, that was it.
+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, '/internal/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, '/internal/saved_objects/_migrate') | ||
.send({}) | ||
.expect(200); | ||
|
||
expect(migratorInstanceMock.runMigrations).toHaveBeenCalledTimes(1); | ||
|
||
await kbnTestServer.request | ||
.post(root, '/internal/saved_objects/_migrate') | ||
.send({}) | ||
.expect(200); | ||
|
||
expect(migratorInstanceMock.runMigrations).toHaveBeenCalledTimes(2); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/* | ||
* 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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There were no There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
validate: false, | ||
options: { | ||
tags: ['access:migrateSavedObjects'], | ||
}, | ||
}, | ||
router.handleLegacyErrors(async (context, req, res) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If so, then yeah, it's fine as it's right now. |
||
const migrator = await migratorPromise; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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?
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added integration tests on the endpoint to ensure it properly calls There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is Platform going to promote any naming convention to clearly distinguish public (with BWC guarantees and documentation) and internal (no BWC guarantees, implicit 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.... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyone against There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
By the way, does it mean that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nevermind, did not understand the |
||
await migrator.runMigrations({ rerun: true }); | ||
return res.ok({ | ||
body: { | ||
success: true, | ||
}, | ||
}); | ||
}) | ||
); | ||
}; |
There was a problem hiding this comment.
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?