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

Fix config validation when elastic.apm is defined #96502

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
46 changes: 43 additions & 3 deletions src/core/server/config/ensure_valid_configuration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,40 @@ describe('ensureValidConfiguration', () => {
beforeEach(() => {
jest.clearAllMocks();
configService = configServiceMock.create();
configService.getUsedPaths.mockReturnValue(Promise.resolve(['core', 'elastic']));

configService.validate.mockResolvedValue();
configService.getUsedPaths.mockReturnValue(Promise.resolve([]));
});

it('returns normally when there is no unused keys', async () => {
configService.getUnusedPaths.mockResolvedValue([]);
it('returns normally when there is no unused keys and when the config validates', async () => {
await expect(ensureValidConfiguration(configService as any)).resolves.toBeUndefined();
});

it('throws when config validation fails', async () => {
configService.validate.mockImplementation(() => {
throw new Error('some message');
});

await expect(ensureValidConfiguration(configService as any)).rejects.toMatchInlineSnapshot(
`[Error: some message]`
);
});

it('throws a `CriticalError` with the correct processExitCode value when config validation fails', async () => {
expect.assertions(2);

configService.validate.mockImplementation(() => {
throw new Error('some message');
});

try {
await ensureValidConfiguration(configService as any);
} catch (e) {
expect(e).toBeInstanceOf(CriticalError);
expect(e.processExitCode).toEqual(78);
}
});

it('throws when there are some unused keys', async () => {
configService.getUnusedPaths.mockResolvedValue(['some.key', 'some.other.key']);

Expand All @@ -44,4 +70,18 @@ describe('ensureValidConfiguration', () => {
expect(e.processExitCode).toEqual(64);
}
});

it('does not throw when all unused keys are included in the ignored paths', async () => {
configService.getUnusedPaths.mockResolvedValue(['dev.someDevKey', 'elastic.apm.enabled']);

await expect(ensureValidConfiguration(configService as any)).resolves.toBeUndefined();
});

it('throws when only some keys are included in the ignored paths', async () => {
configService.getUnusedPaths.mockResolvedValue(['dev.someDevKey', 'some.key']);

await expect(ensureValidConfiguration(configService as any)).rejects.toMatchInlineSnapshot(
`[Error: Unknown configuration key(s): "some.key". Check for spelling errors and ensure that expected plugins are installed.]`
);
});
});
25 changes: 15 additions & 10 deletions src/core/server/config/ensure_valid_configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,27 @@
import { ConfigService } from '@kbn/config';
import { CriticalError } from '../errors';

const ignoredPaths = ['dev.', 'elastic.apm.'];
Copy link
Contributor Author

@pgayvallet pgayvallet Apr 7, 2021

Choose a reason for hiding this comment

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

The dev schema was only added to avoid validation failures. Now that we have this ignore list instead, I removed the dev schema and added it here.


const invalidConfigExitCode = 78;
const legacyInvalidConfigExitCode = 64;
Comment on lines +14 to +15
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping 64 for invalid config keys until 8.0 for BWC. Added a task to change that in the 8.0 meta issue.


export async function ensureValidConfiguration(configService: ConfigService) {
await configService.validate();
try {
await configService.validate();
} catch (e) {
throw new CriticalError(e.message, 'InvalidConfig', invalidConfigExitCode, e);
}

const unusedConfigKeys = await configService.getUnusedPaths();
const unusedPaths = await configService.getUnusedPaths();
const unusedConfigKeys = unusedPaths.filter((unusedPath) => {
return !ignoredPaths.some((ignoredPath) => unusedPath.startsWith(ignoredPath));
});

if (unusedConfigKeys.length > 0) {
const message = `Unknown configuration key(s): ${unusedConfigKeys
.map((key) => `"${key}"`)
.join(', ')}. Check for spelling errors and ensure that expected plugins are installed.`;
throw new InvalidConfigurationError(message);
}
}

class InvalidConfigurationError extends CriticalError {
constructor(message: string) {
super(message, 'InvalidConfig', 64);
Object.setPrototypeOf(this, InvalidConfigurationError.prototype);
throw new CriticalError(message, 'InvalidConfig', legacyInvalidConfigExitCode);
}
}
16 changes: 0 additions & 16 deletions src/core/server/dev/dev_config.ts

This file was deleted.

9 changes: 0 additions & 9 deletions src/core/server/dev/index.ts

This file was deleted.

2 changes: 0 additions & 2 deletions src/core/server/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import { config as cspConfig } from './csp';
import { config as elasticsearchConfig } from './elasticsearch';
import { config as httpConfig } from './http';
import { config as loggingConfig } from './logging';
import { config as devConfig } from './dev';
import { config as kibanaConfig } from './kibana_config';
import { savedObjectsConfig, savedObjectsMigrationConfig } from './saved_objects';
import { config as uiSettingsConfig } from './ui_settings';
Expand Down Expand Up @@ -303,7 +302,6 @@ export class Server {
loggingConfig,
httpConfig,
pluginsConfig,
devConfig,
kibanaConfig,
savedObjectsConfig,
savedObjectsMigrationConfig,
Expand Down