[http] Do not do client version check on serverless as we do for onprem #159101

jloleysens · 2023-06-06T11:15:56Z

Summary

This PR introduces two changes:

(1) Refactors the handler resolution logic to not depend on the --serverless cli arg by adding a new piece of config server.versioned.routeResolution that accepts newest | oldest. This piece of config is passed down instead of the serverless cli arg as well as updating test cases

(2) Adds a new piece of config to turn off the client version checking. This will be needed for rolling upgrades to allow old browser traffic to reach new Kibana servers when there is stack version change.

Close #158723

Open questions

Do we want to make the version check still take major version bumps into account?

…gured, we do not do client version checks

jloleysens · 2023-06-06T12:01:27Z

src/core/server/integration_tests/http/lifecycle_handlers.test.ts

+  it('accepts requests with any version passed in the version header', async () => {
+    await supertest(innerServer.listener)
+      .get(testRoute)
+      .set(versionHeader, 'what-have-you')
+      .expect(200, 'ok');
+  });


This is the most important change/difference. We do not block for version headers.

jloleysens · 2023-06-06T12:01:54Z

packages/core/http/core-http-server-internal/src/http_config.ts

+      handlerResolution: schema.oneOf([schema.literal('newest'), schema.literal('oldest')], {
+        defaultValue: 'oldest',
+      }),


Bring this more in line with other ways of configuring Kibana for serverless

jloleysens · 2023-06-06T12:02:38Z

src/core/server/integration_tests/http/lifecycle_handlers.test.ts

@@ -31,97 +27,29 @@ const setupDeps = {
  executionContext: executionContextServiceMock.createInternalSetupContract(),
 };

