[WIP] fix: when pull image with https fail then change to http#37
[WIP] fix: when pull image with https fail then change to http#37Lvnszn wants to merge 1 commit intohigress-group:istio-1.19from
Conversation
|
|
添加HTTPS失败时的HTTP回退机制及增强日志记录变更文件
时序图sequenceDiagram
participant IF as ImageFetcher
IF->>IF: 执行HTTPS请求
IF-->>IF: 返回错误
IF->>IF: 记录HTTPS错误详情
IF->>IF: 决定回退HTTP并记录
IF->>IF: 重新解析URL为HTTP格式
IF->>IF: 执行HTTP请求
IF-->>IF: 返回结果
IF->>IF: 记录最终结果状态
💡 小贴士与 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 | 1 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 0 | 次要问题，酌情优化。例如：代码格式不规范或注释缺失。 |
总计： 1 个问题
📋 评审意见详情
💡 单文件建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
🔹 pkg/wasm/imagefetcher.go (1 💬)
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. HTTP协议降级引入安全风险，可能违反项目安全架构
当HTTPS请求失败时直接切换到HTTP协议，可能导致明文传输镜像数据。此行为可能违反项目对安全传输的架构要求，特别是在处理包含敏感信息的镜像时，HTTP协议易受中间人攻击。建议评估是否允许协议降级，或添加TLS验证配置选项而非完全禁用加密。
📌 关键代码：
ref, err = name.ParseReference(url, name.Insecure)
...
desc, err = remote.Get(ref, o.fetchOpts...)🔍 2. 错误处理逻辑可能导致错误掩盖
当前逻辑将所有HTTPS错误都导向HTTP重试，但部分错误（如认证失败、镜像不存在）与协议无关。这可能导致真实错误原因被掩盖，增加故障排查难度。建议仅针对TLS握手失败等协议相关错误触发降级。
📌 关键代码：
if err != nil {
// 未区分错误类型即触发协议降级
...
}🔍 3. 缺乏协议降级的配置控制
HTTP降级行为未提供配置选项，无法满足部分场景下必须使用HTTPS的要求。建议添加可配置的降级开关，使管理员能根据安全策略选择是否启用此功能。
🔍 4. 测试覆盖不足可能导致场景遗漏
未发现针对HTTP降级逻辑的专项测试用例。需要补充模拟HTTPS失败后HTTP成功的测试场景，以及非协议错误时的异常处理测试。
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
|
|
||
| wasmLog.Infof("Successfully fetched image using HTTP fallback for URL: %s", url) | ||
| } else { | ||
| wasmLog.Infof("Successfully fetched image using HTTPS for URL: %s", url) |
There was a problem hiding this comment.
Redundant error check after fallback logic.
🟡 Major | 🐞 Bugs
📋 问题详情
The if err != nil check at line 130 is unreachable. The fallback logic already returns in error cases, making this check redundant. Removing it improves code clarity and reduces unnecessary checks.
💡 解决方案
Remove the redundant if err != nil check at line 130.
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
Lvnszn
force-pushed
the
fix_istio_wasm_pull_image
branch
from
d0ec0ab to
e26b3ea
Compare
|
关联 issue：alibaba/higress#2079 |
3 participants
Please provide a description of this PR:
客户端 HTTPS 证书不对或未携带证书：
如果证书不正确或未携带证书，HTTPS 请求会失败。
代码中会检查错误信息是否包含 "server gave HTTP response"，即服务端可能回退到 HTTP 响应的情况。
如果错误中没有类似的提示（例如证书错误），代码不会触发降级到 HTTP 的逻辑。
To help us figure out who should review this PR, please put an X in all the areas that this PR affects.
Please check any characteristics that apply to this pull request.