xds: update trace log to only print non-nullptr exceptions by adisuissa · Pull Request #44026 · envoyproxy/envoy

adisuissa · 2026-03-19T14:44:38Z

Commit Message: xds: update trace log to only print non-nullptr exceptions
Additional Description:
Upon an xDS-error, trace-logs output the exception details. However there are a few cases where an exception is created with nullptr passed.
This PR introduces defensive coding in the cases where the exception details are printed out to the trace logs.

Risk Level: low - only trace-logs are impacted
Testing: Added unit test to avoid regression in this case.
Docs Changes: N/A
Release Notes: N/A - not user facing (trace logs)
Platform Specific Features: N/A

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only · 2026-03-19T14:44:45Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #44026 was opened by adisuissa.

see: more, trace.

adisuissa · 2026-03-19T14:44:53Z

cc @paul-r-gall

botengyao

thanks for the fix!

/wait

botengyao · 2026-03-21T04:40:33Z

      ASSERT(reason != Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure);
      ENVOY_LOG(trace, "ODCDS-manager: error while fetching a single resource {}: {}",
-                resource_name_, e->what());
+                resource_name_, e == nullptr ? "exception not provided" : e->what());


orz, is this causing crashes?

After re-inspecting the code the assertion above ASSERT(reason != Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure) seems to be incorrect, and was removed.

botengyao · 2026-03-21T04:51:41Z

    disableInitFetchTimeoutTimer();
    stats_.update_rejected_.inc();
-    ENVOY_LOG(warn, "gRPC config for {} rejected: {}", type_url_, e->what());
+    ENVOY_LOG(warn, "gRPC config for {} rejected: {}", type_url_,


The above

// We expect Envoy exception to be thrown when update is rejected. ASSERT(e != nullptr);

is asserting not nullptr, and if there are cases that e can be nullptr it needs to be removed, here and other places.

I then wonder if we should continue to use the raw pointer other than OptRef.

RE OptRef vs. pointers, I'm not sure what the difference is in terms of dereferencing a nullptr vs. dereferencing a non-existent optional value.

I'll look into whether e can be null in this case or not.

After further validation, the passed exception can only be nullptr if it is invoked with a reason that is ConnectionFailure (example).
The error-handling code typically distinguishes between the different reasons, for example:

envoy/source/extensions/config_subscription/grpc/grpc_subscription_impl.cc

Line 120 in a19b275

void GrpcSubscriptionImpl::onConfigUpdateFailed(ConfigUpdateFailureReason reason,

.
Not a very safe pattern, but this is what there is at the moment. I've reverted the changes in all the files other than the od-cds subscriptions.

Seems that the exception can be nullptr even with other reasons (e.g., FetchTimedout):

envoy/source/extensions/config_subscription/grpc/grpc_subscription_impl.cc

Line 35 in dbf2b1c

onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, nullptr);

I still think that the pointer access pattern should be defensive, but I can see the point of having a clear understanding of the code paths.
Updated the comment accordingly.

Clicked the wrong button, it is clearly not an approval ;-)

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…_fix Signed-off-by: Adi Suissa-Peleg <adip@google.com>

wbpcode

LGTM for me. Thanks.

wbpcode · 2026-03-25T07:51:39Z

Defer this to @botengyao

wbpcode · 2026-03-25T07:52:56Z

Could you check the CI also?

wbpcode · 2026-03-25T07:53:24Z

/wait

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…_fix Signed-off-by: Adi Suissa-Peleg <adip@google.com>

botengyao

lgtm, thanks!

…y#44026) Commit Message: xds: update trace log to only print non-nullptr exceptions Additional Description: Upon an xDS-error, trace-logs output the exception details. However there are a few cases where an exception is created with nullptr passed. This PR introduces defensive coding in the cases where the exception details are printed out to the trace logs. Risk Level: low - only trace-logs are impacted Testing: Added unit test to avoid regression in this case. Docs Changes: N/A Release Notes: N/A - not user facing (trace logs) Platform Specific Features: N/A --------- Signed-off-by: Adi Suissa-Peleg <adip@google.com> Signed-off-by: Xuyang Tao <taoxuy@google.com>

