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

[UA][Core] Surface integrations with internal APIs in upgrade assistant #199026

Merged
merged 13 commits into from
Nov 12, 2024
5 changes: 4 additions & 1 deletion examples/routing_example/common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,8 @@ export const DEPRECATED_ROUTES = {
DEPRECATED_ROUTE: '/api/routing_example/d/deprecated_route',
REMOVED_ROUTE: '/api/routing_example/d/removed_route',
MIGRATED_ROUTE: '/api/routing_example/d/migrated_route',
VERSIONED_ROUTE: '/api/routing_example/d/versioned',
VERSIONED_ROUTE: '/api/routing_example/d/versioned_route',
INTERNAL_DEPRECATED_ROUTE: '/api/routing_example/d/internal_deprecated_route',
Copy link
Contributor

Choose a reason for hiding this comment

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

These are great for testing within Kibana, where the internal origin header is added to a request sent from console.

However, we need to surface calls to internal APIs mainly when requests originate externally and may not necessarily include the header. Surfacing these will warn end-users about the upcoming change.

We also want telemetry, to track adoption of internal APIs that are being consumed by integrations and use this info to consider public API alternatives instead,

In other words:

server.restrictInternalApis = true // commenting out also works

desired:

  • surface external requests to internal APIs only (current behavior of this PR)
  • increment counters for routes called externally from Kibana (ATM, usage counters include other routes such as unversioned|get|/XXXXXXXXXXXX/bundles/plugin/transform/1.0.0/{path*}, unversioned|get|/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/{path*} and unversioned|get|/XXXXXXXXXXXX/bundles/plugin/aiops/1.0.0/{path*}))
  • no counters for routes called externally 😞

scenario 2:

server.restrictInternalApis = false //explicitly opt-in to use internal APIs.

