Skip to content

Commit

Permalink
features plugin: improve capabilities caching (#153743)
Browse files Browse the repository at this point in the history 
## Summary

- Avoid constructing the capabilities passed to core's capability
provider for each request
- Add proper *explicit* lock mechanism to the feature registry
- Some other minor cleanup, probably

#### Before:

(look at the `getFeatureUICapabilities` block)

<img width="1629" alt="CleanShot 2023-03-26 at 09 55 45@2x"
src="https://user-images.githubusercontent.com/1532934/227951476-1edbd2bc-f60b-4529-8c22-ff0d9511b2b8.png">

#### After

<img width="1640" alt="CleanShot 2023-03-27 at 14 58 31@2x"
src="https://user-images.githubusercontent.com/1532934/227951533-dbe675c3-7089-4b36-86a9-584a3e97c860.png">
  • Loading branch information
@pgayvallet
pgayvallet authored Mar 28, 2023
1 parent b64dc95 commit 8ce2059
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 14 deletions.
35 changes: 34 additions & 1 deletion x-pack/plugins/features/server/feature_registry.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ describe('FeatureRegistry', () => {

const featureRegistry = new FeatureRegistry();
featureRegistry.registerKibanaFeature(feature);
featureRegistry.lockRegistration();
const result = featureRegistry.getAllKibanaFeatures();
expect(result).toHaveLength(1);

Expand Down Expand Up @@ -137,6 +138,7 @@ describe('FeatureRegistry', () => {

const featureRegistry = new FeatureRegistry();
featureRegistry.registerKibanaFeature(feature);
featureRegistry.lockRegistration();
const result = featureRegistry.getAllKibanaFeatures();
expect(result).toHaveLength(1);

Expand Down Expand Up @@ -278,6 +280,7 @@ describe('FeatureRegistry', () => {

const featureRegistry = new FeatureRegistry();
featureRegistry.registerKibanaFeature(feature);
featureRegistry.lockRegistration();
const result = featureRegistry.getAllKibanaFeatures();

expect(result[0].privileges).toHaveProperty('all');
Expand Down Expand Up @@ -313,6 +316,7 @@ describe('FeatureRegistry', () => {

const featureRegistry = new FeatureRegistry();
featureRegistry.registerKibanaFeature(feature);
featureRegistry.lockRegistration();
const result = featureRegistry.getAllKibanaFeatures();

expect(result[0].privileges).toHaveProperty('all');
Expand Down Expand Up @@ -350,6 +354,7 @@ describe('FeatureRegistry', () => {

const featureRegistry = new FeatureRegistry();
featureRegistry.registerKibanaFeature(feature);
featureRegistry.lockRegistration();
const result = featureRegistry.getAllKibanaFeatures();

const reservedPrivilege = result[0]!.reserved!.privileges[0].privilege;
Expand Down Expand Up @@ -383,6 +388,7 @@ describe('FeatureRegistry', () => {

const featureRegistry = new FeatureRegistry();
featureRegistry.registerKibanaFeature(feature);
featureRegistry.lockRegistration();
const result = featureRegistry.getAllKibanaFeatures();

expect(result[0].privileges).toHaveProperty('all');
Expand Down Expand Up @@ -1645,6 +1651,7 @@ describe('FeatureRegistry', () => {

const featureRegistry = new FeatureRegistry();
featureRegistry.registerKibanaFeature(feature);
featureRegistry.lockRegistration();
const result = featureRegistry.getAllKibanaFeatures();
expect(result).toHaveLength(1);
expect(result[0].reserved?.privileges).toHaveLength(2);
Expand Down Expand Up @@ -1802,7 +1809,7 @@ describe('FeatureRegistry', () => {
`);
});

it('cannot register feature after getAll has been called', () => {
it('cannot register kibana feature after lockRegistration has been called', () => {
const feature1: KibanaFeatureConfig = {
id: 'test-feature',
name: 'Test Feature',
Expand All @@ -1820,13 +1827,15 @@ describe('FeatureRegistry', () => {

const featureRegistry = new FeatureRegistry();
featureRegistry.registerKibanaFeature(feature1);
featureRegistry.lockRegistration();
featureRegistry.getAllKibanaFeatures();
expect(() => {
featureRegistry.registerKibanaFeature(feature2);
}).toThrowErrorMatchingInlineSnapshot(
`"Features are locked, can't register new features. Attempt to register test-feature-2 failed."`
);
});

describe('#getAllKibanaFeatures', () => {
const features: KibanaFeatureConfig[] = [
{
Expand Down Expand Up @@ -1879,6 +1888,7 @@ describe('FeatureRegistry', () => {

const registry = new FeatureRegistry();
features.forEach((f) => registry.registerKibanaFeature(f));
registry.lockRegistration();

it('returns all features and sub-feature privileges by default', () => {
const result = registry.getAllKibanaFeatures();
Expand Down Expand Up @@ -1926,6 +1936,7 @@ describe('FeatureRegistry', () => {

const featureRegistry = new FeatureRegistry();
featureRegistry.registerElasticsearchFeature(feature);
featureRegistry.lockRegistration();
const result = featureRegistry.getAllElasticsearchFeatures();
expect(result).toHaveLength(1);

Expand Down Expand Up @@ -1962,6 +1973,7 @@ describe('FeatureRegistry', () => {

const featureRegistry = new FeatureRegistry();
featureRegistry.registerElasticsearchFeature(feature);
featureRegistry.lockRegistration();
const result = featureRegistry.getAllElasticsearchFeatures();
expect(result).toHaveLength(1);

Expand Down Expand Up @@ -2015,6 +2027,27 @@ describe('FeatureRegistry', () => {
featureRegistry.registerElasticsearchFeature(feature)
).toThrowErrorMatchingInlineSnapshot(`"Feature with id test-feature is already registered."`);
});

it('cannot register elasticsearch feature after lockRegistration has been called', () => {
const feature: ElasticsearchFeatureConfig = {
id: 'test-feature',
privileges: [
{
requiredClusterPrivileges: ['all'],
ui: [],
},
],
};

const featureRegistry = new FeatureRegistry();
featureRegistry.lockRegistration();

expect(() =>
featureRegistry.registerElasticsearchFeature(feature)
).toThrowErrorMatchingInlineSnapshot(
`"Features are locked, can't register new features. Attempt to register test-feature failed."`
);
});
});

it('does not allow a Kibana feature to share an id with an Elasticsearch feature', () => {
Expand Down
14 changes: 12 additions & 2 deletions x-pack/plugins/features/server/feature_registry.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ export class FeatureRegistry {
private kibanaFeatures: Record<string, KibanaFeatureConfig> = {};
private esFeatures: Record<string, ElasticsearchFeatureConfig> = {};

public lockRegistration() {
this.locked = true;
}

public registerKibanaFeature(feature: KibanaFeatureConfig) {
if (this.locked) {
throw new Error(
Expand Down Expand Up @@ -58,7 +62,10 @@ export class FeatureRegistry {
}

public getAllKibanaFeatures(license?: ILicense, ignoreLicense = false): KibanaFeature[] {
this.locked = true;
if (!this.locked) {
throw new Error('Cannot retrieve Kibana features while registration is still open');
}

let features = Object.values(this.kibanaFeatures);

const performLicenseCheck = license && !ignoreLicense;
Expand All @@ -84,7 +91,10 @@ export class FeatureRegistry {
}

public getAllElasticsearchFeatures(): ElasticsearchFeature[] {
this.locked = true;
if (!this.locked) {
throw new Error('Cannot retrieve elasticsearch features while registration is still open');
}

return Object.values(this.esFeatures).map(
(featureConfig) => new ElasticsearchFeature(featureConfig)
);
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/features/server/mocks.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ const createSetup = (): jest.Mocked<PluginSetupContract> => {
return {
getKibanaFeatures: jest.fn(),
getElasticsearchFeatures: jest.fn(),
getFeaturesUICapabilities: jest.fn(),
registerKibanaFeature: jest.fn(),
registerElasticsearchFeature: jest.fn(),
enableReportingUiCapabilities: jest.fn(),
Expand Down
24 changes: 14 additions & 10 deletions x-pack/plugins/features/server/plugin.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import {
Logger,
Plugin,
PluginInitializerContext,
Capabilities as UICapabilities,
} from '@kbn/core/server';
import { Capabilities as UICapabilities } from '@kbn/core/server';
import { FeatureRegistry } from './feature_registry';
import { uiCapabilitiesForFeatures } from './ui_capabilities_for_features';
import { buildOSSFeatures } from './oss_features';
Expand All @@ -40,22 +40,24 @@ import {
*/
export interface PluginSetupContract {
registerKibanaFeature(feature: KibanaFeatureConfig): void;

registerElasticsearchFeature(feature: ElasticsearchFeatureConfig): void;

/**
* Calling this function during setup will crash Kibana.
* Use start contract instead.
* @deprecated
* @removeBy 8.8.0
*/
getKibanaFeatures(): KibanaFeature[];

/**
* Calling this function during setup will crash Kibana.
* Use start contract instead.
* @deprecated
* @removeBy 8.8.0
*/
getElasticsearchFeatures(): ElasticsearchFeature[];
getFeaturesUICapabilities(): UICapabilities;

/**
* In the future, OSS features should register their own subfeature
Expand All @@ -80,6 +82,7 @@ export interface PluginSetupContract {

export interface PluginStartContract {
getElasticsearchFeatures(): ElasticsearchFeature[];

getKibanaFeatures(): KibanaFeature[];
}

Expand All @@ -92,6 +95,7 @@ export class FeaturesPlugin
private readonly logger: Logger;
private readonly featureRegistry: FeatureRegistry = new FeatureRegistry();
private isReportingEnabled: boolean = false;
private capabilities?: UICapabilities;

constructor(private readonly initializerContext: PluginInitializerContext) {
this.logger = this.initializerContext.logger.get();
Expand All @@ -103,13 +107,8 @@ export class FeaturesPlugin
featureRegistry: this.featureRegistry,
});

const getFeaturesUICapabilities = () =>
uiCapabilitiesForFeatures(
this.featureRegistry.getAllKibanaFeatures(),
this.featureRegistry.getAllElasticsearchFeatures()
);

core.capabilities.registerProvider(getFeaturesUICapabilities);
// capabilities provider wil never be called before plugin start
core.capabilities.registerProvider(() => this.capabilities!);

return deepFreeze({
registerKibanaFeature: this.featureRegistry.registerKibanaFeature.bind(this.featureRegistry),
Expand All @@ -120,7 +119,6 @@ export class FeaturesPlugin
getElasticsearchFeatures: this.featureRegistry.getAllElasticsearchFeatures.bind(
this.featureRegistry
),
getFeaturesUICapabilities,
enableReportingUiCapabilities: this.enableReportingUiCapabilities.bind(this),
featurePrivilegeIterator,
subFeaturePrivilegeIterator,
Expand All @@ -129,6 +127,12 @@ export class FeaturesPlugin

public start(core: CoreStart): RecursiveReadonly<PluginStartContract> {
this.registerOssFeatures(core.savedObjects);
this.featureRegistry.lockRegistration();

this.capabilities = uiCapabilitiesForFeatures(
this.featureRegistry.getAllKibanaFeatures(),
this.featureRegistry.getAllElasticsearchFeatures()
);

return deepFreeze({
getElasticsearchFeatures: this.featureRegistry.getAllElasticsearchFeatures.bind(
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/features/server/routes/index.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ describe('GET /api/features', () => {
privileges: null,
});

featureRegistry.lockRegistration();

const routerMock = httpServiceMock.createRouter();
defineRoutes({
router: routerMock,
Expand Down

0 comments on commit 8ce2059

Please sign in to comment.