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

[Reporting] Separate internal and public API endpoints in Reporting #162288

Merged
merged 58 commits into from
Aug 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
51b049a
prep to use internal prefix for APIs
tsullivan Jul 19, 2023
8a859d8
internal and public routes objects
tsullivan Jul 19, 2023
27ae018
UI Api Client mistakenly set everything asSystemRequest
tsullivan Jul 19, 2023
f09c9b7
Use the right route paths
tsullivan Jul 19, 2023
d59f0d8
use the public API endpoint in the POST URL
tsullivan Jul 19, 2023
d1710b5
fix public download path for post response
tsullivan Jul 19, 2023
01c6b5d
fix tests import
tsullivan Jul 19, 2023
8ef4e04
update comment
tsullivan Jul 19, 2023
a6950af
Use more correct endpoint paths in tests
tsullivan Jul 19, 2023
9379f44
Delete endpoint should be public
tsullivan Jul 19, 2023
0594845
reorganize part i
tsullivan Jul 20, 2023
264b644
register public routes
tsullivan Jul 20, 2023
99ba556
fix tests
tsullivan Jul 20, 2023
6d46174
update snapshot
tsullivan Jul 20, 2023
fc30dee
fix tests
tsullivan Jul 20, 2023
664aaa4
Tests
tsullivan Jul 20, 2023
167bc55
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Jul 20, 2023
61c611e
Fix tests
tsullivan Jul 20, 2023
40f4721
fix more tests
tsullivan Jul 20, 2023
7d22b64
replace a string with a variable
tsullivan Jul 20, 2023
457b9f9
Merge branch 'main' into reporting/use-internal-endpoints
tsullivan Jul 24, 2023
7539cef
move common report generation code into RequestHandler.handleGenerate…
tsullivan Jul 24, 2023
24ffa4d
--wip-- [skip ci]
tsullivan Jul 24, 2023
64d4352
Merge branch 'main' into reporting/use-internal-endpoints
tsullivan Jul 26, 2023
8c9dda0
reorganize routes libs
tsullivan Jul 26, 2023
543af19
unskip API usage counter tests
tsullivan Jul 26, 2023
8ce821d
unskip usage counter tests
tsullivan Jul 26, 2023
1b146db
improve jobs routes integration tests
tsullivan Jul 26, 2023
5ccabec
fix generation integration tests
tsullivan Jul 26, 2023
a4dd40b
fix console.log
tsullivan Jul 26, 2023
e1c2d87
Merge branch 'main' into reporting/use-internal-endpoints
tsullivan Jul 26, 2023
0eb8a75
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Jul 26, 2023
1438f06
fix internal generation integration test
tsullivan Jul 26, 2023
d361170
Merge branch 'reporting/use-internal-endpoints' of github.com:tsulliv…
tsullivan Jul 26, 2023
73500d4
add usage counter jest integration tests for generation routes
tsullivan Jul 26, 2023
4688106
remove api_counter functional test
tsullivan Jul 26, 2023
7e6a4d5
update route integration tests with api counter testing
tsullivan Jul 26, 2023
a98eb76
change route objects structures
tsullivan Jul 27, 2023
3f16fd7
Add usage counter test for get ILM status
tsullivan Jul 27, 2023
4273309
add usage counter test for diagnose api
tsullivan Jul 27, 2023
55c1751
cleanup diff
tsullivan Jul 27, 2023
80ae40a
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Jul 27, 2023
8c0371b
fix references
tsullivan Jul 27, 2023
d0794d1
Merge branch 'reporting/use-internal-endpoints' of github.com:tsulliv…
tsullivan Jul 27, 2023
848417d
Merge branch 'main' into reporting/use-internal-endpoints
tsullivan Jul 27, 2023
5cfda74
fix references in tests
tsullivan Jul 27, 2023
540a74b
Merge branch 'main' into reporting/use-internal-endpoints
tsullivan Jul 27, 2023
53ac996
consolidate duplicated code for job management routes
tsullivan Jul 27, 2023
ceff224
Merge branch 'main' into reporting/use-internal-endpoints
tsullivan Jul 27, 2023
4031a4d
Use common job management route handlers
tsullivan Jul 27, 2023
5122ed6
add access:public to public endpoints
tsullivan Jul 27, 2023
760bceb
polish diff
tsullivan Jul 28, 2023
0f68a4c
Merge branch 'main' into reporting/use-internal-endpoints
tsullivan Jul 28, 2023
5afefd4
remove unimportant TODOs
tsullivan Jul 28, 2023
0400bc7
add jest testing for requestHandler.getJobParams
tsullivan Jul 28, 2023
9bbc650
add access:internal option to internal route registrations
tsullivan Jul 28, 2023
44c9f03
Merge branch 'main' into reporting/use-internal-endpoints
tsullivan Jul 28, 2023
248219d
Merge branch 'main' into reporting/use-internal-endpoints
tsullivan Jul 31, 2023
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
22 changes: 4 additions & 18 deletions x-pack/plugins/reporting/common/constants/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
*/

