No longer use generated IDs for `X-Opaque-ID` / `requestId` #123197

pgayvallet · 2022-01-18T09:40:54Z

Summary

Fix #123737

rename KibanaRequest.id to opaqueId
no longer generate ids for KibanaRequest.opaqueId when the x-opaque-id header is not provided by the client

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

packages/kbn-server-http-tools/src/get_request_id.ts

src/core/server/http/http_server.ts

pgayvallet · 2022-01-18T10:37:32Z

x-pack/plugins/security/server/audit/audit_service.ts

@@ -166,7 +166,7 @@ export class AuditService {
            session_id: sessionId,
            ...event.kibana,
          },
-          trace: { id: request.id },
+          trace: { id: request.opaqueId },


@elastic/kibana-security request.id was renamed to request.opaqueId, but is now optional, as we're no longer generating a value when not provided by the client / when the configuration forbid its usage.

Is this change fine, or should we fallback to request.uuid when request.opaqueId is empty?

JFYI: Kibana sets tracing fields starting from #118466 The value from meta override them (sorry, I need to update the test suite name 😅 )

kibana/src/core/server/logging/layouts/json_layout.test.ts

Line 331 in b288f90

test('format() meta can not override tracing properties', () => {

maybe audit logging should rely on the trace field supplied by the Kibana server and not overwrite them?

#118466 was merged on 8.1 only, and not backported though, right?

yes, it wasn't. because it's not a bug fix

@elastic/kibana-security request.id was renamed to request.opaqueId, but is now optional, as we're no longer generating a value when not provided by the client / when the configuration forbid its usage.

Is this change fine, or should we fallback to request.uuid when request.opaqueId is empty?

I responded to the linked issue in #120124 (comment).

I'm not on board with this change in general as it hamstrings auditing functionality. If we have to go through with this though, we at least need the ability to correlate audit records within the Kibana audit log which originate from the same request. In that case it would be better to fall back to request.uuid.

pgayvallet · 2022-01-18T10:56:09Z

src/core/server/execution_context/execution_context_service.ts

    if (!this.enabled) return;
-    // requestId may not be present in the case of FakeRequest
+    // requestId may not be present if unspecified by the client or in the case of FakeRequest
    const requestId = this.requestIdStore.getStore()?.requestId ?? 'unknownId';
    const executionContext = this.contextStore.getStore()?.toString();
    const executionContextStr = executionContext ? `;kibana:${executionContext}` : '';


(This is correlated with the change in src/core/server/elasticsearch/client/cluster_client.ts)

ATM we kinda have an inconsistency between when EC is enabled or not, as when EC is enabled, the x-opaque-id value will be coming from EC.getAsHeader, and when it's not, the value is generated within ClusterClient.getScopedHeaders

ATM (with the PR's current change), the value matrix looks like:

EC enabled request.opaqueId present EC present x-opaque-id sent to ES

true true true '${opaqueId};kibana:${executionContext}'

true true false '${opaqueId}'

true false true 'unknownId;kibana:${executionContext}'

true false false 'unknownId'

false true true N/A

false true false '${opaqueId}'

false false true N/A

false false false undefined

I got a few questions here:

is this unknownId default value for opaqueId acceptable?

X-Opaque-ID contains UUID causing ES deduplication to fail #120124 (comment) suggest potentially using kibana as the default value, but then, in case of populated execution context, the value would look like kibana;kibana:${executionContext}. Are we fine with this format? Do we just want to use kibana:${executionContext} in that case instead?

When EC is disabled, the header is populated by ClusterClient.getScopedHeaders. ATM when request.opaqueId is not present, we're not sending any x-opaque-id header to ES. Do we want to change that to send kibana (or any constant value we agreed on from the previous point) instead?

(follow-up of next point) Should we decide to have the EC service the single source of truth for the x-opaque-id header and have getAsHeader always return a value even when disabled

this would require the requestIdStore to always be populated even when EC is disabled, which could be a performance issue as the whole purpose of allowing to disable the EC was for performances reasons

@mshustov @lukeelmers what do you think about this?

Are we fine with this format? Do we just want to use kibana:${executionContext} in that case instead?

Worth noting, ES doesn't parse x-opaque-id value, so we can use any format. I used kibana static value to simplify parsing when an incoming request contains x-opaque-id. If you find it easier to use kibana;${executionContext}, we can change the format.

ATM when request.opaqueId is not present, we're not sending any x-opaque-id header to ES. Do we want to change that to send kibana (or any constant value we agreed on from the previous point) instead?

@pgomulka what is better for the depreciation service of Elasticsearch?

Should we decide to have the EC service the single source of truth for the x-opaque-id header and have getAsHeader always return a value even when disabled

I didn't put this logic in the EC service to have a proper separation of concerns. The fact we put execution_context in x-opaque-id header of outbound requests is an implementation detail. When the buggage header is available, we will use it for context propagation. Having said that, I'd rather keep this logic on the elasticsearch service level. But we can refactor it a bit.

ATM when request.opaqueId is not present, we're not sending any x-opaque-id header to ES. Do we want to change that to send kibana (or any constant value we agreed on from the previous point) instead?

@pgomulka what is better for the depreciation service of Elasticsearch?

either way is good. As long as it is constant (or from a finite set). If Kibana wants to send a constant x-opaque-id-from-kibana it would help identifying its requests

I used kibana static value to simplify parsing when an incoming request contains x-opaque-id. If you find it easier to use kibana;${executionContext}, we can change the format.

Let's not overcomplicate the PR for no good reasons. The current format is good enough to address #120124, let's not change it for now. We'll reevaluate when we'll use the baggage header anyway.

src/core/server/http/router/request.ts

mshustov · 2022-01-18T14:50:30Z

src/core/server/execution_context/execution_context_service.ts

    if (!this.enabled) return;
-    // requestId may not be present in the case of FakeRequest
+    // requestId may not be present if unspecified by the client or in the case of FakeRequest
    const requestId = this.requestIdStore.getStore()?.requestId ?? 'unknownId';
    const executionContext = this.contextStore.getStore()?.toString();
    const executionContextStr = executionContext ? `;kibana:${executionContext}` : '';


Are we fine with this format? Do we just want to use kibana:${executionContext} in that case instead?

Worth noting, ES doesn't parse x-opaque-id value, so we can use any format. I used kibana static value to simplify parsing when an incoming request contains x-opaque-id. If you find it easier to use kibana;${executionContext}, we can change the format.

ATM when request.opaqueId is not present, we're not sending any x-opaque-id header to ES. Do we want to change that to send kibana (or any constant value we agreed on from the previous point) instead?

@pgomulka what is better for the depreciation service of Elasticsearch?

Should we decide to have the EC service the single source of truth for the x-opaque-id header and have getAsHeader always return a value even when disabled

I didn't put this logic in the EC service to have a proper separation of concerns. The fact we put execution_context in x-opaque-id header of outbound requests is an implementation detail. When the buggage header is available, we will use it for context propagation. Having said that, I'd rather keep this logic on the elasticsearch service level. But we can refactor it a bit.

src/core/server/http/http_server.ts

mshustov · 2022-01-18T15:00:38Z

x-pack/plugins/security/server/audit/audit_service.ts

@@ -166,7 +166,7 @@ export class AuditService {
            session_id: sessionId,
            ...event.kibana,
          },
-          trace: { id: request.id },
+          trace: { id: request.opaqueId },


JFYI: Kibana sets tracing fields starting from #118466 The value from meta override them (sorry, I need to update the test suite name 😅 )

kibana/src/core/server/logging/layouts/json_layout.test.ts

Line 331 in b288f90

test('format() meta can not override tracing properties', () => {

maybe audit logging should rely on the trace field supplied by the Kibana server and not overwrite them?

…-uuid

kibana-ci · 2022-02-01T12:00:53Z

💛 Build succeeded, but was flaky

Test Failures

[job] [logs] Default CI Group #10 / Uptime app with generated data certificates with certs page "before each" hook for "displays certificates"

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/server-http-tools`	50	49	-1

History

💚 Build #18735 succeeded c10bd2d
💔 Build #18689 failed 3256a6c
💔 Build #18685 failed 5e9efcf
💔 Build #17938 failed b288f90
💔 Build #17928 failed bc5aa3d
💔 Build #17926 failed 9734eb9

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

rudolf · 2023-01-24T14:26:19Z

Closing as stale PR

change getRequestId to no longer generates UUID

37bc63c

pgayvallet added v7.17.0 v8.0.0 v8.1.0 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 bug Fixes for quality problems that affect the customer experience labels Jan 18, 2022

pgayvallet added 4 commits January 18, 2022 11:00

start fixing stuff and tests

468efa6

fix plugin test data

9734eb9

fix audit service access

bc5aa3d

update comment

b288f90

pgayvallet commented Jan 18, 2022

View reviewed changes

mshustov reviewed Jan 18, 2022

View reviewed changes

pgayvallet mentioned this pull request Jan 19, 2022

X-Opaque-ID contains UUID causing ES deduplication to fail #120124

Closed

pgayvallet added 5 commits January 20, 2022 13:17

Merge remote-tracking branch 'upstream/main' into kbn-120124-opaqueId…

8067e4f

…-uuid

fix test

5e9efcf

improve tsdoc

fd2e8a7

update generated doc

3256a6c

fix uuid-base logic in test

c10bd2d

pgayvallet added Feature:elasticsearch and removed v8.0.0 v7.17.0 bug Fixes for quality problems that affect the customer experience labels Jan 25, 2022

Merge remote-tracking branch 'upstream/main' into kbn-120124-opaqueId…

f2d34a0

…-uuid

mshustov mentioned this pull request Feb 17, 2022

Improve deprecation log traceability elastic/elasticsearch#80232

Closed

pgayvallet mentioned this pull request May 9, 2022

Stop Kibana from generating unique X-Opaque-ID values when not received from the client. #123737

Open

lukeelmers mentioned this pull request Nov 16, 2022

Trace Elasticsearch query to the origin #97934

Closed

rudolf closed this Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No longer use generated IDs for `X-Opaque-ID` / `requestId` #123197

No longer use generated IDs for `X-Opaque-ID` / `requestId` #123197

pgayvallet commented Jan 18, 2022 •

edited

Loading

pgayvallet Jan 18, 2022

mshustov Jan 18, 2022 •

edited

Loading

pgayvallet Jan 18, 2022

mshustov Jan 18, 2022

jportner Jan 18, 2022

pgayvallet Jan 18, 2022

mshustov Jan 18, 2022

pgomulka Jan 18, 2022 •

edited

Loading

pgayvallet Jan 20, 2022

mshustov Jan 18, 2022

mshustov Jan 18, 2022 •

edited

Loading

kibana-ci commented Feb 1, 2022

rudolf commented Jan 24, 2023

EC enabled	request.opaqueId present	EC present	`x-opaque-id` sent to ES
true	true	true	`'${opaqueId};kibana:${executionContext}'`
true	true	false	`'${opaqueId}'`
true	false	true	`'unknownId;kibana:${executionContext}'`
true	false	false	`'unknownId'`
false	true	true	N/A
false	true	false	`'${opaqueId}'`
false	false	true	N/A
false	false	false	`undefined`

No longer use generated IDs for X-Opaque-ID / requestId #123197

No longer use generated IDs for X-Opaque-ID / requestId #123197

Conversation

pgayvallet commented Jan 18, 2022 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

mshustov Jan 18, 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

pgomulka Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Feb 1, 2022

💛 Build succeeded, but was flaky

Test Failures

Metrics [docs]

Public APIs missing comments

History

rudolf commented Jan 24, 2023

No longer use generated IDs for `X-Opaque-ID` / `requestId` #123197

No longer use generated IDs for `X-Opaque-ID` / `requestId` #123197

pgayvallet commented Jan 18, 2022 •

edited

Loading

mshustov Jan 18, 2022 •

edited

Loading

pgomulka Jan 18, 2022 •

edited

Loading

mshustov Jan 18, 2022 •

edited

Loading