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

[HTTP/OAS] Make SO CRUD and resolve APIs internal on serverless #184408

Merged
merged 11 commits into from
Jun 11, 2024
Merged

[HTTP/OAS] Make SO CRUD and resolve APIs internal on serverless #184408

merged 11 commits into from
Jun 11, 2024

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented May 29, 2024 •
edited
Loading

Summary

Add a bit of logic to explicitly make SO CRUD + resolve APIs public for stateful and internal for serverless to bring our OAS in line with our user facing docs. Also added descriptions to APIs (taken from #184184)

How to test

See added functional tests.

Related

@jloleysens
Copy link
Contributor Author

/ci

@jloleysens
Copy link
Contributor Author

/ci

TinaHeiligers
TinaHeiligers requested changes May 30, 2024
View reviewed changes
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.

we cannot make all the SO APIs public, anywhere!

packages/core/saved-objects/core-saved-objects-server-internal/src/routes/export.ts
@@ -143,6 +143,10 @@ export const registerExportRoute = (
router.post(
{
path: '/_export',
options: {
access: 'public',
Copy link
Contributor

Choose a reason for hiding this comment

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

✅

packages/core/saved-objects/core-saved-objects-server-internal/src/routes/bulk_update.ts
) => {
const { allowHttpApiAccess } = config;
router.put(
{
path: '/_bulk_update',
options: {
access,
Copy link
Contributor

Choose a reason for hiding this comment

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

@jloleysens Unless I'm missing some intricate detail, access will be internal according to https://github.com/elastic/kibana/pull/184408/files#diff-311e61dcad4f336ae3320406e2a114cc844823d4526d9c52c35c022d47516843R144.
The PR title had me worried you were making ALL the SO APIs public!

Copy link
Contributor Author

@jloleysens jloleysens May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Access will be set dynamically based on build flavour. Serverless will remain internal and stateful will remain public. This pr just brings those to bear in the code.

packages/core/saved-objects/core-saved-objects-server-internal/src/routes/import.ts
@@ -36,6 +36,8 @@ export const registerImportRoute = (
{
path: '/_import',
options: {
access: 'public',
Copy link
Contributor

Choose a reason for hiding this comment

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

✅

packages/core/saved-objects/core-saved-objects-server-internal/src/routes/index.ts

const internalOnServerless = isServerless ? 'internal' : 'public';

registerGetRoute(router, { config, coreUsageData, logger, access: internalOnServerless });
Copy link
Contributor

Choose a reason for hiding this comment

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

no

packages/core/saved-objects/core-saved-objects-server-internal/src/routes/index.ts
registerBulkUpdateRoute(router, { config, coreUsageData, logger });
registerBulkDeleteRoute(router, { config, coreUsageData, logger });

const internalOnServerless = isServerless ? 'internal' : 'public';
Copy link
Contributor

Choose a reason for hiding this comment

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

no. We cannot use internalOnServerless as is to globally define access for ALL the routes. If we do then we'd make every single one public on non-serverless!
We can't do that and still say we're removing them in v9.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 , this is only used for the APIs documented as public, but that we want internal on serverless. I can add a comment to that effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jloleysens I'll approve this PR as long as we're not contradicting our public docs that clearly state which APIs are deprecated.

@jloleysens jloleysens changed the title [HTTP/OAS] Make SO APIs public on stateful and internal on serverless [HTTP/OAS] Make SO CRUD and resolve APIs public on stateful and internal on serverless May 31, 2024
@jloleysens jloleysens changed the title [HTTP/OAS] Make SO CRUD and resolve APIs public on stateful and internal on serverless [HTTP/OAS] Make SO CRUD and resolve APIs internal on serverless May 31, 2024
TinaHeiligers
TinaHeiligers approved these changes Jun 4, 2024
View reviewed changes
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.

As long as we're not contradicting our public documentation that lists the APIs that are deprecated, I'm ok with this PR.

TinaHeiligers
TinaHeiligers approved these changes Jun 4, 2024
View reviewed changes
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.

approving on behalf of core

@jloleysens
Copy link
Contributor Author

/ci

@jloleysens jloleysens marked this pull request as ready for review June 4, 2024 09:07
@jloleysens jloleysens requested a review from a team as a code owner June 4, 2024 09:07
jloleysens
jloleysens commented Jun 4, 2024
View reviewed changes
packages/core/saved-objects/core-saved-objects-server-internal/src/routes/index.ts
registerBulkCreateRoute(router, { config, coreUsageData, logger, access: internalOnServerless });
registerBulkResolveRoute(router, { config, coreUsageData, logger, access: internalOnServerless });
registerBulkUpdateRoute(router, { config, coreUsageData, logger, access: internalOnServerless });
registerBulkDeleteRoute(router, { config, coreUsageData, logger, access: internalOnServerless });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not add tests for the existing public behaviour as I assume these tests already give us that coverage:

kibana/src/core/server/integration_tests/saved_objects/routes/bulk_get.test.ts

Line 85 in e3ca2c5

const result = await supertest(httpSetup.server.listener)

@jloleysens jloleysens added Feature:http Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.15.0 Feature:OAS Work or issues related to Core-provided mechanisms for generating OAS labels Jun 4, 2024
@elasticmachine
Copy link
Contributor

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

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Unknown metric groups

API count

id before after diff
@kbn/core-http-server 487 488 +1

History

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

@jloleysens jloleysens merged commit dc78221 into elastic:main Jun 11, 2024
21 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 11, 2024
@jloleysens jloleysens deleted the http/make-so-apis-public-on-stateful branch June 11, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TinaHeiligers TinaHeiligers TinaHeiligers approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:http Feature:OAS Work or issues related to Core-provided mechanisms for generating OAS release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jloleysens @elasticmachine @kibana-ci @pgayvallet @TinaHeiligers @kibanamachine