import { CONTENT_TYPE_CSV } from '@kbn/generate-csv/src/constants';
import * as reportTypes from './report_types';
import * as jobTypes from './job_types';
import * as reportTypes from './report_types';

const { PDF_JOB_TYPE, PDF_JOB_TYPE_V2, PNG_JOB_TYPE, PNG_JOB_TYPE_V2 } = jobTypes;

Expand All @@ -28,10 +28,6 @@ export const ALLOWED_JOB_CONTENT_TYPES = [
'text/plain',
];

// Re-export type definitions here for convenience.
export * from './report_types';
export * from './job_types';

type ReportTypeDeclaration = typeof reportTypes;
export type ReportTypes = ReportTypeDeclaration[keyof ReportTypeDeclaration];

Expand Down Expand Up @@ -62,16 +58,6 @@ export const LICENSE_TYPE_GOLD = 'gold' as const;
export const LICENSE_TYPE_PLATINUM = 'platinum' as const;
export const LICENSE_TYPE_ENTERPRISE = 'enterprise' as const;

// Routes
export const API_BASE_URL = '/api/reporting'; // "Generation URL" from share menu
export const API_BASE_GENERATE = `${API_BASE_URL}/generate`;
export const API_LIST_URL = `${API_BASE_URL}/jobs`;
export const API_DIAGNOSE_URL = `${API_BASE_URL}/diagnose`;

export const API_GET_ILM_POLICY_STATUS = `${API_BASE_URL}/ilm_policy_status`;
export const API_MIGRATE_ILM_POLICY_URL = `${API_BASE_URL}/deprecations/migrate_ilm_policy`;
export const API_BASE_URL_V1 = '/api/reporting/v1'; //

export const ILM_POLICY_NAME = 'kibana-reporting';

// Usage counter types
Expand Down Expand Up @@ -111,6 +97,6 @@ export const REPORT_TABLE_ROW_ID = 'reportJobRow';
// intended version is 7.14.0
export const UNVERSIONED_VERSION = '7.14.0';

// hacky endpoint: download CSV without queueing a report
// FIXME: find a way to make these endpoints "generic" instead of hardcoded, as are the queued report export types
export const API_GENERATE_IMMEDIATE = `${API_BASE_URL_V1}/generate/immediate/csv_searchsource`;
export * from './job_types';
export * from './report_types';
export * from './routes';
48 changes: 48 additions & 0 deletions x-pack/plugins/reporting/common/constants/routes.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

const prefixInternalPath = '/internal/reporting';
export const INTERNAL_ROUTES = {
MIGRATE: {
MIGRATE_ILM_POLICY: prefixInternalPath + '/deprecations/migrate_ilm_policy',
GET_ILM_POLICY_STATUS: prefixInternalPath + '/ilm_policy_status',
},
DIAGNOSE: {
BROWSER: prefixInternalPath + '/diagnose/browser',
SCREENSHOT: prefixInternalPath + '/diagnose/screenshot',
},
JOBS: {
COUNT: prefixInternalPath + '/jobs/count',
LIST: prefixInternalPath + '/jobs/list',
INFO_PREFIX: prefixInternalPath + '/jobs/info', // docId is added to the final path
DELETE_PREFIX: prefixInternalPath + '/jobs/delete', // docId is added to the final path
DOWNLOAD_PREFIX: prefixInternalPath + '/jobs/download', // docId is added to the final path
},
DOWNLOAD_CSV: prefixInternalPath + '/generate/immediate/csv_searchsource',
GENERATE_PREFIX: prefixInternalPath + '/generate', // exportTypeId is added to the final path
};

