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

move oss features registration to KP #66524

Merged
merged 11 commits into from
May 15, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -107,4 +107,5 @@ export const savedObjectsServiceMock = {
createInternalStartContract: createInternalStartContractMock,
createStartContract: createStartContractMock,
createMigrationContext: migrationMocks.createContext,
createTypeRegistryMock: typeRegistryMock.create,
};
5 changes: 1 addition & 4 deletions x-pack/legacy/plugins/xpack_main/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,7 @@ export const xpackMain = kibana => {

mirrorPluginStatus(server.plugins.elasticsearch, this, 'yellow', 'red');

featuresPlugin.registerLegacyAPI({
xpackInfo: setupXPackMain(server),
savedObjectTypes: server.savedObjects.types,
});
setupXPackMain(server);

// register routes
xpackInfoRoute(server);
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/endpoint/server/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ describe('test endpoint plugin', () => {
registerFeature: jest.fn(),
getFeatures: jest.fn(),
getFeaturesUICapabilities: jest.fn(),
registerLegacyAPI: jest.fn(),
};
});

Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/features/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"id": "features",
"version": "8.0.0",
"kibanaVersion": "kibana",
"requiredPlugins": ["licensing"],
"optionalPlugins": ["visTypeTimelion"],
"configPath": ["xpack", "features"],
"server": true,
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/features/server/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ const createSetup = (): jest.Mocked<PluginSetupContract> => {
getFeatures: jest.fn(),
getFeaturesUICapabilities: jest.fn(),
registerFeature: jest.fn(),
registerLegacyAPI: jest.fn(),
};
};

Expand Down
97 changes: 97 additions & 0 deletions x-pack/plugins/features/server/plugin.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { coreMock, savedObjectsServiceMock } from 'src/core/server/mocks';

import { Plugin } from './plugin';
const initContext = coreMock.createPluginInitializerContext();
const coreSetup = coreMock.createSetup();
const coreStart = coreMock.createStart();
const typeRegistry = savedObjectsServiceMock.createTypeRegistryMock();
typeRegistry.getAllTypes.mockReturnValue([
{
name: 'foo',
hidden: false,
mappings: { properties: {} },
namespaceType: 'single' as 'single',
},
{
name: 'bar',
hidden: true,
mappings: { properties: {} },
namespaceType: 'agnostic' as 'agnostic',
},
]);
coreStart.savedObjects.getTypeRegistry.mockReturnValue(typeRegistry);

describe('Features Plugin', () => {
it('returns OSS + registered features', async () => {
const plugin = new Plugin(initContext);
const { registerFeature } = await plugin.setup(coreSetup, {});
registerFeature({
id: 'baz',
name: 'baz',
app: [],
privileges: null,
});

const { getFeatures } = await plugin.start(coreStart);

expect(getFeatures().map(f => f.id)).toMatchInlineSnapshot(`
Array [
"baz",
"discover",
"visualize",
"dashboard",
"dev_tools",
"advancedSettings",
"indexPatterns",
"savedObjectsManagement",
]
`);
});

it('returns OSS + registered features with timelion when available', async () => {
const plugin = new Plugin(initContext);
const { registerFeature } = await plugin.setup(coreSetup, {
visTypeTimelion: { uiEnabled: true },
});
registerFeature({
id: 'baz',
name: 'baz',
app: [],
privileges: null,
});

const { getFeatures } = await plugin.start(coreStart);

expect(getFeatures().map(f => f.id)).toMatchInlineSnapshot(`
Array [
"baz",
"discover",
"visualize",
"dashboard",
"dev_tools",
"advancedSettings",
"indexPatterns",
"savedObjectsManagement",
"timelion",
]
`);
});

it('registers not hidden saved objects types', async () => {
const plugin = new Plugin(initContext);
await plugin.setup(coreSetup, {});
const { getFeatures } = await plugin.start(coreStart);

const soTypes =
getFeatures().find(f => f.id === 'savedObjectsManagement')?.privileges?.all.savedObject.all ||
[];

expect(soTypes.includes('foo')).toBe(true);
expect(soTypes.includes('bar')).toBe(false);
});
});
72 changes: 35 additions & 37 deletions x-pack/plugins/features/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@

import {
CoreSetup,
CoreStart,
SavedObjectsServiceStart,
Logger,
PluginInitializerContext,
RecursiveReadonly,
} from '../../../../src/core/server';
import { Capabilities as UICapabilities } from '../../../../src/core/server';
import { deepFreeze } from '../../../../src/core/server';
import { XPackInfo } from '../../../legacy/plugins/xpack_main/server/lib/xpack_info';
import { PluginSetupContract as TimelionSetupContract } from '../../../../src/plugins/vis_type_timelion/server';
import { FeatureRegistry } from './feature_registry';
import { Feature, FeatureConfig } from '../common/feature';
Expand All @@ -25,39 +26,26 @@ import { defineRoutes } from './routes';
*/
export interface PluginSetupContract {
registerFeature(feature: FeatureConfig): void;
/*
* Calling this function during setup will crash Kibana.
* Use start contract instead.
* @deprecated
* */
getFeatures(): Feature[];
getFeaturesUICapabilities(): UICapabilities;
registerLegacyAPI: (legacyAPI: LegacyAPI) => void;
}

