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

[Reporting] Internally correct the hostname to "localhost" if "server.host" is "0.0.0.0" #117022

Merged
merged 9 commits into from
Nov 3, 2021
69 changes: 41 additions & 28 deletions x-pack/plugins/reporting/server/config/create_config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,29 @@
* 2.0.
*/

import * as Rx from 'rxjs';
import { CoreSetup, PluginInitializerContext } from 'src/core/server';
import { coreMock } from 'src/core/server/mocks';
import { LevelLogger } from '../lib';
import { createMockConfigSchema } from '../test_helpers';
import { LevelLogger } from '../lib/level_logger';
import { createMockConfigSchema, createMockLevelLogger } from '../test_helpers';
import { ReportingConfigType } from './';
import { createConfig$ } from './create_config';

const createMockConfig = (
mockInitContext: PluginInitializerContext<unknown>
): Rx.Observable<ReportingConfigType> => mockInitContext.config.create();

describe('Reporting server createConfig$', () => {
let mockCoreSetup: CoreSetup;
let mockInitContext: PluginInitializerContext;
let mockLogger: LevelLogger;
let mockLogger: jest.Mocked<LevelLogger>;

beforeEach(() => {
mockCoreSetup = coreMock.createSetup();
mockInitContext = coreMock.createPluginInitializerContext(
createMockConfigSchema({ kibanaServer: {} })
);
mockLogger = {
warn: jest.fn(),
debug: jest.fn(),
clone: jest.fn().mockImplementation(() => mockLogger),
} as unknown as LevelLogger;
mockLogger = createMockLevelLogger();
});

afterEach(() => {
Expand All @@ -37,19 +39,12 @@ describe('Reporting server createConfig$', () => {
...createMockConfigSchema({ kibanaServer: {} }),
encryptionKey: undefined,
});
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result.encryptionKey).toMatch(/\S{32,}/); // random 32 characters
expect(result.kibanaServer).toMatchInlineSnapshot(`
Object {
"hostname": "localhost",
"port": 80,
"protocol": "http",
}
`);
expect((mockLogger.warn as any).mock.calls.length).toBe(1);
expect((mockLogger.warn as any).mock.calls[0]).toMatchObject([
expect(mockLogger.warn.mock.calls.length).toBe(1);
expect(mockLogger.warn.mock.calls[0]).toMatchObject([
'Generating a random key for xpack.reporting.encryptionKey. To prevent sessions from being invalidated on restart, please set xpack.reporting.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.',
]);
Comment on lines +46 to 49
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that the change was about removing the any. Since I have more relevant feedback below, maybe we can commit the following to prettify the code even more.

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(1);
expect(mockLogger.warn.mock.calls[0]).toMatchObject([
'Generating a random key for xpack.reporting.encryptionKey. To prevent sessions from being invalidated on restart, please set xpack.reporting.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.',
]);
expect(mockLogger.warn).toHaveBeenCalledTimes(1);
expect(mockLogger.warn).toHaveBeenCalledWith(
'Generating a random key for xpack.reporting.encryptionKey. To prevent sessions from being invalidated on restart, please set xpack.reporting.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.'
);

});
Expand All @@ -60,10 +55,10 @@ describe('Reporting server createConfig$', () => {
encryptionKey: 'iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii',
})
);
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();
expect(result.encryptionKey).toMatch('iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii');
expect((mockLogger.warn as any).mock.calls.length).toBe(0);
expect(mockLogger.warn.mock.calls.length).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(0);
expect(mockLogger.warn).not.toHaveBeenCalled();

});

it('uses the user-provided encryption key, reporting kibanaServer settings to override server info', async () => {
Expand All @@ -77,7 +72,7 @@ describe('Reporting server createConfig$', () => {
},
})
);
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result).toMatchInlineSnapshot(`
Expand Down Expand Up @@ -108,7 +103,25 @@ describe('Reporting server createConfig$', () => {
},
}
`);
expect((mockLogger.warn as any).mock.calls.length).toBe(0);
expect(mockLogger.warn.mock.calls.length).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(0);
expect(mockLogger.warn).not.toHaveBeenCalled();

});

it('show warning when kibanaServer.hostName === "0.0.0.0"', async () => {
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
mockInitContext = coreMock.createPluginInitializerContext({
encryptionKey: 'aaaaaaaaaaaaabbbbbbbbbbbbaaaaaaaaa',
kibanaServer: { hostname: '0.0.0.0', port: 5601 },
capture: { browser: { chromium: {} } },
});
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result.kibanaServer).toMatchInlineSnapshot(`
Object {
"hostname": "localhost",
"port": 5601,
"protocol": "http",
}
`);
});

it('uses user-provided disableSandbox: false', async () => {
Expand All @@ -118,11 +131,11 @@ describe('Reporting server createConfig$', () => {
capture: { browser: { chromium: { disableSandbox: false } } },
})
);
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result.capture.browser.chromium).toMatchObject({ disableSandbox: false });
expect((mockLogger.warn as any).mock.calls.length).toBe(0);
expect(mockLogger.warn.mock.calls.length).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(0);
expect(mockLogger.warn).not.toHaveBeenCalled();

});

