request_id: Add option to always set request id in response #10808

euroelessar · 2020-04-16T07:51:06Z

Description: Add configuration option to return x-request-id in all cases, even if tracing is not forced.
Risk Level: LOW (disabled by default)
Testing: unit tests
Docs Changes: proto documentation
Release Notes: updated
Fixes #10807

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

repokitteh-read-only · 2020-04-16T07:51:12Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10808 was opened by euroelessar.

see: more, trace.

euroelessar · 2020-04-16T07:53:03Z

This is a followup for one of the discussion branches in #10429.

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-04-16T09:37:39Z

clang-tidy looks like false positive:

2020-04-16T09:26:08.1123546Z source/common/http/conn_manager_config.h:14:10: error: 'common/stats/symbol_table_impl.h' file not found [clang-diagnostic-error]
2020-04-16T09:26:08.1124954Z #include "common/stats/symbol_table_impl.h"
2020-04-16T09:26:08.1125360Z          ^
2020-04-16T09:26:08.1129473Z 
2020-04-16T09:26:08.1129806Z 59922 warnings and 1 error generated.
2020-04-16T09:26:08.1130302Z Error while processing /source/source/common/http/conn_manager_config.h.

Coverage looks like flake:

[----------] 1 test from EvironmentCredentialsProviderTest
[ RUN      ] EvironmentCredentialsProviderTest.MissingAccessKeyId
test/extensions/common/aws/credentials_provider_impl_test.cc:51: Failure
Value of: credentials.accessKeyId().has_value()
  Actual: true
Expected: false
Stack trace:
  0x8c2e26f: Envoy::Extensions::Common::Aws::EvironmentCredentialsProviderTest_MissingAccessKeyId_Test::TestBody()
  0x121efa46: testing::internal::HandleSehExceptionsInMethodIfSupported<>()
  0x121dc011: testing::internal::HandleExceptionsInMethodIfSupported<>()
  0x121c68e2: testing::Test::Run()
  0x121c73e8: testing::TestInfo::Run()
... Google Test internal frames ...

test/extensions/common/aws/credentials_provider_impl_test.cc:52: Failure
Value of: credentials.secretAccessKey().has_value()
  Actual: true
Expected: false
Stack trace:
  0x8c2e43f: Envoy::Extensions::Common::Aws::EvironmentCredentialsProviderTest_MissingAccessKeyId_Test::TestBody()
  0x121efa46: testing::internal::HandleSehExceptionsInMethodIfSupported<>()
  0x121dc011: testing::internal::HandleExceptionsInMethodIfSupported<>()
  0x121c68e2: testing::Test::Run()
  0x121c73e8: testing::TestInfo::Run()
... Google Test internal frames ...

[  FAILED  ] EvironmentCredentialsProviderTest.MissingAccessKeyId (1420 ms)

htuch

API looks good, just some comment nits.

htuch · 2020-04-17T01:21:46Z

...voy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto

@@ -433,6 +433,11 @@ message HttpConnectionManager {
  // is the current Envoy behaviour. This defaults to false.
  bool preserve_external_request_id = 32;

+  // If set, Envoy will always set :ref:`x-request-id <config_http_conn_man_headers_x-request-id>` header in response.
+  // If this is false or not set, the header is returned in responses only in case of forced tracing.
+  // This defaults to false.


No need to specify a default for proto3 scalars, this is implied.

htuch · 2020-04-17T01:22:01Z

...voy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto

@@ -433,6 +433,11 @@ message HttpConnectionManager {
  // is the current Envoy behaviour. This defaults to false.
  bool preserve_external_request_id = 32;

+  // If set, Envoy will always set :ref:`x-request-id <config_http_conn_man_headers_x-request-id>` header in response.
+  // If this is false or not set, the header is returned in responses only in case of forced tracing.


Can you make "forced tracing" a bit clearer for the reader?

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-04-17T18:00:47Z

I'm not sure what happened with circlici, it tells about compilation errors in a code which does not exist for lines which are not there in the last version of pr.
I'll just rebase to hopefully make it go away.

…request-id-in-response Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

…request-id-in-response Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

yanavlasov · 2020-04-20T17:03:16Z

#10860 should fix your clang-tidy error

…request-id-in-response Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-04-20T22:53:06Z

@yanavlasov clang-tidy is still broken:

source/common/http/conn_manager_config.h:14:10: error: 'common/stats/symbol_table_impl.h' file not found [clang-diagnostic-error]
#include "common/stats/symbol_table_impl.h"

mattklein123 · 2020-04-20T22:55:43Z

Merge current master. You don't have the fix.

/wait

…request-id-in-response Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-04-20T22:59:12Z

Oh, merged. Misread the comment.

…request-id-in-response Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-04-22T01:25:23Z

@htuch @mattklein123 All tests did pass, PTAL

mattklein123

Thanks!

…xy#10808) Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com> Signed-off-by: pengg <pengg@google.com>

Signed-off-by: Spencer Lewis <slewis@squareup.com> * master: (46 commits) allow specifying the API version of bootstrap from the command line (envoyproxy#10803) config: adding connect matcher (unused) (envoyproxy#10894) Add missing dependency on `assert.h` (envoyproxy#10918) Lower heap and disk space used by kafka tests (envoyproxy#10915) [tools] handle commits merged without PR in deprecated script (envoyproxy#10723) tools: including working tree in modified_since_last_github_commit.sh diff. (envoyproxy#10911) rocketmq_proxy: implement rocketmq proxy [docs] PR template to include commit message (envoyproxy#10900) docs: breaking long word to stop content overflow. (envoyproxy#10880) Delete legacy connection pool code. (envoyproxy#10881) wasm: clarify how configuration is passed (envoyproxy#10782) issue template: clarify security/crash reporting (envoyproxy#10885) api/faq: add entry on incremental xDS. (envoyproxy#10876) router: retry overloaded requests (envoyproxy#10847) Remove inclusion of pthread.h, not needed for linux compilation (envoyproxy#10895) request_id: Add option to always set request id in response (envoyproxy#10808) xray: Use correct types for segment document output (envoyproxy#10834) router: fixing a watermark bug for streaming retries (envoyproxy#10866) http: auditing Path() calls for safety with Pathless CONNECT (envoyproxy#10851) Remove hardcoded type urls Part.2 (envoyproxy#10848) ...

request_id: Add option to always set request id in response

a9b9606

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar requested review from alyssawilk and mattklein123 as code owners April 16, 2020 07:51

repokitteh-read-only bot added the api label Apr 16, 2020

compilation fix

e62f04a

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

yanavlasov self-assigned this Apr 16, 2020

htuch reviewed Apr 17, 2020

View reviewed changes

api cr

2525a45

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

yanavlasov previously approved these changes Apr 17, 2020

View reviewed changes

Ruslan Nigmatullin added 2 commits April 17, 2020 11:29

Merge branch 'master' of github.com:envoyproxy/envoy into always-set-…

19feed8

…request-id-in-response Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

fix

1c7854a

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar dismissed yanavlasov’s stale review via 1c7854a April 17, 2020 18:50

Merge branch 'master' of github.com:envoyproxy/envoy into always-set-…

71002c0

…request-id-in-response Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Merge branch 'master' of github.com:envoyproxy/envoy into always-set-…

0856775

…request-id-in-response Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

repokitteh-read-only bot added the waiting label Apr 20, 2020

Merge branch 'master' of github.com:envoyproxy/envoy into always-set-…

3e25c9e

…request-id-in-response Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

repokitteh-read-only bot removed the waiting label Apr 20, 2020

Ruslan Nigmatullin added 3 commits April 20, 2020 19:33

Merge branch 'master' of github.com:envoyproxy/envoy into always-set-…

3da9637

…request-id-in-response Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Merge branch 'master' of github.com:envoyproxy/envoy into always-set-…

d36d7cc

…request-id-in-response Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

reorder

444db4c

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

mattklein123 approved these changes Apr 22, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Apr 22, 2020

mattklein123 merged commit 5cd6ef7 into envoyproxy:master Apr 22, 2020

penguingao pushed a commit to penguingao/envoy that referenced this pull request Apr 22, 2020

request_id: Add option to always set request id in response (envoypro…

d2ddf51

…xy#10808) Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com> Signed-off-by: pengg <pengg@google.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

request_id: Add option to always set request id in response #10808

request_id: Add option to always set request id in response #10808

euroelessar commented Apr 16, 2020

repokitteh-read-only bot commented Apr 16, 2020

euroelessar commented Apr 16, 2020

euroelessar commented Apr 16, 2020 •

edited

Loading

htuch left a comment

htuch Apr 17, 2020

htuch Apr 17, 2020

euroelessar commented Apr 17, 2020

yanavlasov commented Apr 20, 2020

euroelessar commented Apr 20, 2020

mattklein123 commented Apr 20, 2020

euroelessar commented Apr 20, 2020

euroelessar commented Apr 22, 2020

mattklein123 left a comment

request_id: Add option to always set request id in response #10808

request_id: Add option to always set request id in response #10808

Conversation

euroelessar commented Apr 16, 2020

repokitteh-read-only bot commented Apr 16, 2020

euroelessar commented Apr 16, 2020

euroelessar commented Apr 16, 2020 • edited Loading

htuch left a comment

Choose a reason for hiding this comment

htuch Apr 17, 2020

Choose a reason for hiding this comment

htuch Apr 17, 2020

Choose a reason for hiding this comment

euroelessar commented Apr 17, 2020

yanavlasov commented Apr 20, 2020

euroelessar commented Apr 20, 2020

mattklein123 commented Apr 20, 2020

euroelessar commented Apr 20, 2020

euroelessar commented Apr 22, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

euroelessar commented Apr 16, 2020 •

edited

Loading