-interface HttpConfigTestOptions {


Removed and handled by the updated createConfigService utility.

afharo · 2023-06-06T13:06:57Z

packages/core/http/core-http-server-internal/src/register_lifecycle_handlers.ts

+  if (config.versioned.strictClientVersionCheck !== false) {
+    // add check on version
+    registrar.registerOnPostAuth(createVersionCheckPostAuthHandler(env.packageInfo.version));
+  }


nit: I wonder if we still want to warn via some custom header in the response.

This would help when troubleshooting HARs, or even, potentially using that on the browser to warn the user that they should refresh the page.

WDYT?

I like that idea! As you suggest, this is one way to drive the "update available" UX we've spoken about... Currently it will only detect a new version when we ship a new stack version --- this will not be sufficient for more continuous updates. I think updating the Kibana version to contain a git hash or build nr or something would be needed...

I'd like to spend a bit more time thinking about how we want to approach that specifically. Do you think it is still worth adding a header like this now? If so, do you think we should just reuse the existing kbn-version header and set it to update-available or out-of-sync?

Opened #159127

IMO, we should just include the version in the header all the time, and add metadata to our logging-- not just error logging-- with the client version and server version. That metadata will be invaluable to not only knowing if a mismatch is contributing to an error, but also if an error has spiked on a particular version of client + server, and how many clients are "lagging" the upgrade curve.

The version mismatch will drive the UX, for certain, but I would love to have requests and errors tagged all the time. The mismatch will be one data point in an investigation... an important one, but not the only one.

elasticmachine · 2023-06-06T14:35:09Z

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

pgayvallet

Looking good to me as a first stage

config/serverless.yml

pgayvallet · 2023-06-08T07:00:07Z

packages/core/http/core-http-router-server-internal/src/router.ts

+    private readonly options: RouterOptions = {
+      isDev: false,
+      versionedRouteResolution: 'oldest',
+    }


NIT: I know it was done to avoid having to change all the constructor calls, but I wonder how safe having this options be an optional parameter is. I don't think we're using the Router in that many places, so maybe making options mandatory now could make sense, WDYT?

Sounds good to me!

pgayvallet · 2023-06-08T07:02:08Z

packages/core/http/core-http-server-internal/src/http_config.ts

+    versioned: schema.object({
+      // Which handler resolution algo to use: "newest" or "oldest"
+      handlerResolution: schema.oneOf([schema.literal('newest'), schema.literal('oldest')], {
+        defaultValue: 'oldest',
+      }),


NIT: Handler resolution or version resolution? Is the "oldest handler" really a valid term?

Will change to versionResolution

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

pgayvallet · 2023-06-08T07:12:24Z

packages/core/http/core-http-server-mocks/src/test_utils.ts

+export const createConfigService = ({
+  server,
+  externalUrl,
+  csp,
+}: Partial<{
+  server: Partial<HttpConfigType>;
+  externalUrl: Partial<ExternalUrlConfigType>;
+  csp: Partial<CspConfigType>;
+}> = {}) => {


👍 the stub config definition is way better that way

* main: (199 commits) [Lens] Add custom formatter within the Lens editor (elastic#158468) [Uptime] Hide app if no data is available (elastic#159118) [CodeEditor] Add grok highlighting (elastic#159334) Expose decoded cloudId components from the cloud plugin's contract (elastic#159442) [Profiling] Use collector and symbolizer integrations in the add data page (elastic#159481) [Infrastructure UI] Hosts View: Unified Search bar with auto-refresh enabled (elastic#157011) [APM] Add feature flag for not available apm schema (elastic#158911) [Lens] Remove deprecated componentWillReceiveProps usage (elastic#159502) [api-docs] 2023-06-13 Daily api_docs build (elastic#159536) [data views] Use versioned router for REST routes (elastic#158608) [maps] fix geo line source not loaded unless maps application is opened (elastic#159432) [Enterprise Search][Search application]Fix Create Api key url (elastic#159519) [Security Solution] Increase timeout for indexing hosts (elastic#159518) dashboard Reset button disable (elastic#159430) [Security Solution] Unskip Endpoint API tests after package fix (elastic#159484) [Synthetics] adjust alert timing (elastic#159511) [ResponseOps][rule registry] Remove usages of `refresh: true` (elastic#159252) Revert "Remove unused package (elastic#158597)" [Serverless] Adding config to disable authentication on task manager background worker utilization API (elastic#159505) Remove unused package (elastic#158597) ...

kibana-ci · 2023-06-13T12:12:56Z

💚 Build Succeeded

Buildkite Build
Commit: a67fd82

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-http-server-internal`	51	52	+1
`@kbn/core-http-server-mocks`	40	42	+2
total			+3

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-http-server-internal`	57	58	+1
`@kbn/core-http-server-mocks`	41	43	+2
total			+3

ESLint disabled line counts

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

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	17	19	+2
`securitySolution`	491	495	+4
total			+6

History

💚 Build #132923 succeeded 09d76f5
💔 Build #132909 failed 4e44fbe
💔 Build #132856 failed d21ae9a

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

cc @jloleysens

jloleysens added 8 commits June 6, 2023 11:03

add the "version" section to http

450926f

added versioned config to HttpConfig class

15399e1

refactor to use value from config

da8b1a4

update snapshot and serverless.yml

4e722e6

update versioned router test and mocks

dbcef06

update test

67e6187

added new file registration logic

8b31fcf

refactor lifecycle handler test and added test to check that if confi…

d21ae9a

…gured, we do not do client version checks

jloleysens changed the title ~~[http] Do not do version check on serverless~~ [http] Do not do client version check on serverless as we do for onprem Jun 6, 2023

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.9.0 labels Jun 6, 2023

jloleysens self-assigned this Jun 6, 2023

jloleysens commented Jun 6, 2023

View reviewed changes

fix failing tests

4e44fbe

afharo reviewed Jun 6, 2023

View reviewed changes

???

09d76f5

jloleysens mentioned this pull request Jun 6, 2023

[HTTP] How will Kibana server detect a different version UI on serverless? #159127

Closed

jloleysens marked this pull request as ready for review June 6, 2023 14:35

jloleysens requested a review from a team as a code owner June 6, 2023 14:35

pgayvallet approved these changes Jun 8, 2023

View reviewed changes

jloleysens added 7 commits June 13, 2023 12:34

address yaml nit

52d585c

make router options non-optional

d827f55

handlerResolution -> versionResolution

84dfedc

move lifecycle handler registration to own file

f12482a

update snapshot

4850551

fix types

bd0ea15

jloleysens enabled auto-merge (squash) June 13, 2023 11:10

jloleysens merged commit 7d07149 into elastic:main Jun 13, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jun 13, 2023

jloleysens deleted the do-not-do-version-check-on-serverless branch June 13, 2023 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[http] Do not do client version check on serverless as we do for onprem #159101

[http] Do not do client version check on serverless as we do for onprem #159101

jloleysens commented Jun 6, 2023 •

edited

Loading

jloleysens Jun 6, 2023

jloleysens Jun 6, 2023

jloleysens Jun 6, 2023

afharo Jun 6, 2023

jloleysens Jun 6, 2023

jloleysens Jun 6, 2023

clintandrewhall Jul 10, 2023 •

edited

Loading

elasticmachine commented Jun 6, 2023

pgayvallet left a comment

pgayvallet Jun 8, 2023

jloleysens Jun 13, 2023

pgayvallet Jun 8, 2023

jloleysens Jun 13, 2023

pgayvallet Jun 8, 2023

kibana-ci commented Jun 13, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

[http] Do not do client version check on serverless as we do for onprem #159101

[http] Do not do client version check on serverless as we do for onprem #159101

Conversation

jloleysens commented Jun 6, 2023 • edited Loading

Summary

Open questions

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

clintandrewhall Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jun 6, 2023

pgayvallet left a 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

kibana-ci commented Jun 13, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

ESLint disabled line counts

Total ESLint disabled count

History

jloleysens commented Jun 6, 2023 •

edited

Loading

clintandrewhall Jul 10, 2023 •

edited

Loading