it('uses user-provided disableSandbox: true', async () => {
Expand All @@ -132,11 +145,11 @@ describe('Reporting server createConfig$', () => {
capture: { browser: { chromium: { disableSandbox: true } } },
})
);
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result.capture.browser.chromium).toMatchObject({ disableSandbox: true });
expect((mockLogger.warn as any).mock.calls.length).toBe(0);
expect(mockLogger.warn.mock.calls.length).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(0);
expect(mockLogger.warn).not.toHaveBeenCalled();

});

it('provides a default for disableSandbox', async () => {
Expand All @@ -145,10 +158,10 @@ describe('Reporting server createConfig$', () => {
encryptionKey: '888888888888888888888888888888888',
})
);
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result.capture.browser.chromium).toMatchObject({ disableSandbox: expect.any(Boolean) });
expect((mockLogger.warn as any).mock.calls.length).toBe(0);
expect(mockLogger.warn.mock.calls.length).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(0);
expect(mockLogger.warn).not.toHaveBeenCalled();

});
});
18 changes: 15 additions & 3 deletions x-pack/plugins/reporting/server/config/create_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,22 @@ export function createConfig$(
}
const { kibanaServer: reportingServer } = config;
const serverInfo = core.http.getServerInfo();
// kibanaServer.hostname, default to server.host
const kibanaServerHostname = reportingServer.hostname
// set kibanaServer.hostname, default to server.host, don't allow "0.0.0.0" as it breaks in Windows
let kibanaServerHostname = reportingServer.hostname
? reportingServer.hostname
: serverInfo.hostname;
if (kibanaServerHostname === '0.0.0.0') {
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
logger.warn(
i18n.translate('xpack.reporting.serverConfig.invalidServerHostname', {
defaultMessage:
`Found 'server.host: "0.0.0.0"' in Kibana configuration. Reporting is not able to use this as the Kibana server hostname.` +
` To enable PNG/PDF Reporting to work, '{configKey}: localhost' is automatically set in the configuration.` +
` You can prevent this message by adding '{configKey}: localhost' in kibana.yml.`,
values: { configKey: 'xpack.reporting.kibanaServer.hostname' },
})
);
kibanaServerHostname = 'localhost';
}
// kibanaServer.port, default to server.port
const kibanaServerPort = reportingServer.port ? reportingServer.port : serverInfo.port;
// kibanaServer.protocol, default to server.protocol
Expand All @@ -66,7 +78,7 @@ export function createConfig$(
mergeMap(async (config) => {
if (config.capture.browser.chromium.disableSandbox != null) {
// disableSandbox was set by user
return config;
return { ...config };
}

// disableSandbox was not set by user, apply default for OS
Expand Down
11 changes: 10 additions & 1 deletion x-pack/plugins/reporting/server/config/schema.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,20 @@ describe('Reporting Config Schema', () => {
`);
});

it(`logs the proper validation messages`, () => {
it(`logs the proper validation messages for invalid hostname`, () => {
// kibanaServer
const throwValidationErr = () => ConfigSchema.validate({ kibanaServer: { hostname: '0' } });
expect(throwValidationErr).toThrowError(
`[kibanaServer.hostname]: value must be a valid hostname (see RFC 1123).`
);
});

it(`logs the proper validation messages for hostname: 0.0.0.0`, () => {
// kibanaServer
const throwValidationErr = () =>
ConfigSchema.validate({ kibanaServer: { hostname: '0.0.0.0' } });
expect(throwValidationErr).toThrowError(
`[kibanaServer.hostname]: must be a valid hostname, not "0.0.0.0"`
);
});
});
12 changes: 11 additions & 1 deletion x-pack/plugins/reporting/server/config/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,17 @@ import { ByteSizeValue, schema, TypeOf } from '@kbn/config-schema';
import moment from 'moment';

const KibanaServerSchema = schema.object({
hostname: schema.maybe(schema.string({ hostname: true })),
hostname: schema.maybe(
schema.string({
hostname: true,
validate(value) {
if (/^0.0.0.0$/.test(value)) {
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
// prevent setting a hostname that fails in Chromium on Windows
return 'must be a valid hostname, not "0.0.0.0"';
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
}
},
})
),
port: schema.maybe(schema.number()),
protocol: schema.maybe(
schema.string({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@ export function createMockLevelLogger() {

const logger = new LevelLogger(loggingSystemMock.create()) as jest.Mocked<LevelLogger>;

logger.clone.mockImplementation(createMockLevelLogger);
// logger.debug.mockImplementation(consoleLogger('debug')); // uncomment this to see debug logs in jest tests
logger.info.mockImplementation(consoleLogger('info'));
logger.warn.mockImplementation(consoleLogger('warn'));
logger.warning = jest.fn().mockImplementation(consoleLogger('warn'));
logger.error.mockImplementation(consoleLogger('error'));
logger.trace.mockImplementation(consoleLogger('trace'));

logger.clone.mockImplementation(() => logger);

return logger;
}