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

Enable Product check from @elastic/elasticsearch-js #107663

Merged
merged 27 commits into from
Aug 26, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
83d0a5c
Test Product check in @elastic/elasticsearch-js
afharo Aug 4, 2021
75e047b
Remove skipProductCheck from @kbn/es. It shouldn't be needed anymore
afharo Aug 13, 2021
c9ccff1
@kbn/es's es_bin must cover the Product check request now
afharo Aug 19, 2021
909d39b
Remove unused logger mock
afharo Aug 19, 2021
c4cd22b
Merge branch 'master' of github.com:elastic/kibana into test-es-clien…
afharo Aug 20, 2021
d737e9e
PR feedback
afharo Aug 20, 2021
410f11f
Fix types
afharo Aug 20, 2021
160b26e
Do not start SO Migrations if Kibana is shutting down
afharo Aug 20, 2021
86ff0f0
Merge branch 'master' of github.com:elastic/kibana into test-es-clien…
afharo Aug 23, 2021
0e777c8
Halt Kibana if Product Check error is found while starting
afharo Aug 23, 2021
1335965
Cleanup unnecessary objects from tests
afharo Aug 23, 2021
c7395b9
Add logger.error test coverage
afharo Aug 23, 2021
9e1df42
Explicitly emit all the errors to test that it only throws on Product…
afharo Aug 23, 2021
e99a786
Emit some random errors but not all to avoid Flaky tests
afharo Aug 23, 2021
fda3996
Update elasticsearch_service tests based on new usage
afharo Aug 23, 2021
039493d
pollEsNodesVersion mocked to observable$ only on the specific logger …
afharo Aug 23, 2021
94e3c26
Merge branch 'master' into test-es-client-product-check-behaviour
kibanamachine Aug 23, 2021
098db80
Merge branch 'master' into test-es-client-product-check-behaviour
kibanamachine Aug 24, 2021
31e6fdc
Add `skipStartupConnectionCheck` config so http integration tests do …
afharo Aug 24, 2021
c34f8aa
API doc changes
afharo Aug 24, 2021
a2c28fe
Update snapshots after config change
afharo Aug 24, 2021
42da9cc
Skip ES validation on legacy/logging tests
afharo Aug 24, 2021
9a4a210
Skip ES connection check in more integration tests that do not spin u…
afharo Aug 25, 2021
d4bb015
Add new `elasticsearch.skipStartupConnectionCheck` setting to kibana-…
afharo Aug 25, 2021
af9bc7d
Better API docs
afharo Aug 25, 2021
36dc8d4
Merge branch 'master' of github.com:elastic/kibana into test-es-clien…
afharo Aug 26, 2021
8f1d6a3
Allow `skipStartupConnectionCheck` only when running from source
afharo Aug 26, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export declare class ElasticsearchConfig
| [requestTimeout](./kibana-plugin-core-server.elasticsearchconfig.requesttimeout.md) | | <code>Duration</code> | Timeout after which HTTP request will be aborted and retried. |
| [serviceAccountToken](./kibana-plugin-core-server.elasticsearchconfig.serviceaccounttoken.md) | | <code>string</code> | If Elasticsearch security features are enabled, this setting provides the service account token that the Kibana server users to perform its administrative functions.<!-- -->This is an alternative to specifying a username and password. |
| [shardTimeout](./kibana-plugin-core-server.elasticsearchconfig.shardtimeout.md) | | <code>Duration</code> | Timeout for Elasticsearch to wait for responses from shards. Set to 0 to disable. |
| [skipStartupConnectionCheck](./kibana-plugin-core-server.elasticsearchconfig.skipstartupconnectioncheck.md) | | <code>boolean</code> | Skip the valid connection check during startup. The connection check allows Kibana to ensure that the Elasticsearch connection is valid before allowing any other services to be set up. |
| [sniffInterval](./kibana-plugin-core-server.elasticsearchconfig.sniffinterval.md) | | <code>false &#124; Duration</code> | Interval to perform a sniff operation and make sure the list of nodes is complete. If <code>false</code> then sniffing is disabled. |
| [sniffOnConnectionFault](./kibana-plugin-core-server.elasticsearchconfig.sniffonconnectionfault.md) | | <code>boolean</code> | Specifies whether the client should immediately sniff for a more current list of nodes when a connection dies. |
| [sniffOnStart](./kibana-plugin-core-server.elasticsearchconfig.sniffonstart.md) | | <code>boolean</code> | Specifies whether the client should attempt to detect the rest of the cluster when it is first instantiated. |
Expand Down

This file was deleted.

30 changes: 30 additions & 0 deletions src/core/server/elasticsearch/elasticsearch_config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -398,3 +398,33 @@ test('serviceAccountToken does not throw if username is not set', () => {

expect(() => config.schema.validate(obj)).not.toThrow();
});