…y#44026) Commit Message: xds: update trace log to only print non-nullptr exceptions Additional Description: Upon an xDS-error, trace-logs output the exception details. However there are a few cases where an exception is created with nullptr passed. This PR introduces defensive coding in the cases where the exception details are printed out to the trace logs. Risk Level: low - only trace-logs are impacted Testing: Added unit test to avoid regression in this case. Docs Changes: N/A Release Notes: N/A - not user facing (trace logs) Platform Specific Features: N/A --------- Signed-off-by: Adi Suissa-Peleg <adip@google.com> Signed-off-by: Jonathan Wu <jtwu@google.com>

…y#44026) Commit Message: xds: update trace log to only print non-nullptr exceptions Additional Description: Upon an xDS-error, trace-logs output the exception details. However there are a few cases where an exception is created with nullptr passed. This PR introduces defensive coding in the cases where the exception details are printed out to the trace logs. Risk Level: low - only trace-logs are impacted Testing: Added unit test to avoid regression in this case. Docs Changes: N/A Release Notes: N/A - not user facing (trace logs) Platform Specific Features: N/A --------- Signed-off-by: Adi Suissa-Peleg <adip@google.com> Signed-off-by: Nick Shipilov <nick.shipilov.n@gmail.com>

…y#44026) Commit Message: xds: update trace log to only print non-nullptr exceptions Additional Description: Upon an xDS-error, trace-logs output the exception details. However there are a few cases where an exception is created with nullptr passed. This PR introduces defensive coding in the cases where the exception details are printed out to the trace logs. Risk Level: low - only trace-logs are impacted Testing: Added unit test to avoid regression in this case. Docs Changes: N/A Release Notes: N/A - not user facing (trace logs) Platform Specific Features: N/A --------- Signed-off-by: Adi Suissa-Peleg <adip@google.com>

xds: update trace log to only print non-nullptr exceptions

fbb2afb

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa marked this pull request as ready for review March 19, 2026 14:44

botengyao previously approved these changes Mar 21, 2026

View reviewed changes

repokitteh-read-only Bot added the waiting label Mar 21, 2026

address comments

9483688

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only Bot removed the waiting label Mar 24, 2026

adisuissa added 2 commits March 24, 2026 21:25

update comment

9e04d13

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into error_message_print…

922a6fa

…_fix Signed-off-by: Adi Suissa-Peleg <adip@google.com>

wbpcode previously approved these changes Mar 25, 2026

View reviewed changes

wbpcode assigned botengyao Mar 25, 2026

repokitteh-read-only Bot added the waiting label Mar 25, 2026

adisuissa added 2 commits March 25, 2026 14:08

update trace message

50c4a7c

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into error_message_print…

d634e4d

…_fix Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa dismissed wbpcode’s stale review via d634e4d March 25, 2026 14:08

repokitteh-read-only Bot removed the waiting label Mar 25, 2026

botengyao approved these changes Mar 26, 2026

View reviewed changes

botengyao merged commit ede8074 into envoyproxy:main Mar 26, 2026
29 checks passed

Conversation

adisuissa commented Mar 19, 2026

Uh oh!

repokitteh-read-only Bot commented Mar 19, 2026

Uh oh!

adisuissa commented Mar 19, 2026

Uh oh!

botengyao left a comment

Choose a reason for hiding this comment

Uh oh!

botengyao Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

adisuissa Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

adisuissa Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

botengyao Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

adisuissa Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

adisuissa Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

wbpcode left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Mar 25, 2026

Uh oh!

wbpcode commented Mar 25, 2026

Uh oh!

wbpcode commented Mar 25, 2026

Uh oh!

botengyao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adisuissa Mar 24, 2026 •

edited

Loading

wbpcode left a comment •

edited

Loading