[Serverless] Add internal uiSettings routes and expose public routes in self-managed only #160499

ElenaStoeva · 2023-06-26T09:54:15Z

Partially addresses #159590

Summary

This PR adds an an internal uiSettings API that is a duplicate of the public API and is intended for use by the browser-side uiSettings client.

The PR also adds a config settings that is configured in serverless context only and exposes the public uiSettings routes based on the value of this setting (it defaults to false since we don't want to expose the public routes in serverless).

How to test:

I. Verify that in serverless the internal routes are exposed but the public ones aren't:

Start Es with yarn es snapshot and Kibana with yarn serverless-{mode} where {mode} can be es, oblt, or security (the public routes should be disabled for all projects).
Verify that the public endpoints are not accessible. For example, curl --user elastic:changeme 'http://localhost:5601/zhb/api/kibana/settings' -X 'GET' should return {"statusCode":404,"error":"Not Found","message":"Not Found"}.
Verify that the internal endpoints are accessible. For example, curl --user elastic:changeme 'http://localhost:5601/zhb/internal/kibana/settings' -X 'GET' should return {"settings":{"buildNum":{"userValue":9007199254740991},"isDefaultIndexMigrated":{"userValue":true},"defaultRoute":{"isOverridden":true,"userValue":"/app/elasticsearch"}}}

II. Verify that the both public and internal routes are exposed in self-managed:

Start Es with yarn es snapshot and Kibana with yarn start
Verify that the public endpoints are accessible. For example, curl --user elastic:changeme 'http://localhost:5601/zhb/api/kibana/settings' -X 'GET' should return {"settings":{"buildNum":{"userValue":9007199254740991},"isDefaultIndexMigrated":{"userValue":true}}}
Verify that the internal endpoints are accessible. For example, curl --user elastic:changeme 'http://localhost:5601/zhb/internal/kibana/settings' -X 'GET' should return {"settings":{"buildNum":{"userValue":9007199254740991},"isDefaultIndexMigrated":{"userValue":true}}}

III. Verify that the plugins/services that consume the internal uiSettings endpoints work as expected in both self-managed and serverless environment.

For maintainers

This was checked for breaking API changes and was labeled appropriately

…-fix'

elasticmachine · 2023-06-28T11:33:42Z

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

pgayvallet

Approach looks fine to me, but PR still needs changes to be ready to merge.

pgayvallet · 2023-06-30T09:10:48Z

packages/core/ui-settings/core-ui-settings-server-internal/src/routes/delete.ts

@@ -51,15 +51,33 @@ export function registerDeleteRoute(router: InternalUiSettingsRouter) {
      throw error;
    }
  };
