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

add deprecation warning for legacy 3rd party plugins #62401

Merged
7 changes: 6 additions & 1 deletion src/core/server/legacy/legacy_service.test.mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ export const findLegacyPluginSpecsMock = jest.fn().mockImplementation((settings:
uiExports: {},
navLinks: [],
}));
jest.doMock('./plugins/find_legacy_plugin_specs.ts', () => ({
jest.doMock('./plugins/find_legacy_plugin_specs', () => ({
findLegacyPluginSpecs: findLegacyPluginSpecsMock,
}));

export const logLegacyThirdPartyPluginDeprecationWarningMock = jest.fn();
jest.doMock('./plugins/log_legacy_plugins_warning', () => ({
logLegacyThirdPartyPluginDeprecationWarning: logLegacyThirdPartyPluginDeprecationWarningMock,
}));
37 changes: 36 additions & 1 deletion src/core/server/legacy/legacy_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ jest.mock('../../../cli/cluster/cluster_manager');
jest.mock('./config/legacy_deprecation_adapters', () => ({
convertLegacyDeprecationProvider: (provider: any) => Promise.resolve(provider),
}));
import { findLegacyPluginSpecsMock } from './legacy_service.test.mocks';
import {
findLegacyPluginSpecsMock,
logLegacyThirdPartyPluginDeprecationWarningMock,
} from './legacy_service.test.mocks';

import { BehaviorSubject, throwError } from 'rxjs';

Expand Down Expand Up @@ -472,6 +475,38 @@ describe('#discoverPlugins()', () => {
expect(configService.addDeprecationProvider).toHaveBeenCalledWith('', 'providerA');
expect(configService.addDeprecationProvider).toHaveBeenCalledWith('', 'providerB');
});

it(`logs deprecations for legacy third party plugins`, async () => {
const pluginSpecs = [
{ getId: () => 'pluginA', getDeprecationsProvider: () => undefined },
{ getId: () => 'pluginB', getDeprecationsProvider: () => undefined },
];
findLegacyPluginSpecsMock.mockImplementation(
settings =>
Promise.resolve({
pluginSpecs,
pluginExtendedConfig: settings,
disabledPluginSpecs: [],
uiExports: {},
navLinks: [],
}) as any
);

const legacyService = new LegacyService({
coreId,
env,
logger,
configService: configService as any,
});

await legacyService.discoverPlugins();

expect(logLegacyThirdPartyPluginDeprecationWarningMock).toHaveBeenCalledTimes(1);
expect(logLegacyThirdPartyPluginDeprecationWarningMock).toHaveBeenCalledWith({
specs: pluginSpecs,
log: expect.any(Object),
});
});
});

test('Sets the server.uuid property on the legacy configuration', async () => {
Expand Down
7 changes: 6 additions & 1 deletion src/core/server/legacy/legacy_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { DevConfig, DevConfigType, config as devConfig } from '../dev';
import { BasePathProxyServer, HttpConfig, HttpConfigType, config as httpConfig } from '../http';
import { Logger } from '../logging';
import { PathConfigType } from '../path';
import { findLegacyPluginSpecs } from './plugins';
import { findLegacyPluginSpecs, logLegacyThirdPartyPluginDeprecationWarning } from './plugins';
import { convertLegacyDeprecationProvider } from './config';
import {
ILegacyInternals,
Expand Down Expand Up @@ -133,6 +133,11 @@ export class LegacyService implements CoreService {
this.coreContext.env.packageInfo
);

logLegacyThirdPartyPluginDeprecationWarning({
specs: pluginSpecs,
log: this.log,
});
Comment on lines +136 to +139
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I also check for disabled plugins (disabledPluginSpecs ) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We want plugin authors to update their code. They don't disable their plugins likely.


this.legacyPlugins = {
pluginSpecs,
disabledPluginSpecs,
Expand Down
1 change: 1 addition & 0 deletions src/core/server/legacy/plugins/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,4 @@
*/

export { findLegacyPluginSpecs } from './find_legacy_plugin_specs';
export { logLegacyThirdPartyPluginDeprecationWarning } from './log_legacy_plugins_warning';
90 changes: 90 additions & 0 deletions src/core/server/legacy/plugins/log_legacy_plugins_warning.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { loggerMock } from '../../logging/logger.mock';
import { logLegacyThirdPartyPluginDeprecationWarning } from './log_legacy_plugins_warning';
import { LegacyPluginSpec } from '../types';

const createPluginSpec = ({ id, path }: { id: string; path: string }): LegacyPluginSpec => {
return {
getId: () => id,
getExpectedKibanaVersion: () => 'kibana',
getConfigPrefix: () => 'plugin.config',
getDeprecationsProvider: () => undefined,
getPack: () => ({
getPath: () => path,
}),
};
};

describe('logLegacyThirdPartyPluginDeprecationWarning', () => {
let log: ReturnType<typeof loggerMock.create>;

beforeEach(() => {
log = loggerMock.create();
});

it('logs warning for third party plugins', () => {
logLegacyThirdPartyPluginDeprecationWarning({
specs: [createPluginSpec({ id: 'plugin', path: '/some-external-path' })],
log,
});
expect(log.warn).toHaveBeenCalledTimes(1);
expect(log.warn.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"Some installed third party plugin(s) [plugin] are using the legacy plugin format and will no longer work after version 8.0. Please refer to https://github.com/elastic/kibana/blob/master/docs/migration/migrate_8_0.asciidoc for a list of breaking changes and https://github.com/elastic/kibana/blob/master/src/core/MIGRATION.md for documentation on how to migrate legacy plugins.",
]
`);
});

it('lists all the deprecated plugins and only log once', () => {
logLegacyThirdPartyPluginDeprecationWarning({
specs: [
createPluginSpec({ id: 'pluginA', path: '/abs/path/to/pluginA' }),
createPluginSpec({ id: 'pluginB', path: '/abs/path/to/pluginB' }),
createPluginSpec({ id: 'pluginC', path: '/abs/path/to/pluginC' }),
],
log,
});
expect(log.warn).toHaveBeenCalledTimes(1);
expect(log.warn.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"Some installed third party plugin(s) [pluginA, pluginB, pluginC] are using the legacy plugin format and will no longer work after version 8.0. Please refer to https://github.com/elastic/kibana/blob/master/docs/migration/migrate_8_0.asciidoc for a list of breaking changes and https://github.com/elastic/kibana/blob/master/src/core/MIGRATION.md for documentation on how to migrate legacy plugins.",
]
`);
});

it('does not log warning for internal legacy plugins', () => {
logLegacyThirdPartyPluginDeprecationWarning({
specs: [
createPluginSpec({
id: 'plugin',
path: '/absolute/path/to/kibana/src/legacy/core_plugins',
}),
createPluginSpec({
id: 'plugin',
path: '/absolute/path/to/kibana/x-pack',
}),
],
log,
});

expect(log.warn).not.toHaveBeenCalled();
});
});
52 changes: 52 additions & 0 deletions src/core/server/legacy/plugins/log_legacy_plugins_warning.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { Logger } from '../../logging';
import { LegacyPluginSpec } from '../types';

const internalPaths = ['/src/legacy/core_plugins', '/x-pack'];

const breakingChangesUrl =
'https://github.com/elastic/kibana/blob/master/docs/migration/migrate_8_0.asciidoc';
Copy link
Contributor

Choose a reason for hiding this comment

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

const migrationGuideUrl = 'https://github.com/elastic/kibana/blob/master/src/core/MIGRATION.md';

export const logLegacyThirdPartyPluginDeprecationWarning = ({
specs,
log,
}: {
specs: LegacyPluginSpec[];
log: Logger;
}) => {
const thirdPartySpecs = specs.filter(isThirdPartyPluginSpec);
if (thirdPartySpecs.length > 0) {
const pluginIds = thirdPartySpecs.map(spec => spec.getId());
log.warn(
`Some installed third party plugin(s) [${pluginIds.join(
', '
)}] are using the legacy plugin format and will no longer work after version 8.0. ` +
`Please refer to ${breakingChangesUrl} for a list of breaking changes ` +
`and ${migrationGuideUrl} for documentation on how to migrate legacy plugins.`
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that we want to add link to documentation and that cause the message to be quite big, I thought only logging the warning once with all the plugin ids was better.

Input welcome on the actual message. Are these two links enough, or do you see any to add?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should state will no longer work in a future Kibana release in case we decided to remove support earlier than 8.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we would know which version we plan to drop support at, but realistically I guess this is a reasonable change to protect us. Will do.

Apart from that, do you see anything to change/add in the message?

}
};

const isThirdPartyPluginSpec = (spec: LegacyPluginSpec): boolean => {
const pluginPath = spec.getPack().getPath();
return !internalPaths.some(internalPath => pluginPath.indexOf(internalPath) > -1);
};
Comment on lines +49 to +52
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, this is not the most solid piece of software I ever crafted, but as pack.getPath() returns an absolute url, and as I didn't really want to work with kbnDevUtils.REPO_ROOT and path resolving, I felt this was sufficient, as I think it's very unlikely to have false negatives here.

Tell me if we think this is not sufficient and if this should be improved.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably get around using kbnDevUtils by simply doing:

const rootPath = Path.resolve(Path.join(__dirname, '..', '..', '..', '..', '..'))

Also we may want to make the xpack path more specific x-pack/legacy/plugins so that any plugins loaded in functional tests /x-pack/test/ also get the warning (similar to test/plugin_functional)

Copy link
Contributor Author

@pgayvallet pgayvallet Apr 11, 2020

Choose a reason for hiding this comment

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

You could probably get around using kbnDevUtils by simply doing const rootPath = Path.resolve(Path.join(__dirname, '..', '..', '..', '..', '..'))

TBH, I'm not a big fan of relative resolves, as they are imho very fragile to refactoring. If we think this detection implementation is not good enough, I would maybe lean to still use '@kbn/dev-utils' as it will keep the repo root logic in a single place. OTOH the only imports of this module we currently have in src/core/server are in test files. So:

  • Is this implementation too fragile and do we want to switch to real absolute path checking? (I'm good with that)
  • If we do, should we use @kbn/dev-utils's REPO_ROOT or the const rootPath = Path.resolve(Path.join(__dirname, '..', '..', '..', '..', '..')) alternative?

Also we may want to make the xpack path more specific x-pack/legacy/plugins

That would be great, except there is technically only ONE xpack plugin/plugin pack, which is x-pack/index.js.

kibana/x-pack/index.js

Lines 36 to 42 in 982c0da

module.exports = function(kibana) {
return [
xpackMain(kibana),
graph(kibana),
monitoring(kibana),
reporting(kibana),
spaces(kibana),

We don't have any information on the plugins this pack loads. I can change the xpack check to a regexp check to /x-pack$ (or to strict absolute path checking depending on the answer of my comment's first point) however, to only target the 'real' x-pack plugin and allow to log warnings for functional test plugins.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Is this implementation too fragile and do we want to switch to real absolute path checking? (I'm good with that)
  • If we do, should we use @kbn/dev-utils's REPO_ROOT or the const rootPath = Path.resolve(Path.join(__dirname, '..', '..', '..', '..', '..')) alternative?

Good point on the brittleness of refactoring. Though it's unlikely we'll refactor or move any of this legacy code in the near future, let's not create this situation if avoidable.

My main reason for not using @kbn/dev-utils is the fact that we don't use that module in any production code except in src/cli.

Given those two points, let's just keep what you have.

We don't have any information on the plugins this pack loads. I can change the xpack check to a regexp check to /x-pack$ (or to strict absolute path checking depending on the answer of my comment's first point) however, to only target the 'real' x-pack plugin and allow to log warnings for functional test plugins.

I forgot about the x-pack mega plugin pattern in legacy. I'm leaning towards just leaving as-is. There is only one plugin in x-pack/test/plugin_functional and it's a KP plugin, so this warning wouldn't apply anyways.

Copy link
Contributor

@mshustov mshustov Apr 14, 2020

Choose a reason for hiding this comment

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

you can use

const { fromRoot } = require('../../core/server/utils');
fromRoot()

to get absolute path to the root

Also we may want to make the xpack path more specific x-pack/legacy/plugins so that any plugins loaded in functional tests /x-pack/test/

Why we cannot reach out their codeowners directly?

1 change: 1 addition & 0 deletions src/core/server/legacy/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export interface LegacyPluginSpec {
getExpectedKibanaVersion: () => string;
getConfigPrefix: () => string;
getDeprecationsProvider: () => LegacyConfigDeprecationProvider | undefined;
getPack: () => LegacyPluginPack;
}

/**
Expand Down
10 changes: 5 additions & 5 deletions src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -2344,11 +2344,11 @@ export const validBodyOutput: readonly ["data", "stream"];
// Warnings were encountered during analysis:
//
// src/core/server/http/router/response.ts:316:3 - (ae-forgotten-export) The symbol "KibanaResponse" needs to be exported by the entry point index.d.ts
// src/core/server/legacy/types.ts:162:3 - (ae-forgotten-export) The symbol "VarsProvider" needs to be exported by the entry point index.d.ts
// src/core/server/legacy/types.ts:163:3 - (ae-forgotten-export) The symbol "VarsReplacer" needs to be exported by the entry point index.d.ts
// src/core/server/legacy/types.ts:164:3 - (ae-forgotten-export) The symbol "LegacyNavLinkSpec" needs to be exported by the entry point index.d.ts
// src/core/server/legacy/types.ts:165:3 - (ae-forgotten-export) The symbol "LegacyAppSpec" needs to be exported by the entry point index.d.ts
// src/core/server/legacy/types.ts:166:16 - (ae-forgotten-export) The symbol "LegacyPluginSpec" needs to be exported by the entry point index.d.ts
// src/core/server/legacy/types.ts:163:3 - (ae-forgotten-export) The symbol "VarsProvider" needs to be exported by the entry point index.d.ts
// src/core/server/legacy/types.ts:164:3 - (ae-forgotten-export) The symbol "VarsReplacer" needs to be exported by the entry point index.d.ts
// src/core/server/legacy/types.ts:165:3 - (ae-forgotten-export) The symbol "LegacyNavLinkSpec" needs to be exported by the entry point index.d.ts
// src/core/server/legacy/types.ts:166:3 - (ae-forgotten-export) The symbol "LegacyAppSpec" needs to be exported by the entry point index.d.ts
// src/core/server/legacy/types.ts:167:16 - (ae-forgotten-export) The symbol "LegacyPluginSpec" needs to be exported by the entry point index.d.ts
// src/core/server/plugins/plugins_service.ts:47:5 - (ae-forgotten-export) The symbol "InternalPluginInfo" needs to be exported by the entry point index.d.ts
// src/core/server/plugins/types.ts:230:3 - (ae-forgotten-export) The symbol "KibanaConfigType" needs to be exported by the entry point index.d.ts
// src/core/server/plugins/types.ts:230:3 - (ae-forgotten-export) The symbol "SharedGlobalConfigKeys" needs to be exported by the entry point index.d.ts
Expand Down