desired:

  • surface external requests to internal APIs only (only when requests don't include the origin header)
  • log warning when restriction's disabledt
  • usage counters: still logging ****/gundle/**

Copy link
Member Author

@Bamieh Bamieh Nov 11, 2024

Choose a reason for hiding this comment

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

Good catch on the bundle tracking, it should be fixed now, the boolean i was using for non versioned routes excluded the bundles from the calculation. The bundles are public apis so they should not be tracked,

I dont fully understand the scenarios and desired behaviors though. I believe I am covering the desired behavior here but I am not sure i follow the rest of the comment.

These are great for testing within Kibana, where the internal origin header is added to a request sent from console.

I've updated the README and console commands to explicitly set elasticInternalOrigin=false to enable tracking them from the console for quick tesitng. We do have integration tests to ensure proper testing for the behavior of this and the previous api deprecations.

In the dev console we need to explicitly set the query param elasticInternalOrigin to false to track the request as non-internal origin.

# Route deprecations for Versioned routes
GET kbn:/api/routing_example/d/versioned_route?apiVersion=2023-10-31&elasticInternalOrigin=false

# Route deprecations for Non-versioned routes
GET kbn:/api/routing_example/d/removed_route?elasticInternalOrigin=false
GET kbn:/api/routing_example/d/deprecated_route?elasticInternalOrigin=false
POST kbn:/api/routing_example/d/migrated_route?elasticInternalOrigin=false
{}

# Access deprecations
GET kbn:/api/routing_example/d/internal_deprecated_route?elasticInternalOrigin=false
GET kbn:/internal/routing_example/d/internal_only_route?elasticInternalOrigin=false
GET kbn:/internal/routing_example/d/internal_versioned_route?apiVersion=1&elasticInternalOrigin=false

However, we need to surface calls to internal APIs mainly when requests originate externally and may not necessarily include the header. Surfacing these will warn end-users about the upcoming change.

Can you clarify what you mean exactly here? External vs internal calls are specified via the header (or query param), and if the header/queryParam is missing then its an external call which in that case qualifies to be surfaced in the deprecations as access api deprecation (Internal access + the requesting caller is external).

no counters for routes called externally 😞
surface external requests to internal APIs only (only when requests don't include the origin header)

Are you sure? how are you testing this? I've tested locally and we do have unit and integration tests that ensure this behavior is working as expected.

Copy link
Contributor

@TinaHeiligers TinaHeiligers Nov 11, 2024

Choose a reason for hiding this comment

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

Retested and the changes look great! Both scenarios I was testing for work as expected.
With the restriction explicitly disabled in kibana.yml, we get a deprecation entry in UA when the API is called without the internal origin header and the usage counters increment, consistent with logging added in #195696.

Steps to reproduce the scenario without using the example plugin:

  1. disable the restriction in kibana.yml: server.restrictInternalApis: false
  2. run es & kibana in dev
  3. make curl request to an internal API without the internal origin header. For example:
    curl --location 'localhost:5601/abc/internal/kibana/global_settings' --header 'Content-Type: application/json' --header 'Accept-Encoding: gzip, deflate, br' --header 'kbn-xsrf: kibana' --header 'Kbn-Version: 9.0.0' --header 'Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ=='
  4. Navigate to Upgrade Assistant, where you'll see a warning for a Kibana deprecation:
    Screenshot 2024-11-11 at 11 30 50
  5. Selecting the warning allows one to mark it as resolved:
    Screenshot 2024-11-11 at 11 17 01
  6. Get the usage counters from console:
GET .kibana_usage_counters/_search
{
    "query": {
        "bool": {
            "should": [
              {"match": { "usage-counter.counterType": "deprecated_api_call:total"}},
              {"match": { "usage-counter.counterType": "deprecated_api_call:resolved"}},
              {"match": { "usage-counter.counterType": "deprecated_api_call:marked_as_resolved"}}
            ]
        }
    }
}

Response should be similar to

Response should be similar to
{
  "took": 2,
  "timed_out": false,
  "_shards": {
    "total": 1,
    "successful": 1,
    "skipped": 0,
    "failed": 0
  },
  "hits": {
    "total": {
      "value": 3,
      "relation": "eq"
    },
    "max_score": 3.7216692,
    "hits": [
      {
        "_index": ".kibana_usage_counters_9.0.0_001",
        "_id": "usage-counter:core:unversioned|get|/internal/kibana/global_settings:deprecated_api_call:marked_as_resolved:server:20241111",
        "_score": 3.7216692,
        "_source": {
          "usage-counter": {
            "domainId": "core",
            "counterName": "unversioned|get|/internal/kibana/global_settings",
            "counterType": "deprecated_api_call:marked_as_resolved",
            "source": "server",
            "count": 1
          },
          "type": "usage-counter",
          "managed": false,
          "coreMigrationVersion": "8.8.0",
          "updated_at": "2024-11-11T18:13:36.136Z"
        }
      },
      {
        "_index": ".kibana_usage_counters_9.0.0_001",
        "_id": "usage-counter:core:unversioned|get|/internal/kibana/global_settings:deprecated_api_call:resolved:server:20241111",
        "_score": 3.7216692,
        "_source": {
          "usage-counter": {
            "domainId": "core",
            "counterName": "unversioned|get|/internal/kibana/global_settings",
            "counterType": "deprecated_api_call:resolved",
            "source": "server",
            "count": 1
          },
          "type": "usage-counter",
          "managed": false,
          "coreMigrationVersion": "8.8.0",
          "updated_at": "2024-11-11T18:13:36.133Z"
        }
      },
      {
        "_index": ".kibana_usage_counters_9.0.0_001",
        "_id": "usage-counter:core:unversioned|get|/internal/kibana/global_settings:deprecated_api_call:total:server:20241111",
        "_score": 3.7216692,
        "_source": {
          "usage-counter": {
            "domainId": "core",
            "counterName": "unversioned|get|/internal/kibana/global_settings",
            "counterType": "deprecated_api_call:total",
            "source": "server",
            "count": 2
          },
          "type": "usage-counter",
          "managed": false,
          "coreMigrationVersion": "8.8.0",
          "updated_at": "2024-11-11T18:15:06.197Z"
        }
      }
    ]
  }
}

INTERNAL_ONLY_ROUTE: '/internal/routing_example/d/internal_only_route',
VERSIONED_INTERNAL_ROUTE: '/internal/routing_example/d/internal_versioned_route',
};
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
import { IRouter } from '@kbn/core/server';
import { registerDeprecatedRoute } from './unversioned';
import { registerVersionedDeprecatedRoute } from './versioned';
import { registerInternalDeprecatedRoute } from './internal';

export function registerDeprecatedRoutes(router: IRouter) {
registerDeprecatedRoute(router);
registerVersionedDeprecatedRoute(router);
registerInternalDeprecatedRoute(router);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* 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", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import type { IRouter } from '@kbn/core/server';
import { DEPRECATED_ROUTES } from '../../../common';

export const registerInternalDeprecatedRoute = (router: IRouter) => {
router.get(
{
path: DEPRECATED_ROUTES.INTERNAL_DEPRECATED_ROUTE,
validate: false,
options: {
// Explicitly set access is to internal
access: 'internal',
deprecated: {
documentationUrl: 'https://elastic.co/',
severity: 'critical',
message: 'Additonal message for internal deprecated api',
reason: { type: 'deprecate' },
},
Comment on lines +21 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually raises something of an oversight: I don't think ever setting an internal route as deprecated was intended for this API 🤔 , although being able to set critical on it could be useful if we want to remove it and have known integrations.

My gut feel would be to remove it from the types for internal routes (versioned or non-versioned). FWIW I don't think we should block on it here.

Copy link
Member Author

@Bamieh Bamieh Nov 11, 2024

Choose a reason for hiding this comment

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

Lets address this in a separate PR after we think about it for all API deprecations. Created a placeholder issue for now #199675

Copy link
Contributor

Choose a reason for hiding this comment

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

My gut feel would be to remove it from the types for internal routes (versioned or non-versioned).
++ internal APIs aren't supposed to be consumed externally and we can add .jsdocs to warn plugin developers that the route is deprecated.

},
},
async (ctx, req, res) => {
return res.ok({
body: {
result:
'Called deprecated route with `access: internal`. Check UA to see the deprecation.',
},
});
}
);

router.get(
{
path: DEPRECATED_ROUTES.INTERNAL_ONLY_ROUTE,
validate: false,
// If no access is specified then it defaults to internal
},
async (ctx, req, res) => {
return res.ok({
body: {
result:
'Called route with `access: internal` Although this API is not marked as deprecated it will show in UA. Check UA to see the deprecation.',
},
});
}
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -11,42 +11,72 @@ import type { IRouter } from '@kbn/core/server';
import { DEPRECATED_ROUTES } from '../../../common';

export const registerVersionedDeprecatedRoute = (router: IRouter) => {
const versionedRoute = router.versioned.get({
path: DEPRECATED_ROUTES.VERSIONED_ROUTE,
description: 'Routing example plugin deprecated versioned route.',
access: 'internal',
options: {
excludeFromOAS: true,
},
enableQueryVersion: true,
});

versionedRoute.addVersion(
{
router.versioned
.get({
path: DEPRECATED_ROUTES.VERSIONED_ROUTE,
description: 'Routing example plugin deprecated versioned route.',
access: 'public',
options: {
deprecated: {
documentationUrl: 'https://elastic.co/',
severity: 'warning',
reason: { type: 'bump', newApiVersion: '2' },
excludeFromOAS: true,
},
enableQueryVersion: true,
})
.addVersion(
{
options: {
deprecated: {
documentationUrl: 'https://elastic.co/',
severity: 'warning',
reason: { type: 'deprecate' },
},
},
validate: false,
version: '2023-10-31',
},
validate: false,
version: '1',
},
(ctx, req, res) => {
return res.ok({
body: { result: 'Called deprecated version of the API. API version 1 -> 2' },
});
}
);
(ctx, req, res) => {
return res.ok({
body: { result: 'Called deprecated version of the API "2023-10-31"' },
});
}
);

versionedRoute.addVersion(
{
version: '2',
validate: false,
},
(ctx, req, res) => {
return res.ok({ body: { result: 'Called API version 2' } });
}
);
router.versioned
.get({
path: DEPRECATED_ROUTES.VERSIONED_INTERNAL_ROUTE,
description: 'Routing example plugin deprecated versioned route.',
access: 'internal',
options: {
excludeFromOAS: true,
},
enableQueryVersion: true,
})
.addVersion(
{
options: {
deprecated: {
documentationUrl: 'https://elastic.co/',
severity: 'warning',
reason: { type: 'bump', newApiVersion: '2' },
},
},
validate: false,
version: '1',
},
(ctx, req, res) => {
return res.ok({
body: { result: 'Called internal deprecated version of the API 1.' },
});
}
)
.addVersion(
{
validate: false,
version: '2',
},
(ctx, req, res) => {
return res.ok({
body: { result: 'Called non-deprecated version of the API.' },
});
}
);
};
3 changes: 3 additions & 0 deletions examples/routing_example/server/routes/message_routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ export function registerGetMessageByIdRoute(router: IRouter) {
router.get(
{
path: `${INTERNAL_GET_MESSAGE_BY_ID_ROUTE}/{id}`,
options: {
access: 'internal',
},
validate: {
params: schema.object({
id: schema.string(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export type {
BaseDeprecationDetails,
ConfigDeprecationDetails,
FeatureDeprecationDetails,
ApiDeprecationDetails,
DeprecationsDetails,
DomainDeprecationDetails,
DeprecationsGetResponse,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ export type DeprecationsDetails =
/**
* @public
*/
export type DomainDeprecationDetails = DeprecationsDetails & {
export type DomainDeprecationDetails<ExtendedDetails = DeprecationsDetails> = ExtendedDetails & {
domainId: string;
};

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* 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", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import type {
ApiDeprecationDetails,
DomainDeprecationDetails,
} from '@kbn/core-deprecations-common';

import type { PostValidationMetadata } from '@kbn/core-http-server';
import type { BuildApiDeprecationDetailsParams } from '../types';
import {
getApiDeprecationMessage,
getApiDeprecationsManualSteps,
getApiDeprecationTitle,
} from './i18n_texts';

export const getIsAccessApiDeprecation = ({
isInternalApiRequest,
isPublicAccess,
}: PostValidationMetadata): boolean => {
const isNotPublicAccess = !isPublicAccess;
const isNotInternalRequest = !isInternalApiRequest;
Comment on lines +27 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
const isNotPublicAccess = !isPublicAccess;
const isNotInternalRequest = !isInternalApiRequest;
const isInternalAccess = !isPublicAccess;
const isExternalApiRequest = !isInternalApiRequest;


return !!(isNotPublicAccess && isNotInternalRequest);
};

export const buildApiAccessDeprecationDetails = ({
apiUsageStats,
deprecatedApiDetails,
}: BuildApiDeprecationDetailsParams): DomainDeprecationDetails<ApiDeprecationDetails> => {
const { apiId, apiTotalCalls, totalMarkedAsResolved } = apiUsageStats;
const { routeVersion, routePath, routeDeprecationOptions, routeMethod } = deprecatedApiDetails;

const deprecationLevel = routeDeprecationOptions?.severity || 'warning';

return {
apiId,
title: getApiDeprecationTitle(deprecatedApiDetails),
level: deprecationLevel,
message: getApiDeprecationMessage(deprecatedApiDetails, apiUsageStats),
documentationUrl: routeDeprecationOptions?.documentationUrl,
correctiveActions: {
manualSteps: getApiDeprecationsManualSteps(),
mark_as_resolved_api: {
routePath,
routeMethod,
routeVersion,
apiTotalCalls,
totalMarkedAsResolved,
timestamp: new Date(),
},
},
deprecationType: 'api',
domainId: 'core.http.access-deprecations',
};
};
Loading