Skip to content

Commit

Permalink
Remove logger from ElasticsearchConfig
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jportner
jportner committed Jan 9, 2020
1 parent a19698a commit 906c100
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 75 deletions.
95 changes: 29 additions & 66 deletions src/core/server/elasticsearch/elasticsearch_config.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, any> = {}) => {
const deprecations = config.deprecations!(configDeprecationFactory);
const deprecationMessages: string[] = [];
Expand All @@ -57,7 +48,7 @@ const applyElasticsearchDeprecations = (settings: Record<string, any> = {}) => {
};

test('set correct defaults', () => {
const configValue = createElasticsearchConfig(config.schema.validate({}));
const configValue = new ElasticsearchConfig(config.schema.validate({}));
expect(configValue).toMatchInlineSnapshot(`
ElasticsearchConfig {
"apiVersion": "master",
Expand Down Expand Up @@ -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'],
})
Expand All @@ -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'],
})
Expand All @@ -144,37 +135,37 @@ 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);
expect(configValue.ssl.certificateAuthorities).toEqual(['content-of-some-path.ca']);
});

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);
expect(configValue.ssl.certificateAuthorities).toEqual(['content-of-some-path']);
});

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'] },
})
Expand All @@ -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' },
Expand All @@ -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);
Expand All @@ -216,15 +207,15 @@ 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);
expect(configValue.ssl.key).toEqual('content-of-some-path');
});

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);
Expand All @@ -247,33 +238,33 @@ 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'"`
);
});

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'"`
);
});

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'"`
);
Expand All @@ -282,17 +273,17 @@ 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."`
);
});

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'"`
);
Expand All @@ -313,34 +304,6 @@ describe('throws when config is invalid', () => {
});
});

describe('logs warnings', () => {
let logger: ReturnType<typeof loggingServiceMock.create>;
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' });
Expand Down
9 changes: 1 addition & 8 deletions src/core/server/elasticsearch/elasticsearch_config.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'] });
Expand Down Expand Up @@ -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;
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/core/server/elasticsearch/elasticsearch_service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export class ElasticsearchService implements CoreService<InternalElasticsearchSe
this.log = coreContext.logger.get('elasticsearch-service');
this.config$ = coreContext.configService
.atPath<ElasticsearchConfigType>('elasticsearch')
.pipe(map(rawConfig => new ElasticsearchConfig(rawConfig, coreContext.logger.get('config'))));
.pipe(map(rawConfig => new ElasticsearchConfig(rawConfig)));
}

public async setup(deps: SetupDeps): Promise<InternalElasticsearchServiceSetup> {
Expand Down

0 comments on commit 906c100

Please sign in to comment.