stream info: prefer reference rather than shared pointer for upstreamClusterInfo() by wbpcode · Pull Request #44042 · envoyproxy/envoy

wbpcode · 2026-03-20T09:04:15Z

Commit Message: stream info: return reference rather than copy of shared pointer
Additional Description:

Rather then return a copy of shared pointer, we prefer to use reference except we do want to extend the lifetime of the object.

NOTE: the previous upstreamClusterInfo will return an absl::optionalUpstream::ClusterInfoConstSharedPtr and now, the new upstreamClusterInfoSharedPtr will only return Upstream::ClusterInfoConstSharedPtr directly. Because we can find in the whole code base, no one want to distinguish it is nullptr or never been set.

See #44025 for the code style change.

Risk Level: low. Even for the forks, it should be super easy to address this change.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

wbpcode · 2026-03-20T12:45:46Z

/retest

… dev-make-streaminfo-upstream-cluster-info-better

ravenblackx · 2026-03-20T15:03:01Z

+  if (const auto cluster_info = parent_.callbacks()->streamInfo().upstreamClusterInfo()) {
+    stream_info_.setUpstreamClusterInfo(
+        parent_.callbacks()->streamInfo().upstreamClusterInfoSharedPtr());


There is a behavior change in this section - in the old version it was possible for this block to setUpstreamClusterInfo(nullptr) if there are no clusters, vs. after this change setUpstreamClusterInfo will not be called in that case.

I don't know if this is important, since ~everything else that consumes the value appears to check "not nullopt and not nullptr" before doing anything. One way it could matter is if it's possible for the value to transition from not-null to null here.

Yeah. The difference is senseless because at any position where the data is actually be used, we only care an actual valid cluster info.

One way it could matter is if it's possible for the value to transition from not-null to null here.

It's impossible. The upstream request's stream_info is empty here. And we will populate all necessary info here.

I think we even needn't the if check here now. Note, before this PR, the only reason to check whether returned absl::optionalUpstream::ClusterInfoConstSharedPtr has value is to ensure we can access the return value safely and the setUpstreamClusterInfo accept a const Upstream::ClusterInfoConstSharedPtr& as input prameter.

I too thought the check could be removed, then I thought without the check you'd be copying a SharedPtr unnecessarily which could be expensive, but then I realize you'd only be copying a nullptr SharedPtr unnecessarily, which presumably is not expensive since it has no refcount. :)

So I'm happy with removing the check or not as you prefer.

ravenblackx · 2026-03-20T15:23:14Z

