-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Config Service] Expose serverless
contextRef
#156837
Changes from all commits
ea011c9
de65297
d4e619a
859e3e1
855b67a
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 |
---|---|---|
|
@@ -154,7 +154,8 @@ export const configSchema = schema.object({ | |
return '"ignoreVersionMismatch" can only be set to true in development mode'; | ||
} | ||
}, | ||
defaultValue: false, | ||
// When running in serverless mode, default to `true` | ||
defaultValue: schema.contextRef('serverless'), | ||
Comment on lines
+157
to
+158
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 thought about nesting another conditional but found this solution a bit cleaner. Happy to iterate if you think it's best to have the explicit conditional. 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 fine as long as we're absolutely certain there aren't any other valid use cases where the version mismatch doesn't matter. If we're not certain, then a nested conditional is safer. 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. Yeah, fine for now ihmo, we can change later if needed. |
||
}), | ||
schema.boolean({ defaultValue: false }) | ||
), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,8 +97,7 @@ export class ElasticsearchService | |
const esNodesCompatibility$ = pollEsNodesVersion({ | ||
internalClient: this.client.asInternalUser, | ||
log: this.log, | ||
ignoreVersionMismatch: | ||
config.ignoreVersionMismatch || this.coreContext.env.cliArgs.serverless === true, | ||
ignoreVersionMismatch: config.ignoreVersionMismatch, | ||
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. Back to not checking explicitly for serverless in this part of the code... 💆 |
||
esVersionCheckInterval: config.healthCheckDelay.asMilliseconds(), | ||
kibanaVersion: this.kibanaVersion, | ||
}).pipe(takeUntil(this.stop$), shareReplay({ refCount: true, bufferSize: 1 })); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -197,7 +197,7 @@ export interface TestKibanaUtils { | |
|
||
export interface TestUtils { | ||
startES: () => Promise<TestElasticsearchUtils>; | ||
startKibana: () => Promise<TestKibanaUtils>; | ||
startKibana: (abortSignal?: AbortSignal) => Promise<TestKibanaUtils>; | ||
} | ||
|
||
/** | ||
|
@@ -261,7 +261,7 @@ export function createTestServers({ | |
// Add time for KBN and adding users | ||
adjustTimeout(es.getStartTimeout() + 100000); | ||
|
||
const kbnSettings = settings.kbn ?? {}; | ||
const { cliArgs = {}, customKibanaVersion, ...kbnSettings } = settings.kbn ?? {}; | ||
|
||
return { | ||
startES: async () => { | ||
|
@@ -284,8 +284,10 @@ export function createTestServers({ | |
password: kibanaServerTestUser.password, | ||
}; | ||
}, | ||
startKibana: async () => { | ||
const root = createRootWithCorePlugins(kbnSettings); | ||
startKibana: async (abortSignal?: AbortSignal) => { | ||
const root = createRootWithCorePlugins(kbnSettings, cliArgs, customKibanaVersion); | ||
|
||
abortSignal?.addEventListener('abort', async () => await root.shutdown()); | ||
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 needed this so I could stop Kibana when left hanging waiting for a valid ES connection |
||
|
||
await root.preboot(); | ||
const coreSetup = await root.setup(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -241,6 +241,7 @@ export class ConfigService { | |
{ | ||
dev: this.env.mode.dev, | ||
prod: this.env.mode.prod, | ||
serverless: this.env.cliArgs.serverless === true, | ||
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 optional, so I thought it's safer to validate it's |
||
...this.env.packageInfo, | ||
}, | ||
`config validation of [${namespace}]` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,7 +68,6 @@ function createFakeElasticsearchServer() { | |
return server; | ||
} | ||
|
||
// FLAKY: https://github.com/elastic/kibana/issues/129754 | ||
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. left over: no longer flaky |
||
describe('fake elasticsearch', () => { | ||
let esServer: http.Server; | ||
let kibanaServer: Root; | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,135 @@ | ||||||||||||||||||||||||
/* | ||||||||||||||||||||||||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||||||||||||||||||||||||
* or more contributor license agreements. Licensed under the Elastic License | ||||||||||||||||||||||||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||||||||||||||||||||||||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||||||||||||||||||||||||
* Side Public License, v 1. | ||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
import { | ||||||||||||||||||||||||
createTestServers, | ||||||||||||||||||||||||
type TestElasticsearchUtils, | ||||||||||||||||||||||||
type TestKibanaUtils, | ||||||||||||||||||||||||
} from '@kbn/core-test-helpers-kbn-server'; | ||||||||||||||||||||||||
import { esTestConfig } from '@kbn/test'; | ||||||||||||||||||||||||
import { firstValueFrom, Subject } from 'rxjs'; | ||||||||||||||||||||||||
import { CliArgs } from '@kbn/config'; | ||||||||||||||||||||||||
import Semver from 'semver'; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
function nextMinor() { | ||||||||||||||||||||||||
return Semver.inc(esTestConfig.getVersion(), 'minor') || '10.0.0'; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
function previousMinor() { | ||||||||||||||||||||||||
const [major, minor] = esTestConfig | ||||||||||||||||||||||||
.getVersion() | ||||||||||||||||||||||||
.split('.') | ||||||||||||||||||||||||
.map((s) => parseInt(s, 10)); | ||||||||||||||||||||||||
// We should be fine for now. When we jump to the next major, we'll need to handle that. | ||||||||||||||||||||||||
return `${major}.${minor - 1}.0`; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
describe('Version Compatibility', () => { | ||||||||||||||||||||||||
let esServer: TestElasticsearchUtils | undefined; | ||||||||||||||||||||||||
let kibanaServer: TestKibanaUtils | undefined; | ||||||||||||||||||||||||
let abortController: AbortController | undefined; | ||||||||||||||||||||||||
let consoleSpy: jest.SpyInstance; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
beforeEach(() => { | ||||||||||||||||||||||||
consoleSpy = jest.spyOn(console, 'log'); | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
afterEach(async () => { | ||||||||||||||||||||||||
consoleSpy.mockRestore(); | ||||||||||||||||||||||||
if (kibanaServer) { | ||||||||||||||||||||||||
await kibanaServer.stop(); | ||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||
abortController?.abort(); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
if (esServer) { | ||||||||||||||||||||||||
await esServer.stop(); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
kibanaServer = undefined; | ||||||||||||||||||||||||
abortController = undefined; | ||||||||||||||||||||||||
esServer = undefined; | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const startServers = async ({ | ||||||||||||||||||||||||
cliArgs, | ||||||||||||||||||||||||
customKibanaVersion, | ||||||||||||||||||||||||
ignoreVersionMismatch, | ||||||||||||||||||||||||
}: { | ||||||||||||||||||||||||
cliArgs?: Partial<CliArgs>; | ||||||||||||||||||||||||
customKibanaVersion?: string; | ||||||||||||||||||||||||
ignoreVersionMismatch?: boolean; | ||||||||||||||||||||||||
} = {}) => { | ||||||||||||||||||||||||
const { startES, startKibana } = createTestServers({ | ||||||||||||||||||||||||
adjustTimeout: jest.setTimeout, | ||||||||||||||||||||||||
settings: { | ||||||||||||||||||||||||
kbn: { | ||||||||||||||||||||||||
cliArgs, | ||||||||||||||||||||||||
customKibanaVersion, | ||||||||||||||||||||||||
elasticsearch: { | ||||||||||||||||||||||||
ignoreVersionMismatch, | ||||||||||||||||||||||||
}, | ||||||||||||||||||||||||
}, | ||||||||||||||||||||||||
}, | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
esServer = await startES(); | ||||||||||||||||||||||||
abortController = new AbortController(); | ||||||||||||||||||||||||
kibanaServer = await startKibana(abortController.signal); | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
it('should start when versions match', async () => { | ||||||||||||||||||||||||
await expect(startServers({})).resolves.toBeUndefined(); | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
it('should start when ES is next minor', async () => { | ||||||||||||||||||||||||
await expect(startServers({ customKibanaVersion: previousMinor() })).resolves.toBeUndefined(); | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
it('should flag the incompatibility on version mismatch (ES is previous minor)', async () => { | ||||||||||||||||||||||||
const found$ = new Subject<void>(); | ||||||||||||||||||||||||
consoleSpy.mockImplementation((str) => { | ||||||||||||||||||||||||
if (str.includes('is incompatible')) { | ||||||||||||||||||||||||
found$.next(); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
await Promise.race([ | ||||||||||||||||||||||||
firstValueFrom(found$), | ||||||||||||||||||||||||
startServers({ customKibanaVersion: nextMinor() }).then(() => { | ||||||||||||||||||||||||
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 test only asserts against version mismatch between x.n and x.n+1. We should test against a mismatch between x.n and x.n-1 too. Can we assume mismatches between current and previous/next patches won't matter? 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.
Kibana "is compatible" with newer minor versions of ES already: Lines 29 to 39 in 3508350
(which is unitary tested in the associated test file), so I'd say it's fine to only test the other scenario? 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 guess it's good to have an integration test for the "ES is next minor" use case to catch any potential regressions in the future. I added it in Thank you for raising it, @TinaHeiligers |
||||||||||||||||||||||||
throw new Error( | ||||||||||||||||||||||||
'Kibana completed the bootstrap without finding the incompatibility message' | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
}), | ||||||||||||||||||||||||
new Promise((resolve, reject) => | ||||||||||||||||||||||||
setTimeout(() => reject(new Error('Test timedout')), 5 * 60 * 1000) | ||||||||||||||||||||||||
), | ||||||||||||||||||||||||
]).finally(() => found$.complete()); | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
it('should ignore the version mismatch when option is set', async () => { | ||||||||||||||||||||||||
await expect( | ||||||||||||||||||||||||
startServers({ | ||||||||||||||||||||||||
customKibanaVersion: nextMinor(), | ||||||||||||||||||||||||
cliArgs: { dev: true }, | ||||||||||||||||||||||||
ignoreVersionMismatch: true, | ||||||||||||||||||||||||
}) | ||||||||||||||||||||||||
).resolves.toBeUndefined(); | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
it('should not allow the option when not in dev mode', async () => { | ||||||||||||||||||||||||
await expect( | ||||||||||||||||||||||||
startServers({ customKibanaVersion: nextMinor(), ignoreVersionMismatch: true }) | ||||||||||||||||||||||||
).rejects.toThrowErrorMatchingInlineSnapshot( | ||||||||||||||||||||||||
`"[config validation of [elasticsearch].ignoreVersionMismatch]: \\"ignoreVersionMismatch\\" can only be set to true in development mode"` | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
it('should ignore version mismatch when running on serverless mode and complete startup', async () => { | ||||||||||||||||||||||||
await expect( | ||||||||||||||||||||||||
startServers({ customKibanaVersion: nextMinor(), cliArgs: { serverless: true } }) | ||||||||||||||||||||||||
).resolves.toBeUndefined(); | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
}); |
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.
I was tempted with
plansForWorldDomination
, but I toned it down a bit 😅