From 906c1004259c87e84ffaa857a4216ddee846a9cf Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Thu, 9 Jan 2020 13:05:12 -0500 Subject: [PATCH] Remove logger from ElasticsearchConfig This was used to log warnings messages. We have decided to deprecate the config settings instead of just logging warnings. So ElasticsearchConfig doesn't need to have a logger at all anymore. --- .../elasticsearch_config.test.ts | 95 ++++++------------- .../elasticsearch/elasticsearch_config.ts | 9 +- .../elasticsearch/elasticsearch_service.ts | 2 +- 3 files changed, 31 insertions(+), 75 deletions(-) diff --git a/src/core/server/elasticsearch/elasticsearch_config.test.ts b/src/core/server/elasticsearch/elasticsearch_config.test.ts index a9001568691a9..3196c8e32efb8 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.test.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.test.ts @@ -23,20 +23,11 @@ import { mockReadPkcs12Truststore, } from './elasticsearch_config.test.mocks'; -import { ElasticsearchConfig, config, ElasticsearchConfigType } from './elasticsearch_config'; -import { loggingServiceMock } from '../mocks'; -import { Logger } from '../logging'; +import { ElasticsearchConfig, config } from './elasticsearch_config'; import { applyDeprecations, configDeprecationFactory } from '../config/deprecation'; const CONFIG_PATH = 'elasticsearch'; -const createElasticsearchConfig = (rawConfig: ElasticsearchConfigType, log?: Logger) => { - if (!log) { - log = loggingServiceMock.create().get('config'); - } - return new ElasticsearchConfig(rawConfig, log); -}; - const applyElasticsearchDeprecations = (settings: Record = {}) => { const deprecations = config.deprecations!(configDeprecationFactory); const deprecationMessages: string[] = []; @@ -57,7 +48,7 @@ const applyElasticsearchDeprecations = (settings: Record = {}) => { }; test('set correct defaults', () => { - const configValue = createElasticsearchConfig(config.schema.validate({})); + const configValue = new ElasticsearchConfig(config.schema.validate({})); expect(configValue).toMatchInlineSnapshot(` ElasticsearchConfig { "apiVersion": "master", @@ -92,17 +83,17 @@ test('set correct defaults', () => { }); test('#hosts accepts both string and array of strings', () => { - let configValue = createElasticsearchConfig( + let configValue = new ElasticsearchConfig( config.schema.validate({ hosts: 'http://some.host:1234' }) ); expect(configValue.hosts).toEqual(['http://some.host:1234']); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ hosts: ['http://some.host:1234'] }) ); expect(configValue.hosts).toEqual(['http://some.host:1234']); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ hosts: ['http://some.host:1234', 'https://some.another.host'], }) @@ -111,17 +102,17 @@ test('#hosts accepts both string and array of strings', () => { }); test('#requestHeadersWhitelist accepts both string and array of strings', () => { - let configValue = createElasticsearchConfig( + let configValue = new ElasticsearchConfig( config.schema.validate({ requestHeadersWhitelist: 'token' }) ); expect(configValue.requestHeadersWhitelist).toEqual(['token']); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ requestHeadersWhitelist: ['token'] }) ); expect(configValue.requestHeadersWhitelist).toEqual(['token']); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ requestHeadersWhitelist: ['token', 'X-Forwarded-Proto'], }) @@ -144,7 +135,7 @@ describe('reads files', () => { }); it('reads certificate authorities when ssl.keystore.path is specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { keystore: { path: 'some-path' } } }) ); expect(mockReadPkcs12Keystore).toHaveBeenCalledTimes(1); @@ -152,7 +143,7 @@ describe('reads files', () => { }); it('reads certificate authorities when ssl.truststore.path is specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { truststore: { path: 'some-path' } } }) ); expect(mockReadPkcs12Truststore).toHaveBeenCalledTimes(1); @@ -160,21 +151,21 @@ describe('reads files', () => { }); it('reads certificate authorities when ssl.certificateAuthorities is specified', () => { - let configValue = createElasticsearchConfig( + let configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { certificateAuthorities: 'some-path' } }) ); expect(mockReadFileSync).toHaveBeenCalledTimes(1); expect(configValue.ssl.certificateAuthorities).toEqual(['content-of-some-path']); mockReadFileSync.mockClear(); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { certificateAuthorities: ['some-path'] } }) ); expect(mockReadFileSync).toHaveBeenCalledTimes(1); expect(configValue.ssl.certificateAuthorities).toEqual(['content-of-some-path']); mockReadFileSync.mockClear(); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { certificateAuthorities: ['some-path', 'another-path'] }, }) @@ -187,7 +178,7 @@ describe('reads files', () => { }); it('reads certificate authorities when ssl.keystore.path, ssl.truststore.path, and ssl.certificateAuthorities are specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { keystore: { path: 'some-path' }, @@ -207,7 +198,7 @@ describe('reads files', () => { }); it('reads a private key and certificate when ssl.keystore.path is specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { keystore: { path: 'some-path' } } }) ); expect(mockReadPkcs12Keystore).toHaveBeenCalledTimes(1); @@ -216,7 +207,7 @@ describe('reads files', () => { }); it('reads a private key when ssl.key is specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { key: 'some-path' } }) ); expect(mockReadFileSync).toHaveBeenCalledTimes(1); @@ -224,7 +215,7 @@ describe('reads files', () => { }); it('reads a certificate when ssl.certificate is specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { certificate: 'some-path' } }) ); expect(mockReadFileSync).toHaveBeenCalledTimes(1); @@ -247,8 +238,8 @@ describe('throws when config is invalid', () => { it('throws if key is invalid', () => { const value = { ssl: { key: '/invalid/key' } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot( `"ENOENT: no such file or directory, open '/invalid/key'"` ); @@ -256,8 +247,8 @@ describe('throws when config is invalid', () => { it('throws if certificate is invalid', () => { const value = { ssl: { certificate: '/invalid/cert' } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot( `"ENOENT: no such file or directory, open '/invalid/cert'"` ); @@ -265,15 +256,15 @@ describe('throws when config is invalid', () => { it('throws if certificateAuthorities is invalid', () => { const value = { ssl: { certificateAuthorities: '/invalid/ca' } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot(`"ENOENT: no such file or directory, open '/invalid/ca'"`); }); it('throws if keystore path is invalid', () => { const value = { ssl: { keystore: { path: '/invalid/keystore' } } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot( `"ENOENT: no such file or directory, open '/invalid/keystore'"` ); @@ -282,8 +273,8 @@ describe('throws when config is invalid', () => { it('throws if keystore does not contain a key or certificate', () => { mockReadPkcs12Keystore.mockReturnValueOnce({}); const value = { ssl: { keystore: { path: 'some-path' } } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot( `"Did not find key or certificate in Elasticsearch keystore."` ); @@ -291,8 +282,8 @@ describe('throws when config is invalid', () => { it('throws if truststore path is invalid', () => { const value = { ssl: { keystore: { path: '/invalid/truststore' } } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot( `"ENOENT: no such file or directory, open '/invalid/truststore'"` ); @@ -313,34 +304,6 @@ describe('throws when config is invalid', () => { }); }); -describe('logs warnings', () => { - let logger: ReturnType; - let log: Logger; - - beforeAll(() => { - mockReadFileSync.mockResolvedValue('foo'); - }); - - beforeEach(() => { - logger = loggingServiceMock.create(); - log = logger.get('config'); - }); - - it('warns if ssl.key is set and ssl.certificate is not', () => { - createElasticsearchConfig(config.schema.validate({ ssl: { key: 'some-path' } }), log); - expect(loggingServiceMock.collect(logger).warn[0][0]).toMatchInlineSnapshot( - `"Detected a key without a certificate; mutual TLS authentication is disabled."` - ); - }); - - it('warns if ssl.certificate is set and ssl.key is not', () => { - createElasticsearchConfig(config.schema.validate({ ssl: { certificate: 'some-path' } }), log); - expect(loggingServiceMock.collect(logger).warn[0][0]).toMatchInlineSnapshot( - `"Detected a certificate without a key; mutual TLS authentication is disabled."` - ); - }); -}); - describe('deprecations', () => { it('logs a warning if elasticsearch.username is set to "elastic"', () => { const { messages } = applyElasticsearchDeprecations({ username: 'elastic' }); diff --git a/src/core/server/elasticsearch/elasticsearch_config.ts b/src/core/server/elasticsearch/elasticsearch_config.ts index 14a8165233c51..e254f95166b6f 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.ts @@ -22,7 +22,6 @@ import { Duration } from 'moment'; import { readFileSync } from 'fs'; import { ConfigDeprecationProvider } from 'src/core/server'; import { readPkcs12Keystore, readPkcs12Truststore } from '../../utils'; -import { Logger } from '../logging'; import { ServiceConfigDescriptor } from '../internal_types'; const hostURISchema = schema.uri({ scheme: ['http', 'https'] }); @@ -234,7 +233,7 @@ export class ElasticsearchConfig { */ public readonly customHeaders: ElasticsearchConfigType['customHeaders']; - constructor(rawConfig: ElasticsearchConfigType, log: Logger) { + constructor(rawConfig: ElasticsearchConfigType) { this.ignoreVersionMismatch = rawConfig.ignoreVersionMismatch; this.apiVersion = rawConfig.apiVersion; this.logQueries = rawConfig.logQueries; @@ -256,12 +255,6 @@ export class ElasticsearchConfig { const { alwaysPresentCertificate, verificationMode } = rawConfig.ssl; const { key, keyPassphrase, certificate, certificateAuthorities } = readKeyAndCerts(rawConfig); - if (key && !certificate) { - log.warn(`Detected a key without a certificate; mutual TLS authentication is disabled.`); - } else if (certificate && !key) { - log.warn(`Detected a certificate without a key; mutual TLS authentication is disabled.`); - } - this.ssl = { alwaysPresentCertificate, key, diff --git a/src/core/server/elasticsearch/elasticsearch_service.ts b/src/core/server/elasticsearch/elasticsearch_service.ts index de32e7f6cf225..db3fda3a504ab 100644 --- a/src/core/server/elasticsearch/elasticsearch_service.ts +++ b/src/core/server/elasticsearch/elasticsearch_service.ts @@ -52,7 +52,7 @@ export class ElasticsearchService implements CoreService('elasticsearch') - .pipe(map(rawConfig => new ElasticsearchConfig(rawConfig, coreContext.logger.get('config')))); + .pipe(map(rawConfig => new ElasticsearchConfig(rawConfig))); } public async setup(deps: SetupDeps): Promise {