http: prefer reference rather than shared pointer for route()#44078
Conversation
|
Wait #43938 to be merged first. |
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
87d25b1 to
266079c
Compare
|
/retest |
Signed-off-by: code <wbphub@gmail.com>
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
|
/retest |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the route() method across the HTTP filter stack to return an OptRef<const Router::Route> instead of a SharedPtr, introducing routeSharedPtr() for cases where ownership management is required. The changes span the core connection manager, filter manager, and numerous extensions, with corresponding updates to the test suite. Feedback focuses on reducing code duplication in ConnectionManagerImpl and optimizing performance and readability in the CORS and Lua filters by caching the result of route() calls in local variables.
There was a problem hiding this comment.
Pull request overview
Refactors the HTTP filter callback route() API to return an OptRef<const Router::Route> (non-owning optional reference) instead of a shared_ptr, while adding routeSharedPtr() for callers that need ownership extension/transfer.
Changes:
- Updates
StreamFilterCallbacks::route()/DownstreamStreamFilterCallbacks::route(...)to returnOptRef<const Router::Route>. - Introduces
routeSharedPtr()variants and updates core implementations (HCM, filter manager, async client, tcp proxy). - Migrates call sites and tests to the new API patterns (
!route,route.has_value(),route().ptr(), orrouteSharedPtr()where ownership is needed).
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/mocks/http/mocks.h | Update mock route APIs + add routeSharedPtr |
| test/mocks/http/mocks.cc | Default mock behavior for route/routeSharedPtr |
| test/integration/filters/set_route_filter.cc | Use routeSharedPtr for delegating route |
| test/integration/filters/async_upstream_filter.cc | Adapt route assertions to OptRef |
| test/extensions/filters/http/on_demand/on_demand_filter_test.cc | Return empty OptRef in tests |
| test/extensions/filters/http/lua/lua_filter_test.cc | Return empty OptRef in tests |
| test/extensions/filters/http/jwt_authn/jwt_authn_fuzz_test.cc | Set mock route via callback state |
| test/extensions/filters/http/jwt_authn/filter_test.cc | Update test expectations to OptRef |
| test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc | Return empty OptRef in tests |
| test/extensions/filters/http/grpc_json_reverse_transcoder/filter_test.cc | Return empty OptRef in tests |
| test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc | Return empty OptRef in tests |
| test/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter_test.cc | Return empty OptRef in tests |
| test/extensions/filters/http/gcp_authn/gcp_authn_filter_test.cc | Return empty OptRef in tests |
| test/extensions/filters/http/file_system_buffer/filter_test.cc | Update route mocking to OptRef |
| test/extensions/filters/http/ext_proc/ordering_test.cc | Return route as OptRef |
| test/extensions/filters/http/ext_proc/filter_test_common.cc | Return route as OptRef |
| test/extensions/filters/http/ext_proc/filter_test.cc | Return route as OptRef |
| test/extensions/filters/http/ext_authz/ext_authz_test.cc | Return empty OptRef in tests |
| test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc | Remove/adjust route default mocks |
| test/extensions/filters/http/csrf/csrf_filter_test.cc | Return empty OptRef in tests |
| test/extensions/filters/http/cors/cors_filter_test.cc | Return empty OptRef in tests |
| test/extensions/filters/http/cache_v2/cache_filter_test.cc | Return empty OptRef in tests |
| test/extensions/filters/http/cache/cache_filter_test.cc | Return empty OptRef in tests |
| test/common/router/upstream_request_test.cc | Update route comparison for new API |
| test/common/router/router_test.cc | Expect routeSharedPtr() behavior in router test |
| test/common/http/filter_manager_test.cc | Update downstream callback route returns to OptRef |
| test/common/http/conn_manager_impl_test_3.cc | Compare routes via .ptr() / .has_value() |
| test/common/http/conn_manager_impl_test_2.cc | Compare routes via .ptr() / .has_value() |
| test/common/http/conn_manager_impl_test.cc | Update routing tests to new callbacks |
| test/common/http/async_client_impl_test.cc | Update route assertions to OptRef |
| source/extensions/filters/http/ratelimit/ratelimit.cc | Persist route via routeSharedPtr() |
| source/extensions/filters/http/on_demand/on_demand_update.cc | Switch nullptr checks to OptRef |
| source/extensions/filters/http/lua/lua_filter.cc | Switch nullptr checks to OptRef |
| source/extensions/filters/http/local_ratelimit/local_ratelimit.h | Update helper signature to OptRef |
| source/extensions/filters/http/local_ratelimit/local_ratelimit.cc | Adapt route usage to OptRef |
| source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc | Switch nullptr checks to OptRef |
| source/extensions/filters/http/gcp_authn/gcp_authn_filter.cc | Switch nullptr checks to OptRef |
| source/extensions/filters/http/fault/fault_filter.cc | Switch route local to OptRef |
| source/extensions/filters/http/ext_authz/ext_authz.h | Update helper signature to OptRef |
| source/extensions/filters/http/ext_authz/ext_authz.cc | Use OptRef in routing/metadata handling |
| source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc | Switch route local to OptRef |
| source/extensions/filters/http/cors/cors_filter.cc | Switch route checks to OptRef |
| source/extensions/filters/http/cache_v2/cache_filter.cc | Switch route local to OptRef |
| source/extensions/filters/http/cache/cache_filter.cc | Switch route local to OptRef |
| source/common/tcp_proxy/tcp_proxy.h | Implement route() + add routeSharedPtr() |
| source/common/router/upstream_request.cc | Store route via routeSharedPtr() |
| source/common/router/router.cc | Store route via routeSharedPtr() |
| source/common/http/filter_manager.h | Add routeSharedPtr() and OptRef route plumbing |
| source/common/http/filter_manager.cc | Implement new route getters |
| source/common/http/conn_manager_impl.h | Add downstream routeSharedPtr() API |
| source/common/http/conn_manager_impl.cc | Implement downstream route() as OptRef and shared variant |
| source/common/http/async_client_impl.h | Implement route() as OptRef + shared variant |
| envoy/http/filter.h | Update public filter callback interfaces |
| contrib/istio/filters/http/alpn/source/alpn_filter.cc | Switch route local to OptRef |
|
This refactoring have discussed at slack channel and no any logic change. Let's merge it because other 5 PRs have been merged. |
…roxy#44078) Commit Message: stream info: prefer reference rather than shared pointer for route() Additional Description: See new style envoyproxy#44025 for more detail Risk Level: n/a. 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> Signed-off-by: Xuyang Tao <taoxuy@google.com>
…roxy#44078) Commit Message: stream info: prefer reference rather than shared pointer for route() Additional Description: See new style envoyproxy#44025 for more detail Risk Level: n/a. 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> Signed-off-by: Jonathan Wu <jtwu@google.com>
…roxy#44078) Commit Message: stream info: prefer reference rather than shared pointer for route() Additional Description: See new style envoyproxy#44025 for more detail Risk Level: n/a. 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> Signed-off-by: Nick Shipilov <nick.shipilov.n@gmail.com>
…roxy#44078) Commit Message: stream info: prefer reference rather than shared pointer for route() Additional Description: See new style envoyproxy#44025 for more detail Risk Level: n/a. 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>
Commit Message: stream info: prefer reference rather than shared pointer for route()
Additional Description:
See new style #44025 for more detail
Risk Level: n/a.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.