cherry pick 3 pr from envoy-proxy#13
Closed
jue-yin wants to merge 2 commits intohigress-group:envoy-1.27from
Closed
cherry pick 3 pr from envoy-proxy#13jue-yin wants to merge 2 commits intohigress-group:envoy-1.27from
jue-yin wants to merge 2 commits intohigress-group:envoy-1.27from
Conversation
…eful session envelope, aiming to cover MCP 250326 spec SSE cherry-pick from envoyproxy#30573 envoyproxy@62f4a14#diff-c904f9b0d49cdd938ffaa952192372529415afa8602d7dc6acb904e61111d8a5 add strict mode to stateful session, return 503 when destination is not available cherry-pick from envoyproxy#32093 envoyproxy@6354dcf support 0 ttl in cookie stateful sessiion to disable cookie expiration Signed-off-by: 爵银 <jueyin.hsl@alibaba-inc.com>
|
|
jue-yin
changed the title
实现状态会话的信封模式并增强严格模式支持变更概述
变更文件
时序图sequenceDiagram
participant Client as HTTP客户端
participant Proxy as Envoy代理
participant Upstream as 上游服务器
Client->>Proxy: 发送初始请求
Proxy->>Upstream: 转发请求
Upstream->>Proxy: 返回带会话上下文的响应
Proxy->>Client: 返回编码后的会话上下文
Client->>Proxy: 发送后续请求（带会话上下文）
Proxy->>Upstream: 解码会话上下文并转发到指定主机
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 0 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 0 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 1 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 1 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
📄 test/common/upstream/host_utility_test.cc (1 💬)
- 应确保在主机工具测试中正确验证严格模式下的主机选择。 (L174-L183)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 新增的严格模式可能影响负载均衡行为一致性
在 StatefulSession 过滤器中引入了 strict 模式，当设置为 true 时，如果请求的目标主机不可用则返回 503。这种变更会影响负载均衡器的行为，可能导致在某些部署场景下服务的可用性下降。建议评估该模式对现有部署的影响，并确保文档清晰说明其行为差异。
📌 关键代码
// If set to True, the HTTP request must be routed to the requested destination.
// If the requested destination is not available, Envoy returns 503. Defaults to False.
bool strict = 2;
decoder_callbacks_->setUpstreamOverrideHost(
std::make_pair(upstream_address.value(), config->isStrict()));
可能导致服务在部分主机不可用时整体不可访问，降低系统容错能力
🔍2. 新引入的 Envelope 状态会话机制缺乏充分的安全性和健壮性验证
新增的 EnvelopeSessionState 通过编码上游主机地址和原始值到头部中，这种方式可能存在安全风险，例如头部注入或信息泄露。此外，解析逻辑较为复杂，容易出现边界条件处理不当的问题。建议增加针对恶意输入和异常情况的测试，并审查其在高负载下的性能表现。
📌 关键代码
-
api/envoy/extensions/http/stateful_session/envelope/v3/envelope.proto(30-52) -
source/extensions/http/stateful_session/envelope/envelope.cc(9-20)
void EnvelopeSessionStateFactory::SessionStateImpl::onUpdate(
absl::string_view host_address, Envoy::Http::ResponseHeaderMap& headers) {
const auto upstream_value_header = headers.get(factory_.name_);
if (upstream_value_header.size() != 1) {
ENVOY_LOG(trace, "Header {} not exist or occurs multiple times", factory_.name_);
return;
}
const std::string new_header =
absl::StrCat(Envoy::Base64::encode(host_address), ";", OriginUpstreamValuePartFlag,
Envoy::Base64::encode(upstream_value_header[0]->value().getStringView()));
headers.setReferenceKey(factory_.name_, new_header);
}
存在潜在的头部注入、信息泄露和拒绝服务攻击风险
🔍3. 多状态会话实现之间的代码重复和维护成本增加
新增的 EnvelopeSessionState 和已有的 CookieBasedSessionState、HeaderBasedSessionState 在功能上存在相似性，特别是在处理上游地址更新和解析方面。这导致了代码重复，增加了未来的维护成本。建议抽象出通用的状态处理逻辑，减少重复代码并提高可维护性。
📌 关键代码
-
source/extensions/http/stateful_session/cookie/cookie.cc(12-28) -
source/extensions/http/stateful_session/header/header.cc(9-13) -
source/extensions/http/stateful_session/envelope/envelope.cc(9-20)
增加未来修改和扩展的工作量，提高引入错误的可能性
🔍4. Base64 编码函数重载可能引发混淆和潜在错误
在 Base64 类中新增了接受 absl::string_view 参数的 encode 函数重载。虽然提供了便利，但可能会与现有的接受 const char* 和长度的版本造成混淆，尤其是在模板推导或隐式转换时。建议明确区分使用场景，并添加相应的测试用例以避免潜在错误。
📌 关键代码
std::string Base64::encode(absl::string_view input) { return encode(input.data(), input.length()); }
/**
* Base64 encode an input char buffer with a given length.
* @param input string to encode.
*/
static std::string encode(absl::string_view input);
可能导致意外的类型转换或调用错误的重载函数，引起运行时错误
🔍5. HostUtility::allowLBChooseHost 逻辑与 strict 模式存在潜在冲突
HostUtility::allowLBChooseHost 函数根据 overrideHostToSelect 的 strict 标志决定是否允许负载均衡器选择主机。然而，其逻辑是返回 !override_host.value().second，即 strict 为 true 时不允许 LB 选择。这种设计可能与其他地方的 strict 行为不一致，建议统一语义并在文档中明确说明。
📌 关键代码
bool HostUtility::allowLBChooseHost(LoadBalancerContext* context) {
if (context == nullptr) {
return true;
}
auto override_host = context->overrideHostToSelect();
if (!override_host.has_value()) {
return true;
}
// Return opposite value to "strict" setting.
return !override_host.value().second;
}
可能导致负载均衡行为不符合预期，影响服务路由的正确性
审查详情
📒 文件清单 （49 个文件）
✅ 新增: 10 个文件
📝 变更: 39 个文件
✅ 新增文件:
api/envoy/extensions/http/stateful_session/envelope/v3/BUILDapi/envoy/extensions/http/stateful_session/envelope/v3/envelope.protosource/extensions/http/stateful_session/envelope/BUILDsource/extensions/http/stateful_session/envelope/config.ccsource/extensions/http/stateful_session/envelope/config.hsource/extensions/http/stateful_session/envelope/envelope.ccsource/extensions/http/stateful_session/envelope/envelope.htest/extensions/http/stateful_session/envelope/BUILDtest/extensions/http/stateful_session/envelope/config_test.cctest/extensions/http/stateful_session/envelope/envelope_test.cc
📝 变更文件:
CODEOWNERSapi/BUILDapi/envoy/extensions/filters/http/stateful_session/v3/stateful_session.protoapi/envoy/type/http/v3/cookie.protoapi/versioning/BUILDenvoy/http/filter.henvoy/http/stateful_session.henvoy/upstream/load_balancer.hsource/common/common/base64.ccsource/common/common/base64.hsource/common/http/async_client_impl.hsource/common/http/filter_manager.ccsource/common/http/filter_manager.hsource/common/runtime/runtime_features.ccsource/common/upstream/cluster_manager_impl.ccsource/common/upstream/host_utility.ccsource/common/upstream/host_utility.hsource/extensions/common/wasm/context.ccsource/extensions/common/wasm/context.hsource/extensions/extensions_build_config.bzlsource/extensions/extensions_metadata.yamlsource/extensions/filters/http/stateful_session/stateful_session.ccsource/extensions/filters/http/stateful_session/stateful_session.hsource/extensions/http/stateful_session/cookie/cookie.ccsource/extensions/http/stateful_session/cookie/cookie.hsource/extensions/http/stateful_session/header/header.ccsource/extensions/http/stateful_session/header/header.htest/common/http/filter_manager_test.cctest/common/router/router_test.cctest/common/upstream/cluster_manager_impl_test.cctest/common/upstream/host_utility_test.cctest/extensions/filters/http/stateful_session/BUILDtest/extensions/filters/http/stateful_session/stateful_session_integration_test.cctest/extensions/filters/http/stateful_session/stateful_session_test.cctest/extensions/http/stateful_session/cookie/cookie_test.cctest/extensions/http/stateful_session/header/header_test.cctest/mocks/http/mocks.cctest/mocks/http/mocks.htest/mocks/http/stateful_session.h
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
Comment on lines
+174
to
+183
| // Test overriding host in strict and non-strict mode. | ||
| for (const bool strict_mode : {false, true}) { | ||
| { | ||
| // The host map does not contain the expected host. | ||
| LoadBalancerContext::OverrideHost override_host{"1.2.3.4", strict_mode}; | ||
| EXPECT_CALL(context, overrideHostToSelect()) | ||
| .WillOnce(Return(absl::make_optional(override_host))); | ||
| auto host_map = std::make_shared<HostMap>(); | ||
| EXPECT_EQ(nullptr, HostUtility::selectOverrideHost(host_map.get(), HealthyStatus, &context)); | ||
| } |
There was a problem hiding this comment.
应确保在主机工具测试中正确验证严格模式下的主机选择。
🟢 Minor | 🧹 Code Smells
📋 问题详情
在主机工具测试中，SelectOverrideHostTest测试用例用于验证在严格模式下主机选择的行为。当前测试用例可能未完全覆盖所有情况，需要确保在所有情况下都能正确验证严格模式下的主机选择。
💡 解决方案
确保在主机工具测试中正确验证严格模式下的主机选择。
- // Test overriding host in strict and non-strict mode.\n for (const bool strict_mode : {false, true}) {\n {\n // The host map does not contain the expected host.\n LoadBalancerContext::OverrideHost override_host{\"1.2.3.4\", strict_mode};\n EXPECT_CALL(context, overrideHostToSelect())\n .WillOnce(Return(absl::make_optional(override_host)));\n auto host_map = std::make_shared<HostMap>();\n EXPECT_EQ(nullptr, HostUtility::selectOverrideHost(host_map.get(), HealthyStatus, &context));\n }
+ // Test overriding host in strict and non-strict mode.\n for (const bool strict_mode : {false, true}) {\n {\n // The host map does not contain the expected host.\n LoadBalancerContext::OverrideHost override_host{\"1.2.3.4\", strict_mode};\n EXPECT_CALL(context, overrideHostToSelect())\n .WillOnce(Return(absl::make_optional(override_host)));\n auto host_map = std::make_shared<HostMap>();\n EXPECT_EQ(nullptr, HostUtility::selectOverrideHost(host_map.get(), HealthyStatus, &context));\n }您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cherry-pick from envoyproxy#39004 envoyproxy@eb0aa19 new feature stateful session envelope, aiming to cover MCP 250326 spec SSE
cherry-pick from envoyproxy#30573 envoyproxy@62f4a14#diff-c904f9b0d49cdd938ffaa952192372529415afa8602d7dc6acb904e61111d8a5 add strict mode to stateful session, return 503 when destination is not available
cherry-pick from envoyproxy#32093 envoyproxy@6354dcf support 0 ttl in cookie stateful sessiion to disable cookie expiration