feat: Support adding a proxy server in between when forwarding requests to upstream #2710
Conversation
CH3CHO
requested review from
2456868764,
Erica177,
SpecialYang and
johnlanni
as code owners
支持在转发请求到上游时添加代理服务器变更概述
变更文件
💡 小贴士与 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 | 9 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 0 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 2 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 11 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
🔹 pkg/ingress/config/ingress_config.go (3 💬)
- 在处理 WasmPlugin 规则时，未正确处理嵌套结构导致可能的空指针引用。 (L1165-L1169)
- 在 adjustWasmPluginForProxy 方法中，未正确处理规则值的类型检查。 (L1180-L1188)
- 在 adjustRouteClusterForProxy 方法中，未检查服务包装器的代理配置是否有效。 (L1226-L1236)
🔹 pkg/ingress/kube/common/controller.go (2 💬)
- ServiceWrapper 的 DeepCopy 方法未正确复制所有字段。 (L143-L154)
- ProxyWrapper 的 DeepCopy 方法未正确复制所有字段。 (L171-L181)
🔹 registry/memory/cache.go (3 💬)
🔹 registry/proxy/factory.go (1 💬)
- 在构建代理集群补丁时，重复替换了 `{{name}}` 字段，可能导致意外的行为。 (L186-L188)
🔹 registry/reconcile/reconcile.go (1 💬)
🔹 registry/zookeeper/watcher.go (1 💬)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 代理配置的全局一致性与潜在冲突风险
在多个文件中引入了代理配置（ProxyConfig）和相关处理逻辑，但缺乏统一的验证和冲突检测机制。例如，在 pkg/ingress/config/ingress_config.go 中构造 EnvoyFilter 时，没有校验 ProxyName 的全局唯一性，可能导致同名代理配置冲突。此外，ServiceProxyConfig 在多个注册中心（如 Consul、DNS、Eureka）中被独立处理，但未确保配置的一致性，可能引发运行时错误或不可预期的行为。
📌 关键代码
func constructProxyEnvoyFilters(proxyWrappers map[string]*common.ProxyWrapper, serviceWrappers map[string]*common.ServiceWrapper, namespace string) []*config.Config {type ServiceProxyConfig struct {
ProxyName string
UpstreamProtocol common.Protocol
UpstreamSni string
}代理配置冲突可能导致服务路由错误、连接失败或安全漏洞。缺乏全局一致性校验会增加调试和维护难度。
🔍2. 跨文件的重复逻辑与代码抽象不足
在 pkg/ingress/config/ingress_config.go 和多个注册中心的 watcher 文件中（如 registry/direct/watcher.go, registry/eureka/watcher.go），都存在生成 ServiceProxyConfig 的相似逻辑。这些重复代码增加了维护负担，并可能导致功能不一致。应抽象出统一的代理配置生成逻辑，供各模块复用。
📌 关键代码
func (m *IngressConfig) adjustWasmPluginForProxy(out []config.Config) {func (w *watcher) generateProxyConfig(entry *v1alpha3.ServiceEntry) *ingress.ServiceProxyConfig {重复代码增加维护成本，容易引入不一致的逻辑，导致难以追踪的bug。
🔍3. 代理支持对系统性能和可扩展性的影响
新增的代理功能通过构造 EnvoyFilter 来实现，这会增加 Envoy 配置的复杂度和规模。特别是在大规模服务部署场景下，每个服务都可能生成额外的 EnvoyFilter 和 Cluster 配置，可能导致 Pilot 和 Envoy 的性能下降。此外，频繁更新 EnvoyFilter 可能引发 XDS 推送风暴，影响系统稳定性。
📌 关键代码
func constructProxyEnvoyFilters(proxyWrappers map[string]*common.ProxyWrapper, serviceWrappers map[string]*common.ServiceWrapper, namespace string) []*config.Config {在大规模部署中，性能下降、XDS 推送延迟或失败，影响服务可用性和用户体验。
🔍4. 安全风险：代理配置可能绕过安全策略
代理功能允许流量通过中间代理服务器转发，但当前实现未明确考虑安全策略的继承和应用。例如，TLS 配置、认证和授权策略可能无法正确传递到代理服务器，导致流量在代理环节被窃听或篡改。此外，ServiceProxyConfig 中的 UpstreamSni 字段如果配置不当，可能引发证书验证失败或中间人攻击。
📌 关键代码
if proxyConfig.UpstreamProtocol.IsHTTPS() {
tlsTypedConfig := map[string]interface{}{
"@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext",
}
if proxyConfig.UpstreamSni != "" {
tlsTypedConfig["sni"] = proxyConfig.UpstreamSni
}流量在代理环节可能被窃听、篡改或绕过安全策略，导致数据泄露或服务被非法访问。
🔍5. 测试覆盖不足与业务逻辑验证缺失
新增的代理功能涉及复杂的配置转换和 EnvoyFilter 构造逻辑，但未提供相应的单元测试或集成测试来验证其正确性。特别是在 adjustWasmPluginForProxy 和 adjustRouteClusterForProxy 等关键方法中，缺乏对代理配置生效的测试用例，难以保证业务逻辑的正确性和稳定性。
📌 关键代码
func (m *IngressConfig) adjustWasmPluginForProxy(out []config.Config) {func (m *IngressConfig) adjustRouteClusterForProxy(convertOptions *common.ConvertOptions, out []config.Config) {未经充分测试的功能可能包含隐藏的bug，导致生产环境中服务中断或安全漏洞。
🔍6. 依赖管理与版本兼容性问题
新增的代理功能依赖于特定版本的 Envoy API 和 Istio 配置模型。如果这些依赖库发生变更，可能导致代理功能失效或出现兼容性问题。此外，ProxyConfig 和 ServiceProxyConfig 的结构定义与底层 Envoy 配置紧密耦合，未来 Envoy 升级时可能需要大量修改。
📌 关键代码
message ProxyConfig {
string type = 1 [(google.api.field_behavior) = REQUIRED];
string name = 2 [(google.api.field_behavior) = REQUIRED];
string serverAddress = 3 [(google.api.field_behavior) = REQUIRED];
uint32 serverPort = 4 [(google.api.field_behavior) = REQUIRED];
uint32 listenerPort = 5;
uint32 connectTimeout = 6;
}
依赖库升级可能导致功能失效或需要大量重构工作，增加维护成本和系统风险。
审查详情
📒 文件清单 （21 个文件）
✅ 新增: 2 个文件
❌ 删除: 1 个文件
📝 变更: 18 个文件
✅ 新增文件:
pkg/common/proxy.goregistry/proxy/factory.go
❌ 删除文件:
registry/memory/model.go
📝 变更文件:
api/kubernetes/customresourcedefinitions.gen.yamlapi/networking/v1/mcp_bridge.pb.goapi/networking/v1/mcp_bridge.protoapi/networking/v1/mcp_bridge_deepcopy.gen.goapi/networking/v1/mcp_bridge_json.gen.goclient/Makefilepkg/common/protocol.gopkg/ingress/config/ingress_config.gopkg/ingress/kube/common/controller.gopkg/ingress/kube/common/model.goregistry/consul/watcher.goregistry/direct/watcher.goregistry/eureka/watcher.goregistry/memory/cache.goregistry/nacos/v2/watcher.goregistry/nacos/watcher.goregistry/reconcile/reconcile.goregistry/zookeeper/watcher.go
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2710 +/- ##
==========================================
+ Coverage 35.91% 45.31% +9.40%
==========================================
Files 69 82 +13
Lines 11576 13224 +1648
==========================================
+ Hits 4157 5992 +1835
+ Misses 7104 6886 -218
- Partials 315 346 +31
🚀 New features to boost your workflow:
|
CH3CHO
force-pushed
the
feat/proxy-server
branch
from
3a535cb to
0b27967
Compare
CH3CHO
force-pushed
the
feat/proxy-server
branch
from
92ae62d to
1a1509e
Compare
pkg/ingress/config/ingress_config.go
Outdated
| IngressLog.Infof("Found %d number of envoyFilters", len(envoyFilters)) | ||
| m.mutex.Lock() | ||
| m.cachedEnvoyFilters = envoyFilters | ||
| // TODO (by CH3CHO): Make sure pilot can get the latest envoy filters. |
There was a problem hiding this comment.
What is the reason for this TODO?
…ts to upstream (alibaba#2710) Co-authored-by: 澄潭 <[email protected]>
3 participants
Ⅰ. Describe what this PR did
Support adding a proxy server in between when forwarding requests to upstream.
McpBridgeresourcea. Add a new
proxiesfield tospecfor configuring a proxy server list.b. Add a new
proxyNamefield to the item ofregistriesfor linking a proxy server to a registry.EnvoyFilterinregistry.Reconcilerto create a cluster and a listener for each proxy server.EnvoyFilters inIngressConfigto update the cluster workload for each service bound with a proxy server, making them pointing to the local listener of corresponding proxy server with the SNI of the service.Ⅱ. Does this pull request fix one issue?
fixes #2664
Ⅲ. Why don't you add test cases (unit test/integration test)?
TBC
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews