Skip to content

route: replace ENVOY_BUG to ENVOY_LOG#28875

Merged
wbpcode merged 5 commits intoenvoyproxy:mainfrom
wbpcode:dev-fix-route-refresh
Aug 9, 2023
Merged

route: replace ENVOY_BUG to ENVOY_LOG#28875
wbpcode merged 5 commits intoenvoyproxy:mainfrom
wbpcode:dev-fix-route-refresh

Conversation

@wbpcode
Copy link
Copy Markdown
Member

@wbpcode wbpcode commented Aug 7, 2023

Commit Message: route: replace ENVOY_BUG to ENVOY_LOG
Additional Description:

To fix #28626.

To avoid holding the whole route table in a long time and accelarate the release of memory, we forbidden route refreshment or clear after the response headers are sent in the #26045. And an ENVOY_BUG was introduced to ensure that.

This work well in most case because in most case we never refresh route at response phase (and it make no sense). But there are some corner cases will result the ENVOY_BUG be triggerd unexpectedly.
There are two mainly corner cases:

  1. The local response is sent before the initial route is set. This may occur because the request lost some necessary headers.
  2. Call the sendLocalReply after the route cache is cleared in the request/decoder filter chain.
    In both cases above, if a response/encoder filter try to access route by route() method, the ENVOY_BUG may be triggered.

To resolve the problem, this PR do two things:

  1. Ensure the route is set before send local response. This means even the initial route is not set or the route is cleared by filters, we will set the route before the response/encoder filter chain processing starts. (We needn't do this for normal response because router filter will do this. And note the route still may be cleared by response/encode filters, but this case is more like filter bug and should be handled by the filter developers.)
  2. Replace ENVOY_BUG to ENVOY_LOG. Seems ENVOY_BUG is too strict for this scenario. I think warning log should be enough. And even there are some uncovered corner cases, warning log would more friendly than ENVOY_BUG.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Aug 7, 2023

not sure is this a good solution. but at least better than do nothing. 😞

Comment thread source/common/http/conn_manager_impl.h Outdated
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
wbpcode added 2 commits August 8, 2023 06:35
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
// The initial route maybe never be set or the cached route maybe cleared by the filters.
// This will force route refreshment if there is not a cached route to avoid potential
// route refreshment in the response filter chain.
cb->route(nullptr);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is a NOP if the route is already cached? Just making sure we aren't doing extra work on this path for the common case?

Copy link
Copy Markdown
Member Author

@wbpcode wbpcode Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if a route is already cached, then the route() will return directly and this calling will do nothing.

Router::RouteConstSharedPtr
ConnectionManagerImpl::ActiveStream::route(const Router::RouteCallback& cb) {
  if (cached_route_.has_value()) {
    return cached_route_.value();
  }
  refreshCachedRoute(cb);
  return cached_route_.value();
}

@wbpcode wbpcode merged commit 7fb3f62 into envoyproxy:main Aug 9, 2023
@wbpcode wbpcode deleted the dev-fix-route-refresh branch August 9, 2023 14:27
phlax pushed a commit to phlax/envoy that referenced this pull request Aug 9, 2023
* route: replace ENVOY_BUG to ENVOY_LOG

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* minor update

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* fix test

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* fix format

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* revert envoybug change

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

---------

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: Ryan Northey <ryan@synca.io>
jbohanon pushed a commit to solo-io/envoy-fork that referenced this pull request Aug 11, 2023
* route: replace ENVOY_BUG to ENVOY_LOG

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* minor update

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* fix test

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* fix format

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* revert envoybug change

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

---------

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot access route after response headers are sent?

2 participants