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

Conversation

Bamieh
Copy link
Member

@Bamieh Bamieh commented Nov 5, 2024

Summary

In #117241 we're surfacing usage of APIs marked as deprecated: true in the Upgrade Assistant to help users prepare for a major upgrade. While internal APIs aren't really deprecated in the same sense we are making a breaking change by blocking external integrations with these APIs. Since this could be equally disruptive to users depending on these APIs it would help our users to surface such usage in the UA too.

The api deprecations now have two sub types:

  1. routes deprecations options.deprecated: { … }
  2. access deprecations options.access: 'internal'

This PR adds the second api deprecation subtype. The reason i kept one api deprecation type and i didnt create a new type is that they have exactly the same registration process but are triggered by different attributes. The api deprecation is fully managed by the core team internal services and are configured by the user through the route interface so it makes sense to keep them as one type. I also can see us adding more subtypes to this and just piggybacking on the current flow instead of duplicating it everytime.

Checklist

  • Create deprecation subtype
  • Example plugin
  • Surface the deprecation in UA
  • Api access deprecation copy (@florent-leborgne )
  • Update README and code annotations
  • Unit tests
  • Integration tests

Closes #194675

Design decisions:

If the API has both route deprecation (options.deprecated: { … } ) AND is an internal api options.access: 'internal'

The current behavior i went for in my PR:
I show this API once in the UA under the internal access deprecation. While showing the route deprecation details if defined. This seems to make the most sense since users should stop using this API altogether.

Copy decisions:

@florent-leborgne wrote the copy for this deprecation subtype.
image

image

Testing

Run kibana locally with the test example plugin that has deprecated routes

yarn start --plugin-path=examples/routing_example --plugin-path=examples/developer_examples

The following comprehensive deprecated routes examples are registered inside the folder: examples/routing_example/server/routes/deprecated_routes

Run them in the dev console to trigger the deprecation condition so they show up in the UA:

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

@Bamieh Bamieh added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.17.0 labels Nov 5, 2024
@Bamieh Bamieh marked this pull request as ready for review November 10, 2024 14:43
@Bamieh Bamieh requested review from a team as code owners November 10, 2024 14:43
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Surfaces external calls to internal APIs in UA as expected.
Usage counters look off, we're counting calls to native kibana **/bundels/** routes that I don't think we want to include.

x-pack/plugins/upgrade_assistant/README.md Show resolved Hide resolved
@@ -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"
        }
      }
    ]
  }
}

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-deprecations-common 9 12 +3
@kbn/core-http-server 232 242 +10
@kbn/core-usage-data-server 154 156 +2
total +15

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/core-deprecations-common 1 0 -1
Unknown metric groups

API count

id before after diff
@kbn/core-deprecations-common 17 20 +3
@kbn/core-http-server 557 568 +11
@kbn/core-usage-data-server 165 167 +2
total +16

History

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Just reviewed the ELASTIC_INTERNAL_ORIGIN_QUERY_PARAM logic and left a q. Overall approach LGTM.

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

New approach makes sense to me, test changes to UA make sense to me. Tested locally 🚀

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Great work @Bamieh !

Code only review, and left only minor nits with a question about setting internal routes as deprecated we should probably discuss!

Comment on lines +27 to +28
const isNotPublicAccess = !isPublicAccess;
const isNotInternalRequest = !isInternalApiRequest;
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;

