[Cases] Deprecate endpoints #124773

cnasikas · 2022-02-06T17:05:41Z

Summary

This PR deprecates the following Cases endpoints:

GET /api/cases/{case_id}: Deprecate includeComments query parameter.
GET: /api/cases/{case_id}/comments: Deprecate endpoint
GET: /api/cases//user_actions: Deprecate endpoint
GET: /api/cases/status: Deprecate endpoint

It follows the recommendations specified in: #105692

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

Release notes

The following Cases API endpoints are deprecated:

The includeComments query parameter of the GET /api/cases/{case_id} endpoint is deprecated.

x-pack/plugins/cases/server/plugin.ts

elasticmachine · 2022-02-07T11:27:08Z

Pinging @elastic/response-ops (Team:ResponseOps)

elasticmachine · 2022-02-07T11:27:09Z

Pinging @elastic/response-ops-cases (Feature:Cases)

x-pack/test/cases_api_integration/common/lib/utils.ts

pmuellr · 2022-02-07T16:26:26Z

x-pack/plugins/cases/server/routes/api/cases/get_case.ts

@@ -30,6 +33,9 @@ export function initGetCaseApi({ router, logger }: RouteDeps) {
        const id = request.params.case_id;

        return response.ok({
+          headers: {


It looks like the warning header will be returned on every call to this endpoint - but don't you just want the header if they use the query.includeComments property?

Oh, good point! I will update accordingly.

@pmuellr I just noticed that we default it to true if the user omits it so I don't think we can distinguish if the user provided the parameter or not.

Ah, bummer.

You could change the type to boolean | undefined, and then apply the default outside of the validation, but that's clumsy, and changes your typing which could require more undefined checks. Perhaps instead, since false is the "deprecated" version, you can just check after the validation if it's false and then add the header? So it wouldn't be marked deprecated if they used the param and it was true.

Another possibility would be to see if there's a way to get the parameters in more of a "raw" state, after the validation, from the request. And then put the check there.

I really don't think we want this header used every time the API is used.

@pmuellr Amazing idea about the "raw" state. I pushed the changes you suggested.

cnasikas · 2022-02-08T17:01:02Z

x-pack/plugins/cases/server/routes/api/user_actions/get_all_user_actions.ts

@@ -27,10 +30,15 @@ export function initGetAllCaseUserActionsApi({ router, logger }: RouteDeps) {
          return response.badRequest({ body: 'RouteHandlerContext is not registered for cases' });
        }

+        logger.warn(`The get all cases user actions API '${CASE_USER_ACTIONS_URL}' is deprecated.`);


@lukeelmers @kobelb Cases are gonna use these endpoints until we provide alternatives. Is it ok if we log warnings even when Cases are using them?

We don't wanna use logger.warn for deprecations. "warn" is a high logging level that should be used when things are going seriously awry.

@elastic/kibana-core: did core end up exposing a logger that we should be using for intermittent deprecation usage, for example when a client is calling a HTTP API in a deprecated manner?

did core end up exposing a logger that we should be using for intermittent deprecation usage

@kobelb Unfortunately we do not have something like this. There are a few other places in Kibana I am aware of (specifically in Core & Security) where we are logging deprecations for routes at a warn level.

We have other work we want to do in this area to improve the deprecation process for HTTP APIs:

[core.http] Support Warning headers for deprecated http routes #105692

Support Kibana deprecation logging in Upgrade Assistant #117241

But to my knowledge we do not have an issue for what you're specifically mentioning here (that is, a dedicated logger exposed to http routes for logging deprecations specifically).

Cases are gonna use these endpoints until we provide alternatives. Is it ok if we log warnings even when Cases are using them?

My main concern would be if we are logging deprecation warnings when users aren't initiating the calls to the deprecated API. I would recommend either adding some logic to try to differentiate between Kibana & non-Kibana requests, or perhaps creating a duplicate route that's just for use by Cases, and simply don't log deprecations from that route.

If you want to attempt to differentiate between Kibana & non-Kibana requests & conditionally log based on that info, there is some prior art in the spaces plugin: https://github.com/elastic/kibana/blob/ec30f2aeeb10fb64b507935e558832d3ef5abfaa/x-pack/plugins/spaces/server/usage_stats/usage_stats_client.ts#L113-L118

This is an imperfect workaround, but will address the majority of situations.

Just dropping by to add another related issue: #112291

Thank you @lukeelmers! I will go with adding some logic to differentiate between Kibana & non-Kibana requests. We plan to introduce different endpoints as alternatives in the future so I will switch Cases to them when the time comes. Thanks!

If there's precedent for using the warn level for this and we haven't had customers complain, feel free to follow suit @cnasikas.

I do agree that we shouldn't be logging these deprecations for internal HTTP requests. One other option we could explore is having /api public routes and /internal internal routes, and not doing the deprecation logging for the internal routes.

cnasikas · 2022-02-09T13:30:41Z

@elasticmachine merge upstream

kobelb

If we don't have a way to log these deprecations at the moment, we can proceed with the current PR that includes with the Warning response header and add logging in a subsequent PR. I don't see any other Kibana HTTP endpoints including the warning response header, but what you have looks fine by me.

kobelb · 2022-02-09T22:38:02Z

x-pack/plugins/cases/server/routes/api/user_actions/get_all_user_actions.ts

@@ -27,10 +30,15 @@ export function initGetAllCaseUserActionsApi({ router, logger }: RouteDeps) {
          return response.badRequest({ body: 'RouteHandlerContext is not registered for cases' });
        }

+        logger.warn(`The get all cases user actions API '${CASE_USER_ACTIONS_URL}' is deprecated.`);


We don't wanna use logger.warn for deprecations. "warn" is a high logging level that should be used when things are going seriously awry.

@elastic/kibana-core: did core end up exposing a logger that we should be using for intermittent deprecation usage, for example when a client is calling a HTTP API in a deprecated manner?

… deprecate_endpoints

lukeelmers

Changes to the route deprecation LGTM

I don't see any other Kibana HTTP endpoints including the warning response header, but what you have looks fine by me.

I missed this comment earlier, but this is correct -- AFAIK there are no other places where we are manually implementing a Warning header. This implementation seems fine, however once we are able to address #105692, it will be good to come back and change this implementation to be consistent with our long-term solution.

cnasikas · 2022-02-11T09:51:05Z

@elasticmachine merge upstream

kibana-ci · 2022-02-11T11:41:48Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #23206 failed bac177c
💚 Build #22764 succeeded 1c81792
💛 Build #22543 was flaky eb3c26a
💔 Build #22169 failed d59f1b9
💔 Build #22102 failed 8e4a776

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @cnasikas

cnasikas · 2022-02-11T12:03:12Z

Changes to the route deprecation LGTM

I don't see any other Kibana HTTP endpoints including the warning response header, but what you have looks fine by me.

I missed this comment earlier, but this is correct -- AFAIK there are no other places where we are manually implementing a Warning header. This implementation seems fine, however once we are able to address #105692, it will be good to come back and change this implementation to be consistent with our long-term solution.

Thank you @lukeelmers! Yes, when core supports it we will come back and change it.

kibanamachine · 2022-02-11T12:11:50Z

💔 All backports failed

Status	Branch	Result
❌	8.1	Backport failed because of merge conflicts

How to fix

Re-run the backport manually:

node scripts/backport --pr 124773

Questions ?

Please refer to the Backport tool documentation

Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit eb7591b) # Conflicts: # x-pack/plugins/cases/server/routes/api/stats/get_status.ts

Deprecate endpoints

4100b52

cnasikas self-assigned this Feb 6, 2022

cnasikas added Feature:Cases Cases feature Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.1.0 v8.2.0 labels Feb 6, 2022

Pass kibana version

0bdefb6

cnasikas commented Feb 7, 2022

View reviewed changes

x-pack/plugins/cases/server/plugin.ts Show resolved Hide resolved

cnasikas added 2 commits February 7, 2022 13:23

Add tests

85d0e0c

Fix types

15bb9d9

cnasikas marked this pull request as ready for review February 7, 2022 11:27

cnasikas requested a review from a team as a code owner February 7, 2022 11:27

cnasikas added release_note:deprecation release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Feb 7, 2022

cnasikas requested review from lukeelmers and pgayvallet February 7, 2022 12:22

Merge branch 'main' into deprecate_endpoints

8e4a776

cnasikas requested a review from kobelb February 7, 2022 15:49

cnasikas force-pushed the deprecate_endpoints branch from c7b55e7 to 4ea4cee Compare February 7, 2022 15:54

Move kibana version to route deps

d4d8fe2

cnasikas force-pushed the deprecate_endpoints branch from 4ea4cee to d4d8fe2 Compare February 7, 2022 15:55

jonathan-buttner approved these changes Feb 7, 2022

View reviewed changes

x-pack/test/cases_api_integration/common/lib/utils.ts Outdated Show resolved Hide resolved

Move assertWarningHeader to validations

d59f1b9

pmuellr reviewed Feb 7, 2022

View reviewed changes

cnasikas added 4 commits February 8, 2022 15:51

Deprecate status endpoint

7b6cf4f

Fix validation

84ef3b4

Integration test for status API

e63e3cf

Fix types

3bebd6b

Log warnings about deprecated endpoints

eb3c26a

cnasikas commented Feb 8, 2022

View reviewed changes

Warn user only if provided the includeComment query param

4d293cc

Merge branch 'main' into deprecate_endpoints

1c81792

kobelb reviewed Feb 9, 2022

View reviewed changes

cnasikas added 3 commits February 10, 2022 18:23

Merge branch 'main' into deprecate_endpoints

c17b0d2

Log only request from external clients

07598ab

Merge branch 'deprecate_endpoints' of github.com:cnasikas/kibana into…

bac177c

… deprecate_endpoints

lukeelmers approved these changes Feb 10, 2022

View reviewed changes

Merge branch 'main' into deprecate_endpoints

62e5d01

cnasikas merged commit eb7591b into elastic:main Feb 11, 2022

cnasikas deleted the deprecate_endpoints branch February 11, 2022 12:03

cnasikas added the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 11, 2022

cnasikas mentioned this pull request Feb 11, 2022

[8.1] [Cases] Deprecate endpoints (#124773) #125388

Merged

cnasikas added a commit that referenced this pull request Feb 11, 2022

[Cases] Deprecate endpoints (#124773) (#125388)

5ef6caa

Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit eb7591b) # Conflicts: # x-pack/plugins/cases/server/routes/api/stats/get_status.ts

cnasikas removed the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Feb 14, 2022

xcrzx mentioned this pull request Mar 17, 2022

[Security Solution] Deprecate some of the Bulk API endpoints #127756

Closed

9 tasks

cnasikas mentioned this pull request Jun 14, 2022

[Cases] Create new endpoints to replace the deprecated ones. #134344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Deprecate endpoints #124773

[Cases] Deprecate endpoints #124773

cnasikas commented Feb 6, 2022 •

edited

Loading

elasticmachine commented Feb 7, 2022

elasticmachine commented Feb 7, 2022

pmuellr Feb 7, 2022

cnasikas Feb 7, 2022

cnasikas Feb 8, 2022 •

edited

Loading

pmuellr Feb 8, 2022

cnasikas Feb 9, 2022

cnasikas Feb 8, 2022

kobelb Feb 9, 2022

lukeelmers Feb 9, 2022

afharo Feb 10, 2022

cnasikas Feb 10, 2022

kobelb Feb 10, 2022 •

edited

Loading

cnasikas commented Feb 9, 2022

kobelb left a comment

kobelb Feb 9, 2022

lukeelmers left a comment

cnasikas commented Feb 11, 2022

kibana-ci commented Feb 11, 2022

cnasikas commented Feb 11, 2022

kibanamachine commented Feb 11, 2022

[Cases] Deprecate endpoints #124773

[Cases] Deprecate endpoints #124773

Conversation

cnasikas commented Feb 6, 2022 • edited Loading

Summary

Checklist

For maintainers

Release notes

elasticmachine commented Feb 7, 2022

elasticmachine commented Feb 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

cnasikas commented Feb 9, 2022

kobelb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

cnasikas commented Feb 11, 2022

kibana-ci commented Feb 11, 2022

💚 Build Succeeded

Metrics [docs]

History

cnasikas commented Feb 11, 2022

kibanamachine commented Feb 11, 2022

💔 All backports failed

How to fix

Questions ?

cnasikas commented Feb 6, 2022 •

edited

Loading

cnasikas Feb 8, 2022 •

edited

Loading

kobelb Feb 10, 2022 •

edited

Loading