const prefixPublicPath = '/api/reporting';
export const PUBLIC_ROUTES = {
/**
* Public endpoint for POST URL strings and automated report generation
* exportTypeId is added to the final path
*/
GENERATE_PREFIX: prefixPublicPath + `/generate`,
JOBS: {
/**
* Public endpoint used by Watcher and automated report downloads
* jobId is added to the final path
*/
DOWNLOAD_PREFIX: prefixPublicPath + `/jobs/download`,
/**
* Public endpoint potentially used to delete a report after download in automation
* jobId is added to the final path
*/
DELETE_PREFIX: prefixPublicPath + `/jobs/delete`,
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,13 @@
* 2.0.
*/

import { useRequest, UseRequestResponse } from '../../shared_imports';
import { INTERNAL_ROUTES } from '../../../common/constants';
import { IlmPolicyStatusResponse } from '../../../common/types';

import { API_GET_ILM_POLICY_STATUS } from '../../../common/constants';

import { useKibana } from '../../shared_imports';
import { useKibana, useRequest, UseRequestResponse } from '../../shared_imports';

export const useCheckIlmPolicyStatus = (): UseRequestResponse<IlmPolicyStatusResponse> => {
const {
services: { http },
} = useKibana();
return useRequest(http, { path: API_GET_ILM_POLICY_STATUS, method: 'get' });
return useRequest(http, { path: INTERNAL_ROUTES.MIGRATE.GET_ILM_POLICY_STATUS, method: 'get' });
};
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ describe('ReportingAPIClient', () => {
});

describe('getReportURL', () => {
it('should generate report URL', () => {
it('should generate the internal report download URL', () => {
expect(apiClient.getReportURL('123')).toMatchInlineSnapshot(
`"/base/path/api/reporting/jobs/download/123"`
`"/base/path/internal/reporting/jobs/download/123"`
);
});
});
Expand All @@ -52,10 +52,7 @@ describe('ReportingAPIClient', () => {
it('should send a delete request', async () => {
await apiClient.deleteReport('123');

expect(httpClient.delete).toHaveBeenCalledWith(
expect.stringContaining('/delete/123'),
expect.any(Object)
);
expect(httpClient.delete).toHaveBeenCalledWith(expect.stringContaining('/delete/123'));
});
});

Expand Down Expand Up @@ -112,10 +109,7 @@ describe('ReportingAPIClient', () => {
it('should send a get request', async () => {
await apiClient.getInfo('123');

expect(httpClient.get).toHaveBeenCalledWith(
expect.stringContaining('/info/123'),
expect.any(Object)
);
expect(httpClient.get).toHaveBeenCalledWith(expect.stringContaining('/info/123'));
});

it('should return a job instance', async () => {
Expand Down Expand Up @@ -174,7 +168,7 @@ describe('ReportingAPIClient', () => {
describe('getReportingJobPath', () => {
it('should generate a job path', () => {
expect(
apiClient.getReportingJobPath('pdf', {
apiClient.getReportingPublicJobPath('pdf', {
browserTimezone: 'UTC',
objectType: 'something',
title: 'some title',
Expand Down Expand Up @@ -293,21 +287,15 @@ describe('ReportingAPIClient', () => {
it('should send a post request', async () => {
await apiClient.verifyBrowser();

expect(httpClient.post).toHaveBeenCalledWith(
expect.stringContaining('/diagnose/browser'),
expect.any(Object)
);
expect(httpClient.post).toHaveBeenCalledWith(expect.stringContaining('/diagnose/browser'));
});
});

describe('verifyScreenCapture', () => {
it('should send a post request', async () => {
await apiClient.verifyScreenCapture();

expect(httpClient.post).toHaveBeenCalledWith(
expect.stringContaining('/diagnose/screenshot'),
expect.any(Object)
Copy link
Member Author

Choose a reason for hiding this comment

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

the options object was removed from the API calls for many of these, since they mistakenly were used to send the asSystemRequest field

);
expect(httpClient.post).toHaveBeenCalledWith(expect.stringContaining('/diagnose/screenshot'));
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,17 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import type { HttpFetchQuery } from '@kbn/core/public';
import { HttpSetup, IUiSettingsClient } from '@kbn/core/public';
import { i18n } from '@kbn/i18n';
import rison from '@kbn/rison';
import moment from 'moment';
import { stringify } from 'query-string';
import rison from '@kbn/rison';
import type { HttpFetchQuery } from '@kbn/core/public';
import { HttpSetup, IUiSettingsClient } from '@kbn/core/public';
import { buildKibanaPath } from '../../../common/build_kibana_path';
import {
API_BASE_GENERATE,
API_BASE_URL,
API_GENERATE_IMMEDIATE,
API_LIST_URL,
API_MIGRATE_ILM_POLICY_URL,
getRedirectAppPath,
INTERNAL_ROUTES,
PUBLIC_ROUTES,
REPORTING_MANAGEMENT_HOME,
} from '../../../common/constants';
import {
Expand Down Expand Up @@ -46,7 +43,7 @@ export interface DiagnoseResponse {
interface IReportingAPI {
// Helpers
getReportURL(jobId: string): string;
getReportingJobPath<T>(exportType: string, jobParams: BaseParams & T): string; // Return a URL to queue a job, with the job params encoded in the query string of the URL. Used for copying POST URL
getReportingPublicJobPath<T>(exportType: string, jobParams: BaseParams & T): string; // Return a URL to queue a job, with the job params encoded in the query string of the URL. Used for copying POST URL
createReportingJob<T>(exportType: string, jobParams: BaseParams & T): Promise<Job>; // Sends a request to queue a job, with the job params in the POST body
getServerBasePath(): string; // Provides the raw server basePath to allow it to be stripped out from relativeUrls in job params

Expand Down Expand Up @@ -96,9 +93,13 @@ export class ReportingAPIClient implements IReportingAPI {
return href;
}

/**
* Get the internal URL
*/
public getReportURL(jobId: string) {
const apiBaseUrl = this.http.basePath.prepend(API_LIST_URL);
const downloadLink = `${apiBaseUrl}/download/${jobId}`;
const downloadLink = this.http.basePath.prepend(
`${INTERNAL_ROUTES.JOBS.DOWNLOAD_PREFIX}/${jobId}`
);

return downloadLink;
}
Expand All @@ -110,9 +111,7 @@ export class ReportingAPIClient implements IReportingAPI {
}

public async deleteReport(jobId: string) {
return await this.http.delete<void>(`${API_LIST_URL}/delete/${jobId}`, {
asSystemRequest: true,
});
Copy link
Member Author

Choose a reason for hiding this comment

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

deleting a report is manually done by the user: not a system request

return await this.http.delete<void>(`${INTERNAL_ROUTES.JOBS.DELETE_PREFIX}/${jobId}`);
}

public async list(page = 0, jobIds: string[] = []) {
Expand All @@ -122,7 +121,7 @@ export class ReportingAPIClient implements IReportingAPI {
query.ids = jobIds.slice(0, 10).join(',');
}

const jobQueueEntries: ReportApiJSON[] = await this.http.get(`${API_LIST_URL}/list`, {
const jobQueueEntries: ReportApiJSON[] = await this.http.get(INTERNAL_ROUTES.JOBS.LIST, {
query,
asSystemRequest: true,
});
Expand All @@ -131,7 +130,7 @@ export class ReportingAPIClient implements IReportingAPI {
}

public async total() {
return await this.http.get<number>(`${API_LIST_URL}/count`, {
return await this.http.get<number>(INTERNAL_ROUTES.JOBS.COUNT, {
asSystemRequest: true,
});
}
Expand All @@ -151,47 +150,50 @@ export class ReportingAPIClient implements IReportingAPI {
}

public async getInfo(jobId: string) {
const report: ReportApiJSON = await this.http.get(`${API_LIST_URL}/info/${jobId}`, {
asSystemRequest: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

getting report info is manually done by the user: not a system request

});
const report: ReportApiJSON = await this.http.get(
`${INTERNAL_ROUTES.JOBS.INFO_PREFIX}/${jobId}`
);
return new Job(report);
}

public async findForJobIds(jobIds: JobId[]) {
const reports: ReportApiJSON[] = await this.http.fetch(`${API_LIST_URL}/list`, {
const reports: ReportApiJSON[] = await this.http.fetch(INTERNAL_ROUTES.JOBS.LIST, {
query: { page: 0, ids: jobIds.join(',') },
method: 'GET',
});
return reports.map((report) => new Job(report));
}

public getReportingJobPath(exportType: string, jobParams: BaseParams) {
/**
* Returns a string for the public API endpoint used to automate the generation of reports
* This string must be shown when the user selects the option to view/copy the POST URL
*/
public getReportingPublicJobPath(exportType: string, jobParams: BaseParams) {
const params = stringify({
jobParams: rison.encode(jobParams),
});
return `${this.http.basePath.prepend(API_BASE_GENERATE)}/${exportType}?${params}`;
return `${this.http.basePath.prepend(PUBLIC_ROUTES.GENERATE_PREFIX)}/${exportType}?${params}`;
}

/**
* Calls the internal API to generate a report job on-demand
*/
public async createReportingJob(exportType: string, jobParams: BaseParams) {
const jobParamsRison = rison.encode(jobParams);
const resp: { job: ReportApiJSON } = await this.http.post(
`${API_BASE_GENERATE}/${exportType}`,
`${INTERNAL_ROUTES.GENERATE_PREFIX}/${exportType}`,
{
method: 'POST',
body: JSON.stringify({
jobParams: jobParamsRison,
}),
body: JSON.stringify({ jobParams: jobParamsRison }),
}
);

add(resp.job.id);

return new Job(resp.job);
}

public async createImmediateReport(baseParams: BaseParams) {
const { objectType: _objectType, ...params } = baseParams; // objectType is not needed for immediate download api
return this.http.post(`${API_GENERATE_IMMEDIATE}`, {
return this.http.post(INTERNAL_ROUTES.DOWNLOAD_CSV, {
asResponse: true,
body: JSON.stringify(params),
});
Expand All @@ -217,23 +219,19 @@ export class ReportingAPIClient implements IReportingAPI {
this.http.basePath.prepend(REPORTING_MANAGEMENT_HOME);

public getDownloadLink: DownloadReportFn = (jobId: JobId) =>
this.http.basePath.prepend(`${API_LIST_URL}/download/${jobId}`);
this.http.basePath.prepend(`${INTERNAL_ROUTES.JOBS.DOWNLOAD_PREFIX}/${jobId}`);

public getServerBasePath = () => this.http.basePath.serverBasePath;

public verifyBrowser() {
return this.http.post<DiagnoseResponse>(`${API_BASE_URL}/diagnose/browser`, {
asSystemRequest: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

using the diagnostic tool is manually done by the user: not a system request

});
return this.http.post<DiagnoseResponse>(INTERNAL_ROUTES.DIAGNOSE.BROWSER);
}

public verifyScreenCapture() {
return this.http.post<DiagnoseResponse>(`${API_BASE_URL}/diagnose/screenshot`, {
asSystemRequest: true,
});
return this.http.post<DiagnoseResponse>(INTERNAL_ROUTES.DIAGNOSE.SCREENSHOT);
}

public migrateReportingIndicesIlmPolicy() {
return this.http.put(`${API_MIGRATE_ILM_POLICY_URL}`);
return this.http.put(INTERNAL_ROUTES.MIGRATE.MIGRATE_ILM_POLICY);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,11 @@ class ReportingPanelContentUi extends Component<Props, State> {
}

private getAbsoluteReportGenerationUrl = (props: Props) => {
const relativePath = this.props.apiClient.getReportingJobPath(
const relativePath = this.props.apiClient.getReportingPublicJobPath(
props.reportType,
this.props.apiClient.getDecoratedJobParams(this.props.getJobParams(true))
);
return url.resolve(window.location.href, relativePath); // FIXME: '(from: string, to: string): string' is deprecated
Copy link
Member Author

@tsullivan tsullivan Jul 19, 2023

Choose a reason for hiding this comment

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

I can't figure out why my editor says this line uses a deprecated signature, so the comment isn't valid

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice I'm also not seeing any issues in my editor 👍🏻

return url.resolve(window.location.href, relativePath);
};

public componentDidUpdate(_prevProps: Props, prevState: State) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe("Migrate existing indices' ILM policy deprecations", () => {
"correctiveActions": Object {
"api": Object {
"method": "PUT",
"path": "/api/reporting/deprecations/migrate_ilm_policy",
"path": "/internal/reporting/deprecations/migrate_ilm_policy",
},
"manualSteps": Array [
"Update all reporting indices to use the \\"kibana-reporting\\" policy using the index settings API.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import { i18n } from '@kbn/i18n';
import { DeprecationsDetails, GetDeprecationsContext } from '@kbn/core/server';
import { API_MIGRATE_ILM_POLICY_URL, ILM_POLICY_NAME } from '../../common/constants';
import { INTERNAL_ROUTES, ILM_POLICY_NAME } from '../../common/constants';
import { ReportingCore } from '../core';
import { deprecations } from '../lib/deprecations';

Expand Down Expand Up @@ -54,7 +54,7 @@ export const getDeprecationsInfo = async (
],
api: {
method: 'PUT',
path: API_MIGRATE_ILM_POLICY_URL,
path: INTERNAL_ROUTES.MIGRATE.MIGRATE_ILM_POLICY,
},
},
},
Expand Down
Loading