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

Jump to bottom

Surface integrations with internal APIs in upgrade assistant #194675

Closed
rudolf opened this issue Oct 2, 2024 · 3 comments · Fixed by #199026
Closed

Surface integrations with internal APIs in upgrade assistant #194675

rudolf opened this issue Oct 2, 2024 · 3 comments · Fixed by #199026
Assignees
@Bamieh
Labels
Epic:Deprecations Deprecations Feature:Upgrade Assistant Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.17.0

Comments

@rudolf
Copy link
Contributor

rudolf commented Oct 2, 2024

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.
@rudolf rudolf added Feature:Upgrade Assistant Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v9.0.0 labels Oct 2, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@rudolf
Copy link
Contributor Author

rudolf commented Oct 2, 2024

@jloleysens suggested in #193792 (comment):

We should be able to piggy-back off work Ahmad is doing. He is creating a core usage counter for HTTP stuff that we can use to make a new route counter. The "internal route usage" counter should be registered during setup time.
As for where we detect/increment this counter, I think we should do it just before returning the 400 here:

kibana/packages/core/http/core-http-server-internal/src/lifecycle_handlers.ts

Line 57 in 244ff7b

return response.badRequest({

That handler will just need the core usage service injected. But for that we are blocked on #193668

@rudolf rudolf mentioned this issue Oct 2, 2024
Merged
4 tasks
@TinaHeiligers TinaHeiligers self-assigned this Oct 22, 2024
@TinaHeiligers TinaHeiligers added the Epic:Deprecations Deprecations label Oct 22, 2024
@TinaHeiligers
Copy link
Contributor

Locally, tests are passing. I need to do a few more checks before pushing a draft.

@Bamieh Bamieh assigned Bamieh and unassigned TinaHeiligers Oct 31, 2024
@Bamieh Bamieh mentioned this issue Nov 5, 2024
Merged
7 tasks
@Bamieh Bamieh added v8.17.0 and removed v8.18.0 labels Nov 11, 2024
@Bamieh Bamieh closed this as completed in a10eb1f Nov 12, 2024
tkajtoch pushed a commit to tkajtoch/kibana that referenced this issue Nov 12, 2024
@Bamieh @kibanamachine @tkajtoch
[UA][Core] Surface integrations with internal APIs in upgrade assista…
1062027 
…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]>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this issue Nov 18, 2024
@Bamieh @kibanamachine @CAWilson94
[UA][Core] Surface integrations with internal APIs in upgrade assista…
952232d 
…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
Assignees

@Bamieh Bamieh

Labels
Epic:Deprecations Deprecations Feature:Upgrade Assistant Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.17.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[UA][Core] Surface integrations with internal APIs in upgrade assistant Bamieh/kibana
5 participants
@rudolf @Bamieh @TinaHeiligers @elasticmachine @rayafratkina