describe('skipStartupConnectionCheck', () => {
test('defaults to `false`', () => {
const obj = {};
expect(() => config.schema.validate(obj)).not.toThrow();
expect(config.schema.validate(obj)).toEqual(
expect.objectContaining({
skipStartupConnectionCheck: false,
})
);
});

test('accepts `false` on both prod and dev mode', () => {
const obj = {
skipStartupConnectionCheck: false,
};
expect(() => config.schema.validate(obj, { dist: false })).not.toThrow();
expect(() => config.schema.validate(obj, { dist: true })).not.toThrow();
});

test('accepts `true` only when running from source to allow integration tests to run without an ES server', () => {
const obj = {
skipStartupConnectionCheck: true,
};
expect(() => config.schema.validate(obj, { dist: false })).not.toThrow();
expect(() => config.schema.validate(obj, { dist: true })).toThrowErrorMatchingInlineSnapshot(
`"[skipStartupConnectionCheck]: \\"skipStartupConnectionCheck\\" can only be set to true when running from source to allow integration tests to run without an ES server"`
);
});
});
19 changes: 17 additions & 2 deletions src/core/server/elasticsearch/elasticsearch_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,21 @@ export const configSchema = schema.object({
}),
schema.boolean({ defaultValue: false })
),
skipStartupConnectionCheck: schema.boolean({ defaultValue: false }),
skipStartupConnectionCheck: schema.conditional(
// Using dist over dev because integration_tests run with dev: false,
// and this config is solely introduced to allow some of the integration tests to run without an ES server.
schema.contextRef('dist'),
true,
schema.boolean({
Copy link
Contributor

@mshustov mshustov Aug 26, 2021

Choose a reason for hiding this comment

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

nit: you can use schena.never() instead

emsUrl: schema.conditional(
schema.siblingRef('proxyElasticMapsServiceInMaps'),
true,
schema.never(),
schema.string({ defaultValue: '' })
),

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense! but then the types will be skipStartupConnectionCheck?: boolean, meaning that we'll have undefined | false | true, it doesn't change much of the behaviour/logic, but do we want those 3 states?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, we don't. We can normalize them in ElasticsearchConfig:

this.skipStartupConnectionCheck = Boolean(rawConfig.skipStartupConnectionCheck)

Copy link
Member Author

Choose a reason for hiding this comment

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

I like it! The error message is not as self-explanatory, though:

"[skipStartupConnectionCheck]: a value wasn't expected to be present"

as opposed to the current

"[skipStartupConnectionCheck]: \"skipStartupConnectionCheck\" can only be set to true when running from source to allow integration tests to run without an ES server"

If you think it's fine, I'm happy to change to schema.never() :)

Copy link
Contributor

Choose a reason for hiding this comment

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

If you think it's fine, I'm happy to change to

Up to you

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave it as is for now then, for consistency with other keys in the same Elasticsearch config (i.e.: ignoreVersionMismatch).

I've created #110239 to follow up and change them all if we think schema.never() is a better approach.

validate: (rawValue) => {
if (rawValue === true) {
return '"skipStartupConnectionCheck" can only be set to true when running from source to allow integration tests to run without an ES server';
}
},
defaultValue: false,
}),
schema.boolean({ defaultValue: false })
),
});

const deprecations: ConfigDeprecationProvider = () => [
Expand Down Expand Up @@ -222,7 +236,8 @@ export const config: ServiceConfigDescriptor<ElasticsearchConfigType> = {
*/
export class ElasticsearchConfig {
/**
* Skip the valid connection check during startup. The connection check allows
* @internal
* Only valid in dev mode. Skip the valid connection check during startup. The connection check allows
* Kibana to ensure that the Elasticsearch connection is valid before allowing
* any other services to be set up.
*
Expand Down
3 changes: 2 additions & 1 deletion src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ export const config: {
delay: Type<import("moment").Duration>;
}>;
ignoreVersionMismatch: import("@kbn/config-schema/target_types/types").ConditionalType<false, boolean, boolean>;
skipStartupConnectionCheck: Type<boolean>;
skipStartupConnectionCheck: import("@kbn/config-schema/target_types/types").ConditionalType<true, boolean, boolean>;
}>;
};
logging: {
Expand Down Expand Up @@ -826,6 +826,7 @@ export class ElasticsearchConfig {
readonly requestTimeout: Duration;
readonly serviceAccountToken?: string;
readonly shardTimeout: Duration;
// @internal
readonly skipStartupConnectionCheck: boolean;
readonly sniffInterval: false | Duration;
readonly sniffOnConnectionFault: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ kibana_vars=(
elasticsearch.requestTimeout
elasticsearch.serviceAccountToken
elasticsearch.shardTimeout
elasticsearch.skipStartupConnectionCheck
elasticsearch.sniffInterval
elasticsearch.sniffOnConnectionFault
elasticsearch.sniffOnStart
Expand Down