support connection spi #13372
support connection spi #13372
Conversation
|
|
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
添加GRPC长连接SPI扩展及无确认请求发送方法变更文件
时序图sequenceDiagram
participant Client as GrpcBiStreamRequestAcceptor
participant Delegate as ConnectionGeneratorServiceDelegate
participant Service as ConnectionGeneratorServiceImpl
Client->>Delegate: getConnection()
Delegate->>Service: 调用实现类
Service-->>Delegate: 返回GrpcConnection实例
Delegate-->>Client: 返回连接对象
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
There was a problem hiding this comment.
🔍 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 1 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 1 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 1 | 次要问题，酌情优化。例如：代码格式不规范或注释缺失。 |
总计： 3 个问题
📋 评审意见详情
💡 单文件建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
☕ core/src/main/java/com/alibaba/nacos/core/remote/grpc/ConnectionGeneratorService.java (1 💬)
☕ core/src/main/java/com/alibaba/nacos/core/remote/grpc/ConnectionGeneratorServiceDelegate.java (1 💬)
☕ core/src/main/java/com/alibaba/nacos/core/remote/Connection.java (1 💬)
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. 系统属性配置未做白名单校验可能导致SPI加载不可信实现
ConnectionGeneratorServiceDelegate通过系统属性读取'nacos.core.remote.connection.generator'的值来加载SPI实现类，但未对允许的配置值进行白名单校验。这可能导致攻击者通过设置恶意系统属性，加载未授权的SPI实现类，引发安全漏洞。
📌 关键代码：
private String connectionGeneratorType = System.getProperty("nacos.core.remote.connection.generator", "nacos");🔍 2. ConnectionGeneratorServiceDelegate单例模式限制动态配置变更
ConnectionGeneratorServiceDelegate采用单例模式，初始化时一次性加载所有SPI实现类。由于SPI实现类的加载和选择在启动时完成，无法在运行时动态切换实现类型（如通过配置更新），导致系统灵活性不足。
📌 关键代码：
private static final ConnectionGeneratorServiceDelegate INSTANCE = new ConnectionGeneratorServiceDelegate();
public static ConnectionGeneratorServiceDelegate getInstance() {
return INSTANCE;
}🔍 3. 缺乏对SPI实现类的合法性验证
ConnectionGeneratorServiceDelegate在加载SPI实现类时，仅通过getType()方法匹配类型名称，未对实现类本身的来源或权限进行验证。这可能导致恶意实现类被加载，例如通过类加载器注入不可信代码。
📌 关键代码：
for (ConnectionGeneratorService service : NacosServiceLoader.load(ConnectionGeneratorService.class)) {
if (service.getType().equals(connectionGeneratorType)) {
this.connectionGeneratorService = service;
...
}
}🔍 4. 新增的sendRequestNoAck方法未在文档说明异常场景
Connection接口新增的sendRequestNoAck方法未在Javadoc中说明抛出NacosException的具体异常场景（如网络错误、请求格式错误等），可能导致使用者忽略异常处理，引发系统崩溃或数据不一致。
📌 关键代码：
public abstract void sendRequestNoAck(Request request) throws NacosException;💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
| * @param channel | ||
| * @return connection | ||
| */ | ||
| Connection getConnection(ConnectionMeta metaInfo, StreamObserver streamObserver, Channel channel); |
There was a problem hiding this comment.
接口方法参数的泛型类型未显式指定，可能导致类型不安全
🟡 Major | 🧹 Code Smells
📋 问题详情
在getConnection方法中，StreamObserver参数未指定具体泛型类型。未指定类型参数的泛型可能导致运行时类型转换错误，降低代码类型安全性。建议显式指定泛型类型以提升代码健壮性。
💡 解决方案
修改StreamObserver参数为具体泛型类型：
- public Connection getConnection(ConnectionMeta metaInfo, StreamObserver streamObserver, Channel channel);
+ public Connection getConnection(ConnectionMeta metaInfo, StreamObserver<?> streamObserver, Channel channel);或根据实际使用场景指定具体类型（如StreamObserver<Response>）。
🔧 建议代码
‼️ AI 生成代码 - 请在应用前检查逻辑、规范并测试
| Connection getConnection(ConnectionMeta metaInfo, StreamObserver streamObserver, Channel channel); | |
| public Connection getConnection(ConnectionMeta metaInfo, StreamObserver<?> streamObserver, Channel channel); |
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| */ | ||
|
|
||
| public class ConnectionGeneratorServiceDelegate { | ||
| private String connectionGeneratorType = System.getProperty("nacos.core.remote.connection.generator", "nacos"); |
There was a problem hiding this comment.
系统属性配置未做白名单校验，可能导致SPI加载不可信实现
🟠 Critical | 🔓 Vulnerabilities
📋 问题详情
通过系统属性nacos.core.remote.connection.generator指定的连接生成器类型未做白名单校验，攻击者可能通过修改该属性值加载恶意实现类，从而引发代码注入风险。建议对允许的类型进行白名单限制。
💡 解决方案
添加白名单校验逻辑：
+ private static final Set<String> ALLOWED_TYPES = Set.of("nacos", "custom_type");
- private String connectionGeneratorType = System.getProperty(...);
+ private String connectionGeneratorType = System.getProperty(...);
+ if (!ALLOWED_TYPES.contains(connectionGeneratorType)) {
+ throw new IllegalArgumentException("Unsupported connection generator type: " + connectionGeneratorType);
+ }您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| * | ||
| * @param request request. | ||
| * @return response. | ||
| * @throws NacosException nacos exception. |
There was a problem hiding this comment.
新增的sendRequestNoAck方法未在文档说明异常场景
🟢 Minor | 🧹 Code Smells
📋 问题详情
新增的sendRequestNoAck方法抛出NacosException但未在Javadoc中说明异常触发条件。开发者可能忽略异常处理，导致未捕获异常引发程序崩溃。建议补充异常说明以提升代码可维护性。
💡 解决方案
优化异常说明描述：
- * @throws NacosException nacos exception.
+ * @throws NacosException 当请求序列化失败或连接异常时抛出🔧 建议代码
‼️ AI 生成代码 - 请在应用前检查逻辑、规范并测试
| * @throws NacosException nacos exception. | |
| * @throws NacosException 当请求序列化失败或连接异常时抛出 |
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #13372 +/- ##
=============================================
- Coverage 70.34% 70.31% -0.04%
Complexity 11714 11714
=============================================
Files 1592 1594 +2
Lines 50984 51004 +20
Branches 5149 5152 +3
=============================================
- Hits 35865 35862 -3
- Misses 12688 12716 +28
+ Partials 2431 2426 -5
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
|
|
我印象中有一个spi的拓展， 2.0最初的版本是grpc和rsocket并行的， 当初抽象Connection时就是有grpc和rsocket实现的。 @shiyiyue1102 帮忙看下 |
|
cla没过，用了集团内账号提交的 |
支持GRPC长连接SPI扩展，方便开发者扩展长连接功能