+  if (publicApiEnabled) {


General comment for all routes:

Either we decide that for now, those two API prefix (/api and /internal) should have the exact same behavior, in which case I would reuse the handlers, instead of duplicating them, e.g

const deleteHandler: HttpRequestHandler<forgotWhichGenerics> = async (context, request, response) => { const uiSettingsClient = (await context.core).uiSettings.client; return await deleteFromRequest(uiSettingsClient, context, request, response); } if (publicApiEnabled) { router.delete({ path: '/api/kibana/settings/{key}', validate }, deleteHandler); } router.delete({ path: '/internal/kibana/global_settings/{key}', validate, options: { access: 'internal' } }, deleteHandler)

Or we decide that the code may diverge, in which case the registration should be fully dissociated, with an individual file for the public and one for the internal API.

Given we decided to go the "public + internal" way in case we may want to make the APIs diverge, I would go with later.

We can for example have two folders:

packages/core/ui-settings/core-ui-settings-server-internal/src/routes/public

packages/core/ui-settings/core-ui-settings-server-internal/src/routes/internal

Or, what we sometimes do for Core routes:

packages/core/ui-settings/core-ui-settings-server-internal/src/routes/ <- public routes

packages/core/ui-settings/core-ui-settings-server-internal/src/routes/internal <- internal routes

I agree that separating the public and the internal routes in different files is the better option given what we discussed about potentially evolving the public routes in the future.
I took the Core routes approach for structuring the files (having public routes in /src/routes and internal ones in /src/routes/internal).

pgayvallet · 2023-06-30T09:16:15Z

packages/core/ui-settings/core-ui-settings-server-internal/src/routes/index.ts

-  registerDeleteRoute(router);
-  registerSetRoute(router);
-  registerSetManyRoute(router);
+export function registerRoutes(router: InternalUiSettingsRouter, publicApiEnabled: boolean = true) {


publicApiEnabled being optional is misleading and potentially error-prone. Just make the option mandatory and adapt existing usages (if not done already)

This argument is no longer needed after separating the public from the internal routes in different files.

packages/core/ui-settings/core-ui-settings-server-internal/src/ui_settings_config.ts

pgayvallet · 2023-06-30T09:19:09Z

packages/core/ui-settings/core-ui-settings-server-internal/src/ui_settings_service.ts

+    if (config.hasOwnProperty('publicApiEnabled')) {
+      registerRoutes(
+        http.createRouter<InternalUiSettingsRequestHandlerContext>(''),
+        config.publicApiEnabled
+      );
+    } else {
+      registerRoutes(http.createRouter<InternalUiSettingsRequestHandlerContext>(''));
+    }


NIT: unnecessary duplication

const publicApiEnabled = config.hasOwnProperty('publicApiEnabled') && config.publicApiEnabled === true; registerRoutes( http.createRouter<InternalUiSettingsRequestHandlerContext>(''), publicApiEnabled );

Sure, I refactored these lines of code, but I don't think the condition config.hasOwnProperty('publicApiEnabled') && config.publicApiEnabled === true would work for self-managed where this would evaluate to false since we don't have the publicApiEnabled setting configured, so I changed this logic a bit.

pgayvallet · 2023-06-30T09:21:13Z

src/core/server/integration_tests/ui_settings/routes.test.ts

    describe('set', () => {
      it('validates value', async () => {
        const response = await request
-          .post(root, '/api/kibana/settings/custom')
+          .post(root, '/internal/kibana/settings/custom')
          .send({ value: 100 })
          .expect(400);



We gonna need proper test coverage for the public routes too. Those tests needs to be running against the public routes (duplicating them is fine)

Sure, I added a duplicate set of tests for both public and internal routes.

elasticmachine · 2023-06-30T09:21:58Z

Pinging @elastic/apm-ui (Team:APM)

elasticmachine · 2023-06-30T09:21:58Z

Pinging @elastic/fleet (Team:Fleet)

ElenaStoeva · 2023-07-04T17:07:38Z

@elasticmachine merge upstream

ElenaStoeva · 2023-07-04T17:57:59Z

Thanks a lot for the review @pgayvallet! I made the changes that you suggested and left a couple of comments. Let me know if any additional changes are needed.

kibana-ci · 2023-07-04T19:01:33Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 64411f4

Failed CI Steps

Security Solution Tests #1

Test Failures

[job] [logs] Security Solution Tests #1 / Exceptions viewer read only "before each" hook for "Cannot add an exception from empty viewer screen"

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	363.5KB	363.5KB	+10.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	410	414	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	15	17	+2
`securitySolution`	489	493	+4
total			+6

History

💛 Build #138739 was flaky d016110
💔 Build #138567 failed 5a66397
💔 Build #138023 failed 4eb407e

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

cc @ElenaStoeva

pgayvallet

LGTM, thanks for the changes!

ElenaStoeva · 2023-07-07T10:16:41Z

@elastic/apm-ui and @elastic/security-solution, can you please verify that, with these changes, your plugins work as expected in self-managed and serverless environment?

sorenlouv

apm changes lgtm

jbudz

packages/kbn-test LGTM

YulNaumenko

Changes doesn't impact security_solution functionality. Could you please clarify why do we need to switch to internal APIs for the self-managed tests you've changed in security_solution?

ElenaStoeva · 2023-07-13T10:00:40Z

Changes doesn't impact security_solution functionality. Could you please clarify why do we need to switch to internal APIs for the self-managed tests you've changed in security_solution?

Thanks a lot for the review @YulNaumenko!

The internal routes are intended for use by the browser-side clients in both serverless and self-managed (sorry if that wasn't clear from the PR description). This is why I replaced all usages of the public routes across Kibana with the internal ones. The public API is intended for use by the user and will only be available in self-managed. Also, it may be changed or even removed in the future.
Do you think the security_solution tests should use the public routes instead of the internal ones?

dmlemeshko

Appex-QA changes LGTM

[Serverless] Add internal uiSettings routes

4eb407e

ElenaStoeva added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:uiSettings labels Jun 26, 2023

ElenaStoeva self-assigned this Jun 26, 2023

ElenaStoeva added release_note:skip Skip the PR/issue when compiling release notes v8.10.0 labels Jun 26, 2023

ElenaStoeva mentioned this pull request Jun 26, 2023

Disable Advanced Settings for serverless #159590

Closed

3 tasks

ElenaStoeva requested a review from a team June 26, 2023 17:08

ElenaStoeva and others added 5 commits June 27, 2023 16:32

Use internal endpoints from client-side instead of public ones

0427eb7

Merge branch 'main' into serverless-uisettings-routes

5ba09a8

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

5a66397

…-fix'

Update uiSettings api test snapshots

a4d261a

Merge branch 'main' into serverless-uisettings-routes

d016110

ElenaStoeva marked this pull request as ready for review June 28, 2023 11:33

ElenaStoeva requested review from a team as code owners June 28, 2023 11:33

pgayvallet reviewed Jun 30, 2023

View reviewed changes

botelastic bot added Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team labels Jun 30, 2023

juliaElastic approved these changes Jul 4, 2023

View reviewed changes

Make suggested changes

e6ee4c3

Merge branch 'main' into serverless-uisettings-routes

64411f4

pgayvallet approved these changes Jul 5, 2023

View reviewed changes

sorenlouv approved these changes Jul 7, 2023

View reviewed changes

jbudz approved these changes Jul 10, 2023

View reviewed changes

YulNaumenko approved these changes Jul 12, 2023

View reviewed changes

Dosant approved these changes Jul 13, 2023

View reviewed changes

dmlemeshko approved these changes Jul 14, 2023

View reviewed changes

ElenaStoeva merged commit 4105619 into elastic:main Jul 14, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jul 14, 2023

ElenaStoeva deleted the serverless-uisettings-routes branch July 14, 2023 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serverless] Add internal uiSettings routes and expose public routes in self-managed only #160499

[Serverless] Add internal uiSettings routes and expose public routes in self-managed only #160499

ElenaStoeva commented Jun 26, 2023 •

edited

Loading

elasticmachine commented Jun 28, 2023

pgayvallet left a comment

pgayvallet Jun 30, 2023 •

edited

Loading

ElenaStoeva Jul 4, 2023

pgayvallet Jun 30, 2023 •

edited

Loading

ElenaStoeva Jul 4, 2023

pgayvallet Jun 30, 2023

ElenaStoeva Jul 4, 2023

pgayvallet Jun 30, 2023

ElenaStoeva Jul 4, 2023

elasticmachine commented Jun 30, 2023

elasticmachine commented Jun 30, 2023

ElenaStoeva commented Jul 4, 2023

ElenaStoeva commented Jul 4, 2023

kibana-ci commented Jul 4, 2023

ESLint disabled line counts

Total ESLint disabled count

pgayvallet left a comment

ElenaStoeva commented Jul 7, 2023

sorenlouv left a comment

jbudz left a comment

YulNaumenko left a comment

ElenaStoeva commented Jul 13, 2023

dmlemeshko left a comment

[Serverless] Add internal uiSettings routes and expose public routes in self-managed only #160499

[Serverless] Add internal uiSettings routes and expose public routes in self-managed only #160499

Conversation

ElenaStoeva commented Jun 26, 2023 • edited Loading

Summary

For maintainers

elasticmachine commented Jun 28, 2023

pgayvallet left a comment

Choose a reason for hiding this comment

pgayvallet Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

ElenaStoeva Jul 4, 2023

Choose a reason for hiding this comment

pgayvallet Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

ElenaStoeva Jul 4, 2023

Choose a reason for hiding this comment

pgayvallet Jun 30, 2023

Choose a reason for hiding this comment

ElenaStoeva Jul 4, 2023

Choose a reason for hiding this comment

pgayvallet Jun 30, 2023

Choose a reason for hiding this comment

ElenaStoeva Jul 4, 2023

Choose a reason for hiding this comment

elasticmachine commented Jun 30, 2023

elasticmachine commented Jun 30, 2023

ElenaStoeva commented Jul 4, 2023

ElenaStoeva commented Jul 4, 2023

kibana-ci commented Jul 4, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

pgayvallet left a comment

Choose a reason for hiding this comment

ElenaStoeva commented Jul 7, 2023

sorenlouv left a comment

Choose a reason for hiding this comment

jbudz left a comment

Choose a reason for hiding this comment

YulNaumenko left a comment

Choose a reason for hiding this comment

ElenaStoeva commented Jul 13, 2023

dmlemeshko left a comment

Choose a reason for hiding this comment

ElenaStoeva commented Jun 26, 2023 •

edited

Loading

pgayvallet Jun 30, 2023 •

edited

Loading

pgayvallet Jun 30, 2023 •

edited

Loading