+  if (const auto cluster_info = stream_info->upstreamClusterInfo()) {
+    logging_info_->setClusterInfo(stream_info->upstreamClusterInfoSharedPtr());


This behavior differs on what was previously set-but-nullptr too.

This not actually change the behavior because the ExtAuthzLoggingInfo will keep an Upstream::ClusterInfoConstSharedPtr field directly.

Then,
before:

set-but-nullptr: the cluster info is null in the ExtAuthzLoggingInfo.

not-set: the cluster info is null in the ExtAuthzLoggingInfo.

set-and-not-nullptr: the cluster info is valid in the ExtAuthzLoggingInfo.

now:

set-but-nullptr: the cluster info is null in the ExtAuthzLoggingInfo.

not-set: the cluster info is null in the ExtAuthzLoggingInfo.

set-and-not-nullptr: the cluster info is valid in the ExtAuthzLoggingInfo.

TBH, we can also skip the if (const auto cluster_info = stream_info->upstreamClusterInfo()) check because no effect to final result.

ravenblackx · 2026-03-20T15:27:20Z

+  // No cluster info
  {
-    EXPECT_CALL(Const(stream_info), upstreamClusterInfo()).WillOnce(Return(absl::nullopt));
+    EXPECT_CALL(Const(stream_info), upstreamClusterInfo())
+        .WillOnce(Return(OptRef<const Upstream::ClusterInfo>{}));
    EXPECT_TRUE(TestUtility::jsonStringEqual(formatter.format(formatter_context, stream_info),
                                             expected_json_map));
  }
-  // Empty cluster info (nullptr)
+  // Empty cluster info (nullptr shared_ptr)
  {
-    EXPECT_CALL(Const(stream_info), upstreamClusterInfo()).WillOnce(Return(nullptr));
+    EXPECT_CALL(Const(stream_info), upstreamClusterInfo())
+        .WillOnce(Return(OptRef<const Upstream::ClusterInfo>{}));


If we're sticking with the no distinction between unset and set-to-nullptr model then I think this is now testing the same thing twice.

Good catch.

ravenblackx · 2026-03-20T15:29:30Z

                     ->access());

-    EXPECT_EQ(absl::nullopt, stream_info.upstreamClusterInfo());
+    EXPECT_FALSE(stream_info.upstreamClusterInfo());


Little bit more readable (and better failure message) as

Suggested change

EXPECT_FALSE(stream_info.upstreamClusterInfo());

EXPECT_THAT(stream_info.upstreamClusterInfo(), testing::IsNull());

Maybe EXPECT_FALSE(!stream_info.upstreamClusterInfo().has_value());?

My bad, as it was is fine - I was thinking it should have been IsNull before, when it was a pointer, but now that it's an optref just the plain EXPECT_FALSE works for me.

ravenblackx · 2026-03-20T15:30:51Z

    stream_info.setUpstreamClusterInfo(cluster_info);
-    EXPECT_NE(absl::nullopt, stream_info.upstreamClusterInfo());
-    EXPECT_EQ("fake_cluster", stream_info.upstreamClusterInfo().value()->name());
+    EXPECT_TRUE(stream_info.upstreamClusterInfo());


Similarly, but also changing to an ASSERT because otherwise the next line will crash if this line is failing:

Suggested change

EXPECT_TRUE(stream_info.upstreamClusterInfo());

ASSERT_THAT(stream_info.upstreamClusterInfo(), testing::NotNull());

ravenblackx · 2026-03-20T15:35:37Z

+  // No cluster info
  {
-    EXPECT_CALL(Const(stream_info), upstreamClusterInfo()).WillOnce(Return(absl::nullopt));
+    EXPECT_CALL(Const(stream_info), upstreamClusterInfo())
+        .WillOnce(Return(OptRef<const Upstream::ClusterInfo>{}));
    verifyOpenTelemetryOutput(
        formatter.format({&request_header, &response_header, &response_trailer}, stream_info),
        expected);
  }
-  // Empty cluster info (nullptr)
+  // Empty cluster info (nullptr shared_ptr)
  {
-    EXPECT_CALL(Const(stream_info), upstreamClusterInfo()).WillOnce(Return(nullptr));
+    EXPECT_CALL(Const(stream_info), upstreamClusterInfo())
+        .WillOnce(Return(OptRef<const Upstream::ClusterInfo>{}));


In this one, too, I think the same thing is being tested twice if we're sticking with discarding the distinction.

ravenblackx · 2026-03-20T15:42:31Z

-   * @return Upstream Connection's ClusterInfo.
-   * This returns an optional to differentiate between unset(absl::nullopt),
-   * no route or cluster does not exist(nullptr), and set to a valid cluster(not nullptr).


This comment deletion is related to most of my comments on this PR.
The other changes suggest the distinction probably doesn't matter, but there are a couple of cases where the behavior is changed, and I'm not certain whether the old behavior of

if (explicitly set to nullptr) {set another thing to nullptr}

meaningfully differs from the new behavior

if (explicitly set to nullptr) {don't set the other thing}

wbpcode · 2026-03-20T16:07:48Z

  // Only set cluster info in logging info once.
  if (logging_info_->clusterInfo() == nullptr) {
-    logging_info_->setClusterInfo(stream_info->upstreamClusterInfo());
+    logging_info_->setClusterInfo(stream_info->upstreamClusterInfoSharedPtr());


this is same case with the ext authz. Only two different state in logging info: nullptr or valid cluster info.

Oh yeah, these ones are definitely safe because it's explicitly checked whether the destination is already nullptr, and the only case where the change would make a difference is if the destination is not nullptr.

Co-authored-by: Raven Black <ravenblack@gmail.com> Signed-off-by: code <wbphub@gmail.com>

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

…nch 'dev-make-streaminfo-upstream-cluster-info-better' of ssh://ssh.github.com:443/wbpcode/envoy into dev-make-streaminfo-upstream-cluster-info-better

wbpcode · 2026-03-22T01:28:53Z

/retest

agrawroh

LGTM, Thank You!

…ClusterInfo() (envoyproxy#44042) Commit Message: stream info: return reference rather than copy of shared pointer Additional Description: Rather then return a copy of shared pointer, we prefer to use reference except we do want to extend the lifetime of the object. **NOTE: the previous upstreamClusterInfo will return an absl::optional<Upstream::ClusterInfoConstSharedPtr> and now, the new upstreamClusterInfoSharedPtr will only return Upstream::ClusterInfoConstSharedPtr directly. Because we can find in the whole code base, no one want to distinguish it is nullptr or never been set.** See envoyproxy#44025 for the code style change. Risk Level: low. Even for the forks, it should be super easy to address this change. Testing: n/a. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com> Signed-off-by: code <wbphub@gmail.com> Co-authored-by: Raven Black <ravenblack@gmail.com>

…ClusterInfo() (envoyproxy#44042) Commit Message: stream info: return reference rather than copy of shared pointer Additional Description: Rather then return a copy of shared pointer, we prefer to use reference except we do want to extend the lifetime of the object. **NOTE: the previous upstreamClusterInfo will return an absl::optional<Upstream::ClusterInfoConstSharedPtr> and now, the new upstreamClusterInfoSharedPtr will only return Upstream::ClusterInfoConstSharedPtr directly. Because we can find in the whole code base, no one want to distinguish it is nullptr or never been set.** See envoyproxy#44025 for the code style change. Risk Level: low. Even for the forks, it should be super easy to address this change. Testing: n/a. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com> Signed-off-by: code <wbphub@gmail.com> Co-authored-by: Raven Black <ravenblack@gmail.com> Signed-off-by: Xuyang Tao <taoxuy@google.com>

…ClusterInfo() (envoyproxy#44042) Commit Message: stream info: return reference rather than copy of shared pointer Additional Description: Rather then return a copy of shared pointer, we prefer to use reference except we do want to extend the lifetime of the object. **NOTE: the previous upstreamClusterInfo will return an absl::optional<Upstream::ClusterInfoConstSharedPtr> and now, the new upstreamClusterInfoSharedPtr will only return Upstream::ClusterInfoConstSharedPtr directly. Because we can find in the whole code base, no one want to distinguish it is nullptr or never been set.** See envoyproxy#44025 for the code style change. Risk Level: low. Even for the forks, it should be super easy to address this change. Testing: n/a. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com> Signed-off-by: code <wbphub@gmail.com> Co-authored-by: Raven Black <ravenblack@gmail.com> Signed-off-by: Jonathan Wu <jtwu@google.com>

…ClusterInfo() (envoyproxy#44042) Commit Message: stream info: return reference rather than copy of shared pointer Additional Description: Rather then return a copy of shared pointer, we prefer to use reference except we do want to extend the lifetime of the object. **NOTE: the previous upstreamClusterInfo will return an absl::optional<Upstream::ClusterInfoConstSharedPtr> and now, the new upstreamClusterInfoSharedPtr will only return Upstream::ClusterInfoConstSharedPtr directly. Because we can find in the whole code base, no one want to distinguish it is nullptr or never been set.** See envoyproxy#44025 for the code style change. Risk Level: low. Even for the forks, it should be super easy to address this change. Testing: n/a. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com> Signed-off-by: code <wbphub@gmail.com> Co-authored-by: Raven Black <ravenblack@gmail.com> Signed-off-by: Nick Shipilov <nick.shipilov.n@gmail.com>

…ClusterInfo() (envoyproxy#44042) Commit Message: stream info: return reference rather than copy of shared pointer Additional Description: Rather then return a copy of shared pointer, we prefer to use reference except we do want to extend the lifetime of the object. **NOTE: the previous upstreamClusterInfo will return an absl::optional<Upstream::ClusterInfoConstSharedPtr> and now, the new upstreamClusterInfoSharedPtr will only return Upstream::ClusterInfoConstSharedPtr directly. Because we can find in the whole code base, no one want to distinguish it is nullptr or never been set.** See envoyproxy#44025 for the code style change. Risk Level: low. Even for the forks, it should be super easy to address this change. Testing: n/a. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com> Signed-off-by: code <wbphub@gmail.com> Co-authored-by: Raven Black <ravenblack@gmail.com>

stream info: return reference rather than copy of shared pointer

16c9310

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

wbpcode requested review from RyanTheOptimist, abeyad, agrawroh, ggreenway, krinkinmu, kyessenov, mathetake, mattklein123, nezdolik, ravenblackx, tyxia, yanavlasov and yanjunxiang-google as code owners March 20, 2026 09:04

wbpcode assigned ravenblackx, yanavlasov, ggreenway, nezdolik and agrawroh Mar 20, 2026

wbpcode added 3 commits March 20, 2026 09:15

fix mock

1c6432c

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

fix test

56a0f66

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

fix stream info size test

d9c89ea

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

wbpcode changed the title ~~stream info: return reference rather than copy of shared pointer~~ stream info: prefer reference rather than shared pointer for upstreamClusterInfo() Mar 20, 2026

Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy into…

9b424e9

… dev-make-streaminfo-upstream-cluster-info-better

ravenblackx requested changes Mar 20, 2026

View reviewed changes

wbpcode commented Mar 20, 2026

View reviewed changes

wbpcode and others added 3 commits March 21, 2026 00:12

Apply suggestions from code review

cd00bb8

Co-authored-by: Raven Black <ravenblack@gmail.com> Signed-off-by: code <wbphub@gmail.com>

address comments

10003ae

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

fix test because unexpected mock

377ad4e

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>

Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy; bra…

11006bb

…nch 'dev-make-streaminfo-upstream-cluster-info-better' of ssh://ssh.github.com:443/wbpcode/envoy into dev-make-streaminfo-upstream-cluster-info-better

ravenblackx approved these changes Mar 23, 2026

View reviewed changes

agrawroh approved these changes Mar 24, 2026

View reviewed changes

wbpcode merged commit 26d9f5d into envoyproxy:main Mar 25, 2026
29 checks passed

wbpcode deleted the dev-make-streaminfo-upstream-cluster-info-better branch March 25, 2026 11:43

This was referenced Mar 26, 2026

Automator: update envoy@ in istio/proxy@master istio/proxy#6924

Closed

update Envoy version and fix building istio/proxy#6925

Merged

		if (const auto cluster_info = stream_info->upstreamClusterInfo()) {
		logging_info_->setClusterInfo(stream_info->upstreamClusterInfoSharedPtr());

	EXPECT_FALSE(stream_info.upstreamClusterInfo());
	EXPECT_THAT(stream_info.upstreamClusterInfo(), testing::IsNull());

	EXPECT_TRUE(stream_info.upstreamClusterInfo());
	ASSERT_THAT(stream_info.upstreamClusterInfo(), testing::NotNull());

Conversation

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

Uh oh!

Uh oh!

wbpcode commented Mar 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Mar 22, 2026

Uh oh!

agrawroh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wbpcode commented Mar 20, 2026 •

edited

Loading