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

createCluster requires a partial elasticsearch config #40405

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -4,6 +4,7 @@

## ElasticsearchClientConfig type


<b>Signature:</b>

```typescript
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,20 @@

## ElasticsearchServiceSetup.createClient property

Create application specific Elasticsearch cluster API client with customized config.

<b>Signature:</b>

```typescript
readonly createClient: (type: string, config: ElasticsearchClientConfig) => ClusterClient;
readonly createClient: (type: string, clientConfig?: Partial<ElasticsearchClientConfig>) => ClusterClient;
```

## Example


```js
const client = elasticsearch.createCluster('my-app-name', config);
const data = await client.callAsInternalUser();

```

Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export interface ElasticsearchServiceSetup
| Property | Type | Description |
| --- | --- | --- |
| [adminClient$](./kibana-plugin-server.elasticsearchservicesetup.adminclient$.md) | <code>Observable&lt;ClusterClient&gt;</code> | |
| [createClient](./kibana-plugin-server.elasticsearchservicesetup.createclient.md) | <code>(type: string, config: ElasticsearchClientConfig) =&gt; ClusterClient</code> | |
| [createClient](./kibana-plugin-server.elasticsearchservicesetup.createclient.md) | <code>(type: string, clientConfig?: Partial&lt;ElasticsearchClientConfig&gt;) =&gt; ClusterClient</code> | Create application specific Elasticsearch cluster API client with customized config. |
| [dataClient$](./kibana-plugin-server.elasticsearchservicesetup.dataclient$.md) | <code>Observable&lt;ClusterClient&gt;</code> | |
| [legacy](./kibana-plugin-server.elasticsearchservicesetup.legacy.md) | <code>{</code><br/><code> readonly config$: Observable&lt;ElasticsearchConfig&gt;;</code><br/><code> }</code> | |

Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { Logger } from '../logging';
import { ElasticsearchConfig } from './elasticsearch_config';

/**
* @internalremarks Config that consumers can pass to the Elasticsearch JS client is complex and includes
* @privateRemarks Config that consumers can pass to the Elasticsearch JS client is complex and includes
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 only entries from standard `elasticsearch.*` yaml config, but also some Elasticsearch JS
* client specific options like `keepAlive` or `plugins` (that eventually will be deprecated).
*
Expand Down
102 changes: 81 additions & 21 deletions src/core/server/elasticsearch/elasticsearch_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,12 @@ const deps = {
configService.atPath.mockReturnValue(
new BehaviorSubject({
hosts: ['http://1.2.3.4'],
healthCheck: {},
ssl: {},
healthCheck: {
delay: 2000,
},
ssl: {
verificationMode: 'none',
},
} as any)
);

Expand All @@ -57,15 +61,15 @@ beforeEach(() => {
afterEach(() => jest.clearAllMocks());

describe('#setup', () => {
test('returns legacy Elasticsearch config as a part of the contract', async () => {
it('returns legacy Elasticsearch config as a part of the contract', async () => {
const setupContract = await elasticsearchService.setup(deps);

await expect(setupContract.legacy.config$.pipe(first()).toPromise()).resolves.toBeInstanceOf(
ElasticsearchConfig
);
});

test('returns data and admin client observables as a part of the contract', async () => {
it('returns data and admin client observables as a part of the contract', async () => {
const mockAdminClusterClientInstance = { close: jest.fn() };
const mockDataClusterClientInstance = { close: jest.fn() };
MockClusterClient.mockImplementationOnce(
Expand Down Expand Up @@ -103,27 +107,83 @@ describe('#setup', () => {
expect(mockDataClusterClientInstance.close).not.toHaveBeenCalled();
});

test('returns `createClient` as a part of the contract', async () => {
const setupContract = await elasticsearchService.setup(deps);

const mockClusterClientInstance = { close: jest.fn() };
MockClusterClient.mockImplementation(() => mockClusterClientInstance);

const mockConfig = { logQueries: true };
const clusterClient = setupContract.createClient('some-custom-type', mockConfig as any);

expect(clusterClient).toBe(mockClusterClientInstance);

expect(MockClusterClient).toHaveBeenCalledWith(
mockConfig,
expect.objectContaining({ context: ['elasticsearch', 'some-custom-type'] }),
expect.any(Function)
);
describe('#createClient', () => {
it('allows to specify config properties', async () => {
const setupContract = await elasticsearchService.setup(deps);

const mockClusterClientInstance = { close: jest.fn() };
MockClusterClient.mockImplementation(() => mockClusterClientInstance);

const customConfig = { logQueries: true };
const clusterClient = setupContract.createClient('some-custom-type', customConfig);

expect(clusterClient).toBe(mockClusterClientInstance);

expect(MockClusterClient).toHaveBeenCalledWith(
expect.objectContaining(customConfig),
expect.objectContaining({ context: ['elasticsearch', 'some-custom-type'] }),
expect.any(Function)
);
});

it('falls back to elasticsearch default config values if property not specified', async () => {
const setupContract = await elasticsearchService.setup(deps);
// reset all mocks called during setup phase
MockClusterClient.mockClear();

const customConfig = {
hosts: ['http://8.8.8.8'],
logQueries: true,
ssl: { certificate: 'certificate-value' },
};
setupContract.createClient('some-custom-type', customConfig);

const config = MockClusterClient.mock.calls[0][0];
expect(config).toMatchInlineSnapshot(`
Object {
"healthCheckDelay": 2000,
"hosts": Array [
"http://8.8.8.8",
],
"logQueries": true,
"requestHeadersWhitelist": Array [
undefined,
],
"ssl": Object {
"certificate": "certificate-value",
"verificationMode": "none",
},
}
`);
});
it('falls back to elasticsearch config if custom config not passed', async () => {
const setupContract = await elasticsearchService.setup(deps);
// reset all mocks called during setup phase
MockClusterClient.mockClear();

setupContract.createClient('another-type');

const config = MockClusterClient.mock.calls[0][0];
expect(config).toMatchInlineSnapshot(`
Object {
"healthCheckDelay": 2000,
"hosts": Array [
"http://1.2.3.4",
],
"requestHeadersWhitelist": Array [
undefined,
],
"ssl": Object {
"verificationMode": "none",
},
}
`);
});
});
});

describe('#stop', () => {
test('stops both admin and data clients', async () => {
it('stops both admin and data clients', async () => {
const mockAdminClusterClientInstance = { close: jest.fn() };
const mockDataClusterClientInstance = { close: jest.fn() };
MockClusterClient.mockImplementationOnce(
Expand Down
33 changes: 28 additions & 5 deletions src/core/server/elasticsearch/elasticsearch_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
*/

import { ConnectableObservable, Observable, Subscription } from 'rxjs';
import { filter, map, publishReplay, switchMap } from 'rxjs/operators';
import { filter, first, map, publishReplay, switchMap } from 'rxjs/operators';
import { merge } from 'lodash';
import { CoreService } from '../../types';
import { CoreContext } from '../core_context';
import { Logger } from '../logging';
Expand All @@ -44,8 +45,27 @@ export interface ElasticsearchServiceSetup {
readonly legacy: {
readonly config$: Observable<ElasticsearchConfig>;
};

readonly createClient: (type: string, config: ElasticsearchClientConfig) => ClusterClient;
/**
* Create application specific Elasticsearch cluster API client with customized config.
*
* @param type Unique identifier of the client
* @param clientConfig A config consists of Elasticsearch JS client options and
* valid sub-set of Elasticsearch service config.
* We fill all the missing properties in the `clientConfig` using the default
* Elasticsearch config so that we don't depend on default values set and
* controlled by underlying Elasticsearch JS client.
* We don't run validation against passed config expect it to be valid.
Copy link
Contributor Author

@mshustov mshustov Jul 8, 2019

Choose a reason for hiding this comment

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

We should add validation eventually

*
* @example
* ```js
* const client = elasticsearch.createCluster('my-app-name', config);
* const data = await client.callAsInternalUser();
* ```
*/
readonly createClient: (
type: string,
clientConfig?: Partial<ElasticsearchClientConfig>
) => ClusterClient;
readonly adminClient$: Observable<ClusterClient>;
readonly dataClient$: Observable<ClusterClient>;
}
Expand Down Expand Up @@ -101,14 +121,17 @@ export class ElasticsearchService implements CoreService<ElasticsearchServiceSet

this.subscription = clients$.connect();

const config = await this.config$.pipe(first()).toPromise();

return {
legacy: { config$: clients$.pipe(map(clients => clients.config)) },

adminClient$: clients$.pipe(map(clients => clients.adminClient)),
dataClient$: clients$.pipe(map(clients => clients.dataClient)),

createClient: (type: string, clientConfig: ElasticsearchClientConfig) => {
return this.createClusterClient(type, clientConfig, deps.http.auth.getAuthHeaders);
createClient: (type: string, clientConfig: Partial<ElasticsearchClientConfig> = {}) => {
const finalConfig = merge({}, config, clientConfig);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not a full equivalent of

{
  ...esConfig,
  ...clientConfig
}

but I'd expect that user may want to re-write only a part of complex ssl config instead of declaring the full version of it.

return this.createClusterClient(type, finalConfig, deps.http.auth.getAuthHeaders);
},
};
}
Expand Down
3 changes: 1 addition & 2 deletions src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,7 @@ export class ElasticsearchErrorHelpers {
export interface ElasticsearchServiceSetup {
// (undocumented)
readonly adminClient$: Observable<ClusterClient>;
// (undocumented)
readonly createClient: (type: string, config: ElasticsearchClientConfig) => ClusterClient;
readonly createClient: (type: string, clientConfig?: Partial<ElasticsearchClientConfig>) => ClusterClient;
// (undocumented)
readonly dataClient$: Observable<ClusterClient>;
// (undocumented)
Expand Down
8 changes: 1 addition & 7 deletions src/legacy/core_plugins/elasticsearch/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,7 @@ export default function (kibana) {
throw new Error(`cluster '${name}' already exists`);
}

// We fill all the missing properties in the `clientConfig` using the default
// Elasticsearch config so that we don't depend on default values set and
// controlled by underlying Elasticsearch JS client.
const cluster = new Cluster(server.newPlatform.setup.core.elasticsearch.createClient(name, {
...esConfig,
...clientConfig,
}));
const cluster = new Cluster(server.newPlatform.setup.core.elasticsearch.createClient(name, clientConfig));

clusters.set(name, cluster);

Expand Down