export interface PluginStartContract {
getFeatures(): Feature[];
}

/**
* Describes a set of APIs that are available in the legacy platform only and required by this plugin
* to function properly.
*/
export interface LegacyAPI {
xpackInfo: Pick<XPackInfo, 'license'>;
savedObjectTypes: string[];
}

/**
* Represents Features Plugin instance that will be managed by the Kibana plugin system.
*/
export class Plugin {
private readonly logger: Logger;

private readonly featureRegistry: FeatureRegistry = new FeatureRegistry();

private legacyAPI?: LegacyAPI;
private readonly getLegacyAPI = () => {
if (!this.legacyAPI) {
throw new Error('Legacy API is not registered!');
}
return this.legacyAPI;
};
private isTimelionEnabled: boolean = false;

constructor(private readonly initializerContext: PluginInitializerContext) {
this.logger = this.initializerContext.logger.get();
Expand All @@ -67,39 +55,49 @@ export class Plugin {
core: CoreSetup,
{ visTypeTimelion }: { visTypeTimelion?: TimelionSetupContract }
): Promise<RecursiveReadonly<PluginSetupContract>> {
this.isTimelionEnabled = visTypeTimelion !== undefined && visTypeTimelion.uiEnabled;

defineRoutes({
router: core.http.createRouter(),
featureRegistry: this.featureRegistry,
getLegacyAPI: this.getLegacyAPI,
});

return deepFreeze({
registerFeature: this.featureRegistry.register.bind(this.featureRegistry),
getFeatures: this.featureRegistry.getAll.bind(this.featureRegistry),
getFeaturesUICapabilities: () => uiCapabilitiesForFeatures(this.featureRegistry.getAll()),

registerLegacyAPI: (legacyAPI: LegacyAPI) => {
this.legacyAPI = legacyAPI;

// Register OSS features.
for (const feature of buildOSSFeatures({
savedObjectTypes: this.legacyAPI.savedObjectTypes,
includeTimelion: visTypeTimelion !== undefined && visTypeTimelion.uiEnabled,
})) {
this.featureRegistry.register(feature);
}
},
});
}

public start(): RecursiveReadonly<PluginStartContract> {
this.logger.debug('Starting plugin');
public start(core: CoreStart): RecursiveReadonly<PluginStartContract> {
this.registerOssFeatures(core.savedObjects);

return deepFreeze({
getFeatures: this.featureRegistry.getAll.bind(this.featureRegistry),
});
}

public stop() {
this.logger.debug('Stopping plugin');
public stop() {}

private registerOssFeatures(savedObjects: SavedObjectsServiceStart) {
const registry = savedObjects.getTypeRegistry();
const savedObjectTypes = registry
.getAllTypes()
.filter(t => !t.hidden)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pgayvallet I wrongly assumed that getAllTypes should return already filtered types, as savedObjects.type did in LP. Do we consider it's a problem that a plugin can expose all types to a consumer?

Copy link
Contributor

Choose a reason for hiding this comment

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

When we use the type registry in the repository we need to know all types, but I think consumers of this API should usually only operate on types that are visible so I think it would make sense to make that the default. If we have a use case where this should be exposed outside of core we could have an option like includedHiddenTypes: true

Copy link
Contributor

@pgayvallet pgayvallet May 15, 2020

Choose a reason for hiding this comment

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

I named it getAllTypes in an attempt to distinguish it from the legacy types, but it's probably not enough and jsdoc is lacking.

const service = {
types: visibleTypes,

It shouldn't be a problem for a plugin to access / expose all types to a consumer. AFAIK, hidden types were already consumed in legacy, but for different usages, via the schema property that contains both visible and hidden types. Also I think some consumers need to access all types, even if I can't remember exactly for which usages, probably security/spaces)

However I agree that 1/ the jsdoc for getAllTypes should be more explicit, and 2/ the type registry should have a getVisibleTypes method.

I will create a issue for that improvement

Copy link
Contributor

Choose a reason for hiding this comment

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

created #66818

.map(t => t.name);

this.logger.debug(
`Registering OSS features with SO types: ${savedObjectTypes.join(', ')}. "includeTimelion": ${
this.isTimelionEnabled
}.`
);
const features = buildOSSFeatures({
savedObjectTypes,
includeTimelion: this.isTimelionEnabled,
});

for (const feature of features) {
this.featureRegistry.register(feature);
}
}
}
46 changes: 23 additions & 23 deletions x-pack/plugins/features/server/routes/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,20 @@
import { FeatureRegistry } from '../feature_registry';
import { defineRoutes } from './index';

import { httpServerMock, httpServiceMock } from '../../../../../src/core/server/mocks';
import { XPackInfoLicense } from '../../../../legacy/plugins/xpack_main/server/lib/xpack_info_license';
import { httpServerMock, httpServiceMock, coreMock } from '../../../../../src/core/server/mocks';
import { LicenseType } from '../../../licensing/server/';
import { licensingMock } from '../../../licensing/server/mocks';
import { RequestHandler } from '../../../../../src/core/server';
import { FeatureConfig } from '../../common';

let currentLicenseLevel: string = 'gold';
function createContextMock(licenseType: LicenseType = 'gold') {
return {
core: coreMock.createRequestHandlerContext(),
licensing: {
license: licensingMock.createLicense({ license: { type: licenseType } }),
},
};
}

describe('GET /api/features', () => {
let routeHandler: RequestHandler<any, any, any>;
Expand Down Expand Up @@ -53,24 +61,14 @@ describe('GET /api/features', () => {
defineRoutes({
router: routerMock,
featureRegistry,
getLegacyAPI: () => ({
xpackInfo: {
license: {
isOneOf(candidateLicenses: string[]) {
return candidateLicenses.includes(currentLicenseLevel);
},
} as XPackInfoLicense,
},
savedObjectTypes: [],
}),
});

routeHandler = routerMock.get.mock.calls[0][1];
});

it('returns a list of available features, sorted by their configured order', async () => {
const mockResponse = httpServerMock.createResponseFactory();
routeHandler(undefined as any, { query: {} } as any, mockResponse);
routeHandler(createContextMock(), { query: {} } as any, mockResponse);

expect(mockResponse.ok).toHaveBeenCalledTimes(1);
const [call] = mockResponse.ok.mock.calls;
Expand Down Expand Up @@ -98,10 +96,8 @@ describe('GET /api/features', () => {
});

it(`by default does not return features that arent allowed by current license`, async () => {
currentLicenseLevel = 'basic';

const mockResponse = httpServerMock.createResponseFactory();
routeHandler(undefined as any, { query: {} } as any, mockResponse);
routeHandler(createContextMock('basic'), { query: {} } as any, mockResponse);

expect(mockResponse.ok).toHaveBeenCalledTimes(1);
const [call] = mockResponse.ok.mock.calls;
Expand All @@ -126,10 +122,12 @@ describe('GET /api/features', () => {
});

it(`ignoreValidLicenses=false does not return features that arent allowed by current license`, async () => {
currentLicenseLevel = 'basic';

const mockResponse = httpServerMock.createResponseFactory();
routeHandler(undefined as any, { query: { ignoreValidLicenses: false } } as any, mockResponse);
routeHandler(
createContextMock('basic'),
{ query: { ignoreValidLicenses: false } } as any,
mockResponse
);

expect(mockResponse.ok).toHaveBeenCalledTimes(1);
const [call] = mockResponse.ok.mock.calls;
Expand All @@ -154,10 +152,12 @@ describe('GET /api/features', () => {
});

it(`ignoreValidLicenses=true returns features that arent allowed by current license`, async () => {
currentLicenseLevel = 'basic';

const mockResponse = httpServerMock.createResponseFactory();
routeHandler(undefined as any, { query: { ignoreValidLicenses: true } } as any, mockResponse);
routeHandler(
createContextMock('basic'),
{ query: { ignoreValidLicenses: true } } as any,
mockResponse
);

expect(mockResponse.ok).toHaveBeenCalledTimes(1);
const [call] = mockResponse.ok.mock.calls;
Expand Down
7 changes: 3 additions & 4 deletions x-pack/plugins/features/server/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import { schema } from '@kbn/config-schema';
import { IRouter } from '../../../../../src/core/server';
import { LegacyAPI } from '../plugin';
import { FeatureRegistry } from '../feature_registry';

/**
Expand All @@ -15,10 +14,9 @@ import { FeatureRegistry } from '../feature_registry';
export interface RouteDefinitionParams {
router: IRouter;
featureRegistry: FeatureRegistry;
getLegacyAPI: () => LegacyAPI;
}

export function defineRoutes({ router, featureRegistry, getLegacyAPI }: RouteDefinitionParams) {
export function defineRoutes({ router, featureRegistry }: RouteDefinitionParams) {
router.get(
{
path: '/api/features',
Expand All @@ -37,7 +35,8 @@ export function defineRoutes({ router, featureRegistry, getLegacyAPI }: RouteDef
request.query.ignoreValidLicenses ||
!feature.validLicenses ||
!feature.validLicenses.length ||
getLegacyAPI().xpackInfo.license.isOneOf(feature.validLicenses)
(context.licensing!.license.type &&
Copy link
Contributor

@FrankHassanabad FrankHassanabad May 15, 2020

Choose a reason for hiding this comment

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

Did you want the ! or the ? type here? Isn't ? the safe one where instead of turning off the type assertion it will resolve all of this as null/undefined rather than causing a null pointer exception.

Copy link
Contributor Author

@mshustov mshustov May 15, 2020

Choose a reason for hiding this comment

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

I want ! here. License is always available, otherwise features plugin won't start without licensing plugin. There is no way in our typings to mark a field as required.

feature.validLicenses.includes(context.licensing!.license.type))
)
.sort(
(f1, f2) =>
Expand Down