Comment on lines +529 to +531
/**
* Post Validation Route emitter metadata.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is this intended to be @public?

Comment on lines +21 to +26
deprecated: {
documentationUrl: 'https://elastic.co/',
severity: 'critical',
message: 'Additonal message for internal deprecated api',
reason: { type: 'deprecate' },
},
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.

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Works like a charm!

@Bamieh Bamieh merged commit a10eb1f into elastic:main Nov 12, 2024
22 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11796199057

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

You might need to backport the following PRs to 8.x:
- Changes deprecated SO HTTP APIs deprecation field to object (#197936)

Manual backport

To create the backport manually run:

node scripts/backport --pr 199026

Questions ?

Please refer to the Backport tool documentation

@Bamieh
Copy link
Member Author

Bamieh commented Nov 12, 2024

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@Bamieh Bamieh deleted the core/internal_apis_deprecations branch November 12, 2024 11:50
Bamieh added a commit that referenced this pull request Nov 12, 2024
…ssistant (#199026) (#199764)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[UA][Core] Surface integrations with internal APIs in upgrade
assistant (#199026)](#199026)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Ahmad
Bamieh","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-12T11:19:22Z","message":"[UA][Core]
Surface integrations with internal APIs in upgrade assistant
(#199026)\n\n## Summary\r\n\r\n> In
#117241 we're surfacing\r\nusage
of APIs marked as `deprecated: true` in the Upgrade Assistant to\r\nhelp
users prepare for a major upgrade. While internal APIs aren't\r\nreally
deprecated in the same sense we are making a breaking change
by\r\nblocking external integrations with these APIs. Since this could
be\r\nequally disruptive to users depending on these APIs it would help
our\r\nusers to surface such usage in the UA too.\r\n\r\nThe `api`
deprecations now have two sub types:\r\n1. routes deprecations
`options.deprecated: { … }`\r\n2. access deprecations `options.access:
'internal'`\r\n\r\nThis PR adds the second `api` deprecation subtype.
The reason i kept one\r\n`api` deprecation type and i didnt create a new
type is that they have\r\nexactly the same registration process but are
triggered by different\r\nattributes. The `api` deprecation is fully
managed by the core team\r\ninternal services and are configured by the
user through the route\r\ninterface so it makes sense to keep them as
one type. I also can see us\r\nadding more subtypes to this and just
piggybacking on the current flow\r\ninstead of duplicating it
everytime.\r\n\r\n\r\n**Checklist**\r\n- [x] Create deprecation
subtype\r\n- [x] Example plugin\r\n- [x] Surface the deprecation in
UA\r\n- [x] Api access deprecation copy (@florent-leborgne )\r\n- [x]
Update README and code annotations\r\n- [x] Unit tests\r\n- [x]
Integration tests\r\n\r\n\r\nCloses
https://github.com/elastic/kibana/issues/194675\r\n\r\n### Design
decisions:\r\nIf the API has both route deprecation
(`options.deprecated: { … }` ) AND\r\nis an internal api
`options.access: 'internal'`\r\n\r\nThe current behavior i went for in
my PR:\r\nI show this API once in the UA under the internal access
deprecation.\r\nWhile showing the route deprecation details if defined.
This seems to\r\nmake the most sense since users should stop using this
API altogether.\r\n\r\n### Copy decisions:\r\n@florent-leborgne wrote
the copy for this deprecation subtype.\r\n<img width=\"1319\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/9a32f6d1-686a-4405-aec6-786ac5e10130\">\r\n\r\n<img
width=\"713\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/1304c98d-4c64-468e-a7d6-19c1193bf678\">\r\n\r\n\r\n##
Testing\r\n\r\nRun kibana locally with the test example plugin that has
deprecated\r\nroutes\r\n```\r\nyarn start
--plugin-path=examples/routing_example
--plugin-path=examples/developer_examples\r\n```\r\n\r\nThe following
comprehensive deprecated routes examples are registered\r\ninside the
folder:\r\n`examples/routing_example/server/routes/deprecated_routes`\r\n\r\nRun
them in the dev console to trigger the deprecation condition so
they\r\nshow up in the UA:\r\n\r\n```\r\nGET
kbn:/api/routing_example/d/internal_deprecated_route?elasticInternalOrigin=false\r\nGET
kbn:/internal/routing_example/d/internal_only_route?elasticInternalOrigin=false\r\nGET
kbn:/internal/routing_example/d/internal_versioned_route?apiVersion=1&elasticInternalOrigin=false\r\n```\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"a10eb1fe4e55aa0cfbbb4b12a8d740a867463283","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","v8.17.0"],"number":199026,"url":"https://github.com/elastic/kibana/pull/199026","mergeCommit":{"message":"[UA][Core]
Surface integrations with internal APIs in upgrade assistant
(#199026)\n\n## Summary\r\n\r\n> In
#117241 we're surfacing\r\nusage
of APIs marked as `deprecated: true` in the Upgrade Assistant to\r\nhelp
users prepare for a major upgrade. While internal APIs aren't\r\nreally
deprecated in the same sense we are making a breaking change
by\r\nblocking external integrations with these APIs. Since this could
be\r\nequally disruptive to users depending on these APIs it would help
our\r\nusers to surface such usage in the UA too.\r\n\r\nThe `api`
deprecations now have two sub types:\r\n1. routes deprecations
`options.deprecated: { … }`\r\n2. access deprecations `options.access:
'internal'`\r\n\r\nThis PR adds the second `api` deprecation subtype.
The reason i kept one\r\n`api` deprecation type and i didnt create a new
type is that they have\r\nexactly the same registration process but are
triggered by different\r\nattributes. The `api` deprecation is fully
managed by the core team\r\ninternal services and are configured by the
user through the route\r\ninterface so it makes sense to keep them as
one type. I also can see us\r\nadding more subtypes to this and just
piggybacking on the current flow\r\ninstead of duplicating it
everytime.\r\n\r\n\r\n**Checklist**\r\n- [x] Create deprecation
subtype\r\n- [x] Example plugin\r\n- [x] Surface the deprecation in
UA\r\n- [x] Api access deprecation copy (@florent-leborgne )\r\n- [x]
Update README and code annotations\r\n- [x] Unit tests\r\n- [x]
Integration tests\r\n\r\n\r\nCloses
https://github.com/elastic/kibana/issues/194675\r\n\r\n### Design
decisions:\r\nIf the API has both route deprecation
(`options.deprecated: { … }` ) AND\r\nis an internal api
`options.access: 'internal'`\r\n\r\nThe current behavior i went for in
my PR:\r\nI show this API once in the UA under the internal access
deprecation.\r\nWhile showing the route deprecation details if defined.
This seems to\r\nmake the most sense since users should stop using this
API altogether.\r\n\r\n### Copy decisions:\r\n@florent-leborgne wrote
the copy for this deprecation subtype.\r\n<img width=\"1319\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/9a32f6d1-686a-4405-aec6-786ac5e10130\">\r\n\r\n<img
width=\"713\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/1304c98d-4c64-468e-a7d6-19c1193bf678\">\r\n\r\n\r\n##
Testing\r\n\r\nRun kibana locally with the test example plugin that has
deprecated\r\nroutes\r\n```\r\nyarn start
--plugin-path=examples/routing_example
--plugin-path=examples/developer_examples\r\n```\r\n\r\nThe following
comprehensive deprecated routes examples are registered\r\ninside the
folder:\r\n`examples/routing_example/server/routes/deprecated_routes`\r\n\r\nRun
them in the dev console to trigger the deprecation condition so
they\r\nshow up in the UA:\r\n\r\n```\r\nGET
kbn:/api/routing_example/d/internal_deprecated_route?elasticInternalOrigin=false\r\nGET
kbn:/internal/routing_example/d/internal_only_route?elasticInternalOrigin=false\r\nGET
kbn:/internal/routing_example/d/internal_versioned_route?apiVersion=1&elasticInternalOrigin=false\r\n```\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"a10eb1fe4e55aa0cfbbb4b12a8d740a867463283"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/199026","number":199026,"mergeCommit":{"message":"[UA][Core]
Surface integrations with internal APIs in upgrade assistant
(#199026)\n\n## Summary\r\n\r\n> In
#117241 we're surfacing\r\nusage
of APIs marked as `deprecated: true` in the Upgrade Assistant to\r\nhelp
users prepare for a major upgrade. While internal APIs aren't\r\nreally
deprecated in the same sense we are making a breaking change
by\r\nblocking external integrations with these APIs. Since this could
be\r\nequally disruptive to users depending on these APIs it would help
our\r\nusers to surface such usage in the UA too.\r\n\r\nThe `api`
deprecations now have two sub types:\r\n1. routes deprecations
`options.deprecated: { … }`\r\n2. access deprecations `options.access:
'internal'`\r\n\r\nThis PR adds the second `api` deprecation subtype.
The reason i kept one\r\n`api` deprecation type and i didnt create a new
type is that they have\r\nexactly the same registration process but are
triggered by different\r\nattributes. The `api` deprecation is fully
managed by the core team\r\ninternal services and are configured by the
user through the route\r\ninterface so it makes sense to keep them as
one type. I also can see us\r\nadding more subtypes to this and just
piggybacking on the current flow\r\ninstead of duplicating it
everytime.\r\n\r\n\r\n**Checklist**\r\n- [x] Create deprecation
subtype\r\n- [x] Example plugin\r\n- [x] Surface the deprecation in
UA\r\n- [x] Api access deprecation copy (@florent-leborgne )\r\n- [x]
Update README and code annotations\r\n- [x] Unit tests\r\n- [x]
Integration tests\r\n\r\n\r\nCloses
https://github.com/elastic/kibana/issues/194675\r\n\r\n### Design
decisions:\r\nIf the API has both route deprecation
(`options.deprecated: { … }` ) AND\r\nis an internal api
`options.access: 'internal'`\r\n\r\nThe current behavior i went for in
my PR:\r\nI show this API once in the UA under the internal access
deprecation.\r\nWhile showing the route deprecation details if defined.
This seems to\r\nmake the most sense since users should stop using this
API altogether.\r\n\r\n### Copy decisions:\r\n@florent-leborgne wrote
the copy for this deprecation subtype.\r\n<img width=\"1319\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/9a32f6d1-686a-4405-aec6-786ac5e10130\">\r\n\r\n<img
width=\"713\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/1304c98d-4c64-468e-a7d6-19c1193bf678\">\r\n\r\n\r\n##
Testing\r\n\r\nRun kibana locally with the test example plugin that has
deprecated\r\nroutes\r\n```\r\nyarn start
--plugin-path=examples/routing_example
--plugin-path=examples/developer_examples\r\n```\r\n\r\nThe following
comprehensive deprecated routes examples are registered\r\ninside the
folder:\r\n`examples/routing_example/server/routes/deprecated_routes`\r\n\r\nRun
them in the dev console to trigger the deprecation condition so
they\r\nshow up in the UA:\r\n\r\n```\r\nGET
kbn:/api/routing_example/d/internal_deprecated_route?elasticInternalOrigin=false\r\nGET
kbn:/internal/routing_example/d/internal_only_route?elasticInternalOrigin=false\r\nGET
kbn:/internal/routing_example/d/internal_versioned_route?apiVersion=1&elasticInternalOrigin=false\r\n```\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"a10eb1fe4e55aa0cfbbb4b12a8d740a867463283"}},{"branch":"8.x","label":"v8.17.0","labelRegex":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Elastic Machine <[email protected]>
tkajtoch pushed a commit to tkajtoch/kibana that referenced this pull request Nov 12, 2024
…nt (elastic#199026)

## Summary

> In elastic#117241 we're surfacing
usage of APIs marked as `deprecated: true` in the Upgrade Assistant to
help users prepare for a major upgrade. While internal APIs aren't
really deprecated in the same sense we are making a breaking change by
blocking external integrations with these APIs. Since this could be
equally disruptive to users depending on these APIs it would help our
users to surface such usage in the UA too.

The `api` deprecations now have two sub types:
1. routes deprecations `options.deprecated: { … }`
2. access deprecations `options.access: 'internal'`

This PR adds the second `api` deprecation subtype. The reason i kept one
`api` deprecation type and i didnt create a new type is that they have
exactly the same registration process but are triggered by different
attributes. The `api` deprecation is fully managed by the core team
internal services and are configured by the user through the route
interface so it makes sense to keep them as one type. I also can see us
adding more subtypes to this and just piggybacking on the current flow
instead of duplicating it everytime.


**Checklist**
- [x] Create deprecation subtype
- [x] Example plugin
- [x] Surface the deprecation in UA
- [x] Api access deprecation copy (@florent-leborgne )
- [x] Update README and code annotations
- [x] Unit tests
- [x] Integration tests


Closes elastic#194675

### Design decisions:
If the API has both route deprecation (`options.deprecated: { … }` ) AND
is an internal api `options.access: 'internal'`

The current behavior i went for in my PR:
I show this API once in the UA under the internal access deprecation.
While showing the route deprecation details if defined. This seems to
make the most sense since users should stop using this API altogether.

### Copy decisions:
@florent-leborgne wrote the copy for this deprecation subtype.
<img width="1319" alt="image"
src="https://github.com/user-attachments/assets/9a32f6d1-686a-4405-aec6-786ac5e10130">

<img width="713" alt="image"
src="https://github.com/user-attachments/assets/1304c98d-4c64-468e-a7d6-19c1193bf678">


## Testing

Run kibana locally with the test example plugin that has deprecated
routes
```
yarn start --plugin-path=examples/routing_example --plugin-path=examples/developer_examples
```

The following comprehensive deprecated routes examples are registered
inside the folder:
`examples/routing_example/server/routes/deprecated_routes`

Run them in the dev console to trigger the deprecation condition so they
show up in the UA:

```
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
```

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Surface integrations with internal APIs in upgrade assistant
6 participants