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

Migrate CSP usage collector to kibana_usage_collection plugin #75536

Merged
Merged
Show file tree
Hide file tree
Changes from 5 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
6 changes: 0 additions & 6 deletions src/legacy/core_plugins/kibana/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import Fs from 'fs';
import { promisify } from 'util';

import { getUiSettingDefaults } from './server/ui_setting_defaults';
import { registerCspCollector } from './server/lib/csp_usage_collector';

const mkdirAsync = promisify(Fs.mkdir);

Expand Down Expand Up @@ -53,10 +52,5 @@ export default function (kibana) {
throw err;
}
},

init: async function (server) {
const { usageCollection } = server.newPlatform.setup.plugins;
registerCspCollector(usageCollection, server);
},
});
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -17,35 +17,24 @@
* under the License.
*/

import { CspConfig, ICspConfig } from '../../../../../../core/server';
import { CspConfig, ICspConfig } from '../../../../../core/server';
import { createCspCollector } from './csp_collector';

const createMockKbnServer = () => ({
newPlatform: {
setup: {
core: {
http: {
csp: new CspConfig(),
},
},
},
},
});
import { httpServiceMock } from '../../../../../core/server/mocks';

describe('csp collector', () => {
let kbnServer: ReturnType<typeof createMockKbnServer>;
let httpMock: ReturnType<typeof httpServiceMock.createSetupContract>;
const mockCallCluster = null as any;

function updateCsp(config: Partial<ICspConfig>) {
kbnServer.newPlatform.setup.core.http.csp = new CspConfig(config);
httpMock.csp = new CspConfig(config);
}

beforeEach(() => {
kbnServer = createMockKbnServer();
httpMock = httpServiceMock.createSetupContract();
});

test('fetches whether strict mode is enabled', async () => {
const collector = createCspCollector(kbnServer as any);
const collector = createCspCollector(httpMock);

expect((await collector.fetch(mockCallCluster)).strict).toEqual(true);

Expand All @@ -54,7 +43,7 @@ describe('csp collector', () => {
});

test('fetches whether the legacy browser warning is enabled', async () => {
const collector = createCspCollector(kbnServer as any);
const collector = createCspCollector(httpMock);

expect((await collector.fetch(mockCallCluster)).warnLegacyBrowsers).toEqual(true);

Expand All @@ -63,7 +52,7 @@ describe('csp collector', () => {
});

test('fetches whether the csp rules have been changed or not', async () => {
const collector = createCspCollector(kbnServer as any);
const collector = createCspCollector(httpMock);

expect((await collector.fetch(mockCallCluster)).rulesChangedFromDefault).toEqual(false);

Expand All @@ -72,7 +61,7 @@ describe('csp collector', () => {
});

test('does not include raw csp rules under any property names', async () => {
const collector = createCspCollector(kbnServer as any);
const collector = createCspCollector(httpMock);

// It's important that we do not send the value of csp.rules here as it
// can be customized with values that can be identifiable to given
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,21 @@
* under the License.
*/

import { Server } from 'hapi';
import { CspConfig } from '../../../../../../core/server';
import {
UsageCollectionSetup,
CollectorOptions,
} from '../../../../../../plugins/usage_collection/server';
import { UsageCollectionSetup, CollectorOptions } from 'src/plugins/usage_collection/server';
import { HttpServiceSetup, CspConfig } from '../../../../../core/server';

interface Usage {
strict: boolean;
warnLegacyBrowsers: boolean;
rulesChangedFromDefault: boolean;
}

export function createCspCollector(server: Server): CollectorOptions<Usage> {
export function createCspCollector(http: HttpServiceSetup): CollectorOptions<Usage> {
return {
type: 'csp',
isReady: () => true,
async fetch() {
const { strict, warnLegacyBrowsers, header } = server.newPlatform.setup.core.http.csp;
const { strict, warnLegacyBrowsers, header } = http.csp;

return {
strict,
Expand All @@ -60,8 +56,11 @@ export function createCspCollector(server: Server): CollectorOptions<Usage> {
};
}

export function registerCspCollector(usageCollection: UsageCollectionSetup, server: Server): void {
const collectorConfig = createCspCollector(server);
const collector = usageCollection.makeUsageCollector<Usage>(collectorConfig);
export function registerCspCollector(
usageCollection: UsageCollectionSetup,
http: HttpServiceSetup
): void {
const collectorOptions = createCspCollector(http);
const collector = usageCollection.makeUsageCollector<Usage>(collectorOptions);
usageCollection.registerCollector(collector);
Comment on lines +63 to 65
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scripts/telemetry_check.js was kinda picky on the syntax I had to use here: I was not allowed to directly call usageCollection.makeUsageCollector(createCspCollector(http)) and I was forced to manually specify the <Usage> type that is already implicitly defined when calling makeUsageCollector

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... we are working on improving it to be more developer-friendly. Thank you for sharing your frustration about this 🙂
cc/ @Bamieh

}
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ export { registerManagementUsageCollector } from './management';
export { registerApplicationUsageCollector } from './application_usage';
export { registerKibanaUsageCollector } from './kibana';
export { registerOpsStatsCollector } from './ops_stats';
export { registerCspCollector } from './csp';
12 changes: 6 additions & 6 deletions src/plugins/kibana_usage_collection/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
registerManagementUsageCollector,
registerOpsStatsCollector,
registerUiMetricUsageCollector,
registerCspCollector,
} from './collectors';

interface KibanaUsageCollectionPluginsDepsSetup {
Expand All @@ -56,12 +57,9 @@ export class KibanaUsageCollectionPlugin implements Plugin {
this.metric$ = new Subject<OpsMetrics>();
}

public setup(
{ savedObjects }: CoreSetup,
{ usageCollection }: KibanaUsageCollectionPluginsDepsSetup
) {
this.registerUsageCollectors(usageCollection, this.metric$, (opts) =>
savedObjects.registerType(opts)
public setup(coreSetup: CoreSetup, { usageCollection }: KibanaUsageCollectionPluginsDepsSetup) {
this.registerUsageCollectors(usageCollection, coreSetup, this.metric$, (opts) =>
coreSetup.savedObjects.registerType(opts)
);
}

Expand All @@ -79,6 +77,7 @@ export class KibanaUsageCollectionPlugin implements Plugin {

private registerUsageCollectors(
usageCollection: UsageCollectionSetup,
coreSetup: CoreSetup,
metric$: Subject<OpsMetrics>,
registerType: SavedObjectsRegisterType
) {
Expand All @@ -90,5 +89,6 @@ export class KibanaUsageCollectionPlugin implements Plugin {
registerManagementUsageCollector(usageCollection, getUiSettingsClient);
registerUiMetricUsageCollector(usageCollection, registerType, getSavedObjectsClient);
registerApplicationUsageCollector(usageCollection, registerType, getSavedObjectsClient);
registerCspCollector(usageCollection, coreSetup.http);
}
}
4 changes: 0 additions & 4 deletions test/api_integration/apis/status/status.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,6 @@ export default function ({ getService }) {
expect(body.status.overall.state).to.be('green');

expect(body.status.statuses).to.be.an('array');
const kibanaPlugin = body.status.statuses.find((s) => {
return s.id.indexOf('plugin:kibana') === 0;
});
expect(kibanaPlugin.state).to.be('green');
Comment on lines -42 to -45
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the init function from the legacy kibana plugin removed the plugin entry from the status, so I had to adapt two tests relying on that. As we are very close to remove the plugin totally, I felt like it was better to remove that than to preserve an empty init function.

It's also likely we'll adapt and enhance these tests once we finish the status API migration anyway.


expect(body.metrics.collection_interval_in_millis).to.be.a('number');

Expand Down
8 changes: 0 additions & 8 deletions test/functional/apps/status_page/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import expect from '@kbn/expect';
import { FtrProviderContext } from '../../ftr_provider_context';

export default function ({ getService, getPageObjects }: FtrProviderContext) {
const retry = getService('retry');
const testSubjects = getService('testSubjects');
const PageObjects = getPageObjects(['common']);

Expand All @@ -32,13 +31,6 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
await PageObjects.common.navigateToApp('status_page');
});

it('should show the kibana plugin as ready', async () => {
await retry.tryForTime(6000, async () => {
const text = await testSubjects.getVisibleText('statusBreakdown');
expect(text.indexOf('plugin:kibana')).to.be.above(-1);
});
});

it('should show the build hash and number', async () => {
const buildNumberText = await testSubjects.getVisibleText('statusBuildNumber');
expect(buildNumberText).to.contain('BUILD ');
Expand Down