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.

Sign up for GitHub

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

[Saved Objects] Logs warning when deprecated SO APIs are called #150775

Merged
merged 4 commits into from
Feb 13, 2023
Merged

[Saved Objects] Logs warning when deprecated SO APIs are called #150775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5b3267
Logs warning when deprecated SO APIs are called
TinaHeiligers Feb 7, 2023
6018fa6
updates bulk API integration tests
TinaHeiligers Feb 9, 2023
6d34b89
fix bulk_update integration test
TinaHeiligers Feb 9, 2023
868515e
Merge branch 'main' into kbn-150469-SO-APIs-add-deprecations-logs
kibanamachine Feb 12, 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
7 changes: 6 additions & 1 deletion packages/core/saved-objects/core-saved-objects-server-internal/src/routes/bulk_create.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@

import { schema } from '@kbn/config-schema';
import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal';
import type { Logger } from '@kbn/logging';
import type { InternalSavedObjectRouter } from '../internal_types';
import { catchAndReturnBoomErrors, throwIfAnyTypeNotVisibleByAPI } from './utils';

interface RouteDependencies {
coreUsageData: InternalCoreUsageDataSetup;
logger: Logger;
}

export const registerBulkCreateRoute = (
router: InternalSavedObjectRouter,
{ coreUsageData }: RouteDependencies
{ coreUsageData, logger }: RouteDependencies
) => {
router.post(
{
Expand Down Expand Up @@ -49,6 +51,9 @@ export const registerBulkCreateRoute = (
},
},
catchAndReturnBoomErrors(async (context, req, res) => {
logger.warn(
"The bulk create saved object API '/api/saved_objects/_bulk_create' is deprecated."
);
const { overwrite } = req.query;

const usageStatsClient = coreUsageData.getClient();
Expand Down
7 changes: 6 additions & 1 deletion packages/core/saved-objects/core-saved-objects-server-internal/src/routes/bulk_delete.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@

import { schema } from '@kbn/config-schema';
import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal';
import type { Logger } from '@kbn/logging';
import type { InternalSavedObjectRouter } from '../internal_types';
import { catchAndReturnBoomErrors, throwIfAnyTypeNotVisibleByAPI } from './utils';

interface RouteDependencies {
coreUsageData: InternalCoreUsageDataSetup;
logger: Logger;
}

export const registerBulkDeleteRoute = (
router: InternalSavedObjectRouter,
{ coreUsageData }: RouteDependencies
{ coreUsageData, logger }: RouteDependencies
) => {
router.post(
{
Expand All @@ -35,6 +37,9 @@ export const registerBulkDeleteRoute = (
},
},
catchAndReturnBoomErrors(async (context, req, res) => {
logger.warn(
"The bulk update saved object API '/api/saved_objects/_bulk_update' is deprecated."
);
const { force } = req.query;
const usageStatsClient = coreUsageData.getClient();
usageStatsClient.incrementSavedObjectsBulkDelete({ request: req }).catch(() => {});
Expand Down
5 changes: 4 additions & 1 deletion packages/core/saved-objects/core-saved-objects-server-internal/src/routes/bulk_get.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@

import { schema } from '@kbn/config-schema';
import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal';
import type { Logger } from '@kbn/logging';
import type { InternalSavedObjectRouter } from '../internal_types';
import { catchAndReturnBoomErrors, throwIfAnyTypeNotVisibleByAPI } from './utils';

interface RouteDependencies {
coreUsageData: InternalCoreUsageDataSetup;
logger: Logger;
}

export const registerBulkGetRoute = (
router: InternalSavedObjectRouter,
{ coreUsageData }: RouteDependencies
{ coreUsageData, logger }: RouteDependencies
) => {
router.post(
{
Expand All @@ -34,6 +36,7 @@ export const registerBulkGetRoute = (
},
},
catchAndReturnBoomErrors(async (context, req, res) => {
logger.warn("The bulk get saved object API '/api/saved_objects/_bulk_get' is deprecated.");
const usageStatsClient = coreUsageData.getClient();
usageStatsClient.incrementSavedObjectsBulkGet({ request: req }).catch(() => {});

Expand Down
7 changes: 6 additions & 1 deletion packages/core/saved-objects/core-saved-objects-server-internal/src/routes/bulk_resolve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@

import { schema } from '@kbn/config-schema';
import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal';
import type { Logger } from '@kbn/logging';
import type { InternalSavedObjectRouter } from '../internal_types';
import { catchAndReturnBoomErrors, throwIfAnyTypeNotVisibleByAPI } from './utils';

interface RouteDependencies {
coreUsageData: InternalCoreUsageDataSetup;
logger: Logger;
}

export const registerBulkResolveRoute = (
router: InternalSavedObjectRouter,
{ coreUsageData }: RouteDependencies
{ coreUsageData, logger }: RouteDependencies
) => {
router.post(
{
Expand All @@ -32,6 +34,9 @@ export const registerBulkResolveRoute = (
},
},
catchAndReturnBoomErrors(async (context, req, res) => {
logger.warn(
"The bulk resolve saved object API '/api/saved_objects/_bulk_resolve' is deprecated."
);
const usageStatsClient = coreUsageData.getClient();
usageStatsClient.incrementSavedObjectsBulkResolve({ request: req }).catch(() => {});

Expand Down
7 changes: 6 additions & 1 deletion packages/core/saved-objects/core-saved-objects-server-internal/src/routes/bulk_update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@

import { schema } from '@kbn/config-schema';
import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal';
import type { Logger } from '@kbn/logging';
import type { InternalSavedObjectRouter } from '../internal_types';
import { catchAndReturnBoomErrors, throwIfAnyTypeNotVisibleByAPI } from './utils';

interface RouteDependencies {
coreUsageData: InternalCoreUsageDataSetup;
logger: Logger;
}

export const registerBulkUpdateRoute = (
router: InternalSavedObjectRouter,
{ coreUsageData }: RouteDependencies
{ coreUsageData, logger }: RouteDependencies
) => {
router.put(
{
Expand All @@ -44,6 +46,9 @@ export const registerBulkUpdateRoute = (
},
},
catchAndReturnBoomErrors(async (context, req, res) => {
logger.warn(
"The bulk update saved object API '/api/saved_objects/_bulk_update' is deprecated."
);
const usageStatsClient = coreUsageData.getClient();
usageStatsClient.incrementSavedObjectsBulkUpdate({ request: req }).catch(() => {});

Expand Down
5 changes: 4 additions & 1 deletion packages/core/saved-objects/core-saved-objects-server-internal/src/routes/create.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@

import { schema } from '@kbn/config-schema';
import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal';
import type { Logger } from '@kbn/logging';
import type { InternalSavedObjectRouter } from '../internal_types';
import { catchAndReturnBoomErrors, throwIfTypeNotVisibleByAPI } from './utils';

interface RouteDependencies {
coreUsageData: InternalCoreUsageDataSetup;
logger: Logger;
}

export const registerCreateRoute = (
router: InternalSavedObjectRouter,
{ coreUsageData }: RouteDependencies
{ coreUsageData, logger }: RouteDependencies
) => {
router.post(
{
Expand Down Expand Up @@ -48,6 +50,7 @@ export const registerCreateRoute = (
},
},
catchAndReturnBoomErrors(async (context, req, res) => {
logger.warn("The create saved object API '/api/saved_objects/{type}/{id}' is deprecated.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.warn("The create saved object API '/api/saved_objects/{type}/{id}' is deprecated.");
logger.warn("Detected use of saved object HTTP API 'POST /api/saved_objects/{type}/{id}'. This endpoint is deprecated and will be removed soon.");

Copy link
Contributor

@jloleysens jloleysens Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In my suggestion I restructured the warning message(s) slightly, trying to optimise for:

  • Counting: If we prefix every log with the same message "Detected use of saved object HTTP API" it will be easier to query Kibana logs and get a count.
  • Added the fully qualified endpoint description by adding POST in front of the path definition.
  • Tried to make the message sound a bit more scary 😄

Let me know what you think @TinaHeiligers !

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@jloleysens I see what you're saying. I'm aiming at a log message that's consistent with the two we already have. Since we don't have a 'replacement' we can only aim for similar log messages as the legacy_import_export routes. We don't know for certain that we can remove these API's, so adding a second qualifier is risky if we change our minds:
For example, if we say the API's will be removed and end up making them internal rather than completely removing them, a message such as:

logger.warn(
    "The create API '/api/kibana/saved_objects/{type}/{id}' is deprecated. This endpoint will be removed soon."
)

would kinda be lying.

Copy link
Contributor

Choose a reason for hiding this comment

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

for similar log messages

Thanks for linking to this, I was not aware that we'd done something similar.

We don't know for certain that we can remove these API's, so adding a second qualifier is risky if we change our minds

It's a little scary to think they may be around in some form, but I take your point that we don't need to commit one way or the other in the message.

In the pre-existing message you link to a suggestion is made about an alternative. I think a second sentence giving some guidance would be really nice, but I'm struggling to think of something. Maybe:

"Do not rely on saved object APIs for your functionality, instead use plugin-specific endpoints."

const { type, id } = req.params;
const { overwrite } = req.query;
const { attributes, migrationVersion, coreMigrationVersion, references, initialNamespaces } =
Expand Down
5 changes: 4 additions & 1 deletion packages/core/saved-objects/core-saved-objects-server-internal/src/routes/delete.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@

import { schema } from '@kbn/config-schema';
import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal';
import type { Logger } from '@kbn/logging';
import type { InternalSavedObjectRouter } from '../internal_types';
import { catchAndReturnBoomErrors, throwIfTypeNotVisibleByAPI } from './utils';

interface RouteDependencies {
coreUsageData: InternalCoreUsageDataSetup;
logger: Logger;
}

export const registerDeleteRoute = (
router: InternalSavedObjectRouter,
{ coreUsageData }: RouteDependencies
{ coreUsageData, logger }: RouteDependencies
) => {
router.delete(
{
Expand All @@ -33,6 +35,7 @@ export const registerDeleteRoute = (
},
},
catchAndReturnBoomErrors(async (context, req, res) => {
logger.warn("The delete saved object API '/api/saved_objects/{type}/{id}' is deprecated.");
const { type, id } = req.params;
const { force } = req.query;
const { getClient, typeRegistry } = (await context.core).savedObjects;
Expand Down
5 changes: 4 additions & 1 deletion packages/core/saved-objects/core-saved-objects-server-internal/src/routes/find.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@

import { schema } from '@kbn/config-schema';
import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal';
import type { Logger } from '@kbn/logging';
import type { InternalSavedObjectRouter } from '../internal_types';
import { catchAndReturnBoomErrors, throwOnHttpHiddenTypes } from './utils';

interface RouteDependencies {
coreUsageData: InternalCoreUsageDataSetup;
logger: Logger;
}

export const registerFindRoute = (
router: InternalSavedObjectRouter,
{ coreUsageData }: RouteDependencies
{ coreUsageData, logger }: RouteDependencies
) => {
const referenceSchema = schema.object({
type: schema.string(),
Expand Down Expand Up @@ -59,6 +61,7 @@ export const registerFindRoute = (
},
},
catchAndReturnBoomErrors(async (context, req, res) => {
logger.warn("The find saved object API '/api/saved_objects/_find' is deprecated.");
const query = req.query;

const namespaces =
Expand Down
5 changes: 4 additions & 1 deletion packages/core/saved-objects/core-saved-objects-server-internal/src/routes/get.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@

import { schema } from '@kbn/config-schema';
import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal';
import type { Logger } from '@kbn/logging';
import type { InternalSavedObjectRouter } from '../internal_types';
import { catchAndReturnBoomErrors, throwIfTypeNotVisibleByAPI } from './utils';

interface RouteDependencies {
coreUsageData: InternalCoreUsageDataSetup;
logger: Logger;
}

export const registerGetRoute = (
router: InternalSavedObjectRouter,
{ coreUsageData }: RouteDependencies
{ coreUsageData, logger }: RouteDependencies
) => {
router.get(
{
Expand All @@ -30,6 +32,7 @@ export const registerGetRoute = (
},
},
catchAndReturnBoomErrors(async (context, req, res) => {
logger.warn("The get saved object API '/api/saved_objects/{type}/{id}' is deprecated.");
const { type, id } = req.params;

const usageStatsClient = coreUsageData.getClient();
Expand Down
22 changes: 11 additions & 11 deletions packages/core/saved-objects/core-saved-objects-server-internal/src/routes/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,17 @@ export function registerRoutes({
const router =
http.createRouter<InternalSavedObjectsRequestHandlerContext>('/api/saved_objects/');

registerGetRoute(router, { coreUsageData });
registerResolveRoute(router, { coreUsageData });
registerCreateRoute(router, { coreUsageData });
registerDeleteRoute(router, { coreUsageData });
registerFindRoute(router, { coreUsageData });
registerUpdateRoute(router, { coreUsageData });
registerBulkGetRoute(router, { coreUsageData });
registerBulkCreateRoute(router, { coreUsageData });
registerBulkResolveRoute(router, { coreUsageData });
registerBulkUpdateRoute(router, { coreUsageData });
registerBulkDeleteRoute(router, { coreUsageData });
registerGetRoute(router, { coreUsageData, logger });
registerResolveRoute(router, { coreUsageData, logger });
registerCreateRoute(router, { coreUsageData, logger });
registerDeleteRoute(router, { coreUsageData, logger });
registerFindRoute(router, { coreUsageData, logger });
registerUpdateRoute(router, { coreUsageData, logger });
registerBulkGetRoute(router, { coreUsageData, logger });
registerBulkCreateRoute(router, { coreUsageData, logger });
registerBulkResolveRoute(router, { coreUsageData, logger });
registerBulkUpdateRoute(router, { coreUsageData, logger });
registerBulkDeleteRoute(router, { coreUsageData, logger });
registerExportRoute(router, { config, coreUsageData });
registerImportRoute(router, { config, coreUsageData });
registerResolveImportErrorsRoute(router, { config, coreUsageData });
Expand Down
7 changes: 6 additions & 1 deletion packages/core/saved-objects/core-saved-objects-server-internal/src/routes/resolve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@

import { schema } from '@kbn/config-schema';
import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal';
import type { Logger } from '@kbn/logging';
import type { InternalSavedObjectRouter } from '../internal_types';
import { throwIfTypeNotVisibleByAPI } from './utils';

interface RouteDependencies {
coreUsageData: InternalCoreUsageDataSetup;
logger: Logger;
}

export const registerResolveRoute = (
router: InternalSavedObjectRouter,
{ coreUsageData }: RouteDependencies
{ coreUsageData, logger }: RouteDependencies
) => {
router.get(
{
Expand All @@ -30,6 +32,9 @@ export const registerResolveRoute = (
},
},
router.handleLegacyErrors(async (context, req, res) => {
logger.warn(
"The resolve saved object API '/api/saved_objects/resolve/{type}/{id}' is deprecated."
);
const { type, id } = req.params;
const { savedObjects } = await context.core;

Expand Down
5 changes: 4 additions & 1 deletion packages/core/saved-objects/core-saved-objects-server-internal/src/routes/update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,19 @@

import { schema } from '@kbn/config-schema';
import type { SavedObjectsUpdateOptions } from '@kbn/core-saved-objects-api-server';
import type { Logger } from '@kbn/logging';
import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal';
import type { InternalSavedObjectRouter } from '../internal_types';
import { catchAndReturnBoomErrors, throwIfTypeNotVisibleByAPI } from './utils';

interface RouteDependencies {
coreUsageData: InternalCoreUsageDataSetup;
logger: Logger;
}

export const registerUpdateRoute = (
router: InternalSavedObjectRouter,
{ coreUsageData }: RouteDependencies
{ coreUsageData, logger }: RouteDependencies
) => {
router.put(
{
Expand All @@ -45,6 +47,7 @@ export const registerUpdateRoute = (
},
},
catchAndReturnBoomErrors(async (context, req, res) => {
logger.warn("The update saved object API '/api/saved_objects/{type}/{id}' is deprecated.");
const { type, id } = req.params;
const { attributes, version, references, upsert } = req.body;
const options: SavedObjectsUpdateOptions = { version, references, upsert };
Expand Down
23 changes: 22 additions & 1 deletion src/core/server/integration_tests/saved_objects/routes/bulk_create.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
type InternalSavedObjectsRequestHandlerContext,
} from '@kbn/core-saved-objects-server-internal';
import { createHiddenTypeVariants, setupServer } from '@kbn/core-test-helpers-test-utils';
import { loggerMock } from '@kbn/logging-mocks';

type SetupServerReturn = Awaited<ReturnType<typeof setupServer>>;

Expand All @@ -33,6 +34,7 @@ describe('POST /api/saved_objects/_bulk_create', () => {
let handlerContext: SetupServerReturn['handlerContext'];
let savedObjectsClient: ReturnType<typeof savedObjectsClientMock.create>;
let coreUsageStatsClient: jest.Mocked<ICoreUsageStatsClient>;
let loggerWarnSpy: jest.SpyInstance;

beforeEach(async () => {
({ server, httpSetup, handlerContext } = await setupServer());
Expand All @@ -50,7 +52,9 @@ describe('POST /api/saved_objects/_bulk_create', () => {
coreUsageStatsClient = coreUsageStatsClientMock.create();
coreUsageStatsClient.incrementSavedObjectsBulkCreate.mockRejectedValue(new Error('Oh no!')); // intentionally throw this error, which is swallowed, so we can assert that the operation does not fail
const coreUsageData = coreUsageDataServiceMock.createSetupContract(coreUsageStatsClient);
registerBulkCreateRoute(router, { coreUsageData });
const logger = loggerMock.create();
loggerWarnSpy = jest.spyOn(logger, 'warn').mockImplementation();
registerBulkCreateRoute(router, { coreUsageData, logger });

await server.start();
});
Expand Down Expand Up @@ -161,4 +165,21 @@ describe('POST /api/saved_objects/_bulk_create', () => {
'Unsupported saved object type(s): hidden-from-http: Bad Request'
);
});

it('logs a warning message when called', async () => {
await supertest(httpSetup.server.listener)
.post('/api/saved_objects/_bulk_create')
.send([
{
id: 'abc1234',
type: 'index-pattern',
attributes: {
title: 'foo',
},
references: [],
},
])
.expect(200);
expect(loggerWarnSpy).toHaveBeenCalledTimes(1);
});
});
Loading