logger: use server stats scope at grpc logger by lambdai · Pull Request #18067 · envoyproxy/envoy

lambdai · 2021-09-10T05:56:15Z

Commit Message:
Fix the grpc access logger counter raised illegal memory access
by using server scope in shared loggers

Wider stats violation and detection can be found in #18047

Signed-off-by: Yuchen Dai silentdai@gmail.com

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
fix #18066
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

zuercher · 2021-09-10T19:06:44Z

/assign-from @envoyproxy/first-pass-reviewers

repokitteh-read-only · 2021-09-10T19:06:47Z

@envoyproxy/first-pass-reviewers assignee is @rojkov

🐱

Caused by: a #18067 (comment) was created by @zuercher.

see: more, trace.

mathetake · 2021-09-11T05:22:46Z

JFYI #18081 this is somewhat related to this:)

rojkov

Thanks! Looks good. I've added one nit.

/cc @pradeepcrao might add comments.

source/extensions/access_loggers/grpc/config_utils.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-09-13T22:44:00Z

Thank you @rojkov

lambdai · 2021-09-13T22:47:58Z

@mattklein123 @ggreenway Could you take a quick look so that we can decide the next step?

My point is this PR fix the crash described in #18066 and another crash pattern combining the efforts in #18081

The wanted stats can be added in the follow up PRs

rojkov

LGTM given the merge conflict is resolved.

yanavlasov

/wait

source/extensions/access_loggers/grpc/config_utils.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-09-15T16:08:22Z

/retest

repokitteh-read-only · 2021-09-15T16:08:26Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18067 (comment) was created by @lambdai.

see: more, trace.

lambdai · 2021-09-16T23:32:21Z

gentle ping

rojkov

Thanks! This feels less hacky indeed.

jmarantz · 2021-09-17T12:45:51Z

another main-merge is needed.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-09-17T16:31:53Z

another main-merge is needed.

Thanks! Fixing and running local CI.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-09-23T20:29:05Z

Is it worth adding an integration test that would crash/fail without this fix?
Change looks good overall.
/wait-any

Thanks! The scope arg is removed from the factory method. The code that crashes in the past is not compilable now.

What I was thinking was an integration test that uses a grpc logger, then reloads the config, and does more logging, to trigger the case that would have crashed before this fix.

ack. Working on it

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

test/integration/xds_integration_test.cc

ggreenway

A few things to improve in the new integration test, then I think this is ready to merge.

ggreenway · 2021-09-27T21:56:59Z

test/integration/xds_integration_test.cc

  test_server_->waitForCounterGe("listener_manager.listener_create_failure", 1);
 }

+// Verify the grpc cached logger is available after the initial logger filter is destroyed.


Please elaborate on this and describe the stat scope lifetime issue this is testing for

test/integration/xds_integration_test.cc

ggreenway · 2021-09-27T22:00:05Z

/wait

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-09-28T19:50:21Z

/retest

repokitteh-read-only · 2021-09-28T19:50:25Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18067 (comment) was created by @lambdai.

see: more, trace.

ggreenway

LGTM

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-09-29T18:21:32Z

/retest

repokitteh-read-only · 2021-09-29T18:21:36Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18067 (comment) was created by @lambdai.

see: more, trace.

lambdai · 2021-09-29T20:53:41Z

/retest

repokitteh-read-only · 2021-09-29T20:53:45Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18067 (comment) was created by @lambdai.

see: more, trace.

lambdai · 2021-09-30T05:21:05Z

/retest

repokitteh-read-only · 2021-09-30T05:21:09Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18067 (comment) was created by @lambdai.

see: more, trace.

rojkov

LGTM

/assign @yanavlasov for final approval

logger: use server stats scope at grpc logger

81d9a8a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai requested review from yanavlasov and zuercher as code owners September 10, 2021 05:56

lambdai mentioned this pull request Sep 10, 2021

stats scope for grpc access log should be envoy wide #18066

Closed

repokitteh-read-only bot assigned rojkov Sep 10, 2021

rojkov reviewed Sep 13, 2021

View reviewed changes

source/extensions/access_loggers/grpc/config_utils.cc Outdated Show resolved Hide resolved

comment

667f702

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

rojkov previously approved these changes Sep 14, 2021

View reviewed changes

jmarantz assigned ggreenway Sep 14, 2021

yanavlasov requested changes Sep 14, 2021

View reviewed changes

source/extensions/access_loggers/grpc/config_utils.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Sep 14, 2021

lambdai added 2 commits September 15, 2021 05:48

Merge branch 'main' into grpclogger

b14c665

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

add serverScope()

915babb

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai dismissed rojkov’s stale review via 915babb September 15, 2021 07:05

repokitteh-read-only bot removed the waiting label Sep 15, 2021

lambdai requested a review from yanavlasov September 16, 2021 18:10

rojkov previously approved these changes Sep 17, 2021

View reviewed changes

Merge branch 'main' into grpclogger

0d0065c

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

fix build

949689f

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai dismissed rojkov’s stale review via 949689f September 17, 2021 17:59

repokitteh-read-only bot added the waiting:any label Sep 23, 2021

repokitteh-read-only bot removed the waiting:any label Sep 23, 2021

add integration test

caa4d4e

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai requested a review from ggreenway September 23, 2021 22:16

rojkov reviewed Sep 27, 2021

View reviewed changes

test/integration/xds_integration_test.cc Outdated Show resolved Hide resolved

ggreenway requested changes Sep 27, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 27, 2021

mathetake mentioned this pull request Sep 27, 2021

envoy crash when setup envoy access log service istio/istio#34901

Closed

amend test case

a5e81e5

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Sep 28, 2021

Merge branch 'main' into grpclogger

05afe21

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

ggreenway approved these changes Sep 28, 2021

View reviewed changes

Merge branch 'main' into grpclogger

bfdb4c1

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

rojkov approved these changes Sep 30, 2021

View reviewed changes

rojkov assigned yanavlasov Sep 30, 2021

yanavlasov approved these changes Oct 1, 2021

View reviewed changes

yanavlasov merged commit 34edbca into envoyproxy:main Oct 1, 2021

lambdai deleted the grpclogger branch October 1, 2021 16:55

lambdai mentioned this pull request Oct 11, 2021

listener: 2nd attempt create per network filter chain stats scope #18564

Merged

Conversation

lambdai commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuercher commented Sep 10, 2021

Uh oh!

repokitteh-read-only bot commented Sep 10, 2021

Uh oh!

mathetake commented Sep 11, 2021

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lambdai commented Sep 13, 2021

Uh oh!

lambdai commented Sep 13, 2021

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lambdai commented Sep 15, 2021

Uh oh!

repokitteh-read-only bot commented Sep 15, 2021

Uh oh!

lambdai commented Sep 16, 2021

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Sep 17, 2021

Uh oh!

lambdai commented Sep 17, 2021

Uh oh!

lambdai commented Sep 23, 2021

Uh oh!

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ggreenway commented Sep 27, 2021

Uh oh!

lambdai commented Sep 28, 2021

Uh oh!

repokitteh-read-only bot commented Sep 28, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

lambdai commented Sep 29, 2021

Uh oh!

repokitteh-read-only bot commented Sep 29, 2021

Uh oh!

lambdai commented Sep 29, 2021

Uh oh!

repokitteh-read-only bot commented Sep 29, 2021

Uh oh!

lambdai commented Sep 30, 2021

Uh oh!

repokitteh-read-only bot commented Sep 30, 2021

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

lambdai commented Sep 10, 2021 •

edited

Loading